Split BufferedResourceLoader's network callback into separate loading state and progress callbacks.

webkit/media/buffered_data_source.cc

Index: webkit/media/buffered_data_source.cc
diff --git a/webkit/media/buffered_data_source.cc b/webkit/media/buffered_data_source.cc
index 0aec84a95ed9ecb2f044e0fdbccfd36fed2213f6..4528761a1829492f88f85a03ff5c21ea26269793 100644
--- a/webkit/media/buffered_data_source.cc
+++ b/webkit/media/buffered_data_source.cc
@@ -22,6 +22,11 @@ static const int kInitialReadBufferSize = 32768;
 // error.
 static const int kNumCacheMissRetries = 3;
 
+// Non-HTTP resources are assumed to be fully loaded so we ignore any
+// loading/progress related callbacks.

[scherkus (not reviewing), 2012/07/06 16:28:35]

see http://codereview.chromium.org/10692106/ -- I ended up deleting this

[Ami GONE FROM CHROMIUM, 2012/07/09 03:25:10]

FTR, you mean https://chromiumcodereview.appspot.com/10735016/

+static void NonHttpLoadingCallback(BufferedResourceLoader::LoadingState) {}
+static void NonHttpProgressCallback(int64) {}
+
 BufferedDataSource::BufferedDataSource(
     MessageLoop* render_loop,
     WebFrame* frame,
@@ -95,7 +100,8 @@ void BufferedDataSource::Initialize(
     loader_.reset(CreateResourceLoader(0, kPositionNotSpecified));
     loader_->Start(
         base::Bind(&BufferedDataSource::HttpInitialStartCallback, this),
-        base::Bind(&BufferedDataSource::NetworkEventCallback, this),
+        base::Bind(&BufferedDataSource::HttpLoadingCallback, this),
+        base::Bind(&BufferedDataSource::HttpProgressCallback, this),
         frame_);
     return;
   }
@@ -107,7 +113,8 @@ void BufferedDataSource::Initialize(
                                      kPositionNotSpecified));
   loader_->Start(
       base::Bind(&BufferedDataSource::NonHttpInitialStartCallback, this),
-      base::Bind(&BufferedDataSource::NetworkEventCallback, this),
+      base::Bind(&NonHttpLoadingCallback),
+      base::Bind(&NonHttpProgressCallback),
       frame_);
 }
 
@@ -255,12 +262,22 @@ void BufferedDataSource::RestartLoadingTask() {
       return;
   }
 
+  // Start reading from where we last left off until the end of the resource.
   loader_.reset(
       CreateResourceLoader(last_read_start_, kPositionNotSpecified));
