Bug 217031 - Web Inspector: add checkbox to local override popover to allow it to skip the network
Summary: Web Inspector: add checkbox to local override popover to allow it to skip the...
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: Devin Rousso
URL:
Keywords: InRadar
Depends on: 207446
Blocks:
  Show dependency treegraph
 
Reported: 2020-09-27 01:36 PDT by Devin Rousso
Modified: 2020-09-28 20:50 PDT (History)
4 users (show)

See Also:


Attachments
Patch (36.99 KB, patch)
2020-09-27 01:42 PDT, Devin Rousso
bburg: review+
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (41.82 KB, patch)
2020-09-28 18:32 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2020-09-27 01:36:20 PDT
now that we have `Network.interceptRequestWithResponse`, we should be able to allow existing response overrides to skip the network if so desired
Comment 1 Devin Rousso 2020-09-27 01:42:16 PDT
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
Comment 2 Devin Rousso 2020-09-27 01:44:39 PDT
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.
Comment 3 BJ Burg 2020-09-28 09:56:36 PDT
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?
Comment 4 Devin Rousso 2020-09-28 18:31:42 PDT
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.
Comment 5 Devin Rousso 2020-09-28 18:32:00 PDT
Created attachment 409939 [details]
Patch
Comment 6 EWS 2020-09-28 20:49:37 PDT
Committed r267723: <https://trac.webkit.org/changeset/267723>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 409939 [details].
Comment 7 Radar WebKit Bug Importer 2020-09-28 20:50:20 PDT
<rdar://problem/69730820>