[webkit-reviews] review requested: [Bug 38257] Web Inspector: Linkify node and function in the event listeners panel. : [Attachment 54555] [PATCH] Proposed change.

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has asked	for review:
Bug 38257: Web Inspector: Linkify node and function in the event listeners
panel.
https://bugs.webkit.org/show_bug.cgi?id=38257

Attachment 54555: [PATCH] Proposed change.
https://bugs.webkit.org/attachment.cgi?id=54555&action=review

------- Additional Comments from Joseph Pecoraro <joepeck at webkit.org>
> +	   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!