[webkit-reviews] review granted: [Bug 183138] Web Inspector: allow breakpoints to be set for specific event listeners : [Attachment 347521] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Devin Rousso
<drousso at apple.com>'s request for review:
Bug 183138: Web Inspector: allow breakpoints to be set for specific event
listeners
https://bugs.webkit.org/show_bug.cgi?id=183138

Attachment 347521: Patch

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




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

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

r=me

> Source/JavaScriptCore/inspector/protocol/DOM.json:94
> +		   { "name": "hasBreakpoint", "type": "boolean", "optional":
true }

Offline: These will require a slight internal change so hold off on landing
until we talk.

> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:847
> +	   for (auto& inspectorEventListener : m_eventListenerEntries.values())
{
> +	       if (inspectorEventListener.matches(*info.node, info.eventType,
listener.callback(), listener.useCapture())) {

Do you want to break inside this if to avoid looping over times after you've
found a match?

> Source/WebCore/inspector/agents/InspectorDOMAgent.h:305
> +	       return eventTarget.get() == &target && eventListener.get() ==
&listener && eventType == type && useCapture == capture;

Style: Lets break each condition into its own line to make this a little more
readable.

> Source/WebCore/inspector/agents/InspectorDOMDebuggerAgent.cpp:369
> +    bool shouldPause = m_debuggerAgent->pauseOnNextStatementEnabled() ||
m_eventListenerBreakpoints.contains(listenerEventCategoryType + event.type());

Should we rename "listenerEventCategoryType" from "listener:" to
"eventNameCategoryType" with a value of "event-name:"?

I found it confusing to distinction between actual event listener breakpoints
and event name breakpoints, especially given the poorly named
`InspectorDOMDebuggerAgent::setEventListenerBreakpoint` which is event name
breakpoints and we are just keeping the legacy name for backwards
compatibility.

> Source/WebCore/inspector/agents/InspectorDOMDebuggerAgent.cpp:383
> +    if (m_domAgent) {
> +	   int eventListenerId =
m_domAgent->idForEventListener(*event.currentTarget(), event.type(),
registeredEventListener.callback(), registeredEventListener.useCapture());
> +	   if (eventListenerId)
> +	       eventData->setInteger("eventListenerId"_s, eventListenerId);
> +    }

Its a shame that we are doing potentially 2 O(n) iterations to lookup
hasBreakpoints and id for the EventListener, but I think we can assume that the
number of event listener breakpoints is pretty small.

> Source/WebInspectorUI/UserInterface/Controllers/DOMTreeManager.js:53
> +    get eventBreakpoints()

Maybe we name these `eventListenerBreakpoints` so that it doesn't get confused
with `eventBreakpoints` on DOMDebuggerManager? I realize that the type are
WI.EventBreakpoint.

> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:1078
> +		   let eventListener = eventBreakpoint.eventListener;
> +		   if (eventListener && eventListener.eventListenerId ===
pauseData.eventListenerId) {

Would it be safe to drop this 2nd check, or just make it an assert?

    if (eventListener) {
	console.assert(eventListener.eventListenerId ===
pauseData.eventListenerId);
	...
    }

> Source/WebInspectorUI/UserInterface/Views/EventListenerSectionGroup.js:59
> +	       if (DOMAgent.setBreakpointForEventListener &&
DOMAgent.removeBreakpointForEventListener)

Good feature detection 👍!

> LayoutTests/inspector/dom/breakpoint-for-event-listener.html:47
> +		   if (paused)
> +		       InspectorTest.fail("Should not pause twice.");

I don't think this can be reached given `singleFireEventListener` above. So you
could probably drop this if you wanted.

> LayoutTests/inspector/dom/breakpoint-for-event-listener.html:53
> +		   InspectorTest.assert(targetData.pauseReason ===
WI.DebuggerManager.PauseReason.EventListener, `Pause reason should be
EventListener.`);
> +		   InspectorTest.assert(targetData.pauseData.eventName ===
"click", `Pause data eventName should be "click".`);

You could make these InspectorTest.expectThat(..., `...`) so these show up in
the output as PASS / FAIL lines.

> LayoutTests/inspector/dom/breakpoint-for-event-listener.html:112
> +		   if (paused)
> +		       InspectorTest.fail("Should not pause twice.");

Ditto.