Reduce unnecessary network connections.

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 "base/format_macros.h"
 #include "base/stringprintf.h"
 #include "base/string_util.h"
 #include "media/base/media_log.h"
@@ skipping 361 matching lines @@
   // 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)) {
     int error = net::OK;
 
     // Check to see whether the server supports byte ranges.
     std::string accept_ranges =
         response.httpHeaderField("Accept-Ranges").utf8();
     range_supported_ = (accept_ranges.find("bytes") != std::string::npos);

[acolwell GONE FROM CHROMIUM, 2012/03/14 18:24:49]

Should this and the line above be moved into the if(range_requested_) block
below? It seems like we only want to update this field if we actually requested
a range.

[Ami GONE FROM CHROMIUM, 2012/03/14 19:37:20]

Done.

 
     partial_response = (response.httpStatusCode() == kHttpPartialContent);
+    bool ok_response = (response.httpStatusCode() == kHttpOK);
 
     if (range_requested_) {
       // If we have verified the partial response and it is correct, we will
       // return net::OK. It's also possible for a server to support range
       // requests without advertising Accept-Ranges: bytes.
-      if (partial_response && VerifyPartialResponse(response))
+      if (partial_response && VerifyPartialResponse(response)) {
         range_supported_ = true;
-      else
+      } else if (ok_response && first_byte_position_ == 0 &&
+                 last_byte_position_ == kPositionNotSpecified) {
+        // We accept a 200 response for a Range:0- request and down-grade the
+        // data source to streaming.
+        range_supported_ = false;
+      } else {
         error = net::ERR_INVALID_RESPONSE;
+      }
     } else if (response.httpStatusCode() != kHttpOK) {
       // We didn't request a range but server didn't reply with "200 OK".
       error = net::ERR_FAILED;
     }
 
     if (error != net::OK) {
       DoneStart(error);
       return;
     }
   } else {
     // For any protocol other than HTTP and HTTPS, assume range request is
     // always fulfilled.
     partial_response = range_requested_;

[acolwell GONE FROM CHROMIUM, 2012/03/14 18:24:49]

Is this a valid assumption? This code smells a little since range_supported_
doesn't get updated yet we are assuming the range honored. Perhaps a TODO & bug
to investigate further?

[Ami GONE FROM CHROMIUM, 2012/03/14 19:37:20]

partial_response starts out false and is only used at l.422 below to
conditionally set instance_size_.
I agree the code as written is smelly.  I modified it to test for the actual
condition we care about, which is: 
set instance_size_ unless we both asked for a range and got a range.

   }
 
   // Expected content length can be |kPositionNotSpecified|, in that case
   // |content_length_| is not specified and this is a streaming response.
   content_length_ = response.expectedContentLength();
 
   // If we have not requested a range, then the size of the instance is equal
   // to the content length.
   if (!partial_response)
     instance_size_ = content_length_;
@@ skipping 367 matching lines @@
   if (buffer_.get()) {
     media_log_->AddEvent(
         media_log_->CreateBufferedExtentsChangedEvent(
             offset_ - buffer_->backward_bytes(),
             offset_,
             offset_ + buffer_->forward_bytes()));
   }
 }
 
 }  // namespace webkit_media