Bug 135763 – Web Inspector: Introduce an inspector Abstract Syntax Tree.

• Component: Web Inspector
• Status: RESOLVED FIXED

[Saam Barati]

Create a wrapper around the esprima abstract syntax tree and provide helper methods for manipulating the tree.

[Radar WebKit Bug Importer]

<rdar://problem/17961862>

[Saam Barati]

Created attachment 236296 [details]
work in progress

This is the AST portion of type information patch. See following comment for a few comments.

[Saam Barati]

Comment on attachment 236296 [details]
work in progress

View in context: https://bugs.webkit.org/attachment.cgi?id=236296&action=review

> Source/WebInspectorUI/UserInterface/Controllers/FormatterSourceMap.js:45
>      originalToFormatted: function(lineNumber, columnNumber)

Maybe this file should be a separate patch.

> Source/WebInspectorUI/UserInterface/Models/Script.js:137
> +        if (this.content) {

This code path still needs to be tested, but I'm waiting for debugging to not be broken to have this code path be taken.

> Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:759
> +        result.range = node.range;

Right now, range is relative to the text we pass in, but this doesn't account for the offsets in inline script tags. Type profiling currently isn't enabled for inline script tags, it's only enabled in JS files. Maybe we want to keep it this way. But if we ever want to change it to work for inline script tags, we should consider that these ranges can be offset by WebInspector.Script's range (there are probably a few other changes that need to be made as well).

> Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:766
> +/* This is a nice starting off point if you want to write a recursive function:

Should I nix this comment?

[Brian Burg]

Comment on attachment 236296 [details]
work in progress

View in context: https://bugs.webkit.org/attachment.cgi?id=236296&action=review

> Source/WebInspectorUI/UserInterface/Models/Script.js:142
> +                    self._makeSyntaxTree(self.content); 

This code seems a bit rough. All of the options seem to do the same thing, with a different argument to makeSyntaxTree(). Is there a reason for the 100ms delay?
Also, it seems that self.content could be a different value when the callback executes than when the if-condition is evaluated.

I would do something like:

var result = null;
if (this.content)
    result = Promise.resolve(this.content);
else if (/* long test */)
    result = Promise.resolve(this._resource.content);
else
    result = this.requestContent(); // could be combined with first branch; depends on https://bugs.webkit.org/show_bug.cgi?id=135777

result = result.then(function(sourceText) { self._makeSyntaxTree(sourceText); });
return result;


This will always execute _makeSyntaxTree asynchronously even if we already have it.