-  loader_->Start(
-      base::Bind(&BufferedDataSource::PartialReadStartCallback, this),
-      base::Bind(&BufferedDataSource::NetworkEventCallback, this),
-      frame_);
+  if (url_.SchemeIs(kHttpScheme) || url_.SchemeIs(kHttpsScheme)) {
+    loader_->Start(
+        base::Bind(&BufferedDataSource::PartialReadStartCallback, this),
+        base::Bind(&BufferedDataSource::HttpLoadingCallback, this),
+        base::Bind(&BufferedDataSource::HttpProgressCallback, this),
+        frame_);
+  } else {
+    loader_->Start(
+        base::Bind(&BufferedDataSource::NonHttpInitialStartCallback, this),
+        base::Bind(&NonHttpLoadingCallback),
+        base::Bind(&NonHttpProgressCallback),
+        frame_);
+  }
 }
 
 void BufferedDataSource::SetPlaybackRateTask(float playback_rate) {
@@ -504,14 +521,8 @@ void BufferedDataSource::ReadCallback(
     bytes_read = kReadError;
   }
 
-  // We need to prevent calling to filter host and running the callback if
-  // we have received the stop signal. We need to lock down the whole callback
-  // method to prevent bad things from happening. The reason behind this is
-  // that we cannot guarantee tasks on render thread have completely stopped
-  // when we receive the Stop() method call. So only way to solve this is to
-  // let tasks on render thread to run but make sure they don't call outside
-  // this object when Stop() method is ever called. Locking this method is safe
-  // because |lock_| is only acquired in tasks on render thread.
+  // TODO(scherkus): we shouldn't have to lock to signal host(), see
+  // http://crbug.com/113712 for details.
   base::AutoLock auto_lock(lock_);
   if (stop_signal_received_)
     return;
@@ -532,46 +543,58 @@ void BufferedDataSource::ReadCallback(
   DoneRead_Locked(bytes_read);
 }
 
-void BufferedDataSource::NetworkEventCallback() {
+void BufferedDataSource::HttpLoadingCallback(
+    BufferedResourceLoader::LoadingState state) {
   DCHECK(MessageLoop::current() == render_loop_);
-  DCHECK(loader_.get());
-
-  // In case of non-HTTP request we don't need to report network events,
-  // so return immediately.
-  if (!url_.SchemeIs(kHttpScheme) && !url_.SchemeIs(kHttpsScheme))
-    return;
+  DCHECK(url_.SchemeIs(kHttpScheme) || url_.SchemeIs(kHttpsScheme));
 
-  bool is_downloading_data = loader_->is_downloading_data();
-  int64 current_buffered_position = loader_->GetBufferedPosition();
-
-  // If we get an unspecified value, return immediately.
-  if (current_buffered_position == kPositionNotSpecified)
-    return;
-
-  // We need to prevent calling to filter host and running the callback if
-  // we have received the stop signal. We need to lock down the whole callback
-  // method to prevent bad things from happening. The reason behind this is
-  // that we cannot guarantee tasks on render thread have completely stopped
-  // when we receive the Stop() method call. So only way to solve this is to
-  // let tasks on render thread to run but make sure they don't call outside
-  // this object when Stop() method is ever called. Locking this method is safe
-  // because |lock_| is only acquired in tasks on render thread.
+  // TODO(scherkus): we shouldn't have to lock to signal host(), see
+  // http://crbug.com/113712 for details.
   base::AutoLock auto_lock(lock_);
   if (stop_signal_received_)
     return;
 
+  bool is_downloading_data;
+  switch (state) {
+    case BufferedResourceLoader::kLoading:
+      is_downloading_data = true;
+      break;
+    case BufferedResourceLoader::kLoadingDeferred:
+      is_downloading_data = false;
+      break;
+
+    // TODO(scherkus): we don't signal network activity changes when loads
+    // complete or fail to preserve existing behaviour when deferring is
+    // toggled, however SetNetworkActivity() should use a similar enum as there
+    // isn't any signal today to notify the client that loading has
+    // failed/finished (we only get errors on subsequent reads).
+    case BufferedResourceLoader::kLoadingFailed:
+    case BufferedResourceLoader::kLoadingFinished:
+      return;
+  }
+
   if (is_downloading_data != is_downloading_data_) {
     is_downloading_data_ = is_downloading_data;
     if (host())
       host()->SetNetworkActivity(is_downloading_data);
   }
+}
+
+void BufferedDataSource::HttpProgressCallback(int64 position) {
+  DCHECK(MessageLoop::current() == render_loop_);
+  DCHECK(url_.SchemeIs(kHttpScheme) || url_.SchemeIs(kHttpsScheme));
+
+  // TODO(scherkus): we shouldn't have to lock to signal host(), see
+  // http://crbug.com/113712 for details.
+  base::AutoLock auto_lock(lock_);
+  if (stop_signal_received_)
+    return;
 
-  if (host() && current_buffered_position > last_read_start_)
-    host()->AddBufferedByteRange(last_read_start_, current_buffered_position);
+  if (host() && position > last_read_start_)
+    host()->AddBufferedByteRange(last_read_start_, position);
 }
 
 void BufferedDataSource::UpdateHostState_Locked() {
-  // Called from various threads, under lock.
   lock_.AssertAcquired();
 
   if (!host())