Suppress pause-and-buffer behavior when the HTTP response won't satisfy future requests via cache.

webkit/media/buffered_resource_loader.cc

Index: webkit/media/buffered_resource_loader.cc
diff --git a/webkit/media/buffered_resource_loader.cc b/webkit/media/buffered_resource_loader.cc
index 14ca0a12625058e61d70919b3de945f23cb05cbc..109a34fe2e0f174c36ebceaa72887db7152cec3f 100644
--- a/webkit/media/buffered_resource_loader.cc
+++ b/webkit/media/buffered_resource_loader.cc
@@ -4,8 +4,11 @@
 
 #include "webkit/media/buffered_resource_loader.h"
 
+#include <math.h>
+
 #include "base/callback_helpers.h"
 #include "base/format_macros.h"
+#include "base/metrics/histogram.h"
 #include "base/string_number_conversions.h"
 #include "base/string_util.h"
 #include "base/stringprintf.h"
@@ -111,6 +114,7 @@ BufferedResourceLoader::BufferedResourceLoader(
     : buffer_(kMinBufferCapacity, kMinBufferCapacity),
       loader_failed_(false),
       defer_strategy_(strategy),
+      might_be_reused_from_cache_in_future_(true),
       range_supported_(false),
       saved_forward_capacity_(0),
       url_(url),
@@ -352,6 +356,65 @@ void BufferedResourceLoader::didSendData(
   NOTIMPLEMENTED();
 }
 
+// Reasons that a cached WebURLResponse will *not* prevent a future request to
+// the server.
+enum OneOffReason {
+  kUseful = 0,  // Yay!  This response might satisfy a future request.
+  kNoData = 1,  // Not 200 or 206.
+  kPre11PartialResponse = 2,  // 206 but HTTP version < 1.1.

[scherkus (not reviewing), 2012/05/19 02:26:24]

nit: I read this as "Prell" (pee-are-eee-ell-ell)

I don't know if "PreOneOne" is any better though!

[Ami GONE FROM CHROMIUM, 2012/05/19 03:03:02]

...yeah.  Leaving as is.

+  kNoStrongValidatorOnPartialResponse = 3,  // 206, no strong validator.
+  kNoValidatorOnFullResponse = 4,  // 200, no strong or weak validator.
+  kShortMaxAge = 5,  // Max age less than 1h (arbitrary value).
+  kExpiresTooSoon = 6,  // Expires in less than 1h (arbitrary value).
+  kHasMustRevalidate = 7,  // Response asks for revalidation.
+  kNoCache = 8,  // Response included a no-cache header.
+  kMaxReason = kNoCache + 1  // Always keep equal to the greatest value plus 1.

[rvargas (doing something else), 2012/05/19 01:19:41]

nit: don't set a value?

[Ami GONE FROM CHROMIUM, 2012/05/19 03:03:02]

Hah!  Done.

+};
+
+// Return true if "response" might be used for a future request (using the disk
+// cache).  This is basically a laundry list of reasons we *know* the response
+// won't be useful in the future.
+static bool MightBeReusedFromCacheInFuture(const WebURLResponse& response) {
+  std::vector<OneOffReason> reasons;
+  const int code = response.httpStatusCode();
+  const int version = response.httpVersion();
+  if (code != kHttpOK && code != kHttpPartialContent)
+    reasons.push_back(kNoData);
+  if (version < WebURLResponse::HTTP_1_1 && code != kHttpOK)

[scherkus (not reviewing), 2012/05/19 02:26:24]

since this isn't an else branch wouldn't this statement be true if version was
<1.1 and code was, say, 404?

speaking of which, do we want to include non-200/206 responses in our counts?
won't other random responses trickle into our stats and increment counts?

[Ami GONE FROM CHROMIUM, 2012/05/19 03:03:02]

Oops; leftover from previous code.
That's the point of kNoData (so we can account for them).

+    reasons.push_back(kPre11PartialResponse);
+  if (!response.hasCacheValidatorFields()) {

[scherkus (not reviewing), 2012/05/19 02:26:24]

OOC is this is ETag && Last-Modified or ETag || Last-Modified?

[Ami GONE FROM CHROMIUM, 2012/05/19 03:03:02]

||
http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/Source/...

+    if (code == kHttpPartialContent)
+      reasons.push_back(kNoStrongValidatorOnPartialResponse);
+    else if (code == kHttpOK)
+      reasons.push_back(kNoValidatorOnFullResponse);
+  }
+  // TODO(fischman): if 206 && hasCacheValidatorFields() still need to verify
+  // one of the validators is "strong" or else still record
+  // kNoStrongValidatorOnPartialResponse.
+
+  enum { kMinimumAgeForUsefulnessInSeconds = 3600 };  // Arbitrary value.
+  if (!isnan(response.cacheControlMaxAge()) &&
+      response.cacheControlMaxAge() < kMinimumAgeForUsefulnessInSeconds) {
+    reasons.push_back(kShortMaxAge);
+  }
+  if (!isnan(response.expires()) && !isnan(response.date()) &&
+      ((response.expires() - response.date()) <
+       kMinimumAgeForUsefulnessInSeconds)) {
+    reasons.push_back(kExpiresTooSoon);
+  }
+  if (response.cacheControlContainsMustRevalidate())
+    reasons.push_back(kHasMustRevalidate);
+  if (response.cacheControlContainsNoCache())

[rvargas (doing something else), 2012/05/19 01:19:41]

does this check for no-store also?

[Ami GONE FROM CHROMIUM, 2012/05/19 03:03:02]

Nope.  Thought about it and thought I didn't need it, but in retrospect, I do.

+    reasons.push_back(kNoCache);
+
+  if (reasons.empty())
+    UMA_HISTOGRAM_ENUMERATION("Media.OneOffReason", kUseful, kMaxReason);
+  for (size_t i = 0; i < reasons.size(); ++i)
+    UMA_HISTOGRAM_ENUMERATION("Media.OneOffReason", reasons[i], kMaxReason);

[rvargas (doing something else), 2012/05/19 01:19:41]

note that there will be no way to tell what percentage of entries are "good".

[Ami GONE FROM CHROMIUM, 2012/05/19 03:03:02]

Good point.  Reworked slightly.

+
+  return reasons.empty();
+}
+
 void BufferedResourceLoader::didReceiveResponse(
     WebURLLoader* loader,
     const WebURLResponse& response) {
@@ -368,6 +431,9 @@ void BufferedResourceLoader::didReceiveResponse(
   if (start_cb_.is_null())
     return;
 
+  might_be_reused_from_cache_in_future_ =
+      MightBeReusedFromCacheInFuture(response);

[scherkus (not reviewing), 2012/05/19 02:26:24]

do we want to log these requests for all protocols or just http/https? I would
think the latter!

[Ami GONE FROM CHROMIUM, 2012/05/19 03:03:02]

Hmm.  On one hand HTTP will drown out all other stats.  On the other cache/defer
decisions aren't being made differently for http vs. non-http.  Do you have an
idea that that should change?

[scherkus (not reviewing), 2012/05/21 21:35:49]

Maybe. For example, we report 100% buffered when WMPI::is_local_source_ is true.
Pause-and-buffering (and needlessly transmitting bytes between
browser<->renderer) is tantamount to doing unnecessary work with zero benefit.

+
   // Expected content length can be |kPositionNotSpecified|, in that case
   // |content_length_| is not specified and this is a streaming response.
   content_length_ = response.expectedContentLength();
@@ -540,6 +606,8 @@ bool BufferedResourceLoader::HasSingleOrigin() const {
 }
 
 void BufferedResourceLoader::UpdateDeferStrategy(DeferStrategy strategy) {
+  if (!might_be_reused_from_cache_in_future_ && strategy == kNeverDefer)

[scherkus (not reviewing), 2012/05/19 02:26:24]

I'm this -->||<-- close to thinking we should just remove this

as slick as this change is... what if things change? what if we do decide to
cache more aggressively during playback?

The logging is informative but do wonder if we're better off removing
kNeverDefer

[Ami GONE FROM CHROMIUM, 2012/05/19 03:03:02]

I don't want to drop pause-and-buffer where it works well, since it's a
compelling user-visible feature.

+    strategy = kThresholdDefer;
   defer_strategy_ = strategy;
   UpdateDeferBehavior();
 }