Bug 194293 – Web Inspector: DOM: don't send the entire function string with each event listener

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

[Devin Rousso]

There's no reason to send the entire function body when all we really care about is the function name.

[Radar WebKit Bug Importer]

<rdar://problem/47822809>

[Devin Rousso]

Created attachment 361218 [details]
Patch

[EWS Watchlist]

This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.

[Joseph Pecoraro]

Comment on attachment 361218 [details]
Patch

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

r- because I want to see an updated patch RE: the object case.

> Source/JavaScriptCore/inspector/protocol/DOM.json:84
> +                { "name": "handlerName", "type": "string", "optional": true, "description": "Event handler function name." },
> +                { "name": "handlerObject", "$ref": "Runtime.RemoteObject", "optional": true, "description": "Event handler function value." },

Reminder: Email ITML folks regarding the DOM change.

I like the name `handlerObject` since I think this can be an actual object in the case where we have:

    elem.addEventListener("eventname", this);

In that case:

    (1) what is the handlerName
    (2) if we have a `handlerObject` we should update the protocol "description" to not say "function value".

> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:1676
> +        handlerObject = scriptListener.jsFunction(node->document());

Maybe this doesn't work in the object listener case.

> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:1679
> +                handlerName = function->calculatedDisplayName(state->vm());

I wonder if this requires a JSLock or not, I believe this might cause allocations. You could discuss this with Saam or Mark Lam.

> Source/WebInspectorUI/UserInterface/Views/EventListenerSectionGroup.js:91
> +            if (match) {
> +                functionName = match[1];
> +            }

Style: No braces

> LayoutTests/inspector/dom/getEventListenersForNode.html:77
> +        document.addEventListener("alpha", function documentAlpha(event) { }, {passive: true});
> +        document.addEventListener("beta", (event) => {}, {passive: true});

Please add tests for the object listener case, which just means some object that has a `handleEvent` function:
https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener

[Devin Rousso]

Comment on attachment 361218 [details]
Patch

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

>> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:1679
>> +                handlerName = function->calculatedDisplayName(state->vm());
> 
> I wonder if this requires a JSLock or not, I believe this might cause allocations. You could discuss this with Saam or Mark Lam.

We already have acquired a lock above.

>> LayoutTests/inspector/dom/getEventListenersForNode.html:77
>> +        document.addEventListener("beta", (event) => {}, {passive: true});
> 
> Please add tests for the object listener case, which just means some object that has a `handleEvent` function:
> https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener

Right!  I completely forgot about that functionality :P

[Devin Rousso]

Created attachment 361307 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 361307 [details]
Patch

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

r=me

> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:1684
> +                if (auto handleEventValue = handlerObject->get(exec, JSC::Identifier::fromString(exec, "handleEvent")))

This can cause an exception which may need to handled... that said this is all in Web Inspector code which would just ignore the exception.

[Devin Rousso]

Created attachment 361327 [details]
Patch

[Devin Rousso]

Created attachment 361335 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 361335 [details]
Patch

Clearing flags on attachment: 361335

Committed r241110: <https://trac.webkit.org/changeset/241110>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.