Add GetExpirationTimes() to HttpResponseHeader.

Add GetExpirationTimes() to HttpResponseHeaders.

This is the first step in propagating the explicit freshness lifetime
to blink. The eventual goal is to remove much of the code parsing http
caching headers in blink.

R=japhet@chromium.org,rdsmith@chromium.org,darin@chromium.org,kinuko@chromium.org,mmenke@chromium.org
BUG=340087

[gavinp, 2014-02-05 16:13:46]

rvargas: PTAL.

japhet: FYI.

I was tempted to put the freshness expiry time in the ResponseInfo, however it
would have had to be assigned many times. The only advantage would have been to
avoid a double computation of the freshness lifetime in a few cases.

[gavinp, 2014-02-05 16:43:04]

Hold off on reviewing, I had not realised how much of net was exported here.
There's more work.

[gavinp, 2014-02-05 20:26:31]

+rdsmith,michaeln

PTAL: rvargas
PTAL(appcache_update_job.cc): michaeln

FYI: japhet, rdsmith

Can't wait to see the freshness expiry make it up to blink!

[rvargas (doing something else), 2014-02-06 22:39:58]

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

https://codereview.chromium.org/154243006/diff/460001/net/http/http_response_...
net/http/http_response_headers.h:216: 
How about adding a new method instead:

Time HttpResponseHeaders::GetFreshnessLimit(const Time& request_time,
                                            const Time& response_time,
                                            const Time& current_time) const {
  TimeDelta lifetime = GetFreshnessLifetime(response_time);
  if (lifetime == TimeDelta())
    return current_time;

  TimeDelta age = GetCurrentAge(request_time, response_time, current_time);
  if (lifetime <= age)
    return current_time;

  return current_time + lifetime - age;
}

If you don't like the small code duplication we can have:

bool HttpResponseHeaders::RequiresValidation(const Time& request_time,
                                             const Time& response_time,
                                             const Time& current_time) const {
  return GetFreshnessLimit(request_time, response_time, current_time) ==
         current_time;
}

[gavinp, 2014-02-07 15:59:18]

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

https://codereview.chromium.org/154243006/diff/460001/net/http/http_response_...
net/http/http_response_headers.h:216: 
On 2014/02/06 22:39:58, rvargas wrote:
> 
> If you don't like the small code duplication we can have:
> 
> bool HttpResponseHeaders::RequiresValidation(const Time& request_time,
>                                              const Time& response_time,
>                                              const Time& current_time) const {
>   return GetFreshnessLimit(request_time, response_time, current_time) ==
>          current_time;
> }

I originally considered this second idea, of keeping a RequiresValidation()
method so that we wouldn't have to change the call sites. I decided against that
approach for two reasons:

1. API surface area. The call sites don't benefit very much from the
RequiresValidation() call; and so changing the call sites seemed preferable to
having a second method that isn't needed if we make those few changes.

2. The benefit of removing "current_time" parameter. The documentation claimed
it was only for testing, and certainly every call site except the unit test was
passing in Time::Now(). Using FreshnessExpiry meant that this extra parameter
wasn't necessary even for testing, so that's a simplification of the API.

[rvargas (doing something else), 2014-02-08 03:54:09]

On 2014/02/07 15:59:18, gavinp wrote:
>
https://codereview.chromium.org/154243006/diff/460001/net/http/http_response_...
> File net/http/http_response_headers.h (right):
> 
>
https://codereview.chromium.org/154243006/diff/460001/net/http/http_response_...
> net/http/http_response_headers.h:216: 
> On 2014/02/06 22:39:58, rvargas wrote:
> > 
> > If you don't like the small code duplication we can have:
> > 
> > bool HttpResponseHeaders::RequiresValidation(const Time& request_time,
> >                                              const Time& response_time,
> >                                              const Time& current_time) const
{
> >   return GetFreshnessLimit(request_time, response_time, current_time) ==
> >          current_time;
> > }
> 
> I originally considered this second idea, of keeping a RequiresValidation()
> method so that we wouldn't have to change the call sites. I decided against
that
> approach for two reasons:
> 
> 1. API surface area. The call sites don't benefit very much from the
> RequiresValidation() call; and so changing the call sites seemed preferable to
> having a second method that isn't needed if we make those few changes.
> 
> 2. The benefit of removing "current_time" parameter. The documentation claimed
> it was only for testing, and certainly every call site except the unit test
was
> passing in Time::Now(). Using FreshnessExpiry meant that this extra parameter
> wasn't necessary even for testing, so that's a simplification of the API.

I'm really not concerned about changing the callers (only two places?). My main
concern is to change the semantics and name of a very old function that is
deeply embedded into my model of how things work. RequiresValidation is probably
the symbol that I look for more often on response headers as the reports of "the
cache is [not] updating foo" are quite common. I understand that this is a very
selfish reason, but that's what I have.

I would have a hard time remembering "GetFreshnessExpiry". GetFreshnessOrigin
doesn't tell me anything about what it is doing without having to read the
explanation, and GetFreshnessLifetime(base::Time response_time, base::TimeDelta*
finite_freshness_lifetime, bool* was_finite) also looks extremely cryptic to me
without having to read the explanation.

It may be just a matter of familiarity with the old API, but it seems more
self-explanatory than the new one. If the advantages of the new API were more
evident it would be easier to re-train myself, but as it stands it is not
cutting it for me.

[gavinp, 2014-02-12 17:49:46]

-michaeln, as we now have a compatibility function.

rvargas, PTAL. I've added back RequiresValidation() as you asked.

