Collapse HTTP and non-HTTP codepaths into single methods.

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 4528761a1829492f88f85a03ff5c21ea26269793..17431022b85fbffac88150591b31d39b7b592638 100644
--- a/webkit/media/buffered_data_source.cc
+++ b/webkit/media/buffered_data_source.cc
@@ -22,11 +22,6 @@ 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.
-static void NonHttpLoadingCallback(BufferedResourceLoader::LoadingState) {}
-static void NonHttpProgressCallback(int64) {}
-
 BufferedDataSource::BufferedDataSource(
     MessageLoop* render_loop,
     WebFrame* frame,
@@ -98,23 +93,18 @@ void BufferedDataSource::Initialize(
     // Do an unbounded range request starting at the beginning.  If the server
     // responds with 200 instead of 206 we'll fall back into a streaming mode.
     loader_.reset(CreateResourceLoader(0, kPositionNotSpecified));
-    loader_->Start(
-        base::Bind(&BufferedDataSource::HttpInitialStartCallback, this),
-        base::Bind(&BufferedDataSource::HttpLoadingCallback, this),
-        base::Bind(&BufferedDataSource::HttpProgressCallback, this),
-        frame_);
-    return;
+  } else {
+    // For all other protocols, assume they support range request. We fetch

[Ami GONE FROM CHROMIUM, 2012/07/09 03:30:57]

s/support/don't support/?

[scherkus (not reviewing), 2012/07/10 18:37:27]

My memory is foggy but I believe it _is_ support but IIRC we don't get a C-L
header or something when we issue ranges against non-HTTP protocols (i.e.,
bytes=0-), hence issuing a regular GET

I suppose the "real fix" is digging into protocol handler to see when we do get
/ don't get C-L/C-R headers

+    // the full range of the resource to obtain the instance size because
+    // we won't be served HTTP headers.
+    loader_.reset(CreateResourceLoader(kPositionNotSpecified,
+                                       kPositionNotSpecified));
   }
 
-  // For all other protocols, assume they support range request. We fetch
-  // the full range of the resource to obtain the instance size because
-  // we won't be served HTTP headers.
-  loader_.reset(CreateResourceLoader(kPositionNotSpecified,
-                                     kPositionNotSpecified));
   loader_->Start(
-      base::Bind(&BufferedDataSource::NonHttpInitialStartCallback, this),
-      base::Bind(&NonHttpLoadingCallback),
-      base::Bind(&NonHttpProgressCallback),
+      base::Bind(&BufferedDataSource::StartCallback, this),
+      base::Bind(&BufferedDataSource::LoadingCallback, this),
+      base::Bind(&BufferedDataSource::ProgressCallback, this),
       frame_);
 }
 
@@ -263,21 +253,12 @@ void BufferedDataSource::RestartLoadingTask() {
   }
 
   // Start reading from where we last left off until the end of the resource.
-  loader_.reset(
-      CreateResourceLoader(last_read_start_, kPositionNotSpecified));
-  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),

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

I believe this was a bug!

if for whatever reason we retried a failed read on non-HTTP resources we would
never execute the read again

[Ami GONE FROM CHROMIUM, 2012/07/09 03:30:57]

Worth a test?

[scherkus (not reviewing), 2012/07/10 18:37:27]

I'm going to prep a separate CL that I'll land before this CL that beefs up our
test coverage -- we have no tests for non-HTTP codepath at all :s

