Add GetExpirationTimes() to HttpResponseHeader.

net/http/http_response_headers.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef NET_HTTP_HTTP_RESPONSE_HEADERS_H_
 #define NET_HTTP_HTTP_RESPONSE_HEADERS_H_
 
 #include <stddef.h>
 #include <stdint.h>
 
@@ skipping 12 matching lines @@
 class Pickle;
 class PickleIterator;
 class Time;
 class TimeDelta;
 }
 
 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> {
  public:
   // Persist options.
   typedef int PersistOptions;
   static const PersistOptions PERSIST_RAW = -1;  // Raw, unparsed headers.
   static const PersistOptions PERSIST_ALL = 0;  // Parsed headers.
   static const PersistOptions PERSIST_SANS_COOKIES = 1 << 0;
   static const PersistOptions PERSIST_SANS_CHALLENGES = 1 << 1;
   static const PersistOptions PERSIST_SANS_HOP_BY_HOP = 1 << 2;
   static const PersistOptions PERSIST_SANS_NON_CACHEABLE = 1 << 3;
   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[];
 
   // Parses the given raw_headers.  raw_headers should be formatted thus:
   // includes the http status response line, each line is \0-terminated, and
   // it's terminated by an empty line (ie, 2 \0s in a row).
   // (Note that line continuations should have already been joined;
   // see HttpUtil::AssembleRawHeaders)
   //
@@ skipping 148 matching lines @@
   bool GetCharset(std::string* charset) const;
 
   // Returns true if this response corresponds to a redirect.  The target
   // location of the redirect is optionally returned if location is non-null.
   bool IsRedirect(std::string* location) const;
 
   // Returns true if the HTTP response code passed in corresponds to a
   // 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,
   // true is returned and the out param is assigned to the corresponding value.
   bool GetMaxAgeValue(base::TimeDelta* value) const;
   bool GetAgeValue(base::TimeDelta* value) const;
   bool GetDateValue(base::Time* value) const;
   bool GetLastModifiedValue(base::Time* value) const;
   bool GetExpiresValue(base::Time* value) const;
   bool GetStaleWhileRevalidateValue(base::TimeDelta* value) const;
@@ skipping 93 matching lines @@
   // Find the header in our list (case-insensitive) starting with parsed_ at
   // index |from|.  Returns string::npos if not found.
   size_t FindHeader(size_t from, const base::StringPiece& name) const;
 
   // Search the Cache-Control header for a directive matching |directive|. If
   // present, treat its value as a time offset in seconds, write it to |result|,
   // and return true.
   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,
                  std::string::const_iterator name_end,
                  std::string::const_iterator value_begin,
                  std::string::const_iterator value_end);
 
   // Add to parsed_ given the fields of a ParsedHeader object.
   void AddToParsed(std::string::const_iterator name_begin,
                    std::string::const_iterator name_end,
@@ skipping 45 matching lines @@
 
   // The normalized http version (consistent with what GetStatusLine() returns).
   HttpVersion http_version_;
 
   DISALLOW_COPY_AND_ASSIGN(HttpResponseHeaders);
 };
 
 }  // namespace net
 
 #endif  // NET_HTTP_HTTP_RESPONSE_HEADERS_H_