[rvargas (doing something else), 2014-02-12 20:00:16]

I'm sorry, but as I mentioned before I don't find some of the functions that you
are adding or modifying clear enough from the point of view of the API, and I
must insist on just adding the method that you want based on the existing code.

[gavinp, 2014-02-13 17:37:59]

I've now rebased this patch, and it is strictly downstream of the CL
https://codereview.chromium.org/163413004/ .

https://codereview.chromium.org/154243006/diff/980004/net/http/http_response_...
File net/http/http_response_headers.cc (left):

https://codereview.chromium.org/154243006/diff/980004/net/http/http_response_...
net/http/http_response_headers.cc:1012: return
TimeDelta::FromMicroseconds(kint64max);
This line is an abstraction violation that skates very close to using undefined
behaviour. There's no good way to test for this value in a TimeDelta, and so it
can't be used for time math. The current code works because no call site adds a
FreshnessLifetime to another time or time delta. That's a lucky break. The tests
for this CL failed because they do such adds, which overflow a base::Time and
have an undefined result. There's no good way to test for this without violating
abstractions even more. It's bad that this line is here, but it would be worse
to add more of this stuff.

This is why we had to change the signature of GetFreshnessLifetime() to have the
bool. I am not a fan of the signature that resulted from this change either. It
is too cryptic.

A better solution is to add, to base::TimeDelta, an idea of Max()/is_max() like
exists for base::Time(). I'm pursing this change in
https://chromiumcodereview.appspot.com/163413004/ which edits this line as
https://chromiumcodereview.appspot.com/163413004/diff/220001/net/http/http_re...
.

I'm rebasing this CL on top of that CL, so we can remove all changes to
GetFreshnessLifetime() from this CL.

[gavinp, 2014-02-13 17:46:22]

On 2014/02/12 20:00:16, rvargas wrote:
> I'm sorry, but as I mentioned before I don't find some of the functions that
you
> are adding or modifying clear enough from the point of view of the API, and I
> must insist on just adding the method that you want based on the existing
code.

In https://codereview.chromium.org/154243006/#msg4 , you explicitly proposed
defining RequiresValidation in terms of GetFreshnessExpiry, but I guess you
changed your mind.

The latest upload fixes your earlier concerns about GetFreshnessLifetime(), by
making upstream changes that permit a concept of a maximum TimeDelta.

Your suggestion that this method be defined in terms of the existing methods is
perhaps simplest now, but it conceptually is more complex and makes our code
more fragile. If we define GetFreshnessExpiry in terms of GetCurrentAge as
proposed in https://codereview.chromium.org/154243006/#msg4 , we will be adding
and subtracting the current_time to calculate the FreshnessExpiry, which need
not depend on the time. Unnecessary additions and subtractions of unneeded
concepts passed in with unnecessary parameters does not simplify things.

[gavinp, 2014-02-13 18:50:25]

https://codereview.chromium.org/154243006/diff/1580003/net/http/http_response...
File net/http/http_response_headers.cc (right):

