PipelineError is dead. Long live PipelineStatus!

PipelineError was a poor naming choice because most of the time variables of
that type held the value PIPELINE_OK meaning there was in fact no error.

Replaced the idiom of [0-ary callback + GetError()] with
[1-ary callback taking PipelineStatus argument] which makes the Pipeline API
cleaner and less error-prone.  Before, consumers of the API had to make sure to
call GetError() at the top of each callback, or risk missing state transitions
in the pipeline.  Now each callback gets an explicit parameter holding the
pipeline status at the moment the callback was invoked so failing to handle
error conditions should be more apparent in the code.

BUG=none
TEST=media_unittests + trybots: {mac,linux,win}{_layout,}, linux_rel, linux_clang (all pass or fail with unrelated errors)

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

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

7 files changed, 77 insertions, 67 deletions

diff --git a/webkit/glue/media/buffered_data_source.cc b/webkit/glue/media/buffered_data_source.cc
index 135c49e..2622255 100644
--- a/webkit/glue/media/buffered_data_source.cc
+++ b/webkit/glue/media/buffered_data_source.cc
@@ -412,14 +412,15 @@ void BufferedDataSource::DoneRead_Locked(int error) {
   read_buffer_ = 0;
 }
 
-void BufferedDataSource::DoneInitialization_Locked(media::PipelineError error) {
+void BufferedDataSource::DoneInitialization_Locked(
+    media::PipelineStatus status) {
   DCHECK(MessageLoop::current() == render_loop_);
   DCHECK(initialize_callback_.get());
   lock_.AssertAcquired();
 
   scoped_ptr<media::PipelineStatusCallback> initialize_callback(
       initialize_callback_.release());
-  initialize_callback->Run(error);
+  initialize_callback->Run(status);
 }
 
 /////////////////////////////////////////////////////////////////////////////
diff --git a/webkit/glue/media/buffered_data_source.h b/webkit/glue/media/buffered_data_source.h
index 9e043b0..5ab3035 100644
--- a/webkit/glue/media/buffered_data_source.h
+++ b/webkit/glue/media/buffered_data_source.h
@@ -98,7 +98,7 @@ class BufferedDataSource : public WebDataSource {
   void DoneRead_Locked(int error);
 
   // Calls |initialize_callback_| and reset it.
-  void DoneInitialization_Locked(media::PipelineError error);
+  void DoneInitialization_Locked(media::PipelineStatus status);
 
   // Callback method for |loader_| if URL for the resource requested is using
   // HTTP protocol. This method is called when response for initial request is
diff --git a/webkit/glue/media/buffered_data_source_unittest.cc b/webkit/glue/media/buffered_data_source_unittest.cc
index 9763678..dd5c891 100644
--- a/webkit/glue/media/buffered_data_source_unittest.cc
+++ b/webkit/glue/media/buffered_data_source_unittest.cc
@@ -168,7 +168,7 @@ class BufferedDataSourceTest : public testing::Test {
         .WillByDefault(Return(partial_response));
     ON_CALL(*loader_, url())
         .WillByDefault(ReturnRef(gurl_));
-    media::PipelineError expected_init_error = media::PIPELINE_OK;
+    media::PipelineStatus expected_init_status = media::PIPELINE_OK;
     if (initialized_ok) {
       // Expected loaded or not.
       EXPECT_CALL(host_, SetLoaded(loaded));
@@ -184,13 +184,13 @@ class BufferedDataSourceTest : public testing::Test {
         EXPECT_CALL(host_, SetStreaming(true));
       }
     } else {
-      expected_init_error = media::PIPELINE_ERROR_NETWORK;
+      expected_init_status = media::PIPELINE_ERROR_NETWORK;
       EXPECT_CALL(*loader_, Stop());
     }
 
     // Actual initialization of the data source.
     data_source_->Initialize(url,
-        media::NewExpectedStatusCallback(expected_init_error));
+        media::NewExpectedStatusCallback(expected_init_status));
     message_loop_->RunAllPending();
 
     if (initialized_ok) {
diff --git a/webkit/glue/media/simple_data_source.cc b/webkit/glue/media/simple_data_source.cc
index ca258d4..186f6d7 100644
--- a/webkit/glue/media/simple_data_source.cc
+++ b/webkit/glue/media/simple_data_source.cc
@@ -321,12 +321,12 @@ void SimpleDataSource::CancelTask() {
 
 void SimpleDataSource::DoneInitialization_Locked(bool success) {
   lock_.AssertAcquired();
-  media::PipelineError error = media::PIPELINE_ERROR_NETWORK;
+  media::PipelineStatus status = media::PIPELINE_ERROR_NETWORK;
   if (success) {
     state_ = INITIALIZED;
 
     UpdateHostState();
-    error = media::PIPELINE_OK;
+    status = media::PIPELINE_OK;
   } else {
     state_ = UNINITIALIZED;
     url_loader_.reset();
@@ -334,7 +334,7 @@ void SimpleDataSource::DoneInitialization_Locked(bool success) {
 
   scoped_ptr<media::PipelineStatusCallback> initialize_callback(
       initialize_callback_.release());
-  initialize_callback->Run(error);
+  initialize_callback->Run(status);
 }
 
 void SimpleDataSource::UpdateHostState() {
diff --git a/webkit/glue/media/web_data_source_factory.cc b/webkit/glue/media/web_data_source_factory.cc
index f9036c7..d98e2da 100644
--- a/webkit/glue/media/web_data_source_factory.cc
+++ b/webkit/glue/media/web_data_source_factory.cc
@@ -21,7 +21,7 @@ class WebDataSourceFactory::BuildRequest
   virtual void DoStart();
 
  private:
-  void InitDone(media::PipelineError error);
+  void InitDone(media::PipelineStatus status);
 
   scoped_refptr<WebDataSource> data_source_;
   WebDataSourceBuildObserverHack* build_observer_;
@@ -84,17 +84,18 @@ void WebDataSourceFactory::BuildRequest::DoStart() {
   data_source_->Initialize(url(), NewCallback(this, &BuildRequest::InitDone));
 }
 
-void WebDataSourceFactory::BuildRequest::InitDone(media::PipelineError error) {
+void WebDataSourceFactory::BuildRequest::InitDone(
+    media::PipelineStatus status) {
   scoped_refptr<WebDataSource> data_source;
 
-  data_source = (error == media::PIPELINE_OK) ? data_source_ : NULL;
+  data_source = (status == media::PIPELINE_OK) ? data_source_ : NULL;
   data_source_ = NULL;
 
   if (build_observer_ && data_source.get()) {
     build_observer_->Run(data_source.get());
   }
 
-  RequestComplete(error, data_source);
+  RequestComplete(status, data_source);
   // Don't do anything after this line. This object is deleted by
   // RequestComplete().
 }
diff --git a/webkit/glue/webmediaplayer_impl.cc b/webkit/glue/webmediaplayer_impl.cc
index 098cd61..61bb454 100644
--- a/webkit/glue/webmediaplayer_impl.cc
+++ b/webkit/glue/webmediaplayer_impl.cc
@@ -33,6 +33,7 @@
 using WebKit::WebCanvas;
 using WebKit::WebRect;
 using WebKit::WebSize;
+using media::PipelineStatus;
 
 namespace {
 
@@ -171,29 +172,31 @@ void WebMediaPlayerImpl::Proxy::Detach() {
   }
 }
 
-void WebMediaPlayerImpl::Proxy::PipelineInitializationCallback() {
-  render_loop_->PostTask(FROM_HERE, NewRunnableMethod(this,
-      &WebMediaPlayerImpl::Proxy::PipelineInitializationTask));
+void WebMediaPlayerImpl::Proxy::PipelineInitializationCallback(
+    PipelineStatus status) {
+  render_loop_->PostTask(FROM_HERE, NewRunnableMethod(
+      this, &WebMediaPlayerImpl::Proxy::PipelineInitializationTask, status));
 }
 
-void WebMediaPlayerImpl::Proxy::PipelineSeekCallback() {
-  render_loop_->PostTask(FROM_HERE, NewRunnableMethod(this,
-      &WebMediaPlayerImpl::Proxy::PipelineSeekTask));
+void WebMediaPlayerImpl::Proxy::PipelineSeekCallback(PipelineStatus status) {
+  render_loop_->PostTask(FROM_HERE, NewRunnableMethod(
+      this, &WebMediaPlayerImpl::Proxy::PipelineSeekTask, status));
 }
 
-void WebMediaPlayerImpl::Proxy::PipelineEndedCallback() {
-  render_loop_->PostTask(FROM_HERE, NewRunnableMethod(this,
-      &WebMediaPlayerImpl::Proxy::PipelineEndedTask));
+void WebMediaPlayerImpl::Proxy::PipelineEndedCallback(PipelineStatus status) {
+  render_loop_->PostTask(FROM_HERE, NewRunnableMethod(
+      this, &WebMediaPlayerImpl::Proxy::PipelineEndedTask, status));
 }
 
-void WebMediaPlayerImpl::Proxy::PipelineErrorCallback() {
-  render_loop_->PostTask(FROM_HERE, NewRunnableMethod(this,
-      &WebMediaPlayerImpl::Proxy::PipelineErrorTask));
+void WebMediaPlayerImpl::Proxy::PipelineErrorCallback(PipelineStatus error) {
+  DCHECK_NE(error, media::PIPELINE_OK);
+  render_loop_->PostTask(FROM_HERE, NewRunnableMethod(
+      this, &WebMediaPlayerImpl::Proxy::PipelineErrorTask, error));
 }
 
-void WebMediaPlayerImpl::Proxy::NetworkEventCallback() {
-  render_loop_->PostTask(FROM_HERE, NewRunnableMethod(this,
-      &WebMediaPlayerImpl::Proxy::NetworkEventTask));
+void WebMediaPlayerImpl::Proxy::NetworkEventCallback(PipelineStatus status) {
+  render_loop_->PostTask(FROM_HERE, NewRunnableMethod(
+      this, &WebMediaPlayerImpl::Proxy::NetworkEventTask, status));
 }
 
 void WebMediaPlayerImpl::Proxy::AddDataSource(WebDataSource* data_source) {
@@ -213,38 +216,39 @@ void WebMediaPlayerImpl::Proxy::RepaintTask() {
   }
 }
 
-void WebMediaPlayerImpl::Proxy::PipelineInitializationTask() {
+void WebMediaPlayerImpl::Proxy::PipelineInitializationTask(
+    PipelineStatus status) {
   DCHECK(MessageLoop::current() == render_loop_);
   if (webmediaplayer_) {
-    webmediaplayer_->OnPipelineInitialize();
+    webmediaplayer_->OnPipelineInitialize(status);
   }
 }
 
-void WebMediaPlayerImpl::Proxy::PipelineSeekTask() {
+void WebMediaPlayerImpl::Proxy::PipelineSeekTask(PipelineStatus status) {
   DCHECK(MessageLoop::current() == render_loop_);
   if (webmediaplayer_) {
-    webmediaplayer_->OnPipelineSeek();
+    webmediaplayer_->OnPipelineSeek(status);
   }
 }
 
-void WebMediaPlayerImpl::Proxy::PipelineEndedTask() {
+void WebMediaPlayerImpl::Proxy::PipelineEndedTask(PipelineStatus status) {
   DCHECK(MessageLoop::current() == render_loop_);
   if (webmediaplayer_) {
-    webmediaplayer_->OnPipelineEnded();
+    webmediaplayer_->OnPipelineEnded(status);
   }
 }
 
-void WebMediaPlayerImpl::Proxy::PipelineErrorTask() {
+void WebMediaPlayerImpl::Proxy::PipelineErrorTask(PipelineStatus error) {
   DCHECK(MessageLoop::current() == render_loop_);
   if (webmediaplayer_) {
-    webmediaplayer_->OnPipelineError();
+    webmediaplayer_->OnPipelineError(error);
   }
 }
 
-void WebMediaPlayerImpl::Proxy::NetworkEventTask() {
+void WebMediaPlayerImpl::Proxy::NetworkEventTask(PipelineStatus status) {
   DCHECK(MessageLoop::current() == render_loop_);
   if (webmediaplayer_) {
-    webmediaplayer_->OnNetworkEvent();
+    webmediaplayer_->OnNetworkEvent(status);
   }
 }
 
@@ -744,9 +748,9 @@ void WebMediaPlayerImpl::Repaint() {
   GetClient()->repaint();
 }
 
-void WebMediaPlayerImpl::OnPipelineInitialize() {
+void WebMediaPlayerImpl::OnPipelineInitialize(PipelineStatus status) {
   DCHECK(MessageLoop::current() == main_loop_);
-  if (pipeline_->GetError() == media::PIPELINE_OK) {
+  if (status == media::PIPELINE_OK) {
     // Only keep one time range starting from 0.
     WebKit::WebTimeRanges new_buffered(static_cast<size_t>(1));
     new_buffered[0].start = 0.0f;
@@ -763,7 +767,7 @@ void WebMediaPlayerImpl::OnPipelineInitialize() {
       SetNetworkState(WebKit::WebMediaPlayer::Loaded);
     }
   } else {
-    // TODO(hclam): should use pipeline_->GetError() to determine the state
+    // TODO(hclam): should use |status| 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
@@ -775,9 +779,9 @@ void WebMediaPlayerImpl::OnPipelineInitialize() {
   Repaint();
 }
 
-void WebMediaPlayerImpl::OnPipelineSeek() {
+void WebMediaPlayerImpl::OnPipelineSeek(PipelineStatus status) {
   DCHECK(MessageLoop::current() == main_loop_);
-  if (pipeline_->GetError() == media::PIPELINE_OK) {
+  if (status == media::PIPELINE_OK) {
     // Update our paused time.
     if (paused_) {
       paused_time_ = pipeline_->GetCurrentTime();
@@ -789,17 +793,20 @@ void WebMediaPlayerImpl::OnPipelineSeek() {
   }
 }
 
-void WebMediaPlayerImpl::OnPipelineEnded() {
+void WebMediaPlayerImpl::OnPipelineEnded(PipelineStatus status) {
   DCHECK(MessageLoop::current() == main_loop_);
-  if (pipeline_->GetError() == media::PIPELINE_OK) {
+  if (status == media::PIPELINE_OK) {
     GetClient()->timeChanged();
   }
 }
 
-void WebMediaPlayerImpl::OnPipelineError() {
+void WebMediaPlayerImpl::OnPipelineError(PipelineStatus error) {
   DCHECK(MessageLoop::current() == main_loop_);
-  switch (pipeline_->GetError()) {
+  switch (error) {
     case media::PIPELINE_OK:
+      LOG(DFATAL) << "PIPELINE_OK isn't an error!";
+      break;
+
     case media::PIPELINE_ERROR_INITIALIZATION_FAILED:
     case media::PIPELINE_ERROR_REQUIRED_FILTER_MISSING:
     case media::PIPELINE_ERROR_COULD_NOT_RENDER:
@@ -830,9 +837,9 @@ void WebMediaPlayerImpl::OnPipelineError() {
   Repaint();
 }
 
-void WebMediaPlayerImpl::OnNetworkEvent() {
+void WebMediaPlayerImpl::OnNetworkEvent(PipelineStatus status) {
   DCHECK(MessageLoop::current() == main_loop_);
-  if (pipeline_->GetError() == media::PIPELINE_OK) {
+  if (status == media::PIPELINE_OK) {
     if (pipeline_->IsNetworkActive()) {
       SetNetworkState(WebKit::WebMediaPlayer::Loading);
     } else {
@@ -892,7 +899,7 @@ void WebMediaPlayerImpl::Destroy() {
   }
 }
 
-void WebMediaPlayerImpl::PipelineStoppedCallback() {
+void WebMediaPlayerImpl::PipelineStoppedCallback(PipelineStatus status) {
   pipeline_stopped_.Signal();
 }
 
diff --git a/webkit/glue/webmediaplayer_impl.h b/webkit/glue/webmediaplayer_impl.h
index e7acf27..ef356b5 100644
--- a/webkit/glue/webmediaplayer_impl.h
+++ b/webkit/glue/webmediaplayer_impl.h
@@ -111,11 +111,11 @@ class WebMediaPlayerImpl : public WebKit::WebMediaPlayer,
     void AbortDataSources();
 
     // Methods for PipelineImpl -> WebMediaPlayerImpl communication.
-    void PipelineInitializationCallback();
-    void PipelineSeekCallback();
-    void PipelineEndedCallback();
-    void PipelineErrorCallback();
-    void NetworkEventCallback();
+    void PipelineInitializationCallback(media::PipelineStatus status);
+    void PipelineSeekCallback(media::PipelineStatus status);
+    void PipelineEndedCallback(media::PipelineStatus status);
+    void PipelineErrorCallback(media::PipelineStatus error);
+    void NetworkEventCallback(media::PipelineStatus status);
 
     // Returns the message loop used by the proxy.
     MessageLoop* message_loop() { return render_loop_; }
@@ -132,19 +132,20 @@ class WebMediaPlayerImpl : public WebKit::WebMediaPlayer,
     void RepaintTask();
 
     // Notify |webmediaplayer_| that initialization has finished.
-    void PipelineInitializationTask();
+    void PipelineInitializationTask(media::PipelineStatus status);
 
     // Notify |webmediaplayer_| that a seek has finished.
-    void PipelineSeekTask();
+    void PipelineSeekTask(media::PipelineStatus status);
 
     // Notify |webmediaplayer_| that the media has ended.
-    void PipelineEndedTask();
+    void PipelineEndedTask(media::PipelineStatus status);
 
-    // Notify |webmediaplayer_| that a pipeline error has been set.
-    void PipelineErrorTask();
+    // Notify |webmediaplayer_| that a pipeline error has occurred during
+    // playback.
+    void PipelineErrorTask(media::PipelineStatus error);
 
     // Notify |webmediaplayer_| that there's a network event.
-    void NetworkEventTask();
+    void NetworkEventTask(media::PipelineStatus status);
 
     // The render message loop where WebKit lives.
     MessageLoop* render_loop_;
@@ -261,15 +262,15 @@ class WebMediaPlayerImpl : public WebKit::WebMediaPlayer,
 
   void Repaint();
 
-  void OnPipelineInitialize();
+  void OnPipelineInitialize(media::PipelineStatus status);
 
-  void OnPipelineSeek();
+  void OnPipelineSeek(media::PipelineStatus status);
 
-  void OnPipelineEnded();
+  void OnPipelineEnded(media::PipelineStatus status);
 
-  void OnPipelineError();
+  void OnPipelineError(media::PipelineStatus error);
 
-  void OnNetworkEvent();
+  void OnNetworkEvent(media::PipelineStatus status);
 
  private:
   // Helpers that set the network/ready state and notifies the client if
@@ -282,7 +283,7 @@ class WebMediaPlayerImpl : public WebKit::WebMediaPlayer,
 
   // Callback executed after |pipeline_| stops which signals Destroy()
   // to continue.
-  void PipelineStoppedCallback();
+  void PipelineStoppedCallback(media::PipelineStatus status);
 
   // Getter method to |client_|.
   WebKit::WebMediaPlayerClient* GetClient();