Bug 207446 – Web Inspector: request interception

• Component: Web Inspector
• Status: RESOLVED FIXED

[Pavel Feldman]

Local overrides look exciting!

As I understand, the corresponding request is still made though and I was wondering if there is any interest in intercepting requests along with the responses. That's what we typically do in automation - you often need to mock services, ads and third party content and there is no way you can send those requests over the network.

Here is what I've done for Playwright: https://github.com/pavelfeldman/webkit/commit/7a22960724114e2ba1d850515b1cd7360c524ca2 and I'd be happy to see it upstream in a shape and form that makes sense to you. The changes themselves are very local as seen in the patch, note that I've followed up and polished it in some subsequent patches. It should still give you a great idea on the changes necessary.

Here is what happened to the protocol. If you are interested in seeing it upstream, that's what we would need to discuss first. Looking forward to your response!

Network domain commands:
{
    "name": "setInterceptionEnabled",
    "description": "Enable interception of network requests.",
    "parameters": [
        { "name": "enabled", "type": "boolean" },
        { "name": "interceptRequests", "type": "boolean", "optional": true }
    ]
},
{
    "name": "interceptContinue",
    "description": "Continue an interception with no modifications.",
    "parameters": [
        { "name": "requestId", "$ref": "RequestId", "description": "Identifier for the intercepted Network request or response to continue." },
        { "name": "method", "type": "string", "optional": true,"description": "HTTP request method to use for this request" },
        { "name": "headers", "$ref": "Headers", "optional": true, "description": "HTTP response headers to use instead of the original ones for request" },
        { "name": "postData", "type": "string", "optional": true, "description": "HTTP post data to use instead of the original ones for request" }
    ]
},
{
    "name": "interceptAsError",
    "description": "Abort the intercepted request with a given reason.",
    "parameters": [
        { "name": "requestId", "$ref": "RequestId", "description": "Identifier for the intercepted Network request." },
        { "name": "reason", "type": "string", "description": "Deliver error reason for the request." }
    ]
},
{
    "name": "interceptWithResponse",
    "description": "Provide response content for an intercepted request without making the network call.",
    "parameters": [
        { "name": "requestId", "$ref": "RequestId", "description": "Identifier for the intercepted Network request to modify." },
        { "name": "content", "type": "string", "optional": true },
        { "name": "base64Encoded", "type": "boolean", "optional": true, "description": "True, if content was sent as base64." },
        { "name": "mimeType", "type": "string", "optional": true, "description": "MIME Type for the data." },
        { "name": "status", "type": "integer", "optional": true, "description": "HTTP response status code. Pass through original values if unmodified." },
        { "name": "statusText", "type": "string", "optional": true, "description": "HTTP response status text. Pass through original values if unmodified." },
        { "name": "headers", "$ref": "Headers", "optional": true, "description": "HTTP response headers. Pass through original values if unmodified." }
    ]
}

Network domain events:

{
    "name": "requestIntercepted",
    "description": "Fired when HTTP request has been intercepted. The frontend must response with <code>Network.interceptWithError</code>, <code>Network.interceptContinue</code> or <code>Network.interceptWithRespons</code>` to proceed with this response.",
    "parameters": [
        { "name": "requestId", "$ref": "RequestId", "description": "Identifier for this intercepted network. Corresponds with an earlier <code>Network.requestWillBeSent</code>." },
        { "name": "request", "$ref": "Request", "description": "Original request content that would proceed if this is continued as is." }
    ]
}

[Devin Rousso]

We already have some of those commands (namely `Network.setInterceptionEnabled` and `Network.interceptWithResponse`).  Our initial design was to add a `"request"` value to `Network.NetworkStage` and likely a `Network.requestIntercepted` event and `Network.interceptWithRequest` command.

Suffice to say, yes we're interested in adding request interception :)

For more info about our current functionality/capabilities, you can read <https://webkit.org/web-inspector/local-overrides/>.

[Pavel Feldman]

(In reply to Devin Rousso from comment #1)
> We already have some of those commands (namely
> `Network.setInterceptionEnabled` and `Network.interceptWithResponse`).

I understand this and the fragment of the protocol above is on top of what you have. For example, it adds an 'interceptRequests' bit to the existing Network.setInterceptionEnabled. And it reuses the same Network.interceptWithResponse when client wants to fulfill request with the response.

>  Our initial design was to add a `"request"` value to `Network.NetworkStage` and
> likely a `Network.requestIntercepted` event

Yes, Network.requestIntercepted is exactly what I added in our fork.

> and `Network.interceptWithRequest` command.

This is something we called `Network.interceptContinue` in the protocol snippet. It allows augmenting method, headers and post data, but does not allow changing the target URL. Doing that would require serious surgery in the CachedResource, etc.

> Suffice to say, yes we're interested in adding request interception :)
> 

Sounds perfect.

> For more info about our current functionality/capabilities, you can read
> <https://webkit.org/web-inspector/local-overrides/>.

I surely did. If you are Ok roughly with the protocol changes outlined in my comment (and it sounds like you are), I am happy to attach the patch.

[Pavel Feldman]

Created attachment 392092 [details]
Patch

[EWS Watchlist]

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

[Pavel Feldman]

Created attachment 392099 [details]
Patch

[Devin Rousso]

Comment on attachment 392099 [details]
Patch

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

Overall, I like the direction that this is going :)

I didn't dive too much into the code, as most of my comments are about the design.

As far as the protocol, I'd prefer if we kept request interception and response interception sort of distinct from each other (e.g. separate `interceptWithResponse` and `interceptWithRequest`) for the reasons that I've outlined.

As far as the implementation, I'd really like to mimic the existing logic/flow of response interception, as I think that code was really clean and was minimally invasive to any of the existing code. as well as conforming to many existing patterns throughout WebCore/WebKit.

> Source/JavaScriptCore/inspector/protocol/Network.json:241
> +                { "name": "url", "type": "string", "optional": true, "description": "URL pattern to intercept, intercept everything if not specified or empty" },

This seems extremely wasteful and not desired, as now every single network request/response will need to be delayed until the frontend has had a chance to decide what to do, which is most of the time going to be "continue with no changes".

> Source/JavaScriptCore/inspector/protocol/Network.json:259
> +            "description": "Continue an interception with or without modifications.",

I don't like the idea of allowing modifications inside this command.  If modifications are desired, they should be done inside `interceptWithResponse` (or `interceptWithRequest`).  Our design is that the request/response should either be completely untouched or completely replaced.  We'd like to keep it that way.

> Source/JavaScriptCore/inspector/protocol/Network.json:268
> +            "name": "interceptAsError",

Can't this already be achieved with `interceptWIthResponse`?  Why does it need a separate command?

> Source/JavaScriptCore/inspector/protocol/Network.json:364
> +            "description": "Fired when HTTP request has been intercepted. The frontend must respond with <code>Network.interceptAsError</code>, <code>Network.interceptContinue</code> or <code>Network.interceptWithResponse</code>` to resolve this request.",

I'd prefer to have a separate `interceptWIthRequest`, as we will likely have a different set of things that can be modified (e.g. `method` vs `status`).

> Source/WebCore/inspector/InspectorInstrumentation.cpp:1293
> +    if (InspectorNetworkAgent* networkAgent = instrumentingAgents.inspectorNetworkAgent())

NIT: `auto`?

> Source/WebCore/inspector/InspectorInstrumentation.h:321
> +    static bool interceptRequest(ResourceLoader&, Function<void(bool handled)>&&);

Please move this to be right above `interceptResponse` (also move it in the .cpp).

> Source/WebCore/inspector/InspectorInstrumentation.h:523
> +    static bool interceptRequestImpl(InstrumentingAgents&, ResourceLoader&, Function<void(bool handled)>&&);

Ditto (321)

> Source/WebCore/inspector/InspectorInstrumentation.h:1688
> +    if (InstrumentingAgents* instrumentingAgents = instrumentingAgentsForFrame(loader.frame()))

NIT: `auto`?

> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:237
> +            && InspectorInstrumentationWebKit::interceptRequest(resourceLoader, [this, protectedResourceLoader, trackingParameters, shouldClearReferrerOnHTTPSToHTTPRedirect, resource](bool handled) mutable {

Style: I don't think we like having a lambda inside an `if`.

> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:238
> +                if (!handled)

I don't really like this `handled` approach.  I'd prefer if you mimicked the logic of `interceptResponse` (e.g. a `CompletionHandler<const ResourceRequest&>` that replaces the `protectedResourceLoader->request()` but still calls `scheduleLoadFromNetworkProcess`).  I don't think we want request interception to completely ignore this logic.

[Pavel Feldman]

Comment on attachment 392099 [details]
Patch

Thank you for the review!

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

>> Source/JavaScriptCore/inspector/protocol/Network.json:241
>> +                { "name": "url", "type": "string", "optional": true, "description": "URL pattern to intercept, intercept everything if not specified or empty" },
> 
> This seems extremely wasteful and not desired, as now every single network request/response will need to be delayed until the frontend has had a chance to decide what to do, which is most of the time going to be "continue with no changes".

That would only happen when you decide to specify no URL at all. I can leave it as required, no problem. But in reality, all the automation frameworks will be setting ".*" regex and we will be running JSC's regex engine for no good reason.

>> Source/JavaScriptCore/inspector/protocol/Network.json:259
>> +            "description": "Continue an interception with or without modifications.",
> 
> I don't like the idea of allowing modifications inside this command.  If modifications are desired, they should be done inside `interceptWithResponse` (or `interceptWithRequest`).  Our design is that the request/response should either be completely untouched or completely replaced.  We'd like to keep it that way.

I like the idea of "completely the same or completely replaced". But I don't think this can be (easily) achieved due to the CachedResource / ResourceRequest coupling. Do you have an idea on where this can be intercepted without swapping the CachedResource::m_resourceRequest?

>> Source/JavaScriptCore/inspector/protocol/Network.json:268
>> +            "name": "interceptAsError",
> 
> Can't this already be achieved with `interceptWIthResponse`?  Why does it need a separate command?

This can be done. But it would make every parameter of that method optional and will lead to a lot of run-time checks (error-prone).

>> Source/JavaScriptCore/inspector/protocol/Network.json:364
>> +            "description": "Fired when HTTP request has been intercepted. The frontend must respond with <code>Network.interceptAsError</code>, <code>Network.interceptContinue</code> or <code>Network.interceptWithResponse</code>` to resolve this request.",
> 
> I'd prefer to have a separate `interceptWIthRequest`, as we will likely have a different set of things that can be modified (e.g. `method` vs `status`).

Do you mean `interceptContinue` is used for response and a new `interceptWithRequest` is used to set the method? That sounds good.

>> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:237
>> +            && InspectorInstrumentationWebKit::interceptRequest(resourceLoader, [this, protectedResourceLoader, trackingParameters, shouldClearReferrerOnHTTPSToHTTPRedirect, resource](bool handled) mutable {
> 
> Style: I don't think we like having a lambda inside an `if`.

Do you suggest splitting this code into bool foo = and if (foo) or do you want to see ::shouldInterceptRequest here?

>> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:238
>> +                if (!handled)
> 
> I don't really like this `handled` approach.  I'd prefer if you mimicked the logic of `interceptResponse` (e.g. a `CompletionHandler<const ResourceRequest&>` that replaces the `protectedResourceLoader->request()` but still calls `scheduleLoadFromNetworkProcess`).  I don't think we want request interception to completely ignore this logic.

This all boils down to the feasibility of request swap.

[Devin Rousso]

Comment on attachment 392099 [details]
Patch

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

>>> Source/JavaScriptCore/inspector/protocol/Network.json:259
>>> +            "description": "Continue an interception with or without modifications.",
>> 
>> I don't like the idea of allowing modifications inside this command.  If modifications are desired, they should be done inside `interceptWithResponse` (or `interceptWithRequest`).  Our design is that the request/response should either be completely untouched or completely replaced.  We'd like to keep it that way.
> 
> I like the idea of "completely the same or completely replaced". But I don't think this can be (easily) achieved due to the CachedResource / ResourceRequest coupling. Do you have an idea on where this can be intercepted without swapping the CachedResource::m_resourceRequest?

I don't understand what you mean by "CachedResource / ResourceRequest coupling".  What I described is how response interception works right now.  In `InspectorNetworkAgent::interceptWithResponse`, we create our own `ResourceResponse` and set the data as required.  Why is `CachedResource` involved at all?

>>> Source/JavaScriptCore/inspector/protocol/Network.json:268
>>> +            "name": "interceptAsError",
>> 
>> Can't this already be achieved with `interceptWIthResponse`?  Why does it need a separate command?
> 
> This can be done. But it would make every parameter of that method optional and will lead to a lot of run-time checks (error-prone).

All of the parameters for `Network.interceptWithResponse` are already optional.  I'd rather not have unnecessary commands.

>>> Source/JavaScriptCore/inspector/protocol/Network.json:364
>>> +            "description": "Fired when HTTP request has been intercepted. The frontend must respond with <code>Network.interceptAsError</code>, <code>Network.interceptContinue</code> or <code>Network.interceptWithResponse</code>` to resolve this request.",
>> 
>> I'd prefer to have a separate `interceptWIthRequest`, as we will likely have a different set of things that can be modified (e.g. `method` vs `status`).
> 
> Do you mean `interceptContinue` is used for response and a new `interceptWithRequest` is used to set the method? That sounds good.

Here's what we had in mind originally:

Commands:
 - `Network.interceptContinue` is the way that the frontend tells the backend to let an intercepted request/response proceed unmodified
 - `Network.interceptWithRequest` is the way for the frontend to entirely replace request data for an intercepted request
 - `Network.interceptWithResponse` is the way for the frontend to entirely replace response data for an intercepted response

Events:
 - `Network.requestIntercepted` is how the frontend is notified when a request is intercepted
 - `Network.responseIntercepted` is how the frontend is notified when a response is intercepted

This way, the paths for request and response interception are almost entirely separate (the only exceptions being the `Network.setInterceptionEnabled`/`Network.addInterception`/`Network.removeInterception` commands and `Network.interceptContinue`, since none of those actually modify any request/response).

[Pavel Feldman]

Created attachment 399242 [details]
Patch

[Pavel Feldman]

Comment on attachment 392099 [details]
Patch

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

>>> Source/JavaScriptCore/inspector/protocol/Network.json:241
>>> +                { "name": "url", "type": "string", "optional": true, "description": "URL pattern to intercept, intercept everything if not specified or empty" },
>> 
>> This seems extremely wasteful and not desired, as now every single network request/response will need to be delayed until the frontend has had a chance to decide what to do, which is most of the time going to be "continue with no changes".
> 
> That would only happen when you decide to specify no URL at all. I can leave it as required, no problem. But in reality, all the automation frameworks will be setting ".*" regex and we will be running JSC's regex engine for no good reason.

Done.

>>>> Source/JavaScriptCore/inspector/protocol/Network.json:259
>>>> +            "description": "Continue an interception with or without modifications.",
>>> 
>>> I don't like the idea of allowing modifications inside this command.  If modifications are desired, they should be done inside `interceptWithResponse` (or `interceptWithRequest`).  Our design is that the request/response should either be completely untouched or completely replaced.  We'd like to keep it that way.
>> 
>> I like the idea of "completely the same or completely replaced". But I don't think this can be (easily) achieved due to the CachedResource / ResourceRequest coupling. Do you have an idea on where this can be intercepted without swapping the CachedResource::m_resourceRequest?
> 
> I don't understand what you mean by "CachedResource / ResourceRequest coupling".  What I described is how response interception works right now.  In `InspectorNetworkAgent::interceptWithResponse`, we create our own `ResourceResponse` and set the data as required.  Why is `CachedResource` involved at all?

Done.

>>>> Source/JavaScriptCore/inspector/protocol/Network.json:268
>>>> +            "name": "interceptAsError",
>>> 
>>> Can't this already be achieved with `interceptWIthResponse`?  Why does it need a separate command?
>> 
>> This can be done. But it would make every parameter of that method optional and will lead to a lot of run-time checks (error-prone).
> 
> All of the parameters for `Network.interceptWithResponse` are already optional.  I'd rather not have unnecessary commands.

Done.

>>>> Source/JavaScriptCore/inspector/protocol/Network.json:364
>>>> +            "description": "Fired when HTTP request has been intercepted. The frontend must respond with <code>Network.interceptAsError</code>, <code>Network.interceptContinue</code> or <code>Network.interceptWithResponse</code>` to resolve this request.",
>>> 
>>> I'd prefer to have a separate `interceptWIthRequest`, as we will likely have a different set of things that can be modified (e.g. `method` vs `status`).
>> 
>> Do you mean `interceptContinue` is used for response and a new `interceptWithRequest` is used to set the method? That sounds good.
> 
> Here's what we had in mind originally:
> 
> Commands:
>  - `Network.interceptContinue` is the way that the frontend tells the backend to let an intercepted request/response proceed unmodified
>  - `Network.interceptWithRequest` is the way for the frontend to entirely replace request data for an intercepted request
>  - `Network.interceptWithResponse` is the way for the frontend to entirely replace response data for an intercepted response
> 
> Events:
>  - `Network.requestIntercepted` is how the frontend is notified when a request is intercepted
>  - `Network.responseIntercepted` is how the frontend is notified when a response is intercepted
> 
> This way, the paths for request and response interception are almost entirely separate (the only exceptions being the `Network.setInterceptionEnabled`/`Network.addInterception`/`Network.removeInterception` commands and `Network.interceptContinue`, since none of those actually modify any request/response).

Done with the modification: user is still able to issue `Network.interceptWithResponse` upon `Network.requestIntercepted`. This is the single most useful scenario and I don't think the contract that you suggest above supports it. We need a mode where front-end simply fulfills the request and there is no actual network activity going on.

>> Source/WebCore/inspector/InspectorInstrumentation.cpp:1293
>> +    if (InspectorNetworkAgent* networkAgent = instrumentingAgents.inspectorNetworkAgent())
> 
> NIT: `auto`?

Done

>> Source/WebCore/inspector/InspectorInstrumentation.h:321
>> +    static bool interceptRequest(ResourceLoader&, Function<void(bool handled)>&&);
> 
> Please move this to be right above `interceptResponse` (also move it in the .cpp).

Done

>> Source/WebCore/inspector/InspectorInstrumentation.h:523
>> +    static bool interceptRequestImpl(InstrumentingAgents&, ResourceLoader&, Function<void(bool handled)>&&);
> 
> Ditto (321)

Done

>> Source/WebCore/inspector/InspectorInstrumentation.h:1688
>> +    if (InstrumentingAgents* instrumentingAgents = instrumentingAgentsForFrame(loader.frame()))
> 
> NIT: `auto`?

Done

>>> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:237
>>> +            && InspectorInstrumentationWebKit::interceptRequest(resourceLoader, [this, protectedResourceLoader, trackingParameters, shouldClearReferrerOnHTTPSToHTTPRedirect, resource](bool handled) mutable {
>> 
>> Style: I don't think we like having a lambda inside an `if`.
> 
> Do you suggest splitting this code into bool foo = and if (foo) or do you want to see ::shouldInterceptRequest here?

Done.

>>> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:238
>>> +                if (!handled)
>> 
>> I don't really like this `handled` approach.  I'd prefer if you mimicked the logic of `interceptResponse` (e.g. a `CompletionHandler<const ResourceRequest&>` that replaces the `protectedResourceLoader->request()` but still calls `scheduleLoadFromNetworkProcess`).  I don't think we want request interception to completely ignore this logic.
> 
> This all boils down to the feasibility of request swap.

I still have this implicit handled flag due to ability to fulfill request right away.

[Pavel Feldman]

Created attachment 399243 [details]
Patch

[Devin Rousso]

Comment on attachment 392099 [details]
Patch

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

>>>>> Source/JavaScriptCore/inspector/protocol/Network.json:364
>>>>> +            "description": "Fired when HTTP request has been intercepted. The frontend must respond with <code>Network.interceptAsError</code>, <code>Network.interceptContinue</code> or <code>Network.interceptWithResponse</code>` to resolve this request.",
>>>> 
>>>> I'd prefer to have a separate `interceptWIthRequest`, as we will likely have a different set of things that can be modified (e.g. `method` vs `status`).
>>> 
>>> Do you mean `interceptContinue` is used for response and a new `interceptWithRequest` is used to set the method? That sounds good.
>> 
>> Here's what we had in mind originally:
>> 
>> Commands:
>>  - `Network.interceptContinue` is the way that the frontend tells the backend to let an intercepted request/response proceed unmodified
>>  - `Network.interceptWithRequest` is the way for the frontend to entirely replace request data for an intercepted request
>>  - `Network.interceptWithResponse` is the way for the frontend to entirely replace response data for an intercepted response
>> 
>> Events:
>>  - `Network.requestIntercepted` is how the frontend is notified when a request is intercepted
>>  - `Network.responseIntercepted` is how the frontend is notified when a response is intercepted
>> 
>> This way, the paths for request and response interception are almost entirely separate (the only exceptions being the `Network.setInterceptionEnabled`/`Network.addInterception`/`Network.removeInterception` commands and `Network.interceptContinue`, since none of those actually modify any request/response).
> 
> Done with the modification: user is still able to issue `Network.interceptWithResponse` upon `Network.requestIntercepted`. This is the single most useful scenario and I don't think the contract that you suggest above supports it. We need a mode where front-end simply fulfills the request and there is no actual network activity going on.

Your modification doesn't make sense to me.  Having a request be intercepted with a response seems wrong.  If you plan on overriding the response, then why does it matter whether or not the request goes out over the network?  The request could completely fail and it wouldn't matter because the response would be entirely replaced with the pre-configured one.

>>>> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:238
>>>> +                if (!handled)
>>> 
>>> I don't really like this `handled` approach.  I'd prefer if you mimicked the logic of `interceptResponse` (e.g. a `CompletionHandler<const ResourceRequest&>` that replaces the `protectedResourceLoader->request()` but still calls `scheduleLoadFromNetworkProcess`).  I don't think we want request interception to completely ignore this logic.
>> 
>> This all boils down to the feasibility of request swap.
> 
> I still have this implicit handled flag due to ability to fulfill request right away.

Can't that be done by having a `CompletionHandler` be invoked immediately?  FWIW, i would expect that if this code is reached, Web Inspector is guaranteed to replace the request as otherwise the `InspectorInstrumentation` check was wrong (which is bad).

[Pavel Feldman]

Created attachment 399304 [details]
Patch

[Pavel Feldman]

Created attachment 399305 [details]
Patch

[Devin Rousso]

Comment on attachment 399243 [details]
Patch

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

r-, this needs a _lot_ more tests
 - intercept/override URL defined in HTML
 - intercept/override URL defined in CSS
 - test `method` parameter of `interceptWithRequest`
 - test `headers` parameter of `interceptWithRequest`
 - test `postData` parameter of `interceptWithRequest`
there are a _lot_ of different situations/configurations for request interception, and we should have at least one test per.

> Source/JavaScriptCore/inspector/protocol/Network.json:247
> +                { "name": "url", "type": "string", "description": "URL pattern to intercept, intercept everything if not specified or empty" },

I am still not comfortable with this "intercept everything" concept.  I don't think that there's a good way to make a UI for this.  Network interception should be a whitelist not a let-me-decide catch-all.

> Source/JavaScriptCore/inspector/protocol/Network.json:276
> +                { "name": "url", "type": "string", "optional": true,"description": "HTTP request url." },

I'm not sure I like this idea.  This is really more of a redirect rather than an override.

> Source/JavaScriptCore/inspector/protocol/Network.json:278
> +                { "name": "postData", "type": "string", "optional": true, "description": "HTTP POST request data, base64-encoded." }

Why is this _always_ base64-encoded?  That should probably be another parameter, assuming it even needs to exist at all.

> Source/JavaScriptCore/inspector/protocol/Network.json:292
> +                { "name": "errorType", "$ref": "ResourceErrorType", "optional": true, "description": "Deliver error reason for the request failure." }

Where did this come from?  How is it related to _request_ interception?  It seems like you're trying to expose a way to modify internal state/concepts via the inspector protocol, which is not really something I think Web Inspector wants to do.

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1054
> +void InspectorNetworkAgent::addInterception(ErrorString& errorString, const String& url, const bool* optionalCaseSensitive, const bool* optionalIsRegex, const String* optionalNetworkStageString)

The `NetworkStage` actually probably shouldn't be optional.  What would not specifying a `NetworkStage` even mean?  The only thing that makes sense to me would be for not specifying a `NetworkStage` to mean "intercept at every point", but for reasons i've mentioned i don't see why that's needed/wanted.

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1125
> +    String requestId = IdentifiersFactory::requestId(loader.identifier());

We should double-check that the `requestId` isn't already in `m_pendingInterceptRequests`, and if so invoke the `handler` and return.

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1126
> +    auto pendingRequest = makeUnique<PendingInterceptRequest>(&loader, WTFMove(handler));

You could just inline this and avoid the `WTFMove(pendingRequest)`.

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1148
>  void InspectorNetworkAgent::interceptContinue(ErrorString& errorString, const String& requestId)

This should also probably take a `NetworkStage` so that we know whether we're trying to continue the request or the response (also see :1054).

Alternatively, we could add an `InterceptId` that makes this even more clear, but I think the combo of `RequestId` + `NetworkStage` is probably unique enough.

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1167
> +    auto pendingRequest = m_pendingInterceptRequests.take(requestId);

I think this should really be a completely separate command so as to not conflate things, like `interceptRequestWithImmediateResponse`.

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1169
> +        ResourceLoader* loader = pendingRequest->m_loader.get();

`auto`?

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1175
> +        ResourceRequest request = loader->request();

Ditto (:1169)

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1182
> +            for (auto& header : *headers) {

NIT: you could use structured binding
```
    for (auto& [key, value] : *headers)
```

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1194
> +            }

Style: missing newline after block containing `return`

(rest of file too)

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1195
> +            }
> +            Ref<FormData> data = FormData::create(buffer);

Ditto (:1169), or could even just inline it to avoid the `WTFMove(data)`.

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1208
> +    if (pendingRequest && errorTypeString) {

all of these `if`s could be nested under a shared `if (pendingRequest)`

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1252
> +        if (base64Encoded && *base64Encoded && content) {

NIT: you could next these two `if`s under a shared `if (content && !content->isEmpty())`

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1263
> +        ResourceResponse response(pendingRequest->m_loader->url(), *mimeType, data->size(), String());

`data` is not guaranteed to exist

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1275
> +        loader->didReceiveResponse(response, [loader, data = data.releaseNonNull()]() mutable {

Replacing something with its own name is not the easiest to read.  Also, `data` is not guaranteed to exist, which `RefPtr::releaseNonNull` expects.  It should probably either just be a regular `&data` capture or a `buffer = WTFMove(data)`.

Why does this need to be `mutable`?

> Source/WebCore/inspector/agents/InspectorNetworkAgent.h:226
> +        NetworkStage networkStage { NetworkStage::Response };

Ditto (InspectorNetworkAgent.cpp:1054)

> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:240
> +        RefPtr<ResourceLoader> protectedResourceLoader = &resourceLoader;

This is normally done as part of the lambda capture.
```
    [protectedResourceLoader = makeRef(resourceLoader)]
```

> LayoutTests/inspector/network/intercept-request.html:10
> +        const response = await fetch('resources/data.json');

Style: we only use `const` for values that never change between executions (i.e. an actual constant), which is not the case here, so please use `let`

(rest of file too)

> LayoutTests/inspector/network/intercept-request.html:16
> +            header: response.headers.get('my-header')

Style: missing trailing comma

(rest of file too)

> LayoutTests/inspector/network/intercept-request.html:23
> +async function test()

Please don't make the `test` function `async`.  The test harness doesn't really support `async` functions.  Our usual pattern is to set up the entire test suite and then put the `runTestCasesAndFinish` call in the callback.

> LayoutTests/inspector/network/intercept-request.html:25
> +    let suite = ProtocolTest.createAsyncSuite("Network.setInterceptionEnabled");

Oops?

> LayoutTests/inspector/network/intercept-request.html:27
> +    const [, , {result}] = await Promise.all([

Even if it's not used, I'd rather give a name to each item so that I don't have to read the code to figure out what is being "skipped"

(rest of file too)

> LayoutTests/inspector/network/intercept-request.html:43
> +        name: "requestIntercepted.interceptContinue",

NIT: I like to prefix each test case name with the test suite's name so that the relationship is clear

> LayoutTests/inspector/network/intercept-request.html:48
> +                InspectorProtocol.awaitCommand({method: "Runtime.evaluate", params: {

This is definitely not our style.  Usually we only inline objects if there are only a few simple values (e.g. numbers, strings, etc.).  For something this complex, please put things onto separate lines.

(rest of file too)

> LayoutTests/inspector/network/intercept-request.html:55
> +            const [ fetchResult ] = await Promise.all([

Style: no space after `[` and before `]`

> LayoutTests/inspector/network/intercept-request.html:64
> +            ProtocolTest.pass("Response data: '" + fetchResult.result.value.text.trim() + "'");

We shouldn't be using `ProtocolTest.pass` for unknown values, since if this code breaks somehow, we'd now be printing `PASS: ` with some _other_ value, which could seriously confuse bot watchers or future developers as to what the right value is supposed to be.

I'd either use `ProtocolText.expectEqual` and compare against some known/expected value or just use `ProtocolTest.log`.

(rest of file too)

> LayoutTests/inspector/network/intercept-request.html:155
> +        name: "tearDown",

This shouldn't be necessary, as when the `InspectorNetworkAgent` is destroyed (when Web Inspector closes/disconnects) we should automatically continue any pending intercepted activity.

> LayoutTests/inspector/network/intercept-request.html:163
> +    function printFetchResult(result)

Please put this function above it's usage.  I realize that this is perfectly valid JavaScript, but we prefer to declare things before they're used for readability.

[Pavel Feldman]

Created attachment 399457 [details]
Patch

[Pavel Feldman]

Comment on attachment 399243 [details]
Patch

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

Thanks for the review! I'll add as many tests as necessary once we agree on the protocol. Addressed most of the comments, commented on a couple.

>> Source/JavaScriptCore/inspector/protocol/Network.json:247
>> +                { "name": "url", "type": "string", "description": "URL pattern to intercept, intercept everything if not specified or empty" },
> 
> I am still not comfortable with this "intercept everything" concept.  I don't think that there's a good way to make a UI for this.  Network interception should be a whitelist not a let-me-decide catch-all.

I would encourage you to think about this as of Network.setExtraHTTPHeaders on steroids. You can augment any request or response with a cookie, you don't need to know the URL for that.

>> Source/JavaScriptCore/inspector/protocol/Network.json:276
>> +                { "name": "url", "type": "string", "optional": true,"description": "HTTP request url." },
> 
> I'm not sure I like this idea.  This is really more of a redirect rather than an override.

I was following your lead here. You told me to replace one request with another entirely and request has a URL! Strictly speaking, I can imagine a programmatic proxy that would use it. But I can remove it altogether.

>> Source/JavaScriptCore/inspector/protocol/Network.json:278
>> +                { "name": "postData", "type": "string", "optional": true, "description": "HTTP POST request data, base64-encoded." }
> 
> Why is this _always_ base64-encoded?  That should probably be another parameter, assuming it even needs to exist at all.

POST data is rare and is often binary. Why splitting the code flow into two paths? That would mean another optional postDataBase64...

>> Source/JavaScriptCore/inspector/protocol/Network.json:292
>> +                { "name": "errorType", "$ref": "ResourceErrorType", "optional": true, "description": "Deliver error reason for the request failure." }
> 
> Where did this come from?  How is it related to _request_ interception?  It seems like you're trying to expose a way to modify internal state/concepts via the inspector protocol, which is not really something I think Web Inspector wants to do.

You asked me to merge `interceptAsError` into this method in the previous review. One of the outcomes of the request / response is a network error, it accounts for it.

>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1054
>> +void InspectorNetworkAgent::addInterception(ErrorString& errorString, const String& url, const bool* optionalCaseSensitive, const bool* optionalIsRegex, const String* optionalNetworkStageString)
> 
> The `NetworkStage` actually probably shouldn't be optional.  What would not specifying a `NetworkStage` even mean?  The only thing that makes sense to me would be for not specifying a `NetworkStage` to mean "intercept at every point", but for reasons i've mentioned i don't see why that's needed/wanted.

Done. Existing docs say "If not present this applies to all network stages.", but I agree it should be required. I made it such. Do I need to do anything for backwards compat here?

>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1125
>> +    String requestId = IdentifiersFactory::requestId(loader.identifier());
> 
> We should double-check that the `requestId` isn't already in `m_pendingInterceptRequests`, and if so invoke the `handler` and return.

Done.

>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1126
>> +    auto pendingRequest = makeUnique<PendingInterceptRequest>(&loader, WTFMove(handler));
> 
> You could just inline this and avoid the `WTFMove(pendingRequest)`.

Done

>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1148
>>  void InspectorNetworkAgent::interceptContinue(ErrorString& errorString, const String& requestId)
> 
> This should also probably take a `NetworkStage` so that we know whether we're trying to continue the request or the response (also see :1054).
> 
> Alternatively, we could add an `InterceptId` that makes this even more clear, but I think the combo of `RequestId` + `NetworkStage` is probably unique enough.

Done. Added NetworkStage. Anything I need to do for backwards compat?

>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1167
>> +    auto pendingRequest = m_pendingInterceptRequests.take(requestId);
> 
> I think this should really be a completely separate command so as to not conflate things, like `interceptRequestWithImmediateResponse`.

We got ourselves into a tough spot here. Earlier, you made me add `errorType` into `interceptResponse` instead of a standalone `interceptAsError`. So now I won't be able to make parameters of the new `interceptRequestWithImmediateResponse` required. That means that we will end up with two methods with identical signatures (8 optional parameters each) that have the same semantics, but carry a slightly different name. To me that makes things more confusing than having a single method that does what it says. Wdyt?

>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1169
>> +        ResourceLoader* loader = pendingRequest->m_loader.get();
> 
> `auto`?

I would appreciate some style room here unless it is explicitly against the guidelines. Is there a strong recommendation to pick auto* over popular WebCore types? It reads better with the type for me...

>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1175
>> +        ResourceRequest request = loader->request();
> 
> Ditto (:1169)

Ditto!

>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1182
>> +            for (auto& header : *headers) {
> 
> NIT: you could use structured binding
> ```
>     for (auto& [key, value] : *headers)
> ```

Done

>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1194
>> +            }
> 
> Style: missing newline after block containing `return`
> 
> (rest of file too)

Is this a new code style guideline? I can't find it, while I can find the opposite: https://webkit.org/code-style-guidelines/#linebreaking-else-if. What did I miss? I can see a lot of code that does what you say, but I see even more code that doesn't ...

>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1195
>> +            Ref<FormData> data = FormData::create(buffer);
> 
> Ditto (:1169), or could even just inline it to avoid the `WTFMove(data)`.

Done

>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1208
>> +    if (pendingRequest && errorTypeString) {
> 
> all of these `if`s could be nested under a shared `if (pendingRequest)`

Would you be comfortable with me keeping it this way? Having a giant if is hard to read - you need to go all the way up to figure out the root guard. Local guards are easy to follow and show you that the variable is guarded.

>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1252
>> +        if (base64Encoded && *base64Encoded && content) {
> 
> NIT: you could next these two `if`s under a shared `if (content && !content->isEmpty())`

Done.

>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1263
>> +        ResourceResponse response(pendingRequest->m_loader->url(), *mimeType, data->size(), String());
> 
> `data` is not guaranteed to exist

Good catch! I created an empty buffer in case no content to make the rest of the code still read nice.

>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1275
>> +        loader->didReceiveResponse(response, [loader, data = data.releaseNonNull()]() mutable {
> 
> Replacing something with its own name is not the easiest to read.  Also, `data` is not guaranteed to exist, which `RefPtr::releaseNonNull` expects.  It should probably either just be a regular `&data` capture or a `buffer = WTFMove(data)`.
> 
> Why does this need to be `mutable`?

Done. Needs to be mutable, otherwise everything is const ref and we are passing the buffer further.

>> Source/WebCore/inspector/agents/InspectorNetworkAgent.h:226
>> +        NetworkStage networkStage { NetworkStage::Response };
> 
> Ditto (InspectorNetworkAgent.cpp:1054)

Should be ok to default to this, I anyways assign it.

>> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:240
>> +        RefPtr<ResourceLoader> protectedResourceLoader = &resourceLoader;
> 
> This is normally done as part of the lambda capture.
> ```
>     [protectedResourceLoader = makeRef(resourceLoader)]
> ```

Done. You folks have really wide screens. Now 245 columns!

>> LayoutTests/inspector/network/intercept-request.html:10
>> +        const response = await fetch('resources/data.json');
> 
> Style: we only use `const` for values that never change between executions (i.e. an actual constant), which is not the case here, so please use `let`
> 
> (rest of file too)

Done.

>> LayoutTests/inspector/network/intercept-request.html:16
>> +            header: response.headers.get('my-header')
> 
> Style: missing trailing comma
> 
> (rest of file too)

Done.

>> LayoutTests/inspector/network/intercept-request.html:23
>> +async function test()
> 
> Please don't make the `test` function `async`.  The test harness doesn't really support `async` functions.  Our usual pattern is to set up the entire test suite and then put the `runTestCasesAndFinish` call in the callback.

Done.

>> LayoutTests/inspector/network/intercept-request.html:25
>> +    let suite = ProtocolTest.createAsyncSuite("Network.setInterceptionEnabled");
> 
> Oops?

I am not sure what the guidelines for this naming are. Will "Network request interception" work?

>> LayoutTests/inspector/network/intercept-request.html:27
>> +    const [, , {result}] = await Promise.all([
> 
> Even if it's not used, I'd rather give a name to each item so that I don't have to read the code to figure out what is being "skipped"
> 
> (rest of file too)

Done

>> LayoutTests/inspector/network/intercept-request.html:43
>> +        name: "requestIntercepted.interceptContinue",
> 
> NIT: I like to prefix each test case name with the test suite's name so that the relationship is clear

Done

>> LayoutTests/inspector/network/intercept-request.html:48
>> +                InspectorProtocol.awaitCommand({method: "Runtime.evaluate", params: {
> 
> This is definitely not our style.  Usually we only inline objects if there are only a few simple values (e.g. numbers, strings, etc.).  For something this complex, please put things onto separate lines.
> 
> (rest of file too)

Any way I can convince you that inline is better here? Extracting a variable here means that I need to create all params before the Promise.all call. In some cases that is more than a dozen of lines. Reading it gets really tough.

>> LayoutTests/inspector/network/intercept-request.html:55
>> +            const [ fetchResult ] = await Promise.all([
> 
> Style: no space after `[` and before `]`

Done

>> LayoutTests/inspector/network/intercept-request.html:64
>> +            ProtocolTest.pass("Response data: '" + fetchResult.result.value.text.trim() + "'");
> 
> We shouldn't be using `ProtocolTest.pass` for unknown values, since if this code breaks somehow, we'd now be printing `PASS: ` with some _other_ value, which could seriously confuse bot watchers or future developers as to what the right value is supposed to be.
> 
> I'd either use `ProtocolText.expectEqual` and compare against some known/expected value or just use `ProtocolTest.log`.
> 
> (rest of file too)

Done

>> LayoutTests/inspector/network/intercept-request.html:155
>> +        name: "tearDown",
> 
> This shouldn't be necessary, as when the `InspectorNetworkAgent` is destroyed (when Web Inspector closes/disconnects) we should automatically continue any pending intercepted activity.

Done

>> LayoutTests/inspector/network/intercept-request.html:163
>> +    function printFetchResult(result)
> 
> Please put this function above it's usage.  I realize that this is perfectly valid JavaScript, but we prefer to declare things before they're used for readability.

Done

[Pavel Feldman]

Created attachment 399499 [details]
Patch

[Pavel Feldman]

Created attachment 400431 [details]
Patch

[Pavel Feldman]

Created attachment 400507 [details]
Patch

[Pavel Feldman]

Created attachment 400510 [details]
Patch

[Devin Rousso]

Comment on attachment 399243 [details]
Patch

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

>>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1054
>>> +void InspectorNetworkAgent::addInterception(ErrorString& errorString, const String& url, const bool* optionalCaseSensitive, const bool* optionalIsRegex, const String* optionalNetworkStageString)
>> 
>> The `NetworkStage` actually probably shouldn't be optional.  What would not specifying a `NetworkStage` even mean?  The only thing that makes sense to me would be for not specifying a `NetworkStage` to mean "intercept at every point", but for reasons i've mentioned i don't see why that's needed/wanted.
> 
> Done. Existing docs say "If not present this applies to all network stages.", but I agree it should be required. I made it such. Do I need to do anything for backwards compat here?

I don't think there's anything that needs to be done for compat :)

>>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1148
>>>  void InspectorNetworkAgent::interceptContinue(ErrorString& errorString, const String& requestId)
>> 
>> This should also probably take a `NetworkStage` so that we know whether we're trying to continue the request or the response (also see :1054).
>> 
>> Alternatively, we could add an `InterceptId` that makes this even more clear, but I think the combo of `RequestId` + `NetworkStage` is probably unique enough.
> 
> Done. Added NetworkStage. Anything I need to do for backwards compat?

I think the only compat change needed is in `WI.NetworkManager.prototype.responseIntercepted`:
```
    target.NetworkAgent.interceptContinue.invoke({
        requestId,
        networkStage: InspectorBackend.Enum.Network.NetworkStage.Response,
    });
```

>>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1194
>>> +            }
>> 
>> Style: missing newline after block containing `return`
>> 
>> (rest of file too)
> 
> Is this a new code style guideline? I can't find it, while I can find the opposite: https://webkit.org/code-style-guidelines/#linebreaking-else-if. What did I miss? I can see a lot of code that does what you say, but I see even more code that doesn't ...

This is something I personally _strongly_ prefer, as it makes it very clear that subsequent `if` are not related.  I also find that the `return` sticks out more if there's a newline.

I usually use sequential `if` _without_ newlines when they're all related (see `InspectorDOMAgent::buildObjectForAccessibilityProperties` as an example).

>>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1208
>>> +    if (pendingRequest && errorTypeString) {
>> 
>> all of these `if`s could be nested under a shared `if (pendingRequest)`
> 
> Would you be comfortable with me keeping it this way? Having a giant if is hard to read - you need to go all the way up to figure out the root guard. Local guards are easy to follow and show you that the variable is guarded.

I'd prefer it under a shared `if` because then we can avoid having so many repeated checks.  I also originally requested this so that the logic is more clearly separated into two paths: "here's the request path" and "the rest assumes/expects a response".

>>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1275
>>> +        loader->didReceiveResponse(response, [loader, data = data.releaseNonNull()]() mutable {
>> 
>> Replacing something with its own name is not the easiest to read.  Also, `data` is not guaranteed to exist, which `RefPtr::releaseNonNull` expects.  It should probably either just be a regular `&data` capture or a `buffer = WTFMove(data)`.
>> 
>> Why does this need to be `mutable`?
> 
> Done. Needs to be mutable, otherwise everything is const ref and we are passing the buffer further.

Why is everything const ref?  You can add `&` before `loader` to make it a ref instead of a copy, but I don't think that will make it const.

>>> LayoutTests/inspector/network/intercept-request.html:25
>>> +    let suite = ProtocolTest.createAsyncSuite("Network.setInterceptionEnabled");
>> 
>> Oops?
> 
> I am not sure what the guidelines for this naming are. Will "Network request interception" work?

My approach has been to make the suite name (and file name) match the protocol domain/command/event as much as possible, and to use a description otherwise.  We also like to prefix each test case with the name of the suite so the context is carried forward.

In this case, I'd probably use something like `Network.RequestInterception` and then rename the test cases to `Network.RequestInterception.interceptContinue`.

> LayoutTests/inspector/network/intercept-request.html:31
> +            expression: "document.URL"

NIT: I forgot to mention this earlier, but one thing we like to do is use backticks for strings that will be evaluated in the page.  Up to you whether you want to or not, it's just something we do :)

>>> LayoutTests/inspector/network/intercept-request.html:48
>>> +                InspectorProtocol.awaitCommand({method: "Runtime.evaluate", params: {
>> 
>> This is definitely not our style.  Usually we only inline objects if there are only a few simple values (e.g. numbers, strings, etc.).  For something this complex, please put things onto separate lines.
>> 
>> (rest of file too)
> 
> Any way I can convince you that inline is better here? Extracting a variable here means that I need to create all params before the Promise.all call. In some cases that is more than a dozen of lines. Reading it gets really tough.

Oh sorry I wasn't asking you to define stuff outside of this call.  Inlining things is fine.  This is more what I was suggesting:
```
    InspectorProtocol.awaitCommand({
        method: "Runtime.evaluate",
        params: {
            expression: `fetchData()`,
        },
    });
```
When we write object/array literals, we usually either inline the entire thing or put each item on a separate line (with some exceptions here and there if sub-objects/sub-arrays are simple).

[Devin Rousso]

Comment on attachment 399243 [details]
Patch

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

>>> Source/JavaScriptCore/inspector/protocol/Network.json:278
>>> +                { "name": "postData", "type": "string", "optional": true, "description": "HTTP POST request data, base64-encoded." }
>> 
>> Why is this _always_ base64-encoded?  That should probably be another parameter, assuming it even needs to exist at all.
> 
> POST data is rare and is often binary. Why splitting the code flow into two paths? That would mean another optional postDataBase64...

I'm not sure I agree with the statement "POST data is rare and is often binary".  As an example, I believe basic `<form>` POST data is not binary.  This is a common pattern in the Web Inspector protocol, so I would find it weird to not see it here too.

Also, `Network.Request` uses a plain non-base64-encoded string for `postData` right now, so either that's incorrect and we should change it or this `postData` should be made non-base-64-encoded to match.

[Devin Rousso]

Comment on attachment 400510 [details]
Patch

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

r-, as i think we still have untested paths, but this is looking good :)
 - for every optional parameter, we should have a test where it exists and where it doesn't (these can be batched for multiple optional parameters at once)
 - when a request is cancelled
 - invoking the command with a `requestId` that doesn't exist or is for the wrong stage
I'd also recommend additionally copying/mimicking the local overrides tests (both what they do and how many there are) as they're pretty comprehensive.

> Source/JavaScriptCore/inspector/protocol/Network.json:288
> +                { "name": "content", "type": "string", "optional": true },
> +                { "name": "base64Encoded", "type": "boolean", "optional": true, "description": "True, if content was sent as base64." },

this needs tests

> Source/JavaScriptCore/inspector/protocol/Network.json:297
> +            "description": "Provide response content for an intercepted response.",

This should explain more about what this means, namely that it'll completely skip the network.

> Source/JavaScriptCore/inspector/protocol/Network.json:305
> +                { "name": "mimeType", "type": "string", "description": "MIME Type for the data." },
> +                { "name": "status", "type": "integer", "description": "HTTP response status code. Pass through original values if unmodified." },
> +                { "name": "statusText", "type": "string", "description": "HTTP response status text. Pass through original values if unmodified." },
> +                { "name": "headers", "$ref": "Headers", "description": "HTTP response headers. Pass through original values if unmodified." }

These should all probably be optional.

> Source/JavaScriptCore/inspector/protocol/Network.json:310
> +            "description": "Fail request or response with given error type.",

Can this also be used when a response is intercepted, or just for requests?  If the former, we should change the name.  If the latter, we should change the description.

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:878
> +        ResourceLoader* loader = pendingRequest->m_loader.get();
> +        if (loader->identifier())
> +            pendingRequest->m_completionHandler(&loader->request());

Can we create a `PendingInterceptRequest::respondWithOriginalRequest` so this common logic (also seen in `InspectorNetworkAgent::interceptContinue`) isn't repeated?

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1066
> +    auto networkStage = Inspector::Protocol::InspectorHelpers::parseEnumValueFromString<Inspector::Protocol::Network::NetworkStage>(networkStageString);
> +    if (!networkStage) {
> +        errorString = makeString("Unknown networkStage: "_s, networkStageString);
> +        return;
> +    }

Can we leave this part up top?  That way, if we do fail to parse it happens before we create any objects or do any other logic.

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1070
>          errorString = "Intercept for given url and given isRegex already exists"_s;

NIT: this should also mention "given stage"

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1085
> +    auto networkStage = Inspector::Protocol::InspectorHelpers::parseEnumValueFromString<Inspector::Protocol::Network::NetworkStage>(networkStageString);
> +    if (!networkStage) {
> +        errorString = makeString("Unknown networkStage: "_s, networkStageString);
> +        return;
> +    }

Ditto (:1062)

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1089
>          errorString = "Missing intercept for given url and given isRegex"_s;

ditto (:1070)

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1097
> +    return shouldIntercept(request.url(), NetworkStage::Request) || shouldIntercept(request.url(), NetworkStage::Response);

NIT: if you wanted, you could put this on a separate line so it lines up for readability:
```
    return shouldIntercept(request.url(), NetworkStage::Request)
        || shouldIntercept(request.url(), NetworkStage::Response);
```

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1155
> +    if (networkStage.value() == NetworkStage::Request) {

NIT: even though there are only two values, i'd make this into a `switch` so that the paths for each `NetworkStage` value are completely clear:
```
    switch (networkStage.value()) {
    case NetworkStage::Request:
        if (auto pendingInterceptRequest = m_pendingInterceptRequests.take(requestId)) {
            pendingInterceptRequest->respondWithOriginalRequest();
        else
            errorString = "Missing pending intercept request for given requestId"_s;
        return;

    case NetworkStage::Response:
        if (auto pendingInterceptResponse = m_pendingInterceptResponses.take(requestId)) {
            pendingInterceptResponse->respondWithOriginalResponse();
        else
            errorString = "Missing pending intercept response for given requestId"_s;
        return;
    }

    ASSERT_NOT_REACHED();
```

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1161
> +        errorString = "Missing pending intercept request for given requestId"_s;

we should have a test for this path

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1178
> +        if (!loader->identifier()) {

Can we create a `bool ResourceLoader::wasCancelled() const` or something like that so it's more explicit, instead of inferring based on whether the `identifier` is set or not (which seems like an internal implementation detail of `ResourceLoader`)?

Alternatively, could we check `ResourceLoader::reachedTerminalState` instead?

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1182
> +        // Safe to const cast at this point, we are only adjusting the method / headers / post.

this comment doesn't seem accurate/necessary

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1259
> +        errorString = "Missing pending intercept response for given requestId"_s;

s/response/request

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1265
> +        errorString = "Unable to fulfill request, it has already been processed"_s;

we should have a test for this path

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1291
> +    RefPtr<SharedBuffer> data;
> +    if (base64Encoded && *base64Encoded) {
> +        Vector<uint8_t> buffer;
> +        if (!base64Decode(content, buffer)) {
> +            errorString = "Unable to decode given content"_s;
> +            return;
> +        }
> +        data = SharedBuffer::create(WTFMove(buffer));
> +    } else
> +        data = SharedBuffer::create(content.utf8().data(), content.utf8().length());
> +
> +    // Mimic data URL load behavior - report didReceiveResponse & didFinishLoading.
> +    ResourceResponse response(pendingRequest->m_loader->url(), mimeType, data->size(), String());
> +    response.setSource(ResourceResponse::Source::InspectorOverride);
> +    response.setHTTPStatusCode(status);
> +    response.setHTTPStatusText(statusText);
> +    HTTPHeaderMap explicitHeaders;
> +    for (auto& header : headers) {
> +        String headerValue;
> +        if (header.value->asString(headerValue))
> +            explicitHeaders.add(header.key, headerValue);
> +    }
> +    response.setHTTPHeaderFields(WTFMove(explicitHeaders));
> +    response.setHTTPHeaderField(HTTPHeaderName::ContentType, response.mimeType());

Perhaps we can create a new private method `InspectorNetworkAgent::createInterceptResponse` so that we can share much of this logic between `InspectorNetworkAgent::interceptWithResponse` and `InspectorNetworkAgent::interceptRequestWithResponse`.  To be clear, I still think we should have separate commands (and I understand that we're basically duplicating variables), but I don't think that means we can't share some logic between them :)

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1304
> +        errorString = "Missing pending intercept response for given requestId"_s;

Ditto (:1259)

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1310
> +        errorString = "Unable to abort request, it has already been processed"_s;

we should have a test for this path

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1316
> +        errorString = "Unknown error type"_s;

NIT: `makeString("Unknown errorType: "_s, errorTypeString)`

> Source/WebCore/inspector/agents/InspectorNetworkAgent.h:167
> +        PendingInterceptRequest(ResourceLoader* loader, CompletionHandler<void(const ResourceRequest*)>&& completionHandler)

NIT: can we pass `ResourceLoader&` and do `&loader` here instead?  That way, we know with certainty that when we create this object the `loader` exists (e.g. a caller can't pass `nullptr`).

> Source/WebCore/inspector/agents/InspectorNetworkAgent.h:173
> +        RefPtr<ResourceLoader> m_loader;

Do we need a `RefPtr`?  Is it possible for the `ResourceLoader` to go away while Web Inspector is deciding what to do?

> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:164
> -                            networkStage: InspectorBackend.Enum.Network.NetworkStage.Response,
> +                            stage: InspectorBackend.Enum.Network.NetworkStage.Response,

Oh lol oops :P

Also, can we reorder these so that they match the ordering in the protocol?

> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:911
> +        this.dispatchEventToListeners(WI.NetworkManager.Event.RequestIntercepted, {target, requestId, request});

This could have a FIXME with a bug link for adding request interception support to the frontend :)

> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:919
> +            target.NetworkAgent.interceptContinue(requestId, InspectorBackend.Enum.Network.NetworkStage.Response);

I don't think this will be backwards compatible, as older backends will expect the last argument to be a callback, not a `Network.NetworkStage`.  I'd recommend using `invoke` as that handles all that for you :)
```
    target.NetworkAgent.interceptContinue.invoke({
        requestId,
        networkStage: InspectorBackend.Enum.Network.NetworkStage.Response,
    });
```

> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:241
> +            if (request)

Can we make this ` const ResourceRequest&` instead?  I feel like if Web Inspector is going to pass a `nullptr`, it just shouldn't even invoke this `CompletionHandler` (which I believe would have the same result as passing `nullptr`).

> LayoutTests/http/tests/inspector/network/intercept-request-main-resource-with-response.html:9
> +    let suite = InspectorTest.createAsyncSuite("Network.interceptRequest");

This should probably be `Network.interceptRequestWithResponse` as that's really what you're testing.

> LayoutTests/http/tests/inspector/network/intercept-request-main-resource-with-response.html:29
> +                    headers: {},

we should test if this isn't included

> LayoutTests/http/tests/inspector/network/intercept-request-main-resource.html:9
> +    let suite = InspectorTest.createAsyncSuite("Network.interceptRequest");

This should probably be `Network.interceptWithRequest` as that's really what you're testing.

> LayoutTests/http/tests/inspector/network/intercept-request-properties.html:27
> +    let suite = InspectorTest.createAsyncSuite("Network.interceptRequest");

This should probably be `Network.interceptWithRequest` as that's really what you're testing.

> LayoutTests/http/tests/inspector/network/intercept-request-properties.html:52
> +            let data = eval('(' + response.result.value + ')');

Why not `JSON.parse`?

> LayoutTests/http/tests/inspector/network/intercept-request-properties.html:80
> +            let data = eval('(' + response.result.value + ')');

Ditto (:52)

> LayoutTests/http/tests/inspector/network/intercept-request-subresource-with-error.html:9
> +    let suite = InspectorTest.createAsyncSuite("Network.interceptRequest");

This should probably be `Network.interceptRequestWithError` as that's really what you're testing.

> LayoutTests/http/tests/inspector/network/intercept-request-subresource-with-error.html:25
> +                    errorType: "General",

this should use the enum `InspectorBackend.Network.ResourceErrorType.General`

> LayoutTests/http/tests/inspector/network/intercept-request-subresource-with-response.html:9
> +    let suite = InspectorTest.createAsyncSuite("Network.interceptRequest");

This should probably be `Network.interceptRequestWithResponse` as that's really what you're testing.

> LayoutTests/http/tests/inspector/network/intercept-request-subresource.html:15
> +					  let target = WI.assumingMainTarget()

indentation Oops?

> LayoutTests/http/tests/inspector/network/resources/intercept-echo.php:15
> +echo "{\n";
> +echo "  \"method\": \"$method\",\n";
> +echo "  \"headers\": {\n";
> +foreach (getallheaders() as $name => $value) {
> +	echo "    \"$name\": \"$value\",\n";
> +}
> +echo "  },\n";
> +echo "  \"post\": {\n";
> +foreach ($_POST as $name => $value){
> +	echo "    \"$name\": \"$value\",\n";
> +}
> +echo "  },\n";
> +echo "}\n";

Do we have to use `echo`?
```
    {
        "method": "<?php echo $_SERVER['REQUEST_METHOD']; ?>",
        "headers": {
    <?php foreach (getallheaders() as $name => $value) { ?>
            "<?php echo $name; ?>": "<?php echo $value; ?>",
    <?php } ?>
        },
        "post": {
    <?php foreach ($_POST as $name => $value) { ?>
            "<?php echo $name; ?>": "<?php echo $value; ?>",
    <?php } ?>
        },
    }
```

> LayoutTests/http/tests/inspector/network/resources/intercept-request-overriden-page.html:4
> +				<script src="../resources/inspector-test.js">

Why is this included in this file?

> LayoutTests/http/tests/inspector/network/resources/intercept-request-overriden-page.html:8
> +				<p>Overridden page content</p>

indentation Oops?

> LayoutTests/inspector/network/intercept-request.html:25
> +    let suite = ProtocolTest.createAsyncSuite("Network request interception");

This one i'd name `Network.interceptRequest` 😅

> LayoutTests/inspector/network/intercept-request.html:42
> +                InspectorProtocol.awaitCommand({method: "Network.enable", params: {}}),
> +                InspectorProtocol.awaitCommand({method: "Runtime.enable", params: {}}),

NIT: you could put these outside of a test case, as the protocol, is sequential so we know that the `Runtime.evaluate` won't run until these are done

> LayoutTests/inspector/network/intercept-request.html:77
> +                    stage: "request",

this should use the enum `InspectorBackend.Enum.Network.NetworkStage.Request`

> LayoutTests/inspector/network/intercept-request.html:104
> +                    url: url.replace('data.json', 'data2.json'),

Please use a better name than "data2".  How about "data-intercepted"?

> LayoutTests/inspector/network/intercept-request.html:129
> +                InspectorProtocol.awaitCommand({method: "Network.interceptRequestWithResponse", params: {

We should have many of the same tests for this as we have for `Network.interceptWithResponse`.

Also, we should test that the server is _not_ hit when using `Network.interceptRequestWithResponse`.

> LayoutTests/inspector/network/intercept-request.html:163
> +                    errorType: "Cancellation",

this should use the enum `InspectorBackend.Enum.Network.ResourceErrorType.Cancellation`

> LayoutTests/inspector/network/local-resource-override-continue-response.html:49
> +                InspectorProtocol.awaitCommand({method: "Network.interceptContinue", params: {requestId: messageObject.params.requestId, stage: "response"}}),

this should use the enum `InspectorBackend.Enum.Network.NetworkStage.Request`

[Pavel Feldman]

Created attachment 400965 [details]
Patch

[Pavel Feldman]

Created attachment 400973 [details]
Patch

[Pavel Feldman]

Comment on attachment 400510 [details]
Patch

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

Thanks for the thorough review. This has been in the air for so long that I'd like to land it before I add more tests. I'm marking your test requests in the review for the follow up - it isn't that much.

>> Source/JavaScriptCore/inspector/protocol/Network.json:288
>> +                { "name": "base64Encoded", "type": "boolean", "optional": true, "description": "True, if content was sent as base64." },
> 
> this needs tests

Done. Reverted to being required instead.

>> Source/JavaScriptCore/inspector/protocol/Network.json:297
>> +            "description": "Provide response content for an intercepted response.",
> 
> This should explain more about what this means, namely that it'll completely skip the network.

Done.

>> Source/JavaScriptCore/inspector/protocol/Network.json:305
>> +                { "name": "headers", "$ref": "Headers", "description": "HTTP response headers. Pass through original values if unmodified." }
> 
> These should all probably be optional.

We need to populate the request with content, mime & status. Headers could be optional, but I thought of making them also required to emphasize that user is responsible for the entirety of the response.

>> Source/JavaScriptCore/inspector/protocol/Network.json:310
>> +            "description": "Fail request or response with given error type.",
> 
> Can this also be used when a response is intercepted, or just for requests?  If the former, we should change the name.  If the latter, we should change the description.

Done.

>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:878
>> +            pendingRequest->m_completionHandler(&loader->request());
> 
> Can we create a `PendingInterceptRequest::respondWithOriginalRequest` so this common logic (also seen in `InspectorNetworkAgent::interceptContinue`) isn't repeated?

Done.

Named it `continueWithOriginalRequest`. Similarly, encapsulated PendingInterceptRequest::continueWithRequest.

>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1066
>> +    }
> 
> Can we leave this part up top?  That way, if we do fail to parse it happens before we create any objects or do any other logic.

Done.

>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1070
>>          errorString = "Intercept for given url and given isRegex already exists"_s;
> 
> NIT: this should also mention "given stage"

Done.

>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1085
>> +    }
> 
> Ditto (:1062)

Done.

>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1097
>> +    return shouldIntercept(request.url(), NetworkStage::Request) || shouldIntercept(request.url(), NetworkStage::Response);
> 
> NIT: if you wanted, you could put this on a separate line so it lines up for readability:
> ```
>     return shouldIntercept(request.url(), NetworkStage::Request)
>         || shouldIntercept(request.url(), NetworkStage::Response);
> ```

Done.

>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1155
>> +    if (networkStage.value() == NetworkStage::Request) {
> 
> NIT: even though there are only two values, i'd make this into a `switch` so that the paths for each `NetworkStage` value are completely clear:
> ```
>     switch (networkStage.value()) {
>     case NetworkStage::Request:
>         if (auto pendingInterceptRequest = m_pendingInterceptRequests.take(requestId)) {
>             pendingInterceptRequest->respondWithOriginalRequest();
>         else
>             errorString = "Missing pending intercept request for given requestId"_s;
>         return;
> 
>     case NetworkStage::Response:
>         if (auto pendingInterceptResponse = m_pendingInterceptResponses.take(requestId)) {
>             pendingInterceptResponse->respondWithOriginalResponse();
>         else
>             errorString = "Missing pending intercept response for given requestId"_s;
>         return;
>     }
> 
>     ASSERT_NOT_REACHED();
> ```

Done.

>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1161
>> +        errorString = "Missing pending intercept request for given requestId"_s;
> 
> we should have a test for this path

[Carry over]: Adding this in a follow-up patch.

>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1178
>> +        if (!loader->identifier()) {
> 
> Can we create a `bool ResourceLoader::wasCancelled() const` or something like that so it's more explicit, instead of inferring based on whether the `identifier` is set or not (which seems like an internal implementation detail of `ResourceLoader`)?
> 
> Alternatively, could we check `ResourceLoader::reachedTerminalState` instead?

Done, using reachedTerminalState()

>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1182
>> +        // Safe to const cast at this point, we are only adjusting the method / headers / post.
> 
> this comment doesn't seem accurate/necessary

Done.

>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1259
>> +        errorString = "Missing pending intercept response for given requestId"_s;
> 
> s/response/request

Done.

>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1265
>> +        errorString = "Unable to fulfill request, it has already been processed"_s;
> 
> we should have a test for this path

[Carry over]: Adding this in a follow-up patch.

>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1291
>> +    response.setHTTPHeaderField(HTTPHeaderName::ContentType, response.mimeType());
> 
> Perhaps we can create a new private method `InspectorNetworkAgent::createInterceptResponse` so that we can share much of this logic between `InspectorNetworkAgent::interceptWithResponse` and `InspectorNetworkAgent::interceptRequestWithResponse`.  To be clear, I still think we should have separate commands (and I understand that we're basically duplicating variables), but I don't think that means we can't share some logic between them :)

Let me see what I can do here. I'd really like to keep all those required in the interceptRequestWithResponse.

>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1304
>> +        errorString = "Missing pending intercept response for given requestId"_s;
> 
> Ditto (:1259)

Done.

>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1310
>> +        errorString = "Unable to abort request, it has already been processed"_s;
> 
> we should have a test for this path

[Carry over]: Adding this in a follow-up patch.

>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1316
>> +        errorString = "Unknown error type"_s;
> 
> NIT: `makeString("Unknown errorType: "_s, errorTypeString)`

Done.

>> Source/WebCore/inspector/agents/InspectorNetworkAgent.h:167
>> +        PendingInterceptRequest(ResourceLoader* loader, CompletionHandler<void(const ResourceRequest*)>&& completionHandler)
> 
> NIT: can we pass `ResourceLoader&` and do `&loader` here instead?  That way, we know with certainty that when we create this object the `loader` exists (e.g. a caller can't pass `nullptr`).

It is actually created from within our code that ensures it is non-null. I like raw pointer more because we are storing it in a RefPtr after all.

>> Source/WebCore/inspector/agents/InspectorNetworkAgent.h:173
>> +        RefPtr<ResourceLoader> m_loader;
> 
> Do we need a `RefPtr`?  Is it possible for the `ResourceLoader` to go away while Web Inspector is deciding what to do?

Yes, it can be canceled externally.

>> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:164
>> +                            stage: InspectorBackend.Enum.Network.NetworkStage.Response,
> 
> Oh lol oops :P
> 
> Also, can we reorder these so that they match the ordering in the protocol?

Done.

>> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:911
>> +        this.dispatchEventToListeners(WI.NetworkManager.Event.RequestIntercepted, {target, requestId, request});
> 
> This could have a FIXME with a bug link for adding request interception support to the frontend :)

Is there a bug I could refer to? Hesitant requesting something like that. I.e. I can imagine local overrides using the new logic, but not sure what user-facing feature that would be.

>> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:919
>> +            target.NetworkAgent.interceptContinue(requestId, InspectorBackend.Enum.Network.NetworkStage.Response);
> 
> I don't think this will be backwards compatible, as older backends will expect the last argument to be a callback, not a `Network.NetworkStage`.  I'd recommend using `invoke` as that handles all that for you :)
> ```
>     target.NetworkAgent.interceptContinue.invoke({
>         requestId,
>         networkStage: InspectorBackend.Enum.Network.NetworkStage.Response,
>     });
> ```

Done. you mean 'stage' :)

>> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:241
>> +            if (request)
> 
> Can we make this ` const ResourceRequest&` instead?  I feel like if Web Inspector is going to pass a `nullptr`, it just shouldn't even invoke this `CompletionHandler` (which I believe would have the same result as passing `nullptr`).

Done.

>> LayoutTests/http/tests/inspector/network/intercept-request-main-resource-with-response.html:9
>> +    let suite = InspectorTest.createAsyncSuite("Network.interceptRequest");
> 
> This should probably be `Network.interceptRequestWithResponse` as that's really what you're testing.

Done.

>> LayoutTests/http/tests/inspector/network/intercept-request-main-resource-with-response.html:29
>> +                    headers: {},
> 
> we should test if this isn't included

That's required!

>> LayoutTests/http/tests/inspector/network/intercept-request-main-resource.html:9
>> +    let suite = InspectorTest.createAsyncSuite("Network.interceptRequest");
> 
> This should probably be `Network.interceptWithRequest` as that's really what you're testing.

Done.

>> LayoutTests/http/tests/inspector/network/intercept-request-properties.html:27
>> +    let suite = InspectorTest.createAsyncSuite("Network.interceptRequest");
> 
> This should probably be `Network.interceptWithRequest` as that's really what you're testing.

Done

>> LayoutTests/http/tests/inspector/network/intercept-request-properties.html:52
>> +            let data = eval('(' + response.result.value + ')');
> 
> Why not `JSON.parse`?

That is for the relax JSON syntax (trailing commas).

>> LayoutTests/http/tests/inspector/network/intercept-request-properties.html:80
>> +            let data = eval('(' + response.result.value + ')');
> 
> Ditto (:52)

Same as above.

>> LayoutTests/http/tests/inspector/network/intercept-request-subresource-with-error.html:9
>> +    let suite = InspectorTest.createAsyncSuite("Network.interceptRequest");
> 
> This should probably be `Network.interceptRequestWithError` as that's really what you're testing.

Done.

>> LayoutTests/http/tests/inspector/network/intercept-request-subresource-with-error.html:25
>> +                    errorType: "General",
> 
> this should use the enum `InspectorBackend.Network.ResourceErrorType.General`

Done. InspectorBackend.Enum...

>> LayoutTests/http/tests/inspector/network/intercept-request-subresource-with-response.html:9
>> +    let suite = InspectorTest.createAsyncSuite("Network.interceptRequest");
> 
> This should probably be `Network.interceptRequestWithResponse` as that's really what you're testing.

Done.

>> LayoutTests/http/tests/inspector/network/intercept-request-subresource.html:15
>> +					  let target = WI.assumingMainTarget()
> 
> indentation Oops?

Ouch indeed :/

>> LayoutTests/http/tests/inspector/network/resources/intercept-echo.php:15
>> +echo "}\n";
> 
> Do we have to use `echo`?
> ```
>     {
>         "method": "<?php echo $_SERVER['REQUEST_METHOD']; ?>",
>         "headers": {
>     <?php foreach (getallheaders() as $name => $value) { ?>
>             "<?php echo $name; ?>": "<?php echo $value; ?>",
>     <?php } ?>
>         },
>         "post": {
>     <?php foreach ($_POST as $name => $value) { ?>
>             "<?php echo $name; ?>": "<?php echo $value; ?>",
>     <?php } ?>
>         },
>     }
> ```

Done.

>> LayoutTests/http/tests/inspector/network/resources/intercept-request-overriden-page.html:4
>> +				<script src="../resources/inspector-test.js">
> 
> Why is this included in this file?

Done.

>> LayoutTests/http/tests/inspector/network/resources/intercept-request-overriden-page.html:8
>> +				<p>Overridden page content</p>
> 
> indentation Oops?

Yeah, something went wrong.

>> LayoutTests/inspector/network/intercept-request.html:25
>> +    let suite = ProtocolTest.createAsyncSuite("Network request interception");
> 
> This one i'd name `Network.interceptRequest` 😅

Done.

>> LayoutTests/inspector/network/intercept-request.html:42
>> +                InspectorProtocol.awaitCommand({method: "Runtime.enable", params: {}}),
> 
> NIT: you could put these outside of a test case, as the protocol, is sequential so we know that the `Runtime.evaluate` won't run until these are done

Doesn't seem to hurt

>> LayoutTests/inspector/network/intercept-request.html:77
>> +                    stage: "request",
> 
> this should use the enum `InspectorBackend.Enum.Network.NetworkStage.Request`

No such thing in protocol tests I am afraid.

>> LayoutTests/inspector/network/intercept-request.html:104
>> +                    url: url.replace('data.json', 'data2.json'),
> 
> Please use a better name than "data2".  How about "data-intercepted"?

Done.

>> LayoutTests/inspector/network/intercept-request.html:129
>> +                InspectorProtocol.awaitCommand({method: "Network.interceptRequestWithResponse", params: {
> 
> We should have many of the same tests for this as we have for `Network.interceptWithResponse`.
> 
> Also, we should test that the server is _not_ hit when using `Network.interceptRequestWithResponse`.

Yes, those are non-protocol tests, they test the variations of this command. I need http/tests there, so they are front-end tests.

>> LayoutTests/inspector/network/intercept-request.html:163
>> +                    errorType: "Cancellation",
> 
> this should use the enum `InspectorBackend.Enum.Network.ResourceErrorType.Cancellation`

Same thing, no constants here.

>> LayoutTests/inspector/network/local-resource-override-continue-response.html:49
>> +                InspectorProtocol.awaitCommand({method: "Network.interceptContinue", params: {requestId: messageObject.params.requestId, stage: "response"}}),
> 
> this should use the enum `InspectorBackend.Enum.Network.NetworkStage.Request`

Done. You mean Response.

[Pavel Feldman]

Created attachment 400976 [details]
Patch

[Devin Rousso]

Comment on attachment 400510 [details]
Patch

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

>>> Source/JavaScriptCore/inspector/protocol/Network.json:305
>>> +                { "name": "headers", "$ref": "Headers", "description": "HTTP response headers. Pass through original values if unmodified." }
>> 
>> These should all probably be optional.
> 
> We need to populate the request with content, mime & status. Headers could be optional, but I thought of making them also required to emphasize that user is responsible for the entirety of the response.

If they're not optional, then the "Pass through original values if unmodified." makes less sense and is more cumbersome, as that would mean I'd have to provide an empty string instead of not providing the parameter at all.

>>> Source/WebCore/inspector/agents/InspectorNetworkAgent.h:167
>>> +        PendingInterceptRequest(ResourceLoader* loader, CompletionHandler<void(const ResourceRequest*)>&& completionHandler)
>> 
>> NIT: can we pass `ResourceLoader&` and do `&loader` here instead?  That way, we know with certainty that when we create this object the `loader` exists (e.g. a caller can't pass `nullptr`).
> 
> It is actually created from within our code that ensures it is non-null. I like raw pointer more because we are storing it in a RefPtr after all.

If you want a `RefPtr`, then can you make the constructor parameter a `RefPtr<ResourceLoader>`?  We should avoid using raw pointers as much as possible.

>>> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:911
>>> +        this.dispatchEventToListeners(WI.NetworkManager.Event.RequestIntercepted, {target, requestId, request});
>> 
>> This could have a FIXME with a bug link for adding request interception support to the frontend :)
> 
> Is there a bug I could refer to? Hesitant requesting something like that. I.e. I can imagine local overrides using the new logic, but not sure what user-facing feature that would be.

We 100% want to add a UI for this in the frontend.  It would be _very_ useful for many of our users.  We have some internal mockups for what this would look like.

It's fine if you don't want to make a new bug for it, but at least a FIXME comment would let others know that this is the place to add things in case they're unfamiliar with what's already been implemented :)

[Devin Rousso]

Comment on attachment 400976 [details]
Patch

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

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:863
> +        if (intercept.networkStage != networkStage)
> +            continue;
> +        if (intercept.url.isEmpty())
> +            return true;

NIT: I really would prefer if you added newlines after control flow statements like this.  I realize that it's not written down in the WebKit style guide, but if you look at the rest of this file (and the majority of other Web Inspector files), you'll find that we add newlines for clarity as a way of "highlighting" control flow.  If you feel really strongly about keeping it as is that's fine (like I said it's not written down), but we'd appreciate matching the style of the surrounding/related code :)

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1068
> +        errorString = "Intercept for given url, given isRegex and given stage already exists"_s;

NIT: oxford comma 😅

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1157
> +        {

Style: this is not how WebKit does `case` blocks.  Please remove the `{` and `}` and de-indent.

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1159
> +            auto pendingInterceptRequest = m_pendingInterceptRequests.take(requestId);
> +            if (pendingInterceptRequest)

NIT: why not put this inside the `if`?

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1292
> +    loader->didReceiveResponse(response, [loader, buffer = data.releaseNonNull()]() mutable {

You're capturing `loader` by value.  Do you mean to do that, or can we do an auto-capture by reference instead?
```
    loader->didReceiveResponse(response, [&, buffer = data.releaseNonNull()]() mutable {
```

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1295
> +        loader->didFinishLoading(NetworkLoadMetrics());

Should we provide at least some amount of timing info?  I think we could set the `fetchStart`, `requestStart`, and `responseStart` to something like `resourceLoader->loadTiming().fetchStart()`

> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:235
> +        if (resourceLoader.options().serviceWorkerRegistrationIdentifier || !InspectorInstrumentationWebKit::shouldInterceptRequest(resourceLoader.frame(), resourceLoader.request())) {

NIT: I'd switch this and put the inspector bits inside the `if` and leave the existing code as is, as that way the inspector bits are grouped
```
    if (!resourceLoader.options().serviceWorkerRegistrationIdentifier && InspectorInstrumentationWebKit::shouldInterceptRequest(resourceLoader.frame(), resourceLoader.request())) {
        InspectorInstrumentationWebKit::interceptRequest(resourceLoader, [&this, protectedResourceLoader = makeRefPtr(&resourceLoader), &trackingParameters, shouldClearReferrerOnHTTPSToHTTPRedirect, &resource](const ResourceRequest& request) {
            scheduleLoadFromNetworkProcess(*protectedResourceLoader, request, trackingParameters, shouldClearReferrerOnHTTPSToHTTPRedirect, maximumBufferingTime(resource));
        });
        return;
    }

    scheduleLoadFromNetworkProcess(resourceLoader, resourceLoader.request(), trackingParameters, shouldClearReferrerOnHTTPSToHTTPRedirect, maximumBufferingTime(resource));
```

> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:240
> +        InspectorInstrumentationWebKit::interceptRequest(resourceLoader, [this, protectedResourceLoader = makeRefPtr(&resourceLoader), trackingParameters, shouldClearReferrerOnHTTPSToHTTPRedirect, resource](const ResourceRequest& request) mutable {

Ditto (InspectorNetworkAgent.cpp:235)

Also, I'm fairly certain that this does not need to be `mutable`.

> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:242
> +            return;

NIT: unnecessary `return`

> Tools/Scripts/webkitpy/layout_tests/servers/apache_http_server.py:-182
> -        _log.debug('Starting %s server, cmd="%s"' % (self._name, str(self._start_cmd)))

why was this changed?

> LayoutTests/inspector/network/intercept-request.html:43
> +                InspectorProtocol.awaitCommand({method: "Runtime.evaluate", params: {

As I've mentioned, this is not Web Inspector style/parlance.  Please either inline the entire object or prettify it.

```
    InspectorProtocol.awaitCommand({method: "Runtime.evaluate", params: {expression: "document.URL"}});
```

 or

```
    InspectorProtocol.awaitCommand({
        method: "Runtime.evaluate",
        params: {
            expression: "document.URL".
        },
    });
```

If you feel strongly about this style you're welcome to bring it up with the other Web Inspector developers/reviewers and we can all come to a consensus, but until then this is not our style.

[Devin Rousso]

Comment on attachment 400510 [details]
Patch

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

>>>> Source/JavaScriptCore/inspector/protocol/Network.json:305
>>>> +                { "name": "headers", "$ref": "Headers", "description": "HTTP response headers. Pass through original values if unmodified." }
>>> 
>>> These should all probably be optional.
>> 
>> We need to populate the request with content, mime & status. Headers could be optional, but I thought of making them also required to emphasize that user is responsible for the entirety of the response.
> 
> If they're not optional, then the "Pass through original values if unmodified." makes less sense and is more cumbersome, as that would mean I'd have to provide an empty string instead of not providing the parameter at all.

Oh I see what you're referring to.  Yeah I think it makes sense for these to _not_ be optional since there is no "original response".  As such, I'd just remove that part of the description :P

[Pavel Feldman]

Created attachment 401237 [details]
Patch

[Pavel Feldman]

Comment on attachment 400510 [details]
Patch

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

>>>>> Source/JavaScriptCore/inspector/protocol/Network.json:305
>>>>> +                { "name": "headers", "$ref": "Headers", "description": "HTTP response headers. Pass through original values if unmodified." }
>>>> 
>>>> These should all probably be optional.
>>> 
>>> We need to populate the request with content, mime & status. Headers could be optional, but I thought of making them also required to emphasize that user is responsible for the entirety of the response.
>> 
>> If they're not optional, then the "Pass through original values if unmodified." makes less sense and is more cumbersome, as that would mean I'd have to provide an empty string instead of not providing the parameter at all.
> 
> Oh I see what you're referring to.  Yeah I think it makes sense for these to _not_ be optional since there is no "original response".  As such, I'd just remove that part of the description :P

Done.

>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1089
>>          errorString = "Missing intercept for given url and given isRegex"_s;
> 
> ditto (:1070)

Done

>>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1161
>>> +        errorString = "Missing pending intercept request for given requestId"_s;
>> 
>> we should have a test for this path
> 
> [Carry over]: Adding this in a follow-up patch.

Done, http/tests/inspector/network/intercept-request-continue.html

>>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1265
>>> +        errorString = "Unable to fulfill request, it has already been processed"_s;
>> 
>> we should have a test for this path
> 
> [Carry over]: Adding this in a follow-up patch.

Done. http/tests/inspector/network/intercept-aborted-request.html

>>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1310
>>> +        errorString = "Unable to abort request, it has already been processed"_s;
>> 
>> we should have a test for this path
> 
> [Carry over]: Adding this in a follow-up patch.

Done. http/tests/inspector/network/intercept-aborted-request.html

>>>> Source/WebCore/inspector/agents/InspectorNetworkAgent.h:167
>>>> +        PendingInterceptRequest(ResourceLoader* loader, CompletionHandler<void(const ResourceRequest*)>&& completionHandler)
>>> 
>>> NIT: can we pass `ResourceLoader&` and do `&loader` here instead?  That way, we know with certainty that when we create this object the `loader` exists (e.g. a caller can't pass `nullptr`).
>> 
>> It is actually created from within our code that ensures it is non-null. I like raw pointer more because we are storing it in a RefPtr after all.
> 
> If you want a `RefPtr`, then can you make the constructor parameter a `RefPtr<ResourceLoader>`?  We should avoid using raw pointers as much as possible.

Done.

>>>> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:911
>>>> +        this.dispatchEventToListeners(WI.NetworkManager.Event.RequestIntercepted, {target, requestId, request});
>>> 
>>> This could have a FIXME with a bug link for adding request interception support to the frontend :)
>> 
>> Is there a bug I could refer to? Hesitant requesting something like that. I.e. I can imagine local overrides using the new logic, but not sure what user-facing feature that would be.
> 
> We 100% want to add a UI for this in the frontend.  It would be _very_ useful for many of our users.  We have some internal mockups for what this would look like.
> 
> It's fine if you don't want to make a new bug for it, but at least a FIXME comment would let others know that this is the place to add things in case they're unfamiliar with what's already been implemented :)

Done

[Pavel Feldman]

Created attachment 401238 [details]
Patch

[Pavel Feldman]

Comment on attachment 400976 [details]
Patch

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

>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:863
>> +            return true;
> 
> NIT: I really would prefer if you added newlines after control flow statements like this.  I realize that it's not written down in the WebKit style guide, but if you look at the rest of this file (and the majority of other Web Inspector files), you'll find that we add newlines for clarity as a way of "highlighting" control flow.  If you feel really strongly about keeping it as is that's fine (like I said it's not written down), but we'd appreciate matching the style of the surrounding/related code :)

Done

>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1068
>> +        errorString = "Intercept for given url, given isRegex and given stage already exists"_s;
> 
> NIT: oxford comma 😅

Done

>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1157
>> +        {
> 
> Style: this is not how WebKit does `case` blocks.  Please remove the `{` and `}` and de-indent.

I needed a scope here for the pending request, but inlining it into if() gives me that scope.

>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1159
>> +            if (pendingInterceptRequest)
> 
> NIT: why not put this inside the `if`?

Done

>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1292
>> +    loader->didReceiveResponse(response, [loader, buffer = data.releaseNonNull()]() mutable {
> 
> You're capturing `loader` by value.  Do you mean to do that, or can we do an auto-capture by reference instead?
> ```
>     loader->didReceiveResponse(response, [&, buffer = data.releaseNonNull()]() mutable {
> ```

`loader` is a RefPtr on the stack, capturing it by reference would result in use after free. I do need to retain the loader until my call.

>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1295
>> +        loader->didFinishLoading(NetworkLoadMetrics());
> 
> Should we provide at least some amount of timing info?  I think we could set the `fetchStart`, `requestStart`, and `responseStart` to something like `resourceLoader->loadTiming().fetchStart()`

NetworkLoadMetrics populates window.performance API for client-side instrumentation. We can add those, but I don't think this is necessary since we are 'debugging' / 'overriding' the page and reporting the performance data back to the origin does not make much sense in this case.

>> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:235
>> +        if (resourceLoader.options().serviceWorkerRegistrationIdentifier || !InspectorInstrumentationWebKit::shouldInterceptRequest(resourceLoader.frame(), resourceLoader.request())) {
> 
> NIT: I'd switch this and put the inspector bits inside the `if` and leave the existing code as is, as that way the inspector bits are grouped
> ```
>     if (!resourceLoader.options().serviceWorkerRegistrationIdentifier && InspectorInstrumentationWebKit::shouldInterceptRequest(resourceLoader.frame(), resourceLoader.request())) {
>         InspectorInstrumentationWebKit::interceptRequest(resourceLoader, [&this, protectedResourceLoader = makeRefPtr(&resourceLoader), &trackingParameters, shouldClearReferrerOnHTTPSToHTTPRedirect, &resource](const ResourceRequest& request) {
>             scheduleLoadFromNetworkProcess(*protectedResourceLoader, request, trackingParameters, shouldClearReferrerOnHTTPSToHTTPRedirect, maximumBufferingTime(resource));
>         });
>         return;
>     }
> 
>     scheduleLoadFromNetworkProcess(resourceLoader, resourceLoader.request(), trackingParameters, shouldClearReferrerOnHTTPSToHTTPRedirect, maximumBufferingTime(resource));
> ```

Done. But we should not capture stack objects by reference - that all leads to use after free.

>> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:240
>> +        InspectorInstrumentationWebKit::interceptRequest(resourceLoader, [this, protectedResourceLoader = makeRefPtr(&resourceLoader), trackingParameters, shouldClearReferrerOnHTTPSToHTTPRedirect, resource](const ResourceRequest& request) mutable {
> 
> Ditto (InspectorNetworkAgent.cpp:235)
> 
> Also, I'm fairly certain that this does not need to be `mutable`.

Done.

>> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:242
>> +            return;
> 
> NIT: unnecessary `return`

Done.

>> Tools/Scripts/webkitpy/layout_tests/servers/apache_http_server.py:-182
>> -        _log.debug('Starting %s server, cmd="%s"' % (self._name, str(self._start_cmd)))
> 
> why was this changed?

Done, reverted.

>> LayoutTests/inspector/network/intercept-request.html:43
>> +                InspectorProtocol.awaitCommand({method: "Runtime.evaluate", params: {
> 
> As I've mentioned, this is not Web Inspector style/parlance.  Please either inline the entire object or prettify it.
> 
> ```
>     InspectorProtocol.awaitCommand({method: "Runtime.evaluate", params: {expression: "document.URL"}});
> ```
> 
>  or
> 
> ```
>     InspectorProtocol.awaitCommand({
>         method: "Runtime.evaluate",
>         params: {
>             expression: "document.URL".
>         },
>     });
> ```
> 
> If you feel strongly about this style you're welcome to bring it up with the other Web Inspector developers/reviewers and we can all come to a consensus, but until then this is not our style.

Done. I removed this test, it is now redundant.

[Pavel Feldman]

Created attachment 401646 [details]
Patch

[Pavel Feldman]

Created attachment 401648 [details]
Patch

[Devin Rousso]

Comment on attachment 401648 [details]
Patch

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

r-, as I think there a couple more edge cases we should test (i imagine the next iteration will be perfect to land), but this is looking phenomenal!
Really well done! =D
Thanks for iterating on it with me :)

> Source/JavaScriptCore/inspector/protocol/Network.json:285
> +                { "name": "method", "type": "string", "optional": true,"description": "HTTP request method." },
> +                { "name": "url", "type": "string", "optional": true,"description": "HTTP request url." },

NIT: we should put `url` above/before `method` to match the ordering in `Network.Request`.

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1089
> +        errorString = "Missing intercept for given url, given isRegex, and given stage already exists"_s;

remove the "already exists" (perhaps a copypaste error?)

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1176
> +    if (pendingRequest) {

NIT: you could flip this and have the error be an early return so most of this code is less indented if you wanted
```
    if (!pendingRequest) {
        errorString = "Missing pending intercept request for given requestId"_s;
        return;
    }

    auto& request = pendingRequest->m_loader->request();
    ...
```

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1177
> +        ResourceLoader* loader = pendingRequest->m_loader.get();

Why not dereference it here instead of keeping it as a pointer?
```
    auto& loader = *pendingRequest->m_loader;
```

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1200
> +        pendingRequest->continueWithRequest(request);

we might want a FIXME here for when we add this to the frontend to figure out how to identify when a request has been overridden (i.e. add a `ResourceRequestBase::Requester::InspectorOverride` and somehow send it to the frontend)

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1259
> +    RefPtr<ResourceLoader> loader = pendingRequest->m_loader.get();

Ditto (:1177)

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1260
> +    if (!loader->identifier()) {

Should this be `reachedTerminalState()`?

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1282
> +    for (auto& header : headers) {

NIT: you could use structured binding
```
    for (auto& [key, value] : *headers) {
```

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1304
> +    ResourceLoader* loader = pendingRequest->m_loader.get();

Ditto (:1177)

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1305
> +    if (!loader->identifier()) {

Ditto (:1260)

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1316
> +    ResourceError error;

NIT: is there that much benefit to creating this variable vs just calling `loader->didFail` inside each `case`?
```
    switch (errorType.value()) {
    case Inspector::Protocol::Network::ResourceErrorType::General:
        loader->didFail(ResourceError(errorDomainWebKitInternal, 0, loader->url(), "Request intercepted"_s, ResourceError::Type::General));
        return;

    case Inspector::Protocol::Network::ResourceErrorType::AccessControl:
        loader->didFail(ResourceError(errorDomainWebKitInternal, 0, loader->url(), "Access denied"_s, ResourceError::Type::AccessControl));
        return;

    case Inspector::Protocol::Network::ResourceErrorType::Cancellation:
        loader->didFail(ResourceError(errorDomainWebKitInternal, 0, loader->url(), "Request canceled"_s, ResourceError::Type::Cancellation));
        return;

    case Inspector::Protocol::Network::ResourceErrorType::Timeout:
        loader->didFail(ResourceError(errorDomainWebKitInternal, 0, loader->url(), "Request timed out"_s, ResourceError::Type::Timeout));
        return;
    }

    ASSERT_NOT_REACHED();
```

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1332
> +    default:
> +        ASSERT_NOT_REACHED();
> +        return;

NIT: is this actually necessary?  i would think not because the `switch` handles all of the `enum` values 🤔

> Source/WebCore/inspector/agents/InspectorNetworkAgent.h:167
> +        PendingInterceptRequest(RefPtr<ResourceLoader> loader, CompletionHandler<void(const ResourceRequest&)>&& completionHandler)

Can we make this either a `RefPtr<ResourceLoader>&` or `RefPtr<ResourceLoader>&&`?

> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:913
> +        // FIXME: add request interception support to the frontend.

Thanks!

> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:914
> +        this.dispatchEventToListeners(WI.NetworkManager.Event.RequestIntercepted, {target, requestId, request});

NIT: do we need to include `target`?  The Network domain only supports the main target right now anyways, so it should be equivalent to `WI.mainTarget`.

> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:-232
> -        return;

I think we want to keep this, as otherwise we will incorrectly hit the log below

> LayoutTests/http/tests/inspector/network/intercept-aborted-request.html:9
> +async function fetchAndAbort() {

Can we test that it works the same with `XMLHttpRequest.prototype.abort` too, or does that have the same codepath?

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

oops

> LayoutTests/http/tests/inspector/network/intercept-aborted-request.html:21
> +    let target = WI.assumingMainTarget();

You don't need to do this in tests.  `window.FooAgent` is a getter for `WI.mainTarget.FooAgent`.

> LayoutTests/http/tests/inspector/network/intercept-aborted-request.html:26
> +            await target.NetworkAgent.addInterception.invoke({

NIT: you can do this outside of a test case, as it is guaranteed to evaluate before any other protocol messaging

> LayoutTests/http/tests/inspector/network/intercept-aborted-request.html:36
> +        async test() {

This test reads pretty oddly, as I have to jump up and down in the code to follow the logic.  I think there's a way of making it simpler and more linear/straightforward:
```
    async test() {
        InspectorTest.log("Triggering load...");
        let [requestInterceptedEvent] = await Promise.all([
            WI.networkManager.awaitEvent(WI.NetworkManager.Event.RequestIntercepted),
            InspectorTest.evaluateInPage(`fetchAndAbort()`),
        ]);

        await InspectorTest.expectException(async () => {
            await NetworkAgent.interceptRequestWithResponse.invoke({
                requestId: requestInterceptedEvent.data.requestId,
                stage: InspectorBackend.Enum.Network.NetworkStage.Request,
                content: "FAIL",
                base64Encoded: false,
                mimeType: "text/plain",
                status: 200,
                statusText: "OK",
                headers: {},
            });
        });
    }
```
this also matches the flow of most of our (newer) tests :)

> LayoutTests/http/tests/inspector/network/intercept-aborted-request.html:38
> +            let interceptRequestWithResponseResult = new Promise(f => callback =  f);

Style: please use more descriptive names than something like `f`
Style: we always wrap the arguments of an arrow function with parenthesis
Style: extra space
```
    let interceptRequestWithResponseResult = new Promise((resolve) => callback = resolve);
```

> LayoutTests/http/tests/inspector/network/intercept-aborted-request.html:60
> +            const error = await interceptRequestWithResponseResult;

Style: we normally only use `const` for things that are truly constant

> LayoutTests/http/tests/inspector/network/intercept-aborted-request.html:69
> +        async test() {

Ditto (:36)

> LayoutTests/http/tests/inspector/network/intercept-aborted-request.html:99
> +<script src="resources/override.js"></script>

Is this needed for this test?

> LayoutTests/http/tests/inspector/network/intercept-request-continue.html:16
> +    let target = WI.assumingMainTarget();

Ditto (intercept-aborted-request.html:21)

> LayoutTests/http/tests/inspector/network/intercept-request-continue.html:21
> +            await target.NetworkAgent.addInterception.invoke({

Ditto (intercept-aborted-request.html:26)

> LayoutTests/http/tests/inspector/network/intercept-request-continue.html:53
> +        async test() {

Ditto (intercept-aborted-request.html:36)

> LayoutTests/http/tests/inspector/network/intercept-request-continue.html:80
> +        async test() {

Ditto (intercept-aborted-request.html:36)

> LayoutTests/http/tests/inspector/network/intercept-request-main-resource-with-response.html:15
> +            let target = WI.assumingMainTarget();

Ditto (intercept-aborted-request.html:21)

> LayoutTests/http/tests/inspector/network/intercept-request-main-resource-with-response.html:21
> +            let interceptWithResponse = (event) => {

Why create a variable instead of inlining this in the `addEventListener` call?

> LayoutTests/http/tests/inspector/network/intercept-request-main-resource-with-response.html:33
> +            WI.networkManager.addEventListener(WI.NetworkManager.Event.RequestIntercepted, interceptWithResponse);

Please use either `awaitEvent` (and a `Promise`) or `singleFireEventListener` if you're not going to remove the event listener before this test cases finishes.  It _shouldn't_ be a problem to leave the event listener like this, but there have been issues with how state is reset between tests in the past so i'd like to avoid exacerbating any possible issues by leaving event listeners around.

> LayoutTests/http/tests/inspector/network/intercept-request-main-resource-with-response.html:35
> +            InspectorTest.reloadPage({ignoreCache: false, revalidateAllResources: true});

Please `await` this.  Because this test case is `async`, I can imagine this failing (being flaky) if there is too much time between this returning and the `WI.NetworkManager.Event.RequestIntercepted` being fired.

> LayoutTests/http/tests/inspector/network/intercept-request-main-resource.html:15
> +            let target = WI.assumingMainTarget();

Ditto (intercept-aborted-request.html:21)

> LayoutTests/http/tests/inspector/network/intercept-request-main-resource.html:21
> +            let interceptWithRequest = (event) => {

Ditto (intercept-request-main-resource-with-response.html:21)

> LayoutTests/http/tests/inspector/network/intercept-request-main-resource.html:25
> +                    requestId

Style: missing trailing comma

> LayoutTests/http/tests/inspector/network/intercept-request-main-resource.html:28
> +            WI.networkManager.addEventListener(WI.NetworkManager.Event.RequestIntercepted, interceptWithRequest);

Ditto (intercept-request-main-resource-with-response.html:33)

> LayoutTests/http/tests/inspector/network/intercept-request-main-resource.html:30
> +            InspectorTest.reloadPage({ignoreCache: false, revalidateAllResources: true});

Ditto (intercept-request-main-resource-with-response.html:35)

> LayoutTests/http/tests/inspector/network/intercept-request-properties.html:12
> +    data["responseURL"] = response.url;

`data.responseURL`?

> LayoutTests/http/tests/inspector/network/intercept-request-properties.html:21
> +            "Content-Type": "application/x-www-form-urlencoded"

Ditto (intercept-request-main-resource.html:25)

> LayoutTests/http/tests/inspector/network/intercept-request-properties.html:23
> +        body: "value=original"

Ditto (intercept-request-main-resource.html:25)

> LayoutTests/http/tests/inspector/network/intercept-request-properties.html:27
> +    data["responseURL"] = response.url;

Ditto (:12)

> LayoutTests/http/tests/inspector/network/intercept-request-properties.html:34
> +    let target = WI.assumingMainTarget();

Ditto (intercept-aborted-request.html:21)

> LayoutTests/http/tests/inspector/network/intercept-request-properties.html:39
> +            await target.NetworkAgent.addInterception.invoke({

Ditto (intercept-aborted-request.html:26)

> LayoutTests/http/tests/inspector/network/intercept-request-properties.html:91
> +            async test() {

Ditto (intercept-aborted-request.html:36)

> LayoutTests/http/tests/inspector/network/intercept-request-properties.html:134
> +        description: "Tests request method interception with NONSTANDARD",

oops

> LayoutTests/http/tests/inspector/network/intercept-request-properties.html:140
> +        name: "Network.interceptRequest.Url",

NIT: s/Url/URL

> LayoutTests/http/tests/inspector/network/intercept-request-properties.html:147
> +        name: "Network.interceptRequest.UrlFragment",

Ditto (:140)

> LayoutTests/http/tests/inspector/network/intercept-request-properties.html:150
> +        overrides: { url: "http://127.0.0.1:8000/inspector/network/resources/intercept-echo.php#fragment" },

Can we add a test (or modify these tests) to check that the URL of the corresponding `WI.Resource` response matches?  I'd like to test/confirm that fragments eventually make it back to the response (or if not to make sure that it stays that way).

> LayoutTests/http/tests/inspector/network/intercept-request-properties.html:154
> +        name: "Network.interceptRequest.UrlEmpty",

Ditto (:140)

> LayoutTests/http/tests/inspector/network/intercept-request-subresource-with-error.html:15
> +            let target = WI.assumingMainTarget();

Ditto (intercept-aborted-request.html:21)

> LayoutTests/http/tests/inspector/network/intercept-request-subresource-with-error.html:21
> +            let interceptWithRequest = (event) => {

Ditto (intercept-request-main-resource-with-response.html:21)

> LayoutTests/http/tests/inspector/network/intercept-request-subresource-with-error.html:25
> +                    errorType: InspectorBackend.Enum.Network.ResourceErrorType.General,

Can we test that this error is actually used?  AFAICT, the only way to confirm that this test is actually working is the fact that there is only one ALERT, which is not very immediately identifiable.  We should be able to check that the `override.js` resource actually errored and that the error matches what we want.

> LayoutTests/http/tests/inspector/network/intercept-request-subresource-with-error.html:28
> +            WI.networkManager.addEventListener(WI.NetworkManager.Event.RequestIntercepted, interceptWithRequest);

Ditto (intercept-request-main-resource-with-response.html:33)

> LayoutTests/http/tests/inspector/network/intercept-request-subresource-with-response.html:15
> +            let target = WI.assumingMainTarget();

Ditto (intercept-aborted-request.html:21)

> LayoutTests/http/tests/inspector/network/intercept-request-subresource-with-response.html:22
> +            let interceptWithRequest = (event) => {

Ditto (intercept-request-main-resource-with-response.html:21)

> LayoutTests/http/tests/inspector/network/intercept-request-subresource-with-response.html:34
> +            WI.networkManager.addEventListener(WI.NetworkManager.Event.RequestIntercepted, interceptWithRequest);

Ditto (intercept-request-main-resource-with-response.html:33)

> LayoutTests/http/tests/inspector/network/intercept-request-subresource.html:15
> +            let target = WI.assumingMainTarget();

Ditto (intercept-aborted-request.html:21)

> LayoutTests/http/tests/inspector/network/intercept-request-subresource.html:20
> +            let interceptWithRequest = (event) => {

Ditto (intercept-request-main-resource-with-response.html:21)

> LayoutTests/http/tests/inspector/network/intercept-request-subresource.html:24
> +                    requestId

Ditto (intercept-request-main-resource.html:25)

> LayoutTests/http/tests/inspector/network/intercept-request-subresource.html:27
> +            WI.networkManager.addEventListener(WI.NetworkManager.Event.RequestIntercepted, interceptWithRequest);

Ditto (intercept-request-main-resource-with-response.html:33)

> LayoutTests/http/tests/inspector/network/intercept-request-with-response.html:21
> +            headers: [...response.headers]

`Array.from`?
Ditto (intercept-request-main-resource.html:25)

> LayoutTests/http/tests/inspector/network/intercept-request-with-response.html:30
> +    InspectorTest.debug();

oops

> LayoutTests/http/tests/inspector/network/intercept-request-with-response.html:32
> +    let target = WI.assumingMainTarget();

Ditto (intercept-aborted-request.html:21)

> LayoutTests/http/tests/inspector/network/intercept-request-with-response.html:37
> +            await target.NetworkAgent.addInterception.invoke({

Ditto (intercept-aborted-request.html:26)

> LayoutTests/http/tests/inspector/network/intercept-request-with-response.html:50
> +        let keys = [...response.headers.keys()];

Ditto (:21)

> LayoutTests/http/tests/inspector/network/intercept-request-with-response.html:64
> +            async test() {

Ditto (intercept-aborted-request.html:36)

> LayoutTests/http/tests/inspector/network/intercept-request-with-response.html:148
> +        expression: `triggerOverrideLoad("#frag")`,

Ditto (intercept-request-properties.html:150)

[Pavel Feldman]

Created attachment 401823 [details]
Patch

[Pavel Feldman]

Comment on attachment 401648 [details]
Patch

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

>> Source/JavaScriptCore/inspector/protocol/Network.json:285
>> +                { "name": "url", "type": "string", "optional": true,"description": "HTTP request url." },
> 
> NIT: we should put `url` above/before `method` to match the ordering in `Network.Request`.

Done.

>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1089
>> +        errorString = "Missing intercept for given url, given isRegex, and given stage already exists"_s;
> 
> remove the "already exists" (perhaps a copypaste error?)

Done

>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1176
>> +    if (pendingRequest) {
> 
> NIT: you could flip this and have the error be an early return so most of this code is less indented if you wanted
> ```
>     if (!pendingRequest) {
>         errorString = "Missing pending intercept request for given requestId"_s;
>         return;
>     }
> 
>     auto& request = pendingRequest->m_loader->request();
>     ...
> ```

Done.

>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1177
>> +        ResourceLoader* loader = pendingRequest->m_loader.get();
> 
> Why not dereference it here instead of keeping it as a pointer?
> ```
>     auto& loader = *pendingRequest->m_loader;
> ```

Done.

>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1200
>> +        pendingRequest->continueWithRequest(request);
> 
> we might want a FIXME here for when we add this to the frontend to figure out how to identify when a request has been overridden (i.e. add a `ResourceRequestBase::Requester::InspectorOverride` and somehow send it to the frontend)

Done.

>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1259
>> +    RefPtr<ResourceLoader> loader = pendingRequest->m_loader.get();
> 
> Ditto (:1177)

Can't do it here - I'm retaining this refprt.

>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1260
>> +    if (!loader->identifier()) {
> 
> Should this be `reachedTerminalState()`?

Done.

>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1282
>> +    for (auto& header : headers) {
> 
> NIT: you could use structured binding
> ```
>     for (auto& [key, value] : *headers) {
> ```

Done.

>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1304
>> +    ResourceLoader* loader = pendingRequest->m_loader.get();
> 
> Ditto (:1177)

Done.

>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1305
>> +    if (!loader->identifier()) {
> 
> Ditto (:1260)

Done.

>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1316
>> +    ResourceError error;
> 
> NIT: is there that much benefit to creating this variable vs just calling `loader->didFail` inside each `case`?
> ```
>     switch (errorType.value()) {
>     case Inspector::Protocol::Network::ResourceErrorType::General:
>         loader->didFail(ResourceError(errorDomainWebKitInternal, 0, loader->url(), "Request intercepted"_s, ResourceError::Type::General));
>         return;
> 
>     case Inspector::Protocol::Network::ResourceErrorType::AccessControl:
>         loader->didFail(ResourceError(errorDomainWebKitInternal, 0, loader->url(), "Access denied"_s, ResourceError::Type::AccessControl));
>         return;
> 
>     case Inspector::Protocol::Network::ResourceErrorType::Cancellation:
>         loader->didFail(ResourceError(errorDomainWebKitInternal, 0, loader->url(), "Request canceled"_s, ResourceError::Type::Cancellation));
>         return;
> 
>     case Inspector::Protocol::Network::ResourceErrorType::Timeout:
>         loader->didFail(ResourceError(errorDomainWebKitInternal, 0, loader->url(), "Request timed out"_s, ResourceError::Type::Timeout));
>         return;
>     }
> 
>     ASSERT_NOT_REACHED();
> ```

I'd prefer to leave it as is and keep a single return statement in this case for readability.

>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1332
>> +        return;
> 
> NIT: is this actually necessary?  i would think not because the `switch` handles all of the `enum` values 🤔

It handles all, but I will remove the assertion and default to general.

>> Source/WebCore/inspector/agents/InspectorNetworkAgent.h:167
>> +        PendingInterceptRequest(RefPtr<ResourceLoader> loader, CompletionHandler<void(const ResourceRequest&)>&& completionHandler)
> 
> Can we make this either a `RefPtr<ResourceLoader>&` or `RefPtr<ResourceLoader>&&`?

I'm passing &loader into this constructor, so neither would make much sense.

>> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:914
>> +        this.dispatchEventToListeners(WI.NetworkManager.Event.RequestIntercepted, {target, requestId, request});
> 
> NIT: do we need to include `target`?  The Network domain only supports the main target right now anyways, so it should be equivalent to `WI.mainTarget`.

Does not matter to me much, but it seems logical to pass it for attribution.

>> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:-232
>> -        return;
> 
> I think we want to keep this, as otherwise we will incorrectly hit the log below

Done. Thanks, that's what I meant by the review churn - things start falling through the cracks.

>> LayoutTests/http/tests/inspector/network/intercept-aborted-request.html:9
>> +async function fetchAndAbort() {
> 
> Can we test that it works the same with `XMLHttpRequest.prototype.abort` too, or does that have the same codepath?

Both should land in the ThreadedLoader::cancel(), but I'm adding tests for failures.

>> LayoutTests/http/tests/inspector/network/intercept-aborted-request.html:19
>> +    InspectorTest.debug();
> 
> oops

Done.

>> LayoutTests/http/tests/inspector/network/intercept-aborted-request.html:21
>> +    let target = WI.assumingMainTarget();
> 
> You don't need to do this in tests.  `window.FooAgent` is a getter for `WI.mainTarget.FooAgent`.

Done.

>> LayoutTests/http/tests/inspector/network/intercept-aborted-request.html:26
>> +            await target.NetworkAgent.addInterception.invoke({
> 
> NIT: you can do this outside of a test case, as it is guaranteed to evaluate before any other protocol messaging

Done.

>> LayoutTests/http/tests/inspector/network/intercept-aborted-request.html:36
>> +        async test() {
> 
> This test reads pretty oddly, as I have to jump up and down in the code to follow the logic.  I think there's a way of making it simpler and more linear/straightforward:
> ```
>     async test() {
>         InspectorTest.log("Triggering load...");
>         let [requestInterceptedEvent] = await Promise.all([
>             WI.networkManager.awaitEvent(WI.NetworkManager.Event.RequestIntercepted),
>             InspectorTest.evaluateInPage(`fetchAndAbort()`),
>         ]);
> 
>         await InspectorTest.expectException(async () => {
>             await NetworkAgent.interceptRequestWithResponse.invoke({
>                 requestId: requestInterceptedEvent.data.requestId,
>                 stage: InspectorBackend.Enum.Network.NetworkStage.Request,
>                 content: "FAIL",
>                 base64Encoded: false,
>                 mimeType: "text/plain",
>                 status: 200,
>                 statusText: "OK",
>                 headers: {},
>             });
>         });
>     }
> ```
> this also matches the flow of most of our (newer) tests :)

Done. I thought you did not like multi-line arrow functions in this review.

>> LayoutTests/http/tests/inspector/network/intercept-aborted-request.html:38
>> +            let interceptRequestWithResponseResult = new Promise(f => callback =  f);
> 
> Style: please use more descriptive names than something like `f`
> Style: we always wrap the arguments of an arrow function with parenthesis
> Style: extra space
> ```
>     let interceptRequestWithResponseResult = new Promise((resolve) => callback = resolve);
> ```

Done.

>> LayoutTests/http/tests/inspector/network/intercept-aborted-request.html:60
>> +            const error = await interceptRequestWithResponseResult;
> 
> Style: we normally only use `const` for things that are truly constant

Done.

>> LayoutTests/http/tests/inspector/network/intercept-aborted-request.html:69
>> +        async test() {
> 
> Ditto (:36)

Done.

>> LayoutTests/http/tests/inspector/network/intercept-aborted-request.html:99
>> +<script src="resources/override.js"></script>
> 
> Is this needed for this test?

Done.

>> LayoutTests/http/tests/inspector/network/intercept-request-continue.html:16
>> +    let target = WI.assumingMainTarget();
> 
> Ditto (intercept-aborted-request.html:21)

Done.

>> LayoutTests/http/tests/inspector/network/intercept-request-continue.html:21
>> +            await target.NetworkAgent.addInterception.invoke({
> 
> Ditto (intercept-aborted-request.html:26)

Done.

>> LayoutTests/http/tests/inspector/network/intercept-request-continue.html:53
>> +        async test() {
> 
> Ditto (intercept-aborted-request.html:36)

Done.

>> LayoutTests/http/tests/inspector/network/intercept-request-continue.html:80
>> +        async test() {
> 
> Ditto (intercept-aborted-request.html:36)

Done.

>> LayoutTests/http/tests/inspector/network/intercept-request-main-resource-with-response.html:15
>> +            let target = WI.assumingMainTarget();
> 
> Ditto (intercept-aborted-request.html:21)

Done.

>> LayoutTests/http/tests/inspector/network/intercept-request-main-resource-with-response.html:21
>> +            let interceptWithResponse = (event) => {
> 
> Why create a variable instead of inlining this in the `addEventListener` call?

Done. Somehow I think I've done it based on your request in this review.Done.

>> LayoutTests/http/tests/inspector/network/intercept-request-main-resource-with-response.html:33
>> +            WI.networkManager.addEventListener(WI.NetworkManager.Event.RequestIntercepted, interceptWithResponse);
> 
> Please use either `awaitEvent` (and a `Promise`) or `singleFireEventListener` if you're not going to remove the event listener before this test cases finishes.  It _shouldn't_ be a problem to leave the event listener like this, but there have been issues with how state is reset between tests in the past so i'd like to avoid exacerbating any possible issues by leaving event listeners around.

Done.

>> LayoutTests/http/tests/inspector/network/intercept-request-main-resource-with-response.html:35
>> +            InspectorTest.reloadPage({ignoreCache: false, revalidateAllResources: true});
> 
> Please `await` this.  Because this test case is `async`, I can imagine this failing (being flaky) if there is too much time between this returning and the `WI.NetworkManager.Event.RequestIntercepted` being fired.

Done.

>> LayoutTests/http/tests/inspector/network/intercept-request-main-resource.html:15
>> +            let target = WI.assumingMainTarget();
> 
> Ditto (intercept-aborted-request.html:21)

Done.

>> LayoutTests/http/tests/inspector/network/intercept-request-main-resource.html:21
>> +            let interceptWithRequest = (event) => {
> 
> Ditto (intercept-request-main-resource-with-response.html:21)

Done.

>> LayoutTests/http/tests/inspector/network/intercept-request-main-resource.html:25
>> +                    requestId
> 
> Style: missing trailing comma

Done.

>> LayoutTests/http/tests/inspector/network/intercept-request-main-resource.html:28
>> +            WI.networkManager.addEventListener(WI.NetworkManager.Event.RequestIntercepted, interceptWithRequest);
> 
> Ditto (intercept-request-main-resource-with-response.html:33)

Done.

>> LayoutTests/http/tests/inspector/network/intercept-request-main-resource.html:30
>> +            InspectorTest.reloadPage({ignoreCache: false, revalidateAllResources: true});
> 
> Ditto (intercept-request-main-resource-with-response.html:35)

Done.

>> LayoutTests/http/tests/inspector/network/intercept-request-properties.html:12
>> +    data["responseURL"] = response.url;
> 
> `data.responseURL`?

Done.

>> LayoutTests/http/tests/inspector/network/intercept-request-properties.html:21
>> +            "Content-Type": "application/x-www-form-urlencoded"
> 
> Ditto (intercept-request-main-resource.html:25)

Done.

>> LayoutTests/http/tests/inspector/network/intercept-request-properties.html:23
>> +        body: "value=original"
> 
> Ditto (intercept-request-main-resource.html:25)

Done.

>> LayoutTests/http/tests/inspector/network/intercept-request-properties.html:27
>> +    data["responseURL"] = response.url;
> 
> Ditto (:12)

Done.

>> LayoutTests/http/tests/inspector/network/intercept-request-properties.html:34
>> +    let target = WI.assumingMainTarget();
> 
> Ditto (intercept-aborted-request.html:21)

Done.

>> LayoutTests/http/tests/inspector/network/intercept-request-properties.html:39
>> +            await target.NetworkAgent.addInterception.invoke({
> 
> Ditto (intercept-aborted-request.html:26)

Done.

>> LayoutTests/http/tests/inspector/network/intercept-request-properties.html:91
>> +            async test() {
> 
> Ditto (intercept-aborted-request.html:36)

Done. Note that I'd need to await the evaluate result anyways.

>> LayoutTests/http/tests/inspector/network/intercept-request-properties.html:134
>> +        description: "Tests request method interception with NONSTANDARD",
> 
> oops

Done

>> LayoutTests/http/tests/inspector/network/intercept-request-properties.html:140
>> +        name: "Network.interceptRequest.Url",
> 
> NIT: s/Url/URL

Done.

>> LayoutTests/http/tests/inspector/network/intercept-request-properties.html:147
>> +        name: "Network.interceptRequest.UrlFragment",
> 
> Ditto (:140)

Done.

>> LayoutTests/http/tests/inspector/network/intercept-request-properties.html:150
>> +        overrides: { url: "http://127.0.0.1:8000/inspector/network/resources/intercept-echo.php#fragment" },
> 
> Can we add a test (or modify these tests) to check that the URL of the corresponding `WI.Resource` response matches?  I'd like to test/confirm that fragments eventually make it back to the response (or if not to make sure that it stays that way).

I already print the Response URL and it does not have fragment.

>> LayoutTests/http/tests/inspector/network/intercept-request-properties.html:154
>> +        name: "Network.interceptRequest.UrlEmpty",
> 
> Ditto (:140)

Done.

>> LayoutTests/http/tests/inspector/network/intercept-request-subresource-with-error.html:15
>> +            let target = WI.assumingMainTarget();
> 
> Ditto (intercept-aborted-request.html:21)

Done.

>> LayoutTests/http/tests/inspector/network/intercept-request-subresource-with-error.html:21
>> +            let interceptWithRequest = (event) => {
> 
> Ditto (intercept-request-main-resource-with-response.html:21)

Done.

>> LayoutTests/http/tests/inspector/network/intercept-request-subresource-with-error.html:25
>> +                    errorType: InspectorBackend.Enum.Network.ResourceErrorType.General,
> 
> Can we test that this error is actually used?  AFAICT, the only way to confirm that this test is actually working is the fact that there is only one ALERT, which is not very immediately identifiable.  We should be able to check that the `override.js` resource actually errored and that the error matches what we want.

Done.

>> LayoutTests/http/tests/inspector/network/intercept-request-subresource-with-error.html:28
>> +            WI.networkManager.addEventListener(WI.NetworkManager.Event.RequestIntercepted, interceptWithRequest);
> 
> Ditto (intercept-request-main-resource-with-response.html:33)

Done.

>> LayoutTests/http/tests/inspector/network/intercept-request-subresource-with-response.html:15
>> +            let target = WI.assumingMainTarget();
> 
> Ditto (intercept-aborted-request.html:21)

Done.

>> LayoutTests/http/tests/inspector/network/intercept-request-subresource-with-response.html:22
>> +            let interceptWithRequest = (event) => {
> 
> Ditto (intercept-request-main-resource-with-response.html:21)

Done.

>> LayoutTests/http/tests/inspector/network/intercept-request-subresource-with-response.html:34
>> +            WI.networkManager.addEventListener(WI.NetworkManager.Event.RequestIntercepted, interceptWithRequest);
> 
> Ditto (intercept-request-main-resource-with-response.html:33)

Done.

>> LayoutTests/http/tests/inspector/network/intercept-request-subresource.html:15
>> +            let target = WI.assumingMainTarget();
> 
> Ditto (intercept-aborted-request.html:21)

Done.

>> LayoutTests/http/tests/inspector/network/intercept-request-subresource.html:20
>> +            let interceptWithRequest = (event) => {
> 
> Ditto (intercept-request-main-resource-with-response.html:21)

Done.

>> LayoutTests/http/tests/inspector/network/intercept-request-subresource.html:24
>> +                    requestId
> 
> Ditto (intercept-request-main-resource.html:25)

Done.

>> LayoutTests/http/tests/inspector/network/intercept-request-subresource.html:27
>> +            WI.networkManager.addEventListener(WI.NetworkManager.Event.RequestIntercepted, interceptWithRequest);
> 
> Ditto (intercept-request-main-resource-with-response.html:33)

Done.

>> LayoutTests/http/tests/inspector/network/intercept-request-with-response.html:21
>> +            headers: [...response.headers]
> 
> `Array.from`?
> Ditto (intercept-request-main-resource.html:25)

Done.

>> LayoutTests/http/tests/inspector/network/intercept-request-with-response.html:30
>> +    InspectorTest.debug();
> 
> oops

Done.

>> LayoutTests/http/tests/inspector/network/intercept-request-with-response.html:32
>> +    let target = WI.assumingMainTarget();
> 
> Ditto (intercept-aborted-request.html:21)

Done.

>> LayoutTests/http/tests/inspector/network/intercept-request-with-response.html:37
>> +            await target.NetworkAgent.addInterception.invoke({
> 
> Ditto (intercept-aborted-request.html:26)

Done.

>> LayoutTests/http/tests/inspector/network/intercept-request-with-response.html:50
>> +        let keys = [...response.headers.keys()];
> 
> Ditto (:21)

Done.

>> LayoutTests/http/tests/inspector/network/intercept-request-with-response.html:64
>> +            async test() {
> 
> Ditto (intercept-aborted-request.html:36)

Done.

>> LayoutTests/http/tests/inspector/network/intercept-request-with-response.html:148
>> +        expression: `triggerOverrideLoad("#frag")`,
> 
> Ditto (intercept-request-properties.html:150)

Adding intercept-request-fragment.html

[Pavel Feldman]

Added new intercept-request-fragment.html to cover fragments and re-implemented intercept-request-subresource-with-error.html to examine failure descriptions.

[Pavel Feldman]

Created attachment 401876 [details]
Patch

[Pavel Feldman]

Created attachment 401878 [details]
Patch

[Devin Rousso]

Comment on attachment 401878 [details]
Patch

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

r=me, nice work!  thanks for iterating and adding the tests :)

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1126
> +        return;
> +    }

Style: please add a newline after control flow with a `return`

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1179
> +        return;
> +    }

Style: please add a newline after control flow with a `return`

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1180
> +    auto& loader = *pendingRequest->m_loader.get();

NIT: the `.get()` is unnecessary

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1200
> +            return;
> +        }

Style: please add a newline after control flow with a `return`

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1203
> +    // FIXME: figure out how to identify when a request has been overridden when we add this to the frontend.

thanks!

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1259
> +    RefPtr<ResourceLoader> loader = pendingRequest->m_loader.get();

(In reply to Pavel Feldman from comment #40)
> 
> View in context: https://bugs.webkit.org/attachment.cgi?id=401648&action=review
> 
>>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1259
>>> +    RefPtr<ResourceLoader> loader = pendingRequest->m_loader.get();
>> 
>> Ditto (:1177)
> 
> Can't do it here - I'm retaining this refprt.

Ah I see.  Well then since you're explicitly retaining this, I'd suggest adding a comment as such so it's clear if someone comes along later and is unfamiliar with this code :)

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1260
> +    if (!loader->identifier()) {

Should this be `reachedTerminalState()`?

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1271
> +            return;
> +        }

Style: please add a newline after control flow with a `return`

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1304
> +    auto& loader = *pendingRequest->m_loader.get();

Ditto (:1180)

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1332
> +    default:
> +        error = ResourceError(errorDomainWebKitInternal, 0, loader.url(), "Request intercepted"_s, ResourceError::Type::General);
> +        break;

(In reply to Pavel Feldman from comment #40)
> 
> View in context: https://bugs.webkit.org/attachment.cgi?id=401648&action=review
> 
>>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1332
>>> +        return;
>> 
>> NIT: is this actually necessary?  i would think not because the `switch` handles all of the `enum` values 🤔
> 
> It handles all, but I will remove the assertion and default to general.

I was more asking whether a `default` is needed at all.  `Inspector::Protocol::Network::ResourceErrorType` should be an `enum class` and you've added a `case` for each value so I don't think we even need a `default` at all.  Am I missing something?

> LayoutTests/http/tests/inspector/network/intercept-request-continue.html:37
> +            let response = await RuntimeAgent.evaluate("fetchData('resources/script.js')");
> +            response = await RuntimeAgent.awaitPromise(response.result.objectId, true);

NIT: we don't normally reassign to the same variable as it makes it harder to introspect values at the end, but I get what you're going here (and the tests pass :P ) so perhaps it's fine :)

> LayoutTests/http/tests/inspector/network/intercept-request-continue.html:75
> +                    requestId: 'wrongId',

Style: please use double quotes

> LayoutTests/http/tests/inspector/network/intercept-request-fragment.html:19
> +    let suite = InspectorTest.createAsyncSuite("Network.interceptWithRequest");
> +    NetworkAgent.addInterception.invoke({

Style: I'd add a newline between since these lines are unrelated

> LayoutTests/http/tests/inspector/network/intercept-request-fragment.html:24
> +    function addBaselineTestCase({name, description, expression, setup, teardown}) {

NIT: none of the callers actually use `setup` or `teardown`, so can we remove them?

> LayoutTests/http/tests/inspector/network/intercept-request-fragment.html:26
> +            name, description, setup, teardown,

Style: for multi-line objects, we don't do partial inlining, so please put these on separate lines (for readability too)

> LayoutTests/http/tests/inspector/network/intercept-request-fragment.html:32
> +                    RuntimeAgent.evaluate(expression),

NIT: we normally use `InspectorTest.evaluateInPage` for things like this as it also handles if `wasThrown` is `true` (which would NOT reject `Runtime.evaluate`), but they both do similar things so this is fine if you'd prefer it

> LayoutTests/http/tests/inspector/network/intercept-request-fragment.html:34
> +                InspectorTest.log('Request URL: ' + requestEvent.data.resource.url);

Style: please use double quotes

> LayoutTests/http/tests/inspector/network/intercept-request-fragment.html:35
> +                InspectorTest.log('Response URL: ' + responseEvent.target.url);

Style: please use double quotes

> LayoutTests/http/tests/inspector/network/intercept-request-fragment.html:39
> +    function addTestCase({name, description, expression, setup, teardown, url}) {

NIT: none of the callers actually use `setup` or `teardown`, so can we remove them?

> LayoutTests/http/tests/inspector/network/intercept-request-fragment.html:41
> +            name, description, setup, teardown,

Style: for multi-line objects, we don't do partial inlining, so please put these on separate lines (for readability too)

> LayoutTests/http/tests/inspector/network/intercept-request-fragment.html:49
> +                InspectorTest.log('Request URL: ' + requestEvent.data.resource.url);

Style: please use double quotes

> LayoutTests/http/tests/inspector/network/intercept-request-fragment.html:58
> +                InspectorTest.log('Response URL: ' + responseEvent.target.url);

Style: please use double quotes

> LayoutTests/http/tests/inspector/network/intercept-request-properties.html:12
> +        let data = eval('(' + (await response.text() || "{}") + ')');

Style: please use double quotes

> LayoutTests/http/tests/inspector/network/intercept-request-properties.html:30
> +    let data = eval('(' + (await response.text() || "{}") + ')');

Style: please use double quotes

> LayoutTests/http/tests/inspector/network/intercept-request-properties.html:49
> +            return;
> +        }

Style: please add a newline after control flow with a `return`

> LayoutTests/http/tests/inspector/network/intercept-request-properties.html:84
> +    function addTestCase({name, description, expression, setup, teardown, overrides}) {

NIT: none of the callers actually use `setup` or `teardown`, so can we remove them?

> LayoutTests/http/tests/inspector/network/intercept-request-properties.html:86
> +            name, description, setup, teardown,

Style: for multi-line objects, we don't do partial inlining, so please put these on separate lines (for readability too)

> LayoutTests/http/tests/inspector/network/intercept-request-subresource-with-error.html:13
> +    link.onerror = () => alert('Error loading stylesheet');

Does this actually add anything?  Logs like these don't really help IMO :(

> LayoutTests/http/tests/inspector/network/intercept-request-subresource-with-error.html:25
> +    function addTestCase({name, description, expression, setup, teardown, errorType}) {

NIT: none of the callers actually use `setup` or `teardown`, so can we remove them?

> LayoutTests/http/tests/inspector/network/intercept-request-subresource-with-error.html:27
> +            name, description, setup, teardown,

Style: for multi-line objects, we don't do partial inlining, so please put these on separate lines (for readability too)

> LayoutTests/http/tests/inspector/network/intercept-request-subresource-with-error.html:29
> +                WI.Resource.singleFireEventListener(WI.Resource.Event.LoadingDidFail, (event) => {

Given that this is expected to fire in order for the test to pass, I feel like we should include this in the `async` flow as otherwise timing flakiness _might_ cause problems.
```
    let [loadingDidFailEvent] = await Promise.all([
        WI.Resource.awaitEvent(WI.Resource.Event.LoadingDidFail),
        NetworkAgent.interceptRequestWithError.invoke({
            requestId: requestInterceptedEvent.data.requestId,
            errorType,
        }),
    ]);

    InspectorTest.log("FAILURE TEXT: " + loadingDidFailEvent.target.failureReasonText);
```

> LayoutTests/http/tests/inspector/network/intercept-request-subresource-with-error.html:30
> +                    InspectorTest.log('FAILURE TEXT: ' + event.target.failureReasonText);

Style: please use double quotes

> LayoutTests/http/tests/inspector/network/intercept-request-subresource-with-error.html:35
> +                    RuntimeAgent.evaluate('loadSubresource()'),

Style: please use double quotes

> LayoutTests/http/tests/inspector/network/intercept-request-with-response.html:54
> +    function addTestCase({name, description, setup, teardown, expression, responseData}) {

NIT: none of the callers actually use `setup` or `teardown`, so can we remove them?

> LayoutTests/http/tests/inspector/network/intercept-request-with-response.html:56
> +            name, description, setup, teardown,

Style: for multi-line objects, we don't do partial inlining, so please put these on separate lines (for readability too)

[Pavel Feldman]

Created attachment 401935 [details]
Patch

[Pavel Feldman]

Comment on attachment 401878 [details]
Patch

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

>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1179
>> +    }
> 
> Style: please add a newline after control flow with a `return`

Done.

>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1180
>> +    auto& loader = *pendingRequest->m_loader.get();
> 
> NIT: the `.get()` is unnecessary

Done.

>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1200
>> +        }
> 
> Style: please add a newline after control flow with a `return`

Done.

>>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1259
>>> +    RefPtr<ResourceLoader> loader = pendingRequest->m_loader.get();
>> 
>> (In reply to Pavel Feldman from comment #40)
> 
> Ah I see.  Well then since you're explicitly retaining this, I'd suggest adding a comment as such so it's clear if someone comes along later and is unfamiliar with this code :)

Done.

>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1260
>> +    if (!loader->identifier()) {
> 
> Should this be `reachedTerminalState()`?

Done. Hm, I thought it was all.

>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1271
>> +        }
> 
> Style: please add a newline after control flow with a `return`

Done.

>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1304
>> +    auto& loader = *pendingRequest->m_loader.get();
> 
> Ditto (:1180)

Done.

>>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1332
>>> +        break;
>> 
>> (In reply to Pavel Feldman from comment #40)
> 
> I was more asking whether a `default` is needed at all.  `Inspector::Protocol::Network::ResourceErrorType` should be an `enum class` and you've added a `case` for each value so I don't think we even need a `default` at all.  Am I missing something?

I didn't want someone who adds a new literal into the ResourceError to be blocked with this code. General should work just fine for them until inspector has a reason to differentiate the new outcome.

>> LayoutTests/http/tests/inspector/network/intercept-request-continue.html:75
>> +                    requestId: 'wrongId',
> 
> Style: please use double quotes

Done.

>> LayoutTests/http/tests/inspector/network/intercept-request-fragment.html:19
>> +    NetworkAgent.addInterception.invoke({
> 
> Style: I'd add a newline between since these lines are unrelated

Done.

>> LayoutTests/http/tests/inspector/network/intercept-request-fragment.html:24
>> +    function addBaselineTestCase({name, description, expression, setup, teardown}) {
> 
> NIT: none of the callers actually use `setup` or `teardown`, so can we remove them?

Done.

>> LayoutTests/http/tests/inspector/network/intercept-request-fragment.html:26
>> +            name, description, setup, teardown,
> 
> Style: for multi-line objects, we don't do partial inlining, so please put these on separate lines (for readability too)

Done. This was taken from local-resource-overrides!

>> LayoutTests/http/tests/inspector/network/intercept-request-fragment.html:32
>> +                    RuntimeAgent.evaluate(expression),
> 
> NIT: we normally use `InspectorTest.evaluateInPage` for things like this as it also handles if `wasThrown` is `true` (which would NOT reject `Runtime.evaluate`), but they both do similar things so this is fine if you'd prefer it

Thanks!

>> LayoutTests/http/tests/inspector/network/intercept-request-fragment.html:34
>> +                InspectorTest.log('Request URL: ' + requestEvent.data.resource.url);
> 
> Style: please use double quotes

Done.

>> LayoutTests/http/tests/inspector/network/intercept-request-fragment.html:35
>> +                InspectorTest.log('Response URL: ' + responseEvent.target.url);
> 
> Style: please use double quotes

Done.

>> LayoutTests/http/tests/inspector/network/intercept-request-fragment.html:39
>> +    function addTestCase({name, description, expression, setup, teardown, url}) {
> 
> NIT: none of the callers actually use `setup` or `teardown`, so can we remove them?

Done.

>> LayoutTests/http/tests/inspector/network/intercept-request-fragment.html:41
>> +            name, description, setup, teardown,
> 
> Style: for multi-line objects, we don't do partial inlining, so please put these on separate lines (for readability too)

Done.

>> LayoutTests/http/tests/inspector/network/intercept-request-fragment.html:49
>> +                InspectorTest.log('Request URL: ' + requestEvent.data.resource.url);
> 
> Style: please use double quotes

Done.

>> LayoutTests/http/tests/inspector/network/intercept-request-fragment.html:58
>> +                InspectorTest.log('Response URL: ' + responseEvent.target.url);
> 
> Style: please use double quotes

Done.

>> LayoutTests/http/tests/inspector/network/intercept-request-properties.html:30
>> +    let data = eval('(' + (await response.text() || "{}") + ')');
> 
> Style: please use double quotes

Done.

>> LayoutTests/http/tests/inspector/network/intercept-request-properties.html:49
>> +        }
> 
> Style: please add a newline after control flow with a `return`

Done.

>> LayoutTests/http/tests/inspector/network/intercept-request-properties.html:84
>> +    function addTestCase({name, description, expression, setup, teardown, overrides}) {
> 
> NIT: none of the callers actually use `setup` or `teardown`, so can we remove them?

Done.

>> LayoutTests/http/tests/inspector/network/intercept-request-properties.html:86
>> +            name, description, setup, teardown,
> 
> Style: for multi-line objects, we don't do partial inlining, so please put these on separate lines (for readability too)

Done.

>> LayoutTests/http/tests/inspector/network/intercept-request-subresource-with-error.html:13
>> +    link.onerror = () => alert('Error loading stylesheet');
> 
> Does this actually add anything?  Logs like these don't really help IMO :(

We check that it fires on the user-land level. Does not hurt, could catch a real related bug though.

>> LayoutTests/http/tests/inspector/network/intercept-request-subresource-with-error.html:25
>> +    function addTestCase({name, description, expression, setup, teardown, errorType}) {
> 
> NIT: none of the callers actually use `setup` or `teardown`, so can we remove them?

Done.

>> LayoutTests/http/tests/inspector/network/intercept-request-subresource-with-error.html:27
>> +            name, description, setup, teardown,
> 
> Style: for multi-line objects, we don't do partial inlining, so please put these on separate lines (for readability too)

Done.

>> LayoutTests/http/tests/inspector/network/intercept-request-subresource-with-error.html:29
>> +                WI.Resource.singleFireEventListener(WI.Resource.Event.LoadingDidFail, (event) => {
> 
> Given that this is expected to fire in order for the test to pass, I feel like we should include this in the `async` flow as otherwise timing flakiness _might_ cause problems.
> ```
>     let [loadingDidFailEvent] = await Promise.all([
>         WI.Resource.awaitEvent(WI.Resource.Event.LoadingDidFail),
>         NetworkAgent.interceptRequestWithError.invoke({
>             requestId: requestInterceptedEvent.data.requestId,
>             errorType,
>         }),
>     ]);
> 
>     InspectorTest.log("FAILURE TEXT: " + loadingDidFailEvent.target.failureReasonText);
> ```

Done

>> LayoutTests/http/tests/inspector/network/intercept-request-subresource-with-error.html:30
>> +                    InspectorTest.log('FAILURE TEXT: ' + event.target.failureReasonText);
> 
> Style: please use double quotes

Done.

>> LayoutTests/http/tests/inspector/network/intercept-request-subresource-with-error.html:35
>> +                    RuntimeAgent.evaluate('loadSubresource()'),
> 
> Style: please use double quotes

Done.

>> LayoutTests/http/tests/inspector/network/intercept-request-with-response.html:54
>> +    function addTestCase({name, description, setup, teardown, expression, responseData}) {
> 
> NIT: none of the callers actually use `setup` or `teardown`, so can we remove them?

Done.

>> LayoutTests/http/tests/inspector/network/intercept-request-with-response.html:56
>> +            name, description, setup, teardown,
> 
> Style: for multi-line objects, we don't do partial inlining, so please put these on separate lines (for readability too)

Done.

[Devin Rousso]

Comment on attachment 401878 [details]
Patch

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

>>>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1332
>>>> +        break;
>>> 
>>> (In reply to Pavel Feldman from comment #40)
>> 
>> I was more asking whether a `default` is needed at all.  `Inspector::Protocol::Network::ResourceErrorType` should be an `enum class` and you've added a `case` for each value so I don't think we even need a `default` at all.  Am I missing something?
> 
> I didn't want someone who adds a new literal into the ResourceError to be blocked with this code. General should work just fine for them until inspector has a reason to differentiate the new outcome.

You're switching on `Inspector::Protocol::Network::ResourceErrorType` tho, so why would a change to `WebCore::ResourceError` matter?  If a developer is changing `Inspector::Protocol::Network::ResourceErrorType` I would _want_ them to be blocked on this code, as I'd want them to add logic to handle their changes.

>>> LayoutTests/http/tests/inspector/network/intercept-request-subresource-with-error.html:13
>>> +    link.onerror = () => alert('Error loading stylesheet');
>> 
>> Does this actually add anything?  Logs like these don't really help IMO :(
> 
> We check that it fires on the user-land level. Does not hurt, could catch a real related bug though.

Can we include some other information then so we don't have four identical "Error loading stylesheet" logs?  I'm just trying to make this more usable/actionable if this test becomes flakey in the future :)

[Pavel Feldman]

Created attachment 401940 [details]
Patch

[Pavel Feldman]

Created attachment 401941 [details]
Patch

[Devin Rousso]

Comment on attachment 401941 [details]
Patch

r=me

[EWS]

Committed r263072: <https://trac.webkit.org/changeset/263072>

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

[Radar WebKit Bug Importer]

<rdar://problem/64388297>

[Jason Lawrence]

There are debug assert crashes associated with this commit which are tracked here: 
https://bugs.webkit.org/show_bug.cgi?id=213252