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=22075

[Alpha Left Google, 2009-07-28 22:57:36]

Looks like you are out of sync, please do a sync first and upload the changes.

http://codereview.chromium.org/160298/diff/1/4
File media/base/pipeline_impl.cc (left):

http://codereview.chromium.org/160298/diff/1/4#oldcode76
Line 76: clock_(&base::Time::Now),
looks like you are out of sync.

[kylep, 2009-07-28 23:07:54]

Whoops. Let's try this again...

Thanks!

[Alpha Left Google, 2009-07-28 23:18:15]

The code looks good, but make sure the layout tests are still passing (with H264
supported libraries)

http://codereview.chromium.org/160298/diff/1012/1013
File media/base/pipeline_impl.h (right):

http://codereview.chromium.org/160298/diff/1012/1013#newcode10
Line 10: #include <map>
where is <map> used?

http://codereview.chromium.org/160298/diff/1012/1016
File webkit/glue/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/160298/diff/1012/1016#newcode413
Line 413: // We're in a good state. Do nothing.
I think you won't be called with pipeline_stopping, maybe a DCHECK works better?

[scherkus (not reviewing), 2009-07-28 23:21:35]

http://codereview.chromium.org/160298/diff/1012/1016
File webkit/glue/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/160298/diff/1012/1016#newcode152
Line 152: NOTREACHED() << "Could not start PipelineThread";
add a "return" after this line, otherwise we'll explode in a full-release build

http://codereview.chromium.org/160298/diff/1012/1016#newcode154
Line 154: pipeline_ = new media::PipelineImpl(pipeline_thread_.message_loop());
then you can remove the "else" clause

http://codereview.chromium.org/160298/diff/1012/1016#newcode413
Line 413: // We're in a good state. Do nothing.
On 2009/07/28 23:18:15, Alpha wrote:
> I think you won't be called with pipeline_stopping, maybe a DCHECK works
better?

yeah I'm inclined to agree

you should do a NOTREACHED() << "We shouldn't get called with this crap"

[kylep, 2009-07-29 00:52:59]

Well, it's no longer crashing on h264 files, but now ogg audio doesn't play in
the the test shell....

http://codereview.chromium.org/160298/diff/1012/1013
File media/base/pipeline_impl.h (right):

http://codereview.chromium.org/160298/diff/1012/1013#newcode10
Line 10: #include <map>
On 2009/07/28 23:18:15, Alpha wrote:
> where is <map> used?

I don't know. Lint told me to add it. It's obviously causing more confusion than
anything, so I'll get rid of it.

http://codereview.chromium.org/160298/diff/1012/1016
File webkit/glue/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/160298/diff/1012/1016#newcode152
Line 152: NOTREACHED() << "Could not start PipelineThread";
On 2009/07/28 23:21:35, scherkus wrote:
> add a "return" after this line, otherwise we'll explode in a full-release
build

Done.

http://codereview.chromium.org/160298/diff/1012/1016#newcode154
Line 154: pipeline_ = new media::PipelineImpl(pipeline_thread_.message_loop());
On 2009/07/28 23:21:35, scherkus wrote:
> then you can remove the "else" clause

Done.

http://codereview.chromium.org/160298/diff/1012/1016#newcode413
Line 413: // We're in a good state. Do nothing.
On 2009/07/28 23:21:35, scherkus wrote:
> On 2009/07/28 23:18:15, Alpha wrote:
> > I think you won't be called with pipeline_stopping, maybe a DCHECK works
> better?
> 
> yeah I'm inclined to agree
> 
> you should do a NOTREACHED() << "We shouldn't get called with this crap"

Sounds good. Though I think I'll use cleaner language... ~^

[scherkus (not reviewing), 2009-07-29 17:53:47]

LGTM

are you still witness ogg issues?  yesterday was a bad day for us... bunch of
regressions got fixed

I'll try locally

[kylep, 2009-07-30 17:34:50]

Ok, I'm 99% certain my sync issues are from my ARB code, not this. I'm synced
with the latest revision, so when the trybots turn green, I'm committing this.