Bug 170570 – Web Inspector: Event Listeners section does not update when listeners are added/removed

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Blaze Burg]

If a listener is removed using removeEventListener, or it's a once listener, it doesn't get removed from the sidebar until you select away and back to the element.

[Radar WebKit Bug Importer]

<rdar://problem/31501645>

[Devin Rousso]

Created attachment 320593 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 320593 [details]
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.

[Devin Rousso]

Comment on attachment 320593 [details]
Patch

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

>> Source/WebCore/inspector/InspectorDOMAgent.cpp:2190
>> +    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.

This seems reasonable.  I think we can move the backend logic to `DOM.setSelectedNode(nodeId)` in a future patch.  The frontend will not need to change, as all that will happen is the rest of the nodes will stop receiving events :P

Filed a followup <https://webkit.org/b/176827>

[Devin Rousso]

Created attachment 320625 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 320625 [details]
Patch

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

r=me Nice tests!

> LayoutTests/inspector/dom/event-listener-add-remove.html:110
> +                    logListeners(0)
> +                    .then(resolve, reject);

I really dislike this .then on a random line style. It is so unusual, especially in a case like this.

[Devin Rousso]

Created attachment 320703 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 320703 [details]
Patch

Clearing flags on attachment: 320703

Committed r222002: <http://trac.webkit.org/changeset/222002>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.