Rather than have to create an event breakpoint per event, there should be a way to enable/disable a special breakpoint for all events (just like "All Requests").
Created attachment 375212 [details] Patch
Created attachment 375213 [details] [Image] After Patch is applied (context menu)
Created attachment 375214 [details] [Image] After Patch is applied
This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.
Created attachment 375215 [details] Patch Adjust [I] icon to be slightly thinner.
Comment on attachment 375215 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=375215&action=review r=me. Nice tests! > Source/JavaScriptCore/ChangeLog:10 > + situations where the event name isn't known, or where one simply want's to pause on the next Typo: "want's" => "wants" > Source/WebCore/inspector/agents/InspectorDOMDebuggerAgent.cpp:501 > + m_debuggerAgent->schedulePauseOnNextStatement(oneShot ? Inspector::DebuggerFrontendDispatcher::Reason::Timeout : Inspector::DebuggerFrontendDispatcher::Reason::Interval, nullptr); Style: Pull `reason` out into a local to make the line easier to read. > Source/WebCore/inspector/agents/InspectorDOMDebuggerAgent.h:109 > + HashSet<String> m_listenerBreakpoints; Much better! > Source/WebCore/inspector/agents/InspectorDOMDebuggerAgent.h:118 > + Style: Remove this empty line to sidle it up against the other pauseOnAll bool! > Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:35 > + this._listenerBreakpoints = []; Style: Put `this._urlBreakpoints` next to this? I'd find readability better. > Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:107 > + switch (breakpoint.type) { This switch looks like migration code. It should have a comment as such. > Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:176 > + if (!this._allRequestsBreakpoint.disabled) Again with the order. It seems more logical to me to group these as: • Special Breakpoints • Listener Breakpoints list * URL Breakpoints list > Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:657 > + if (!DOMDebuggerManager.supportsAllListenersBreakpoint()) > + return; I suspect this shouldn't happen and could be an assert? Or maybe if you have this set on trunk and then open the inspector for an older device? > Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:552 > + else if (breakpoint === WI.domDebuggerManager.allListenersBreakpoint) > + options.title = WI.UIString("All Events"); This just admits how weird it is to call this `Listeners` when we show "Events" to the user. "All Events" might be a little generic. It could be "All Event Listeners" because it technically doesn't pause on events that don't have handlers. But "All Events" sounds good to me. > Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:1594 > + contextMenu.appendSeparator(); I'd remove this separate and keep the various timers together.
Comment on attachment 375215 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=375215&action=review >> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:657 >> + return; > > I suspect this shouldn't happen and could be an assert? Or maybe if you have this set on trunk and then open the inspector for an older device? The case you described is exactly the case.
Created attachment 375487 [details] Patch
Comment on attachment 375487 [details] Patch Clearing flags on attachment: 375487 Committed r248201: <https://trac.webkit.org/changeset/248201>
All reviewed patches have been landed. Closing bug.
<rdar://problem/53903843>