Reduce unnecessary network connections.

Two different scenarios are fixed by this:
a) When Range:0- receives a 200 request, don't make a new connection asking for
   the same data without the Range:0- request heaeder (bug 104783). 
b) When a 4xx or malformed HTTP response is received on a Range:0- request,
   don't re-attempt the request without the Range header (bug 105230).

BUG=104783,105230
TEST=awesome scherkus@ tests now pass w/ the extra expectations removed.


Review URL: http://codereview.chromium.org/9699035

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@126970 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 17 insertions, 63 deletions

diff --git a/webkit/media/buffered_data_source.cc b/webkit/media/buffered_data_source.cc
index bd261f0..1f67f5b 100644
--- a/webkit/media/buffered_data_source.cc
+++ b/webkit/media/buffered_data_source.cc
@@ -42,7 +42,6 @@ BufferedDataSource::BufferedDataSource(
       media_is_paused_(true),
       media_has_played_(false),
       preload_(media::AUTO),
-      using_range_request_(true),
       cache_miss_retries_left_(kNumCacheMissRetries),
       bitrate_(0),
       playback_rate_(0.0),
@@ -392,19 +391,6 @@ void BufferedDataSource::HttpInitialStartCallback(int error) {
     loader_->Stop();
   }
 
-  if (error == net::ERR_INVALID_RESPONSE && using_range_request_) {
-    // Assuming that the Range header was causing the problem. Retry without
-    // the Range header.
-    using_range_request_ = false;
-    loader_.reset(CreateResourceLoader(kPositionNotSpecified,
-                                       kPositionNotSpecified));
-    loader_->Start(
-        base::Bind(&BufferedDataSource::HttpInitialStartCallback, this),
-        base::Bind(&BufferedDataSource::NetworkEventCallback, this),
-        frame_);
-    return;
-  }
-
   // Reference to prevent destruction while inside the |initialize_cb_|
   // call. This is a temporary fix to prevent crashes caused by holding the
   // lock and running the destructor.
diff --git a/webkit/media/buffered_data_source.h b/webkit/media/buffered_data_source.h
index 331a63b..e9d979e 100644
--- a/webkit/media/buffered_data_source.h
+++ b/webkit/media/buffered_data_source.h
@@ -196,10 +196,6 @@ class BufferedDataSource : public WebDataSource {
   // element.
   media::Preload preload_;
 
-  // Keeps track of whether we used a Range header in the initialization
-  // request.
-  bool using_range_request_;
-
   // Number of cache miss retries left.
   int cache_miss_retries_left_;
 
diff --git a/webkit/media/buffered_data_source_unittest.cc b/webkit/media/buffered_data_source_unittest.cc
index 6680633..bb7c8be 100644
--- a/webkit/media/buffered_data_source_unittest.cc
+++ b/webkit/media/buffered_data_source_unittest.cc
@@ -202,14 +202,6 @@ TEST_F(BufferedDataSourceTest, Range_Supported) {
 
 TEST_F(BufferedDataSourceTest, Range_NotFound) {
   Initialize(media::PIPELINE_ERROR_NETWORK);
-
-  // It'll try again.
-  //
-  // TODO(scherkus): don't try again on errors http://crbug.com/105230
-  ExpectCreateResourceLoader();
-  Respond(response_generator_.Generate404());
-
-  // Now it's done and will fail.
   Respond(response_generator_.Generate404());
 
   EXPECT_FALSE(data_source_->loading());
@@ -218,14 +210,6 @@ TEST_F(BufferedDataSourceTest, Range_NotFound) {
 
 TEST_F(BufferedDataSourceTest, Range_NotSupported) {
   Initialize(media::PIPELINE_OK);
-
-  // It'll try again.
-  //
-  // TODO(scherkus): try to reuse existing connection http://crbug.com/104783
-  ExpectCreateResourceLoader();
-  Respond(response_generator_.Generate200());
-
-  // Now it'll succeed.
   EXPECT_CALL(host_, SetTotalBytes(response_generator_.content_length()));
   EXPECT_CALL(host_, SetBufferedBytes(0));
   Respond(response_generator_.Generate200());
@@ -237,15 +221,6 @@ TEST_F(BufferedDataSourceTest, Range_NotSupported) {
 
 TEST_F(BufferedDataSourceTest, Range_MissingContentRange) {
   Initialize(media::PIPELINE_ERROR_NETWORK);
-
-  // It'll try again.
-  //
-  // TODO(scherkus): don't try again on errors http://crbug.com/105230
-  ExpectCreateResourceLoader();
-  Respond(response_generator_.Generate206(
-      0, TestResponseGenerator::kNoContentRange));
-
-  // Now it's done and will fail.
   Respond(response_generator_.Generate206(
       0, TestResponseGenerator::kNoContentRange));
 
@@ -269,12 +244,6 @@ TEST_F(BufferedDataSourceTest, Range_MissingContentLength) {
 TEST_F(BufferedDataSourceTest, Range_WrongContentRange) {
   Initialize(media::PIPELINE_ERROR_NETWORK);
 
-  // It'll try again.
-  //
-  // TODO(scherkus): don't try again on errors http://crbug.com/105230
-  ExpectCreateResourceLoader();
-  Respond(response_generator_.Generate206(1337));
-
   // Now it's done and will fail.
   Respond(response_generator_.Generate206(1337));
 
diff --git a/webkit/media/buffered_resource_loader.cc b/webkit/media/buffered_resource_loader.cc
index 53a236e..953f265 100644
--- a/webkit/media/buffered_resource_loader.cc
+++ b/webkit/media/buffered_resource_loader.cc
@@ -376,21 +376,28 @@ void BufferedResourceLoader::didReceiveResponse(
   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);
-
     partial_response = (response.httpStatusCode() == kHttpPartialContent);
+    bool ok_response = (response.httpStatusCode() == kHttpOK);
 
     if (range_requested_) {
+      // 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);
+
       // 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;
@@ -400,19 +407,15 @@ void BufferedResourceLoader::didReceiveResponse(
       DoneStart(error);
       return;
     }
-  } else {
-    // For any protocol other than HTTP and HTTPS, assume range request is
-    // always fulfilled.
-    partial_response = range_requested_;
   }
 
   // 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)
+  // If we have not requested a range or have not received a range, then the
+  // size of the instance is equal to the content length.
+  if (!range_requested_ || !partial_response)
     instance_size_ = content_length_;
 
   // Calls with a successful response.