Split a portion of BufferedResourceLoader into a separate class ActiveLoader.

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 b571dbc108408ce01ead35bbee58e981eae0739f..536ffdb738e19d7f2347cdd8656b2ea1360c5297 100644
--- a/webkit/media/buffered_resource_loader.cc
+++ b/webkit/media/buffered_resource_loader.cc
@@ -93,7 +93,6 @@ static void ComputeTargetBufferWindow(float playback_rate, int bitrate,
     std::swap(*out_forward_capacity, *out_backward_capacity);
 }
 
-
 BufferedResourceLoader::BufferedResourceLoader(
     const GURL& url,
     int64 first_byte_position,
@@ -102,9 +101,7 @@ BufferedResourceLoader::BufferedResourceLoader(
     int bitrate,
     float playback_rate,
     media::MediaLog* media_log)
-    : deferred_(false),
-      defer_strategy_(strategy),
-      completed_(false),
+    : defer_strategy_(strategy),
       range_requested_(false),
       range_supported_(false),
       saved_forward_capacity_(0),
@@ -122,7 +119,6 @@ BufferedResourceLoader::BufferedResourceLoader(
       read_buffer_(NULL),
       first_offset_(0),
       last_offset_(0),
-      keep_test_loader_(false),
       bitrate_(bitrate),
       playback_rate_(playback_rate),
       media_log_(media_log) {
@@ -134,10 +130,7 @@ BufferedResourceLoader::BufferedResourceLoader(
   buffer_.reset(new media::SeekableBuffer(backward_capacity, forward_capacity));
 }
 
-BufferedResourceLoader::~BufferedResourceLoader() {
-  if (!completed_ && url_loader_.get())
-    url_loader_->cancel();
-}
+BufferedResourceLoader::~BufferedResourceLoader() {}
 
 void BufferedResourceLoader::Start(net::OldCompletionCallback* start_callback,
                                    const base::Closure& event_callback,
@@ -158,10 +151,6 @@ void BufferedResourceLoader::Start(net::OldCompletionCallback* start_callback,
     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);
@@ -180,17 +169,21 @@ void BufferedResourceLoader::Start(net::OldCompletionCallback* start_callback,
       WebString::fromUTF8(net::HttpRequestHeaders::kAcceptEncoding),
       WebString::fromUTF8("identity;q=1, *;q=0"));
 
-  // This flag is for unittests as we don't want to reset |url_loader|
-  if (!keep_test_loader_) {
+  // Check for our test WebURLLoader.
+  WebURLLoader* loader = NULL;
+  if (test_loader_.get()) {
+    loader = test_loader_.release();
+  } else {
     WebURLLoaderOptions options;
     options.allowCredentials = true;
     options.crossOriginRequestPolicy =
         WebURLLoaderOptions::CrossOriginRequestPolicyAllow;
-    url_loader_.reset(frame->createAssociatedURLLoader(options));
+    loader = frame->createAssociatedURLLoader(options);
   }
 
   // Start the resource loading.
-  url_loader_->loadAsynchronously(request, this);
+  loader->loadAsynchronously(request, this);
+  active_loader_.reset(new ActiveLoader(this, loader));
 }
 
 void BufferedResourceLoader::Stop() {
@@ -207,16 +200,8 @@ void BufferedResourceLoader::Stop() {
   // Destroy internal buffer.
   buffer_.reset();
 
-  if (url_loader_.get()) {
-    if (deferred_)
-      url_loader_->setDefersLoading(false);
-    deferred_ = false;
-
-    if (!completed_) {
-      url_loader_->cancel();
-      completed_ = true;
-    }
-  }
+  // Cancel and reset any active loaders.
+  active_loader_.reset();
 }
 
 void BufferedResourceLoader::Read(int64 position,
@@ -292,9 +277,7 @@ void BufferedResourceLoader::Read(int64 position,
     }
 
     // Make sure we stop deferring now that there's additional capacity.
-    //
-    // XXX: can we DCHECK(url_loader_.get()) at this point in time?
-    if (deferred_)
+    if (active_loader_->deferred())
       SetDeferred(false);
 
     DCHECK(!ShouldEnableDefer())
@@ -326,16 +309,15 @@ bool BufferedResourceLoader::range_supported() {
 }
 
 bool BufferedResourceLoader::is_downloading_data() {
-  return !completed_ && !deferred_;
+  return active_loader_.get() && !active_loader_->deferred();
 }
 
 const GURL& BufferedResourceLoader::url() {
   return url_;
 }
 
-void BufferedResourceLoader::SetURLLoaderForTest(WebURLLoader* mock_loader) {
-  url_loader_.reset(mock_loader);
-  keep_test_loader_ = true;
+void BufferedResourceLoader::SetURLLoaderForTest(WebURLLoader* test_loader) {
+  test_loader_.reset(test_loader);
 }
 
 /////////////////////////////////////////////////////////////////////////////
@@ -371,6 +353,7 @@ void BufferedResourceLoader::didReceiveResponse(
     WebURLLoader* loader,
     const WebURLResponse& response) {
   VLOG(1) << "didReceiveResponse: " << response.httpStatusCode();
+  DCHECK(active_loader_.get());
 
   // The loader may have been stopped and |start_callback| is destroyed.
   // In this case we shouldn't do anything.
@@ -408,7 +391,6 @@ void BufferedResourceLoader::didReceiveResponse(
 
     if (error != net::OK) {
       DoneStart(error);
-      Stop();
       return;
     }
   } else {
@@ -436,8 +418,7 @@ void BufferedResourceLoader::didReceiveData(
     int data_length,
     int encoded_data_length) {
   VLOG(1) << "didReceiveData: " << data_length << " bytes";
-
-  DCHECK(!completed_);
+  DCHECK(active_loader_.get());
   DCHECK_GT(data_length, 0);
 
   // If this loader has been stopped, |buffer_| would be destroyed.
@@ -485,9 +466,8 @@ void BufferedResourceLoader::didFinishLoading(
     WebURLLoader* loader,
     double finishTime) {
   VLOG(1) << "didFinishLoading";
-
-  DCHECK(!completed_);
-  completed_ = true;
+  DCHECK(active_loader_.get());
+  active_loader_.reset();
 
   // If we didn't know the |instance_size_| we do now.
   if (instance_size_ == kPositionNotSpecified) {
@@ -517,35 +497,27 @@ void BufferedResourceLoader::didFinishLoading(
 
   // Notify that network response is completed.
   NotifyNetworkEvent();
-
-  url_loader_.reset();
-  Release();
 }
 
 void BufferedResourceLoader::didFail(
     WebURLLoader* loader,
     const WebURLError& error) {
   VLOG(1) << "didFail: " << error.reason;
+  DCHECK(active_loader_.get());
+  active_loader_.reset();
 
-  DCHECK(!completed_);
-  completed_ = true;
+  NotifyNetworkEvent();
 
-  // If there is a start callback, calls it.
+  // Don't leave start callbacks hanging around.
   if (start_callback_.get()) {
     DoneStart(error.reason);
+    return;

[vrk (LEFT CHROMIUM), 2011/12/01 00:22:28]

As mentioned offline: DCHECK that you don't have both a start callback and a
read callback.

[scherkus (not reviewing), 2011/12/02 18:24:24]

Done.

   }
 
-  // If there is a pending read but the request failed, return with the
-  // reason for the error.
+  // Don't leave read callbacks hanging around.
   if (HasPendingRead()) {
     DoneRead(error.reason);
   }
-
-  // Notify that network response is completed.
-  NotifyNetworkEvent();
-
-  url_loader_.reset();
-  Release();
 }
 
 bool BufferedResourceLoader::HasSingleOrigin() const {
@@ -594,26 +566,23 @@ void BufferedResourceLoader::UpdateBufferWindow() {
 }
 
 void BufferedResourceLoader::UpdateDeferBehavior() {
-  if (!url_loader_.get() || !buffer_.get())
+  if (!active_loader_.get() || !buffer_.get())
     return;
 
   // If necessary, toggle defer state and continue/pause downloading data
   // accordingly.
   if (ShouldEnableDefer() || ShouldDisableDefer())
-    SetDeferred(!deferred_);
+    SetDeferred(!active_loader_->deferred());
 }
 
 void BufferedResourceLoader::SetDeferred(bool deferred) {
-  deferred_ = deferred;
-  if (url_loader_.get()) {
-    url_loader_->setDefersLoading(deferred);
-    NotifyNetworkEvent();
-  }
+  active_loader_->SetDeferred(deferred);
+  NotifyNetworkEvent();
 }
 
-bool BufferedResourceLoader::ShouldEnableDefer() {
+bool BufferedResourceLoader::ShouldEnableDefer() const {
   // If we're already deferring, then enabling makes no sense.
-  if (deferred_)
+  if (active_loader_->deferred())
     return false;
 
   switch(defer_strategy_) {
@@ -633,9 +602,9 @@ bool BufferedResourceLoader::ShouldEnableDefer() {
   return false;
 }
 
-bool BufferedResourceLoader::ShouldDisableDefer() {
+bool BufferedResourceLoader::ShouldDisableDefer() const {
   // If we're not deferring, then disabling makes no sense.
-  if (!deferred_)
+  if (!active_loader_->deferred())
     return false;
 
   switch(defer_strategy_) {
@@ -662,7 +631,7 @@ bool BufferedResourceLoader::ShouldDisableDefer() {
   return false;
 }
 
-bool BufferedResourceLoader::CanFulfillRead() {
+bool BufferedResourceLoader::CanFulfillRead() const {
   // If we are reading too far in the backward direction.
   if (first_offset_ < 0 &&
       first_offset_ + static_cast<int>(buffer_->backward_bytes()) < 0)
@@ -674,7 +643,7 @@ bool BufferedResourceLoader::CanFulfillRead() {
 
   // At the point, we verified that first byte requested is within the buffer.
   // If the request has completed, then just returns with what we have now.
-  if (completed_)
+  if (!active_loader_.get())
     return true;
 
   // If the resource request is still active, make sure the whole requested
@@ -685,7 +654,7 @@ bool BufferedResourceLoader::CanFulfillRead() {
   return true;
 }
 
-bool BufferedResourceLoader::WillFulfillRead() {
+bool BufferedResourceLoader::WillFulfillRead() const {
   // Trying to read too far behind.
   if (first_offset_ < 0 &&
       first_offset_ + static_cast<int>(buffer_->backward_bytes()) < 0)
@@ -698,7 +667,7 @@ bool BufferedResourceLoader::WillFulfillRead() {
 
   // The resource request has completed, there's no way we can fulfill the
   // read request.
-  if (completed_)
+  if (!active_loader_.get())
     return false;
 
   return true;
@@ -764,8 +733,6 @@ std::string BufferedResourceLoader::GenerateHeaders(
 }
 
 void BufferedResourceLoader::DoneRead(int error) {
-  read_callback_->RunWithParams(Tuple1<int>(error));
-  read_callback_.reset();
   if (buffer_.get() && saved_forward_capacity_) {
     buffer_->set_forward_capacity(saved_forward_capacity_);
     saved_forward_capacity_ = 0;
@@ -776,11 +743,16 @@ void BufferedResourceLoader::DoneRead(int error) {
   first_offset_ = 0;
   last_offset_ = 0;
   Log();
+
+  scoped_ptr<net::OldCompletionCallback> read_callback;
+  read_callback.swap(read_callback_);
+  read_callback->RunWithParams(Tuple1<int>(error));
 }
 
 void BufferedResourceLoader::DoneStart(int error) {
-  start_callback_->RunWithParams(Tuple1<int>(error));
-  start_callback_.reset();
+  scoped_ptr<net::OldCompletionCallback> start_callback;
+  start_callback.swap(start_callback_);
+  start_callback->RunWithParams(Tuple1<int>(error));
 }
 
 void BufferedResourceLoader::NotifyNetworkEvent() {