There's no reason to send the entire function body when all we really care about is the function name.
<rdar://problem/47822809>
Created attachment 361218 [details] Patch
This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.
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 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
Created attachment 361307 [details] Patch
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.
Created attachment 361327 [details] Patch
Created attachment 361335 [details] Patch
Comment on attachment 361335 [details] Patch Clearing flags on attachment: 361335 Committed r241110: <https://trac.webkit.org/changeset/241110>
All reviewed patches have been landed. Closing bug.