Media Remoting: Don't report remote renderer init error to pipeline.

Media Remoting: Don't report remote renderer init error to pipeline.

When error occurs during initializing the remote renderer,
CourierRenderer should not report the failure to media pipeline.
Instead, it should always report success to the media pipeline, and
then become a no-op renderer after that point.  The remoting session
will soon end, and a normal local renderer will be created instead.

BUG=687750
Review-Url: https://codereview.chromium.org/2669033004
Cr-Commit-Position: refs/heads/master@{#447911}
Committed: https://chromium.googlesource.com/chromium/src/+/fa3876bfc4b420d126b2a52f653b3bbb13944de5

[xjz, 2017-02-02 01:37:12]

Patchset #1 (id:1) has been deleted

[xjz, 2017-02-02 01:37:47]

The CQ bit was checked by xjz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-02 01:38:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xjz, 2017-02-02 01:42:46]

xjz@chromium.org changed reviewers:
+ miu@chromium.org

[xjz, 2017-02-02 01:42:46]

PTAL

[commit-bot: I haz the power, 2017-02-02 05:35:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-02 05:35:19]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)

[miu, 2017-02-02 20:50:16]

I think it'd be good to have a quick unit test to confirm this, so that our code
is robust to future changes where developers might not be aware of our
assumptions here. Something like CourierRendererTest.Initialize, but instead the
initialization fails and then make sure that the pipeline calls into the
media::Renderer methods do no-ops:

1. Calling Flush() would result in the flush_cb being run without any RPCs being
sent.
2. Calling StartPlayingFrom(), SetPlaybackRate(), etc. don't crash (and no RPCs
are sent).

https://codereview.chromium.org/2669033004/diff/20001/media/remoting/courier_...
File media/remoting/courier_renderer.cc (right):

https://codereview.chromium.org/2669033004/diff/20001/media/remoting/courier_...
media/remoting/courier_renderer.cc:322:
DCHECK(!init_workflow_done_callback_.is_null());
Could this DCHECK() trip? Probably not, but maybe we should move it to after the
|state_| check a few lines below...

[xjz, 2017-02-02 22:06:49]

The CQ bit was checked by xjz@chromium.org to run a CQ dry run

[xjz, 2017-02-02 22:07:11]

Addressed comments. PTAL. Thanks!

https://codereview.chromium.org/2669033004/diff/20001/media/remoting/courier_...
File media/remoting/courier_renderer.cc (right):

https://codereview.chromium.org/2669033004/diff/20001/media/remoting/courier_...
media/remoting/courier_renderer.cc:322:
DCHECK(!init_workflow_done_callback_.is_null());
On 2017/02/02 20:50:16, miu wrote:
> Could this DCHECK() trip? Probably not, but maybe we should move it to after
the
> |state_| check a few lines below...

Done. I couldn't think of a case that this DCHECK fails. But anyway, it doesn't
hurt moving it a few lines below. :)

[commit-bot: I haz the power, 2017-02-02 22:07:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[miu, 2017-02-02 22:37:58]

lgtm

https://codereview.chromium.org/2669033004/diff/40001/media/remoting/courier_...
File media/remoting/courier_renderer_unittest.cc (right):

https://codereview.chromium.org/2669033004/diff/40001/media/remoting/courier_...
media/remoting/courier_renderer_unittest.cc:351: // Indicates whether the
renderer on receiver side is successfully
nit: To be clearer, let's say "...whether the test wants to simulate successful
initialization in the renderer on the receiver side."

https://codereview.chromium.org/2669033004/diff/40001/media/remoting/courier_...
media/remoting/courier_renderer_unittest.cc:370: TEST_F(CourierRendererTest,
InitializeFailed) {
Looks great! :)

[xjz, 2017-02-02 23:03:31]

Addressed comments. Thanks!

https://codereview.chromium.org/2669033004/diff/40001/media/remoting/courier_...
File media/remoting/courier_renderer_unittest.cc (right):

https://codereview.chromium.org/2669033004/diff/40001/media/remoting/courier_...
media/remoting/courier_renderer_unittest.cc:351: // Indicates whether the
renderer on receiver side is successfully
On 2017/02/02 22:37:58, miu wrote:
> nit: To be clearer, let's say "...whether the test wants to simulate
successful
> initialization in the renderer on the receiver side."

Done.

[xjz, 2017-02-02 23:04:45]

The CQ bit was checked by xjz@chromium.org

[xjz, 2017-02-02 23:04:45]

The patchset sent to the CQ was uploaded after l-g-t-m from miu@chromium.org
Link to the patchset: https://codereview.chromium.org/2669033004/#ps60001
(title: "Addressed comments.")

[commit-bot: I haz the power, 2017-02-02 23:05:24]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-03 03:45:01]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1486076685438730,
"parent_rev": "3ba9c67e210df1f039f93548c1ad5bb2dfe3a945", "commit_rev":
"fa3876bfc4b420d126b2a52f653b3bbb13944de5"}

[commit-bot: I haz the power, 2017-02-03 03:45:39]

Description was changed from

==========
Media Remoting: Don't report remote renderer init error to pipeline.

When error occurs during initializing the remote renderer,
CourierRenderer should not report the failure to media pipeline.
Instead, it should always report success to the media pipeline, and
then become a no-op renderer after that point.  The remoting session
will soon end, and a normal local renderer will be created instead.

BUG=687750
==========

to

==========
Media Remoting: Don't report remote renderer init error to pipeline.

When error occurs during initializing the remote renderer,
CourierRenderer should not report the failure to media pipeline.
Instead, it should always report success to the media pipeline, and
then become a no-op renderer after that point.  The remoting session
will soon end, and a normal local renderer will be created instead.

BUG=687750

Review-Url: https://codereview.chromium.org/2669033004
Cr-Commit-Position: refs/heads/master@{#447911}
Committed:
https://chromium.googlesource.com/chromium/src/+/fa3876bfc4b420d126b2a52f653b...
==========

[commit-bot: I haz the power, 2017-02-03 03:45:41]

Committed patchset #3 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/fa3876bfc4b420d126b2a52f653b...