Pipeline will execute a callback whenever an run-time error has happened....

Pipeline will execute a callback whenever an run-time error has happened.
BUG=16738
TEST=pipeline_impl_unittest.cc

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

[kylep, 2009-07-24 23:01:58]

This what you're looking for?

http://codereview.chromium.org/159373/diff/7/10
File webkit/glue/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/159373/diff/7/10#newcode417
Line 417: // We're in a good state. Do nothing.
There are probably other cases to handle, but they'll be easy to add.

[scherkus (not reviewing), 2009-07-25 00:46:06]

looking great!

what if the error callback was settable?  so constructor stayed the same but you
could set the callback and remove it via NULL?

what do you think?

http://codereview.chromium.org/159373/diff/28/1027
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/159373/diff/28/1027#newcode507
Line 507: if (error_callback_.get()) {
comment!

http://codereview.chromium.org/159373/diff/28/1026
File media/base/pipeline_impl.h (right):

http://codereview.chromium.org/159373/diff/28/1026#newcode10
Line 10: #include <map>
not seeing map used, unless it's needed for set?

http://codereview.chromium.org/159373/diff/28/1026#newcode45
Line 45: PipelineImpl(MessageLoop* message_loop, PipelineCallback*
error_callback);
comment on what error_Callback does

can it be NULL? etc

http://codereview.chromium.org/159373/diff/28/1030
File media/base/pipeline_impl_unittest.cc (right):

http://codereview.chromium.org/159373/diff/28/1030#newcode51
Line 51: NewCallback(reinterpret_cast<CallbackHelper*>(&callbacks_),
hmm... it's scary to have initializers depend on the order of the list

I would get rid of initializing pipeline_ in the initializer list and have it
implemented in the contructor

http://codereview.chromium.org/159373/diff/28/1031
File media/player/movie.cc (right):

http://codereview.chromium.org/159373/diff/28/1031#newcode199
Line 199: // Do nothing?
perhaps pass in NULL?

http://codereview.chromium.org/159373/diff/28/1028
File webkit/glue/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/159373/diff/28/1028#newcode418
Line 418: default:
these should set FormatError:
PIPELINE_ERROR_INITIALIZATION_FAILED
PIPELINE_ERROR_REQUIRED_FILTER_MISSING
PIPELINE_ERROR_COULD_NOT_RENDER

these should pass through (break):
PIPELINE_OK
PIPELINE_STOPPING

remove the default: clause and have all other errors set DecodeError (this way
compiler will catch missing cases in the situation we change the PipelineError
enum)

[kylep, 2009-07-27 19:53:11]

http://codereview.chromium.org/159373/diff/28/1027
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/159373/diff/28/1027#newcode507
Line 507: if (error_callback_.get()) {
On 2009/07/25 00:46:06, scherkus wrote:
> comment!

Done.

http://codereview.chromium.org/159373/diff/28/1026
File media/base/pipeline_impl.h (right):

http://codereview.chromium.org/159373/diff/28/1026#newcode10
Line 10: #include <map>
On 2009/07/25 00:46:06, scherkus wrote:
> not seeing map used, unless it's needed for set?

Idk. Ask lint. Should I remove it?

http://codereview.chromium.org/159373/diff/28/1026#newcode45
Line 45: PipelineImpl(MessageLoop* message_loop, PipelineCallback*
error_callback);
On 2009/07/25 00:46:06, scherkus wrote:
> comment on what error_Callback does
> 
> can it be NULL? etc

Done.

http://codereview.chromium.org/159373/diff/28/1030
File media/base/pipeline_impl_unittest.cc (right):

http://codereview.chromium.org/159373/diff/28/1030#newcode51
Line 51: NewCallback(reinterpret_cast<CallbackHelper*>(&callbacks_),
On 2009/07/25 00:46:06, scherkus wrote:
> hmm... it's scary to have initializers depend on the order of the list
> 
> I would get rid of initializing pipeline_ in the initializer list and have it
> implemented in the contructor

I moved the callback setting down to the body of the ctor, do you want me to
change the original so all the pipeline creation happens in the body?

http://codereview.chromium.org/159373/diff/28/1031
File media/player/movie.cc (right):

http://codereview.chromium.org/159373/diff/28/1031#newcode199
Line 199: // Do nothing?
On 2009/07/25 00:46:06, scherkus wrote:
> perhaps pass in NULL?

Done.

http://codereview.chromium.org/159373/diff/28/1028
File webkit/glue/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/159373/diff/28/1028#newcode418
Line 418: default:
On 2009/07/25 00:46:06, scherkus wrote:
> these should set FormatError:
> PIPELINE_ERROR_INITIALIZATION_FAILED
> PIPELINE_ERROR_REQUIRED_FILTER_MISSING
> PIPELINE_ERROR_COULD_NOT_RENDER
> 
> these should pass through (break):
> PIPELINE_OK
> PIPELINE_STOPPING
> 
> remove the default: clause and have all other errors set DecodeError (this way
> compiler will catch missing cases in the situation we change the PipelineError
> enum)

Done.

[scherkus (not reviewing), 2009-07-28 16:09:08]

http://codereview.chromium.org/159373/diff/28/1026
File media/base/pipeline_impl.h (right):

http://codereview.chromium.org/159373/diff/28/1026#newcode10
Line 10: #include <map>
On 2009/07/27 19:53:11, kylep wrote:
> On 2009/07/25 00:46:06, scherkus wrote:
> > not seeing map used, unless it's needed for set?
> 
> Idk. Ask lint. Should I remove it?

nah it's ok

http://codereview.chromium.org/159373/diff/28/1030
File media/base/pipeline_impl_unittest.cc (right):

http://codereview.chromium.org/159373/diff/28/1030#newcode51
Line 51: NewCallback(reinterpret_cast<CallbackHelper*>(&callbacks_),
On 2009/07/27 19:53:11, kylep wrote:
> On 2009/07/25 00:46:06, scherkus wrote:
> > hmm... it's scary to have initializers depend on the order of the list
> > 
> > I would get rid of initializing pipeline_ in the initializer list and have
it
> > implemented in the contructor
> 
> I moved the callback setting down to the body of the ctor, do you want me to
> change the original so all the pipeline creation happens in the body?

looks good as it is

http://codereview.chromium.org/159373/diff/1064/43
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/159373/diff/1064/43#newcode543
Line 543: if (error_callback_.get() && error != PIPELINE_STOPPING) {
are we guaranteed the error can't be PIPELINE_OK ?

http://codereview.chromium.org/159373/diff/1064/42
File media/base/pipeline_impl.h (right):

http://codereview.chromium.org/159373/diff/1064/42#newcode87
Line 87: // |error_callback_| is NULL, it is ignored.
I would briefly mention the ownership policy for error_callback

http://codereview.chromium.org/159373/diff/1064/42#newcode88
Line 88: virtual void SetPipelineErrorCallback(PipelineCallback*
error_callback);
blank line after

http://codereview.chromium.org/159373/diff/1064/44
File webkit/glue/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/159373/diff/1064/44#newcode413
Line 413: case media::PIPELINE_ERROR_INITIALIZATION_FAILED:
blank line before

http://codereview.chromium.org/159373/diff/1064/44#newcode419
Line 419: case media::PIPELINE_ERROR_URL_NOT_FOUND:
blank line before

[kylep, 2009-07-28 17:44:48]

http://codereview.chromium.org/159373/diff/1064/43
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/159373/diff/1064/43#newcode543
Line 543: if (error_callback_.get() && error != PIPELINE_STOPPING) {
On 2009/07/28 16:09:08, scherkus wrote:
> are we guaranteed the error can't be PIPELINE_OK ?

See line 517 or line 293. It's been doublely DCHECK'd.

http://codereview.chromium.org/159373/diff/1064/42
File media/base/pipeline_impl.h (right):

http://codereview.chromium.org/159373/diff/1064/42#newcode87
Line 87: // |error_callback_| is NULL, it is ignored.
On 2009/07/28 16:09:08, scherkus wrote:
> I would briefly mention the ownership policy for error_callback

Done.

http://codereview.chromium.org/159373/diff/1064/42#newcode88
Line 88: virtual void SetPipelineErrorCallback(PipelineCallback*
error_callback);
On 2009/07/28 16:09:08, scherkus wrote:
> blank line after

Done.

http://codereview.chromium.org/159373/diff/1064/44
File webkit/glue/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/159373/diff/1064/44#newcode413
Line 413: case media::PIPELINE_ERROR_INITIALIZATION_FAILED:
On 2009/07/28 16:09:08, scherkus wrote:
> blank line before

Done.

http://codereview.chromium.org/159373/diff/1064/44#newcode419
Line 419: case media::PIPELINE_ERROR_URL_NOT_FOUND:
On 2009/07/28 16:09:08, scherkus wrote:
> blank line before

Done.

[scherkus (not reviewing), 2009-07-28 18:09:23]

LGTM!