Bug 156178 – Web Inspector: Improve JavaScript pretty printing

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Joseph Pecoraro]

* 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

[Radar WebKit Bug Importer]

<rdar://problem/25535719>

[Joseph Pecoraro]

Created attachment 275569 [details]
[PATCH] Part 1 - EsprimaFormatter

[Joseph Pecoraro]

Created attachment 275570 [details]
[PATCH] Part 2 - Tests

[Joseph Pecoraro]

Created attachment 275571 [details]
[IMAGE] Formatting Tool

[Joseph Pecoraro]

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.

[Blaze Burg]

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?

[Build Bot]

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

[Build Bot]

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

[Joseph Pecoraro]

(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.

[Joseph Pecoraro]

*** Bug 134003 has been marked as a duplicate of this bug. ***

[Joseph Pecoraro]

*** Bug 117716 has been marked as a duplicate of this bug. ***

[Joseph Pecoraro]

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".

[Joseph Pecoraro]

Created attachment 275864 [details]
[PATCH] Follow-up: WithStatement

[Joseph Pecoraro]

Created attachment 275865 [details]
[PATCH] Follow-up: Correct formatted line endings from EsprimaFormatter

[Joseph Pecoraro]

Created attachment 275866 [details]
[PATCH] Follow-up: Improve semicolon handling in for loop headers

[Joseph Pecoraro]

Created attachment 275867 [details]
[PATCH] Follow-up: Pretty print JSON and fix up EsprimaFormatter error logic

[Joseph Pecoraro]

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.

[Timothy Hatcher]

Follow ups look good.

[Joseph Pecoraro]

http://trac.webkit.org/changeset/199168