Add GetExpirationTimes() to HttpResponseHeader.

net/http/http_response_headers.h

Index: net/http/http_response_headers.h
diff --git a/net/http/http_response_headers.h b/net/http/http_response_headers.h
index 653eacc4d44e30b86d15c41532ec2514ef8ba435..f8f1864dc0b3e2c38ef46faea4659c399e517fb1 100644
--- a/net/http/http_response_headers.h
+++ b/net/http/http_response_headers.h
@@ -30,12 +30,6 @@ namespace net {
 
 class HttpByteRange;
 
-enum ValidationType {
-  VALIDATION_NONE,          // The resource is fresh.
-  VALIDATION_ASYNCHRONOUS,  // The resource requires async revalidation.
-  VALIDATION_SYNCHRONOUS    // The resource requires sync revalidation.
-};
-
 // HttpResponseHeaders: parses and holds HTTP response headers.
 class NET_EXPORT HttpResponseHeaders
     : public base::RefCountedThreadSafe<HttpResponseHeaders> {
@@ -51,12 +45,28 @@ class NET_EXPORT HttpResponseHeaders
   static const PersistOptions PERSIST_SANS_RANGES = 1 << 4;
   static const PersistOptions PERSIST_SANS_SECURITY_STATE = 1 << 5;
 
-  struct FreshnessLifetimes {
+  struct ExpirationTimes {
+    // The time after which the resource must be validated.
+    base::Time GetExpirationTime() const {

[mmenke, 2016/04/29 16:15:53]

expiration_time()?

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

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?

+      return corrected_response_time + freshness_lifetime;
+    }
+
+    // The time after which the resource must be validated synchronously.
+    base::Time GetAsyncExpirationTime() const {

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

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?

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

async_expiration_time()?

+      return corrected_response_time + staleness_lifetime;
+    }
+

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

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?

+    // The |base::Time::Now()| value for which this request would have an age of
+    // exactly zero. You can subtract this from |base::Time::Now()| to get the
+    // current age.
+    base::Time corrected_response_time;
+
     // How long the resource will be fresh for.

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

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 (??)")

-    base::TimeDelta freshness;
+    base::TimeDelta freshness_lifetime;

[mmenke, 2016/04/29 16:15:53]

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().

+
     // How long after becoming not fresh that the resource will be stale but
     // usable (if async revalidation is enabled).

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

This comment is wrong.  This implies that GetAsyncExpirationTime should be
corrected_response_time + staleness_lifetime + freshness_lifetime.

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

+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)

-    base::TimeDelta staleness;
+    base::TimeDelta staleness_lifetime;
   };
 
   static const char kContentRange[];
@@ -225,32 +235,12 @@ class NET_EXPORT HttpResponseHeaders
   // redirect.
   static bool IsRedirectResponseCode(int response_code);
 
-  // Returns VALIDATION_NONE if the response can be reused without
-  // validation. VALIDATION_ASYNCHRONOUS means the response can be re-used, but
-  // asynchronous revalidation must be performed. VALIDATION_SYNCHRONOUS means
-  // that the result cannot be reused without revalidation.
-  // The result is relative to the current_time parameter, which is
-  // a parameter to support unit testing.  The request_time parameter indicates
-  // the time at which the request was made that resulted in this response,
-  // which was received at response_time.
-  ValidationType RequiresValidation(const base::Time& request_time,
-                                    const base::Time& response_time,
-                                    const base::Time& current_time) const;
-
-  // Calculates the amount of time the server claims the response is fresh from
-  // the time the response was generated.  See section 13.2.4 of RFC 2616.  See
-  // RequiresValidation for a description of the response_time parameter.  See
-  // the definition of FreshnessLifetimes above for the meaning of the return
-  // value.  See RFC 5861 section 3 for the definition of
-  // stale-while-revalidate.
-  FreshnessLifetimes GetFreshnessLifetimes(
-      const base::Time& response_time) const;
-
-  // Returns the age of the response.  See section 13.2.3 of RFC 2616.
-  // See RequiresValidation for a description of this method's parameters.
-  base::TimeDelta GetCurrentAge(const base::Time& request_time,
-                                const base::Time& response_time,
-                                const base::Time& current_time) const;
+  // Returns an ExpirationTimes struct which can be used to calculate the
+  // freshness and stale-white-revalidate status of this response. The values in
+  // the ExpirationTimes struct do not depend on the current time, so they can
+  // be stored and used later as well.
+  ExpirationTimes GetExpirationTimes(const base::Time& request_time,
+                                     const base::Time& response_time) const;

[mmenke, 2016/04/29 16:15:53]

const base::Time& -> base::Time

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

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?

 
   // The following methods extract values from the response headers.  If a
   // value is not present, or is invalid, then false is returned.  Otherwise,
@@ -364,6 +354,15 @@ class NET_EXPORT HttpResponseHeaders
   bool GetCacheControlDirective(const base::StringPiece& directive,
                                 base::TimeDelta* result) const;
 
+  // Calculates the amount of time the server claims the response is fresh from
+  // the time the response was generated. See section 13.2.4 of RFC 2616. See
+  // RequiresValidation for a description of the response_time parameter. See
+  // the definition of FreshnessLifetimes above for the meaning of the return

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

I don't believe that RequiresValidation() or FreshnessLifetimes exist anymore,
at least under those names.  This comment needs to be rewritten?

+  // value. See RFC 5861 section 3 for the definition of stale-while-revalidate.
+  void CalculateLifetimes(const base::Time& response_time,
+                          base::TimeDelta* out_freshness_lifetime,
+                          base::TimeDelta* out_staleness_lifetime) const;
+
   // Add a header->value pair to our list.  If we already have header in our
   // list, append the value to it.
   void AddHeader(std::string::const_iterator name_begin,