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

5 files changed, 101 insertions, 141 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);
 }
 
diff --git a/webkit/glue/webmediaplayer_impl.cc b/webkit/glue/webmediaplayer_impl.cc
index 3bd5138..051e7d4 100644
--- a/webkit/glue/webmediaplayer_impl.cc
+++ b/webkit/glue/webmediaplayer_impl.cc
@@ -56,58 +56,6 @@ void WebMediaPlayerImpl::Proxy::Repaint() {
   }
 }
 
-void WebMediaPlayerImpl::Proxy::TimeChanged() {
-  render_loop_->PostTask(FROM_HERE,
-      NewRunnableMethod(this, &WebMediaPlayerImpl::Proxy::TimeChangedTask));
-}
-
-void WebMediaPlayerImpl::Proxy::NetworkStateChanged(
-    WebKit::WebMediaPlayer::NetworkState state) {
-  render_loop_->PostTask(FROM_HERE,
-      NewRunnableMethod(this,
-                        &WebMediaPlayerImpl::Proxy::NetworkStateChangedTask,
-                        state));
-}
-
-void WebMediaPlayerImpl::Proxy::ReadyStateChanged(
-    WebKit::WebMediaPlayer::ReadyState state) {
-  render_loop_->PostTask(FROM_HERE,
-      NewRunnableMethod(this,
-                        &WebMediaPlayerImpl::Proxy::ReadyStateChangedTask,
-                        state));
-}
-
-void WebMediaPlayerImpl::Proxy::RepaintTask() {
-  DCHECK(MessageLoop::current() == render_loop_);
-  {
-    AutoLock auto_lock(lock_);
-    --outstanding_repaints_;
-    DCHECK_GE(outstanding_repaints_, 0);
-  }
-  if (webmediaplayer_)
-    webmediaplayer_->Repaint();
-}
-
-void WebMediaPlayerImpl::Proxy::TimeChangedTask() {
-  DCHECK(MessageLoop::current() == render_loop_);
-  if (webmediaplayer_)
-    webmediaplayer_->TimeChanged();
-}
-
-void WebMediaPlayerImpl::Proxy::NetworkStateChangedTask(
-    WebKit::WebMediaPlayer::NetworkState state) {
-  DCHECK(MessageLoop::current() == render_loop_);
-  if (webmediaplayer_)
-    webmediaplayer_->SetNetworkState(state);
-}
-
-void WebMediaPlayerImpl::Proxy::ReadyStateChangedTask(
-    WebKit::WebMediaPlayer::ReadyState state) {
-  DCHECK(MessageLoop::current() == render_loop_);
-  if (webmediaplayer_)
-    webmediaplayer_->SetReadyState(state);
-}
-
 void WebMediaPlayerImpl::Proxy::SetVideoRenderer(
     VideoRendererImpl* video_renderer) {
   video_renderer_ = video_renderer;
@@ -134,27 +82,40 @@ void WebMediaPlayerImpl::Proxy::Detach() {
   video_renderer_ = NULL;
 }
 
-void WebMediaPlayerImpl::Proxy::PipelineInitializationCallback(bool success) {
-  if (success) {
-    // Since we have initialized the pipeline, say we have everything.
-    // TODO(hclam): change this to report the correct status. Should also post
-    // a task to call to |webmediaplayer_|.
-    ReadyStateChanged(WebKit::WebMediaPlayer::HaveMetadata);
-    ReadyStateChanged(WebKit::WebMediaPlayer::HaveEnoughData);
-    NetworkStateChanged(WebKit::WebMediaPlayer::Loaded);
-  } else {
-    // TODO(hclam): should use pipeline_->GetError() to determine the state
-    // properly and reports error using MediaError.
-    // WebKit uses FormatError to indicate an error for bogus URL or bad file.
-    // Since we are at the initialization stage we can safely treat every error
-    // as format error. Should post a task to call to |webmediaplayer_|.
-    NetworkStateChanged(WebKit::WebMediaPlayer::FormatError);
+void WebMediaPlayerImpl::Proxy::PipelineInitializationCallback() {
+  render_loop_->PostTask(FROM_HERE, NewRunnableMethod(this,
+      &WebMediaPlayerImpl::Proxy::PipelineInitializationTask));
+}
+
+void WebMediaPlayerImpl::Proxy::PipelineSeekCallback() {
+  render_loop_->PostTask(FROM_HERE, NewRunnableMethod(this,
+      &WebMediaPlayerImpl::Proxy::PipelineSeekTask));
+}
+
+void WebMediaPlayerImpl::Proxy::RepaintTask() {
+  DCHECK(MessageLoop::current() == render_loop_);
+  {
+    AutoLock auto_lock(lock_);
+    --outstanding_repaints_;
+    DCHECK_GE(outstanding_repaints_, 0);
+  }
+  if (webmediaplayer_) {
+    webmediaplayer_->Repaint();
   }
 }
 
-void WebMediaPlayerImpl::Proxy::PipelineSeekCallback(bool success) {
-  if (success)
-    TimeChanged();
+void WebMediaPlayerImpl::Proxy::PipelineInitializationTask() {
+  DCHECK(MessageLoop::current() == render_loop_);
+  if (webmediaplayer_) {
+    webmediaplayer_->OnPipelineInitialize();
+  }
+}
+
+void WebMediaPlayerImpl::Proxy::PipelineSeekTask() {
+  DCHECK(MessageLoop::current() == render_loop_);
+  if (webmediaplayer_) {
+    webmediaplayer_->OnPipelineSeek();
+  }
 }
 
 /////////////////////////////////////////////////////////////////////////////
@@ -393,9 +354,29 @@ void WebMediaPlayerImpl::Repaint() {
   GetClient()->repaint();
 }
 
-void WebMediaPlayerImpl::TimeChanged() {
+void WebMediaPlayerImpl::OnPipelineInitialize() {
+  DCHECK(MessageLoop::current() == main_loop_);
+  if (pipeline_->GetError() == media::PIPELINE_OK) {
+    // Since we have initialized the pipeline, say we have everything.
+    // TODO(hclam): change this to report the correct status.
+    SetReadyState(WebKit::WebMediaPlayer::HaveMetadata);
+    SetReadyState(WebKit::WebMediaPlayer::HaveEnoughData);
+    SetNetworkState(WebKit::WebMediaPlayer::Loaded);
+  } else {
+    // TODO(hclam): should use pipeline_->GetError() to determine the state
+    // properly and reports error using MediaError.
+    // WebKit uses FormatError to indicate an error for bogus URL or bad file.
+    // Since we are at the initialization stage we can safely treat every error
+    // as format error. Should post a task to call to |webmediaplayer_|.
+    SetNetworkState(WebKit::WebMediaPlayer::FormatError);
+  }
+}
+
+void WebMediaPlayerImpl::OnPipelineSeek() {
   DCHECK(MessageLoop::current() == main_loop_);
-  GetClient()->timeChanged();
+  if (pipeline_->GetError() == media::PIPELINE_OK) {
+    GetClient()->timeChanged();
+  }
 }
 
 void WebMediaPlayerImpl::SetNetworkState(
diff --git a/webkit/glue/webmediaplayer_impl.h b/webkit/glue/webmediaplayer_impl.h
index c13d30f..9e9522e 100644
--- a/webkit/glue/webmediaplayer_impl.h
+++ b/webkit/glue/webmediaplayer_impl.h
@@ -93,46 +93,29 @@ class WebMediaPlayerImpl : public WebKit::WebMediaPlayer,
           WebMediaPlayerImpl* webmediaplayer);
     virtual ~Proxy();
 
-    // Fire a repaint event to WebKit.
+    // Public methods called from the video renderer.
     void Repaint();
+    void SetVideoRenderer(VideoRendererImpl* video_renderer);
 
-    // Report to WebKit that time has changed.
-    void TimeChanged();
-
-    // Report to WebKit that network state has changed.
-    void NetworkStateChanged(WebKit::WebMediaPlayer::NetworkState state);
+    // Public methods called from WebMediaPlayerImpl.
+    void Paint(skia::PlatformCanvas* canvas, const gfx::Rect& dest_rect);
+    void SetSize(const gfx::Rect& rect);
+    void Detach();
 
-    // Report the WebKit that ready state has changed.
-    void ReadyStateChanged(WebKit::WebMediaPlayer::ReadyState state);
-
-    // Public methods to be called from video renderer.
-    void SetVideoRenderer(VideoRendererImpl* video_renderer);
+    // Public methods called from the pipeline via callback issued by
+    // WebMediaPlayerImpl.
+    void PipelineInitializationCallback();
+    void PipelineSeekCallback();
 
    private:
-    friend class WebMediaPlayerImpl;
-
     // Invoke |webmediaplayer_| to perform a repaint.
     void RepaintTask();
 
-    // Invoke |webmediaplayer_| to notify a time change event.
-    void TimeChangedTask();
-
-    // Saves the internal network state and notify WebKit to pick up the change.
-    void NetworkStateChangedTask(WebKit::WebMediaPlayer::NetworkState state);
+    // Notify |webmediaplayer_| that initialization has finished.
+    void PipelineInitializationTask();
 
-    // Saves the internal ready state and notify WebKit to pick the change.
-    void ReadyStateChangedTask(WebKit::WebMediaPlayer::ReadyState state);
-
-    void Paint(skia::PlatformCanvas* canvas, const gfx::Rect& dest_rect);
-
-    void SetSize(const gfx::Rect& rect);
-
-    // Detach from |webmediaplayer_|.
-    void Detach();
-
-    void PipelineInitializationCallback(bool success);
-
-    void PipelineSeekCallback(bool success);
+    // Notify |webmediaplayer_| that a seek has finished.
+    void PipelineSeekTask();
 
     // The render message loop where WebKit lives.
     MessageLoop* render_loop_;
@@ -224,13 +207,16 @@ class WebMediaPlayerImpl : public WebKit::WebMediaPlayer,
 
   void Repaint();
 
-  void TimeChanged();
+  void OnPipelineInitialize();
 
-  void SetNetworkState(WebKit::WebMediaPlayer::NetworkState state);
+  void OnPipelineSeek();
 
+ private:
+  // Helpers that set the network/ready state and notifies the client if
+  // they've changed.
+  void SetNetworkState(WebKit::WebMediaPlayer::NetworkState state);
   void SetReadyState(WebKit::WebMediaPlayer::ReadyState state);
 
- private:
   // Destroy resources held.
   void Destroy();