Bug 38257 – Web Inspector: Linkify node and function in the event listeners panel.

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Pavel Feldman]

Patch to follow.

[Pavel Feldman]

Created attachment 54552 [details]
[IMAGE] Looks with the patch applied.

[Pavel Feldman]

Created attachment 54555 [details]
[PATCH] Proposed change.

[Timothy Hatcher]

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.

[Joseph Pecoraro]

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!

[Joseph Pecoraro]

(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!!

[Joseph Pecoraro]

Apparently my mid-air collision removed Timothy Hatcher's r+. Still, I think some of my comments and questions should be addressed before landing.

[Timothy Hatcher]

(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…

[Pavel Feldman]

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).

[Joseph Pecoraro]

Reopening so that some of my comments can be addressed!

[Pavel Feldman]

> > +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!

[Pavel Feldman]

> Even worse, its the wrong bug number!!

Guilty. I was landing multiple things at a time without using proper tooling.

[Joseph Pecoraro]

> >  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.

[Pavel Feldman]

Created attachment 54592 [details]
[PATCH] Follow up change.

[Joseph Pecoraro]

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.

[WebKit Commit Bot]

Comment on attachment 54592 [details]
[PATCH] Follow up change.

Clearing flags on attachment: 54592

Committed r58869: <http://trac.webkit.org/changeset/58869>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.