Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(155)

Issue 455623003: stale-while-revalidate experimental implementation. (Closed)

Created:
6 years, 4 months ago by Adam Rice
Modified:
6 years, 2 months ago
CC:
chromium-reviews, cbentzel+watch_chromium.org, gavinp+disk_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Project:
chromium
Visibility:
Public.

Description

stale-while-revalidate experimental implementation. Simplistic implementation of stale-while-revalidate. See design doc at https://docs.google.com/document/d/1DMCIMAKjyKeYiu69jlI5OsO2pGyAMb81XflYK4hxsNM/edit?usp=sharing This implementation has known flaws: 1. Set-Cookie, Strict-Transport-Security and Public-Key-Pins headers are lost from the responses of asynchronous revalidations. 2. Conditional requests sent from the Blink cache do not have the stale-while-revalidate logic applied to them. 3. Support for the bandwidth reduction proxy is hacky. It will be replaced with a better implementation if/when the concept is proven. BUG=348877 TEST=net_unittests Committed: https://crrev.com/64c07d7959386d30a0e4aa1371e2bdcd3d436476 Cr-Commit-Position: refs/heads/master@{#298649}

Patch Set 1 #

Patch Set 2 : Implement HttpCache parts of asynchronous validation. #

Patch Set 3 : Separate NetLog source for async revalidation and add tests. #

Patch Set 4 : Rebase. #

Patch Set 5 : Add tests for HEAD and POST methods and to check the URLs match. #

Patch Set 6 : Add tests for load flags suppressing async revalidation and certificate failure causing synchronous… #

Patch Set 7 : clang-format fix. #

Patch Set 8 : Rebase. #

Total comments: 32

Patch Set 9 : Add new flags to histograms.xml #

Patch Set 10 : Fixes from yhirano review. #

Patch Set 11 : Renames RequiresValidation() to RequiredValidationType() #

Patch Set 12 : Add HttpCache.AsyncValidationTimeSaved histogram. #

Patch Set 13 : Support the bandwidth reduction proxy. #

Total comments: 4

Patch Set 14 : Ensure |lifetime| is always cleared in GetFreshnessLifetime() #

Patch Set 15 : Stop setting Cache-Control: max-age=0 on async revalidations. #

Total comments: 41

Patch Set 16 : Clean up GetFreshnessLifetime() miscellaneous other things. #

Patch Set 17 : Add load flag LOAD_ASYNC_REVALIDATION. #

Total comments: 29

Patch Set 18 : GetFreshnessLifetimes now returns two times. Duplicate AsyncValidations for the same URL are discar… #

Total comments: 25

Patch Set 19 : Cleanups. #

Total comments: 7

Patch Set 20 : Remove --disable-stale-while-revalidate flag and strengthen must-revalidate. #

Total comments: 18

Patch Set 21 : Unit test fixes. #

Patch Set 22 : Add support for restarting a transaction for auth. #

Patch Set 23 : Rebase. #

Patch Set 24 : Compile fix for ResourcePrefreshPredictor. #

Total comments: 1

Patch Set 25 : Clarify the summary for the histogram. #

Patch Set 26 : Rebase. #

