Only make Range requests when the desired range doesn't cover the whole file.

webkit/glue/media/buffered_resource_loader.cc

 // Copyright (c) 2011 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/glue/media/buffered_resource_loader.h"
 
 #include "base/format_macros.h"
 #include "base/string_util.h"
 #include "net/base/net_errors.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebKit.h"
@@ skipping 35 matching lines @@
 
 BufferedResourceLoader::BufferedResourceLoader(
     const GURL& url,
     int64 first_byte_position,
     int64 last_byte_position)
     : buffer_(new media::SeekableBuffer(kBackwardCapcity, kForwardCapacity)),
       deferred_(false),
       defer_strategy_(kReadThenDefer),
       completed_(false),
       range_requested_(false),
-      partial_response_(false),
+      range_supported_(false),
       url_(url),
       first_byte_position_(first_byte_position),
       last_byte_position_(last_byte_position),
       single_origin_(true),
       start_callback_(NULL),
       offset_(0),
       content_length_(kPositionNotSpecified),
       instance_size_(kPositionNotSpecified),
       read_callback_(NULL),
       read_position_(0),
@@ skipping 16 matching lines @@
   DCHECK(!start_callback_.get());
   DCHECK(!event_callback_.get());
   DCHECK(start_callback);
   DCHECK(event_callback);
   CHECK(frame);
 
   start_callback_.reset(start_callback);
   event_callback_.reset(event_callback);
 
   if (first_byte_position_ != kPositionNotSpecified) {
-    range_requested_ = true;
     // TODO(hclam): server may not support range request so |offset_| may not
     // equal to |first_byte_position_|.
     offset_ = first_byte_position_;
   }
 
   // Increment the reference count right before we start the request. This
   // reference will be release when this request has ended.
   AddRef();
 
   // Prepare the request.
   WebURLRequest request(url_);
   request.setTargetType(WebURLRequest::TargetIsMedia);
-  request.setHTTPHeaderField(WebString::fromUTF8("Range"),
-                             WebString::fromUTF8(GenerateHeaders(
-                                                 first_byte_position_,
-                                                 last_byte_position_)));
+
+  if (!IsWholeFileRange()) {
+    range_requested_ = true;
+    request.setHTTPHeaderField(WebString::fromUTF8("Range"),
+                               WebString::fromUTF8(GenerateHeaders(
+                                   first_byte_position_,
+                                   last_byte_position_)));
+  }
   frame->setReferrerForRequest(request, WebKit::WebURL());
 
   // This flag is for unittests as we don't want to reset |url_loader|
   if (!keep_test_loader_)
     url_loader_.reset(frame->createAssociatedURLLoader());
 
   // Start the resource loading.
   url_loader_->loadAsynchronously(request, this);
 }
 
@@ skipping 85 matching lines @@
 }
 
 int64 BufferedResourceLoader::content_length() {
   return content_length_;
 }
 
 int64 BufferedResourceLoader::instance_size() {
   return instance_size_;
 }
 
-bool BufferedResourceLoader::partial_response() {
-  return partial_response_;
+bool BufferedResourceLoader::range_supported() {
+  return range_supported_;
 }
 
 bool BufferedResourceLoader::network_activity() {
   return !completed_ && !deferred_;
 }
 
 const GURL& BufferedResourceLoader::url() {
   return url_;
 }
 
@@ skipping 40 matching lines @@
 
 void BufferedResourceLoader::didReceiveResponse(
     WebURLLoader* loader,
     const WebURLResponse& response) {
 
   // The loader may have been stopped and |start_callback| is destroyed.
   // In this case we shouldn't do anything.
   if (!start_callback_.get())
     return;
 
+  bool partial_response = false;
+
   // 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;
 
-    if (response.httpStatusCode() == kHttpPartialContent)
-      partial_response_ = true;
+    // 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 (range_requested_ && partial_response_) {
+    partial_response = (response.httpStatusCode() == kHttpPartialContent);
+
+    if (range_requested_) {
       // If we have verified the partial response and it is correct, we will
       // return net::OK.
-      if (!VerifyPartialResponse(response))
+      if (!partial_response || !VerifyPartialResponse(response))
         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);
       Stop();
       return;
     }
   } else {
     // For any protocol other than HTTP and HTTPS, assume range request is
     // always fulfilled.
-    partial_response_ = range_requested_;
+    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 (!partial_response)
     instance_size_ = content_length_;
 
   // Calls with a successful response.
   DoneStart(net::OK);
 }
 
 void BufferedResourceLoader::didReceiveData(
     WebURLLoader* loader,
     const char* data,
     int data_length,
@@ skipping 40 matching lines @@
     int data_length) {
   NOTIMPLEMENTED();
 }
 
 void BufferedResourceLoader::didFinishLoading(
     WebURLLoader* loader,
     double finishTime) {
   DCHECK(!completed_);
   completed_ = true;
 
+  if (instance_size_ == kPositionNotSpecified) {
+    // If we didn't know the |instance_size_| we do now.

[scherkus (not reviewing), 2011/04/11 17:41:55]

pedantic nit: the rest of this file seems to put if-statement comments before
the if as opposed to inside the if

[acolwell GONE FROM CHROMIUM, 2011/04/18 22:09:56]

Done.

+    instance_size_ = offset_ + buffer_->forward_bytes();
+  }
+
   // If there is a start callback, calls it.
   if (start_callback_.get()) {
     DoneStart(net::OK);
   }
 
   // If there is a pending read but the request has ended, returns with what
   // we have.
   if (HasPendingRead()) {
     // Make sure we have a valid buffer before we satisfy a read request.
     DCHECK(buffer_.get());
@@ skipping 237 matching lines @@
 void BufferedResourceLoader::DoneStart(int error) {
   start_callback_->RunWithParams(Tuple1<int>(error));
   start_callback_.reset();
 }
 
 void BufferedResourceLoader::NotifyNetworkEvent() {
   if (event_callback_.get())
     event_callback_->Run();
 }
 
+bool BufferedResourceLoader::IsWholeFileRange() const {
+  return ((first_byte_position_ == kPositionNotSpecified ||
+           first_byte_position_ == 0) &&
+          last_byte_position_ == kPositionNotSpecified);

[scherkus (not reviewing), 2011/04/11 17:41:55]

is it worth checking last_byte_position_ == content_length_ ?

if content_length_ is still kPositionNotSpecified, then we're good
if content_length_ is properly set, then we're also good (however I do find this
extremely unintuitive so perhaps better to not handle this case)

[acolwell GONE FROM CHROMIUM, 2011/04/18 22:09:56]

I don't think this is worth checking. It would cause inconsistent behavior if
the the range was passed in the constructor and then a Read() for the same range
was issued. In the first request, it would use the range because we don't know
how long the file is yet. In the second request the range would not be used
because the file size is known. I can see this causing confusion at some point
down the line.

I suppose I could change the name of the method to something a little more
specific like IsUnspecifiedOrZeroToEndRange(). That seems a little long winded,
but more accurate.

+}
+
 }  // namespace webkit_glue