Collapse HTTP and non-HTTP codepaths into single methods.

webkit/media/buffered_data_source.cc

 // Copyright (c) 2012 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/media/buffered_data_source.h"
 
 #include "base/bind.h"
 #include "base/message_loop.h"
 #include "media/base/media_log.h"
 #include "net/base/net_errors.h"
 
 using WebKit::WebFrame;
 
 namespace {
 
 // BufferedDataSource has an intermediate buffer, this value governs the initial
 // size of that buffer. It is set to 32KB because this is a typical read size
 // of FFmpeg.
 const int kInitialReadBufferSize = 32768;
 
 // Number of cache misses we allow for a single Read() before signaling an
 // error.
 const int kNumCacheMissRetries = 3;
 
 }  // namespace
 
 namespace webkit_media {
 
-// Non-HTTP resources are assumed to be fully loaded so we ignore any
-// loading/progress related callbacks.
-static void NonHttpLoadingStateChangedCallback(
-    BufferedResourceLoader::LoadingState) {
-}
-static void NonHttpProgressCallback(int64) {}
-
 BufferedDataSource::BufferedDataSource(
     MessageLoop* render_loop,
     WebFrame* frame,
     media::MediaLog* media_log,
     const DownloadingCB& downloading_cb)
     : cors_mode_(BufferedResourceLoader::kUnspecified),
       total_bytes_(kPositionNotSpecified),
       assume_fully_buffered_(false),
       streaming_(false),
       frame_(frame),
@@ skipping 56 matching lines @@
   DCHECK(!loader_.get());
   url_ = url;
   cors_mode_ = cors_mode;
 
   initialize_cb_ = initialize_cb;
 
   if (url_.SchemeIs(kHttpScheme) || url_.SchemeIs(kHttpsScheme)) {
     // 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::HttpLoadingStateChangedCallback, this),
-        base::Bind(&BufferedDataSource::HttpProgressCallback, this),
-        frame_);
-    return;
+  } else {
+    // 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));
+    assume_fully_buffered_ = true;

[scherkus (not reviewing), 2012/07/11 00:25:44]

NOTE!!!

my newly minted tests revealed that since StartCallback() is executed after
LoadingStateChangedCallback(), we would report that we are downloading in the
non-HTTP case

I've isolated the HTTP-vs-non-HTTP checks to the single one here and replaced
subsequent ones with assume_fully_buffered_

[Ami GONE FROM CHROMIUM, 2012/07/11 00:56:41]

I mildly dislike that assume_fully_buffered_ === !is_http.
I don't know how to improve this.

[scherkus (not reviewing), 2012/07/11 18:39:02]

That's true today but perhaps one day ftp:// (and other network-y protocols)
will not get included w/ assume_fully_buffered_ but it requires more
investigation

   }
 
-  // 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(&NonHttpLoadingStateChangedCallback),
-      base::Bind(&NonHttpProgressCallback),
+      base::Bind(&BufferedDataSource::StartCallback, this),
+      base::Bind(&BufferedDataSource::LoadingStateChangedCallback, this),
+      base::Bind(&BufferedDataSource::ProgressCallback, this),
       frame_);
 }
 
 void BufferedDataSource::SetPreload(Preload preload) {
   DCHECK(MessageLoop::current() == render_loop_);
   preload_ = preload;
 }
 
 bool BufferedDataSource::HasSingleOrigin() {
   DCHECK(MessageLoop::current() == render_loop_);
@@ skipping 132 matching lines @@
     return;
 
   {
     // If there's no outstanding read then return early.
     base::AutoLock auto_lock(lock_);
     if (read_cb_.is_null())
       return;
   }
 
   // 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::HttpLoadingStateChangedCallback, this),
-        base::Bind(&BufferedDataSource::HttpProgressCallback, this),
-        frame_);
-  } else {
-    loader_->Start(
-        base::Bind(&BufferedDataSource::PartialReadStartCallback, this),
-        base::Bind(&NonHttpLoadingStateChangedCallback),
-        base::Bind(&NonHttpProgressCallback),
-        frame_);
-  }
+  loader_.reset(CreateResourceLoader(last_read_start_, kPositionNotSpecified));
+  loader_->Start(
+      base::Bind(&BufferedDataSource::PartialReadStartCallback, this),
+      base::Bind(&BufferedDataSource::LoadingStateChangedCallback, this),
+      base::Bind(&BufferedDataSource::ProgressCallback, this),
+      frame_);
 }
 
 void BufferedDataSource::SetPlaybackRateTask(float playback_rate) {
   DCHECK(MessageLoop::current() == render_loop_);
   DCHECK(loader_.get());
 
   if (playback_rate != 0)
     media_has_played_ = true;
 
   playback_rate_ = playback_rate;
@@ skipping 62 matching lines @@
   DCHECK(MessageLoop::current() == render_loop_);
   DCHECK(!initialize_cb_.is_null());
   lock_.AssertAcquired();
 
   initialize_cb_.Run(status);
   initialize_cb_.Reset();
 }
 
 /////////////////////////////////////////////////////////////////////////////
 // BufferedResourceLoader callback methods.
