Replace a few VLOGs with CHECKs inside media::Pipeline.

Printing out a log message and getting stuck in a bad state isn't as helpful as crashing.

Review URL: https://chromiumcodereview.appspot.com/9249027

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@118175 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 17 insertions, 38 deletions

diff --git a/media/base/pipeline.cc b/media/base/pipeline.cc
index 39bd6db..7995796 100644
--- a/media/base/pipeline.cc
+++ b/media/base/pipeline.cc
@@ -96,35 +96,21 @@ void Pipeline::Init(const PipelineStatusCB& ended_callback,
   network_callback_ = network_callback;
 }
 
-// Creates the PipelineInternal and calls it's start method.
-bool Pipeline::Start(scoped_ptr<FilterCollection> collection,
+void Pipeline::Start(scoped_ptr<FilterCollection> collection,
                      const std::string& url,
                      const PipelineStatusCB& start_callback) {
   base::AutoLock auto_lock(lock_);
+  CHECK(!running_) << "Media pipeline is already running";
 
-  if (running_) {
-    VLOG(1) << "Media pipeline is already running";
-    return false;
-  }
-
-  if (collection->IsEmpty()) {
-    return false;
-  }
-
-  // Kick off initialization!
   running_ = true;
   message_loop_->PostTask(FROM_HERE, base::Bind(
       &Pipeline::StartTask, this, base::Passed(&collection),
       url, start_callback));
-  return true;
 }
 
 void Pipeline::Stop(const PipelineStatusCB& stop_callback) {
   base::AutoLock auto_lock(lock_);
-  if (!running_) {
-    VLOG(1) << "Media pipeline has already stopped";
-    return;
-  }
+  CHECK(running_) << "Media pipeline isn't running";
 
   if (video_decoder_) {
     video_decoder_->PrepareForShutdownHack();
@@ -139,10 +125,7 @@ void Pipeline::Stop(const PipelineStatusCB& stop_callback) {
 void Pipeline::Seek(base::TimeDelta time,
                     const PipelineStatusCB& seek_callback) {
   base::AutoLock auto_lock(lock_);
-  if (!running_) {
-    VLOG(1) << "Media pipeline must be running";
-    return;
-  }
+  CHECK(running_) << "Media pipeline isn't running";
 
   download_rate_monitor_.Stop();
 
diff --git a/media/base/pipeline.h b/media/base/pipeline.h
index 1c6e102..b4c8d9f 100644
--- a/media/base/pipeline.h
+++ b/media/base/pipeline.h
@@ -140,21 +140,21 @@ class MEDIA_EXPORT Pipeline
             const NetworkEventCB& network_callback);
 
   // Build a pipeline to render the given URL using the given filter collection
-  // to construct a filter chain.  Returns true if successful, false otherwise
-  // (i.e., pipeline already started).  Note that a return value of true
-  // only indicates that the initialization process has started successfully.
+  // to construct a filter chain.
+  //
   // Pipeline initialization is an inherently asynchronous process.  Clients can
-  // either poll the IsInitialized() method (discouraged) or use the
-  // |start_callback| as described below.
+  // either poll the IsInitialized() method (discouraged) or optionally pass in
+  // |start_callback|, which will be executed when initialization completes.
+  //
+  // It is an error to call this method after the pipeline has already started.
   //
-  // This method is asynchronous and can execute a callback when completed.
-  // If the caller provides a |start_callback|, it will be called when the
-  // pipeline initialization completes.
-  bool Start(scoped_ptr<FilterCollection> filter_collection,
+  // TODO(scherkus): remove IsInitialized() and force clients to use callbacks.
+  void Start(scoped_ptr<FilterCollection> filter_collection,
              const std::string& url,
              const PipelineStatusCB& start_callback);
 
   // Asynchronously stops the pipeline and resets it to an uninitialized state.
+  //
   // If provided, |stop_callback| will be executed when the pipeline has been
   // completely torn down and reset to an uninitialized state.  It is acceptable
   // to call Start() again once the callback has finished executing.
@@ -162,6 +162,8 @@ class MEDIA_EXPORT Pipeline
   // Stop() must be called before destroying the pipeline.  Clients can
   // determine whether Stop() must be called by checking IsRunning().
   //
+  // It is an error to call this method if the pipeline has not started.
+  //
   // TODO(scherkus): ideally clients would destroy the pipeline after calling
   // Stop() and create a new pipeline as needed.
   void Stop(const PipelineStatusCB& stop_callback);
@@ -171,6 +173,8 @@ class MEDIA_EXPORT Pipeline
   //
   // Clients are expected to call GetCurrentTime() to check whether the seek
   // succeeded.
+  //
+  // It is an error to call this method if the pipeline has not started.
   void Seek(base::TimeDelta time, const PipelineStatusCB& seek_callback);
 
   // Returns true if the pipeline has been started via Start().  If IsRunning()
diff --git a/media/base/pipeline_unittest.cc b/media/base/pipeline_unittest.cc
index 58f028f..9980e2f 100644
--- a/media/base/pipeline_unittest.cc
+++ b/media/base/pipeline_unittest.cc
@@ -281,14 +281,6 @@ class PipelineTest : public ::testing::Test {
 TEST_F(PipelineTest, NotStarted) {
   const base::TimeDelta kZero;
 
-  // StrictMock<> will ensure these never get called, and valgrind will
-  // make sure the callbacks are instantly deleted.
-  pipeline_->Stop(base::Bind(&CallbackHelper::OnStop,
-                             base::Unretained(&callbacks_)));
-  pipeline_->Seek(kZero,
-                  base::Bind(&CallbackHelper::OnSeek,
-                             base::Unretained(&callbacks_)));
-
   EXPECT_FALSE(pipeline_->IsRunning());
   EXPECT_FALSE(pipeline_->IsInitialized());
   EXPECT_FALSE(pipeline_->HasAudio());