PipelineError is dead. Long live PipelineStatus!

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)

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=78379

[acolwell GONE FROM CHROMIUM, 2011-03-15 03:56:27]

LGTM w/ minor suggestions.

http://codereview.chromium.org/6686061/diff/2001/media/base/filter_host.h
File media/base/filter_host.h (right):

http://codereview.chromium.org/6686061/diff/2001/media/base/filter_host.h#new...
media/base/filter_host.h:1: // Copyright (c) 2010 The Chromium Authors. All
rights reserved.
copyright

http://codereview.chromium.org/6686061/diff/2001/media/base/pipeline_impl.cc
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/6686061/diff/2001/media/base/pipeline_impl.cc#...
media/base/pipeline_impl.cc:21: callback_.reset(::NewCallback(this,
&PipelineStatusNotification::Notify));
Is :: necessary here?

http://codereview.chromium.org/6686061/diff/2001/media/base/pipeline_impl.h
File media/base/pipeline_impl.h (right):

http://codereview.chromium.org/6686061/diff/2001/media/base/pipeline_impl.h#n...
media/base/pipeline_impl.h:103: virtual void Seek(base::TimeDelta time,
PipelineStatusCallback* seek_callback);
over 80 chars.

http://codereview.chromium.org/6686061/diff/2001/media/base/pipeline_impl_uni...
File media/base/pipeline_impl_unittest.cc (left):

