...
<rdar://problem/22611037>
*** Bug 148972 has been marked as a duplicate of this bug. ***
Created attachment 261698 [details] patch
Comment on attachment 261698 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=261698&action=review r=me, but there are some changes that I think deserve to be made to ScriptSyntaxTree at the same time. > Source/WebInspectorUI/ChangeLog:17 > + * UserInterface/External/Esprima/esprima.js: > + (assert): > + (isDecimalDigit): > + (isOctalDigit): > + (octalToDecimal): You can drop everything past esprima.js in the ChangeLog, it is not particularly useful. > Source/WebInspectorUI/UserInterface/External/Esprima/esprima.js:108 > + AssignmentPattern: 'AssignmentPattern', Of the new Nodes, there are three that WebInspector.ScriptSyntaxTree does not support but probably should: AssignmentPattern (destructuring) MetaProperty (new.target?) Super (super?) The rest are things we know about and haven't yet supported: ArrowFunctionExpression ExportAllDeclaration ExportDefaultDeclaration ExportNamedDeclaration ExportSpecifier ImportDeclaration ImportDefaultSpecifier ImportNamespaceSpecifier ImportSpecifier RestElement YieldStatement ---- It seems that, at least for this patch ScriptSyntaxTree should: 1. add support for AssignmentPattern to prevent breaking type profiling with destructuring assignments. 2. Cover MetaProperty and Super, which should be rather basic, to avoid potential issues parsing classes Do you agree, or were things working in these cases? > Source/WebInspectorUI/UserInterface/External/Esprima/esprima.js:2137 > + finishProgram: function (body, sourceType) { > + this.type = Syntax.Program; > + this.body = body; > + this.sourceType = sourceType; The "sourceType" property of Program is new. Maybe we should reflect that in ScriptSyntaxTree. It seems sourceType can be the string 'script' or 'module'. > Source/WebInspectorUI/UserInterface/External/Esprima/esprima.js:2246 > + finishTryStatement: function (block, handler, finalizer) { > + this.type = Syntax.TryStatement; > + this.block = block; > + this.guardedHandlers = []; > + this.handlers = handler ? [handler] : []; > + this.handler = handler; This is a particularly interesting change. There appears to be a single 'handler' property alongside 'handlers' which is either a single element array or empty array. Perhaps we should also move to a single handler as well.
Comment on attachment 261698 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=261698&action=review >> Source/WebInspectorUI/UserInterface/External/Esprima/esprima.js:2246 >> + this.handler = handler; > > This is a particularly interesting change. There appears to be a single 'handler' property alongside 'handlers' which is either a single element array or empty array. Perhaps we should also move to a single handler as well. Yes, further discussion on ESTree. Funny, this was the first ESTree issue! <https://github.com/estree/estree/issues/1>
(In reply to comment #5) > Comment on attachment 261698 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=261698&action=review > > >> Source/WebInspectorUI/UserInterface/External/Esprima/esprima.js:2246 > >> + this.handler = handler; > > > > This is a particularly interesting change. There appears to be a single 'handler' property alongside 'handlers' which is either a single element array or empty array. Perhaps we should also move to a single handler as well. > > Yes, further discussion on ESTree. Funny, this was the first ESTree issue! > <https://github.com/estree/estree/issues/1> And we will drop that in Esprima 3.0 (major version bump, breaking change): https://github.com/jquery/esprima/issues/1030
Created attachment 261717 [details] patch
Comment on attachment 261717 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=261717&action=review r=me, nice! > Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:449 > + case WebInspector.ScriptSyntaxTree.NodeType.Super: You could share the implementation of Meta/Super/This, since they are so super similar. Or we could try to keep this mostly alphabetical. Either way works! > Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:556 > + Style: Unnecessary newline. > LayoutTests/inspector/model/parse-script-syntax-tree.html:449 > + node = makeNode("class C { constructor() { super(); new.target; } }", false); Oh snap, I forgot about this test. It would also be awesome to have some tests for template strings / default arguments (which I recently added but did not test). But that is totally up to you.
Comment on attachment 261717 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=261717&action=review >> Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:449 >> + case WebInspector.ScriptSyntaxTree.NodeType.Super: > > You could share the implementation of Meta/Super/This, since they are so super similar. Or we could try to keep this mostly alphabetical. Either way works! I'll collect all leaf nodes into one section. >> LayoutTests/inspector/model/parse-script-syntax-tree.html:449 >> + node = makeNode("class C { constructor() { super(); new.target; } }", false); > > Oh snap, I forgot about this test. It would also be awesome to have some tests for template strings / default arguments (which I recently added but did not test). But that is totally up to you. Lets do this in another patch.
landed in: http://trac.webkit.org/changeset/190110