[webkit-reviews] review granted: [Bug 136272] Web Inspector: Write tests for ScriptSyntaxTree and fix bugs in the data structure : [Attachment 237279] patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Saam Barati
<sbarati at apple.com>'s request for review:
Bug 136272: Web Inspector: Write tests for ScriptSyntaxTree and fix bugs in the
data structure
https://bugs.webkit.org/show_bug.cgi?id=136272

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

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


Nice! Some suggestions for a few more tests.

> LayoutTests/inspector/model/parse-script-syntax-tree.html:203
> +    node = makeNode("true;", true);
> +    InspectorTest.assert(node.type ===
WebInspector.ScriptSyntaxTree.NodeType.Literal);
> +    InspectorTest.assert(node.value === true);
> +    InspectorTest.assert(node.raw === "true");

We might want to also test a few other literals?

  "null"
  "/regex/ig"
  "0x10"
  "0777"
  "\"a\""

> LayoutTests/inspector/model/parse-script-syntax-tree.html:276
> +    node = makeNode("x = {foo:20};", true);
> +    InspectorTest.assert(node.right.properties[0].type ===
WebInspector.ScriptSyntaxTree.NodeType.Property);
> +    InspectorTest.assert(node.right.properties[0].key);
> +    InspectorTest.assert(node.right.properties[0].key.type ===
WebInspector.ScriptSyntaxTree.NodeType.Identifier);
> +    InspectorTest.assert(node.right.properties[0].value);
> +    InspectorTest.assert(node.right.properties[0].value.type ===
WebInspector.ScriptSyntaxTree.NodeType.Literal);
> +    node = makeNode("x = {'foo':20};", true);
> +    InspectorTest.assert(node.right.properties[0].key);
> +    InspectorTest.assert(node.right.properties[0].key.type ===
WebInspector.ScriptSyntaxTree.NodeType.Literal);
> +    InspectorTest.log("passed Property");

I'd suggest testing isGetterOrSetter is false here, and testing cases where it
is true.

    "x = {get foo() {return 20}}"
    "x = {set foo(x) {}}"

> LayoutTests/inspector/model/parse-script-syntax-tree.html:327
> +    node = makeNode("foo++;", true);
> +    InspectorTest.assert(node.type ===
WebInspector.ScriptSyntaxTree.NodeType.UpdateExpression);
> +    InspectorTest.log("passed UpdateExpression");

Include a test for prefix === false here and a pre-increment/decrement with
prefix === true.