[webkit-reviews] review requested: [Bug 195623] Link prefetch not useful for top-level navigation : [Attachment 366000] Patch

[bugzilla-daemon at webkit.org]

Rob Buis <rbuis at igalia.com> has asked  for review:
Bug 195623: Link prefetch not useful for top-level navigation
https://bugs.webkit.org/show_bug.cgi?id=195623

Attachment 366000: Patch

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




--- Comment #93 from Rob Buis <rbuis at igalia.com> ---
Comment on attachment 366000
  --> https://bugs.webkit.org/attachment.cgi?id=366000
Patch

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

>> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:213
>> +	if (m_parameters.options.mode == FetchOptions::Mode::Navigate) {
> 
> We probably want to restrict to top level navigation for now.

Done.

>> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:768
>> +	}
> 
> We should probably add a check on didReceiveResponse/didReceiveBuffer to not
send data if it is a prefetch request.

Done.

>> LayoutTests/http/tests/cache/link-prefetch-main-resource.html:1
>> +<!-- webkit-test-runner [ experimental:LinkPrefetch=false ] -->
> 
> Should be true?

I think it has no effect, probably since link prefetch is an experimental flag
and will get enabled on test runs by default, so I removed the line.

>> LayoutTests/http/tests/cache/link-prefetch-main-resource.html:10
>> +	internals.settings.setStorageBlockingPolicy('BlockThirdParty')
> 
> Why is it needed?

I think this was needed at some point but it does not have an effect anymore, I
removed it.

>> LayoutTests/http/tests/cache/link-prefetch-main-resource.html:40
>> +<link rel="prefetch"
href="http://localhost:8000/cache/resources/prefetched-main-resource.php"
onload="setTimeout(loadAfterPrefetch, 0);">
> 
> We might want to contribute these tests to WPT maybe.

I hope they are good enough :)

>> LayoutTests/http/tests/cache/resources/prefetched-main-resource.php:4
>> +	header("Access-Control-Allow-Origin: http://127.0.0.1:8000");
> 
> Maybe we should have echo "parent.window.postMessage('PASS', '*');";

Done.

>> LayoutTests/http/tests/cache/resources/prefetched-main-resource.php:12
>> +echo "parent.window.postMessage('nonprefetch', '*');";
> 
> Maybe we should have echo "parent.window.postMessage('FAIL', '*');";

Done.