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 webkit_media {
 
 // 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.
 static const int kInitialReadBufferSize = 32768;
 
 // Number of cache misses we allow for a single Read() before signalling an
 // 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,
     media::MediaLog* media_log)
     : total_bytes_(kPositionNotSpecified),
       buffered_bytes_(0),
       streaming_(false),
       frame_(frame),
       loader_(NULL),
       is_downloading_data_(false),
@@ skipping 51 matching lines @@
   DCHECK(!initialize_cb.is_null());
   DCHECK(!loader_.get());
   url_ = url;
 
   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::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_);
 }
 
 void BufferedDataSource::SetPreload(Preload preload) {
   DCHECK(MessageLoop::current() == render_loop_);
   preload_ = preload;
 }
 
 bool BufferedDataSource::HasSingleOrigin() {
   DCHECK(MessageLoop::current() == render_loop_);
@@ skipping 128 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::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) {
   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;
-  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.
+    // 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

+    }
+  }
+
+  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_)
       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;
-    buffered_bytes_ = total_bytes_;
-  } 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 29 matching lines @@
     total_bytes_ = loader_->instance_size();
 
     if (host() && total_bytes_ != kPositionNotSpecified) {
       host()->SetTotalBytes(total_bytes_);
       host()->AddBufferedByteRange(last_read_start_, total_bytes_);
     }
   }
   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.
   base::AutoLock auto_lock(lock_);
   if (stop_signal_received_)
     return;
 
   bool is_downloading_data;
   switch (state) {
     case BufferedResourceLoader::kLoading:
@@ skipping 13 matching lines @@
       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) {
+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_);
   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 (buffered_bytes_ > last_read_start_)
     host()->AddBufferedByteRange(last_read_start_, buffered_bytes_);
 }
 
 }  // namespace webkit_media