Web Inspector: Display "Resource has no content" for about:blank iframes instead of an error

• Component: Web Inspector
• Status: RESOLVED FIXED

[Nikita Vasilyev]

Created attachment 370230 [details]
[HTML] Reduction

When "about:blank" iframes get created in JS, they don't have any content*. When selecting them in Resources tab, we show a message in red text:

    An error occurred trying to load the resource

This is misleading. No errors occurred - it had no content!


*: iframes with srcdoc do have content, which we should continue displaying in the content view.

<rdar://problem/41911608>

[Nikita Vasilyev]

Created attachment 370231 [details]
Patch

[Nikita Vasilyev]

Created attachment 370232 [details]
[Image] With patch applied

[Devin Rousso]

Comment on attachment 370231 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/ResourceContentView.js:40
> +        if (resource.url === "about:blank") {

This feels like a hack.  Do you know what the protocol error actually is?  Could we modify the backend to treat this as an empty resource content instead of an error?

Does this also need to be done for any `TextEditor` subclasses?

[Nikita Vasilyev]

(In reply to Devin Rousso from comment #3)
> Comment on attachment 370231 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=370231&action=review
> 
> > Source/WebInspectorUI/UserInterface/Views/ResourceContentView.js:40
> > +        if (resource.url === "about:blank") {
> 
> This feels like a hack.  Do you know what the protocol error actually is?

Resource.js:

    requestContentFromBackend()
    {
        ...
        // There is no request identifier or frame to request content from.
        if (this._parentFrame)
            return PageAgent.getResourceContent(this._parentFrame.id, this._url);

This results in the following error:

    column: 29
    line: 158
    message: "No resource with given URL found"
    sourceURL: "file:///Volumes/.../WebKitBuild/Release/WebInspectorUI.framework/Resources/Protocol/Connection.js"
    stack: "_dispatchResponseToPromise@file:///Volumes/.../WebKitBuild/Release/WebInspectorUI.framework/Resources/Proto…"

[Devin Rousso]

Comment on attachment 370231 [details]
Patch

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

>>> Source/WebInspectorUI/UserInterface/Views/ResourceContentView.js:40
>>> +        if (resource.url === "about:blank") {
>> 
>> This feels like a hack.  Do you know what the protocol error actually is?  Could we modify the backend to treat this as an empty resource content instead of an error?
>> 
>> Does this also need to be done for any `TextEditor` subclasses?
> 
> Resource.js:
> 
>     requestContentFromBackend()
>     {
>         ...
>         // There is no request identifier or frame to request content from.
>         if (this._parentFrame)
>             return PageAgent.getResourceContent(this._parentFrame.id, this._url);
> 
> This results in the following error:
> 
>     column: 29
>     line: 158
>     message: "No resource with given URL found"
>     sourceURL: "file:///Volumes/.../WebKitBuild/Release/WebInspectorUI.framework/Resources/Protocol/Connection.js"
>     stack: "_dispatchResponseToPromise@file:///Volumes/.../WebKitBuild/Release/WebInspectorUI.framework/Resources/Proto…"

Ah, well I guess that makes sense.

> Source/WebInspectorUI/UserInterface/Views/ResourceContentView.js:42
> +            return;

I personally think we should avoid having early-returns in constructors, as that's kinda funky.  It won't "mess up" the `ResourceContentView` (see <https://tc39.es/ecma262/#sec-ecmascript-function-objects-construct-argumentslist-newtarget>), but it's just an odd thing to see.  It also won't prevent subclasses from executing, which may have issues (e.g. `TextResourceContentView` creates a `SourceCodeTextEditor`, which also calls `requestContent`).

It may be easier/simpler to have `requestContent`/`requestContentFromBackend` have a special case for <about:blank> (but only if the resource is a `Resource.Type.Document`).

We should also update `WI.appendContextMenuItemsForSourceCode` so that it doesn't provide a "Save File" in this case.

[Devin Rousso]

Comment on attachment 370231 [details]
Patch

r- until my comments/concerns are addressed

[Nikita Vasilyev]

Created attachment 372981 [details]
Patch

[Nikita Vasilyev]

Created attachment 372982 [details]
[Image] With patch applied

[Nikita Vasilyev]

Created attachment 372985 [details]
Patch

[Nikita Vasilyev]

(In reply to Devin Rousso from comment #5)
> It may be easier/simpler to have
> `requestContent`/`requestContentFromBackend` have a special case for
> <about:blank> (but only if the resource is a `Resource.Type.Document`).

Somehow, about:blank iframes (without srcdoc) appear as `Resource.Type.Other`.
You could see on the screenshot that it has a different icon in the navigation sidebar.

[Nikita Vasilyev]

Created attachment 372989 [details]
Patch

Whoops, this wasn't my latest patch.

[Devin Rousso]

Comment on attachment 372989 [details]
Patch

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

r-, as I don't agree with creating a special message just for <about:blank>.  We should use one of the other paths for empty resources that don't error.

> Source/WebInspectorUI/UserInterface/Models/Resource.js:825
> +                message: WI.unlocalizedString("about:blank")

Rather than pass along a special message, I think I'd rather just see either "Resource has no content" (from `ResourceContentView.prototype.showGenericNoContentMessage`) or just an empty text editor.  It seems weird to have a special case message just for <about:blank>.

> Source/WebInspectorUI/UserInterface/Views/ContextMenuUtilities.js:69
> +    if (sourceCodeOrLocation.url !== "about:blank") {

If we override the content for <about:blank> resources to be "", then we should actually be able to save those files, as there is actual content (albeit just "") to save.  As such, this restriction is not necessary.

[Nikita Vasilyev]

Created attachment 373590 [details]
Patch

[Devin Rousso]

Comment on attachment 373590 [details]
Patch

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

r-, due to the following:

> Source/WebInspectorUI/UserInterface/Views/ResourceContentView.js:139
> +            this.showGenericNoContentMessage();

After talking with @Joe, we agreed that it actually probably should just show "about:blank" as a message.  The reasoning for this is that <about:blank> is this kinda special "resource" that doesn't actually go over the network, but has this "implicit" concept of "this is an empty resource".  Along these lines, we also agreed that resources requested over the network that have no content (including <data:text/html,>) should show as an empty text editor, not "Resource has no content".  The only time that we should show that message (if ever) is if somehow we no longer have the content (e.g. the resource cache ran out of room).

Sorry for the back and forth :(

[Nikita Vasilyev]

(In reply to Devin Rousso from comment #14)
> ...
> Along these lines, we also agreed that resources requested over the network
> that have no content (including <data:text/html,>) should show as an empty
> text editor, not "Resource has no content".

I think it's fine to show an empty text editor. It especially makes sense if the resource is editable (e.g. CSS).

"Resource has no content" does however make it crystal clear that there's no content here. If I see an empty text editor, I'd have to attempt to scroll it and select all text to see if there's white space somewhere.

[Devin Rousso]

(In reply to Nikita Vasilyev from comment #15)
> (In reply to Devin Rousso from comment #14)
> > ...
> > Along these lines, we also agreed that resources requested over the network that have no content (including <data:text/html,>) should show as an empty text editor, not "Resource has no content".
> 
> I think it's fine to show an empty text editor. It especially makes sense if the resource is editable (e.g. CSS).
Agreed.  I think I actually made a change like this somewhat recently, but I can't remember 🤔

> "Resource has no content" does however make it crystal clear that there's no content here. If I see an empty text editor, I'd have to attempt to scroll it and select all text to see if there's white space somewhere.
You could also tell by looking to see if the file has any line numbers in it.

[Nikita Vasilyev]

Created attachment 373900 [details]
Patch

[Devin Rousso]

Comment on attachment 373900 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Models/Resource.js:820
>      requestContentFromBackend()

What about the other subclasses of `WI.SourceCode`?  I realize that it's probably extremely rare for someone to `<script src="about:blank"></script>`, but it's still something we should be able to handle.

[Nikita Vasilyev]

Comment on attachment 373900 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Models/Resource.js:820
>>      requestContentFromBackend()
> 
> What about the other subclasses of `WI.SourceCode`?  I realize that it's probably extremely rare for someone to `<script src="about:blank"></script>`, but it's still something we should be able to handle.

`<script src="about:blank"></script>` displays an empty text editor, same as for about:blank iframe.
I don't know why would somebody use about:blank script.

[Devin Rousso]

Comment on attachment 373900 [details]
Patch

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

>>> Source/WebInspectorUI/UserInterface/Models/Resource.js:820
>>>      requestContentFromBackend()
>> 
>> What about the other subclasses of `WI.SourceCode`?  I realize that it's probably extremely rare for someone to `<script src="about:blank"></script>`, but it's still something we should be able to handle.
> 
> `<script src="about:blank"></script>` displays an empty text editor, same as for about:blank iframe.
> I don't know why would somebody use about:blank script.

I'm suggesting that we make `<script src="about:blank"></script>` "special", just like we did for <iframe>, in that it would show a message "about:blank" instead of an empty text editor.

For resources that _do_ go over the network (e.g. not "about:blank") and have no content, we _should_ show an empty content view (which is what this patch does right now, so that's awesome =D ).

As mentioned in comment 14, "about:blank" is somewhat special, in that it doesn't go over the network and effectively _can't_ have any content whatsoever.  As such, I believe that anywhere we could show a content view for an "about:blank" resource, we should instead show an "about:blank" message, not an empty content view.

Personally, I'd go about doing this by having `WI.SourceCode` have a `specialContentForURL` method, that contains this code, and have each of the other `requestContentFromBackend` methods call it to check.

SourceCode.js
```js
    generateSpecialContentForURL(url)
    {
        if (this._url === "about:blank") {
            return Promise.resolve({
                content: "",
                message: WI.unlocalizedString("about:blank")
            });
        }
        return null;
    }
```

{subclasses of `WI.SourceCode`}.js
```js
    let specialContentForURLPromise = this.generateSpecialContentForURL(this._url);
    if (specialContentForURLPromise)
        return specialContentForURLPromise;

    ...
```

[Nikita Vasilyev]

Created attachment 374642 [details]
Patch

[Devin Rousso]

Comment on attachment 374642 [details]
Patch

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

r=me

> Source/WebInspectorUI/ChangeLog:20
> +        * UserInterface/Views/TextResourceContentView.js:

I think you mean `SourceCodeTextEditor.js`?

[Nikita Vasilyev]

(In reply to Devin Rousso from comment #22)
> Comment on attachment 374642 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=374642&action=review
> 
> r=me
> 
> > Source/WebInspectorUI/ChangeLog:20
> > +        * UserInterface/Views/TextResourceContentView.js:
> 
> I think you mean `SourceCodeTextEditor.js`?

I uploaded the old changelog file 🤦‍♂️

[Nikita Vasilyev]

Created attachment 374709 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 374709 [details]
Patch

Clearing flags on attachment: 374709

Committed r247747: <https://trac.webkit.org/changeset/247747>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.