Web Inspector: Render non-document HTML resources as HTML

[Vsevolod Vlasov]

Render non-document HTML resources as HTML

[Vsevolod Vlasov]

Created attachment 90189 [details]
Patch

[Vsevolod Vlasov]

Created attachment 90190 [details]
Fixed quotes

[Alexander Pavlov (apavlov)]

Comment on attachment 90190 [details]
Fixed quotes

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

> Source/WebCore/inspector/front-end/ResourceHTMLView.js:1
> +/*

Please add it into WebCore.vcproj as well (and checking if all of our JS files are there is a good idea).

> Source/WebCore/inspector/front-end/ResourceHTMLView.js:57
> +        iframe.sandbox=""; // Forbid to run JavaScript and set unique origin

Comments are full sentences in WebKit (end in a period).

[Vsevolod Vlasov]

Created attachment 90201 [details]
Patch with fixes

Done these.
I'll add missing files in .vcproj in separate bug

[Adam Barth]

Comment on attachment 90201 [details]
Patch with fixes

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

> Source/WebCore/inspector/front-end/NetworkItemView.js:58
> +    if (resource.type !== WebInspector.Resource.Type.Document && resource.content && resource.mimeType == "text/html") {

Has mimeType already been canonicalized (e.g., converted to lower case)?  What about application/xml+html ?

> Source/WebCore/inspector/front-end/ResourceHTMLView.js:57
> +        iframe.sandbox=""; // Forbid to run JavaScript and set unique origin.

You should use setAttribute rather than the DOM reflection of the attribute.

[Vsevolod Vlasov]

> > Source/WebCore/inspector/front-end/NetworkItemView.js:58
> > +    if (resource.type !== WebInspector.Resource.Type.Document && resource.content && resource.mimeType == "text/html") {
> 
> Has mimeType already been canonicalized (e.g., converted to lower case)?  What about application/xml+html ?
I removed this check. We decided that we should render HTML only for resources with error status code.

> > Source/WebCore/inspector/front-end/ResourceHTMLView.js:57
> > +        iframe.sandbox=""; // Forbid to run JavaScript and set unique origin.
> 
> You should use setAttribute rather than the DOM reflection of the attribute.
Done

[Vsevolod Vlasov]

Created attachment 90351 [details]
Patch with fixes

[Pavel Feldman]

Comment on attachment 90351 [details]
Patch with fixes

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

> Source/WebCore/inspector/front-end/NetworkItemView.js:48
> +        var responseView = new WebInspector.ResourceSourceFrame(resource);

You could reuse existing SourceFrame in case previewView is instance of ResourceSourceFrame. In fact, you should adjust resourceViewForResource to cache only SourceFrame views.

> Source/WebCore/inspector/front-end/Resource.js:795
> +            if (data == null)

pending callbacks should be notified upon missing content.

> Source/WebCore/inspector/front-end/ResourceHTMLView.js:52
> +        this.element.textContent = "";

this.element.removeChildren()

> Source/WebCore/inspector/front-end/ResourceView.js:49
> +        return new WebInspector.ResourceHTMLView(resource);

Reuse SourceFrame-based views when possible as per comment above.

> Source/WebCore/inspector/front-end/ResourceView.js:80
> +        if (WebInspector.ResourceJSONView.parseJSON(resource.content))

This seems to be a bit too expensive. Testing for the prototype would be sufficient.

[Vsevolod Vlasov]

(In reply to comment #8)
> (From update of attachment 90351 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=90351&action=review
> 
> > Source/WebCore/inspector/front-end/NetworkItemView.js:48
> > +        var responseView = new WebInspector.ResourceSourceFrame(resource);
> 
> You could reuse existing SourceFrame in case previewView is instance of ResourceSourceFrame. In fact, you should adjust resourceViewForResource to cache only SourceFrame views.
We ar enot caching SourceFrames anymore, but I reuse SourceFrame for the response and rpeview view the same resource.

> > Source/WebCore/inspector/front-end/Resource.js:795
> > +            if (data == null)
> 
> pending callbacks should be notified upon missing content.
Removed that.

> > Source/WebCore/inspector/front-end/ResourceHTMLView.js:52
> > +        this.element.textContent = "";
> 
> this.element.removeChildren()
Done.

> > Source/WebCore/inspector/front-end/ResourceView.js:49
> > +        return new WebInspector.ResourceHTMLView(resource);
> 
> Reuse SourceFrame-based views when possible as per comment above.
Reusing SourceFrame between preview and response view now.

> > Source/WebCore/inspector/front-end/ResourceView.js:80
> > +        if (WebInspector.ResourceJSONView.parseJSON(resource.content))
> 
> This seems to be a bit too expensive. Testing for the prototype would be sufficient.
This method was removed, so this is not needed anymore.

[Vsevolod Vlasov]

Created attachment 94979 [details]
Patch (with fixes)

[Pavel Feldman]

Comment on attachment 94979 [details]
Patch (with fixes)

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

> Source/WebCore/inspector/front-end/Settings.js:71
> +    this.installApplicationSetting("resourceViewTab", "preview");

Where will user be navigated in case he has "content" in his settings?

[Vsevolod Vlasov]

(In reply to comment #11)
> (From update of attachment 94979 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=94979&action=review
> 
> > Source/WebCore/inspector/front-end/Settings.js:71
> > +    this.installApplicationSetting("resourceViewTab", "preview");
> 
> Where will user be navigated in case he has "content" in his settings?
To headers tab that is the default one.
http://codesearch.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/inspector/front-end/NetworkItemView.js&q=resourceViewTab&exact_package=chromium&l=83

[WebKit Review Bot]

Comment on attachment 94979 [details]
Patch (with fixes)

Clearing flags on attachment: 94979

Committed r88348: <http://trac.webkit.org/changeset/88348>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Yury Semikhatsky]

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