Bug 217032 – Web Inspector: add UI for request interception

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

[Devin Rousso]

now that we have `Network.interceptWithRequest`, we should have UI to use it

[Radar WebKit Bug Importer]

<rdar://problem/69925768>

[Devin Rousso]

Created attachment 412371 [details]
Patch

[Devin Rousso]

Created attachment 412372 [details]
[Image] after Patch is applied

[Devin Rousso]

Created attachment 412373 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 412373 [details]
Patch

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

Looks good to me!

> Source/WebInspectorUI/UserInterface/Base/HTTPUtilities.js:91
> +    Connect: "CONNECT",

Having the enum values be all caps I think would be nice, but maybe that's just me.

> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:432
> +                if (aRank < bRank)
> +                    return -1;
> +                if (bRank < aRank)
> +                    return 1;
> +                return 0;

A common way of writing this is just: `return a - b`

> Source/WebInspectorUI/UserInterface/Models/LocalResource.js:219
> +                data: this.requestData,

In the JSON description above (line 32) there should be a comma before "data". =O

> Source/WebInspectorUI/UserInterface/Views/ContentView.js:262
> -        if (representedObject instanceof WI.LocalResourceOverride)
> +        if (representedObject instanceof WI.LocalResourceOverride && representedObject.type !== WI.LocalResourceOverride.InterceptType.Request)
>              return representedObject.localResource;

This happens to be the last in the chain but perhaps if this change got longer it might make more sense to just handle this type here completely once it has been identified:

    if (representedObject instanceof WI.LocalResourceOverride) {
        if (representedObject.type !== WI.LocalResourceOverride.InterceptType.Request)
            return representedObject.localResource;
        return representedObject;
    }

> Source/WebInspectorUI/UserInterface/Views/FontResourceContentView.js:123
> +        // Request overrides cannot be created/updated from a file as files don't have network info.

Hmm, we could create a ".request" file format... but probably not worth it and certainly not worth it right now.

> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.js:300
> +            ].forEach((method) => {

Should we allow an arbitrary type instead of these fixed types? `fetch()` seems to allow setting the method string.

> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.js:525
> +            if (requestURLRow)
> +                requestURLRow.element.hidden = !isRequest;
> +            if (methodRowElement)
> +                methodRowElement.hidden = !isRequest;
> +
> +            mimeTypeRow.element.hidden = isRequest;
> +            statusCodeRow.element.hidden = isRequest;
> +            if (optionsRowElement)
> +                optionsRowElement.hidden = isRequest;

Maybe just if check all of these? It makes the code easier to read.

> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.js:531
> +                if (contentTypeDataGridNode.parent)
> +                    this._headersDataGrid.removeChild(contentTypeDataGridNode);

How about just `contentTypeDataGridNode.remove()`

> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverrideTreeElement.js:123
> +        if (WI.networkManager.localResourceOverridesForURL(serializedData.url).some((existingOverride) => existingOverride !== this._localResourceOverride && existingOverride.url === serializedData.url && existingOverride.isCaseSensitive === serializedData.isCaseSensitive && existingOverride.isRegex === serializedData.isRegex)) {

This line is hard to read.

> Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js:811
> +        if (WI.networkManager.localResourceOverridesForURL(serializedData.url).some((existingOverride) => existingOverride.url === serializedData.url && existingOverride.isCaseSensitive === serializedData.isCaseSensitive && existingOverride.isRegex === serializedData.isRegex)) {

This line is hard to read.

> LayoutTests/http/tests/inspector/network/intercept-request-properties.html:37
> +    InspectorTest.debug();

Drop this, or leaving it in for a reason?

> LayoutTests/http/tests/inspector/network/local-resource-override-basic.html:-296
> -    suite.addTestCase({
> -        name: "LocalResourceOverride.URL.Fragment",
> -        description: "LocalResourceOverride creation strips a fragment",
> -        async test() {
> -            let localResourceOverride = WI.LocalResourceOverride.create(WI.LocalResourceOverride.InterceptType.Response, {
> -                url: "http://127.0.0.1:8000/inspector/network/resources/override.txt#test",
> -                mimeType: "text/plain",
> -                content: "OVERRIDDEN TEXT",
> -                base64Encoded: false,
> -                statusCode: 200,
> -                statusText: "OK",
> -                headers: {},
> -            });
> -
> -            InspectorTest.expectEqual(localResourceOverride.localResource.url, "http://127.0.0.1:8000/inspector/network/resources/override.txt", "LocalResourceOverride creation should strip fragments.");
> -        }

Do we have an equivalent test for this now?

[Devin Rousso]

Comment on attachment 412373 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Base/HTTPUtilities.js:91
>> +    Connect: "CONNECT",
> 
> Having the enum values be all caps I think would be nice, but maybe that's just me.

sure

>> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:432
>> +                return 0;
> 
> A common way of writing this is just: `return a - b`

https://www.youtube.com/watch?v=XZxzJGgox_E

>> Source/WebInspectorUI/UserInterface/Models/LocalResource.js:219
>> +                data: this.requestData,
> 
> In the JSON description above (line 32) there should be a comma before "data". =O

lol ill fix :)

>> Source/WebInspectorUI/UserInterface/Views/ContentView.js:262
>>              return representedObject.localResource;
> 
> This happens to be the last in the chain but perhaps if this change got longer it might make more sense to just handle this type here completely once it has been identified:
> 
>     if (representedObject instanceof WI.LocalResourceOverride) {
>         if (representedObject.type !== WI.LocalResourceOverride.InterceptType.Request)
>             return representedObject.localResource;
>         return representedObject;
>     }

I was following the pattern of other `if` in this function, but I agree.  I'll change the above as well.

>> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.js:300
>> +            ].forEach((method) => {
> 
> Should we allow an arbitrary type instead of these fixed types? `fetch()` seems to allow setting the method string.

I considered doing something like this, but I felt that the complexity of adding it to this patch was already kinda overboard (not to mention I think custom methods are probably extremely rare).  I'd personally rather do it as a followup so it's easier to evaluate by itself, but I am happy to do it here if you'd prefer.

>> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.js:525
>> +                optionsRowElement.hidden = isRequest;
> 
> Maybe just if check all of these? It makes the code easier to read.

These checks only need to exist so long as we support iOS 13.4 and before.  This is kinda the same as what we do for `WI.NavigationItem` that are only added if the necessary backend feature exists too.

>> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.js:531
>> +                    this._headersDataGrid.removeChild(contentTypeDataGridNode);
> 
> How about just `contentTypeDataGridNode.remove()`

That doesn't exist actually.  These are `WI.DataGridNode` not DOM nodes 😅

>> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverrideTreeElement.js:123
>> +        if (WI.networkManager.localResourceOverridesForURL(serializedData.url).some((existingOverride) => existingOverride !== this._localResourceOverride && existingOverride.url === serializedData.url && existingOverride.isCaseSensitive === serializedData.isCaseSensitive && existingOverride.isRegex === serializedData.isRegex)) {
> 
> This line is hard to read.

I can split this out into a helper.

>> Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js:811
>> +        if (WI.networkManager.localResourceOverridesForURL(serializedData.url).some((existingOverride) => existingOverride.url === serializedData.url && existingOverride.isCaseSensitive === serializedData.isCaseSensitive && existingOverride.isRegex === serializedData.isRegex)) {
> 
> This line is hard to read.

ditto

>> LayoutTests/http/tests/inspector/network/intercept-request-properties.html:37
>> +    InspectorTest.debug();
> 
> Drop this, or leaving it in for a reason?

oops!

>> LayoutTests/http/tests/inspector/network/local-resource-override-basic.html:-296
>> -        }
> 
> Do we have an equivalent test for this now?

fragment stripping is handled by the UI code now, as it doesn't make sense to do with regex overrides

[Devin Rousso]

Created attachment 413135 [details]
Patch

[Devin Rousso]

Created attachment 413136 [details]
Patch

oops forgot the new files/tests

[Blaze Burg]

Comment on attachment 413136 [details]
Patch

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

r=me with some wholesome comments, great work!

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:1031
> +localizedStrings["Redirect"] = "Redirect";

Most of these string additions need a better UIString description.

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:1083
> +/* Text indicating that the local override will skip all network activity and instead immediately serve the response. */

I bet this is going to be a heavy lift to localize. Thanks for adding more context!

> Source/WebInspectorUI/UserInterface/Base/ReferencePage.js:30
> +    LocalOverrides: "local-overrides",

Nice.

> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:214
> +    get localResourceOverrides() { return this._localResourceOverrides; }

This should return a copy to retain the same behavior.

Is there a more modern way to do this besides Array.prototype.splice()?

> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:382
> +        console.assert(!this._localResourceOverrides.includes(localResourceOverride), "Already had an existing local resource override.");

Please update the assert message ("local override already added"). It's now possible for >1 override per resource so the current message no longer describes an error condition.

> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:417
> +        const rankFunctions = [

A short comment would be helpful for the uninitiated reader.

"Order local resource overrides that match the URL using multiple ranking functions.
An override that matches an earlier ranking function will be ordered before an override
that matches a later ranking function. Overrides with the same rank are in insertion order."

Related question: is it intended for local resource overrides to maintain insertion order when finding a match? That's what I would expect intuitively as a user. I'm not sure that the code currently guarantees this, as overrides with equal rank may not necessarily retain their insertion order during the sorting algorithm.

> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:428
> +                return aRank - bRank;

If aRank == bRank, then sort by insertion order. I don't think the order is saved anywhere in this patch.

> Source/WebInspectorUI/UserInterface/Models/LocalResourceOverride.js:85
> +        console.assert(false, type);

Nit: 'Unexpected type:', type

> Source/WebInspectorUI/UserInterface/Models/LocalResourceOverride.js:163
> +            displayName = WI.UIString("%s (Case Insensitive)").format(displayName);

This UIString needs more context ("label for a case-insensitive URL match pattern in a local resource override").

> Source/WebInspectorUI/UserInterface/Models/LocalResourceOverride.js:184
> +        if (localResourceOverrideOrSerializedData === this)

Huh?

> Source/WebInspectorUI/UserInterface/Models/Resource.js:188
> +        let shouldBeOverridden = resource.isLoading() && WI.networkManager.localResourceOverridesForURL(resource.url).some((localResourceOverride) => !localResourceOverride.disabled);

Nice.

> Source/WebInspectorUI/UserInterface/Views/ContentView.js:102
> +            if (representedObject.type === WI.LocalResourceOverride.InterceptType.Request)

I'd prefer a switch since this is an enum, but this works too.

> Source/WebInspectorUI/UserInterface/Views/ContextMenuUtilities.js:87
> +                        initiatorHint: WI.TabBrowser.TabNavigationInitiator.ContextMenu,

Nice, thanks for adding this.

> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverrideLabelView.js:45
> +        labelElement.textContent = this._localResourceOverride.type === WI.LocalResourceOverride.InterceptType.Request ? WI.UIString("Request Override") : WI.UIString("Response Override");

Might want to add more UIString context. Where is this View in the UI?

> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.css:68
> +.popover .local-resource-override-popover-content:not(.request) .editor.url {

Nit: it would be clearer to use .response instead of :not(.request).

> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.js:60
> +            isCaseSensitive: !this._isCaseSensitiveCheckbox || this._isCaseSensitiveCheckbox.checked,

This looks wrong without reading the full context. In what case is this._isCaseSensitiveCheckbox not assigned?

It would be nicer to use this.supportsCaseSensitive or something to represent if this is user-modifiable for a given local override. I had to go look up all the assignments to understand that this._isCaseSensitiveCheckbox is indeed tied to a backend feature check.

> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.js:96
> +            // NOTE: We can allow an empty mimeType / statusCode / statusText to pass

I prefer to call this inherits/inherited fields to "pass through".

EDIT: Ah this is just code moving around. Nevermind.

> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.js:97
> +            // network values through, but lets require them for overrides so that

Nit: lets

> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.js:105
> +            if (isNaN(data.responseStatusCode))

This could be a followup, but it would be nice to have some visual feedback if the inputted values are not valid (HTTP 666 for example).

> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.js:210
> +

Nit: extra newlines

> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.js:281
> +            let methodLabelElement = methodHeaderElement.appendChild(document.createElement("label"));

Ditto (:210)

> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.js:319
> +        let statusCodeRow = createRow(WI.UIString("Status"), "status", valueData.statusCode || "", placeholderData.statusCode);

This UIString needs some love. It's not obvious that this is a label for an HTTP status code in the local overrides popover. I'd add a new string with a different key.

> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverrideRequestContentView.js:56
> +        saveData.suggestedName = WI.UIString("%s Request Data").format(this.representedObject.url) + ".txt";

Needs a better UIString indicating that this is part of a suggested file name.

[Devin Rousso]

Comment on attachment 413136 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:214
>> +    get localResourceOverrides() { return this._localResourceOverrides; }
> 
> This should return a copy to retain the same behavior.
> 
> Is there a more modern way to do this besides Array.prototype.splice()?

This is actually only used in two places, both of which are just iterated over, so I think it's fine to keep it as-is.  FWIW the only reason why we had to create a copy before was because it used to be a `Set` (although frankly even then it could've also just been exposed, but possibly not as easily for any clients that might've wanted to do a `filter` or something).  It's fairly common for us to expose the actual data structure instead of a copy (e.g. almost any usage of `WI.Collection` and its subclasses).

Most clients use `localResourceOverridesForURL` instead, which is a subset-array.

>> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:382
>> +        console.assert(!this._localResourceOverrides.includes(localResourceOverride), "Already had an existing local resource override.");
> 
> Please update the assert message ("local override already added"). It's now possible for >1 override per resource so the current message no longer describes an error condition.

oh good catch

>> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:428
>> +                return aRank - bRank;
> 
> If aRank == bRank, then sort by insertion order. I don't think the order is saved anywhere in this patch.

I believe `Array.prototype.sort` is a stable sort, so this shouldn't be a problem.

e.g.
```
    [{x: 1, y: 2}, {x: 1, y: 3}, {x: 1, y: 4}, {x: 1, y: 5}].sort((a, b) => a.x - b.x)
```

>> Source/WebInspectorUI/UserInterface/Models/LocalResourceOverride.js:184
>> +        if (localResourceOverrideOrSerializedData === this)
> 
> Huh?

Ok yeah I should move this to the one cases where it's necessary in `WI.LocalResourceOverrideTreeElement.prototype.willDismissPopover`.  The idea is that we want to check if the edits that've just been made to the selected local override clash with any _other_ existing local overrides.

Looking at those callsites again, I think they're actually wrong because `WI.networkManager.localResourceOverridesForURL(serializedData.url)` won't actually get you anything useful if `serializedData.isRegex`.  Instead we should `WI.networkManager.localResourceOverrides.some((existingOverride) => existingOverride !== this._localResourceOverride && existingOverride.equals(this._localResourceOverride))`.

>> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.css:68
>> +.popover .local-resource-override-popover-content:not(.request) .editor.url {
> 
> Nit: it would be clearer to use .response instead of :not(.request).

we actually don't have a `.response`, but i can add it

>> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.js:60
>> +            isCaseSensitive: !this._isCaseSensitiveCheckbox || this._isCaseSensitiveCheckbox.checked,
> 
> This looks wrong without reading the full context. In what case is this._isCaseSensitiveCheckbox not assigned?
> 
> It would be nicer to use this.supportsCaseSensitive or something to represent if this is user-modifiable for a given local override. I had to go look up all the assignments to understand that this._isCaseSensitiveCheckbox is indeed tied to a backend feature check.

FWIW this logic was basically already here, just lower down.  I'll add a compatibility comment.

>> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.js:210
>> +
> 
> Nit: extra newlines

I prefer to have newlines here so that it's clear what logic is done for each element.

>> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.js:281
>> +            let methodLabelElement = methodHeaderElement.appendChild(document.createElement("label"));
> 
> Ditto (:210)

ditto

[Devin Rousso]

Created attachment 415788 [details]
Patch

[EWS]

Committed r270604: <https://trac.webkit.org/changeset/270604>

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