Bug 200285 – Web Inspector: DOM: add a special breakpoint for "All Events"

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Devin Rousso]

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").

[Devin Rousso]

Created attachment 375212 [details]
Patch

[Devin Rousso]

Created attachment 375213 [details]
[Image] After Patch is applied (context menu)

[Devin Rousso]

Created attachment 375214 [details]
[Image] After Patch is applied

[EWS Watchlist]

This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.

[Devin Rousso]

Created attachment 375215 [details]
Patch

Adjust [I] icon to be slightly thinner.

[Joseph Pecoraro]

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.

[Devin Rousso]

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.

[Devin Rousso]

Created attachment 375487 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 375487 [details]
Patch

Clearing flags on attachment: 375487

Committed r248201: <https://trac.webkit.org/changeset/248201>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/53903843>