[webkit-reviews] review granted: [Bug 201535] Web Inspector: Formatter: Pretty Print HTML resources (including inline <script>/<style>) : [Attachment 378658] [PATCH] Proposed Fix

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Joseph Pecoraro
<joepeck at webkit.org>'s request for review:
Bug 201535: Web Inspector: Formatter: Pretty Print HTML resources (including
inline <script>/<style>)
https://bugs.webkit.org/show_bug.cgi?id=201535

Attachment 378658: [PATCH] Proposed Fix

https://bugs.webkit.org/attachment.cgi?id=378658&action=review




--- Comment #23 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 378658
  --> https://bugs.webkit.org/attachment.cgi?id=378658
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=378658&action=review

r=me, given how large this patch is, it's getting very difficult to track
differences between each iteration, but the tests look great and I think I've
looked at enough of the code enough times to think that this is good to go.

Please run this through ESLint once before landing, as I did notice some style
issues (I pointed out the ones I saw, but there may be others).

Also, it looks like inspector/formatting/source-map-html-1.html is still too
slow for some reason, so that needs to be fixed as well.

Fantastic work!

Please make some followup bugs for some of the other things we've discussed:
 - ensure that `<p><p>` are siblings, not parent-child
 - add XML/SVG support

>
Source/WebInspectorUI/UserInterface/Workers/Formatter/FormatterContentBuilder.j
s:120
> +    appendNonToken(string)

Why does this need to exist?  Shouldn't every character that isn't whitespace
be considered a token?

>
Source/WebInspectorUI/UserInterface/Workers/Formatter/FormatterContentBuilder.j
s:143
> +	   console.assert(!string.includes("\n"), "Appended a string with
newlines. This breaks the source map.");

We should also assert that `!/\s$/.test(string)`, since any trailing whitespace
should really be handled by `appendSpace()`.

>
Source/WebInspectorUI/UserInterface/Workers/Formatter/FormatterContentBuilder.j
s:155
> +

Style: extra newline

>
Source/WebInspectorUI/UserInterface/Workers/Formatter/FormatterContentBuilder.j
s:178
> +	   this.lastTokenWasWhitespace = false;

Shouldn't this be `true` if we just added an indent?

>
Source/WebInspectorUI/UserInterface/Workers/Formatter/FormatterContentBuilder.j
s:303
> +	       let indent = this._indentCache[this._indent];
> +	       if (indent)

This seems wrong.  If it doesn't exist in `this._indentCache`, shouldn't we add
it?  Is it even possible to hit this?

> Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLFormatter.js:100
> +	   for (let childNode of children) {

NIT: this could just be `child`

> Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLFormatter.js:123
> +

Style: extra newline

> Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLFormatter.js:144
> +	   let {name, value, quote, namePos, valuePos} = attr;

Why not just destructure this in the parameters list?

> Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLFormatter.js:147
> +	       let q;

Style: this needs a default value

> Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLFormatter.js:293
> +	       let closingDoctypeString = ">";

Style: `const`

> Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLFormatter.js:300
> +	       let closingCDataString = "]]>";

Ditto (293)

> Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLParser.js:32
> +	   console.assert(treeBuilder);

What about an assertion for `sourceText`?

> Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLParser.js:54
> +    _isEOF()

Aside: we could totally make a base `Parser` class that holds all of these
functions, as I'm sure they'd also be useful elsewhere

> Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLParser.js:64
> +    _peekChararacter(c)

This is spelled very wrong lol.

Can we merge this with `_peekString`?  Since there's no real difference between
a character and a string in JS, we could just have `_peekString` handle single
character peeks.

> Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLParser.js:69
> +    _peekRegex(regex)

This name may be a bit misleading, because it only peeks a single character,
and `RegExp` can handle more than that.  Perhaps this should be
`_peekCharacterRegex`?	Or even better, assert that
`regex.source.startsWith("^")` and allow it to test
`this._data.substring(this._pos)`.

> Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLParser.js:90
> +	       let c = str[i];

This should be moved lower (or inlined) so that it isn't evaluated if we take
the `!d` branch.

> Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLParser.js:106
> +

Style: extra whitespace

> Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLParser.js:276
> +		   if (this._peekChararacter("/")) {
> +		       if (this._peekString("/>")) {

Why are we doing double the effort and peeking for "/" twice?  Can we not just
`this._peekString("/>")`?

> Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLParser.js:308
> +	   if (this._peekChararacter("/")) {
> +	       if (this._peekString("/>")) {

Ditto (275)

>
Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLTreeBuilderFormatter.
js:102
> +		   nodesToPop++;

Please move this above the `if` since it's called in both branches.  That way,
we only need one call.

>
Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLTreeBuilderFormatter.
js:153
> +	   }

Style: missing semicolon

>
Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLTreeBuilderFormatter.
js:164
> +	   }

Ditto (153)