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

webkit/media/buffered_resource_loader.cc

 // 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.
 
 #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"
 #include "media/base/media_log.h"
 #include "net/http/http_request_headers.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebKit.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebURLLoaderOptions.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/platform/WebKitPlatformSupport.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/platform/WebString.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/platform/WebURLError.h"
@@ skipping 85 matching lines @@
     const GURL& url,
     int64 first_byte_position,
     int64 last_byte_position,
     DeferStrategy strategy,
     int bitrate,
     float playback_rate,
     media::MediaLog* media_log)
     : 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),
       first_byte_position_(first_byte_position),
       last_byte_position_(last_byte_position),
       single_origin_(true),
       offset_(0),
       content_length_(kPositionNotSpecified),
       instance_size_(kPositionNotSpecified),
       read_position_(0),
@@ skipping 221 matching lines @@
   url_ = newRequest.url();
 }
 
 void BufferedResourceLoader::didSendData(
     WebURLLoader* loader,
     unsigned long long bytes_sent,
     unsigned long long total_bytes_to_be_sent) {
   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) {
   DVLOG(1) << "didReceiveResponse: HTTP/"
            << (response.httpVersion() == WebURLResponse::HTTP_0_9 ? "0.9" :
                response.httpVersion() == WebURLResponse::HTTP_1_0 ? "1.0" :
                response.httpVersion() == WebURLResponse::HTTP_1_1 ? "1.1" :
                "Unknown")
            << " " << response.httpStatusCode();
   DCHECK(active_loader_.get());
 
   // The loader may have been stopped and |start_cb| is destroyed.
   // In this case we shouldn't do anything.
   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();
 
   // We make a strong assumption that when we reach here we have either
   // received a response from HTTP/HTTPS protocol or the request was
   // successful (in particular range request). So we only verify the partial
   // response for HTTP and HTTPS protocol.
   if (url_.SchemeIs(kHttpScheme) || url_.SchemeIs(kHttpsScheme)) {
     bool partial_response = (response.httpStatusCode() == kHttpPartialContent);
@@ skipping 152 matching lines @@
   }
 }
 
 bool BufferedResourceLoader::HasSingleOrigin() const {
   DCHECK(start_cb_.is_null())
       << "Start() must complete before calling HasSingleOrigin()";
   return single_origin_;
 }
 
 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();
 }
 
 void BufferedResourceLoader::SetPlaybackRate(float playback_rate) {
   playback_rate_ = playback_rate;
 
   // This is a pause so don't bother updating the buffer window as we'll likely
   // get unpaused in the future.
   if (playback_rate_ == 0.0)
@@ skipping 260 matching lines @@
 
 void BufferedResourceLoader::Log() {
   media_log_->AddEvent(
       media_log_->CreateBufferedExtentsChangedEvent(
           offset_ - buffer_.backward_bytes(),
           offset_,
           offset_ + buffer_.forward_bytes()));
 }
 
 }  // namespace webkit_media