Removed the bool parameter from PipelineCallback and cleaned up WebMediaPlayerImpl::Proxy.

This forces clients to check Pipeline::GetError() instead of using a simple true/false check for success.  Also the bool parameter wasn't being used for Seek() and Stop() callbacks, further hinting at its removal.

BUG=16009
TEST=media_unittests pass, layout tests pass

Review URL: http://codereview.chromium.org/149584

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

3 files changed, 28 insertions, 35 deletions

diff --git a/media/base/pipeline.h b/media/base/pipeline.h
index d1f5d90..d6f6241 100644
--- a/media/base/pipeline.h
+++ b/media/base/pipeline.h
@@ -44,11 +44,9 @@ enum PipelineError {
   DEMUXER_ERROR_COULD_NOT_CREATE_THREAD,
 };
 
-// Client-provided callbacks for various pipeline operations.
-//
-// TODO(scherkus): consider returning a PipelineError instead of a bool, or
-// perhaps a client callback interface.
-typedef Callback1<bool>::Type PipelineCallback;
+// Client-provided callbacks for various pipeline operations.  Clients should
+// inspect the Pipeline for errors.
+typedef Callback0::Type PipelineCallback;
 
 class Pipeline {
  public:
@@ -62,10 +60,8 @@ class Pipeline {
   //
   // 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.  If successful, the callback's bool
-  // parameter will be true.  If the callback is called with false, then the
-  // client can use GetError() to obtain more information about the reason
-  // initialization failed.
+  // pipeline initialization completes.  Clients are expected to call GetError()
+  // to check whether initialization succeeded.
   virtual bool Start(FilterFactory* filter_factory,
                      const std::string& url,
                      PipelineCallback* start_callback) = 0;
@@ -84,8 +80,8 @@ class Pipeline {
 
   // Attempt to seek to the position specified by time.  |seek_callback| will be
   // executed when the all filters in the pipeline have processed the seek.
-  // The callback will return true if the seek was carried out, false otherwise
-  // (i.e., streaming media).
+  //
+  // Clients are expected to call GetTime() to check whether the seek succeeded.
   virtual void Seek(base::TimeDelta time, PipelineCallback* seek_callback) = 0;
 
   // Returns true if the pipeline has been started via Start().  If IsRunning()
diff --git a/media/base/pipeline_impl.cc b/media/base/pipeline_impl.cc
index eeeeda6..f021ad3 100644
--- a/media/base/pipeline_impl.cc
+++ b/media/base/pipeline_impl.cc
@@ -377,9 +377,6 @@ void PipelineInternal::StartTask(FilterFactory* filter_factory,
 // FilterHost's InitializationComplete() method, the pipeline will update its
 // state to kStarted and |init_callback_|, will be executed.
 //
-// If initialization fails, the client's callback will still be called, but
-// the bool parameter passed to it will be false.
-//
 // TODO(hclam): InitializeTask() is now starting the pipeline asynchronously. It
 // works like a big state change table. If we no longer need to start filters
 // in order, we need to get rid of all the state change.
@@ -454,7 +451,7 @@ void PipelineInternal::InitializeTask() {
     state_ = kStarted;
     filter_factory_ = NULL;
     if (start_callback_.get()) {
-      start_callback_->Run(true);
+      start_callback_->Run();
       start_callback_.reset();
     }
   }
@@ -490,7 +487,7 @@ void PipelineInternal::StopTask(PipelineCallback* stop_callback) {
 
   // Notify the client that stopping has finished.
   if (stop_callback_.get()) {
-    stop_callback_->Run(true);
+    stop_callback_->Run();
     stop_callback_.reset();
   }
 }
@@ -509,7 +506,7 @@ void PipelineInternal::ErrorTask(PipelineError error) {
 
   // Notify the client that starting did not complete, if necessary.
   if (IsPipelineInitializing() && start_callback_.get()) {
-    start_callback_->Run(false);
+    start_callback_->Run();
   }
   start_callback_.reset();
   filter_factory_ = NULL;
@@ -566,7 +563,7 @@ void PipelineInternal::SeekTask(base::TimeDelta time,
   // frames/packets.
   SetTime(time);
   if (seek_callback_.get()) {
-    seek_callback_->Run(true);
+    seek_callback_->Run();
     seek_callback_.reset();
   }
 }
diff --git a/media/base/pipeline_impl_unittest.cc b/media/base/pipeline_impl_unittest.cc
index 12cb40c..b673cfb 100644
--- a/media/base/pipeline_impl_unittest.cc
+++ b/media/base/pipeline_impl_unittest.cc
@@ -27,9 +27,9 @@ class CallbackHelper {
   CallbackHelper() {}
   virtual ~CallbackHelper() {}
 
-  MOCK_METHOD1(OnStart, void(bool result));
-  MOCK_METHOD1(OnSeek, void(bool result));
-  MOCK_METHOD1(OnStop, void(bool result));
+  MOCK_METHOD0(OnStart, void());
+  MOCK_METHOD0(OnSeek, void());
+  MOCK_METHOD0(OnStop, void());
 
  private:
   DISALLOW_COPY_AND_ASSIGN(CallbackHelper);
@@ -54,7 +54,7 @@ class PipelineImplTest : public ::testing::Test {
     }
 
     // Expect a stop callback if we were started.
-    EXPECT_CALL(callbacks_, OnStop(true));
+    EXPECT_CALL(callbacks_, OnStop());
     pipeline_.Stop(NewCallback(reinterpret_cast<CallbackHelper*>(&callbacks_),
                                &CallbackHelper::OnStop));
     message_loop_.RunAllPending();
@@ -127,10 +127,10 @@ class PipelineImplTest : public ::testing::Test {
   }
 
   // Sets up expectations on the callback and initializes the pipeline.  Called
-  // afters tests have set expectations any filters they wish to use.
-  void InitializePipeline(bool callback_result) {
+  // after tests have set expectations any filters they wish to use.
+  void InitializePipeline() {
     // Expect an initialization callback.
-    EXPECT_CALL(callbacks_, OnStart(callback_result));
+    EXPECT_CALL(callbacks_, OnStart());
     pipeline_.Start(mocks_, "",
                     NewCallback(reinterpret_cast<CallbackHelper*>(&callbacks_),
                                 &CallbackHelper::OnStart));
@@ -220,13 +220,13 @@ TEST_F(PipelineImplTest, NeverInitializes) {
   // verify that nothing has been called, then set our expectation for the call
   // made during tear down.
   Mock::VerifyAndClear(&callbacks_);
-  EXPECT_CALL(callbacks_, OnStart(false));
+  EXPECT_CALL(callbacks_, OnStart());
 }
 
 TEST_F(PipelineImplTest, RequiredFilterMissing) {
   mocks_->set_creation_successful(false);
 
-  InitializePipeline(false);
+  InitializePipeline();
   EXPECT_FALSE(pipeline_.IsInitialized());
   EXPECT_EQ(PIPELINE_ERROR_REQUIRED_FILTER_MISSING,
             pipeline_.GetError());
@@ -239,7 +239,7 @@ TEST_F(PipelineImplTest, URLNotFound) {
                       Return(false)));
   EXPECT_CALL(*mocks_->data_source(), Stop());
 
-  InitializePipeline(false);
+  InitializePipeline();
   EXPECT_FALSE(pipeline_.IsInitialized());
   EXPECT_EQ(PIPELINE_ERROR_URL_NOT_FOUND, pipeline_.GetError());
 }
@@ -259,7 +259,7 @@ TEST_F(PipelineImplTest, NoStreams) {
       .WillRepeatedly(Return(0));
   EXPECT_CALL(*mocks_->demuxer(), Stop());
 
-  InitializePipeline(false);
+  InitializePipeline();
   EXPECT_FALSE(pipeline_.IsInitialized());
   EXPECT_EQ(PIPELINE_ERROR_COULD_NOT_RENDER, pipeline_.GetError());
 }
@@ -275,7 +275,7 @@ TEST_F(PipelineImplTest, AudioStream) {
   InitializeAudioDecoder(stream);
   InitializeAudioRenderer();
 
-  InitializePipeline(true);
+  InitializePipeline();
   EXPECT_TRUE(pipeline_.IsInitialized());
   EXPECT_EQ(PIPELINE_OK, pipeline_.GetError());
   EXPECT_TRUE(pipeline_.IsRendered(media::mime_type::kMajorTypeAudio));
@@ -293,7 +293,7 @@ TEST_F(PipelineImplTest, VideoStream) {
   InitializeVideoDecoder(stream);
   InitializeVideoRenderer();
 
-  InitializePipeline(true);
+  InitializePipeline();
   EXPECT_TRUE(pipeline_.IsInitialized());
   EXPECT_EQ(PIPELINE_OK, pipeline_.GetError());
   EXPECT_FALSE(pipeline_.IsRendered(media::mime_type::kMajorTypeAudio));
@@ -316,7 +316,7 @@ TEST_F(PipelineImplTest, AudioVideoStream) {
   InitializeVideoDecoder(video_stream);
   InitializeVideoRenderer();
 
-  InitializePipeline(true);
+  InitializePipeline();
   EXPECT_TRUE(pipeline_.IsInitialized());
   EXPECT_EQ(PIPELINE_OK, pipeline_.GetError());
   EXPECT_TRUE(pipeline_.IsRendered(media::mime_type::kMajorTypeAudio));
@@ -349,10 +349,10 @@ TEST_F(PipelineImplTest, Seek) {
   EXPECT_CALL(*mocks_->video_renderer(), Seek(expected));
 
   // We expect a successful seek callback.
-  EXPECT_CALL(callbacks_, OnSeek(true));
+  EXPECT_CALL(callbacks_, OnSeek());
 
   // Initialize then seek!
-  InitializePipeline(true);
+  InitializePipeline();
   pipeline_.Seek(expected,
                  NewCallback(reinterpret_cast<CallbackHelper*>(&callbacks_),
                              &CallbackHelper::OnSeek));
@@ -375,7 +375,7 @@ TEST_F(PipelineImplTest, SetVolume) {
   EXPECT_CALL(*mocks_->audio_renderer(), SetVolume(expected));
 
   // Initialize then set volume!
-  InitializePipeline(true);
+  InitializePipeline();
   pipeline_.SetVolume(expected);
 }