[webkit-reviews] review granted: [Bug 207446] Web Inspector: request interception : [Attachment 401878] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted 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 401878: Patch

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




--- Comment #44 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 401878
  --> https://bugs.webkit.org/attachment.cgi?id=401878
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-err
or.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-err
or.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-err
or.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-err
or.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-err
or.html:30
> +		       InspectorTest.log('FAILURE TEXT: ' +
event.target.failureReasonText);

Style: please use double quotes

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

Style: please use double quotes

>
LayoutTests/http/tests/inspector/network/intercept-request-with-response.html:5
4
> +    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:5
6
> +	       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)