[webkit-reviews] review granted: [Bug 201046] Web Inspector: provide a way to view XML/HTML/SVG resource responses as a DOM tree : [Attachment 377065] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Devin Rousso
<drousso at apple.com>'s request for review:
Bug 201046: Web Inspector: provide a way to view XML/HTML/SVG resource
responses as a DOM tree
https://bugs.webkit.org/show_bug.cgi?id=201046

Attachment 377065: Patch

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




--- Comment #4 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 377065
  --> https://bugs.webkit.org/attachment.cgi?id=377065
Patch

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

r=me

> Source/WebInspectorUI/UserInterface/Views/LocalDOMContentView.js:45
> +	   let content = this._content.replace(/`/g, "\\`");
> +	   let mimeType = this._mimeType.replace(/"/g, "\\\"");
> +	   return `(new DOMParser).parseFromString(\`${content}\`,
"${mimeType}")`;

Can these just be `JSON.stringify(x)` calls?

    return `(new DOMParser).parseFromString(${JSON.stringify(content)}),
${JSON.stringify(mimeType)})`;

> Source/WebInspectorUI/UserInterface/Views/LocalDOMContentView.js:56
> +		   treeOutline[WI.DOMTreeOutline.usingLocalDOMNodeSymbol] =
true;

Why not make this an API method instead of a symbol property?

> Source/WebInspectorUI/UserInterface/Views/LocalRemoteObjectContentView.js:59
> +	   RuntimeAgent.evaluate.invoke(options, (error, result, wasThrown) =>
{

I wonder if we should give this a `target` now?

I think this could be any target, in which case we could just use
`WI.mainTarget`.

> Source/WebInspectorUI/UserInterface/Views/ResourceClusterContentView.js:408
> +	   case "text/xml":
> +	   case "application/xml":
> +	   case "application/xhtml+xml":
> +	   case "image/svg+xml":

It is very common for arbitrary mime types to have a "+xml" suffix, so we
should probably do:

    if (mimeType.endsWith("/xml") || mimeType.endsWith("+xml"))
	...;

> Source/WebInspectorUI/UserInterface/Views/ResourceClusterContentView.js:412
> +	       } catch {}

Style: We normally have a space in here: `catch { }`.

> Source/WebInspectorUI/UserInterface/Views/ResourceClusterContentView.js:497
> +    Request: "request",
> +    RequestJSON: "request-json",

Why not Request DOM? I'm sure someone out there still sends XML to their
server!