https://codereview.chromium.org/154243006/diff/1580003/net/http/http_response...
net/http/http_response_headers.cc:930: bool
HttpResponseHeaders::RequiresValidation(base::Time request_time,
s/base::Time/Time/

[rvargas (doing something else), 2014-02-13 22:16:25]

On 2014/02/13 17:46:22, gavinp wrote:
> On 2014/02/12 20:00:16, rvargas wrote:
> > I'm sorry, but as I mentioned before I don't find some of the functions that
> you
> > are adding or modifying clear enough from the point of view of the API, and
I
> > must insist on just adding the method that you want based on the existing
> code.
> 
> In https://codereview.chromium.org/154243006/#msg4 , you explicitly proposed
> defining RequiresValidation in terms of GetFreshnessExpiry, but I guess you
> changed your mind.

No, I definitely have not changed my mind. As I said on that comment: "If you
don't like the small code duplication...". I just don't see the need to repeat
every detail with every message, but maybe I should.

> 
> The latest upload fixes your earlier concerns about GetFreshnessLifetime(), by
> making upstream changes that permit a concept of a maximum TimeDelta.
> 
> Your suggestion that this method be defined in terms of the existing methods
is
> perhaps simplest now, but it conceptually is more complex and makes our code
> more fragile. If we define GetFreshnessExpiry in terms of GetCurrentAge as
> proposed in https://codereview.chromium.org/154243006/#msg4 , we will be
adding
> and subtracting the current_time to calculate the FreshnessExpiry, which need
> not depend on the time. Unnecessary additions and subtractions of unneeded
> concepts passed in with unnecessary parameters does not simplify things.

Looking at the code that I posted on comment 4, it seems that your concern about
"unnecessary additions and subtractions" can be directed to:

return current_time + lifetime - age;

where you object having to add |current_time|. That looks to me like an
extremely minor thing to be arguing about. Are you really worried about one
extra CPU cycle doing that addition?

If your objection is that GetCurrentAge is not clear or can be optimized, I
would agree that it certainly can be optimized and made it less verbose;
however, I think it is valuable to have that function be an explicit and direct
transcription of RFC 2616 section 13.2.3 (as stated in the comment), leaving the
optimization to the compiler. I see no references to "freshness origin" on that
RFC.

[gavinp, 2014-02-26 01:54:11]

On 2014/02/13 22:16:25, rvargas wrote:
> On 2014/02/13 17:46:22, gavinp wrote:
> > On 2014/02/12 20:00:16, rvargas wrote:
> > > I'm sorry, but as I mentioned before I don't find some of the functions
that
> > you
> > > are adding or modifying clear enough from the point of view of the API,
and
> I
> > > must insist on just adding the method that you want based on the existing
> > code.
> > 
> > In https://codereview.chromium.org/154243006/#msg4 , you explicitly proposed
> > defining RequiresValidation in terms of GetFreshnessExpiry, but I guess you
> > changed your mind.
> 
> No, I definitely have not changed my mind. As I said on that comment: "If you
> don't like the small code duplication...". I just don't see the need to repeat
> every detail with every message, but maybe I should.

Before I uploaded patch set 11
https://codereview.chromium.org/154243006/#ps980004 , I read
https://codereview.chromium.org/154243006/#msg4 . You now tell me that comment
#4 was intended to say that the second option you presented is totally
unacceptable. I read, and reread that comment, and I cannot find that reading in
it. In fact, my reading is basically that it is permissive, saying that the
second proposed approach can be used. It is very unfortunate that I read message
#4 as having the opposite meaning of what you intended. Even with your
clarifications and restatement in message #12 and message #8, I can't find the
reading you intended in message #4.

That's really unfortunate. I think you're being sarcastic when you suggest that
you repeat every detail with every message, however, more verbosity and more
clarity would definitely help.

> > 
> > The latest upload fixes your earlier concerns about GetFreshnessLifetime(),
by
> > making upstream changes that permit a concept of a maximum TimeDelta.
> > 
> > Your suggestion that this method be defined in terms of the existing methods
> is
> > perhaps simplest now, but it conceptually is more complex and makes our code
> > more fragile. If we define GetFreshnessExpiry in terms of GetCurrentAge as
> > proposed in https://codereview.chromium.org/154243006/#msg4 , we will be
> adding
> > and subtracting the current_time to calculate the FreshnessExpiry, which
need
> > not depend on the time. Unnecessary additions and subtractions of unneeded
> > concepts passed in with unnecessary parameters does not simplify things.
> 
> Looking at the code that I posted on comment 4, it seems that your concern
about
> "unnecessary additions and subtractions" can be directed to:
> 
> return current_time + lifetime - age;
> 
> where you object having to add |current_time|. That looks to me like an
> extremely minor thing to be arguing about. Are you really worried about one
> extra CPU cycle doing that addition?

No, that is not my objection.

The concern is about conceptual complexity. The class is simpler when the
methods take fewer arguments (i.e. not current_time). The comment on
RequiresValidation indicates the current_time is passed only for debugging,
even! Further, current_time makes these functions look strangely non-idempotent.
Reformulating them in a way that doesn't vary with current_time makes the
concepts simpler.

There's a big bonus coming down stream of course, in that this freshness expiry
concept is really useful for cutting out bug-causing duplicate header parses in
blink. Of course we can achieve that with the implementation based on the
current methods, but while we're here I think let's simplify and clean up this
code.

Consider, for instance, this implementation of GetFreshnessExpiry(), based on
the one in patch set #4 that you proposed:

Time HttpResponseHeaders::GetFreshnessExpiry(Time request_time, Time
response_time) const {
  TimeDelta lifetime = GetFreshnessLifetime(response_time);
  if (lifetime == TimeDelta())
    return Time();

  // Compute resource lifetime and age relative to the attack on Pearl Harbor.
  const Time::Exploded exploded_time_of_infamy = { 1942, 12, 0, 7, 17, 48, 0, 0
};
  const Time time_of_infamy = Time::FromUTCExploded(exploded_time_of_infamy);
  const TimeDelta age = GetCurrentAge(request_time, response_time,
time_of_infamy);

  return time_of_infamy + lifetime - age;
}

This function is perfectly correct; since |GetCurrentAge(a, b, c) = c -
GetFreshnessOrigin(a, b)|; so this reduces to the same thing, modulo a few
overflow issues happening as the current_time approaches base::Time::Max(). Yet,
when I read it, it's pretty strange that we chose this arbitrary time. I submit
that current_time is equally arbitrary. Is there some reason that it's better to
add and subtract current_time over the time_of_infamy?

> 
> If your objection is that GetCurrentAge is not clear or can be optimized, I
> would agree that it certainly can be optimized and made it less verbose;

I would exclamation-mark ell gee tee em any such change that "optimized" that
code. The math should follow the RFC as closely as reasonable.

> however, I think it is valuable to have that function be an explicit and
direct
> transcription of RFC 2616 section 13.2.3 (as stated in the comment), leaving
the
> optimization to the compiler. I see no references to "freshness origin" on
that
> RFC.

As an alternative, I'm preparing an upload that gets rid of the (effectively in
use private) GetFreshnessOrigin/GetCurrentAge function, eliding it into the
GetFreshnessExpiry() function, which was the only call site anyway.

While it's true that freshness lifetime and freshness origin are not defined in
the RFC, their derivation is clearly described, and the concepts are useful.

Further, if not being defined in the RFC means the code is bad, then why is this
legacy name RequiresValidation OK? There's no such concept defined in the RFC,
except in a reading that combines section 13.2.4's definition of "stale"
together with the first paragraph of 13.3! That's pretty synthetic, and really
about as tricky as the definition of FreshnessExpiry. I don't think the spec is
an implementation; having our code obviously follow it is good when we can, but
diverging because the spec had a bit of not-needed conceptual complexity is
probably a good idea. I guess I'm proposing that this upload is such a
divergence.

[gavinp, 2014-02-26 02:05:44]

+darin as reviewer.

Darin,

