[webkit-reviews] review requested: [Bug 199499] Make storing cross-origin top-level prefetches in HTTP cache optional : [Attachment 374596] Patch

[bugzilla-daemon at webkit.org]

Rob Buis <rbuis at igalia.com> has asked  for review:
Bug 199499: Make storing cross-origin top-level prefetches in HTTP cache
optional
https://bugs.webkit.org/show_bug.cgi?id=199499

Attachment 374596: Patch

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




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

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

>> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:220
>>		if (auto entry = session->prefetchCache().take(request.url()))
{
> 
> Let's add a FIXME with the bugzilla link to do some validation for the
prefetch entry, for instance the Vary header.

Done.

>> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:223
>> +			loader->retrieveCacheEntryInternal(WTFMove(cacheEntry),
ResourceRequest {request});
> 
> s/ {request}/ { request }/

Done.

>> LayoutTests/http/tests/cache/resources/prefetched-main-resource-iframe.php:3
>>	header("Access-Control-Allow-Origin: http://127.0.0.1:8000");
> 
> Why do we need Access-Control-Allow-Origin header for the prefetch case?

You are right, I removed it.

>> LayoutTests/http/tests/cache/resources/prefetched-main-resource.php:3
>> +header("Access-Control-Allow-Origin: http://127.0.0.1:8000");
> 
> Why do we need ACAO here?
> Isn't prefetched-main-resource.php be loaded using
http://localhost8000/cache/resources/prefetched-main-resource.php URL?

Ditto.

>> LayoutTests/http/tests/cache/resources/prefetched-main-resource.php:13
>> +	  document.getElementById('log').innerText = 'FAIL';
> 
> Maybe we should make sure to have this 'FAIL' be different from below 'FAIL'.
> Change it to 'FAIL: resource is not in the disk cache.' might help debugging.

Done.

>> LayoutTests/http/wpt/prefetch/resources/main-resource-skip-disk-cache.py:2
>> +	headers = [("Content-Type", "text/html")]
> 
> This might depend on how we do cache validation, setting Cache-Control:
no-store would probably defeat using the prefetch.
> In that case, the prefetch code should probably cancel the load whenever
receiving the response.

Do you mean, when a prefetch gets a response with Cache-Control: no-store, it
should cancel the prefetch?

>> LayoutTests/http/wpt/prefetch/resources/main-resource-skip-disk-cache.py:7
>> +  var ret = '%(prefetch)s';
> 
> s/ret/result/

Done.

>> LayoutTests/http/wpt/prefetch/resources/main-resource-skip-disk-cache.py:9
>> +	fetch('%(url)s').then(function(response) {
> 
> You need to await here so that ret can go to 'FAIL'.
> Otherwise you will call postMessage and fetch will then change ret.

Done.

>> LayoutTests/http/wpt/prefetch/resources/navigate-skip-disk-cache.html:10
>> +  setTimeout(() => { window.location = get_host_info().HTTP_REMOTE_ORIGIN +
"/WebKit/prefetch/resources/main-resource-skip-disk-cache.py" }, 500);
> 
> If we would like to move away from 500, one possibility would be to add an
Internals API that would tell when the link prefetch is done.

I added such an API. Unfortunately that makes it WPT incompatible. Still not
sue how to fix that, WebDriver maybe?