Bug 162281 – [Fetch API] Support Request cache mode

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[youenn fablet]

We should support cache mode.

[youenn fablet]

Created attachment 289358 [details]
Patch

[Build Bot]

Comment on attachment 289358 [details]
Patch

Attachment 289358 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2113101

New failing tests:
imported/w3c/web-platform-tests/fetch/api/request/request-error.html

[Build Bot]

Created attachment 289373 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 289358 [details]
Patch

Attachment 289358 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2113094

New failing tests:
imported/w3c/web-platform-tests/fetch/api/request/request-error.html

[Build Bot]

Created attachment 289374 [details]
Archive of layout-test-results from ews112 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 289358 [details]
Patch

Attachment 289358 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2113172

New failing tests:
imported/w3c/web-platform-tests/fetch/api/request/request-error.html

[Build Bot]

Created attachment 289376 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 289358 [details]
Patch

Attachment 289358 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2113183

New failing tests:
imported/w3c/web-platform-tests/fetch/api/request/request-error.html

[Build Bot]

Created attachment 289378 [details]
Archive of layout-test-results from ews122 for ios-simulator-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-elcapitan-wk2  Platform: Mac OS X 10.11.6

[youenn fablet]

Created attachment 289448 [details]
Patch

[youenn fablet]

Created attachment 289456 [details]
Removing WK2 changes

[youenn fablet]

Comment on attachment 289456 [details]
Removing WK2 changes

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

> Source/WebCore/loader/FrameLoader.cpp:2564
> +ResourceRequestCachePolicy FrameLoader::defaultRequestCachingPolicy(const ResourceRequest& request, FrameLoadType loadType, bool isMainResource)

The integration of fetch tasks with navigation/frame load tasks is far from perfect and makes the loading code difficult to grasp.
Future refactoring steps should be done to clarify this, without breaking behavior...

> LayoutTests/imported/w3c/web-platform-tests/fetch/api/request/request-cache-expected.txt:99
> +FAIL RequestCache "reload" mode does store the response in the cache with date and fresh response assert_equals: expected 1 but got 2

These two tests are not passing as the CFNetwork cache does not contain the reloaded entry.
The reason is unclear to me and will need further investigations.

> LayoutTests/platform/wk2/TestExpectations:694
> +imported/w3c/web-platform-tests/fetch/api/request/request-cache.html [ Skip ]

We skip it for the moment as wk2 test result is different from wk1.
In particular, some console log messages are added and they change for every run. We need something like bug 161310 to land first.
We also need network cache to be skipped in case request cache policy is reload.

[youenn fablet]

Ping review?

[Brady Eidson]

