Fix MediaRemoting MediaObserver

Fix MediaRemoting MediaObserver

https: //codereview.chromium.org/2905613003
Review-Url: https://codereview.chromium.org/2913153004
Cr-Commit-Position: refs/heads/master@{#477193}
Committed: https://chromium.googlesource.com/chromium/src/+/6fd962cb3e87ed2c1738f63b6a730f63badf7bf8

[chcunningham, 2017-06-02 00:56:14]

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

[commit-bot: I haz the power, 2017-06-02 00:57:08]

Dry run: CQ is trying da patch.

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

[chcunningham, 2017-06-02 00:57:30]

chcunningham@chromium.org changed reviewers:
+ jochen@chromium.org, miu@chromium.org

[chcunningham, 2017-06-02 00:57:31]

I broke MediaObserver plumbing in a previous CL, so just fixing that. Miu@, I'm
surprised no tests failed?

[commit-bot: I haz the power, 2017-06-02 02:56:41]

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

[commit-bot: I haz the power, 2017-06-02 02:56:42]

Dry run: This issue passed the CQ dry run.

[jochen (gone - plz use gerrit), 2017-06-02 12:17:18]

can you add a test please?

[chcunningham, 2017-06-02 17:32:44]

On 2017/06/02 12:17:18, jochen wrote:
> can you add a test please?

Happy to do so. I'm not familiar with remoting details (I just found this via
code review), so Yuri, please advise on the where/how for a test.

[chcunningham, 2017-06-02 17:36:23]

On 2017/06/02 17:32:44, chcunningham wrote:
> On 2017/06/02 12:17:18, jochen wrote:
> > can you add a test please?
> 
> Happy to do so. I'm not familiar with remoting details (I just found this via
> code review), so Yuri, please advise on the where/how for a test.

FYI - duplicate change here
https://chromium-review.googlesource.com/c/522166/

[whywhat, 2017-06-02 19:32:19]

On 2017/06/02 at 12:17:18, jochen wrote:
> can you add a test please?

The integration tests are being worked on, tracking bug is here:
https://bugs.chromium.org/p/chromium/issues/detail?id=684065
Seems like they're ready to go but wait for some refactoring to land. xjz,
what's the status and what do you think of the test?

Would a unit test that checks the out_media_observer is not null (ifdefed with
ENABLE_MEDIA_REMOTING) be sufficient for this CL?

[whywhat, 2017-06-02 19:32:31]

avayvod@chromium.org changed reviewers:
+ avayvod@chromium.org, xjz@chromium.org

[whywhat, 2017-06-02 19:32:31]

+xjz

[chcunningham, 2017-06-05 17:58:19]

> Would a unit test that checks the out_media_observer is not null (ifdefed with
> ENABLE_MEDIA_REMOTING) be sufficient for this CL?

Works for me. Stand by for a unit test.

[xjz, 2017-06-05 19:00:25]

On 2017/06/05 17:58:19, chcunningham wrote:
> > Would a unit test that checks the out_media_observer is not null (ifdefed
with
> > ENABLE_MEDIA_REMOTING) be sufficient for this CL?
> 
> Works for me. Stand by for a unit test.

A simple check might be good just for this CL.
The browser integration tests for media remoting were hold off because the big
refactoring I am currently working on.
miu@: Maybe we should land some simple tests first? WDYT?

[miu, 2017-06-05 20:02:03]

lgtm

https://codereview.chromium.org/2913153004/diff/20001/content/renderer/media/...
File content/renderer/media/media_factory.cc (right):

https://codereview.chromium.org/2913153004/diff/20001/content/renderer/media/...
content/renderer/media/media_factory.cc:352: *out_media_observer =
remoting_controller->GetWeakPtr();
Hmm...Surprised the old code didn't generate a compiler error (unused var).

[xjz, 2017-06-05 21:12:34]

lgtm

Just had a quick chat with miu@. The end to end browser tests will cover this
regression later. For now, this type of regression will be caught in Canary. So
just for this CL it's up to you whether add a test to make sure that the
observer is not null when media remoting is enabled :).

[chcunningham, 2017-06-05 22:34:26]

Thanks all

> A simple check might be good just for this CL.

Done.

[chcunningham, 2017-06-05 22:34:42]

The CQ bit was checked by chcunningham@chromium.org

[chcunningham, 2017-06-05 22:34:43]

The patchset sent to the CQ was uploaded after l-g-t-m from xjz@chromium.org,
miu@chromium.org
Link to the patchset: https://codereview.chromium.org/2913153004/#ps40001
(title: "Add DCHECK")

[commit-bot: I haz the power, 2017-06-05 22:35:25]

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-06-06 02:54:50]

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

[commit-bot: I haz the power, 2017-06-06 02:54:52]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[chcunningham, 2017-06-06 03:22:06]

The CQ bit was checked by chcunningham@chromium.org

[commit-bot: I haz the power, 2017-06-06 03:22:34]

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-06-06 04:23:05]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1496719326508040,
"parent_rev": "14fb5d1f03c1d705a5c0e1d33904df5da7153221", "commit_rev":
"6fd962cb3e87ed2c1738f63b6a730f63badf7bf8"}

[commit-bot: I haz the power, 2017-06-06 04:23:20]

Description was changed from

==========
Fix MediaRemoting MediaObserver

Previous CL caused the media observer to always be null.
https://codereview.chromium.org/2905613003
==========

to

==========
Fix MediaRemoting MediaObserver

https: //codereview.chromium.org/2905613003
Review-Url: https://codereview.chromium.org/2913153004
Cr-Commit-Position: refs/heads/master@{#477193}
Committed:
https://chromium.googlesource.com/chromium/src/+/6fd962cb3e87ed2c1738f63b6a73...
==========

[commit-bot: I haz the power, 2017-06-06 04:23:21]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/6fd962cb3e87ed2c1738f63b6a73...