Ricardo and I can't see eye to eye on this change, being done in preparation for
removing the redundant HTTP header parsing code for freshness in blink (see the
referenced bug).

Darin, can you please go through this review, and help us make the call on how
to proceed? Thanks!

The discussion is long, and the comments many; but the disagreement seems to
boil down to a trade off. I'll try to summarize it succinctly and accurately
below, I hope Ricardo can help clear this up for me if I'm missing something.

I think that Ricardo would prefer a much more minimal change, introducing the
FreshnessExpiry concept in terms of the existing functions. He'll certainly be a
better advocate for his view, but I think he likes this approach because it is
consistent with current practice and keeps a method "GetCurrentAge" which has a
name identical to a concept defined in RFC 2616.

I see this, but on balance would like to just get the current_time argument out
of this code. Of the patch sets I've uploaded, I like patch set 15 (not the
final one!) best of all, although I uploaded patch set 16 to see if that
addresses a concern of Ricardo's (a function that is not named after a named RFC
2616 concept).

By following the definitions from the RFC closely, we end up adding and
subtracting current_time from the same value when calculating the
FreshnessExpiry, which makes these header parsing functions look non-idempotent
(since it takes a current_time argument!), and even worse, confusingly
introduces a function that does not ever depend on one of its arguments (except
in case of overflow).

[gavinp, 2014-02-26 02:06:37]

Nota Bene: this CL is downstream of https://codereview.chromium.org/163413004/ ,
which is as of this writing in the CQ.

[darin (slow to review), 2014-02-26 05:51:06]

When I wrote this code, I intentionally tried to model some of the methods after
the wording of RFC 2616. I thought that would make the code easier to
understand. I wasn't concerned about a little extra math.

Stepping back, I'm thrilled that you are working on eliminating redundant header
parsing code in Blink. That has always bothered me. WebKit didn't have that back
in 2006!

I'd be curious to understand the big picture of how you plan to fully eliminate
redundant header parsing code in Blink. Will WebURLResponse get more methods
that map to methods on net::HttpResponseHeaders?

https://codereview.chromium.org/154243006/diff/2280001/net/http/http_response...
File net/http/http_response_headers.cc (right):

https://codereview.chromium.org/154243006/diff/2280001/net/http/http_response...
net/http/http_response_headers.cc:927: // From RFC 2616 section 13.2.3:
hmm, I personally like exposing the current age calculation as a primitive
method on this class, but I suppose if we don't need it elsewhere, then it
doesn't strictly speaking need to be a separate method.

https://codereview.chromium.org/154243006/diff/2280001/net/http/http_response...
File net/http/http_response_headers.h (right):