Total comments: 17
Unified diffs Side-by-side diffs Delta from patch set Stats (+1029 lines, -119 lines) Patch
M build/ios/grit_whitelist.txt View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/app/generated_resources.grd View 1 chunk +6 lines, -0 lines 0 comments Download
M chrome/browser/about_flags.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 1 chunk +7 lines, -0 lines 0 comments Download
M chrome/browser/io_thread.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/io_thread.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 4 chunks +10 lines, -0 lines 4 comments Download
M chrome/browser/net/chrome_network_delegate.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 1 chunk +4 lines, -2 lines 2 comments Download
M chrome/browser/predictors/resource_prefetch_predictor.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/browser/resources/net_internals/source_entry.js View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M chrome/common/chrome_switches.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 1 chunk +1 line, -0 lines 0 comments Download
M chrome/common/chrome_switches.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 1 chunk +5 lines, -0 lines 0 comments Download
M net/base/load_flags_list.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +3 lines, -0 lines 0 comments Download
M net/base/net_log_event_type_list.h View 1 2 3 4 5 6 7 8 9 1 chunk +23 lines, -0 lines 0 comments Download
M net/base/net_log_source_type_list.h View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M net/http/http_cache.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 6 chunks +29 lines, -0 lines 0 comments Download
M net/http/http_cache.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 6 chunks +200 lines, -0 lines 0 comments Download
M net/http/http_cache_transaction.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 3 chunks +7 lines, -2 lines 0 comments Download
M net/http/http_cache_transaction.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 9 chunks +65 lines, -20 lines 0 comments Download
M net/http/http_cache_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 4 chunks +464 lines, -28 lines 0 comments Download
M net/http/http_network_session.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 1 chunk +1 line, -0 lines 0 comments Download
M net/http/http_network_session.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 1 chunk +1 line, -0 lines 0 comments Download
M net/http/http_response_headers.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 3 chunks +35 lines, -12 lines 11 comments Download
M net/http/http_response_headers.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 4 chunks +60 lines, -27 lines 0 comments Download
M net/http/http_response_headers_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 6 chunks +82 lines, -24 lines 0 comments Download
M net/http/http_transaction_test_util.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 1 chunk +7 lines, -2 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 2 chunks +12 lines, -0 lines 0 comments Download

Messages

