Bug 143171

Summary: Web Inspector: [ES6] Improve Type Profiler Support for Arrow Functions
Product: WebKit Reporter: Joseph Pecoraro <joepeck>
Component: Web InspectorAssignee: Saam Barati <saam>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, graouts, joepeck, jonowells, mattbaker, nvasilyev, saam, timothy, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
patch
joepeck: review-
patch joepeck: review+

Joseph Pecoraro
Reported 2015-03-27 18:44:38 PDT
* SUMMARY Improve Type Profiler Support for ES6 Syntax. • ScriptSyntaxTree should support ES6 syntax that Esprima produces - template strings - modules, packages (export/import) - arrow functions - binding patterns - comprehensions? (ES7?) • BasicBlockAnnotator for method syntax is a bit off - closing brace doesn't appear to be included
Attachments
patch (14.39 KB, patch)
2015-09-22 15:08 PDT, Saam Barati
joepeck: review-
patch (18.96 KB, patch)
2015-09-22 17:26 PDT, Saam Barati
joepeck: review+
Radar WebKit Bug Importer
Comment 1 2015-03-27 18:45:09 PDT
Joseph Pecoraro
Comment 2 2015-09-21 15:31:30 PDT
Retitling to be specifically about Arrow Functions. I think we've handled the rest for ES6, and Block Annotations is aa separate issue.
Saam Barati
Comment 3 2015-09-22 15:08:23 PDT
WebKit Commit Bot
Comment 4 2015-09-22 15:10:53 PDT
Attachment 261768 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/TypeProfiler.h:107: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 1 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
Joseph Pecoraro
Comment 5 2015-09-22 15:31:11 PDT
Comment on attachment 261768 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=261768&action=review > Source/JavaScriptCore/runtime/TypeProfiler.h:73 > + } > + bool operator==(const QueryKey& other) const Style: Normally there are newlines between multi-line member functions. > Source/JavaScriptCore/tests/typeProfiler/arrow-functions.js:4 > +let foo = (x) => x; > +let bar = abc => abc; Might want to also test a case with {}s. let baz = x => { return x } > Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:152 > + console.assert( node.type === WebInspector.ScriptSyntaxTree.NodeType.FunctionDeclaration || node.type === WebInspector.ScriptSyntaxTree.NodeType.FunctionExpression || node.type === WebInspector.ScriptSyntaxTree.NodeType.MethodDefinition || node.type === WebInspector.ScriptSyntaxTree.NodeType.ArrowFunctionExpression); Style: Unnecessary leading space inside the assert(...). > Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:372 > + case WebInspector.ScriptSyntaxTree.NodeType.ArrowFunctionExpression: > callback(node, state); > this._recurse(node.id, callback, state); > this._recurseArray(node.params, callback, state); r- ArrowFunctionExpression has "expression" but FunctionExpression/FunctionDeclaration don't. We'll need to walk over that. Either a new case, or check `if (node.expression)`. > Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:955 > + ArrowFunctionExpression: Symbol("arrow-function-expression"), Nit: Alphabetical order (`sort`) We should add a basic test for the frontend's new node type in: LayoutTests/inspector/model/parse-script-syntax-tree.html
Joseph Pecoraro
Comment 6 2015-09-22 15:32:20 PDT
Comment on attachment 261768 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=261768&action=review >> Source/JavaScriptCore/tests/typeProfiler/arrow-functions.js:4 >> +let bar = abc => abc; > > Might want to also test a case with {}s. > > let baz = x => { return x } And maybe even an empty case? () => {}
Saam Barati
Comment 7 2015-09-22 17:26:40 PDT
Created attachment 261782 [details] patch Fixed comments. Also noted in comments that the "expression" field is just a boolean. Not an AST node.
Joseph Pecoraro
Comment 8 2015-09-22 17:29:41 PDT
Comment on attachment 261782 [details] patch r=me!
Saam Barati
Comment 9 2015-09-22 17:39:47 PDT
Note You need to log in before you can comment on or make changes to this bug.