[webkit-reviews] review granted: [Bug 201758] Web Inspector: HTML Formatter - XML mode : [Attachment 378754] [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 201758: Web Inspector: HTML Formatter - XML mode
https://bugs.webkit.org/show_bug.cgi?id=201758

Attachment 378754: [PATCH] Proposed Fix

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




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

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

r=me, I thought that XML would be simple, but not this simple! :D

> Source/WebInspectorUI/UserInterface/Views/TextEditor.js:185
> +	   return mode === "javascript" || mode === "css" || mode ===
"htmlmixed" || mode === "xml";

Just curious, which mode does SVG fall under?

> Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLFormatter.js:28
> +    constructor(sourceText, mode, builder, indentString = "	  ")

Can we name this `sourceType` to match the JSFormatter?

> Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLFormatter.js:39
> +		   let options = {xml: mode === HTMLFormatter.Mode.XML};

Style: given that this isn’t very “simple” (e.g. not just adding a key or
remapping a variable) or “short” , I’d prefer this on multiple lines for
readability.

> Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLFormatter.js:120
> +	       console.assert(false, "Unknown mode", mode);

I personally prefer putting these types of “not reached” outside of the
switch-case altogether and in order to have it be less indented and therefore
possibly more visible.

Also `mode` isn’t defined so this would throw an error :(

> Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLParser.js:30
> +    parseDocument(sourceText, treeBuilder, {xml} = {})

`isXML`?

> Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLParser.js:42
> +	   this._xml = xml || false;

Rather than have a fallback, since we expect a boolean, can we just `!!isXML’`?

>
Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLTreeBuilderFormatter.
js:37
> +    constructor({xml} = {})

Ditto

>
Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLTreeBuilderFormatter.
js:39
> +	   this._xml = xml || false;

Ditto