[webkit-reviews] review granted: [Bug 200285] Web Inspector: DOM: add a special breakpoint for "All Events" : [Attachment 375215] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sat Aug 3 01:19:18 PDT 2019


Joseph Pecoraro <joepeck at webkit.org> has granted Devin Rousso
<drousso at apple.com>'s request for review:
Bug 200285: Web Inspector: DOM: add a special breakpoint for "All Events"
https://bugs.webkit.org/show_bug.cgi?id=200285

Attachment 375215: Patch

https://bugs.webkit.org/attachment.cgi?id=375215&action=review




--- Comment #6 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 375215
  --> https://bugs.webkit.org/attachment.cgi?id=375215
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.


More information about the webkit-reviews mailing list