Bug 202375 – Web Inspector: Local Resource Overrides: allow substitution based on a url pattern

• Component: Web Inspector
• Status: RESOLVED FIXED

[Devin Rousso]

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.

[Devin Rousso]

Created attachment 382434 [details]
Patch

[EWS Watchlist]

This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.

[Devin Rousso]

Created attachment 382436 [details]
Patch

[Blaze Burg]

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 on attachment 382436 [details]
Patch

Clearing r? since there are open issues.

[Radar WebKit Bug Importer]

<rdar://problem/56963810>

[Devin Rousso]

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]

Created attachment 383198 [details]
Patch

[Blaze Burg]

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 on attachment 383198 [details]
Patch

Clearing flags on attachment: 383198

Committed r252614: <https://trac.webkit.org/changeset/252614>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.