* SUMMARY Improve JavaScript pretty printing. Goals: - Performance: Faster on large inputs - Correctness: Nested if/else/switch/try/for/while etc have always had problems - Asynchronous: Having the ability to run non-synchronously (in a Worker) would prevent possible UI hangs - Test Coverage: Our test coverage is currently pretty small for pretty-printing, we should expand it
<rdar://problem/25535719>
Created attachment 275569 [details] [PATCH] Part 1 - EsprimaFormatter
Created attachment 275570 [details] [PATCH] Part 2 - Tests
Created attachment 275571 [details] [IMAGE] Formatting Tool
In measuring the performance. Comparing the old formatter to the new formatter on jquery.min.js in the LayoutTests on my machine: Old: First run: 227ms Second run: 217ms Best run: 186ms New: First run: 181ms Second run: 97ms Best run: 76ms The slow first run of the new formatter was better than the best case of the old formatter. And very soon after it improves dramatically. This is likely due to JavaScriptCore's really great performance with Esprima and not necessarily with how the formatter is written. Worst case in this scenario we get about a 20% improvement, best case we see up to a 2.5x improvement. Both of these are using the same content builder, which along with building the output string builds the data we need to produce a SourceMap mapping the formatted content back to the original content.
Comment on attachment 275569 [details] [PATCH] Part 1 - EsprimaFormatter View in context: https://bugs.webkit.org/attachment.cgi?id=275569&action=review > Source/WebInspectorUI/UserInterface/Controllers/FormatterSourceMap.js:39 > + static fromSourceMapData({originalLineEndings, formattedLineEndings, mapping}) That is slick. Can we do more of this? > Source/WebInspectorUI/UserInterface/Workers/Formatter/EsprimaFormatter.js:42 > + })(); This seems to be handling both null return value and exceptions. Does esprima do both?
Comment on attachment 275569 [details] [PATCH] Part 1 - EsprimaFormatter Attachment 275569 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1099620 New failing tests: inspector/codemirror/prettyprinting-javascript.html inspector/codemirror/prettyprinting-css-rules.html inspector/codemirror/prettyprinting-css.html
Created attachment 275576 [details] Archive of layout-test-results from ews101 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-yosemite Platform: Mac OS X 10.10.5
(In reply to comment #6) > Comment on attachment 275569 [details] > [PATCH] Part 1 - EsprimaFormatter > > View in context: > https://bugs.webkit.org/attachment.cgi?id=275569&action=review > > > Source/WebInspectorUI/UserInterface/Controllers/FormatterSourceMap.js:39 > > + static fromSourceMapData({originalLineEndings, formattedLineEndings, mapping}) > > That is slick. Can we do more of this? Yes, I've been moving in this direction. All of the async Worker/Proxy code uses it. > > Source/WebInspectorUI/UserInterface/Workers/Formatter/EsprimaFormatter.js:42 > > + })(); > > This seems to be handling both null return value and exceptions. Does > esprima do both? Looking at Esprima, parse() either returns an AST where the first node is a Program node, or it throws an exception. It should not return null.
*** Bug 134003 has been marked as a duplicate of this bug. ***
*** Bug 117716 has been marked as a duplicate of this bug. ***
Comment on attachment 275570 [details] [PATCH] Part 2 - Tests View in context: https://bugs.webkit.org/attachment.cgi?id=275570&action=review > LayoutTests/inspector/formatting/resources/javascript-tests/generators-expected.js:4 > +function* gen() {} > +function* gen() { > + 1 > +} We may want to change the "*" placement to be "function *gen".
Created attachment 275864 [details] [PATCH] Follow-up: WithStatement
Created attachment 275865 [details] [PATCH] Follow-up: Correct formatted line endings from EsprimaFormatter
Created attachment 275866 [details] [PATCH] Follow-up: Improve semicolon handling in for loop headers
Created attachment 275867 [details] [PATCH] Follow-up: Pretty print JSON and fix up EsprimaFormatter error logic
Now that I've tested this a bit more with real world content I found a few minor issues and addressed them in small follow-up patches. Ultimately I intend to land this all at once, so these are just small patches on top of the existing Part 1 and 2 that will be squashed when landing.
Follow ups look good.
http://trac.webkit.org/changeset/199168