Bug 201758 – Web Inspector: HTML Formatter - XML mode

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

[Joseph Pecoraro]

HTML Formatter - XML mode

- Handle CodeMirror "xml" mode.
- Turn off any of the HTML special casing in XML mode

[Joseph Pecoraro]

Created attachment 378754 [details]
[PATCH] Proposed Fix

This makes me think maybe we should avoid indenting in CDATA...

[Devin Rousso]

Comment on attachment 378754 [details]
[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

[Joseph Pecoraro]

Comment on attachment 378754 [details]
[PATCH] Proposed Fix

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

>> Source/WebInspectorUI/UserInterface/Views/TextEditor.js:185
>> +        return mode === "javascript" || mode === "css" || mode === "htmlmixed" || mode === "xml";
> 
> Just curious, which mode does SVG fall under?

If the MIME type is "image/svg+xml" then...

`htmlmixed`, but only because we special case it in CodeMirrorAdditions.js:

    var extraHTMLTypes = ["application/xhtml+xml", "image/svg+xml"];
    extraHTMLTypes.forEach(function(type) {
        CodeMirror.defineMIME(type, "htmlmixed");
    });

Otherwise I suspect it would have fallen to "application/xml" / "xml" in CodeMirror proper (codemirror.js):

    function resolveMode(spec) {
        ...
      } else if (typeof spec == "string" && /^[\w\-]+\/[\w\-]+\+xml$/.test(spec)) {
        return resolveMode("application/xml")
        ...
    }

>> Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLFormatter.js:28
>> +    constructor(sourceText, mode, builder, indentString = "    ")
> 
> Can we name this `sourceType` to match the JSFormatter?

Nice! "mode" was getting overloaded.

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

Nice

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

Sounds good, I debated this myself but though `_xml` read better. I'll change it to `_isXML` and likewise the option.

>> 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’`?

Sounds good.

[Joseph Pecoraro]

https://trac.webkit.org/r249867

[Radar WebKit Bug Importer]

<rdar://problem/55360465>

[Radar WebKit Bug Importer]

<rdar://problem/55360466>