Total messages: 41 (2 generated)
Adam Rice
6 years, 4 months ago (2014-08-25 15:26:59 UTC) #1
yhirano
https://codereview.chromium.org/455623003/diff/140001/chrome/browser/io_thread.cc File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/455623003/diff/140001/chrome/browser/io_thread.cc#newcode712 chrome/browser/io_thread.cc:712: if (base::FieldTrialList::FindFullName(kStaleWhileRevalidateFieldTrialName) == Can you tell me when and ...
6 years, 3 months ago (2014-08-26 07:52:02 UTC) #2
Adam Rice
https://codereview.chromium.org/455623003/diff/140001/chrome/browser/io_thread.cc File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/455623003/diff/140001/chrome/browser/io_thread.cc#newcode712 chrome/browser/io_thread.cc:712: if (base::FieldTrialList::FindFullName(kStaleWhileRevalidateFieldTrialName) == On 2014/08/26 07:52:01, yhirano wrote: > ...
6 years, 3 months ago (2014-08-26 13:38:43 UTC) #3
Adam Rice
Sorry about the extra changes. I have stopped adding stuff now. PTAL.
6 years, 3 months ago (2014-08-27 04:36:50 UTC) #4
yhirano
https://codereview.chromium.org/455623003/diff/240001/net/http/http_cache.cc File net/http/http_cache.cc (right): https://codereview.chromium.org/455623003/diff/240001/net/http/http_cache.cc#newcode335 net/http/http_cache.cc:335: if (network_delegate) { Can you pass before_proxy_headers_sent_callback_ from HttpCache::Transaction? ...
6 years, 3 months ago (2014-08-27 09:24:29 UTC) #5
Adam Rice
I will have to add another patch set after this one because I found a ...
6 years, 3 months ago (2014-08-28 03:14:35 UTC) #6
yhirano
lgtm
6 years, 3 months ago (2014-08-28 06:34:16 UTC) #7
Adam Rice
ricea@chromium.org changed reviewers: + rvargas@chromium.org
6 years, 3 months ago (2014-08-28 11:42:17 UTC) #8
Adam Rice
+rvargas PTAL. As you probably know, there has been some controversy about checking this in ...
6 years, 3 months ago (2014-08-28 11:42:17 UTC) #9
rvargas (doing something else)
I'm sorry we didn't catch issues with the design doc, especially because it is exceptionally ...
6 years, 3 months ago (2014-08-29 23:36:07 UTC) #10
Adam Rice
I have added a section to the implementation design doc about how we back out ...
6 years, 3 months ago (2014-09-01 14:43:00 UTC) #11
rvargas (doing something else)
https://codereview.chromium.org/455623003/diff/280001/net/http/http_cache.cc File net/http/http_cache.cc (right): https://codereview.chromium.org/455623003/diff/280001/net/http/http_cache.cc#newcode359 net/http/http_cache.cc:359: transaction_->ValidateInternalCache(); On 2014/09/01 14:43:00, Adam Rice wrote: > On ...
6 years, 3 months ago (2014-09-03 01:25:53 UTC) #12
Adam Rice
https://codereview.chromium.org/455623003/diff/280001/net/http/http_cache.cc File net/http/http_cache.cc (right): https://codereview.chromium.org/455623003/diff/280001/net/http/http_cache.cc#newcode359 net/http/http_cache.cc:359: transaction_->ValidateInternalCache(); On 2014/09/03 01:25:53, rvargas wrote: > On 2014/09/01 ...
6 years, 3 months ago (2014-09-05 15:17:11 UTC) #13
rvargas (doing something else)
https://codereview.chromium.org/455623003/diff/280001/net/http/http_cache.cc File net/http/http_cache.cc (right): https://codereview.chromium.org/455623003/diff/280001/net/http/http_cache.cc#newcode412 net/http/http_cache.cc:412: cache_->DoomEntry(transaction_->key(), transaction_.get()); On 2014/09/05 15:17:11, Adam Rice wrote: > ...
6 years, 3 months ago (2014-09-05 22:52:40 UTC) #14
Adam Rice
https://codereview.chromium.org/455623003/diff/280001/net/http/http_cache.cc File net/http/http_cache.cc (right): https://codereview.chromium.org/455623003/diff/280001/net/http/http_cache.cc#newcode412 net/http/http_cache.cc:412: cache_->DoomEntry(transaction_->key(), transaction_.get()); On 2014/09/05 22:52:39, rvargas wrote: > On ...
6 years, 3 months ago (2014-09-09 12:36:46 UTC) #15
rvargas (doing something else)
https://codereview.chromium.org/455623003/diff/320001/net/base/load_flags_list.h File net/base/load_flags_list.h (right): https://codereview.chromium.org/455623003/diff/320001/net/base/load_flags_list.h#newcode130 net/base/load_flags_list.h:130: // flag just prevents re-entering the async code path, ...
6 years, 3 months ago (2014-09-11 02:33:07 UTC) #16
Adam Rice
https://codereview.chromium.org/455623003/diff/320001/net/http/http_cache.cc File net/http/http_cache.cc (right): https://codereview.chromium.org/455623003/diff/320001/net/http/http_cache.cc#newcode360 net/http/http_cache.cc:360: transaction_.reset(static_cast<Transaction*>(transaction.release())); On 2014/09/11 02:33:05, rvargas wrote: > On 2014/09/09 ...
6 years, 3 months ago (2014-09-12 01:46:49 UTC) #17
rvargas (doing something else)
https://codereview.chromium.org/455623003/diff/320001/net/http/http_response_headers.cc File net/http/http_response_headers.cc (right): https://codereview.chromium.org/455623003/diff/320001/net/http/http_response_headers.cc#newcode1028 net/http/http_response_headers.cc:1028: return Freshness(lifetime); On 2014/09/12 01:46:48, Adam Rice wrote: > ...
6 years, 3 months ago (2014-09-12 18:33:13 UTC) #18
Adam Rice
https://codereview.chromium.org/455623003/diff/320001/net/http/http_response_headers.cc File net/http/http_response_headers.cc (right): https://codereview.chromium.org/455623003/diff/320001/net/http/http_response_headers.cc#newcode1028 net/http/http_response_headers.cc:1028: return Freshness(lifetime); On 2014/09/12 18:33:12, rvargas wrote: > On ...
6 years, 3 months ago (2014-09-16 11:53:01 UTC) #19
rvargas (doing something else)
lgtm, thanks. https://codereview.chromium.org/455623003/diff/360001/net/http/http_response_headers.cc File net/http/http_response_headers.cc (right): https://codereview.chromium.org/455623003/diff/360001/net/http/http_response_headers.cc#newcode1015 net/http/http_response_headers.cc:1015: HasHeaderValue("vary", "*")) { // see RFC 2616 ...
6 years, 3 months ago (2014-09-17 04:32:20 UTC) #20
Adam Rice
https://codereview.chromium.org/455623003/diff/380001/net/http/http_cache_unittest.cc File net/http/http_cache_unittest.cc (right): https://codereview.chromium.org/455623003/diff/380001/net/http/http_cache_unittest.cc#newcode42 net/http/http_cache_unittest.cc:42: using base::RunLoop; On 2014/09/17 04:32:19, rvargas wrote: > nit: ...
6 years, 2 months ago (2014-09-30 13:44:51 UTC) #21
Adam Rice
I added support to HC::AsyncValidation for restarting the transaction for auth. I had to do ...
6 years, 2 months ago (2014-10-02 14:47:42 UTC) #22
rvargas (doing something else)
PS22 lgtm
6 years, 2 months ago (2014-10-02 23:21:43 UTC) #23
Adam Rice
+asvitkine for histograms.xml (new histogram is logged at http_cache.cc:443) +mmenke for chrome/browser/io_thread.*, chrome/browser/net, chrome/browser/resources/net_internals/ +thestig ...
6 years, 2 months ago (2014-10-07 08:26:47 UTC) #25
Alexei Svitkine (slow)
lgtm % nit https://codereview.chromium.org/455623003/diff/460001/tools/metrics/histograms/histograms.xml File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/455623003/diff/460001/tools/metrics/histograms/histograms.xml#newcode9822 tools/metrics/histograms/histograms.xml:9822: + The time spent doing HTTP ...
6 years, 2 months ago (2014-10-07 18:25:44 UTC) #26
Lei Zhang
chrome/browser/predictors/ lgtm, but please try someone in chrome/browser/predictors/OWNERS first next time.
6 years, 2 months ago (2014-10-07 18:56:20 UTC) #27
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/455623003/500001
6 years, 2 months ago (2014-10-08 01:38:19 UTC) #29
commit-bot: I haz the power
Committed patchset #26 (id:500001) as 0b4c8f92e4a0e40ae3824918eda7d3eb0075f688
6 years, 2 months ago (2014-10-08 03:37:12 UTC) #30
commit-bot: I haz the power
Patchset 26 (id:??) landed as https://crrev.com/64c07d7959386d30a0e4aa1371e2bdcd3d436476 Cr-Commit-Position: refs/heads/master@{#298649}
6 years, 2 months ago (2014-10-08 03:37:57 UTC) #31
Adam Rice
+mmenke Sorry, I didn't realise I was missing an lgtm. Do you have anything you'd ...
6 years, 2 months ago (2014-10-09 09:10:56 UTC) #32
mmenke
A couple nits. https://codereview.chromium.org/455623003/diff/500001/chrome/browser/io_thread.cc File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/455623003/diff/500001/chrome/browser/io_thread.cc#newcode667 chrome/browser/io_thread.cc:667: globals_->use_stale_while_revalidate = true; nit: Use braces ...
6 years, 2 months ago (2014-10-09 14:29:58 UTC) #33
mmenke
Oh, and sorry about slowness - the entire network stack team is at an offsite. ...
6 years, 2 months ago (2014-10-09 14:30:49 UTC) #34
rvargas (doing something else)
https://codereview.chromium.org/455623003/diff/500001/net/http/http_response_headers.h File net/http/http_response_headers.h (right): https://codereview.chromium.org/455623003/diff/500001/net/http/http_response_headers.h#newcode57 net/http/http_response_headers.h:57: base::TimeDelta stale; On 2014/10/09 14:29:58, mmenke wrote: > I ...
6 years, 2 months ago (2014-10-09 19:13:45 UTC) #35
mmenke
https://codereview.chromium.org/455623003/diff/500001/net/http/http_response_headers.h File net/http/http_response_headers.h (right): https://codereview.chromium.org/455623003/diff/500001/net/http/http_response_headers.h#newcode57 net/http/http_response_headers.h:57: base::TimeDelta stale; On 2014/10/09 19:13:45, rvargas wrote: > On ...
6 years, 2 months ago (2014-10-09 19:32:01 UTC) #36
rvargas (doing something else)
https://codereview.chromium.org/455623003/diff/500001/net/http/http_response_headers.h File net/http/http_response_headers.h (right): https://codereview.chromium.org/455623003/diff/500001/net/http/http_response_headers.h#newcode57 net/http/http_response_headers.h:57: base::TimeDelta stale; On 2014/10/09 19:32:01, mmenke wrote: > On ...
6 years, 2 months ago (2014-10-09 21:29:32 UTC) #37
Alexei Svitkine (slow)
https://codereview.chromium.org/455623003/diff/500001/chrome/browser/io_thread.cc File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/455623003/diff/500001/chrome/browser/io_thread.cc#newcode663 chrome/browser/io_thread.cc:663: if (command_line.HasSwitch(switches::kEnableStaleWhileRevalidate)) I didn't review this part earlier, but ...
6 years, 2 months ago (2014-10-09 21:33:39 UTC) #38
rvargas (doing something else)
https://codereview.chromium.org/455623003/diff/500001/net/http/http_response_headers.h File net/http/http_response_headers.h (right): https://codereview.chromium.org/455623003/diff/500001/net/http/http_response_headers.h#newcode57 net/http/http_response_headers.h:57: base::TimeDelta stale; On 2014/10/09 21:29:32, rvargas wrote: > On ...
6 years, 2 months ago (2014-10-09 21:47:14 UTC) #39
mmenke
https://codereview.chromium.org/455623003/diff/500001/net/http/http_response_headers.h File net/http/http_response_headers.h (right): https://codereview.chromium.org/455623003/diff/500001/net/http/http_response_headers.h#newcode57 net/http/http_response_headers.h:57: base::TimeDelta stale; On 2014/10/09 21:47:14, rvargas wrote: > On ...
6 years, 2 months ago (2014-10-09 21:50:28 UTC) #40
Adam Rice
6 years, 2 months ago (2014-10-20 09:31:24 UTC) #41
Message was sent while issue was closed.
https://codereview.chromium.org/455623003/diff/500001/chrome/browser/io_threa...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/455623003/diff/500001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:663: if
(command_line.HasSwitch(switches::kEnableStaleWhileRevalidate))
On 2014/10/09 21:33:39, Alexei Svitkine wrote:
> I didn't review this part earlier, but now that I see this, I suggest using
the
> pattern suggested at go/finch-and-flags (where you assign the value from the
> field trial to a local first, so that the trial is activated and then check
the
> command-line, followed by the group comparison).

