Patch to follow.
Created attachment 54552 [details] [IMAGE] Looks with the patch applied.
Created attachment 54555 [details] [PATCH] Proposed change.
Comment on attachment 54555 [details] [PATCH] Proposed change. WebCore/inspector/front-end/ElementsPanel.js:775 + link.addEventListener("mousedown", selectNode, false); I think we normally do links on click not mousedown.
Comment on attachment 54555 [details] [PATCH] Proposed change. > + Web Inspector: Linkify node and function in the event listeners panel. > + > + https://bugs.webkit.org/show_bug.cgi?id=38251 NIT: Where is this blank line coming from? Also, the ChangeLog could use some content! > +bool eventListenerHandlerLocation(ScriptExecutionContext*, ScriptState*, EventListener*, String&, int&) > +{ > + return false; > +} A FIXME here, and opening up a bug report (if there isn't one already) would be best, so this doesn't get lost. > + sourceName = ""; > + lineNumber = 1; > + if (!origin.ResourceName().IsEmpty()) { > + sourceName = toWebCoreString(origin.ResourceName()); > + lineNumber = function->GetScriptLineNumber() + 1; > + return true; > + } I don't have v8 available at the moment. Two points. You set sourceName and lineNumber to defaults even if you return false, that doesn't seem necessary since they are ignored when the return value is false. Also, does this handle eval'd scripts or do those not have a ResourceName? > + linkifyNodeReference: function(node) > + { > + function selectNode(e) > + { > + WebInspector.updateFocusedNode(node.id); > + } > + > + var link = document.createElement("span"); > + link.className = "node-link"; > + link.addEventListener("mousedown", selectNode, false); > + this.decorateNodeLabel(node, link); > + return link; > + }, A function who's content is a single line typically cries out the outer function is not necessary. If you decide to keep the selectNode function please change its parameter "e" to "event" to match other handlers. But my suggestion would be something like this, since you're already burdened by creating a new function which bind does: > link.addEventListener("mousedown", WebInspector.updateFocusedNode.bind(null, node.id), false); By the way, where did the preventDefault go? Was it not needed? > + var subtitle = ""; > + var payload = this.eventListener; Both don't seem to be used. Subtitle you can remove, but you might want to keep "payload" and use that for the reminder of the code, in place of this.eventListener. Are functions linked in the general case (listing Object properties). For example: js> ({ a:WebInspector.loaded }); (is it linked in the normal object properties tree)? Waiting on an answer to some of my questions before flipping the flag!
(In reply to comment #4) > (From update of attachment 54555 [details]) > > + Web Inspector: Linkify node and function in the event listeners panel. > > + > > + https://bugs.webkit.org/show_bug.cgi?id=38251 > > NIT: Where is this blank line coming from? Also, the ChangeLog could use some > content! Even worse, its the wrong bug number!!
Apparently my mid-air collision removed Timothy Hatcher's r+. Still, I think some of my comments and questions should be addressed before landing.
(In reply to comment #4) > (From update of attachment 54555 [details]) > > + Web Inspector: Linkify node and function in the event listeners panel. > > + > > + https://bugs.webkit.org/show_bug.cgi?id=38251 > > NIT: Where is this blank line coming from? Also, the ChangeLog could use some > content! I prefer the blank line here. It bugs me the tool dosen't do it…
Committing to http://svn.webkit.org/repository/webkit/trunk ... M WebCore/ChangeLog M WebCore/bindings/js/ScriptEventListener.cpp M WebCore/bindings/js/ScriptEventListener.h M WebCore/bindings/v8/ScriptEventListener.cpp M WebCore/bindings/v8/ScriptEventListener.h M WebCore/inspector/InspectorDOMAgent.cpp M WebCore/inspector/front-end/ElementsPanel.js M WebCore/inspector/front-end/EventListenersSidebarPane.js M WebCore/inspector/front-end/StylesSidebarPane.js M WebCore/inspector/front-end/inspector.css Committed r58412 (review comments addressed).
Reopening so that some of my comments can be addressed!
> > +bool eventListenerHandlerLocation(ScriptExecutionContext*, ScriptState*, EventListener*, String&, int&) > > +{ > > + return false; > > +} > > A FIXME here, and opening up a bug report (if there isn't one already) would be > best, so this doesn't get lost. > Will add FIXME in follow up. > > > + sourceName = ""; > > + lineNumber = 1; > > + if (!origin.ResourceName().IsEmpty()) { > > + sourceName = toWebCoreString(origin.ResourceName()); > > + lineNumber = function->GetScriptLineNumber() + 1; > > + return true; > > + } > > I don't have v8 available at the moment. Two points. You set sourceName and > lineNumber to defaults even if you return false, that doesn't seem necessary > since they are ignored when the return value is false. Also, does this handle > eval'd scripts or do those not have a ResourceName? > Will do. I have no idea wrt eval names:) > > link.addEventListener("mousedown", WebInspector.updateFocusedNode.bind(null, node.id), false); > WebInspector.updateFocusedNode.bind(WebInspector, node.id) you mean. Point taken! > By the way, where did the preventDefault go? Was it not needed? > Now that we are a span, not <a> we don't need it. > > > + var subtitle = ""; > > + var payload = this.eventListener; > > Both don't seem to be used. Subtitle you can remove, but you might want to keep > "payload" and use that for the reminder of the code, in place of > this.eventListener. Ok. > > > > Are functions linked in the general case (listing Object properties). For > example: > > js> ({ a:WebInspector.loaded }); > (is it linked in the normal object properties tree)? > I don't think so. > Waiting on an answer to some of my questions before flipping the flag!
> Even worse, its the wrong bug number!! Guilty. I was landing multiple things at a time without using proper tooling.
> > link.addEventListener("mousedown", WebInspector.updateFocusedNode.bind(null, node.id), false); > > WebInspector.updateFocusedNode.bind(WebInspector, node.id) you mean. Point taken! Ahh, correct because WebInspector.updateFocusedNode uses "this". > > Are functions linked in the general case (listing Object properties). > > > > I don't think so. I think this would be useful. It can be done in another bug for sure. It would need to handle anonymous functions gracefully, which it looks like this did fine with.
Created attachment 54592 [details] [PATCH] Follow up change.
Comment on attachment 54592 [details] [PATCH] Follow up change. WebCore/inspector/front-end/EventListenersSidebarPane.js: + var payload = this.eventListener; I recommended keeping the payload "cached lookup", but we don't typically do that throughout the inspector, so its fine this way.
Comment on attachment 54592 [details] [PATCH] Follow up change. Clearing flags on attachment: 54592 Committed r58869: <http://trac.webkit.org/changeset/58869>
All reviewed patches have been landed. Closing bug.