Summary: | [WK2][NetworkCache] We only cache responses with status codes that are cacheable by default | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||||
Component: | WebKit2 | Assignee: | Chris Dumez <cdumez> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | barraclough, commit-queue, koivisto, webkit-bug-importer | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Chris Dumez
2015-03-26 20:01:47 PDT
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. |