Bug 162281

Created attachment 289358 [details] Patch

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

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

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

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

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

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

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

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

Created attachment 289448 [details] Patch

Created attachment 289456 [details] Removing WK2 changes

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.

Ping review?

(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.

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?

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

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

Ping review?

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?

Created attachment 291246 [details] Patch for landing

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.

Comment on attachment 291246 [details] Patch for landing Clearing flags on attachment: 291246 Committed r207086: <http://trac.webkit.org/changeset/207086>

All reviewed patches have been landed. Closing bug.

WebKit2 follow-up patch is tracked by bug 163332