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.
Created attachment 379197 [details] [PATCH] Proposed Fix
Created attachment 379198 [details] [IMAGE] Light Mode
Created attachment 379199 [details] [IMAGE] Dark Mode
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 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 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.
Created attachment 379269 [details] [PATCH] Proposed Fix
Created attachment 379270 [details] [IMAGE] Subtitle
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 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.
https://trac.webkit.org/changeset/250407/webkit
<rdar://problem/55767987>