http://codereview.chromium.org/6686061/diff/2001/media/base/pipeline_impl_uni...
media/base/pipeline_impl_unittest.cc:341: TEST_F(PipelineImplTest, URLNotFound)
{
Was this test delete by accident?

http://codereview.chromium.org/6686061/diff/2001/media/base/pipeline_impl_uni...
File media/base/pipeline_impl_unittest.cc (right):

http://codereview.chromium.org/6686061/diff/2001/media/base/pipeline_impl_uni...
media/base/pipeline_impl_unittest.cc:749: FlexibleCallbackRunner(base::TimeDelta
delay, PipelineStatus status,
How about just an int called delayInMs? It would remove the conversion to and
from TimeDelta

http://codereview.chromium.org/6686061/diff/2001/webkit/glue/webmediaplayer_i...
File webkit/glue/webmediaplayer_impl.h (right):

http://codereview.chromium.org/6686061/diff/2001/webkit/glue/webmediaplayer_i...
webkit/glue/webmediaplayer_impl.h:117: void
PipelineStatusCallback(media::PipelineStatus status);
I think this should stay PipelineErrorCallback() since it is only called to
indicate when an error has occurred during playback.

http://codereview.chromium.org/6686061/diff/2001/webkit/glue/webmediaplayer_i...
webkit/glue/webmediaplayer_impl.h:143: // Notify |webmediaplayer_| that a
pipeline error has been set.
Maybe change to something like, "Notify |webmediaplayer_| when a pipeline error
has occurred during playback?"

http://codereview.chromium.org/6686061/diff/2001/webkit/glue/webmediaplayer_i...
webkit/glue/webmediaplayer_impl.h:144: void
PipelineStatusTask(media::PipelineStatus status);
PipelineErrorTask() since it is only called on an error condition?

http://codereview.chromium.org/6686061/diff/2001/webkit/glue/webmediaplayer_i...
webkit/glue/webmediaplayer_impl.h:270: void
OnPipelineStatus(media::PipelineStatus status);
Same comment as above.

[Ami GONE FROM CHROMIUM, 2011-03-15 17:37:18]

Thanks for the review!  PTAL.

http://codereview.chromium.org/6686061/diff/2001/media/base/filter_host.h
File media/base/filter_host.h (right):

http://codereview.chromium.org/6686061/diff/2001/media/base/filter_host.h#new...
media/base/filter_host.h:1: // Copyright (c) 2010 The Chromium Authors. All
rights reserved.
On 2011/03/15 03:56:28, acolwell wrote:
> copyright

Done.  Next CL: presubmit that checks this for me.

http://codereview.chromium.org/6686061/diff/2001/media/base/pipeline_impl.cc
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/6686061/diff/2001/media/base/pipeline_impl.cc#...
media/base/pipeline_impl.cc:21: callback_.reset(::NewCallback(this,
&PipelineStatusNotification::Notify));
On 2011/03/15 03:56:28, acolwell wrote:
> Is :: necessary here?

Not anymore (it used to be, back when I had PSN::Callback() named
PSN::NewCallback()).  Gone.

http://codereview.chromium.org/6686061/diff/2001/media/base/pipeline_impl.h
File media/base/pipeline_impl.h (right):

http://codereview.chromium.org/6686061/diff/2001/media/base/pipeline_impl.h#n...
media/base/pipeline_impl.h:103: virtual void Seek(base::TimeDelta time,
PipelineStatusCallback* seek_callback);
On 2011/03/15 03:56:28, acolwell wrote:
> over 80 chars.

Done.

http://codereview.chromium.org/6686061/diff/2001/media/base/pipeline_impl_uni...
File media/base/pipeline_impl_unittest.cc (left):

http://codereview.chromium.org/6686061/diff/2001/media/base/pipeline_impl_uni...
media/base/pipeline_impl_unittest.cc:341: TEST_F(PipelineImplTest, URLNotFound)
{
On 2011/03/15 03:56:28, acolwell wrote:
> Was this test delete by accident?

Apparently so.

http://codereview.chromium.org/6686061/diff/2001/media/base/pipeline_impl_uni...
File media/base/pipeline_impl_unittest.cc (right):

http://codereview.chromium.org/6686061/diff/2001/media/base/pipeline_impl_uni...
media/base/pipeline_impl_unittest.cc:749: FlexibleCallbackRunner(base::TimeDelta
delay, PipelineStatus status,
On 2011/03/15 03:56:28, acolwell wrote:
> How about just an int called delayInMs? It would remove the conversion to and
> from TimeDelta

Nice catch.

http://codereview.chromium.org/6686061/diff/2001/webkit/glue/webmediaplayer_i...
File webkit/glue/webmediaplayer_impl.h (right):

http://codereview.chromium.org/6686061/diff/2001/webkit/glue/webmediaplayer_i...
webkit/glue/webmediaplayer_impl.h:117: void
PipelineStatusCallback(media::PipelineStatus status);
On 2011/03/15 03:56:28, acolwell wrote:
> I think this should stay PipelineErrorCallback() since it is only called to
> indicate when an error has occurred during playback.

Done.

http://codereview.chromium.org/6686061/diff/2001/webkit/glue/webmediaplayer_i...
webkit/glue/webmediaplayer_impl.h:143: // Notify |webmediaplayer_| that a
pipeline error has been set.
On 2011/03/15 03:56:28, acolwell wrote:
> Maybe change to something like, "Notify |webmediaplayer_| when a pipeline
error
> has occurred during playback?"

Done.

http://codereview.chromium.org/6686061/diff/2001/webkit/glue/webmediaplayer_i...
webkit/glue/webmediaplayer_impl.h:144: void
PipelineStatusTask(media::PipelineStatus status);
On 2011/03/15 03:56:28, acolwell wrote:
> PipelineErrorTask() since it is only called on an error condition?

Done.

http://codereview.chromium.org/6686061/diff/2001/webkit/glue/webmediaplayer_i...
webkit/glue/webmediaplayer_impl.h:270: void
OnPipelineStatus(media::PipelineStatus status);
On 2011/03/15 03:56:28, acolwell wrote:
> Same comment as above.

Done, but note that the impl used to treat PIPELINE_OK as a network error (!). 
PTAL at the impl change.

[acolwell GONE FROM CHROMIUM, 2011-03-15 23:43:55]

http://codereview.chromium.org/6686061/diff/5004/media/base/pipeline_impl.cc
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/6686061/diff/5004/media/base/pipeline_impl.cc#...
media/base/pipeline_impl.cc:20: : cv_(&lock_), status_(PIPELINE_OK),
notified_(false) {
I think each initializer needs to be on a separate line.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Constructor_In...

http://codereview.chromium.org/6686061/diff/5004/media/base/pipeline_impl.h
File media/base/pipeline_impl.h (right):

http://codereview.chromium.org/6686061/diff/5004/media/base/pipeline_impl.h#n...
media/base/pipeline_impl.h:34: // argument was.
Please add a comment about Callback() being a one-shot.

http://codereview.chromium.org/6686061/diff/5004/media/base/pipeline_impl.h#n...
media/base/pipeline_impl.h:51: scoped_ptr<media::PipelineStatusCallback>
callback_;
DISALLOW_COPY_AND_ASSIGN

http://codereview.chromium.org/6686061/diff/5004/media/base/pipeline_impl_uni...
File media/base/pipeline_impl_unittest.cc (right):

http://codereview.chromium.org/6686061/diff/5004/media/base/pipeline_impl_uni...
media/base/pipeline_impl_unittest.cc:343: EXPECT_CALL(callbacks_,
OnError(PIPELINE_ERROR_URL_NOT_FOUND));
This doesn't smell right. I wouldn't expect an OnError() call here. I'd just
expected the start callback to report the error. Perhaps add a TODO and fix this
with a separate CL?

http://codereview.chromium.org/6686061/diff/5004/media/base/pipeline_impl_uni...
media/base/pipeline_impl_unittest.cc:355: EXPECT_CALL(callbacks_,
OnError(PIPELINE_ERROR_COULD_NOT_RENDER));
This doesn't smell right. I wouldn't expect an OnError() call here. I'd just
expected the start callback to report the error. Perhaps add a TODO and fix this
with a separate CL?

http://codereview.chromium.org/6686061/diff/5004/media/base/pipeline_impl_uni...
media/base/pipeline_impl_unittest.cc:763: : delayInMs_(delayInMs),
status_(status), callback_(callback) {
I think these need to be on separate lines.

http://codereview.chromium.org/6686061/diff/5004/media/tools/player_x11/playe...
File media/tools/player_x11/player_x11.cc (right):

http://codereview.chromium.org/6686061/diff/5004/media/tools/player_x11/playe...
media/tools/player_x11/player_x11.cc:57: MessageLoopQuitter(MessageLoop* loop) :
loop_(loop) {}
explicit

http://codereview.chromium.org/6686061/diff/5004/media/tools/player_x11/playe...
media/tools/player_x11/player_x11.cc:63: MessageLoop* loop_;
DISALLOW_COPY_AND_ASSIGN

http://codereview.chromium.org/6686061/diff/5004/webkit/glue/webmediaplayer_i...
File webkit/glue/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/6686061/diff/5004/webkit/glue/webmediaplayer_i...
webkit/glue/webmediaplayer_impl.cc:902: void
WebMediaPlayerImpl::PipelineStoppedCallback(PipelineStatus status) {
Could we replace the code that uses this with your new
PipelineStatusNotification object?

[Ami GONE FROM CHROMIUM, 2011-03-16 00:01:01]

http://codereview.chromium.org/6686061/diff/5004/media/base/pipeline_impl.cc
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/6686061/diff/5004/media/base/pipeline_impl.cc#...
media/base/pipeline_impl.cc:20: : cv_(&lock_), status_(PIPELINE_OK),
notified_(false) {
On 2011/03/15 23:43:55, acolwell wrote:
> I think each initializer needs to be on a separate line.
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Constructor_In...

I don't think so.

http://codereview.chromium.org/6686061/diff/5004/media/base/pipeline_impl.h
File media/base/pipeline_impl.h (right):

http://codereview.chromium.org/6686061/diff/5004/media/base/pipeline_impl.h#n...
media/base/pipeline_impl.h:34: // argument was.
On 2011/03/15 23:43:55, acolwell wrote:
> Please add a comment about Callback() being a one-shot.

Done.

http://codereview.chromium.org/6686061/diff/5004/media/base/pipeline_impl.h#n...
media/base/pipeline_impl.h:51: scoped_ptr<media::PipelineStatusCallback>
callback_;
On 2011/03/15 23:43:55, acolwell wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

http://codereview.chromium.org/6686061/diff/5004/media/base/pipeline_impl_uni...
File media/base/pipeline_impl_unittest.cc (right):

http://codereview.chromium.org/6686061/diff/5004/media/base/pipeline_impl_uni...
media/base/pipeline_impl_unittest.cc:343: EXPECT_CALL(callbacks_,
OnError(PIPELINE_ERROR_URL_NOT_FOUND));
On 2011/03/15 23:43:55, acolwell wrote:
> This doesn't smell right. I wouldn't expect an OnError() call here. I'd just
> expected the start callback to report the error. Perhaps add a TODO and fix
this
> with a separate CL?

Done.

http://codereview.chromium.org/6686061/diff/5004/media/base/pipeline_impl_uni...
media/base/pipeline_impl_unittest.cc:355: EXPECT_CALL(callbacks_,
OnError(PIPELINE_ERROR_COULD_NOT_RENDER));
On 2011/03/15 23:43:55, acolwell wrote:
> This doesn't smell right. I wouldn't expect an OnError() call here. I'd just
> expected the start callback to report the error. Perhaps add a TODO and fix
this
> with a separate CL?

Done.

http://codereview.chromium.org/6686061/diff/5004/media/base/pipeline_impl_uni...
media/base/pipeline_impl_unittest.cc:763: : delayInMs_(delayInMs),
status_(status), callback_(callback) {
On 2011/03/15 23:43:55, acolwell wrote:
> I think these need to be on separate lines.

I don't think so.

http://codereview.chromium.org/6686061/diff/5004/media/tools/player_x11/playe...
File media/tools/player_x11/player_x11.cc (right):

http://codereview.chromium.org/6686061/diff/5004/media/tools/player_x11/playe...
media/tools/player_x11/player_x11.cc:57: MessageLoopQuitter(MessageLoop* loop) :
loop_(loop) {}
On 2011/03/15 23:43:55, acolwell wrote:
> explicit

Done.

http://codereview.chromium.org/6686061/diff/5004/media/tools/player_x11/playe...
media/tools/player_x11/player_x11.cc:63: MessageLoop* loop_;
On 2011/03/15 23:43:55, acolwell wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

http://codereview.chromium.org/6686061/diff/5004/webkit/glue/webmediaplayer_i...
File webkit/glue/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/6686061/diff/5004/webkit/glue/webmediaplayer_i...
webkit/glue/webmediaplayer_impl.cc:902: void
WebMediaPlayerImpl::PipelineStoppedCallback(PipelineStatus status) {
On 2011/03/15 23:43:55, acolwell wrote:
> Could we replace the code that uses this with your new
> PipelineStatusNotification object?

Next CL.

[acolwell GONE FROM CHROMIUM, 2011-03-16 00:05:23]

LGTM

[scherkus (not reviewing), 2011-03-23 05:45:13]

FYI this is awesome