We currently only cache responses with status codes that are cacheable by default [1]. However, RFC 7234 [2] allows us to cache responses with other status codes, as long as they have explicit headers allowing caching: - 'Expires' header field - 'max-age' response directive [1] http://tools.ietf.org/html/rfc7231#page-48 [2] http://tools.ietf.org/html/rfc7234#section-4.3.2
<rdar://problem/20321172>
Created attachment 249550 [details] Patch
Created attachment 249551 [details] Patch
Comment on attachment 249551 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=249551&action=review > Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:286 > + // Check for expiration headers allowing us to cache. > + // http://tools.ietf.org/html/rfc7234#section-4.3.2 > + if (std::isfinite(response.expires()) || std::isfinite(response.cacheControlMaxAge())) > + return StoreDecision::Yes; http://tools.ietf.org/html/rfc7234#section-3 A cache MUST NOT store a response to any request, unless: - the response status code is understood by the cache, and - ... To me this sounds like we might still want to have a list of status codes that we "understand" and not just cache everything with >0 cache lifetime specified in headers. Maybe just a list of all valid status codes. > Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:294 > switch (response.httpStatusCode()) { > + // These status codes are cacheable by default: > + // http://tools.ietf.org/html/rfc7231#page-48 > case 200: // OK > case 203: // Non-Authoritative Information > case 204: // No Content > case 300: // Multiple Choices Would be nice to factor this into a function: isResponseCacheableByDefault(). Then you don't need to write a comment explaining what these are about.
Created attachment 249579 [details] Patch
Comment on attachment 249579 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=249579&action=review > Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:345 > + LOG(NetworkCache, "(NetworkProcess) status code %d is not cacheable by default", response.httpStatusCode()); The message is now wrong, you should drop "by default"
Created attachment 249580 [details] Patch
Comment on attachment 249580 [details] Patch Clearing flags on attachment: 249580 Committed r182071: <http://trac.webkit.org/changeset/182071>
All reviewed patches have been landed. Closing bug.