[webkit-reviews] review denied: [Bug 135763] Web Inspector: Introduce an inspector Abstract Syntax Tree. : [Attachment 236491] patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has denied Saam Barati
<sbarati at apple.com>'s request for review:
Bug 135763: Web Inspector: Introduce an inspector Abstract Syntax Tree.
https://bugs.webkit.org/show_bug.cgi?id=135763

Attachment 236491: patch
https://bugs.webkit.org/attachment.cgi?id=236491&action=review

------- Additional Comments from Joseph Pecoraro <joepeck at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=236491&action=review


This looks good. Some cleanup suggestions. I'd like to see one more patch then
this should be good to land.

> Source/WebInspectorUI/ChangeLog:21
> +	  
(WebInspector.Script.prototype.requestScriptSyntaxTree.else.makeSyntaxTreeAfter
Timeout):

Heh this line can be removed.

> Source/WebInspectorUI/UserInterface/Models/Script.js:134
> +	   var self = this;

Style: We avoid self = this idioms and instead bind functions with "this". So
you should remove this and bind where necessary.

> Source/WebInspectorUI/UserInterface/Models/Script.js:137
> +	       setTimeout(function() { callback(self._scriptSyntaxTree); }, 0);


So this would be:

    setTimeout(function() { callback(this._scriptSyntaxTree; }.bind(this), 0);

> Source/WebInspectorUI/UserInterface/Models/Script.js:162
> +	   if (this.content)
> +	       makeSyntaxTreeAfterTimeout = function() {
makeSyntaxTreeAndCallCallback(self.content); };
> +	   else if (this._resource && this._resource.type ===
WebInspector.Resource.Type.Script && this._resource.finished &&
this._resource.content)
> +	       makeSyntaxTreeAfterTimeout = function() {
makeSyntaxTreeAndCallCallback(self._resource.content); };
> +	   else {
> +	       makeSyntaxTreeAfterTimeout = function() 
> +	       {
> +		   self.requestContent(function(error, sourceText) {
> +		       makeSyntaxTreeAndCallCallback(sourceText);
> +		   });
> +	       };
> +	   }
> +
> +	   setTimeout(makeSyntaxTreeAfterTimeout, 0);

This is a lot of indirection. For example requestContent already ensures a
setTimeout of its own. How about:

    var content = this.content;
    if (!content && this._resource && this._resource.type ===
WebInspector.Resource.Type.Script && this._resource.finished)
	content = this._resource.content
    if (content) {
	setTimeout(makeSyntaxTreeAndCallCallback.bind(this), 0, content);
	return;
    }

    this.requestContent(function(error, sourceText) {
	makeSyntaxTreeAndCallCallback.call(this, sourceText);
    }.bind(this));

> Source/WebInspectorUI/UserInterface/Models/Script.js:216
> +    _makeSyntaxTree: function(sourceText) {
> +	   if (!sourceText)
> +	       return;

I think you can also early return if this._scriptSyntaxTree is non-null.

This can happen if requestScriptSyntaxTree is called multiple times early on.
As it will queue a task to run "_makeSyntaxTree" multiple times. And we want to
avoid duplicating that work as it is an expensive operation.

> Source/WebInspectorUI/UserInterface/Models/Script.js:222
> +	       this._scriptSyntaxTree = null;

Nit: This line can be removed, the syntax tree will already be null.

> Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:30
> +    console.assert(script && script instanceof WebInspector.Script, script);

> +    WebInspector.Object.call(this);
> +    this._script = script;

Style: Lets put newlines between these lines for readability. See other
constructors like WebInspector.Probe or WebInspector.ProbeSet constructors.

> Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:45
> +WebInspector.ScriptSyntaxTree.NodeType = {
> +    AssignmentExpression:
"script-syntax-tree-node-type-assignment-expression",
> +    ArrayExpression: "script-syntax-tree-node-type-array-expression",
> +    BlockStatement: "script-syntax-tree-node-type-block-statement",

Call me crash, but I think removing the "script-syntax-tree-node-type-" prefix
would actually be beneficial here. So these 3 would just be:

    AssignmentExpression: "assignment-expression",
    ArrayExpression: "array-expression",
    BlockStatement: "block-statement",

Etc.

> Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:94
> +    forEachNode: function(func)
> +    {
> +	   this._recurse(this._syntaxTree, func, this._defaultParserState());
> +    },

I like Brian's suggestion of naming the parameter "callback".

You were hesitant about using "callback" because it can be called more than
once. But I think that ship has sailed. Even Array.prototype.forEach
terminology calls its function parameter a callback:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Object
s/Array/forEach

    arr.forEach(callback[, thisArg])

> Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:96
> +    filter: function(predicateFunction, startNode) 

I like the name "predicate" over "predicateFunction". We know a predicate is a
function.

> Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:117
> +	   var start = 0;
> +	   var end = 1;

Lets make these "const". They are just helper words for indexes into the range
pair, they are not variables expected to change.

> Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:120
> +	       // prog_start	range	  program end

Might as well spell this out to "program start". Add a few more spaces to each
of the lines to make it look this good. I found this comment helpful.

> Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:125
> +	       // If a node's range ends before the range we're interested in
starts, we don't need to search any of its
> +	       // enclosing ranges, because, by definition, those enclosing
ranges are contained within this node's outer range.

Nit: "within this node's outer range" => "within this node's range". The new
term outer range is confusing and made me think of something else.

> Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:129
> +	       // We are only insterested in nodes whose start pos is within
our range.

Typo: "insterested" => "interested"

> Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:157
> +	   var hasReturnStatement = false;

Might as well make this "hasNonEmptyReturnStatement" for clarity.

> Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:183
> +		   node.params.forEach(function (param) {

Style: No space between function and param list.

> Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:211
> +

Nit: console.assert(allRequests.length === allRequestNodes.length);

> Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:224
> +	       var i;
> +	       var node;
> +	       var typeInformation;
> +	       for (i = 0; i < typeInformationArray.length; i++) {
> +		   node = allRequestNodes[i];
> +		   typeInformation = typeInformationArray[i];

Style: Put the 'var's with their variables. No need for hoisting the var up
yourself.

> Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:276
> +	   case WebInspector.ScriptSyntaxTree.NodeType.BreakStatement:
> +	       func(node, state);
> +	       break;

Nit: In perhaps my lamest nit ever, break statements may have labels. I suspect
this should be the same as ContinueStatement.

Example program:

    outer:
    while (true) {
      console.log("outer");
      while (true) {
      console.log("inner");
	break outer;
      }
    }

Esprima tree:

    {
	"type": "BreakStatement",
	"label": {
	    "type": "Identifier",
	    "name": "outer"
	}
    }

Note: you do handle this in other cases.

> Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:453
> +    _recurseArray: function(array, func, state) 
> +    {
> +	   function forEach(node) {
> +	       this._recurse(node, func, state);
> +	   }
> +
> +	   array.forEach(forEach.bind(this));
> +    },

Nit: Unnecessary bind! Heh. How about:

    function each(node) {
	this._recurse(node, func, state);
    }

    array.forEach(each, this);

The duplicate "forEach" names are confusing.

> Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:761
> +};
> +/*
> + * Copyright (C) 2014 Apple Inc. All rights reserved.
> + *

Oops, looks like this file got duplicated. I don't know which you want to keep.
There are subtle differences. The bottom half has TernaryExpression and the top
doesn't.