| Summary: | Web Inspector: [ES6] Improve Type Profiler Support for Arrow Functions | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Joseph Pecoraro <joepeck> | ||||||
| Component: | Web Inspector | Assignee: | 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: |
|
||||||||
Retitling to be specifically about Arrow Functions. I think we've handled the rest for ES6, and Block Annotations is aa separate issue. Created attachment 261768 [details]
patch
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.
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 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? () => {} Created attachment 261782 [details]
patch
Fixed comments.
Also noted in comments that the "expression" field is just a boolean. Not an AST node.
Comment on attachment 261782 [details]
patch
r=me!
landed in: http://trac.webkit.org/changeset/190146 |
* 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