media: Use associated interface for mojo RendererClient

media: Use associated interface for mojo RendererClient

Use associated interface for the RendererClient so that client calls
runs on the same message pipe as the main interface, so that we can
guarantee the delivery order of client calls and main interface
callbacks.

Also update MojoRendererTest to use TestMessageLoop so that pending
tasks are automatically executed during destruction. This helps make
sure we don't leak objects.

BUG=646054
Committed: https://crrev.com/ea1e36d904dbc72f46a9f0ae695796ac3e55bfa1
Cr-Commit-Position: refs/heads/master@{#420430}

[xhwang, 2016-09-22 06:29:32]

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

[commit-bot: I haz the power, 2016-09-22 06:29:47]

Dry run: 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, 2016-09-22 06:48:15]

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

[commit-bot: I haz the power, 2016-09-22 06:48:16]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[xhwang, 2016-09-22 07:00:03]

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

[commit-bot: I haz the power, 2016-09-22 07:00:15]

Dry run: 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, 2016-09-22 10:02:08]

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

[commit-bot: I haz the power, 2016-09-22 10:02:15]

Dry run: This issue passed the CQ dry run.

[xhwang, 2016-09-22 16:30:47]

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

[xhwang, 2016-09-22 16:30:48]

alokp: PTAL

See https://codereview.chromium.org/2330273002/ for the context.

[alokp, 2016-09-22 17:30:08]

take my lgtm with a grain of salt because I have not used Associated interface
myself :)

https://codereview.chromium.org/2358413002/diff/20001/media/mojo/clients/mojo...
File media/mojo/clients/mojo_renderer.cc (right):

https://codereview.chromium.org/2358413002/diff/20001/media/mojo/clients/mojo...
media/mojo/clients/mojo_renderer.cc:107: mojom::RendererClientAssociatedPtrInfo
client_ptr_info;
move these two lines into BindRemoteRendererIfNeeded so that it is not repeated
in InitializeRendererFromUrl?

[alokp, 2016-09-22 17:30:09]

take my lgtm with a grain of salt because I have not used Associated interface
myself :)

https://codereview.chromium.org/2358413002/diff/20001/media/mojo/clients/mojo...
File media/mojo/clients/mojo_renderer.cc (right):

https://codereview.chromium.org/2358413002/diff/20001/media/mojo/clients/mojo...
media/mojo/clients/mojo_renderer.cc:107: mojom::RendererClientAssociatedPtrInfo
client_ptr_info;
move these two lines into BindRemoteRendererIfNeeded so that it is not repeated
in InitializeRendererFromUrl?

[xhwang, 2016-09-22 18:11:57]

https://codereview.chromium.org/2358413002/diff/20001/media/mojo/clients/mojo...
File media/mojo/clients/mojo_renderer.cc (right):

https://codereview.chromium.org/2358413002/diff/20001/media/mojo/clients/mojo...
media/mojo/clients/mojo_renderer.cc:107: mojom::RendererClientAssociatedPtrInfo
client_ptr_info;
On 2016/09/22 17:30:08, alokp wrote:
> move these two lines into BindRemoteRendererIfNeeded so that it is not
repeated
> in InitializeRendererFromUrl?

I see the benefit of less code duplication. But really this is about binding the
"client ptr", not the "remote renderer". So it seems a wrong place to me.

[xhwang, 2016-09-22 18:16:39]

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

[xhwang, 2016-09-22 18:16:40]

dcheng: Please OWNERS/security review the one word change in the mojom file.
Basically I am using "associated" interface now.

CC yzshen@ FYI

[alokp, 2016-09-22 18:17:24]

https://codereview.chromium.org/2358413002/diff/20001/media/mojo/clients/mojo...
File media/mojo/clients/mojo_renderer.cc (right):

https://codereview.chromium.org/2358413002/diff/20001/media/mojo/clients/mojo...
media/mojo/clients/mojo_renderer.cc:107: mojom::RendererClientAssociatedPtrInfo
client_ptr_info;
On 2016/09/22 18:11:57, xhwang (slow) wrote:
> On 2016/09/22 17:30:08, alokp wrote:
> > move these two lines into BindRemoteRendererIfNeeded so that it is not
> repeated
> > in InitializeRendererFromUrl?
> 
> I see the benefit of less code duplication. But really this is about binding
the
> "client ptr", not the "remote renderer". So it seems a wrong place to me.

Acknowledged.

[dcheng, 2016-09-22 19:21:52]

mojo lgtm

[xhwang, 2016-09-22 19:39:55]

The CQ bit was checked by xhwang@chromium.org

[commit-bot: I haz the power, 2016-09-22 19:40:23]

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, 2016-09-22 19:46:18]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-09-22 19:49:16]

Description was changed from

==========
media: Use associated interface for mojo RendererClient

Use associated interface for the RendererClient so that client calls
runs on the same message pipe as the main interface, so that we can
guarantee the delivery order of client calls and main interface
callbacks.

Also update MojoRendererTest to use TestMessageLoop so that pending
tasks are automatically executed during destruction. This helps make
sure we don't leak objects.

BUG=646054
==========

to

==========
media: Use associated interface for mojo RendererClient

Use associated interface for the RendererClient so that client calls
runs on the same message pipe as the main interface, so that we can
guarantee the delivery order of client calls and main interface
callbacks.

Also update MojoRendererTest to use TestMessageLoop so that pending
tasks are automatically executed during destruction. This helps make
sure we don't leak objects.

BUG=646054

Committed: https://crrev.com/ea1e36d904dbc72f46a9f0ae695796ac3e55bfa1
Cr-Commit-Position: refs/heads/master@{#420430}
==========

[commit-bot: I haz the power, 2016-09-22 19:49:18]

Patchset 2 (id:??) landed as
https://crrev.com/ea1e36d904dbc72f46a9f0ae695796ac3e55bfa1
Cr-Commit-Position: refs/heads/master@{#420430}