[webkit-reviews] review denied: [Bug 207446] Web Inspector: request interception : [Attachment 399243] Patch

Thu May 14 15:57:21 PDT 2020

Devin Rousso <drousso at apple.com> has denied  review:
Bug 207446: Web Inspector: request interception
https://bugs.webkit.org/show_bug.cgi?id=207446

Attachment 399243: Patch

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

--- Comment #15 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 399243
  --> https://bugs.webkit.org/attachment.cgi?id=399243
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.