Done in https://crrev.com/665023002/

https://codereview.chromium.org/455623003/diff/500001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:667: globals_->use_stale_while_revalidate = true;
On 2014/10/09 14:29:58, mmenke wrote:
> nit:  Use braces when the conditional of an if statement takes more than one
> line.

Indirectly fixed by change requested by asvitkine.

https://codereview.chromium.org/455623003/diff/500001/net/http/http_response_...
File net/http/http_response_headers.h (right):

https://codereview.chromium.org/455623003/diff/500001/net/http/http_response_...
net/http/http_response_headers.h:32: VALIDATION_NONE = 0,      // The resource
is fresh.
On 2014/10/09 14:29:58, mmenke wrote:
> Does this "= 0" give us anything?

No. But all the cool kids are doing it.

Removed in https://crrev.com/665023002/

https://codereview.chromium.org/455623003/diff/500001/net/http/http_response_...
net/http/http_response_headers.h:57: base::TimeDelta stale;
On 2014/10/09 21:50:28, mmenke wrote:
> On 2014/10/09 21:47:14, rvargas wrote:
> > On 2014/10/09 21:29:32, rvargas wrote:
> > > On 2014/10/09 19:32:01, mmenke wrote:
> > > > On 2014/10/09 19:13:45, rvargas wrote:
> > > > > On 2014/10/09 14:29:58, mmenke wrote:
> > > > > > I think these names are very confusing - it doesn't imply they're
> > deltas. 
> > > > > > "fresh" sounds like a bool to me "FreshnessLifetime" sounds like
> either
> > an
> > > > > > expiration time, or total duration, neither of which is correct, so
> > > doesn't
> > > > > > really help.
> > > > > > 
> > > > > > Also, "fresh" and "stale" seem a bit confusing - generally something
> is
> > > > > > considered fresh until it's stale.
> > > > > > 
> > > > > > Maybe rename these:
> > > > > > fresh -> fresh_time_remaining
> > > > > > stale -> usable_time_remaining (Or
> stale_while_revalidate_time_remaining
> > -
> > > a
> > > > > bit
> > > > > > of a mouthful, but clearer)
> > > > > > 
> > > > > > Or maybe time_until_stale / time_until_unusable?  I'm open to other
> > ideas.
> > > > > 
> > > > > FreshnessLifetime is the standard term for time delta until the
resource
> > > > becomes
> > > > > stale: http://tools.ietf.org/html/rfc7234#section-4.2.1 so it should
> > provide
> > > > > enough context for the meaning of the members (maybe add a link to the
> > > rfc?).
> > > > > The idea is that now we compute two lifetimes instead of one.
> > > > >
> > > > > maybe fresh_delta or freshness_delta? I don't want to deviate too much
> > from
> > > > the
> > > > > standard name.
> > > > 
> > > > 
> > > > Thanks for the link!  I guess this function only makes sense for for
> > recently
> > > > received headers, and not those read from the cache, which means the
time
> > from
> > > > now until expiration is the same as freshness lifetime?
> > > 
> > > GetFreshnessLifetimes() is used by RequiresValidation() to calculate
> > validation
> > > requirements for cached content.
> > > 
> > > > 
> > > > "Fresh" really does sounds like a bool to me - either something is
fresh,
> or
> > > it
> > > > isn't, which initially confused me.  Since it's a defined term, as you
> point
> > > > out, maybe just freshness_lifetime?
> > > 
> > > That works for me.
> > > 
> > > > 
> > > > > The second part is more complicated because from the point of view of
> the
> > > > > standard there are multiple conditions that allow use of stale data
even
> > > > without
> > > > > considering SWR.
> > > > > 
> > > > > maybe stale_while_revalidate_delta?
> > > > > 
> > > > > I'm not very fond of adding part of the type name to the name of
> variables
> > > > > though.
> > > > 
> > > > Understandable - given my misunderstanding about when this method is
used,
> > I'm
> > > > fine with using something that just implies "not a bool", rather than
> > > > specifically "time delta".  stale_while_revalidate_until?
> > > > 
> > > 
> > > "until" suggests to me an absolute value (base::Time) :(
> > > 
> > > Maybe stale_while_revalidate_lifetime? stale_while_revalidate_value?
> > > 
> > > > Anyhow, I'm not really too picky.
> > > 
> > 
> > BTW, another thing to consider is how the code reads when it is used.
> > 
> > Today this means
> >   lifetime.fresh.InSeconds(),
> >   lifetime.stale.InSeconds(),
> > 
> > and
> >   TimeDelta freshness_lifetime =
> >      foo->GetFreshnessLifetimes(blah).fresh;
> > 
> > crazy idea: 
> >   lifetime.freshness.InSeconds(),
> >   lifetime.staleness.InSeconds(),
> > 
> > on the upside (and downside), I don't think "staleness" means anything :p
> 
> I was looking at it in chrome_network_delegate.cc..By the time I'd gotten to
the
> end of that line, I'd forgotten about what the start looked like.

I have implemented the freshness/staleness approach in
https://crrev.com/665023002/

Just to complicate things, I thought of another possible naming strategy:

UsableLifetime usable_lifetime = headers.GetUsableLifetime(...);
usable_lifetime.as_fresh.InSeconds(),
usable_lifetime.as_stale.InSeconds()

https://codereview.chromium.org/455623003/diff/500001/net/http/http_response_...
net/http/http_response_headers.h:234: // without revalidation, and
|FreshnessLifetimes.stale| to the length of time
On 2014/10/09 14:29:58, mmenke wrote:
> nit:  Only use || around argument names.  I documenting FreshnessLifetime's
> fields again here isn't needed, since it's redundant with the struct - suspect
> the documentation here is much more likely to become outdated if things ever
> change.  Fine to mention RFC5861, and the usable while stale behavior.

Done.

Powered by Google App Engine
This is Rietveld 408576698