Bug 202023 - Web Inspector: Include LocalResourceOverrides in the Open Resource dialog
Summary: Web Inspector: Include LocalResourceOverrides in the Open Resource dialog
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: Joseph Pecoraro
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-09-19 19:54 PDT by Joseph Pecoraro
Modified: 2019-09-26 19:29 PDT (History)
4 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (2.30 KB, patch)
2019-09-19 19:58 PDT, Joseph Pecoraro
joepeck: review-
Details | Formatted Diff | Diff
[IMAGE] Light Mode (1.07 MB, image/png)
2019-09-19 19:59 PDT, Joseph Pecoraro
no flags Details
[IMAGE] Dark Mode (1.06 MB, image/png)
2019-09-19 19:59 PDT, Joseph Pecoraro
no flags Details
[PATCH] Proposed Fix (4.58 KB, patch)
2019-09-20 14:26 PDT, Joseph Pecoraro
hi: review+
Details | Formatted Diff | Diff
[IMAGE] Subtitle (1.00 MB, image/png)
2019-09-20 14:27 PDT, Joseph Pecoraro
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2019-09-19 19:54:23 PDT
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.
Comment 1 Joseph Pecoraro 2019-09-19 19:58:35 PDT
Created attachment 379197 [details]
[PATCH] Proposed Fix
Comment 2 Joseph Pecoraro 2019-09-19 19:59:00 PDT
Created attachment 379198 [details]
[IMAGE] Light Mode
Comment 3 Joseph Pecoraro 2019-09-19 19:59:11 PDT
Created attachment 379199 [details]
[IMAGE] Dark Mode
Comment 4 Devin Rousso 2019-09-20 00:29:02 PDT
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" :)
Comment 5 Joseph Pecoraro 2019-09-20 13:49:38 PDT
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?
Comment 6 Joseph Pecoraro 2019-09-20 13:59:30 PDT
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.
Comment 7 Joseph Pecoraro 2019-09-20 14:26:43 PDT
Created attachment 379269 [details]
[PATCH] Proposed Fix
Comment 8 Joseph Pecoraro 2019-09-20 14:27:01 PDT
Created attachment 379270 [details]
[IMAGE] Subtitle
Comment 9 Devin Rousso 2019-09-24 13:30:45 PDT
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.
Comment 10 Joseph Pecoraro 2019-09-26 19:24:14 PDT
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.
Comment 11 Joseph Pecoraro 2019-09-26 19:28:44 PDT
https://trac.webkit.org/changeset/250407/webkit
Comment 12 Radar WebKit Bug Importer 2019-09-26 19:29:20 PDT
<rdar://problem/55767987>