Bug 217031 – Web Inspector: add checkbox to local override popover to allow it to skip the network

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Devin Rousso]

now that we have `Network.interceptRequestWithResponse`, we should be able to allow existing response overrides to skip the network if so desired

[Devin Rousso]

Created attachment 409822 [details]
Patch

this could probably have some special UI treatment (i.e. maybe use `StepOver.svg` inside a box for "skip network" and then have up/down arrows inside a box for request/response), but i imagine that much of that will only really be needed once a UI exists for request interception

[Devin Rousso]

Comment on attachment 409822 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:939
> +        if (window.InspectorTest) {
> +            // FIXME: <https://webkit.org/b/217032> Web Inspector: add UI for request interception
> +            this.dispatchEventToListeners(WI.NetworkManager.Event.RequestIntercepted, {target, requestId, request});
> +            return;
> +        }

I am planning on migrating all of the request interception tests to use `WI.LocalResourceOverride` in bug 217032.  I didn't do that in this patch because that's likely going to be a more involved effort and `Network.interceptRequestWithResponse` uses the same data as the existing `Network.interceptWithResponse`, so there's very little new here.

[Blaze Burg]

Comment on attachment 409822 [details]
Patch

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

r=me, looks good overall.

I prefer 'WI.LocalResourceOverride.Type.InterceptRequest' to InterceptRequestWithResponse. I don't think the 'WithResponse' adds anything.

Also, please investigate mac-wk2 EWS failure.

> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:961
> +            base64Encoded: revision.base64Encoded,

Nit: !! would make it more clear this is a bool.

> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:1479
> +        if (localResourceOverride.disabled)

Soo nice.

> Source/WebInspectorUI/UserInterface/Models/LocalResourceOverride.js:75
> +        // COMPATIBILITY (iOS 13.4): Network.interceptWithRequest/Network.interceptRequestWithResponse did not exist yet.

Have you tested this patch when inspecting iOS 13.4?

[Devin Rousso]

Comment on attachment 409822 [details]
Patch

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

(In reply to Brian Burg from comment #3)
> I prefer 'WI.LocalResourceOverride.Type.InterceptRequest' to InterceptRequestWithResponse. I don't think the 'WithResponse' adds anything.
So actually, there's a difference between `InterceptRequest` (which I plan to add in bug 217032) and `InterceptRequestWithResponse`.  The former is/(will be) used to replace the request part of network activity matching the URL with the data as configured in the UI using `Network.interceptWithRequest`.  The latter is specifically about intercepting requests matching the URL, stopping them, and then simulating a response with the data as configured in the UI using `Network.interceptRequestWithResponse`, thereby "skipping" the network entirely (i.e. nothing leaves the computer, the server doesn't know about it, etc.).  This patch only adds support for the latter, as there are very few changes that need to be made to the UI (or really any of the frontend logic) because it uses the same data as `Network.interceptWithResponse` meaning that it can share the same UI as response overrides.

I'll rename this to `WI.LocalResourceOverride.InterceptType.ResponseSkippingNetwork` for clarity :)

> Also, please investigate mac-wk2 EWS failure.
this was me forgetting to update `WI.LocalResourceOverride.create` in LayoutTests 🤦‍♂️

>> Source/WebInspectorUI/UserInterface/Models/LocalResourceOverride.js:75
>> +        // COMPATIBILITY (iOS 13.4): Network.interceptWithRequest/Network.interceptRequestWithResponse did not exist yet.
> 
> Have you tested this patch when inspecting iOS 13.4?

Kinda?  This is actually a compatibility for inspecting macOS too if the user has a local override and then updates Safari.  I didn't directly test inspecting iOS13.4 but I had an existing local override before I wrote this patch that was "upgraded" without issue.

[Devin Rousso]

Created attachment 409939 [details]
Patch

[EWS]

Committed r267723: <https://trac.webkit.org/changeset/267723>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 409939 [details].

[Radar WebKit Bug Importer]

<rdar://problem/69730820>