https://codereview.chromium.org/154243006/diff/2280001/net/http/http_response...
net/http/http_response_headers.h:197: base::Time GetFreshnessExpiry(base::Time
request_time,
nit: Wouldn't this more canonically be called GetExpirationTime? Why invent the
"freshness expiry" term?

[gavinp, 2014-02-26 17:12:04]

On 2014/02/26 05:51:06, darin wrote:
> When I wrote this code, I intentionally tried to model some of the methods
after
> the wording of RFC 2616. I thought that would make the code easier to
> understand. I wasn't concerned about a little extra math.

I think everyone here agrees that was a good decision, and thinks we should
maintain this as much as possible.

> Stepping back, I'm thrilled that you are working on eliminating redundant
header
> parsing code in Blink. That has always bothered me. WebKit didn't have that
back
> in 2006!

There's at least one unfortunate bug from this, see crbug.com/103458 .

> I'd be curious to understand the big picture of how you plan to fully
eliminate
> redundant header parsing code in Blink. Will WebURLResponse get more methods
> that map to methods on net::HttpResponseHeaders?

For step one, removing the expiration calculation code, I was thinking of just
adding GetExpirationTime to WebURLResponse, so we can just use that in the
memory cache directly to determine expiry, comparing against current time.
There's some other potential uses of expiration time as well, for instance if we
saved it in the disk cache index at low latency, a resource being expired is a
really solid preconnect signal, and a resource being in freshness is a really
good don't preconnect or preresolve signal.

In the followup bug, crbug.com/340088 , there's some more subtle issues about
keeping the disk cache and the memory cache in sync when we do a revalidation. I
haven't fully thought that far ahead yet. We'll have to decide how sad it makes
us to copy payload again each time we revalidate (I suspect this makes us sad,
what do you think?). If that makes us sad, then we can add a field
cache_response_id, which is different for every unique cache response. The
memory cache when revalidating would then just send an unconditional request
down to the browser process, and check if the response_id changes. If it hasn't,
it can squash the request before any payload has crossed the IPC.

> 
>
https://codereview.chromium.org/154243006/diff/2280001/net/http/http_response...
> File net/http/http_response_headers.cc (right):
> 
>
https://codereview.chromium.org/154243006/diff/2280001/net/http/http_response...
> net/http/http_response_headers.cc:927: // From RFC 2616 section 13.2.3:
> hmm, I personally like exposing the current age calculation as a primitive
> method on this class, but I suppose if we don't need it elsewhere, then it
> doesn't strictly speaking need to be a separate method.
> 
>
https://codereview.chromium.org/154243006/diff/2280001/net/http/http_response...
> File net/http/http_response_headers.h (right):
> 
>
https://codereview.chromium.org/154243006/diff/2280001/net/http/http_response...
> net/http/http_response_headers.h:197: base::Time GetFreshnessExpiry(base::Time
> request_time,
> nit: Wouldn't this more canonically be called GetExpirationTime? Why invent
the
> "freshness expiry" term?

[gavinp, 2014-02-26 17:12:27]

https://codereview.chromium.org/154243006/diff/2280001/net/http/http_response...
File net/http/http_response_headers.cc (right):

https://codereview.chromium.org/154243006/diff/2280001/net/http/http_response...
net/http/http_response_headers.cc:927: // From RFC 2616 section 13.2.3:
On 2014/02/26 05:51:07, darin wrote:
> hmm, I personally like exposing the current age calculation as a primitive
> method on this class, but I suppose if we don't need it elsewhere, then it
> doesn't strictly speaking need to be a separate method.

I like exposing it too. But we can't calculate current_age because we don't have
a current_time. The spec is pretty silent on what to call this thing I've been
calling freshness_origin. In the definition of "freshness lifetime" in RFC 2616
1.3, it's called the time of "generation of a response" which doesn't suggest a
good name. In the discussion in 13.2.3, it's not named either.

How about "corrected response time" as the name? That's never used in the RFC,
but it's in keeping with the other nomenclature?

https://codereview.chromium.org/154243006/diff/2280001/net/http/http_response...
File net/http/http_response_headers.h (right):

https://codereview.chromium.org/154243006/diff/2280001/net/http/http_response...
net/http/http_response_headers.h:197: base::Time GetFreshnessExpiry(base::Time
request_time,
On 2014/02/26 05:51:07, darin wrote:
> nit: Wouldn't this more canonically be called GetExpirationTime? Why invent
the
> "freshness expiry" term?

Yes, that's a more canonical name. There's no reason to invent this new term.
Fixed.

[gavinp, 2014-02-26 21:03:13]

The upstream CL has landed, and this is now based on trunk.

[rvargas (doing something else), 2014-02-26 21:03:22]

On 2014/02/26 01:54:11, gavinp wrote:
> On 2014/02/13 22:16:25, rvargas wrote:
> > On 2014/02/13 17:46:22, gavinp wrote:
> > > On 2014/02/12 20:00:16, rvargas wrote:
> > > > I'm sorry, but as I mentioned before I don't find some of the functions
> that
> > > you
> > > > are adding or modifying clear enough from the point of view of the API,
> and
> > I
> > > > must insist on just adding the method that you want based on the
existing
> > > code.
> > > 
> > > In https://codereview.chromium.org/154243006/#msg4 , you explicitly
proposed
> > > defining RequiresValidation in terms of GetFreshnessExpiry, but I guess
you
> > > changed your mind.
> > 
> > No, I definitely have not changed my mind. As I said on that comment: "If
you
> > don't like the small code duplication...". I just don't see the need to
repeat
> > every detail with every message, but maybe I should.
> 
> Before I uploaded patch set 11
> https://codereview.chromium.org/154243006/#ps980004 , I read
> https://codereview.chromium.org/154243006/#msg4 . You now tell me that comment
> #4 was intended to say that the second option you presented is totally
> unacceptable. I read, and reread that comment, and I cannot find that reading
in
> it. In fact, my reading is basically that it is permissive, saying that the
> second proposed approach can be used. It is very unfortunate that I read
message
> #4 as having the opposite meaning of what you intended. Even with your
> clarifications and restatement in message #12 and message #8, I can't find the
> reading you intended in message #4.
> 
> That's really unfortunate. I think you're being sarcastic when you suggest
that
> you repeat every detail with every message, however, more verbosity and more
> clarity would definitely help.

I'm not trying to be sarcastic, and I don't understand why you think that I'm
saying that the second option of comment 4 is not acceptable. I just didn't feel
the need to reiterate that it _is_ a valid option, and I have not changed my
mind about it. If it was not a valid option I would have never written that in
the first place.


> 
> > > 
> > > The latest upload fixes your earlier concerns about
GetFreshnessLifetime(),
> by
> > > making upstream changes that permit a concept of a maximum TimeDelta.
> > > 
> > > Your suggestion that this method be defined in terms of the existing
methods
> > is
> > > perhaps simplest now, but it conceptually is more complex and makes our
code
> > > more fragile. If we define GetFreshnessExpiry in terms of GetCurrentAge as
> > > proposed in https://codereview.chromium.org/154243006/#msg4 , we will be
> > adding
> > > and subtracting the current_time to calculate the FreshnessExpiry, which
> need
> > > not depend on the time. Unnecessary additions and subtractions of unneeded
> > > concepts passed in with unnecessary parameters does not simplify things.
> > 
> > Looking at the code that I posted on comment 4, it seems that your concern
> about
> > "unnecessary additions and subtractions" can be directed to:
> > 
> > return current_time + lifetime - age;
> > 
> > where you object having to add |current_time|. That looks to me like an
> > extremely minor thing to be arguing about. Are you really worried about one
> > extra CPU cycle doing that addition?
> 
> No, that is not my objection.
> 
> The concern is about conceptual complexity. The class is simpler when the
> methods take fewer arguments (i.e. not current_time). The comment on
> RequiresValidation indicates the current_time is passed only for debugging,
> even! Further, current_time makes these functions look strangely
non-idempotent.
> Reformulating them in a way that doesn't vary with current_time makes the
> concepts simpler.
> 
> There's a big bonus coming down stream of course, in that this freshness
expiry
> concept is really useful for cutting out bug-causing duplicate header parses
in
> blink. Of course we can achieve that with the implementation based on the
> current methods, but while we're here I think let's simplify and clean up this
> code.
> 
> Consider, for instance, this implementation of GetFreshnessExpiry(), based on
> the one in patch set #4 that you proposed:
> 
> Time HttpResponseHeaders::GetFreshnessExpiry(Time request_time, Time
> response_time) const {
>   TimeDelta lifetime = GetFreshnessLifetime(response_time);
>   if (lifetime == TimeDelta())
>     return Time();
> 
>   // Compute resource lifetime and age relative to the attack on Pearl Harbor.
>   const Time::Exploded exploded_time_of_infamy = { 1942, 12, 0, 7, 17, 48, 0,
0
> };
>   const Time time_of_infamy = Time::FromUTCExploded(exploded_time_of_infamy);
>   const TimeDelta age = GetCurrentAge(request_time, response_time,
> time_of_infamy);
> 
>   return time_of_infamy + lifetime - age;
> }
> 
> This function is perfectly correct; since |GetCurrentAge(a, b, c) = c -
> GetFreshnessOrigin(a, b)|; so this reduces to the same thing, modulo a few
> overflow issues happening as the current_time approaches base::Time::Max().
Yet,
> when I read it, it's pretty strange that we chose this arbitrary time. I
submit
> that current_time is equally arbitrary. Is there some reason that it's better
to
> add and subtract current_time over the time_of_infamy?
> 
> > 
> > If your objection is that GetCurrentAge is not clear or can be optimized, I
> > would agree that it certainly can be optimized and made it less verbose;
> 
> I would exclamation-mark ell gee tee em any such change that "optimized" that
> code. The math should follow the RFC as closely as reasonable.
> 
> > however, I think it is valuable to have that function be an explicit and
> direct
> > transcription of RFC 2616 section 13.2.3 (as stated in the comment), leaving
> the
> > optimization to the compiler. I see no references to "freshness origin" on
> that
> > RFC.
> 
> As an alternative, I'm preparing an upload that gets rid of the (effectively
in
> use private) GetFreshnessOrigin/GetCurrentAge function, eliding it into the
> GetFreshnessExpiry() function, which was the only call site anyway.
> 
> While it's true that freshness lifetime and freshness origin are not defined
in
> the RFC, their derivation is clearly described, and the concepts are useful.
> 
> Further, if not being defined in the RFC means the code is bad, then why is
this
> legacy name RequiresValidation OK? There's no such concept defined in the RFC,
> except in a reading that combines section 13.2.4's definition of "stale"
> together with the first paragraph of 13.3! That's pretty synthetic, and really
> about as tricky as the definition of FreshnessExpiry. I don't think the spec
is
> an implementation; having our code obviously follow it is good when we can,
but
> diverging because the spec had a bit of not-needed conceptual complexity is
> probably a good idea. I guess I'm proposing that this upload is such a
> divergence.

[gavinp, 2014-03-26 12:59:43]

+rdsmith, PTAL.

darin: I've remediated to your requests; are you going to take a second look?

I wish to advance this. We continue to have problems because blink has its own
parsing logic, and any future work to remove this is blocked on this CL, which
is now over a month old.

[darin (slow to review), 2014-03-26 16:16:27]

Sorry for the delayed response. I didn't realize you were waiting on me :-(

https://codereview.chromium.org/154243006/diff/2320001/net/http/http_response...
File net/http/http_response_headers.cc (right):

https://codereview.chromium.org/154243006/diff/2320001/net/http/http_response...
net/http/http_response_headers.cc:908: // Into this equation, we substitute
|current_age = now - freshness_origin|,
what is "freshness_origin"?

https://codereview.chromium.org/154243006/diff/2320001/net/http/http_response...
net/http/http_response_headers.cc:916: // freshness_expiry.
nit: this is no longer called "freshness_expiry". it is just called expiration
time, right?

https://codereview.chromium.org/154243006/diff/2320001/net/http/http_response...
net/http/http_response_headers.cc:951: //   current_age   =
corrected_initial_age + resident_time;
it would help to add a comment below here (a "NOTE:") explaining that the code
below computes |now - current_age|. that sounds a bit like a birthday :-)

[gavinp, 2016-04-29 15:01:47]

Description was changed from

==========
Add GetExpirationTime() to HttpResponseHeader.

This is the first step in propagating the explicit freshness lifetime
to blink. The eventual goal is to remove much of the code parsing http
caching headers in blink.

R=rvargas@chromium.org,japhet@chromium.org,rdsmith@chromium.org,darin@chromiu...
BUG=340087
==========

to

==========
Add GetExpirationTimes() to HttpResponseHeaders.

This is the first step in propagating the explicit freshness lifetime
to blink. The eventual goal is to remove much of the code parsing http
caching headers in blink.

R=japhet@chromium.org,rdsmith@chromium.org,darin@chromium.org,kinuko@chromium...
BUG=340087
==========

[gavinp, 2016-04-29 15:01:48]

gavinp@chromium.org changed reviewers:
- rvargas@chromium.org

[gavinp, 2016-04-29 15:02:54]

Description was changed from

==========
Add GetExpirationTimes() to HttpResponseHeaders.

This is the first step in propagating the explicit freshness lifetime
to blink. The eventual goal is to remove much of the code parsing http
caching headers in blink.

R=japhet@chromium.org,rdsmith@chromium.org,darin@chromium.org,kinuko@chromium...
BUG=340087
==========

to

==========
Add GetExpirationTimes() to HttpResponseHeaders.

This is the first step in propagating the explicit freshness lifetime
to blink. The eventual goal is to remove much of the code parsing http
caching headers in blink.

R=japhet@chromium.org,rdsmith@chromium.org,darin@chromium.org,kinuko@chromium...
BUG=340087
==========

[gavinp, 2016-04-29 15:02:55]

gavinp@chromium.org changed reviewers:
+ kinuko@chromium.org, mmenke@chromium.org

[gavinp, 2016-04-29 15:04:51]

Here's another long in the tooth CL that is coming back to life.

I believe that the expiration times idea is conceptually significantly simpler
than having HttpResponseHeaders calculate ages; as well, it has the highly
desirable virtue that the result can be saved by a client and used later; i.e.
the blink memory cache can store this information.

mmenke: wdyt?
kinuko,darin,japhat: fyi

https://codereview.chromium.org/154243006/diff/2320001/net/http/http_response...
File net/http/http_response_headers.cc (right):

https://codereview.chromium.org/154243006/diff/2320001/net/http/http_response...
net/http/http_response_headers.cc:908: // Into this equation, we substitute
|current_age = now - freshness_origin|,
On 2014/03/26 16:16:27, darin (slow to review) wrote:
> what is "freshness_origin"?

Removed this.

https://codereview.chromium.org/154243006/diff/2320001/net/http/http_response...
net/http/http_response_headers.cc:916: // freshness_expiry.
On 2014/03/26 16:16:27, darin wrote:
> nit: this is no longer called "freshness_expiry". it is just called expiration
> time, right?

That's right. Thanks for catching this.

https://codereview.chromium.org/154243006/diff/2320001/net/http/http_response...
net/http/http_response_headers.cc:951: //   current_age   =
corrected_initial_age + resident_time;
On 2014/03/26 16:16:27, darin (slow to review) wrote:
> it would help to add a comment below here (a "NOTE:") explaining that the code
> below computes |now - current_age|. that sounds a bit like a birthday :-)

