Require Renderer::Initialize() to return a status code via callback.

Require Renderer::Initialize() to return a status code via callback.

Using OnError() to deliver status codes during Initialize() complicates
upstream state paths, so require clients to return status via the
initialization callback provided instead of via the error callback.

BUG=450764
TEST=media_unittests, mojo://media_test

Committed: https://crrev.com/470461169311f3336e430fb91436366fa916af68
Cr-Commit-Position: refs/heads/master@{#312958}

[DaleCurtis, 2015-01-22 19:52:54]

dalecurtis@chromium.org changed reviewers:
+ xhwang@chromium.org

[xhwang, 2015-01-23 00:22:01]

looking good in general. I only have a few comments.

https://codereview.chromium.org/870693002/diff/1/media/base/renderer.h
File media/base/renderer.h (right):

https://codereview.chromium.org/870693002/diff/1/media/base/renderer.h#newcode45
media/base/renderer.h:45: // - |error_cb|: Executed if any error was encountered
after initialization.
Can we also update the comments in AudioRenderer and VideoRenderer:

https://code.google.com/p/chromium/codesearch#chromium/src/media/base/audio_r...

https://code.google.com/p/chromium/codesearch#chromium/src/media/base/video_r...

https://codereview.chromium.org/870693002/diff/1/media/filters/renderer_impl.cc
File media/filters/renderer_impl.cc (left):

https://codereview.chromium.org/870693002/diff/1/media/filters/renderer_impl....
media/filters/renderer_impl.cc:266: OnError(status);
I agree this is confusing! :)

https://codereview.chromium.org/870693002/diff/1/media/filters/renderer_impl.cc
File media/filters/renderer_impl.cc (right):

https://codereview.chromium.org/870693002/diff/1/media/filters/renderer_impl....
media/filters/renderer_impl.cc:53: DCHECK(init_cb_.is_null());
hmm, what if Pipeline::StopTask() is called during the initializetion? Is that
not possible?

https://codereview.chromium.org/870693002/diff/1/media/filters/renderer_impl....
media/filters/renderer_impl.cc:267: // they initialized successfully (due to
delayed output device setup).
Ouch, this is painful :(

[DaleCurtis, 2015-01-23 20:31:21]

https://codereview.chromium.org/870693002/diff/1/media/base/renderer.h
File media/base/renderer.h (right):

https://codereview.chromium.org/870693002/diff/1/media/base/renderer.h#newcode45
media/base/renderer.h:45: // - |error_cb|: Executed if any error was encountered
after initialization.
On 2015/01/23 00:22:01, xhwang wrote:
> Can we also update the comments in AudioRenderer and VideoRenderer:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/base/audio_r...
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/base/video_r...

Done.

https://codereview.chromium.org/870693002/diff/1/media/filters/renderer_impl.cc
File media/filters/renderer_impl.cc (right):

https://codereview.chromium.org/870693002/diff/1/media/filters/renderer_impl....
media/filters/renderer_impl.cc:53: DCHECK(init_cb_.is_null());
On 2015/01/23 00:22:01, xhwang wrote:
> hmm, what if Pipeline::StopTask() is called during the initializetion? Is that
> not possible?

Right, this is possible. Changed this to run init_cb_ with abort status.

[xhwang, 2015-01-23 21:03:08]

LGTM % nits

https://codereview.chromium.org/870693002/diff/20001/media/filters/renderer_i...
File media/filters/renderer_impl.cc (right):

https://codereview.chromium.org/870693002/diff/20001/media/filters/renderer_i...
media/filters/renderer_impl.cc:54:
base::ResetAndReturn(&init_cb_).Run(PIPELINE_ERROR_ABORT);
I don't think we could have |flush_cb| during initialization, so we could just
return here.

https://codereview.chromium.org/870693002/diff/20001/media/filters/renderer_i...
media/filters/renderer_impl.cc:574: void RendererImpl::FireAllPendingCallbacks()
{
Now this function only fires |flush_cb_|, maybe we don't need a dedicated
function for this any more. WDYT?

[DaleCurtis, 2015-01-23 21:19:42]

https://codereview.chromium.org/870693002/diff/20001/media/filters/renderer_i...
File media/filters/renderer_impl.cc (right):

https://codereview.chromium.org/870693002/diff/20001/media/filters/renderer_i...
media/filters/renderer_impl.cc:54:
base::ResetAndReturn(&init_cb_).Run(PIPELINE_ERROR_ABORT);
On 2015/01/23 21:03:07, xhwang wrote:
> I don't think we could have |flush_cb| during initialization, so we could just
> return here.

Changed to else.

https://codereview.chromium.org/870693002/diff/20001/media/filters/renderer_i...
media/filters/renderer_impl.cc:574: void RendererImpl::FireAllPendingCallbacks()
{
On 2015/01/23 21:03:07, xhwang wrote:
> Now this function only fires |flush_cb_|, maybe we don't need a dedicated
> function for this any more. WDYT?

Agreed.

[DaleCurtis, 2015-01-23 21:50:43]

The CQ bit was checked by dalecurtis@chromium.org

[commit-bot: I haz the power, 2015-01-23 21:51:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/870693002/40001

[commit-bot: I haz the power, 2015-01-23 22:58:51]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-01-23 22:59:50]

Patchset 3 (id:??) landed as
https://crrev.com/470461169311f3336e430fb91436366fa916af68
Cr-Commit-Position: refs/heads/master@{#312958}