Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1481143002:
Added HttpUtils::HasValidators and HttpResponse::HasValidators (Closed)
|
Created:
5 years ago by jamartin Modified:
4 years, 9 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, wifiprefetch-reviews_google.com, jamartin Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdded HttpUtils::HasValidators and HttpResponse::HasValidators
Used in components/precache_fetcher.cc, part of the project "Wi-Fi Prefetch for Chrome Mobile" (see BUG), in order to increase the coverage of cacheable resources.
Tested:
$ ./build/gyp_chromium -D OS=linux && \
ninja -C out/Debug components_unittests net_unittests && \
out/Debug/components_unittests --gtest_filter=Precache* && \
out/Debug/net_unittests --gtest_filter=*.HasValidators*
BUG=309216
Committed: https://crrev.com/2edf0e7ce56f2126ed7a22392b61a25dcc62f3dd
Cr-Commit-Position: refs/heads/master@{#378226}
Patch Set 1 #
Total comments: 16
Patch Set 2 : Addressed first batch of bengr@'s comments #
Total comments: 4
Patch Set 3 : Fixed typos in the documentation #
Total comments: 5
Patch Set 4 : Sync'ed and rebased to master #Patch Set 5 : Removed loops from the test cases #
Total comments: 8
Patch Set 6 : Tests for empty ("\"\"") ETags #
Total comments: 8
Patch Set 7 : Clarified the documentation #
Messages
Total messages: 43 (17 generated)
jamartin@chromium.org changed reviewers: + bengr@chromium.org, twifkak@chromium.org
jamartin@google.com changed reviewers: + gavinp@chromium.org, jamartin@google.com
gavinp@chromium.org: Please review changes in net/http/
https://codereview.chromium.org/1481143002/diff/1/net/http/http_response_head... File net/http/http_response_headers.h (right): https://codereview.chromium.org/1481143002/diff/1/net/http/http_response_head... net/http/http_response_headers.h:261: // section 13.3.3 of RFC 2616. I'd add that it returns true specifically when the response has Last-Modified or ETag. https://codereview.chromium.org/1481143002/diff/1/net/http/http_response_head... File net/http/http_response_headers_unittest.cc (right): https://codereview.chromium.org/1481143002/diff/1/net/http/http_response_head... net/http/http_response_headers_unittest.cc:1804: TEST(HttpResponseHeadersTest, HasValidatorsNone) { Add tests that expect false when the HTTP version is < 1.0, irrespective of whether there are Etag or Last-Modified headers. Add a test that expect false for HTTP/1.0 when an ETag is provided without a Last-Modified. Add a test that expects true for HTTP/1.0 when both an ETag and Last-Modified are supplied. https://codereview.chromium.org/1481143002/diff/1/net/http/http_response_head... net/http/http_response_headers_unittest.cc:1823: "Last-Modified: Wed, 28 Nov 2007 00:40:10 GMT"); Add a test that expects false when last-modified has an incorrectly formatted date. https://codereview.chromium.org/1481143002/diff/1/net/http/http_util.cc File net/http/http_util.cc (right): https://codereview.chromium.org/1481143002/diff/1/net/http/http_util.cc#newco... net/http/http_util.cc:739: return ((date - last_modified).InSeconds() >= 60); I'd add a comment here that a Last-Modified header is implicitly weak unless it is at least 60 seconds before the Date Value. https://codereview.chromium.org/1481143002/diff/1/net/http/http_util.h File net/http/http_util.h (right): https://codereview.chromium.org/1481143002/diff/1/net/http/http_util.h#newcod... net/http/http_util.h:207: // strong or weak). See section 13.3.3 of RFC 2616. I'd also add to the comment that it returns true specifically when the response has Last-Modified or ETag. https://codereview.chromium.org/1481143002/diff/1/net/http/http_util_unittest.cc File net/http/http_util_unittest.cc (right): https://codereview.chromium.org/1481143002/diff/1/net/http/http_util_unittest... net/http/http_util_unittest.cc:1181: TEST(HttpUtilTest, HasValidators) { For completeness, I think you should test the following cases for each of http/0.9, http/1.0, and http/1.1: no etag, no last-modified weak etag, no last-modified strong etag, no last-modified no etag, last-modified weak etag, last-modified strong etag, last-modified no etag, last-modified with bad date weak etag, last-modified with bad date For http 0.9 and 1.0, I'm ok with not testing strong and weak etags separately. strong etag, last-modified with bad date https://codereview.chromium.org/1481143002/diff/1/net/http/http_util_unittest... net/http/http_util_unittest.cc:1186: EXPECT_FALSE(HttpUtil::HasValidators(HttpVersion(1, 0), "stub etag", "")); Maybe use the string "anything" here too, as well as below. https://codereview.chromium.org/1481143002/diff/1/net/http/http_util_unittest... net/http/http_util_unittest.cc:1188: EXPECT_TRUE(HttpUtil::HasValidators(HttpVersion(1, 0), "", Also EXPECT_TRUE when both an etag and a last-modified are supplied.
jamartin@google.com changed reviewers: - jamartin@google.com
PTAL. https://codereview.chromium.org/1481143002/diff/1/net/http/http_response_head... File net/http/http_response_headers.h (right): https://codereview.chromium.org/1481143002/diff/1/net/http/http_response_head... net/http/http_response_headers.h:261: // section 13.3.3 of RFC 2616. On 2015/12/03 17:42:21, bengr wrote: > I'd add that it returns true specifically when the response has Last-Modified or > ETag. Done. https://codereview.chromium.org/1481143002/diff/1/net/http/http_response_head... File net/http/http_response_headers_unittest.cc (right): https://codereview.chromium.org/1481143002/diff/1/net/http/http_response_head... net/http/http_response_headers_unittest.cc:1804: TEST(HttpResponseHeadersTest, HasValidatorsNone) { On 2015/12/03 17:42:21, bengr wrote: > Add tests that expect false when the HTTP version is < 1.0, irrespective of > whether there are Etag or Last-Modified headers. > > Add a test that expect false for HTTP/1.0 when an ETag is provided without a > Last-Modified. > Add a test that expects true for HTTP/1.0 when both an ETag and Last-Modified > are supplied. Those test are already in http_util_unittest.cc, which I delegate to (and I've extended now). Do you think the added complexity of this wrapper (i.e., the calls to EnumerateHeader) requires having similar tests here? https://codereview.chromium.org/1481143002/diff/1/net/http/http_response_head... net/http/http_response_headers_unittest.cc:1823: "Last-Modified: Wed, 28 Nov 2007 00:40:10 GMT"); On 2015/12/03 17:42:21, bengr wrote: > Add a test that expects false when last-modified has an incorrectly formatted > date. Please see my reply above. https://codereview.chromium.org/1481143002/diff/1/net/http/http_util.cc File net/http/http_util.cc (right): https://codereview.chromium.org/1481143002/diff/1/net/http/http_util.cc#newco... net/http/http_util.cc:739: return ((date - last_modified).InSeconds() >= 60); On 2015/12/03 17:42:21, bengr wrote: > I'd add a comment here that a Last-Modified header is implicitly weak unless it > is at least 60 seconds before the Date Value. Done. https://codereview.chromium.org/1481143002/diff/1/net/http/http_util.h File net/http/http_util.h (right): https://codereview.chromium.org/1481143002/diff/1/net/http/http_util.h#newcod... net/http/http_util.h:207: // strong or weak). See section 13.3.3 of RFC 2616. On 2015/12/03 17:42:21, bengr wrote: > I'd also add to the comment that it returns true specifically when the response > has Last-Modified or ETag. Done. https://codereview.chromium.org/1481143002/diff/1/net/http/http_util_unittest.cc File net/http/http_util_unittest.cc (right): https://codereview.chromium.org/1481143002/diff/1/net/http/http_util_unittest... net/http/http_util_unittest.cc:1181: TEST(HttpUtilTest, HasValidators) { On 2015/12/03 17:42:21, bengr wrote: > For completeness, I think you should test the following cases for each of > http/0.9, http/1.0, and http/1.1: > > no etag, no last-modified > weak etag, no last-modified > strong etag, no last-modified > no etag, last-modified > weak etag, last-modified > strong etag, last-modified > no etag, last-modified with bad date > weak etag, last-modified with bad date > > For http 0.9 and 1.0, I'm ok with not testing strong and weak etags separately. > strong etag, last-modified with bad date Done. I've gone for the full N^3 approach on this class but not in the other (http_response_header_unittest.cc). Let me know if you want me to replicate these tests in there as well. https://codereview.chromium.org/1481143002/diff/1/net/http/http_util_unittest... net/http/http_util_unittest.cc:1186: EXPECT_FALSE(HttpUtil::HasValidators(HttpVersion(1, 0), "stub etag", "")); On 2015/12/03 17:42:21, bengr wrote: > Maybe use the string "anything" here too, as well as below. Done. https://codereview.chromium.org/1481143002/diff/1/net/http/http_util_unittest... net/http/http_util_unittest.cc:1188: EXPECT_TRUE(HttpUtil::HasValidators(HttpVersion(1, 0), "", On 2015/12/03 17:42:21, bengr wrote: > Also EXPECT_TRUE when both an etag and a last-modified are supplied. Done.
lgtm, with nits on comments. https://codereview.chromium.org/1481143002/diff/20001/net/http/http_response_... File net/http/http_response_headers.h (right): https://codereview.chromium.org/1481143002/diff/20001/net/http/http_response_... net/http/http_response_headers.h:261: // an ETag) regardless if they are strong or weak. See section 13.3.3 of RFC regardless if they are -> regardless of whether it is https://codereview.chromium.org/1481143002/diff/20001/net/http/http_util.h File net/http/http_util.h (right): https://codereview.chromium.org/1481143002/diff/20001/net/http/http_util.h#ne... net/http/http_util.h:207: // an ETag) regardless if they are strong or weak. See section 13.3.3 of RFC Fix this comment to as per my last comment.
All done. I'm ready to submit now. https://codereview.chromium.org/1481143002/diff/20001/net/http/http_response_... File net/http/http_response_headers.h (right): https://codereview.chromium.org/1481143002/diff/20001/net/http/http_response_... net/http/http_response_headers.h:261: // an ETag) regardless if they are strong or weak. See section 13.3.3 of RFC On 2016/02/21 21:08:05, bengr wrote: > regardless if they are -> regardless of whether it is Done. https://codereview.chromium.org/1481143002/diff/20001/net/http/http_util.h File net/http/http_util.h (right): https://codereview.chromium.org/1481143002/diff/20001/net/http/http_util.h#ne... net/http/http_util.h:207: // an ETag) regardless if they are strong or weak. See section 13.3.3 of RFC On 2016/02/21 21:08:05, bengr wrote: > Fix this comment to as per my last comment. Done.
On 2016/02/22 16:49:43, jamartin1 wrote: > All done. I'm ready to submit now. > > https://codereview.chromium.org/1481143002/diff/20001/net/http/http_response_... > File net/http/http_response_headers.h (right): > > https://codereview.chromium.org/1481143002/diff/20001/net/http/http_response_... > net/http/http_response_headers.h:261: // an ETag) regardless if they are strong > or weak. See section 13.3.3 of RFC > On 2016/02/21 21:08:05, bengr wrote: > > regardless if they are -> regardless of whether it is > > Done. > > https://codereview.chromium.org/1481143002/diff/20001/net/http/http_util.h > File net/http/http_util.h (right): > > https://codereview.chromium.org/1481143002/diff/20001/net/http/http_util.h#ne... > net/http/http_util.h:207: // an ETag) regardless if they are strong or weak. > See section 13.3.3 of RFC > On 2016/02/21 21:08:05, bengr wrote: > > Fix this comment to as per my last comment. > > Done. Your trybots need to pass and you need an LGTM from an owner of each file.
lgtm LG so you don't have to wait for another earth rotation for me to be awake, but a mutual exclusion (your choice) of my two test comments is required. Your trybot error is a patch failure - you probably just need to `git checkout master && gclient sync && git checkout topic-branch && git rebase && git cl upload`. (Proper etiquette is to make the rebase its own upload, so the diffs don't get mixed up with your own changes.) https://codereview.chromium.org/1481143002/diff/40001/net/http/http_util_unit... File net/http/http_util_unittest.cc (right): https://codereview.chromium.org/1481143002/diff/40001/net/http/http_util_unit... net/http/http_util_unittest.cc:1189: HttpUtil::HasValidators(HttpVersion(0, 9), etag, last_modified)); add `<< "with etag = " << etag << " and last_modified = "<< last_modified`, so we know what loop iteration failed. (Ditto below.) https://codereview.chromium.org/1481143002/diff/40001/net/http/http_util_unit... net/http/http_util_unittest.cc:1191: } There are 3*3=9 etag/last-modified pairs. Suggestion: 1. Make char[] constants for the five values you specify on lines 1182-3 (and which you duplicate below!). 2. Just hard-code the 9 pairs without a loop, e.g.: EXPECT_FALSE(Http::HasValidators(HttpVersion(0, 9), kEmpty, kLastModifiedInvalid)); ... This will: 1. Be easier to read, thanks to not having to use variables or boolean logic in your expectations. 2. Not be much brittler, thanks to the constants. 3. Only be two lines longer than the current loop-based tests.
s/required/strongly preferred/. I came across a little harsh, there. Thanks for this change!
The CQ bit was checked by jamartin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bengr@chromium.org, twifkak@chromium.org Link to the patchset: https://codereview.chromium.org/1481143002/#ps80001 (title: "Removed loops from the test cases")
The CQ bit was unchecked by jamartin@chromium.org
The CQ bit was checked by jamartin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1481143002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1481143002/80001
All done. Thanks for the reviews. I'm now waiting for Gavin's approval for the OWNERship. https://codereview.chromium.org/1481143002/diff/40001/net/http/http_util_unit... File net/http/http_util_unittest.cc (right): https://codereview.chromium.org/1481143002/diff/40001/net/http/http_util_unit... net/http/http_util_unittest.cc:1189: HttpUtil::HasValidators(HttpVersion(0, 9), etag, last_modified)); On 2016/02/22 23:44:44, twifkak wrote: > add `<< "with etag = " << etag << " and last_modified = "<< last_modified`, so > we know what loop iteration failed. (Ditto below.) I've applied your second suggestion, which obsoletes this. https://codereview.chromium.org/1481143002/diff/40001/net/http/http_util_unit... net/http/http_util_unittest.cc:1191: } On 2016/02/22 23:44:44, twifkak wrote: > There are 3*3=9 etag/last-modified pairs. > > Suggestion: > 1. Make char[] constants for the five values you specify on lines 1182-3 (and > which you duplicate below!). > 2. Just hard-code the 9 pairs without a loop, e.g.: > EXPECT_FALSE(Http::HasValidators(HttpVersion(0, 9), kEmpty, > kLastModifiedInvalid)); > ... > > This will: > 1. Be easier to read, thanks to not having to use variables or boolean logic in > your expectations. > 2. Not be much brittler, thanks to the constants. > 3. Only be two lines longer than the current loop-based tests. Done. There is one advantage of the loop approach, though: the condition for the TRUE/FALSE evaluation is made explicit. But I do agree with you that it is now as simple as it gets.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from
==========
Added HttpUtils::HasValidators and HttpResponse::HasValidators
Used in components/precache_fetcher.cc
Tested:
$ ./build/gyp_chromium -D OS=linux && \
ninja -C out/Debug components_unittests net_unittests && \
out/Debug/components_unittests --gtest_filter=Precache* && \
out/Debug/net_unittests --gtest_filter=*.HasValidators*
==========
to
==========
Added HttpUtils::HasValidators and HttpResponse::HasValidators
Used in components/precache_fetcher.cc
Tested:
$ ./build/gyp_chromium -D OS=linux && \
ninja -C out/Debug components_unittests net_unittests && \
out/Debug/components_unittests --gtest_filter=Precache* && \
out/Debug/net_unittests --gtest_filter=*.HasValidators*
==========
jamartin@chromium.org changed reviewers: + mmenke@chromium.org - gavinp@chromium.org
lgtm https://codereview.chromium.org/1481143002/diff/40001/net/http/http_util_unit... File net/http/http_util_unittest.cc (right): https://codereview.chromium.org/1481143002/diff/40001/net/http/http_util_unit... net/http/http_util_unittest.cc:1191: } On 2016/02/23 14:03:35, jamartin1 wrote: > On 2016/02/22 23:44:44, twifkak wrote: > > There are 3*3=9 etag/last-modified pairs. > > > > Suggestion: > > 1. Make char[] constants for the five values you specify on lines 1182-3 (and > > which you duplicate below!). > > 2. Just hard-code the 9 pairs without a loop, e.g.: > > EXPECT_FALSE(Http::HasValidators(HttpVersion(0, 9), kEmpty, > > kLastModifiedInvalid)); > > ... > > > > This will: > > 1. Be easier to read, thanks to not having to use variables or boolean logic > in > > your expectations. > > 2. Not be much brittler, thanks to the constants. > > 3. Only be two lines longer than the current loop-based tests. > > Done. > > There is one advantage of the loop approach, though: the condition for the > TRUE/FALSE evaluation is made explicit. But I do agree with you that it is now > as simple as it gets. LGTM. Some rambling thoughts: Agreed, that the logic is harder to read now. On the plus side, you can read it in the prod code, and the test is much stronger when prod and test are implemented differently. I see your point, though -- some future coder might not know what to do with this wall of assertions. I lean toward simpler still, but I'm not at all steadfast on that.
mmenke@chromium.org changed reviewers: + gavinp@chromium.org
[+gavinp]: Mind reviewing? I'm unfamiliar with validators. There should be a filed bug for this, referenced in the CL description. https://codereview.chromium.org/1481143002/diff/80001/net/http/http_response_... File net/http/http_response_headers.cc (right): https://codereview.chromium.org/1481143002/diff/80001/net/http/http_response_... net/http/http_response_headers.cc:1251: bool HttpResponseHeaders::HasValidators() const { Erm...Wait...I'm not an expert on validators, but shouldn't strong validators be a subset of validators, not the other way around?
Description was changed from
==========
Added HttpUtils::HasValidators and HttpResponse::HasValidators
Used in components/precache_fetcher.cc
Tested:
$ ./build/gyp_chromium -D OS=linux && \
ninja -C out/Debug components_unittests net_unittests && \
out/Debug/components_unittests --gtest_filter=Precache* && \
out/Debug/net_unittests --gtest_filter=*.HasValidators*
==========
to
==========
Added HttpUtils::HasValidators and HttpResponse::HasValidators
Used in components/precache_fetcher.cc, part of the project "Wi-Fi Prefetch for
Chrome Mobile" (see BUG), in order to increase the coverage of cacheable
resources.
Tested:
$ ./build/gyp_chromium -D OS=linux && \
ninja -C out/Debug components_unittests net_unittests && \
out/Debug/components_unittests --gtest_filter=Precache* && \
out/Debug/net_unittests --gtest_filter=*.HasValidators*
BUG=309216
==========
https://codereview.chromium.org/1481143002/diff/80001/net/http/http_response_... File net/http/http_response_headers.cc (right): https://codereview.chromium.org/1481143002/diff/80001/net/http/http_response_... net/http/http_response_headers.cc:1251: bool HttpResponseHeaders::HasValidators() const { On 2016/02/25 07:56:45, mmenke wrote: > Erm...Wait...I'm not an expert on validators, but shouldn't strong validators be > a subset of validators, not the other way around? Yes it should and I believe it is so in this implementation. A strong validator requires either a strong eTag or a LastModified older than 1 min. This function only checks for any eTag (weak or strong) or any LastModified that can be parsed. Am I missing something?
https://codereview.chromium.org/1481143002/diff/80001/net/http/http_response_... File net/http/http_response_headers.cc (right): https://codereview.chromium.org/1481143002/diff/80001/net/http/http_response_... net/http/http_response_headers.cc:1251: bool HttpResponseHeaders::HasValidators() const { On 2016/02/25 11:30:49, jamartin1 wrote: > On 2016/02/25 07:56:45, mmenke wrote: > > Erm...Wait...I'm not an expert on validators, but shouldn't strong validators > be > > a subset of validators, not the other way around? > > Yes it should and I believe it is so in this implementation. > > A strong validator requires either a strong eTag or a LastModified older than 1 > min. This function only checks for any eTag (weak or strong) or any LastModified > that can be parsed. > > Am I missing something? Sorry, I was just confused by the fact this actually has no logic in it, and relies on HttpUtil methods.
Looks good and useful; just two annoying questions. https://codereview.chromium.org/1481143002/diff/80001/net/http/http_util.cc File net/http/http_util.cc (right): https://codereview.chromium.org/1481143002/diff/80001/net/http/http_util.cc#n... net/http/http_util.cc:740: const std::string& date_header) { Could HasStrongValidators call HasValidators? https://codereview.chromium.org/1481143002/diff/80001/net/http/http_util_unit... File net/http/http_util_unittest.cc (right): https://codereview.chromium.org/1481143002/diff/80001/net/http/http_util_unit... net/http/http_util_unittest.cc:1225: EXPECT_FALSE(HttpUtil::HasValidators(v1_0, kEmpty, kEmpty)); Is an empty ETag the same as a not-present ETag for RFC 2616 13.3.3 purposes?
PTAL. https://codereview.chromium.org/1481143002/diff/80001/net/http/http_util.cc File net/http/http_util.cc (right): https://codereview.chromium.org/1481143002/diff/80001/net/http/http_util.cc#n... net/http/http_util.cc:740: const std::string& date_header) { On 2016/02/25 18:56:20, gavinp wrote: > Could HasStrongValidators call HasValidators? Sure although it won't make any difference in the current logic. It makes explicit that HasStrongValidators ⊆ HasValidators, though, which is a benefit on its own right. Is there any other reason why you want to have this call? https://codereview.chromium.org/1481143002/diff/80001/net/http/http_util_unit... File net/http/http_util_unittest.cc (right): https://codereview.chromium.org/1481143002/diff/80001/net/http/http_util_unit... net/http/http_util_unittest.cc:1225: EXPECT_FALSE(HttpUtil::HasValidators(v1_0, kEmpty, kEmpty)); On 2016/02/25 18:56:20, gavinp wrote: > Is an empty ETag the same as a not-present ETag for RFC 2616 13.3.3 purposes? Good question. This is a tricky one that took me some time to research. An empty ETag is allowed and it is even given as an example (RFC 2616 14.19). However, the syntax enforces that any ETag should be a quoted-string (see RFC 2616 3.11 for entity-tag and RFC 2616 2.2 for quoted-string), so an empty ETag would be, in this case, "\"\"". This means that, for valid headers, I can safely assume that an empty string means that the etag was not present. LastModified values are not quoted but in this case I'm also safe because I'm parsing them to make sure it is a valid date, and the empty string is not a valid date. Tests and comments added.
LGTM. Take or leave my nits as you see fit. Really awesome on updating the unit test. I wonder how much of our codebase is as clean about empty/non-present ETags as these proved to be. https://codereview.chromium.org/1481143002/diff/80001/net/http/http_util.cc File net/http/http_util.cc (right): https://codereview.chromium.org/1481143002/diff/80001/net/http/http_util.cc#n... net/http/http_util.cc:740: const std::string& date_header) { On 2016/02/26 14:47:50, jamartin1 wrote: > On 2016/02/25 18:56:20, gavinp wrote: > > Could HasStrongValidators call HasValidators? > > Sure although it won't make any difference in the current logic. It makes > explicit that HasStrongValidators ⊆ HasValidators, though, which is a > benefit on its own right. Is there any other reason why you want to have this > call? The documentary purpose was the only one; but it felt compelling to me: I find the specifications and definitions in this area deeply confusing, and so code that contains strong unambiguous statements about the presumed relationships is really good. https://codereview.chromium.org/1481143002/diff/100001/net/http/http_util.cc File net/http/http_util.cc (right): https://codereview.chromium.org/1481143002/diff/100001/net/http/http_util.cc#... net/http/http_util.cc:744: if (version < HttpVersion(1, 1)) Nit: I think this is redundant now. https://codereview.chromium.org/1481143002/diff/100001/net/http/http_util.cc#... net/http/http_util.cc:761: return false; Nit: isn't this NOTREACHED() now? https://codereview.chromium.org/1481143002/diff/100001/net/http/http_util.cc#... net/http/http_util.cc:784: // empty ETags aren't empty strings (i.e., "\"\""). Nit: how about (i.e. an empty ETag might be "\"\"") so it's totally unambiguously clear you're not giving an example of a nonempty ETag?
(in which I admit to being rong earlier about a thing) https://codereview.chromium.org/1481143002/diff/100001/net/http/http_util.cc File net/http/http_util.cc (right): https://codereview.chromium.org/1481143002/diff/100001/net/http/http_util.cc#... net/http/http_util.cc:761: return false; On 2016/02/26 15:32:59, gavinp wrote: > Nit: isn't this NOTREACHED() now? No, this is not NOTREACHED. A weak ETag together with no last modified value or an unparseable last modified value does constitute a validator, but not a strong one.
mmenke@chromium.org changed reviewers: - mmenke@chromium.org
Thanks for the review. Submitting... https://codereview.chromium.org/1481143002/diff/100001/net/http/http_util.cc File net/http/http_util.cc (right): https://codereview.chromium.org/1481143002/diff/100001/net/http/http_util.cc#... net/http/http_util.cc:744: if (version < HttpVersion(1, 1)) On 2016/02/26 15:32:59, gavinp wrote: > Nit: I think this is redundant now. No, it isn't. A message with LastModified, ETag and v1.0 would be wrongly considered as HasStrongValidators = True. Granted this should not occur since ETag were only introduced in HTTP v1.1 but I better be safe than sorry. https://codereview.chromium.org/1481143002/diff/100001/net/http/http_util.cc#... net/http/http_util.cc:761: return false; On 2016/02/26 15:35:14, gavinp wrote: > On 2016/02/26 15:32:59, gavinp wrote: > > Nit: isn't this NOTREACHED() now? > > No, this is not NOTREACHED. A weak ETag together with no last modified value or > an unparseable last modified value does constitute a validator, but not a strong > one. Acknowledged. https://codereview.chromium.org/1481143002/diff/100001/net/http/http_util.cc#... net/http/http_util.cc:784: // empty ETags aren't empty strings (i.e., "\"\""). On 2016/02/26 15:32:59, gavinp wrote: > Nit: how about (i.e. an empty ETag might be "\"\"") so it's totally > unambiguously clear you're not giving an example of a nonempty ETag? Done.
Thanks! I learned a lot in the review. https://codereview.chromium.org/1481143002/diff/100001/net/http/http_util.cc File net/http/http_util.cc (right): https://codereview.chromium.org/1481143002/diff/100001/net/http/http_util.cc#... net/http/http_util.cc:744: if (version < HttpVersion(1, 1)) On 2016/02/29 14:40:01, jamartin1 wrote: > On 2016/02/26 15:32:59, gavinp wrote: > > Nit: I think this is redundant now. > > No, it isn't. A message with LastModified, ETag and v1.0 would be wrongly > considered as HasStrongValidators = True. Granted this should not occur since > ETag were only introduced in HTTP v1.1 but I better be safe than sorry. Good point. I agree. Thanks.
Thanks! I learned a lot in the review. https://codereview.chromium.org/1481143002/diff/100001/net/http/http_util.cc File net/http/http_util.cc (right): https://codereview.chromium.org/1481143002/diff/100001/net/http/http_util.cc#... net/http/http_util.cc:744: if (version < HttpVersion(1, 1)) On 2016/02/29 14:40:01, jamartin1 wrote: > On 2016/02/26 15:32:59, gavinp wrote: > > Nit: I think this is redundant now. > > No, it isn't. A message with LastModified, ETag and v1.0 would be wrongly > considered as HasStrongValidators = True. Granted this should not occur since > ETag were only introduced in HTTP v1.1 but I better be safe than sorry. Good point. I agree. Thanks.
The CQ bit was checked by jamartin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bengr@chromium.org, twifkak@chromium.org, gavinp@chromium.org Link to the patchset: https://codereview.chromium.org/1481143002/#ps120001 (title: "Clarified the documentation")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1481143002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1481143002/120001
Message was sent while issue was closed.
Description was changed from
==========
Added HttpUtils::HasValidators and HttpResponse::HasValidators
Used in components/precache_fetcher.cc, part of the project "Wi-Fi Prefetch for
Chrome Mobile" (see BUG), in order to increase the coverage of cacheable
resources.
Tested:
$ ./build/gyp_chromium -D OS=linux && \
ninja -C out/Debug components_unittests net_unittests && \
out/Debug/components_unittests --gtest_filter=Precache* && \
out/Debug/net_unittests --gtest_filter=*.HasValidators*
BUG=309216
==========
to
==========
Added HttpUtils::HasValidators and HttpResponse::HasValidators
Used in components/precache_fetcher.cc, part of the project "Wi-Fi Prefetch for
Chrome Mobile" (see BUG), in order to increase the coverage of cacheable
resources.
Tested:
$ ./build/gyp_chromium -D OS=linux && \
ninja -C out/Debug components_unittests net_unittests && \
out/Debug/components_unittests --gtest_filter=Precache* && \
out/Debug/net_unittests --gtest_filter=*.HasValidators*
BUG=309216
==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from
==========
Added HttpUtils::HasValidators and HttpResponse::HasValidators
Used in components/precache_fetcher.cc, part of the project "Wi-Fi Prefetch for
Chrome Mobile" (see BUG), in order to increase the coverage of cacheable
resources.
Tested:
$ ./build/gyp_chromium -D OS=linux && \
ninja -C out/Debug components_unittests net_unittests && \
out/Debug/components_unittests --gtest_filter=Precache* && \
out/Debug/net_unittests --gtest_filter=*.HasValidators*
BUG=309216
==========
to
==========
Added HttpUtils::HasValidators and HttpResponse::HasValidators
Used in components/precache_fetcher.cc, part of the project "Wi-Fi Prefetch for
Chrome Mobile" (see BUG), in order to increase the coverage of cacheable
resources.
Tested:
$ ./build/gyp_chromium -D OS=linux && \
ninja -C out/Debug components_unittests net_unittests && \
out/Debug/components_unittests --gtest_filter=Precache* && \
out/Debug/net_unittests --gtest_filter=*.HasValidators*
BUG=309216
Committed: https://crrev.com/2edf0e7ce56f2126ed7a22392b61a25dcc62f3dd
Cr-Commit-Position: refs/heads/master@{#378226}
==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/2edf0e7ce56f2126ed7a22392b61a25dcc62f3dd Cr-Commit-Position: refs/heads/master@{#378226} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
