Bug 194293 - Web Inspector: DOM: don't send the entire function string with each event listener
Summary: Web Inspector: DOM: don't send the entire function string with each event lis...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-02-05 10:38 PST by Devin Rousso
Modified: 2019-02-06 16:32 PST (History)
10 users (show)

See Also:


Attachments
Patch (13.93 KB, patch)
2019-02-05 13:59 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (16.33 KB, patch)
2019-02-06 11:18 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (16.54 KB, patch)
2019-02-06 14:12 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (16.55 KB, patch)
2019-02-06 15:10 PST, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2019-02-05 10:38:31 PST
There's no reason to send the entire function body when all we really care about is the function name.
Comment 1 Radar WebKit Bug Importer 2019-02-05 10:38:51 PST
<rdar://problem/47822809>
Comment 2 Devin Rousso 2019-02-05 13:59:10 PST
Created attachment 361218 [details]
Patch
Comment 3 Build Bot 2019-02-05 14:01:51 PST
This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.
Comment 4 Joseph Pecoraro 2019-02-05 16:12:19 PST
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
Comment 5 Devin Rousso 2019-02-06 10:31:35 PST
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
Comment 6 Devin Rousso 2019-02-06 11:18:35 PST
Created attachment 361307 [details]
Patch
Comment 7 Joseph Pecoraro 2019-02-06 11:28:12 PST
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.
Comment 8 Devin Rousso 2019-02-06 14:12:12 PST
Created attachment 361327 [details]
Patch
Comment 9 Devin Rousso 2019-02-06 15:10:08 PST
Created attachment 361335 [details]
Patch
Comment 10 WebKit Commit Bot 2019-02-06 16:32:02 PST
Comment on attachment 361335 [details]
Patch

Clearing flags on attachment: 361335

Committed r241110: <https://trac.webkit.org/changeset/241110>
Comment 11 WebKit Commit Bot 2019-02-06 16:32:04 PST
All reviewed patches have been landed.  Closing bug.