We should also provide support for breaking whenever a `setTimeout`, `setInterval`, or `requestAnimationFrame` is fired.
Created attachment 347612 [details] Patch
Comment on attachment 347612 [details] Patch Attachment 347612 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/8927318 New failing tests: inspector/dom-debugger/event-breakpoint-with-navigation.html
Created attachment 347620 [details] Archive of layout-test-results from ews104 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Created attachment 347622 [details] Patch
<rdar://problem/43572345>
Created attachment 347702 [details] Patch Remove old protocol methods since they don't really do what we want
Comment on attachment 347702 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=347702&action=review Looks good! r-, only because this lacks compatibility checks for older backends. We shouldn't present these new breakpoint types in that case. > Source/WebCore/inspector/agents/InspectorDOMDebuggerAgent.cpp:401 > + String eventName = oneShot ? "setTimeout" : "setInterval"; I think you want to use the new user-defined literal for ASCIILiteral here: String eventName = oneShot ? "setTimeout"_s : "setInterval"_s; > Source/WebCore/inspector/agents/InspectorDOMDebuggerAgent.cpp:413 > + String eventName = "requestAnimationFrame"; Ditto. > Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:960 > + this._pauseReasonTreeOutline = this.createContentTreeOutline(true); I'd use a named const above this line, like we do elsewhere in this function. > Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:1135 > + this._pauseReasonTreeOutline = this.createContentTreeOutline(true); Ditto. > Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:1316 > + if (popover instanceof WI.XHRBreakpointPopover && popover.result === WI.InputPopover.Result.Committed) { It looks like we aren't using InputPopover anywhere right now. Maybe the check below for `url` is enough, and we can remove the check for Result.Committed. > Source/WebInspectorUI/UserInterface/Views/EventBreakpointPopover.js:80 > + this._domEventNameInputElement.placeholder = "click"; When adding a symbolic breakpoint in Xcode, the placeholder for the Symbol and Library fields are prefixed with "Example: ". Maybe we should do: this._domEventNameInputElement.placeholder = "Example: \"click\""; > LayoutTests/inspector/dom-debugger/event-animation-frame-breakpoints-expected.txt:7 > +Firing "requestAnimationFrame" on window... I find all the ellipses at the end of each log message distracting. How about just a period? > LayoutTests/inspector/dom-debugger/event-animation-frame-breakpoints.html:14 > + requestAnimationFrame(handleWindow_requestAnimationFrame, 100); requestAnimationFrame only takes a callback. The interval is determined by the engine. > LayoutTests/inspector/dom-debugger/event-animation-frame-breakpoints.html:55 > + description: "Check that debugger does the not pause for disabled breakpoints.", "Check that debugger does not pause for disabled breakpoints." > LayoutTests/inspector/dom-debugger/event-listener-breakpoints.html:69 > + description: "Check that debugger does the not pause for disabled breakpoints.", "Check that debugger does not pause for disabled breakpoints."
Comment on attachment 347702 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=347702&action=review (In reply to Matt Baker from comment #7) > Looks good! r-, only because this lacks compatibility checks for older > backends. We shouldn't present these new breakpoint types in that case. This was the reason that I removed `setInstrumentationBreakpoint` and `removeInstrumentationBreakpoint`. That way, we can check for the existence of the new functions I created. See the usage of the following: - `WI.DOMDebuggerManager.supportsEventAnimationFrameBreakpoints` - `WI.DOMDebuggerManager.supportsEventListenerBreakpoints` - `WI.DOMDebuggerManager.supportsEventTimerBreakpoints` >> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:1316 >> + if (popover instanceof WI.XHRBreakpointPopover && popover.result === WI.InputPopover.Result.Committed) { > > It looks like we aren't using InputPopover anywhere right now. Maybe the check below for `url` is enough, and we can remove the check for Result.Committed. This is used by `WI.XHRBreakpointPopover`, so I'd like to not mess with it too much. Based on what I see, however, I think we could drop this pretty safely (in another patch). >> LayoutTests/inspector/dom-debugger/event-animation-frame-breakpoints-expected.txt:7 >> +Firing "requestAnimationFrame" on window... > > I find all the ellipses at the end of each log message distracting. How about just a period? I find them useful to quickly indicate that something will be triggered/fired. I've seen that "..." is used in our tests right before we do something to the page that will normally trigger an event. I think it's useful to have, but I don't have a strong preference. >> LayoutTests/inspector/dom-debugger/event-animation-frame-breakpoints.html:14 >> + requestAnimationFrame(handleWindow_requestAnimationFrame, 100); > > requestAnimationFrame only takes a callback. The interval is determined by the engine. Oops. Copypasta :P
Created attachment 347716 [details] Patch
Comment on attachment 347716 [details] Patch Attachment 347716 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/8938725 New failing tests: http/wpt/workers/name-property-enhanced.html
Created attachment 347755 [details] Archive of layout-test-results from ews113 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews113 Port: mac-sierra Platform: Mac OS X 10.12.6
Created attachment 347768 [details] [Image] After Patch is applied
Comment on attachment 347716 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=347716&action=review > Source/JavaScriptCore/inspector/protocol/DOMDebugger.json:34 > + { "name": "eventName", "type": "string", "description": "Animation frame event name to stop on." } Does this take an eventName in case we choose to add breakpoints for canceling and scheduling later on? > Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:456 > + } That's a lot of checks! This will only get worse if we ever add support for more instrumentation points (cancelAnimationFrame, etc). Why was setInstrumentationBreakpoint with an enum for the type (animationFrameFired, timerFired, etc) discarded? It seems like a lot of code would be simplified. > Source/WebInspectorUI/UserInterface/Views/EventBreakpointPopover.js:72 > + createOption(WI.unlocalizedString("requestAnimationFrame"), "requestAnimationFrame"); Shouldn't the option value be WI.EventBreakpoint.Type.AnimationFrame? Then you could eliminate the data massaging that happens in Popover.dismiss later on. Maybe I'm missing something. > Source/WebInspectorUI/UserInterface/Views/EventBreakpointPopover.js:80 > + this._domEventNameInputElement.placeholder = WI.UIString("Example: \"%s\"").format("click"); It looks like escaped quotes trips up the localizable string extractor. This works: WI.UIString("Example: “%s”") > Source/WebInspectorUI/UserInterface/Views/EventBreakpointPopover.js:102 > + if (type === WI.EventBreakpoint.Type.Listener && WI.DOMDebuggerManager.supportsEventListenerBreakpoints()) The checks for support seem redundant. You already checked for backend support when creating the options.
Comment on attachment 347716 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=347716&action=review >> Source/JavaScriptCore/inspector/protocol/DOMDebugger.json:34 >> + { "name": "eventName", "type": "string", "description": "Animation frame event name to stop on." } > > Does this take an eventName in case we choose to add breakpoints for canceling and scheduling later on? Yeah, as well as any other potentially future "event"s that may be added to the rAF system. >> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:456 >> + } > > That's a lot of checks! This will only get worse if we ever add support for more instrumentation points (cancelAnimationFrame, etc). Why was setInstrumentationBreakpoint with an enum for the type (animationFrameFired, timerFired, etc) discarded? It seems like a lot of code would be simplified. The main reason I wanted to split this out into different functions was to provide more dedicated paths for each "type" of event. Additionally, the previous implementation of `setInstrumentationBreakpoint` wouldn't have worked for what we wanted to do, specifically since it didn't distinguish between `setTimeout` and `setInterval`. I like your idea of using an enum! >> Source/WebInspectorUI/UserInterface/Views/EventBreakpointPopover.js:72 >> + createOption(WI.unlocalizedString("requestAnimationFrame"), "requestAnimationFrame"); > > Shouldn't the option value be WI.EventBreakpoint.Type.AnimationFrame? Then you could eliminate the data massaging that happens in Popover.dismiss later on. Maybe I'm missing something. This is a bit future-forward in that if we decide to add other AnimationFrame breakpoint support, we'd have two <option> with the same value of `WI.EventBreakpoint.Type.AnimationFrame`. It is for this reason that we don't use `WI.EventBreakpoint.Type.Timer` below for the `setTimeout` and `setInterval` <option>.
Created attachment 347824 [details] Patch Changed to use a single set/remove breakpoint command with enum for all event breakpoints
Comment on attachment 347824 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=347824&action=review Overall this is very solid. I have feedback on various parts. Please post a new patch. > Source/JavaScriptCore/inspector/protocol/DOMDebugger.json:15 > + "enum": ["animation-frame", "listener", "timer"], Good idea. > Source/JavaScriptCore/inspector/protocol/DOMDebugger.json:37 > + "name": "setEventBreakpoint", Is this going to work with older backends? Were both of these just added in trunk or have they been shipped yet? > Source/WebCore/inspector/InspectorInstrumentation.cpp:-84 > -static const char* const timerFiredEventName = "timerFired"; LOL, sad. > Source/WebCore/inspector/InspectorInstrumentation.cpp:1058 > + if (InspectorDOMDebuggerAgent* domDebuggerAgent = instrumentingAgents.inspectorDOMDebuggerAgent()) I think it's fine to put the agent existence checks at each callsite. However, the current approach drops the difference between sync and async pausing. Is this expected? Does the new implementation work with things that used to be sync (rAF events)? > Source/WebCore/inspector/agents/InspectorDOMDebuggerAgent.cpp:142 > + if (breakpointType != animationFrameCategoryType && breakpointType != listenerCategoryType && breakpointType != timerCategoryType) { Please use the generated enum values and parsing helper. Here's an example: std::optional<Protocol::Timeline::Instrument> instrumentType = Protocol::InspectorHelpers::parseEnumValueFromString<Protocol::Timeline::Instrument>(enumValueString); > Source/WebCore/inspector/agents/InspectorDOMDebuggerAgent.cpp:152 > + m_eventBreakpoints.add(formatEventBreakpointString(breakpointType, eventName)); Please store std::pair<BreakpointType, eventName> instead of requiring string allocations to do a hash lookup. > Source/WebCore/inspector/agents/InspectorDOMDebuggerAgent.cpp:162 > + if (breakpointType != animationFrameCategoryType && breakpointType != listenerCategoryType && breakpointType != timerCategoryType) { Ditto. > Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:434 > + if (breakpoint.disabled) { Please put legacy paths together at top level here; it's okay to check breakpoint.disabled again. It's hard to follow either code path (legacy or modern) right now. > Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:956 > + if (pauseData) { Make this an early return if !pauseData? > Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:1133 > + if (pauseData) { Ditto > Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:1319 > + if (popover instanceof WI.XHRBreakpointPopover && popover.result === WI.InputPopover.Result.Committed) { Why? > Source/WebInspectorUI/UserInterface/Views/EventBreakpointTreeElement.js:32 > + let classNames = ["breakpoint", "event", breakpoint.type]; Please adjust the last one to be "breakpoint-for-${breakpoint.type}". This makes the type-dependent CSS class searchable to this call site. > LayoutTests/inspector/dom-debugger/event-animation-frame-breakpoints.html:15 > +} These two function names are confusing. How about trigger_requestAnimationFrame and calledBy_requestAnimationFrame ? > LayoutTests/inspector/dom-debugger/resources/event-breakpoint-utilities.js:2 > + window.teardown = function(resolve, reject) { It's not safe to put these into the global namespace inside the Inspector as other utility files could pick the same names. The pattern used in other files is to namespace these under InspectorTest.EventBreakpoint.<the thing> Which is more wordy but guaranteed to not have this problem. > LayoutTests/inspector/dom-debugger/resources/event-breakpoint-utilities.js:51 > + InspectorTest.assert(!breakpoint.disabled, "Breakpoint should not be disabled initially."); I think this should reference event.data.breakpoint.
Comment on attachment 347824 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=347824&action=review >> Source/JavaScriptCore/inspector/protocol/DOMDebugger.json:37 >> + "name": "setEventBreakpoint", > > Is this going to work with older backends? Were both of these just added in trunk or have they been shipped yet? The event listener breakpoints work with older backends, but the instrumentation breakpoints do not. >> Source/WebCore/inspector/InspectorInstrumentation.cpp:-84 >> -static const char* const timerFiredEventName = "timerFired"; > > LOL, sad. Yeah. :/ This is why I'm dropping the `*InstrumentationBreakpoint` functions, since they don't use the name most people would expect. >> Source/WebCore/inspector/InspectorInstrumentation.cpp:1058 >> + if (InspectorDOMDebuggerAgent* domDebuggerAgent = instrumentingAgents.inspectorDOMDebuggerAgent()) > > I think it's fine to put the agent existence checks at each callsite. However, the current approach drops the difference between sync and async pausing. Is this expected? Does the new implementation work with things that used to be sync (rAF events)? The only sync events were the actual function calls (e.g. setTimeout, clearTimeout, setInterval, clearInterval, requestAnimationFrame, cancelAnimationFrame). This patch focuses on the callback that will be invoked, which is always async. >> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:956 >> + if (pauseData) { > > Make this an early return if !pauseData? I was following the other cases, but I guess there's no reason to do that 😛 One thing that is nice about this is that it allows us to use let/const, since we are inside an if-block, not a case. >> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:1319 >> + if (popover instanceof WI.XHRBreakpointPopover && popover.result === WI.InputPopover.Result.Committed) { > > Why? I don't want to break the current functionality of `WI.XHRBreakpointPopver`. I removed the if statement above that uses this. `WI.XHRBreakpointPopver` uses `WI.InputPopoverResult` for some reason, and rather than make this patch more complex, I just want to keep supporting it as is. I can address this in a followup.
(In reply to Devin Rousso from comment #17) > Comment on attachment 347824 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=347824&action=review > > >> Source/JavaScriptCore/inspector/protocol/DOMDebugger.json:37 > >> + "name": "setEventBreakpoint", > > > > Is this going to work with older backends? Were both of these just added in trunk or have they been shipped yet? > > The event listener breakpoints work with older backends, but the > instrumentation breakpoints do not. AFAIK, instrumentation breakpoints were never hooked up in the frontend. There shouldn't be any compatibility issues.
Created attachment 347907 [details] Patch Added `DefaultHash` and `HashTraits` to any enums used in Inspector protocol, since we need a way to hash them when used inside `HashMap`/`HashSet`
This patch modifies the inspector protocol generator. Please ensure that you have rebaselined any generator test results (i.e., by running `Tools/Scripts/run-inspector-generator-tests --reset-results`)
Attachment 347907 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:438: [CppProtocolTypesHeaderGenerator._generate_hash_declarations] Instance of 'CppProtocolTypesHeaderGenerator' has no 'type_declarations_for_domain' member [pylint/E1101] [5] Total errors found: 1 in 41 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 347907 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=347907&action=review > Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:68 > + sections.extend(self._generate_declarations_for_enum_conversion_methods(domains)) Seems fine. > Source/JavaScriptCore/inspector/scripts/tests/generic/expected/commands-with-async-attribute.json-result:735 > +template<> Per discussion on IRC, this can be simplified a lot: - The HashTraits can be defined at the declaration of the map/set to use PairHashTraits<HashTraits<String>, StrongEnumHashTraits<EnumType>>. Thus we don't need to define custom hash traits. - The generated DefaultHash<EnumType> can be implemented as: struct DefaultHash<EnumType> { typedef IntHash<EnumType> Hash; }
Comment on attachment 347907 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=347907&action=review > LayoutTests/inspector/dom-debugger/event-timer-breakpoints.html:137 > + .catch((reason) => { This can simply be .catch(InspectorTest.reportUnhandledRejection.bind(InspectorTest)) Bonus points if you fix reportUnhandledRejection to not use `this` so it doesn't need binding. ;-)
Comment on attachment 347907 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=347907&action=review >> LayoutTests/inspector/dom-debugger/event-timer-breakpoints.html:137 >> + .catch((reason) => { > > This can simply be > > .catch(InspectorTest.reportUnhandledRejection.bind(InspectorTest)) > > Bonus points if you fix reportUnhandledRejection to not use `this` so it doesn't need binding. ;-) Frankly, I'd rather just use the local `reject`, since that's going to resolve the current chain instead of abruptly exiting. Another reason I wanted to manually call `InspectorTest.fail` was so that we didn't skip the rest of the promise chain (we sill `resolve`, so we would run the next test). In this case, however, if we fail to `resume`, we should exit anyways. I can look at the binding "issue" in a followup :)
Created attachment 347933 [details] Patch
Attachment 347933 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:433: [CppProtocolTypesHeaderGenerator._generate_hash_declarations] Instance of 'CppProtocolTypesHeaderGenerator' has no 'type_declarations_for_domain' member [pylint/E1101] [5] Total errors found: 1 in 40 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 347933 [details] Patch Attachment 347933 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/8959397 New failing tests: accessibility/smart-invert-reference.html
Created attachment 347940 [details] Archive of layout-test-results from ews104 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 347933 [details] Patch r=me, EWS failure looks unrelated.
Comment on attachment 347933 [details] Patch Clearing flags on attachment: 347933 Committed r235248: <https://trac.webkit.org/changeset/235248>
All reviewed patches have been landed. Closing bug.