Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
391763002:
Initial implementation of Chrome-Freshness header. (Closed)
DescriptionInitial implementation of Chrome-Freshness header.
Add a header like
Chrome-Freshness: max-age=30,stale-while-revalidate=60,age=10
when sending a revalidation request to a server which supplied the
Cache-Control stale-while-revalidate directive on the previous response.
Design doc: https://docs.google.com/document/d/1DMCIMAKjyKeYiu69jlI5OsO2pGyAMb81XflYK4hxsNM/edit
BUG=348877
TEST=net_unittests
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=286482
Patch Set 1 #Patch Set 2 : Add test that Chrome-Freshness header is added. #Patch Set 3 : Use PrivacyMode setting to control sending Chrome-Freshness header. #
Total comments: 6
Patch Set 4 : Make stale-while-revalidate value parsing stricter #
Total comments: 6
Patch Set 5 : Comment and test name fixes. #
Total comments: 9
Patch Set 6 : Rename header and move GetStaleWhileRevalidateValue #
Total comments: 13
Patch Set 7 : Rebase. #Patch Set 8 : Add tests for GetMaxAgeValue and GetStaleWhileRevalidateValue. #
Total comments: 2
Patch Set 9 : Remove const from directive_size. #
Messages
Total messages: 22 (0 generated)
please wrap the CL description to 80 col https://codereview.chromium.org/391763002/diff/40001/net/http/http_cache_tran... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/391763002/diff/40001/net/http/http_cache_tran... net/http/http_cache_transaction.cc:260: base::StringToInt64( handle failure? https://codereview.chromium.org/391763002/diff/40001/net/http/http_cache_tran... net/http/http_cache_transaction.cc:262: value.end()), indent https://codereview.chromium.org/391763002/diff/40001/net/http/http_cache_tran... net/http/http_cache_transaction.cc:2301: // Also don't send the header when DO_NOT_SEND_COOKIES is set, to avoid the if-clause is looking at privacy_mode which is set not only when DO_NOT_SEND_COOKIES but also when DO_NOT_SAVE_COOKIES is set or CanEnablePrivacyMode() is true?
https://codereview.chromium.org/391763002/diff/40001/net/http/http_cache_tran... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/391763002/diff/40001/net/http/http_cache_tran... net/http/http_cache_transaction.cc:260: base::StringToInt64( On 2014/07/18 02:29:55, tyoshino wrote: > handle failure? It looks like the code I copy-and-pasted relied on the "best-effort" parsing to return 0 for an invalid value. I think I will be a bit more strict. https://codereview.chromium.org/391763002/diff/40001/net/http/http_cache_tran... net/http/http_cache_transaction.cc:262: value.end()), On 2014/07/18 02:29:55, tyoshino wrote: > indent Done. https://codereview.chromium.org/391763002/diff/40001/net/http/http_cache_tran... net/http/http_cache_transaction.cc:2301: // Also don't send the header when DO_NOT_SEND_COOKIES is set, to avoid On 2014/07/18 02:29:54, tyoshino wrote: > the if-clause is looking at privacy_mode which is set not only when > DO_NOT_SEND_COOKIES but also when DO_NOT_SAVE_COOKIES is set or > CanEnablePrivacyMode() is true? Sorry, yes, I found that DO_NOT_SEND_COOKIES was insufficient. It is not set when the user has turned off third-party cookies, or when they are blocking cookies for the domain. So I changed to use privacy_mode instead, but I forgot to update the comment.
lgtm https://codereview.chromium.org/391763002/diff/60001/net/http/http_cache_unit... File net/http/http_cache_unittest.cc (right): https://codereview.chromium.org/391763002/diff/60001/net/http/http_cache_unit... net/http/http_cache_unittest.cc:6480: // write to the cache Use a capital letter at the head of a sentence in a comment to be consistent with the rest of the file? https://codereview.chromium.org/391763002/diff/60001/net/http/http_cache_unit... net/http/http_cache_unittest.cc:6498: TEST(HttpCache, StaleWhileRevalidateHeaderNotSent) { ChromeFreshnessHeaderNotSent?
https://codereview.chromium.org/391763002/diff/60001/net/http/http_cache_unit... File net/http/http_cache_unittest.cc (right): https://codereview.chromium.org/391763002/diff/60001/net/http/http_cache_unit... net/http/http_cache_unittest.cc:6453: EXPECT_EQ("max-age=3600,stale-while-revalidate=7200,age=228658821", value); could you please write a comment explaining why the age is expected to be this value
https://codereview.chromium.org/391763002/diff/60001/net/http/http_cache_unit... File net/http/http_cache_unittest.cc (right): https://codereview.chromium.org/391763002/diff/60001/net/http/http_cache_unit... net/http/http_cache_unittest.cc:6453: EXPECT_EQ("max-age=3600,stale-while-revalidate=7200,age=228658821", value); On 2014/07/22 06:31:55, tyoshino wrote: > could you please write a comment explaining why the age is expected to be this > value I wrote a comment explaining that I don't know why the age is this value. It doesn't really make sense that it has a fixed value. Maybe the test should be relaxed to accept any positive integer? I will run the try bots to check that it doesn't very between machines. https://codereview.chromium.org/391763002/diff/60001/net/http/http_cache_unit... net/http/http_cache_unittest.cc:6480: // write to the cache On 2014/07/22 06:29:34, tyoshino wrote: > Use a capital letter at the head of a sentence in a comment to be consistent > with the rest of the file? Done. https://codereview.chromium.org/391763002/diff/60001/net/http/http_cache_unit... net/http/http_cache_unittest.cc:6498: TEST(HttpCache, StaleWhileRevalidateHeaderNotSent) { On 2014/07/22 06:29:34, tyoshino wrote: > ChromeFreshnessHeaderNotSent? Done.
+rvargas for net OWNERS.
https://codereview.chromium.org/391763002/diff/80001/net/http/http_cache_unit... File net/http/http_cache_unittest.cc (right): https://codereview.chromium.org/391763002/diff/80001/net/http/http_cache_unit... net/http/http_cache_unittest.cc:6455: // same. Hmm... FYI, 18 Apr 2007 01:10:43 + 228658821sec = 16 July 2014, 13:31:04
https://codereview.chromium.org/391763002/diff/80001/net/http/http_cache_tran... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/391763002/diff/80001/net/http/http_cache_tran... net/http/http_cache_transaction.cc:55: static const char kFreshnessHeader[] = "Chrome-Freshness"; Shouldn't we use some other prefix that states more clearly that this is private/experimental/not part of any standard? X-? In fact, while there was a discussion about supporting stale-while-revalidate, I don't think I've seen any official talk about the new header (I'm well aware of its value for server-side measurements). Given that this is not part of any standard, should there be something posted somewhere around the open web platform? (I don't follow those lists so I don't know if that happened already, or what is our policy about starting to send random headers from Chrome). Which reminds me that this code should go directly into builds of other browsers than Chrome... https://codereview.chromium.org/391763002/diff/80001/net/http/http_cache_tran... net/http/http_cache_transaction.cc:241: // TODO(ricea): Move this to a more appropriate home. Also, this is closely I think we should move this to http_response_headers with this cl. https://codereview.chromium.org/391763002/diff/80001/net/http/http_cache_tran... net/http/http_cache_transaction.cc:246: const std::string name = "cache-control"; nit: remove const and use constructor initialization instead of assignment. https://codereview.chromium.org/391763002/diff/80001/net/http/http_cache_tran... net/http/http_cache_transaction.cc:2310: custom_request_->extra_headers.SetHeader( We should probably send the header only when stale_while_revalidate is > max_age (and > 0, which allows better code reuse).
> https://codereview.chromium.org/391763002/diff/80001/net/http/http_cache_tran... > net/http/http_cache_transaction.cc:55: static const char kFreshnessHeader[] = > "Chrome-Freshness"; > Shouldn't we use some other prefix that states more clearly that this is > private/experimental/not part of any standard? X-? IIUC, the use of X- has been deprecated: http://tools.ietf.org/html/rfc6648 Mark Nottingham told me that we should feel free to experiment first and follow-up with a spec if the results are encouraging. > > In fact, while there was a discussion about supporting stale-while-revalidate, I > don't think I've seen any official talk about the new header (I'm well aware of > its value for server-side measurements). Given that this is not part of any > standard, should there be something posted somewhere around the open web > platform? (I don't follow those lists so I don't know if that happened already, > or what is our policy about starting to send random headers from Chrome). I mentioned this custom header a couple of times in the chromium-dev + blink-dev thread: https://groups.google.com/a/chromium.org/d/msg/chromium-dev/zchogDvIYrY/ZqWSd... I can call the attention of folks one more time. This is only for experiment purpose and since this is being sent by user agents I don't expect we'll run into the typical compatibility issues where everyone start to use what was supposed to be a temporary name. Unless, other browser vendors start to support stale-while-revalidate before we're done defining a proper name. Prefixing this with Chrome or Google further reduces that possibility. The PRD proposed : Resource-Freshness as the final name. I think Chrome-Freshness might be misconstrued as having something to do about Chrome's freshness... How about Chrome-Resource-Freshness? > https://codereview.chromium.org/391763002/diff/80001/net/http/http_cache_tran... > net/http/http_cache_transaction.cc:2310: > custom_request_->extra_headers.SetHeader( > We should probably send the header only when stale_while_revalidate is > max_age > (and > 0, which allows better code reuse). Actually, this would be incorrect. stale-while-revalidate is a second window of time happening after max-age (so it doesn't have to be > max-age.)
As an FYI, I just published a draft/tentative specification for the header at https://github.com/KenjiBaheux/resource-freshness Comments welcomed.
On 2014/07/24 05:11:57, kenjibaheux wrote: > > > https://codereview.chromium.org/391763002/diff/80001/net/http/http_cache_tran... > > net/http/http_cache_transaction.cc:55: static const char kFreshnessHeader[] = > > "Chrome-Freshness"; > > Shouldn't we use some other prefix that states more clearly that this is > > private/experimental/not part of any standard? X-? > > IIUC, the use of X- has been deprecated: http://tools.ietf.org/html/rfc6648 > Mark Nottingham told me that we should feel free to experiment first and > follow-up with a spec if the results are encouraging. > > > > > > In fact, while there was a discussion about supporting stale-while-revalidate, > I > > don't think I've seen any official talk about the new header (I'm well aware > of > > its value for server-side measurements). Given that this is not part of any > > standard, should there be something posted somewhere around the open web > > platform? (I don't follow those lists so I don't know if that happened > already, > > or what is our policy about starting to send random headers from Chrome). > > I mentioned this custom header a couple of times in the chromium-dev + blink-dev > thread: > https://groups.google.com/a/chromium.org/d/msg/chromium-dev/zchogDvIYrY/ZqWSd... > > I can call the attention of folks one more time. > > > This is only for experiment purpose and since this is being sent by user agents > I don't expect we'll run into the typical compatibility issues where everyone > start to use what was supposed to be a temporary name. Unless, other browser > vendors start to support stale-while-revalidate before we're done defining a > proper name. Prefixing this with Chrome or Google further reduces that > possibility. > > The PRD proposed : Resource-Freshness as the final name. > I think Chrome-Freshness might be misconstrued as having something to do about > Chrome's freshness... > How about Chrome-Resource-Freshness? How about the fact that this goes right away (I think) into Opera's browser? Chromium- maybe? Something else to make obvious that this is a name that will change? (although to be fair the actual risk of someone using this name and forgetting to update it later should be quite low so) > > > > > https://codereview.chromium.org/391763002/diff/80001/net/http/http_cache_tran... > > net/http/http_cache_transaction.cc:2310: > > custom_request_->extra_headers.SetHeader( > > We should probably send the header only when stale_while_revalidate is > > max_age > > (and > 0, which allows better code reuse). > > Actually, this would be incorrect. stale-while-revalidate is a second window of > time happening after max-age (so it doesn't have to be > max-age.) ah, ok.
On 2014/07/24 18:11:11, rvargas wrote: > On 2014/07/24 05:11:57, kenjibaheux wrote: > > > > > > https://codereview.chromium.org/391763002/diff/80001/net/http/http_cache_tran... > > > net/http/http_cache_transaction.cc:55: static const char kFreshnessHeader[] > = > > > "Chrome-Freshness"; > > > Shouldn't we use some other prefix that states more clearly that this is > > > private/experimental/not part of any standard? X-? > > > > IIUC, the use of X- has been deprecated: http://tools.ietf.org/html/rfc6648 > > Mark Nottingham told me that we should feel free to experiment first and > > follow-up with a spec if the results are encouraging. > > > > > > > > > > In fact, while there was a discussion about supporting > stale-while-revalidate, > > I > > > don't think I've seen any official talk about the new header (I'm well aware > > of > > > its value for server-side measurements). Given that this is not part of any > > > standard, should there be something posted somewhere around the open web > > > platform? (I don't follow those lists so I don't know if that happened > > already, > > > or what is our policy about starting to send random headers from Chrome). > > > > I mentioned this custom header a couple of times in the chromium-dev + > blink-dev > > thread: > > > https://groups.google.com/a/chromium.org/d/msg/chromium-dev/zchogDvIYrY/ZqWSd... > > > > I can call the attention of folks one more time. > > > > > > This is only for experiment purpose and since this is being sent by user > agents > > I don't expect we'll run into the typical compatibility issues where everyone > > start to use what was supposed to be a temporary name. Unless, other browser > > vendors start to support stale-while-revalidate before we're done defining a > > proper name. Prefixing this with Chrome or Google further reduces that > > possibility. > > > > The PRD proposed : Resource-Freshness as the final name. > > I think Chrome-Freshness might be misconstrued as having something to do about > > Chrome's freshness... > > How about Chrome-Resource-Freshness? > > How about the fact that this goes right away (I think) into Opera's browser? > Chromium- maybe? Something else to make obvious that this is a name that will > change? (although to be fair the actual risk of someone using this name and > forgetting to update it later should be quite low so) > Ah, good point. I'm fine with a Chromium- prefix. Meanwhile I'll do my best to move this through the IETF process for registering new header fields.
Unfortunately, from http://www.chromium.org/developers/coding-style#Naming : - "Chromium" is the name of the project, not the product, and should never appear in code, variable names, API names etc. Use "chrome" instead. I guess it doesn't explicitly say "header names" so I will ignore it for now. On 25 July 2014 08:44, <kenjibaheux@chromium.org> wrote: > On 2014/07/24 18:11:11, rvargas wrote: > >> On 2014/07/24 05:11:57, kenjibaheux wrote: >> > > >> > >> > > https://codereview.chromium.org/391763002/diff/80001/net/ > http/http_cache_transaction.cc#newcode55 > >> > > net/http/http_cache_transaction.cc:55: static const char >> > kFreshnessHeader[] > >> = >> > > "Chrome-Freshness"; >> > > Shouldn't we use some other prefix that states more clearly that this >> is >> > > private/experimental/not part of any standard? X-? >> > >> > IIUC, the use of X- has been deprecated: http://tools.ietf.org/html/ >> rfc6648 >> > Mark Nottingham told me that we should feel free to experiment first and >> > follow-up with a spec if the results are encouraging. >> > >> > >> > > >> > > In fact, while there was a discussion about supporting >> stale-while-revalidate, >> > I >> > > don't think I've seen any official talk about the new header (I'm well >> > aware > >> > of >> > > its value for server-side measurements). Given that this is not part >> of >> > any > >> > > standard, should there be something posted somewhere around the open >> web >> > > platform? (I don't follow those lists so I don't know if that happened >> > already, >> > > or what is our policy about starting to send random headers from >> Chrome). >> > >> > I mentioned this custom header a couple of times in the chromium-dev + >> blink-dev >> > thread: >> > >> > > https://groups.google.com/a/chromium.org/d/msg/chromium- > dev/zchogDvIYrY/ZqWSdt3LJdMJ > >> > >> > I can call the attention of folks one more time. >> > >> > >> > This is only for experiment purpose and since this is being sent by user >> agents >> > I don't expect we'll run into the typical compatibility issues where >> > everyone > >> > start to use what was supposed to be a temporary name. Unless, other >> browser >> > vendors start to support stale-while-revalidate before we're done >> defining a >> > proper name. Prefixing this with Chrome or Google further reduces that >> > possibility. >> > >> > The PRD proposed : Resource-Freshness as the final name. >> > I think Chrome-Freshness might be misconstrued as having something to do >> > about > >> > Chrome's freshness... >> > How about Chrome-Resource-Freshness? >> > > How about the fact that this goes right away (I think) into Opera's >> browser? >> Chromium- maybe? Something else to make obvious that this is a name that >> will >> change? (although to be fair the actual risk of someone using this name >> and >> forgetting to update it later should be quite low so) >> > > > Ah, good point. > I'm fine with a Chromium- prefix. > Meanwhile I'll do my best to move this through the IETF process for > registering > new header fields. > > https://codereview.chromium.org/391763002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/391763002/diff/80001/net/http/http_cache_tran... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/391763002/diff/80001/net/http/http_cache_tran... net/http/http_cache_transaction.cc:55: static const char kFreshnessHeader[] = "Chrome-Freshness"; On 2014/07/24 00:54:06, rvargas_Out_until_7-23 wrote: > Shouldn't we use some other prefix that states more clearly that this is > private/experimental/not part of any standard? X-? > > In fact, while there was a discussion about supporting stale-while-revalidate, I > don't think I've seen any official talk about the new header (I'm well aware of > its value for server-side measurements). Given that this is not part of any > standard, should there be something posted somewhere around the open web > platform? (I don't follow those lists so I don't know if that happened already, > or what is our policy about starting to send random headers from Chrome). > > Which reminds me that this code should go directly into builds of other browsers > than Chrome... Okay, I've changed to "Chromium-Resource-Freshness" for now. https://codereview.chromium.org/391763002/diff/80001/net/http/http_cache_tran... net/http/http_cache_transaction.cc:241: // TODO(ricea): Move this to a more appropriate home. Also, this is closely On 2014/07/24 00:54:06, rvargas wrote: > I think we should move this to http_response_headers with this cl. Done. https://codereview.chromium.org/391763002/diff/80001/net/http/http_cache_tran... net/http/http_cache_transaction.cc:246: const std::string name = "cache-control"; On 2014/07/24 00:54:06, rvargas wrote: > nit: remove const and use constructor initialization instead of assignment. Done. https://codereview.chromium.org/391763002/diff/80001/net/http/http_cache_tran... net/http/http_cache_transaction.cc:2310: custom_request_->extra_headers.SetHeader( On 2014/07/24 00:54:06, rvargas_Out_until_7-23 wrote: > We should probably send the header only when stale_while_revalidate is > max_age > (and > 0, which allows better code reuse). I think this would probably make analysis too difficult. There are lots of reasons why the request might not include the Freshness header, including: * A proxy stripped it * A proxy stripped stale-while-revalidate from the original response * The browser isn't really Chrome * This revalidation is coming from a cache in the middle, and not from Chrome itself * This browser isn't in the experiment group we're sending stale-while-revalidate too I could be mistaken, but I think the only way to do a sound analysis is is to exclude all responses that don't have the Freshness header.
https://codereview.chromium.org/391763002/diff/100001/net/http/http_cache_uni... File net/http/http_cache_unittest.cc (right): https://codereview.chromium.org/391763002/diff/100001/net/http/http_cache_uni... net/http/http_cache_unittest.cc:6463: const MockTransaction kStaleWhileRevalidateTransaction = { It may be more clear to use a predefined transaction (kSimpleGET ?) and override the response headers. That way there is less noise of things that are not related to the test. https://codereview.chromium.org/391763002/diff/100001/net/http/http_response_... File net/http/http_response_headers.cc (right): https://codereview.chromium.org/391763002/diff/100001/net/http/http_response_... net/http/http_response_headers.cc:1121: bool HttpResponseHeaders::GetStaleWhileRevalidateValue( Please add HttpResponseHeaders unit tests to make sure this method behaves as expected. https://codereview.chromium.org/391763002/diff/100001/net/http/http_response_... net/http/http_response_headers.cc:1131: const size_t directive_size = directive.size(); nit: no const https://codereview.chromium.org/391763002/diff/100001/net/http/http_response_... net/http/http_response_headers.cc:1135: if (value.size() > directive_size + 1 && This '+ 1' is a slight change from the previous code, as before 'foo=' was enough to trigger the second check and return true with result = 0, but now we fail instead. And it looks like GetMaxAgeValue doesn't have unit tests :( https://codereview.chromium.org/391763002/diff/100001/net/http/http_response_... File net/http/http_response_headers.h (right): https://codereview.chromium.org/391763002/diff/100001/net/http/http_response_... net/http/http_response_headers.h:234: bool GetCacheControlDirective(const base::StringPiece& directive, If we keep this declaration here we need documentation about what it does and how to use it. It may be better to make this a private member (just because there is no need so far for consumers to call this directly). In that case, the doc can be more sparse.
https://codereview.chromium.org/391763002/diff/100001/net/http/http_cache_uni... File net/http/http_cache_unittest.cc (right): https://codereview.chromium.org/391763002/diff/100001/net/http/http_cache_uni... net/http/http_cache_unittest.cc:6463: const MockTransaction kStaleWhileRevalidateTransaction = { On 2014/07/28 19:24:00, rvargas wrote: > It may be more clear to use a predefined transaction (kSimpleGET ?) and override > the response headers. That way there is less noise of things that are not > related to the test. Done. https://codereview.chromium.org/391763002/diff/100001/net/http/http_response_... File net/http/http_response_headers.cc (right): https://codereview.chromium.org/391763002/diff/100001/net/http/http_response_... net/http/http_response_headers.cc:1121: bool HttpResponseHeaders::GetStaleWhileRevalidateValue( On 2014/07/28 19:24:00, rvargas wrote: > Please add HttpResponseHeaders unit tests to make sure this method behaves as > expected. Done. I didn't add as many tests as for max-age as there isn't any legacy behaviour to worry about. https://codereview.chromium.org/391763002/diff/100001/net/http/http_response_... net/http/http_response_headers.cc:1131: const size_t directive_size = directive.size(); On 2014/07/28 19:24:00, rvargas wrote: > nit: no const May I ask why? const clarifies for the reader that this value will not change and also provides a compiler guarantee of that invariant. It has no cost except 6 letters. https://codereview.chromium.org/391763002/diff/100001/net/http/http_response_... net/http/http_response_headers.cc:1135: if (value.size() > directive_size + 1 && On 2014/07/28 19:24:01, rvargas wrote: > This '+ 1' is a slight change from the previous code, as before 'foo=' was > enough to trigger the second check and return true with result = 0, but now we > fail instead. > > And it looks like GetMaxAgeValue doesn't have unit tests :( I added unit tests for GetMaxAgeValue() and verified the behaviour is the same with both the new and old implementations. max-age= without a value returns false on both (directive_size in the new implementation is one less than kMaxAgePrefixLen in the old implementation). https://codereview.chromium.org/391763002/diff/100001/net/http/http_response_... File net/http/http_response_headers.h (right): https://codereview.chromium.org/391763002/diff/100001/net/http/http_response_... net/http/http_response_headers.h:234: bool GetCacheControlDirective(const base::StringPiece& directive, On 2014/07/28 19:24:01, rvargas wrote: > If we keep this declaration here we need documentation about what it does and > how to use it. > > It may be better to make this a private member (just because there is no need so > far for consumers to call this directly). In that case, the doc can be more > sparse. I originally had it public just because there are lots of similar public methods. I've moved it to private.
LGTM https://codereview.chromium.org/391763002/diff/100001/net/http/http_response_... File net/http/http_response_headers.cc (right): https://codereview.chromium.org/391763002/diff/100001/net/http/http_response_... net/http/http_response_headers.cc:1121: bool HttpResponseHeaders::GetStaleWhileRevalidateValue( On 2014/07/29 17:04:45, Adam Rice wrote: > On 2014/07/28 19:24:00, rvargas wrote: > > Please add HttpResponseHeaders unit tests to make sure this method behaves as > > expected. > > Done. I didn't add as many tests as for max-age as there isn't any legacy > behaviour to worry about. The tests look great, thanks. https://codereview.chromium.org/391763002/diff/100001/net/http/http_response_... net/http/http_response_headers.cc:1131: const size_t directive_size = directive.size(); On 2014/07/29 17:04:45, Adam Rice wrote: > On 2014/07/28 19:24:00, rvargas wrote: > > nit: no const > > May I ask why? const clarifies for the reader that this value will not change > and also provides a compiler guarantee of that invariant. It has no cost except > 6 letters. To me it goes with the overall spirit of not going crazy with using const from the style guide (granted, this is not really excessive, it looks straightforward). This is not a "real" constant, it is just a variable that happens to retain the same value during the whole call (after this assignment), in the current implementation, but there is nothing fundamentally wrong if the code changes in the future and allows the variable to be updated. As for the compiler, it should have no issue performing any optimization when the const is omitted, as it should be able to see that there is no subsequent assignment. In other words, if a local variable should be a constant (have const) it should also be named kFoo (I don't think this line passes that test). The cost is very small, but the value IMO is also very small, and anything that I don't have to read when going over the code counts in my book (as in, ideally, everything in the code should be important and provide valuable information, so that my mind doesn't have to start masking out parts of what I read) https://codereview.chromium.org/391763002/diff/140001/net/http/http_response_... File net/http/http_response_headers.cc (right): https://codereview.chromium.org/391763002/diff/140001/net/http/http_response_... net/http/http_response_headers.cc:756: bool HttpResponseHeaders::GetCacheControlDirective(const StringPiece& directive, wow, methods are quite out of order in this file :(
https://codereview.chromium.org/391763002/diff/100001/net/http/http_response_... File net/http/http_response_headers.cc (right): https://codereview.chromium.org/391763002/diff/100001/net/http/http_response_... net/http/http_response_headers.cc:1131: const size_t directive_size = directive.size(); On 2014/07/29 19:21:22, rvargas wrote: > On 2014/07/29 17:04:45, Adam Rice wrote: > > On 2014/07/28 19:24:00, rvargas wrote: > > > nit: no const > > > > May I ask why? const clarifies for the reader that this value will not change > > and also provides a compiler guarantee of that invariant. It has no cost > except > > 6 letters. > > To me it goes with the overall spirit of not going crazy with using const from > the style guide (granted, this is not really excessive, it looks > straightforward). This is not a "real" constant, it is just a variable that > happens to retain the same value during the whole call (after this assignment), > in the current implementation, but there is nothing fundamentally wrong if the > code changes in the future and allows the variable to be updated. As for the > compiler, it should have no issue performing any optimization when the const is > omitted, as it should be able to see that there is no subsequent assignment. > > In other words, if a local variable should be a constant (have const) it should > also be named kFoo (I don't think this line passes that test). > > The cost is very small, but the value IMO is also very small, and anything that > I don't have to read when going over the code counts in my book (as in, ideally, > everything in the code should be important and provide valuable information, so > that my mind doesn't have to start masking out parts of what I read) Okay, you're the owner so I took it out. For me, anything that reduces the state space I need to consider while reading the code is helpful. https://codereview.chromium.org/391763002/diff/140001/net/http/http_response_... File net/http/http_response_headers.cc (right): https://codereview.chromium.org/391763002/diff/140001/net/http/http_response_... net/http/http_response_headers.cc:756: bool HttpResponseHeaders::GetCacheControlDirective(const StringPiece& directive, On 2014/07/29 19:21:22, rvargas wrote: > wow, methods are quite out of order in this file :( Yes. I probably just made work for you by moving the method. Sorry about that.
The CQ bit was checked by ricea@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ricea@chromium.org/391763002/160001
Message was sent while issue was closed.
Change committed as 286482 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