(In reply to comment #12)
> Comment on attachment 289456 [details]
> Removing WK2 changes
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=289456&action=review
> 
> > Source/WebCore/loader/FrameLoader.cpp:2564
> > +ResourceRequestCachePolicy FrameLoader::defaultRequestCachingPolicy(const ResourceRequest& request, FrameLoadType loadType, bool isMainResource)
> 
> The integration of fetch tasks with navigation/frame load tasks is far from
> perfect and makes the loading code difficult to grasp.
> Future refactoring steps should be done to clarify this, without breaking
> behavior...
> 
> > LayoutTests/imported/w3c/web-platform-tests/fetch/api/request/request-cache-expected.txt:99
> > +FAIL RequestCache "reload" mode does store the response in the cache with date and fresh response assert_equals: expected 1 but got 2
> 
> These two tests are not passing as the CFNetwork cache does not contain the
> reloaded entry.
> The reason is unclear to me and will need further investigations.

We don't use the CFNetwork cache in WK2. How is it that this expectation change applies to both WK1 and WK2?

> 
> > LayoutTests/platform/wk2/TestExpectations:694
> > +imported/w3c/web-platform-tests/fetch/api/request/request-cache.html [ Skip ]
> 
> We skip it for the moment as wk2 test result is different from wk1.
> In particular, some console log messages are added and they change for every
> run. We need something like bug 161310 to land first.
> We also need network cache to be skipped in case request cache policy is
> reload.

Where is the bug to re-enable once 161310 lands, or something else allows re-enabling?
We tend to skip a lot of tests without tracking re-enabling them later, which is bad.

[Brady Eidson]

Comment on attachment 289456 [details]
Removing WK2 changes

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

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:605
> +static inline void updateRequestAccordingCacheMode(CachedResourceRequest& request)

Are all of the options in here from the spec?

If not, where are they from?

[youenn fablet]

Comment on attachment 289456 [details]
Removing WK2 changes

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

>>> Source/WebCore/loader/FrameLoader.cpp:2564
>>> +ResourceRequestCachePolicy FrameLoader::defaultRequestCachingPolicy(const ResourceRequest& request, FrameLoadType loadType, bool isMainResource)
>> 
>> The integration of fetch tasks with navigation/frame load tasks is far from perfect and makes the loading code difficult to grasp.
>> Future refactoring steps should be done to clarify this, without breaking behavior...
> 
> We don't use the CFNetwork cache in WK2. How is it that this expectation change applies to both WK1 and WK2?

I am not sure to understand your question.
In WK2, the network session cache is using the request policy cache to decide whether to serve content or not from it.
The results for request-cache.html being different between WK1 and WK2 implementation and request-cache.html being flaky for WK2, we rebase request-cache.html expectation using WK1 implementation results.

>> Source/WebCore/loader/cache/CachedResourceLoader.cpp:605
>> +static inline void updateRequestAccordingCacheMode(CachedResourceRequest& request)
> 
> Are all of the options in here from the spec?
> 
> If not, where are they from?

The cache options are defined at https://fetch.spec.whatwg.org/#concept-request-cache-mode
This specific processing is defined in steps 10-12 of https://fetch.spec.whatwg.org/#http-network-or-cache-fetch

>>> LayoutTests/platform/wk2/TestExpectations:694
>>> +imported/w3c/web-platform-tests/fetch/api/request/request-cache.html [ Skip ]
>> 
>> We skip it for the moment as wk2 test result is different from wk1.
>> In particular, some console log messages are added and they change for every run. We need something like bug 161310 to land first.
>> We also need network cache to be skipped in case request cache policy is reload.
> 
> Where is the bug to re-enable once 161310 lands, or something else allows re-enabling?
> We tend to skip a lot of tests without tracking re-enabling them later, which is bad.

I can create a bug for it specifically at landing time, but this can be added to bug 161310 as well if this patch lands first.

In lieu of skipping this test, I can make it [Pass Failure], this would check for unexpected crash/timeout.

Also, it is not only about reactivating this test but also to pass all tests of request-cache.html.
For instance, we should be able to bypass WK2 network session cache entirely which is not supported yet.
I plan to work on that once this patch lands

[youenn fablet]

Also bug 159683 is tracking request-cache.html specifically.

[youenn fablet]

Ping review?

[Alex Christensen]

Comment on attachment 289456 [details]
Removing WK2 changes

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

r=me.  This is a big progression with some things that will be further investigated.

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:624
> +        request.mutableResourceRequest().addHTTPHeaderFieldIfNotPresent(HTTPHeaderName::Pragma, ASCIILiteral("no-cache"));
> +        request.mutableResourceRequest().addHTTPHeaderFieldIfNotPresent(HTTPHeaderName::CacheControl, ASCIILiteral("no-cache"));

Strings can go in HTTPHeaderValues.h, right?

[youenn fablet]

Created attachment 291246 [details]
Patch for landing

[youenn fablet]

Thanks for the review.

(In reply to comment #19)
> Comment on attachment 289456 [details]
> Removing WK2 changes
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=289456&action=review
> 
> r=me.  This is a big progression with some things that will be further
> investigated.
> 
> > Source/WebCore/loader/cache/CachedResourceLoader.cpp:624
> > +        request.mutableResourceRequest().addHTTPHeaderFieldIfNotPresent(HTTPHeaderName::Pragma, ASCIILiteral("no-cache"));
> > +        request.mutableResourceRequest().addHTTPHeaderFieldIfNotPresent(HTTPHeaderName::CacheControl, ASCIILiteral("no-cache"));
> 
> Strings can go in HTTPHeaderValues.h, right?

Yep.

[WebKit Commit Bot]

Comment on attachment 291246 [details]
Patch for landing

Clearing flags on attachment: 291246

Committed r207086: <http://trac.webkit.org/changeset/207086>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[youenn fablet]

WebKit2 follow-up patch is tracked by bug 163332