[webkit-reviews] review granted: [Bug 170570] Web Inspector: Event Listeners section does not update when listeners are added/removed : [Attachment 320593] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Devin Rousso
<webkit at devinrousso.com>'s request for review:
Bug 170570: Web Inspector: Event Listeners section does not update when
listeners are added/removed
https://bugs.webkit.org/show_bug.cgi?id=170570

Attachment 320593: Patch

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




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

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

r=me, but I'm fine with looking at another patch if you have a bunch of
changes.

> Source/WebCore/inspector/InspectorDOMAgent.cpp:2183
> +    UNUSED_PARAM(eventType);

Instead of doing this you should just remove the parameter. We don't use it.

> Source/WebCore/inspector/InspectorDOMAgent.cpp:2190
> +    Node* node = target.toNode();
> +    if (!node)
> +	   return;
> +
> +    int nodeId = boundNodeId(node);
> +    m_frontendDispatcher->didAddEventListener(nodeId);

This should bail if `nodeId` is `0`, meaning it is a node that has not been
sent to the frontend:

    int nodeId = boundNodeId(node);
    if (!nodeId)
	return;

However, I think we may want to reduce the traffic here by only sending updates
to the frontend for the Active / Selected node.

Currently the frontend tells the backend about the selected node via
`Console.addInspectedNode(nodeId)`, which informs `$0` in the Command Line API.

Maybe we should make that a real concept, like `DOM.setActiveNode(nodeId)` or
`DOM.setSelectedNode(nodeId)`, and here we could do:

    Node* node = target.toNode();
    if (!node)
	return;

    if (node != ...activeNode...)
	return;

I'm not sure how I feel about this.

    Pro: A lot less spam sent to the frontend for event listener changes.
    Con: The frontend would only be able to monitor changes to the active node,
	 so if we ever want to more broadly monitor we would have issues.

I guess I'd want to know how spammy this is on heavy pages. How many event
listener events are sent to the inspector? Given this is limited to bound nodes
it seems fine because only if the user starts digging around their DOM tree
would it get spammy. Given I currently suspect this is not too much of a
problem (DOM modifications are likely to be far more frequent) I'm only with
this as is.

> Source/WebCore/inspector/InspectorDOMAgent.cpp:2197
> +    UNUSED_PARAM(eventType);
> +    UNUSED_PARAM(listener);
> +    UNUSED_PARAM(capture);

Instead of doing this you should just remove the parameters. We don't use them.

> Source/WebInspectorUI/UserInterface/Controllers/DOMTreeManager.js:111
> +	   node.dispatchEventToListeners(WI.DOMNode.Event.EventListenerAdded);

Since there is no state in the frontend maybe we should just dispatch a
WI.DOMNode.Event.EventListenersChanged?

Also I can't think of a situation where a client would want to know about only
one event. Every client will want to know about both Add and Remove. So lets
merge them.

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:41
> +    addEventListeners()

These would then add and remove a single listener instead of two.

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:756
> +	   this._refreshEventListeners();

Does this happen even if the panel is not visible? That would seem like wasted
work. However its likely to be small and rare.

> LayoutTests/inspector/dom/event-listener-add-remove.html:62
> +    suite.addTestCase({
> +	   name: "DOM.eventListeners.Add",
> +	   description: "Test that adding an event listener is tracked.",

Can we add an attribute event listeners and verify this triggers.

    elem.onclick = function() { };

Also what happens if there was an attribute listener and then someone replaces
it. Do we get a Remove + Add? Do we get no Add? It seems we would want
something to know to update the frontend for the new value.