> Source/WebInspectorUI/UserInterface/Models/Script.js:222
> +        if (this._scriptSyntaxTreeDidLoadCallback) {

This callback thingy is overly coupled with callers. If you do want callback-passing style, it should be an argument to _makeSyntaxTree. I would prefer the promise-based approach above, though.

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

semicolon;

> Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:33
> +        var esprimaAST = WebInspector._esprima.parse(sourceText, { range : true });

nit: spacing like so:

{range: true}

Also, I think you could replace uses of AST and ast here with syntaxTree.

> Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:35
> +    } catch(e) {

Should this exception be handled here? Is there anything we can do with this class if the code is unparseable? I wonder if we could handle this one level higher.

> Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:36
> +        console.error("Couldn't parse JavaScript File: " + script.url + "\n" + e.message);

Passing e.message as its own argument will allow it to show in the console as a TreeOutline, rather than stringifying it.

console.error("...", e.message);

> Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:43
> +// Note that this corresponds identically to an enum in JavaSciptCore/runtime/HighFidelityTypeProfiler.h

"This should be kept in sync with the enum in ..."

> Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:45
> +    TypeProfilerSearchDescriptorNormal: 1,

No need to prefix enum elements with the enum name. It would read like: WebInspector.ScriptSyntaxTree.TypeProfilerSearchDescriptor.TypeProfilerSearchDescriptorNormal, which is redundant.

> Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:99
> +    forEachNode: function(func)

func=callback

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

func=predicate?

> Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:106
> +        startNode = startNode || this._ast;

check arguments. (null? callable? a node? hopefully the node is in the tree..)

> Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:125
> +        function forEach(node, state)

forEach=filterWithinRange? anything is a better name than forEach.

> Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:157
> +        function filter(node)

filter=hasNonFunctionType?

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

Pick a more descriptive name.

> Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:183
> +            var Types = WebInspector.ScriptSyntaxTree.NodeType;

I know it reads better if shortened, but it makes it harder to find/replace.

> Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:262
> +        case Types.AssignmentExpression:

Same comment as above.

> Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:465
> +    // This function translates from esprima's AST to ours. Mostly, this is just the identity function, but there are a few nodes and a few properties that are renamed.

Would prefer keeping lines to 100 columns or less.

> Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:467
> +    // ConditionalExpression is renamed to TernaryExpression.

Do these AST nodes have any relationship to JSC's AST nodes? I'm not quite sure why these specific things were renamed.

> Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:474
> +        switch(node.type) {

nit: switch (

> Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:475
> +        case 'AssignmentExpression':

In inspector code, we try to use the full name for string constants. This makes it much easier to do find/replace.

> Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:760
> +        result._attachments = {}; // This is an object for which you can add fields to an AST node without worrying about polluting the syntax-related fields of the node.

Put this comment on its own line.

>> Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:766
>> +/* This is a nice starting off point if you want to write a recursive function:
> 
> Should I nix this comment?

I think so, yes.

[Saam Barati]

Thanks again for the comments.

(In reply to comment #4)
> (From update of attachment 236296 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=236296&action=review
> 
> > Source/WebInspectorUI/UserInterface/Models/Script.js:142
> > +                    self._makeSyntaxTree(self.content); 
> 
> This code seems a bit rough. All of the options seem to do the same thing, with a different argument to makeSyntaxTree(). Is there a reason for the 100ms delay?

No, I was testing to see if it improved responsiveness when parsing a huge file, and it doesn't.

> Also, it seems that self.content could be a different value when the callback executes than when the if-condition is evaluated.
> 
> I would do something like:
> 
> var result = null;
> if (this.content)
>     result = Promise.resolve(this.content);
> else if (/* long test */)
>     result = Promise.resolve(this._resource.content);
> else
>     result = this.requestContent(); // could be combined with first branch; depends on https://bugs.webkit.org/show_bug.cgi?id=135777
> 
> result = result.then(function(sourceText) { self._makeSyntaxTree(sourceText); });
> return result;
> 

Yeah. This is much nicer. I think my nasty code originates from me being a promise n00b. I think this also gets rid of the necessity to have the ugly private callback (Given requestContent is changed to a promise based API).

> > Source/WebInspectorUI/UserInterface/Models/Script.js:222
> > +        if (this._scriptSyntaxTreeDidLoadCallback) {
> 
> This callback thingy is overly coupled with callers. If you do want callback-passing style, it should be an argument to _makeSyntaxTree. I would prefer the promise-based approach above, though.


> > Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:33
> > +        var esprimaAST = WebInspector._esprima.parse(sourceText, { range : true });
> 
> nit: spacing like so:
> 
> {range: true}
> 
> Also, I think you could replace uses of AST and ast here with syntaxTree.
> 
> > Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:35
> > +    } catch(e) {
> 
> Should this exception be handled here? Is there anything we can do with this class if the code is unparseable? I wonder if we could handle this one level higher.

Maybe this is the correct solution, that way Script will just set it's AST to null, which is expected behavior that it might be null.

> 
> > Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:36
> > +        console.error("Couldn't parse JavaScript File: " + script.url + "\n" + e.message);
> 
> Passing e.message as its own argument will allow it to show in the console as a TreeOutline, rather than stringifying it.
> 
> console.error("...", e.message);

Yeah this is nicer. I think I will just pass the whole error though, which seems like the better approach.

> > Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:99
> > +    forEachNode: function(func)
> 
> func=callback

To me, callback doesn't seem right. Because I think of a callback as only being called once. While this function is called for every node. I'm not sure what the best name for it would be though.

> 
> > Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:104
> > +    filter: function(func, startNode) 
> 
> func=predicate?

I agree.

> 
> > Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:106
> > +        startNode = startNode || this._ast;
> 
> check arguments. (null? callable? a node? hopefully the node is in the tree..)

This is actually never null, I think I had the null check in there from something I used to do but no longer do. I don't think there is any good way to assert that the node is in the tree without killing performance unless we attach a pointer to each node that points to the AST. What do you think we should do?

> 
> > Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:125
> > +        function forEach(node, state)
> 
> forEach=filterWithinRange? anything is a better name than forEach.
> 
> > Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:157
> > +        function filter(node)
> 
> filter=hasNonFunctionType?
> 
> > Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:182
> > +        function forEach(node) {
> 
> Pick a more descriptive name.
> 
> > Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:183
> > +            var Types = WebInspector.ScriptSyntaxTree.NodeType;
> 
> I know it reads better if shortened, but it makes it harder to find/replace.
> 
> > Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:262
> > +        case Types.AssignmentExpression:
> 
> Same comment as above.
> 
> > Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:465
> > +    // This function translates from esprima's AST to ours. Mostly, this is just the identity function, but there are a few nodes and a few properties that are renamed.
> 
> Would prefer keeping lines to 100 columns or less.
> 
> > Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:467
> > +    // ConditionalExpression is renamed to TernaryExpression.
> 
> Do these AST nodes have any relationship to JSC's AST nodes? I'm not quite sure why these specific things were renamed.

- Purely for aesthetic taste. I think prefix/postfix should be separate nodes.
- I would also say FunctionCall => FunctionCallExpression, but I suppose we can just keep it CallExpression.
- Also, I'm okay with moving from TernaryExpression => ConditionalExpression. It seems like many ASTs name ternary expressions this way. I just happen to disagree with that name. 

What are your thoughts on the naming?

As to what JSC does, I don't remember off the top of my head. JSC has a crazy AST that has different nodes for all kinds of stuff. I know it does separate prefix/postfix nodes, but I think it goes further and has nodes for:
bar++
foo.bar++
foo["bar"]++

[Saam Barati]

(In reply to comment #5)

> > 
> > > Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:125
> > > +        function forEach(node, state)
> > 
> > forEach=filterWithinRange? anything is a better name than forEach.
> > 
> > > Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:157
> > > +        function filter(node)
> > 
> > filter=hasNonFunctionType?
> > 
> > > Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:182
> > > +        function forEach(node) {
> > 
> > Pick a more descriptive name.
> > 
> > > Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:183
> > > +            var Types = WebInspector.ScriptSyntaxTree.NodeType;
> > 
> > I know it reads better if shortened, but it makes it harder to find/replace.
> > 
> > > Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:262
> > > +        case Types.AssignmentExpression:
> > 
> > Same comment as above.
> > 
> > > Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:465
> > > +    // This function translates from esprima's AST to ours. Mostly, this is just the identity function, but there are a few nodes and a few properties that are renamed.
> > 
> > Would prefer keeping lines to 100 columns or less.

(Sorry, this should all have been nixed from the above reply).

[Saam Barati]

Also, I replied in textual form to your comments, so when viewing them in context, their locations all got skewed, so just look at it in textual form.

[Saam Barati]

Comment on attachment 236296 [details]
work in progress

View in context: https://bugs.webkit.org/attachment.cgi?id=236296&action=review

>> Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:475
>> +        case 'AssignmentExpression':
> 
> In inspector code, we try to use the full name for string constants. This makes it much easier to do find/replace.

These constants are ever only used within this function because they are the names of the nodes inside esprima's AST.

[Saam Barati]

Created attachment 236397 [details]
patch

Changes accounted for. Let me know what you think

[Timothy Hatcher]

Comment on attachment 236397 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=236397&action=review

> Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:32
> +    var esprimaSyntaxTree = WebInspector._esprima.parse(sourceText, {range: true});

Can we control this entry point? I think WebInspector.Esprima or just Esprima would be best. The underscore denotes private and looks like a layering violation.

> Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:43
> +    AssignmentExpression: 'AssignmentExpression',

Use double quotes. (Apples through the file.)

We like to make the string something like: "script-syntax-tree-node-type-assignment-expression" so it is not ambiguous and debuggable.

[Saam Barati]

Created attachment 236491 [details]
patch

This patch adds an Abstract Syntax Tree to the Web inspector. 
This syntax tree is modeled off the Esprima.js syntax tree
which complies with the Mozilla Parser API:
https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Parser_API
Script is the owner of the Web Inspector syntax tree, and has an API
for accessing its syntax tree property.

[Joseph Pecoraro]

Comment on attachment 236491 [details]
patch

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

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

[Saam Barati]

Created attachment 236614 [details]
patch

Updated with Joe's suggestions.

[Timothy Hatcher]

Comment on attachment 236614 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=236614&action=review

I'll let Joe look again before cq+, but I think r+.

> Source/WebInspectorUI/UserInterface/Models/Script.js:215
> +        try {
> +            this._scriptSyntaxTree = new WebInspector.ScriptSyntaxTree(sourceText, this);
> +        } catch(error) {
> +            console.error("Couldn't parse JavaScript File: " + this._url, error);
> +        }

I assume this catches exceptions from Esprima? We should catch those exceptions inside the WebInspector.ScriptSyntaxTree class and log the error there. Clients of WebInspector.ScriptSyntaxTree should not need to care about Esprima exceptions, that is a layering violation.

> Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:109
> +        }
> +        this._recurse(startNode, filter, this._defaultParserState());

I like newlines between lines like this.

[Timothy Hatcher]

Comment on attachment 236614 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=236614&action=review

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

for (..of..)?

> Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:211
> +        nodesToUpdate.forEach(gatherTypeInformationRequestForNode);

Ditto.

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

for (..of..)!

[Saam Barati]

Created attachment 236674 [details]
patch

for(...of...) to rule them all.

Also added a property on the SyntaxTree indicating if it was successfully parsed.

[Joseph Pecoraro]

Comment on attachment 236674 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=236674&action=review

Looks good to me. A few style ignorable style nits.

> Source/WebInspectorUI/UserInterface/Models/Script.js:135
> +        if (this._scriptSyntaxTree) {
> +            setTimeout(function() { callback(this._scriptSyntaxTree); }.bind(this), 0);

Now that I read this, I think this might be more efficient, avoiding the bind.

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

> Source/WebInspectorUI/UserInterface/Models/Script.js:140
> +        var makeSyntaxTreeAndCallCallback = function(content)
> +        {

Style: I would prefer this style for the closure:

    function makeSyntaxTreeAndCallCallback(content) {
        ...
    }.bind(this)

> Source/WebInspectorUI/UserInterface/Models/Script.js:207
> +    _makeSyntaxTree: function(sourceText) {

Style: brace on new line.

[Saam Barati]

Created attachment 236688 [details]
patch

style fix.

[Joseph Pecoraro]

Comment on attachment 236688 [details]
patch

r=me

[WebKit Commit Bot]

Comment on attachment 236688 [details]
patch

Clearing flags on attachment: 236688

Committed r172722: <http://trac.webkit.org/changeset/172722>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.