-void BufferedDataSource::HttpInitialStartCallback(
+void BufferedDataSource::StartCallback(
     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;
   }
 
-  bool success = status == BufferedResourceLoader::kOk;
+  // All responses must be successful. Resources that are assumed to be fully
+  // buffered must have a known content length.
+  bool success = status == BufferedResourceLoader::kOk &&
+      (!assume_fully_buffered_ ||
+       loader_->instance_size() != kPositionNotSpecified);
+
   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();
+    streaming_ = !assume_fully_buffered_ &&
+        (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.
+  // 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_)
       return;
 
     if (!success) {
-      DoneInitialization_Locked(media::PIPELINE_ERROR_NETWORK);
-      return;
-    }
-
-    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;
-    assume_fully_buffered_ = true;
-  } else {
-    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_ || initialize_cb_.is_null())
-      return;
-
-    if (!success) {
       DoneInitialization_Locked(media::PIPELINE_ERROR_NETWORK);
       return;
     }
 
     UpdateHostState_Locked();
     DoneInitialization_Locked(media::PIPELINE_OK);
   }
 }
 
 void BufferedDataSource::PartialReadStartCallback(
     BufferedResourceLoader::Status status) {
   DCHECK(MessageLoop::current() == render_loop_);
   DCHECK(loader_.get());
 
   if (status == BufferedResourceLoader::kOk) {
     // Once the request has started successfully, we can proceed with
     // reading from it.
     ReadInternal();
     return;
   }
 
   // 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;
   DoneRead_Locked(kReadError);
 }
 
 void BufferedDataSource::ReadCallback(
     BufferedResourceLoader::Status status,
     int bytes_read) {
   DCHECK(MessageLoop::current() == render_loop_);
@@ skipping 30 matching lines @@
 
     if (host() && total_bytes_ != kPositionNotSpecified) {
       host()->SetTotalBytes(total_bytes_);
       host()->AddBufferedByteRange(loader_->first_byte_position(),
                                    total_bytes_);
     }
   }
   DoneRead_Locked(bytes_read);
 }
 
-void BufferedDataSource::HttpLoadingStateChangedCallback(
+void BufferedDataSource::LoadingStateChangedCallback(
     BufferedResourceLoader::LoadingState state) {
   DCHECK(MessageLoop::current() == render_loop_);
-  DCHECK(url_.SchemeIs(kHttpScheme) || url_.SchemeIs(kHttpsScheme));
+
+  if (assume_fully_buffered_)
+    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 we considering changing DownloadingCB to also
     // propagate loading state. For example 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;
   }
 
   downloading_cb_.Run(is_downloading_data);
 }
 
-void BufferedDataSource::HttpProgressCallback(int64 position) {
+void BufferedDataSource::ProgressCallback(int64 position) {
   DCHECK(MessageLoop::current() == render_loop_);
-  DCHECK(url_.SchemeIs(kHttpScheme) || url_.SchemeIs(kHttpsScheme));
+
+  if (assume_fully_buffered_)
+    return;
 
   // 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() && position > last_read_start_)
     host()->AddBufferedByteRange(last_read_start_, position);
 }
 
 void BufferedDataSource::UpdateHostState_Locked() {
   lock_.AssertAcquired();
 
   if (!host())
     return;
 
   if (total_bytes_ != kPositionNotSpecified) {
     host()->SetTotalBytes(total_bytes_);
 
     if (assume_fully_buffered_)
       host()->AddBufferedByteRange(0, total_bytes_);
   }
 }
 
 }  // namespace webkit_media