[webkit-reviews] review denied: [Bug 183118] Web Inspector: support breakpoints for arbitrary event names : [Attachment 335762] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has denied Devin Rousso
<webkit at devinrousso.com>'s request for review:
Bug 183118: Web Inspector: support breakpoints for arbitrary event names
https://bugs.webkit.org/show_bug.cgi?id=183118

Attachment 335762: Patch

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




--- Comment #19 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 335762
  --> https://bugs.webkit.org/attachment.cgi?id=335762
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=335762&action=review

This looks great to me! But the patch no longer applies. Rebaseline it, and
address my few comments above, and lets get this in! I may have some additional
comments on the next round after I get to play with this.

> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:213
> +	   if (this._eventListenerBreakpoints.includes(breakpoint))
> +	       return;
> +
> +	   if (this._eventListenerBreakpoints.some((item) => item.eventName ===
breakpoint.eventName))
> +	       return;

Given we always do the bottom expression I guess we can drop the includes
check, because it is just redundant.

>
Source/WebInspectorUI/UserInterface/Controllers/EventListenerBreakpointTreeCont
roller.js:26
> +WI.EventListenerBreakpointTreeController = class
EventListenerBreakpointTreeController extends WI.Object

May not need to extend WI.Object if you don't dispatch events.

> Source/WebInspectorUI/UserInterface/Models/EventListenerBreakpoint.js:31
> +

Nit: I like throwing validation lines in here, as it'll show what arguments are
required and their expected types. For example here I would probably just do at
least:

    console.assert(typeof eventName === "string")

> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:1237
> +	   popover.show(event.target.element, [WI.RectEdge.MAX_Y,
WI.RectEdge.MIN_Y, WI.RectEdge.MAX_X]);

After reading this, and pointing my finger in various directions like a
complete fool, I wonder if it would be easier to have aliases of these:

    WI.Popover.Present.Above
    WI.Popover.Present.Below
    WI.Popover.Present.After (which would be Left or Right depending on RTL)
    WI.Popover.Present.Before (which would be Left or Right depending on RTL)

>
Source/WebInspectorUI/UserInterface/Views/EventListenerBreakpointPopover.js:57
> +	   label.textContent = WI.UIString("Break on listeners for event with
name:");

UI: In the screenshot the <div> and <input> look a little too close together.
We may want to run this past someone with UI chops. This label also feels too
long.

Take a look at the Xcode UI for adding Symbolic breakpoints. I think we could
do something simpler like that.

>
Source/WebInspectorUI/UserInterface/Views/EventListenerBreakpointPopover.js:59
> +	   this._inputElement =
contentElement.appendChild(document.createElement("input"));

I'd like to see a placeholder for this. Perhaps just:

    this._inputElement.placeholder = "click"

Would be enough for users to know what to type in here.

We may even be able to include autocompletion for a bunch of event names.
WI.ScriptTimelineRecord.EventType.displayName has 146 of them, but some look
out of date. We could do better and autogenerate from EventNames.h.

>
Source/WebInspectorUI/UserInterface/Views/EventListenerBreakpointTreeElement.js
:33
> +	   if (!className)
> +	       className =
WI.BreakpointTreeElement.GenericLineIconStyleClassName;

UI: I'd like the default to be a blue [E] icon. The core ideas being:

    • [E] means Event / Event Listener in other places in our UI
    • We pretty consistently use blue [#] icons to mean breakpoints

Basically modify EventListener.svg to have the color of Exception.svg.
NOTE: I just did that to produce the attached EventBreakpoint.svg.

> LayoutTests/inspector/dom-debugger/event-listener-breakpoints-expected.txt:6
> +Adding Breakpoint...

This would read better if you included the event name in the test. Perhaps
something like:

    Adding Breakpoint (click)...
    Adding "click" Event Breakpoint...

> LayoutTests/inspector/dom-debugger/event-listener-breakpoints-expected.txt:35
> +-- Running test teardown.

I would like to see another test for a custom event name and dispatch: (it
could be a separate test file if you want to keep this fundamental)
https://developer.mozilla.org/en-US/docs/Web/Guide/Events/Creating_and_triggeri
ng_events

If I understand things correctly if code on the page dispatches a custom event
"TestEvent", and we have a breakpoint on that name, we could potentially show
the dispatchEvent(...) in the backtrace, which would be pretty cool.

This also brings to mind that we should have a `$event` exposed to the console
when breaking inside of an event listener, like we do `$exception` when
breaking inside of an exception handler (catch block). That can be a separate
enhancement, but we should file a bug report on that. Technically this could be
different from `window.event` but maybe that is good enough for an initial
implementation.

> LayoutTests/inspector/dom-debugger/event-listener-breakpoints.html:37
> +	   return new Promise((resolve, reject) => {
> +	       InspectorTest.evaluateInPage(`clickBody()`, (error) => {
> +		   if (error)
> +		       reject(error);
> +		   else
> +		       resolve();
> +	       });
> +	   });
> +    }

This ma be the same as just:

    return InspectorTest.evaluateInPage(`clickBody()`);