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

[bugzilla-daemon at webkit.org]

Timothy Hatcher <timothy at apple.com> has granted 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 236614: patch
https://bugs.webkit.org/attachment.cgi?id=236614&action=review

------- Additional Comments from Timothy Hatcher <timothy at apple.com>
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.