RESOLVED FIXED 202375
Web Inspector: Local Resource Overrides: allow substitution based on a url pattern
https://bugs.webkit.org/show_bug.cgi?id=202375
Summary Web Inspector: Local Resource Overrides: allow substitution based on a url pa...
Devin Rousso
Reported 2019-09-30 15:34:57 PDT
Often, websites will load content with a timestamp-based URL query parameter for cache-busting or logging purposes. If a developer is trying to override these resources (or is trying to have an existing override also match these resources), they'd need to edit the local resource override's URL to match in addition to editing the resource that loads it (e.g. changing the <script> in an HTML file), which can sometimes be tricky of the content is dynamically loaded (e.g. an XHR with a non-hardcoded URL). We should support pattern-based local resource overrides to make this easier.
Attachments
Patch (39.01 KB, patch)
2019-10-31 00:20 PDT, Devin Rousso
no flags
Patch (39.02 KB, patch)
2019-10-31 00:27 PDT, Devin Rousso
no flags
Patch (46.53 KB, patch)
2019-11-08 19:20 PST, Devin Rousso
no flags
Devin Rousso
Comment 1 2019-10-31 00:20:04 PDT
EWS Watchlist
Comment 2 2019-10-31 00:21:04 PDT
This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.
Devin Rousso
Comment 3 2019-10-31 00:27:03 PDT
Blaze Burg
Comment 4 2019-11-01 12:01:13 PDT
Comment on attachment 382436 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=382436&action=review I have questions about fragment identifier behavior. > Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:846 > +bool InspectorNetworkAgent::hasIntercept(URL url) ::shouldIntercept ? > Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:848 > + url.removeFragmentIdentifier(); I wonder if users find it surprising that fragments are silently dropped. Are they also stripped from the displayed URL in the overrides UI? > Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1044 > + if (!m_intercepts.appendIfNotContains(Intercept { url, isRegex && *isRegex })) Nice. > Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1045 > + errorString = "Intercept for given url and given isRegex already exists"_s; "Intercept URL or pattern already registered" > Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:-1028 > -void InspectorNetworkAgent::removeInterception(ErrorString& errorString, const String& url, const String* networkStageString) Ugh, someday I'll fix the argument types here. > Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1061 > + errorString = "Missing intercept for given url and given isRegex"_s; Ditto as above. > Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:170 > + target.NetworkAgent.addInterception.invoke(commandArguments); Nice. > Source/WebInspectorUI/UserInterface/Models/LocalResourceOverride.js:49 > + url: isRegex ? url : WI.urlWithoutFragment(url), I see, we had already stripped any fragment in the provided intercept URL prior to saving it. > Source/WebInspectorUI/UserInterface/Models/LocalResourceOverride.js:70 > + return new WI.LocalResourceOverride(WI.LocalResource.fromJSON(localResource), {isRegex, disabled}); Nice, this should just fallback on older JSONs without any work. > Source/WebInspectorUI/UserInterface/Models/LocalResourceOverride.js:125 > + let regex = new RegExp(this.url, "i"); Does the user know their regex will always run case-insensitively? Maybe worth mentioning in a tips and tricks article. > Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.js:153 > + this._urlCodeMirror.setOption("mode", isRegex ? "text/x-regex" : "text/x-local-override-url"); I don't think this will work because you've captured isRegex instead of checking the value of the checkbox (either via the event or by updating the model directly). I would expect the 'change' event listener to set LocalResourceOverride.isRegex, then an event listener for WI.LocalResourceOverride.Event.IsRegexChanged will update CodeMirror mode. > Source/WebInspectorUI/UserInterface/Views/LocalResourceOverrideTreeElement.js:46 > + return "/" + this.resource.url + "/"; Nit: template literal? > LayoutTests/http/tests/inspector/network/local-resource-override-basic.html:172 > + url: "http://127.0.0.1:8000/inspector/network/resources/override.txt?t=*", so... is this a glob or a real regex? I wouldn't have expected this to work.
Blaze Burg
Comment 5 2019-11-04 15:16:02 PST
Comment on attachment 382436 [details] Patch Clearing r? since there are open issues.
Radar WebKit Bug Importer
Comment 6 2019-11-06 16:17:58 PST
Devin Rousso
Comment 7 2019-11-08 19:18:33 PST
Comment on attachment 382436 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=382436&action=review >> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:848 >> + url.removeFragmentIdentifier(); > > I wonder if users find it surprising that fragments are silently dropped. Are they also stripped from the displayed URL in the overrides UI? Yeah, we strip it in the frontend. Additionally, there's a special CodeMirror mode `text/x-local-override-url` that changes the text color of any invalid schemes and/or fragments to red so it's very visible. >> Source/WebInspectorUI/UserInterface/Models/LocalResourceOverride.js:125 >> + let regex = new RegExp(this.url, "i"); > > Does the user know their regex will always run case-insensitively? Maybe worth mentioning in a tips and tricks article. Actually, this is my mistake. We should probably provide another option to match case sensitivity, in addition to regular expression. >> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.js:153 >> + this._urlCodeMirror.setOption("mode", isRegex ? "text/x-regex" : "text/x-local-override-url"); > > I don't think this will work because you've captured isRegex instead of checking the value of the checkbox (either via the event or by updating the model directly). > > I would expect the 'change' event listener to set LocalResourceOverride.isRegex, then an event listener for WI.LocalResourceOverride.Event.IsRegexChanged will update CodeMirror mode. Good catch!
Devin Rousso
Comment 8 2019-11-08 19:20:43 PST
Blaze Burg
Comment 9 2019-11-18 15:47:28 PST
Comment on attachment 383198 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383198&action=review r=me > Source/WebInspectorUI/UserInterface/Views/LocalResourceOverrideTreeElement.js:49 > + text += "i"; Good attention to detail. Thank you!
WebKit Commit Bot
Comment 10 2019-11-18 17:30:58 PST
Comment on attachment 383198 [details] Patch Clearing flags on attachment: 383198 Committed r252614: <https://trac.webkit.org/changeset/252614>
WebKit Commit Bot
Comment 11 2019-11-18 17:31:00 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.