[webkit-reviews] review denied: [Bug 194293] Web Inspector: DOM: don't send the entire function string with each event listener : [Attachment 361218] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has denied Devin Rousso
<drousso at apple.com>'s request for review:
Bug 194293: Web Inspector: DOM: don't send the entire function string with each
event listener
https://bugs.webkit.org/show_bug.cgi?id=194293

Attachment 361218: Patch

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




--- Comment #4 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 361218
  --> https://bugs.webkit.org/attachment.cgi?id=361218
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