Bug 200285 - Web Inspector: DOM: add a special breakpoint for "All Events"
Summary: Web Inspector: DOM: add a special breakpoint for "All Events"
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-07-30 15:56 PDT by Devin Rousso
Modified: 2019-08-03 12:01 PDT (History)
11 users (show)

See Also:


Attachments
Patch (162.07 KB, patch)
2019-07-31 00:02 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
[Image] After Patch is applied (context menu) (94.06 KB, image/png)
2019-07-31 00:03 PDT, Devin Rousso
no flags Details
[Image] After Patch is applied (37.21 KB, image/png)
2019-07-31 00:03 PDT, Devin Rousso
no flags Details
Patch (162.07 KB, patch)
2019-07-31 00:11 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (162.09 KB, patch)
2019-08-03 10:54 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2019-07-30 15:56:42 PDT
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").
Comment 1 Devin Rousso 2019-07-31 00:02:24 PDT
Created attachment 375212 [details]
Patch
Comment 2 Devin Rousso 2019-07-31 00:03:31 PDT
Created attachment 375213 [details]
[Image] After Patch is applied (context menu)
Comment 3 Devin Rousso 2019-07-31 00:03:43 PDT
Created attachment 375214 [details]
[Image] After Patch is applied
Comment 4 EWS Watchlist 2019-07-31 00:04:17 PDT Comment hidden (obsolete)
Comment 5 Devin Rousso 2019-07-31 00:11:10 PDT
Created attachment 375215 [details]
Patch

Adjust [I] icon to be slightly thinner.
Comment 6 Joseph Pecoraro 2019-08-03 01:19:18 PDT
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 7 Devin Rousso 2019-08-03 10:53:46 PDT
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.
Comment 8 Devin Rousso 2019-08-03 10:54:34 PDT
Created attachment 375487 [details]
Patch
Comment 9 WebKit Commit Bot 2019-08-03 12:00:07 PDT
Comment on attachment 375487 [details]
Patch

Clearing flags on attachment: 375487

Committed r248201: <https://trac.webkit.org/changeset/248201>
Comment 10 WebKit Commit Bot 2019-08-03 12:00:09 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Radar WebKit Bug Importer 2019-08-03 12:01:22 PDT
<rdar://problem/53903843>