Bug 198029 - Web Inspector: Display "Resource has no content" for about:blank iframes instead of an error
Summary: Web Inspector: Display "Resource has no content" for about:blank iframes inst...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Nikita Vasilyev
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-05-19 17:55 PDT by Nikita Vasilyev
Modified: 2019-07-23 14:51 PDT (History)
4 users (show)

See Also:


Attachments
[HTML] Reduction (668 bytes, text/html)
2019-05-19 17:55 PDT, Nikita Vasilyev
no flags Details
Patch (1.64 KB, patch)
2019-05-19 18:02 PDT, Nikita Vasilyev
drousso: review-
Details | Formatted Diff | Diff
[Image] With patch applied (142.29 KB, image/png)
2019-05-19 18:03 PDT, Nikita Vasilyev
no flags Details
Patch (3.59 KB, patch)
2019-06-26 18:06 PDT, Nikita Vasilyev
no flags Details | Formatted Diff | Diff
[Image] With patch applied (137.35 KB, image/png)
2019-06-26 18:07 PDT, Nikita Vasilyev
no flags Details
Patch (3.59 KB, patch)
2019-06-26 18:15 PDT, Nikita Vasilyev
no flags Details | Formatted Diff | Diff
Patch (4.96 KB, patch)
2019-06-26 18:43 PDT, Nikita Vasilyev
drousso: review-
Details | Formatted Diff | Diff
Patch (2.46 KB, patch)
2019-07-06 18:50 PDT, Nikita Vasilyev
drousso: review-
Details | Formatted Diff | Diff
Patch (4.64 KB, patch)
2019-07-10 20:14 PDT, Nikita Vasilyev
no flags Details | Formatted Diff | Diff
Patch (7.65 KB, patch)
2019-07-22 15:26 PDT, Nikita Vasilyev
drousso: review+
Details | Formatted Diff | Diff
Patch (8.09 KB, patch)
2019-07-23 14:21 PDT, Nikita Vasilyev
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikita Vasilyev 2019-05-19 17:55:23 PDT
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>
Comment 1 Nikita Vasilyev 2019-05-19 18:02:46 PDT
Created attachment 370231 [details]
Patch
Comment 2 Nikita Vasilyev 2019-05-19 18:03:46 PDT
Created attachment 370232 [details]
[Image] With patch applied
Comment 3 Devin Rousso 2019-05-20 10:29:00 PDT
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?
Comment 4 Nikita Vasilyev 2019-06-10 16:34:54 PDT
(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…"
Comment 5 Devin Rousso 2019-06-10 23:12:25 PDT
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.
Comment 6 Devin Rousso 2019-06-19 11:13:21 PDT
Comment on attachment 370231 [details]
Patch

r- until my comments/concerns are addressed
Comment 7 Nikita Vasilyev 2019-06-26 18:06:08 PDT
Created attachment 372981 [details]
Patch
Comment 8 Nikita Vasilyev 2019-06-26 18:07:20 PDT
Created attachment 372982 [details]
[Image] With patch applied
Comment 9 Nikita Vasilyev 2019-06-26 18:15:10 PDT
Created attachment 372985 [details]
Patch
Comment 10 Nikita Vasilyev 2019-06-26 18:17:49 PDT
(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.
Comment 11 Nikita Vasilyev 2019-06-26 18:43:18 PDT
Created attachment 372989 [details]
Patch

Whoops, this wasn't my latest patch.
Comment 12 Devin Rousso 2019-07-05 12:33:27 PDT
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.
Comment 13 Nikita Vasilyev 2019-07-06 18:50:54 PDT
Created attachment 373590 [details]
Patch
Comment 14 Devin Rousso 2019-07-08 14:32:13 PDT
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 :(
Comment 15 Nikita Vasilyev 2019-07-08 16:25:07 PDT
(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.
Comment 16 Devin Rousso 2019-07-08 16:30:17 PDT
(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.
Comment 17 Nikita Vasilyev 2019-07-10 20:14:07 PDT
Created attachment 373900 [details]
Patch
Comment 18 Devin Rousso 2019-07-19 00:04:34 PDT
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.
Comment 19 Nikita Vasilyev 2019-07-19 18:05:16 PDT
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.
Comment 20 Devin Rousso 2019-07-20 13:11:15 PDT
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;

    ...
```
Comment 21 Nikita Vasilyev 2019-07-22 15:26:06 PDT
Created attachment 374642 [details]
Patch
Comment 22 Devin Rousso 2019-07-22 23:05:46 PDT
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`?
Comment 23 Nikita Vasilyev 2019-07-23 14:19:53 PDT
(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 🤦‍♂️
Comment 24 Nikita Vasilyev 2019-07-23 14:21:17 PDT
Created attachment 374709 [details]
Patch
Comment 25 WebKit Commit Bot 2019-07-23 14:51:05 PDT
Comment on attachment 374709 [details]
Patch

Clearing flags on attachment: 374709

Committed r247747: <https://trac.webkit.org/changeset/247747>
Comment 26 WebKit Commit Bot 2019-07-23 14:51:07 PDT
All reviewed patches have been landed.  Closing bug.