[webkit-reviews] review denied: [Bug 207446] Web Inspector: request interception : [Attachment 401648] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Jun 12 15:28:39 PDT 2020
Devin Rousso <drousso at apple.com> has denied Pavel Feldman
<pfeldman at chromium.org>'s request for review:
Bug 207446: Web Inspector: request interception
https://bugs.webkit.org/show_bug.cgi?id=207446
Attachment 401648: Patch
https://bugs.webkit.org/attachment.cgi?id=401648&action=review
--- Comment #38 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 401648
--> https://bugs.webkit.org/attachment.cgi?id=401648
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-r
esponse.html:15
> + let target = WI.assumingMainTarget();
Ditto (intercept-aborted-request.html:21)
>
LayoutTests/http/tests/inspector/network/intercept-request-main-resource-with-r
esponse.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-r
esponse.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-r
esponse.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:1
5
> + let target = WI.assumingMainTarget();
Ditto (intercept-aborted-request.html:21)
>
LayoutTests/http/tests/inspector/network/intercept-request-main-resource.html:2
1
> + let interceptWithRequest = (event) => {
Ditto (intercept-request-main-resource-with-response.html:21)
>
LayoutTests/http/tests/inspector/network/intercept-request-main-resource.html:2
5
> + requestId
Style: missing trailing comma
>
LayoutTests/http/tests/inspector/network/intercept-request-main-resource.html:2
8
> +
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:3
0
> + 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-err
or.html:15
> + let target = WI.assumingMainTarget();
Ditto (intercept-aborted-request.html:21)
>
LayoutTests/http/tests/inspector/network/intercept-request-subresource-with-err
or.html:21
> + let interceptWithRequest = (event) => {
Ditto (intercept-request-main-resource-with-response.html:21)
>
LayoutTests/http/tests/inspector/network/intercept-request-subresource-with-err
or.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-err
or.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-res
ponse.html:15
> + let target = WI.assumingMainTarget();
Ditto (intercept-aborted-request.html:21)
>
LayoutTests/http/tests/inspector/network/intercept-request-subresource-with-res
ponse.html:22
> + let interceptWithRequest = (event) => {
Ditto (intercept-request-main-resource-with-response.html:21)
>
LayoutTests/http/tests/inspector/network/intercept-request-subresource-with-res
ponse.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:2
1
> + headers: [...response.headers]
`Array.from`?
Ditto (intercept-request-main-resource.html:25)
>
LayoutTests/http/tests/inspector/network/intercept-request-with-response.html:3
0
> + InspectorTest.debug();
oops
>
LayoutTests/http/tests/inspector/network/intercept-request-with-response.html:3
2
> + let target = WI.assumingMainTarget();
Ditto (intercept-aborted-request.html:21)
>
LayoutTests/http/tests/inspector/network/intercept-request-with-response.html:3
7
> + await target.NetworkAgent.addInterception.invoke({
Ditto (intercept-aborted-request.html:26)
>
LayoutTests/http/tests/inspector/network/intercept-request-with-response.html:5
0
> + let keys = [...response.headers.keys()];
Ditto (:21)
>
LayoutTests/http/tests/inspector/network/intercept-request-with-response.html:6
4
> + async test() {
Ditto (intercept-aborted-request.html:36)
>
LayoutTests/http/tests/inspector/network/intercept-request-with-response.html:1
48
> + expression: `triggerOverrideLoad("#frag")`,
Ditto (intercept-request-properties.html:150)
More information about the webkit-reviews
mailing list