Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2644063003:
HTTP Cache Age calculation mechanism changed with RFC 7234 (Closed)
DescriptionHTTP Cache Age calculation mechanism changed with RFC 7234
Current cached resource age calculation implementation in chromium references and uses RFC 2616
which has been obsoleted by RFC 7234. The new spec has minor modification with
respect to age calculation ([1] & [2])
Old spec: https://tools.ietf.org/html/rfc2616#section-13.2.3
New spec: http://httpwg.org/specs/rfc7234.html#rfc.section.4.2.3
BUG=682606
Review-Url: https://codereview.chromium.org/2644063003
Cr-Commit-Position: refs/heads/master@{#445964}
Committed: https://chromium.googlesource.com/chromium/src/+/c767da1904a215d821f211fc01cd5a1da0096858
Patch Set 1 #
Total comments: 4
Patch Set 2 : HTTP Cache Age calculation mechanism changed with RFC 7234 #
Total comments: 4
Patch Set 3 : HTTP Cache Age calculation mechanism changed with RFC 7234 #Patch Set 4 : HTTP Cache Age calculation mechanism changed with RFC 7234 #
Messages
Total messages: 36 (23 generated)
billy.jayan@samsung.com changed reviewers: + mmenke@chromium.org
PTAL.
mmenke@chromium.org changed reviewers: + gavinp@chromium.org, jkarlin@chromium.org
[+jkarlin, +gavinp]: Could one of you review this? Tiny CL, but I'm not at all familiar with any of the caching logic. https://codereview.chromium.org/2644063003/diff/1/net/http/http_response_head... File net/http/http_response_headers.cc (right): https://codereview.chromium.org/2644063003/diff/1/net/http/http_response_head... net/http/http_response_headers.cc:1131: // I assume this comment needs to be updated, too (And the RFC reference is certainly wrong now) https://codereview.chromium.org/2644063003/diff/1/net/http/http_response_head... net/http/http_response_headers.cc:1149: TimeDelta corrected_initial_age = std::max(apparent_age, corrected_age_value); We should have a test for the changed behavior.
The CQ bit was checked by mmenke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/01/19 16:11:18, mmenke wrote: > [+jkarlin, +gavinp]: Could one of you review this? Tiny CL, but I'm not at all > familiar with any of the caching logic. > > https://codereview.chromium.org/2644063003/diff/1/net/http/http_response_head... > File net/http/http_response_headers.cc (right): > > https://codereview.chromium.org/2644063003/diff/1/net/http/http_response_head... > net/http/http_response_headers.cc:1131: // > I assume this comment needs to be updated, too (And the RFC reference is > certainly wrong now) > > https://codereview.chromium.org/2644063003/diff/1/net/http/http_response_head... > net/http/http_response_headers.cc:1149: TimeDelta corrected_initial_age = > std::max(apparent_age, corrected_age_value); > We should have a test for the changed behavior. Looking good to me, though I agree with mmenke's two comments.
Also, could you update the bug= line to be "bug=682606"?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL. Hope the tests are sufficient https://codereview.chromium.org/2644063003/diff/1/net/http/http_response_head... File net/http/http_response_headers.cc (right): https://codereview.chromium.org/2644063003/diff/1/net/http/http_response_head... net/http/http_response_headers.cc:1131: // On 2017/01/19 16:11:18, mmenke wrote: > I assume this comment needs to be updated, too (And the RFC reference is > certainly wrong now) Done. https://codereview.chromium.org/2644063003/diff/1/net/http/http_response_head... net/http/http_response_headers.cc:1149: TimeDelta corrected_initial_age = std::max(apparent_age, corrected_age_value); On 2017/01/19 16:11:18, mmenke wrote: > We should have a test for the changed behavior. Done.
The CQ bit was checked by jkarlin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
https://codereview.chromium.org/2644063003/diff/20001/net/http/http_response_... File net/http/http_response_headers_unittest.cc (right): https://codereview.chromium.org/2644063003/diff/20001/net/http/http_response_... net/http/http_response_headers_unittest.cc:2239: base::Time::FromString(test.request_time, &request_time); You'll need to check the return values of FromString here and below. ../../net/http/http_response_headers_unittest.cc:2158:3: error: ignoring return value of function declared with 'warn_unused_result' attribute [-Werror,-Wunused-result] base::Time::FromString(test.request_time, &request_time); https://codereview.chromium.org/2644063003/diff/20001/net/http/http_response_... net/http/http_response_headers_unittest.cc:2263: // date_value > response_time. Please also add a case where date_value > response_time and there is no Age header.
Description was changed from ========== HTTP Cache Age calculation mechanism changed with RFC 7234 Current cached resource age calculation implementation in chromium references and uses RFC 2616 which has been obsoleted by RFC 7234. The new spec has minor modification with respect to age calculation ([1] & [2]) Old spec: https://tools.ietf.org/html/rfc2616#section-13.2.3 New spec: http://httpwg.org/specs/rfc7234.html#rfc.section.4.2.3 BUG=682606 ========== to ========== HTTP Cache Age calculation mechanism changed with RFC 7234 Current cached resource age calculation implementation in chromium references and uses RFC 2616 which has been obsoleted by RFC 7234. The new spec has minor modification with respect to age calculation ([1] & [2]) Old spec: https://tools.ietf.org/html/rfc2616#section-13.2.3 New spec: http://httpwg.org/specs/rfc7234.html#rfc.section.4.2.3 BUG=682606 ==========
mmenke@chromium.org changed reviewers: - mmenke@chromium.org
PTAL. https://codereview.chromium.org/2644063003/diff/20001/net/http/http_response_... File net/http/http_response_headers_unittest.cc (right): https://codereview.chromium.org/2644063003/diff/20001/net/http/http_response_... net/http/http_response_headers_unittest.cc:2239: base::Time::FromString(test.request_time, &request_time); On 2017/01/23 14:09:41, jkarlin wrote: > You'll need to check the return values of FromString here and below. > > ../../net/http/http_response_headers_unittest.cc:2158:3: error: ignoring return > value of function declared with 'warn_unused_result' attribute > [-Werror,-Wunused-result] > base::Time::FromString(test.request_time, &request_time); Done. https://codereview.chromium.org/2644063003/diff/20001/net/http/http_response_... net/http/http_response_headers_unittest.cc:2263: // date_value > response_time. On 2017/01/23 14:09:41, jkarlin wrote: > Please also add a case where date_value > response_time and there is no Age > header. Done.
The CQ bit was checked by jkarlin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
lgtm
You'll need an lgtm from a net owner as well. gavinp@ PTAL
lgtm, ty
The CQ bit was checked by billy.jayan@samsung.com
The CQ bit was unchecked by billy.jayan@samsung.com
The CQ bit was checked by billy.jayan@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by billy.jayan@samsung.com
The CQ bit was checked by billy.jayan@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1485323266301690,
"parent_rev": "13f2e28123b749889096143c033a79307510a3aa", "commit_rev":
"c767da1904a215d821f211fc01cd5a1da0096858"}
Message was sent while issue was closed.
Description was changed from ========== HTTP Cache Age calculation mechanism changed with RFC 7234 Current cached resource age calculation implementation in chromium references and uses RFC 2616 which has been obsoleted by RFC 7234. The new spec has minor modification with respect to age calculation ([1] & [2]) Old spec: https://tools.ietf.org/html/rfc2616#section-13.2.3 New spec: http://httpwg.org/specs/rfc7234.html#rfc.section.4.2.3 BUG=682606 ========== to ========== HTTP Cache Age calculation mechanism changed with RFC 7234 Current cached resource age calculation implementation in chromium references and uses RFC 2616 which has been obsoleted by RFC 7234. The new spec has minor modification with respect to age calculation ([1] & [2]) Old spec: https://tools.ietf.org/html/rfc2616#section-13.2.3 New spec: http://httpwg.org/specs/rfc7234.html#rfc.section.4.2.3 BUG=682606 Review-Url: https://codereview.chromium.org/2644063003 Cr-Commit-Position: refs/heads/master@{#445964} Committed: https://chromium.googlesource.com/chromium/src/+/c767da1904a215d821f211fc01cd... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/c767da1904a215d821f211fc01cd... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