Hmmm, except it doesn't, because this function lacks a concept of "Now". I've
cleaned up the comment above this function to describe what it's calculating
better.

https://codereview.chromium.org/154243006/diff/2430001/content/browser/appcac...
File content/browser/appcache/appcache_update_job.cc (left):

https://codereview.chromium.org/154243006/diff/2430001/content/browser/appcac...
content/browser/appcache/appcache_update_job.cc:1476:
http_info->headers->RequiresValidation(http_info->request_time,
Scary that this relied on the implicit conversion to bool from the enum.

[mmenke, 2016-04-29 16:15:54]

Seems like a significant API improvement to me.  Quick comments.

https://codereview.chromium.org/154243006/diff/2530001/net/http/http_response...
File net/http/http_response_headers.cc (right):

https://codereview.chromium.org/154243006/diff/2530001/net/http/http_response...
net/http/http_response_headers.cc:803: void
HttpResponseHeaders::CalculateLifetimes(
Definition order should match delcaration order.  Or could just moving this up
to an anonymous namespace, since it doesn't depend on private variables or
methods.

https://codereview.chromium.org/154243006/diff/2530001/net/http/http_response...
net/http/http_response_headers.cc:804: const Time& response_time,
nit:  Believe the preferred style is not to pass times (And TimeDeltas) by
reference.  Passing 64 bits is probably more efficient than a 32-bit pointer, on
32-bit systems.

https://codereview.chromium.org/154243006/diff/2530001/net/http/http_response...
net/http/http_response_headers.cc:821: // Cache-Control directive
must_revalidate overrides stale-while-revalidate.
nit:  must-revalidate

https://codereview.chromium.org/154243006/diff/2530001/net/http/http_response...
net/http/http_response_headers.cc:826: DCHECK_EQ(TimeDelta(),
*out_staleness_lifetime);
This DCHECK seems out of place - we aren't DCHECKing out_freshness_lifetime is 0
when GetMaxAgeValue returns false, for instance.

https://codereview.chromium.org/154243006/diff/2530001/net/http/http_response...
net/http/http_response_headers.cc:850: DCHECK_EQ(TimeDelta(),
*out_freshness_lifetime);
Should this be under GetDateValue?  How many DCHECKs do we need that methods
don't set their arguments when they return false?  Seems like we should DCHECK
all or none.

https://codereview.chromium.org/154243006/diff/2530001/net/http/http_response...
net/http/http_response_headers.cc:885: *out_freshness_lifetime = (date_value -
last_modified_value) / 10;
This "/ 10" is just a magic heuristic?  Maybe comment on that?

https://codereview.chromium.org/154243006/diff/2530001/net/http/http_response...
net/http/http_response_headers.cc:891: // These responses are implicitly fresh
(unless otherwise overruled):
"Otherwise overruled" does not include having the "must_revalidate" header?

https://codereview.chromium.org/154243006/diff/2530001/net/http/http_response...
net/http/http_response_headers.cc:895: *out_staleness_lifetime = TimeDelta(); 
// It should never be stale.
In the calling function, should we just always set out_staleness_lifetime to 0,
whenever it's less than freshness lifetime?

https://codereview.chromium.org/154243006/diff/2530001/net/http/http_response...
File net/http/http_response_headers.h (right):

https://codereview.chromium.org/154243006/diff/2530001/net/http/http_response...
net/http/http_response_headers.h:50: base::Time GetExpirationTime() const {
expiration_time()?

https://codereview.chromium.org/154243006/diff/2530001/net/http/http_response...
net/http/http_response_headers.h:55: base::Time GetAsyncExpirationTime() const {
async_expiration_time()?

https://codereview.chromium.org/154243006/diff/2530001/net/http/http_response...
net/http/http_response_headers.h:55: base::Time GetAsyncExpirationTime() const {
I think this comment needs to be clearer.  What does it mean for this to be less
than expiration time, for instance?  Can it even be less than expiration time?

https://codereview.chromium.org/154243006/diff/2530001/net/http/http_response...
net/http/http_response_headers.h:65: base::TimeDelta freshness_lifetime;
I think the names of related functions / values should match.  ExpirationTime /
freshness_lifetime should be more clearly related.  Same for staleness /
async_expiration_time.

Or maybe we just make all these values private?  Think the expiration times make
for clearer code.  Looks like only two classes care about any of these.  One
just cares if a resource is cacheable at all, so doesn't really care about the
actual lifetime.  The other wants to histogram non-cached responses based on
lifetime, so can just use GetExpirationTime() - base::Time::Now().

https://codereview.chromium.org/154243006/diff/2530001/net/http/http_response...
net/http/http_response_headers.h:68: // usable (if async revalidation is
enabled).
This comment is wrong.  This implies that GetAsyncExpirationTime should be
corrected_response_time + staleness_lifetime + freshness_lifetime.

https://codereview.chromium.org/154243006/diff/2530001/net/http/http_response...
net/http/http_response_headers.h:243: const base::Time& response_time) const;
const base::Time& -> base::Time

https://codereview.chromium.org/154243006/diff/2530001/net/http/http_response...
net/http/http_response_headers.h:243: const base::Time& response_time) const;
A bit unrelated to this CL, but I've always wondered about putting all this
logic in HttpResponseHeaders - sure, they information is derived from the
headers, but "HttpResponseHeaders" implies to me "POD class that contains
response headers", rather than "class with all sorts of funky logic to derive
weird data from HTTP headers".  This method, in particular, is weird, because it
needs data from HttpResponseInfo (Which also owns the header...So should this be
a method on that?)

Thoughts?

[mmenke, 2016-04-29 17:30:05]

And, just for the record (And after a quick conversation with Gavin), I'm aware
most of these comments are about old moved code, just feel that while we're
moving it, it's a good chance to do some minor cleanups to old code -
particularly cleanups not really worth a separate CL.

[Randy Smith (Not in Mondays), 2016-05-04 19:22:16]

This all looks pretty good to me--I don't expect any major comments after this
round.  I agree with Matt about cleaning up the comments as part of the code
movement (and feel pretty strongly about not referring to functions and
structures that no longer exist in those comments).

https://codereview.chromium.org/154243006/diff/2530001/net/http/http_response...
File net/http/http_response_headers.cc (right):

https://codereview.chromium.org/154243006/diff/2530001/net/http/http_response...
net/http/http_response_headers.cc:830: // Expires header after checking for
max-age in GetFreshnessLifetimes.  This
This comment seems out of date, since I don't think there's a function of this
name in this file anymore.

https://codereview.chromium.org/154243006/diff/2530001/net/http/http_response...
net/http/http_response_headers.cc:844: // The expires value can be a date in the
past!
nit, suggestion: I'd value a comment here indicating what the spec behavior is
when the expires value is a date in the past.  It looks like it's ignored in
that case, but that's surprising to me, so I'd ideally have it confirmed by
comment.

https://codereview.chromium.org/154243006/diff/2530001/net/http/http_response...
net/http/http_response_headers.cc:885: *out_freshness_lifetime = (date_value -
last_modified_value) / 10;
On 2016/04/29 16:15:53, mmenke wrote:
> This "/ 10" is just a magic heuristic?  Maybe comment on that?

There's a comment about it at the top of the function; maybe a reference to that
comment?

https://codereview.chromium.org/154243006/diff/2530001/net/http/http_response...
File net/http/http_response_headers.h (right):

https://codereview.chromium.org/154243006/diff/2530001/net/http/http_response...
net/http/http_response_headers.h:50: base::Time GetExpirationTime() const {
I find it confusing (found it confusing in one of the call sites) to have a
GetExpirationTime() function on an ExpirationTime class; the function is a
specialization of some sort, but the name doesn't indicate what sort.  How would
you feel about GetSyncExpirationTime() instead?

https://codereview.chromium.org/154243006/diff/2530001/net/http/http_response...
net/http/http_response_headers.h:58: 
Why can't the rest of this structure be private?  It seems to me as if the
appropriate information to export to consumers it above this point in the
structure; under what circumstances do the consumers need to know what the
lifetime TimeDelta values are?

https://codereview.chromium.org/154243006/diff/2530001/net/http/http_response...
net/http/http_response_headers.h:64: // How long the resource will be fresh for.
nit: Given that this is a time delta, wouldn't it make sense to have the anchor
(e.g. "... will be fresh for after the response is received (??)")

https://codereview.chromium.org/154243006/diff/2530001/net/http/http_response...
net/http/http_response_headers.h:68: // usable (if async revalidation is
enabled).
On 2016/04/29 16:15:54, mmenke wrote:
> This comment is wrong.  This implies that GetAsyncExpirationTime should be
> corrected_response_time + staleness_lifetime + freshness_lifetime.

+1 (mostly as a reminder to myself to come back and understand the resolution of
this comment, since I think it affects my private/public/etc. musings above)

https://codereview.chromium.org/154243006/diff/2530001/net/http/http_response...
net/http/http_response_headers.h:360: // the definition of FreshnessLifetimes
above for the meaning of the return
I don't believe that RequiresValidation() or FreshnessLifetimes exist anymore,
at least under those names.  This comment needs to be rewritten?