Bug 202023 – Web Inspector: Include LocalResourceOverrides in the Open Resource dialog

• Component: Web Inspector
• Status: RESOLVED FIXED

[Joseph Pecoraro]

Include LocalResourceOverrides in the Open Resource dialog

Steps to Reproduce:
1. Open inspector
2. Include a local resource override
3. ⇧⌘O and type the name of the local override
  => Override is not in the dialog

Notes:
• Devin also pointed out that some Worker resources are not in the dialog.

[Joseph Pecoraro]

Created attachment 379197 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

Created attachment 379198 [details]
[IMAGE] Light Mode

[Joseph Pecoraro]

Created attachment 379199 [details]
[IMAGE] Dark Mode

[Devin Rousso]

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

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

> Source/WebInspectorUI/UserInterface/Views/OpenResourceDialog.js:341
> +            if (script.resource)
> +                continue;

Do you have any idea why these scripts don't show up as a resource?  That seems very weird, and probably wrong :(

> Source/WebInspectorUI/UserInterface/Views/OpenResourceDialog.js:342
> +            this._addResource(script, suppressFilterUpdate);

This is scary.  We're passing a `WI.Script` as if it was a `WI.Resource`.  Have you checked that all the various the paths that branch from this can handle that?  FWICT, it looked like it was mostly `sourceMaps` and `displayName`, but I may have missed something.  Perhaps we should rename this to `OpenSourceCodeDialog`, or at the very least rename the prototype functions within this class to match that?  Alternatively, we could throw some assertions in related constructors that the `resource` is actually a `WI.SourceCode`, but that also seems hacky (it shouldn't be called `resource` then).

> Source/WebInspectorUI/UserInterface/Views/OpenResourceDialog.js:348
> +        if (!WI.NetworkManager.supportsLocalResourceOverrides())

Theoretically, we probably don't need this check as `WI.networkManager.localResourceOverrides` should be empty if `!WI.NetworkManager.supportsLocalResourceOverrides()`, but I do like adding these "gates" :)

[Joseph Pecoraro]

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

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

>> Source/WebInspectorUI/UserInterface/Views/OpenResourceDialog.js:341
>> +                continue;
> 
> Do you have any idea why these scripts don't show up as a resource?  That seems very weird, and probably wrong :(

This was just defensive as the loop above already does this.

>> Source/WebInspectorUI/UserInterface/Views/OpenResourceDialog.js:342
>> +            this._addResource(script, suppressFilterUpdate);
> 
> This is scary.  We're passing a `WI.Script` as if it was a `WI.Resource`.  Have you checked that all the various the paths that branch from this can handle that?  FWICT, it looked like it was mostly `sourceMaps` and `displayName`, but I may have missed something.  Perhaps we should rename this to `OpenSourceCodeDialog`, or at the very least rename the prototype functions within this class to match that?  Alternatively, we could throw some assertions in related constructors that the `resource` is actually a `WI.SourceCode`, but that also seems hacky (it shouldn't be called `resource` then).

This code already handles Scripts. The loop above already handles this. The QueryController ultimately only uses `displayName`. And The OpenResourceDialog class already knows how to deal with scripts:

        function createTreeElement(representedObject)
        {
            let treeElement = null;

            if (representedObject instanceof WI.SourceMapResource)
                treeElement = new WI.SourceMapResourceTreeElement(representedObject);
            else if (representedObject instanceof WI.Resource)
                treeElement = new WI.ResourceTreeElement(representedObject);
            else if (representedObject instanceof WI.Script)
                treeElement = new WI.ScriptTreeElement(representedObject);

            return treeElement;
        }

I agree we could change all of the Resource names to SourceCode. Do you want me to do that?

[Joseph Pecoraro]

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

We have to label which is the override. Since once you load the page and a resource has been overridden, both will have the same icon and the same text. I think I'll probably including something in the subtitle.

[Joseph Pecoraro]

Created attachment 379269 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

Created attachment 379270 [details]
[IMAGE] Subtitle

[Devin Rousso]

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

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

r=me

> Source/WebInspectorUI/UserInterface/Views/OpenResourceDialog.css:170
> +    vertical-align: 1px;
> +    font-size: 11px;

NIT: I'd swap the order of these.

> Source/WebInspectorUI/UserInterface/Views/OpenResourceDialog.js:115
> +                treeElement.subtitle = WI.UIString("Local Override");

Should we assert that there isn't already a `subtitle` set?  IIRC, `WI.ResourceTreeElement` normally uses the subtitle if the resource is from a different origin than the main origin.

On a more general note, I've often wondered if "incorrect" (or unexpected) subtitle overriding is a prevalent occurrence/issue, and if it's something we could proactively counteract.  Just a thought.

[Joseph Pecoraro]

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

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

>> Source/WebInspectorUI/UserInterface/Views/OpenResourceDialog.js:115
>> +                treeElement.subtitle = WI.UIString("Local Override");
> 
> Should we assert that there isn't already a `subtitle` set?  IIRC, `WI.ResourceTreeElement` normally uses the subtitle if the resource is from a different origin than the main origin.
> 
> On a more general note, I've often wondered if "incorrect" (or unexpected) subtitle overriding is a prevalent occurrence/issue, and if it's something we could proactively counteract.  Just a thought.

I did this intentionally. I think we will want to overwrite a subtitle if it exists because "Local Override" immediately tells you all you want to know about this resource.

While it is technically possible for someone to have an override of resource "foo" on `example.com` and `example.org`, it seems unlikely. And I'd much rather show "Local Override" than the subdomain if there are multiple "foo" resources loaded.

[Joseph Pecoraro]

https://trac.webkit.org/changeset/250407/webkit

[Radar WebKit Bug Importer]

<rdar://problem/55767987>