-        base::Bind(&NonHttpLoadingCallback),
-        base::Bind(&NonHttpProgressCallback),
-        frame_);
-  }
+  loader_.reset(CreateResourceLoader(last_read_start_, kPositionNotSpecified));
+  loader_->Start(
+      base::Bind(&BufferedDataSource::PartialReadStartCallback, this),
+      base::Bind(&BufferedDataSource::LoadingCallback, this),
+      base::Bind(&BufferedDataSource::ProgressCallback, this),
+      frame_);
 }
 
 void BufferedDataSource::SetPlaybackRateTask(float playback_rate) {
@@ -360,7 +341,7 @@ void BufferedDataSource::DoneInitialization_Locked(
 
 /////////////////////////////////////////////////////////////////////////////
 // BufferedResourceLoader callback methods.
-void BufferedDataSource::HttpInitialStartCallback(
+void BufferedDataSource::StartCallback(
     BufferedResourceLoader::Status status) {
   DCHECK(MessageLoop::current() == render_loop_);
   DCHECK(loader_.get());
@@ -375,90 +356,36 @@ void BufferedDataSource::HttpInitialStartCallback(
     return;
   }
 
-  bool success = status == BufferedResourceLoader::kOk;
-  if (success) {
-    // TODO(hclam): Needs more thinking about supporting servers without range
-    // request or their partial response is not complete.
-    total_bytes_ = loader_->instance_size();
-    streaming_ = (total_bytes_ == kPositionNotSpecified) ||
-        !loader_->range_supported();
+  bool success = false;
+  if (url_.SchemeIs(kHttpScheme) || url_.SchemeIs(kHttpsScheme)) {
+    // HTTP responses only require a successful response even if content length

[Ami GONE FROM CHROMIUM, 2012/07/09 03:30:57]

I think you meant the "only" to bind to "HTTP responses" instead of "require".
Reword?

[Ami GONE FROM CHROMIUM, 2012/07/09 03:30:57]

I think this if/else would be a lot clearer if you laid out the criteria for
success without interleaving them with member-setting statements.

bool is_http = url_.SchemeIs(kHttpScheme) || url_.SchemeIs(kHttpsScheme);
bool success = (status == BufferedResourceLoader::kOk) && 
    (is_http || loader_->instance_size() != kPositionNotSpecified);

if (success) {
  if (is_http) {
    ...
  } else {
    ...
  }
} else {
  loader_->Stop();
}

[scherkus (not reviewing), 2012/07/10 18:37:27]

Done.

[scherkus (not reviewing), 2012/07/10 18:37:27]

Done.

+    // isn't known at the time.
+    if (status == BufferedResourceLoader::kOk) {
+      success = true;
+      total_bytes_ = loader_->instance_size();
+      streaming_ = (total_bytes_ == kPositionNotSpecified) ||
+          !loader_->range_supported();
+    }
   } else {
-    // TODO(hclam): In case of failure, we can retry several times.
-    loader_->Stop();
-  }
-
-  // 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.
-  // TODO: Review locking in this class and figure out a way to run the callback
-  //       w/o the lock.
-  scoped_refptr<BufferedDataSource> destruction_guard(this);
-  {
-    // 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. The 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.
-    base::AutoLock auto_lock(lock_);
-    if (stop_signal_received_)
-      return;
-
-    if (!success) {
-      DoneInitialization_Locked(media::PIPELINE_ERROR_NETWORK);
-      return;
+    // Non-HTTP resources should be full responses with known content length.
+    if (status == BufferedResourceLoader::kOk &&
+        loader_->instance_size() != kPositionNotSpecified) {
+      success = true;
+      total_bytes_ = loader_->instance_size();
+      buffered_bytes_ = loader_->instance_size();

[Ami GONE FROM CHROMIUM, 2012/07/09 03:30:57]

This is consistent with the "before" code, but is it right?  Does this CB only
fire after the full FTP file (or whatevs) is d/l'ed, or do we lie?

[scherkus (not reviewing), 2012/07/10 18:37:27]

We currently lie for all non-HTTP protocols

     }
-
-    UpdateHostState_Locked();
-    DoneInitialization_Locked(media::PIPELINE_OK);
   }
-}
 
-void BufferedDataSource::NonHttpInitialStartCallback(
-    BufferedResourceLoader::Status status) {
-  DCHECK(MessageLoop::current() == render_loop_);
-  DCHECK(loader_.get());
-
-  bool initialize_cb_is_null = false;
-  {
-    base::AutoLock auto_lock(lock_);
-    initialize_cb_is_null = initialize_cb_.is_null();
-  }
-  if (initialize_cb_is_null) {
-    loader_->Stop();
-    return;
-  }
-
-  int64 instance_size = loader_->instance_size();
-  bool success = status == BufferedResourceLoader::kOk &&
-     instance_size != kPositionNotSpecified;
-
-  if (success) {
-    total_bytes_ = instance_size;
-    buffered_bytes_ = total_bytes_;
-  } else {
+  if (!success) {
     loader_->Stop();
   }
 
-  // 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.
-  // TODO: Review locking in this class and figure out a way to run the callback
-  //       w/o the lock.
+  // TODO(scherkus): we shouldn't have to lock to signal host(), see
+  // http://crbug.com/113712 for details.
   scoped_refptr<BufferedDataSource> destruction_guard(this);
   {
-    // 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. The 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.
     base::AutoLock auto_lock(lock_);
-    if (stop_signal_received_ || initialize_cb_.is_null())
+    if (stop_signal_received_)
       return;
 
     if (!success) {
@@ -486,14 +413,8 @@ void BufferedDataSource::PartialReadStartCallback(
   // Stop the resource loader since we have received an error.
   loader_->Stop();
 
-  // 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;
@@ -543,10 +464,13 @@ void BufferedDataSource::ReadCallback(
   DoneRead_Locked(bytes_read);
 }
 
-void BufferedDataSource::HttpLoadingCallback(
+void BufferedDataSource::LoadingCallback(
     BufferedResourceLoader::LoadingState state) {
   DCHECK(MessageLoop::current() == render_loop_);
-  DCHECK(url_.SchemeIs(kHttpScheme) || url_.SchemeIs(kHttpsScheme));
+
+  // We assume non-HTTP resources are fully loaded: ignore any udpates.

[Ami GONE FROM CHROMIUM, 2012/07/09 03:30:57]

udpates

[Ami GONE FROM CHROMIUM, 2012/07/09 03:30:57]

Can we DCHECK the assumption?

[scherkus (not reviewing), 2012/07/10 18:37:27]

Done.

[scherkus (not reviewing), 2012/07/10 18:37:27]

I've switched to using assume_fully_buffered_ since buffered_bytes_ was only
used for non-HTTP case -- let me know what you think

+  if (!url_.SchemeIs(kHttpScheme) && !url_.SchemeIs(kHttpsScheme))
+    return;
 
   // TODO(scherkus): we shouldn't have to lock to signal host(), see
   // http://crbug.com/113712 for details.
@@ -580,10 +504,14 @@ void BufferedDataSource::HttpLoadingCallback(
   }
 }
 
-void BufferedDataSource::HttpProgressCallback(int64 position) {
+void BufferedDataSource::ProgressCallback(int64 position) {
   DCHECK(MessageLoop::current() == render_loop_);
   DCHECK(url_.SchemeIs(kHttpScheme) || url_.SchemeIs(kHttpsScheme));
 
+  // We assume non-HTTP resources are fully loaded: ignore any udpates.

[Ami GONE FROM CHROMIUM, 2012/07/09 03:30:57]

ditto to DCHECKing if possible.

[scherkus (not reviewing), 2012/07/10 18:37:27]

Done.

+  if (!url_.SchemeIs(kHttpScheme) && !url_.SchemeIs(kHttpsScheme))
+    return;
+
   // TODO(scherkus): we shouldn't have to lock to signal host(), see
   // http://crbug.com/113712 for details.
   base::AutoLock auto_lock(lock_);