Pass MojoMediaClient instance to MojoMediaApplication constructor.

Pass MojoMediaClient instance to MojoMediaApplication constructor.
BUG=570508
Committed: https://crrev.com/e8f09fd119cacc6c036de2f041313d64971873e0
Cr-Commit-Position: refs/heads/master@{#365883}

[alokp, 2015-12-16 19:37:25]

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

[xhwang, 2015-12-16 21:30:31]

looking good. just some nits and comments.

https://codereview.chromium.org/1529063004/diff/1/media/mojo/services/mojo_me...
File media/mojo/services/mojo_media_application.cc (right):

https://codereview.chromium.org/1529063004/diff/1/media/mojo/services/mojo_me...
media/mojo/services/mojo_media_application.cc:30:
media_client_(media_client.Pass()),
replace Pass() with std::move

https://codereview.chromium.org/1529063004/diff/1/media/mojo/services/mojo_me...
media/mojo/services/mojo_media_application.cc:62: media_client_.get());
ServiceFactoryImpl is owned by the pipe but media_client_ is owned by the
MojoMediaApplication. How does the lifetime work here to ensure this pointer is
always valid?

Probably add a comment that since ServiceFactoryImpl owns a ref to the
AppRefCount, this is safe etc.

https://codereview.chromium.org/1529063004/diff/1/media/mojo/services/mojo_me...
File media/mojo/services/mojo_media_application.h (right):

https://codereview.chromium.org/1529063004/diff/1/media/mojo/services/mojo_me...
media/mojo/services/mojo_media_application.h:22: explicit
MojoMediaApplication(bool enable_logging,
remove explicit

https://codereview.chromium.org/1529063004/diff/1/media/mojo/services/mojo_me...
media/mojo/services/mojo_media_application.h:37: scoped_ptr<MojoMediaClient>
media_client_;
s/media_client_/mojo_media_client_ since we have a separate thing called
MediaClient in media/

https://codereview.chromium.org/1529063004/diff/1/media/mojo/services/mojo_me...
File media/mojo/services/mojo_media_client.h (right):

https://codereview.chromium.org/1529063004/diff/1/media/mojo/services/mojo_me...
media/mojo/services/mojo_media_client.h:62: scoped_ptr<PlatformMojoMediaClient>
impl_;
What's your plan about MojoMediaClient::Create() in the chromecast world? I
never really liked the structure here where we have a different
PlatformMojoMediaClient interface. 

With your changes, maybe we can just remove PlatformMojoMediaClient and let each
platform create it's MojoMediaClient directly?

https://codereview.chromium.org/1529063004/diff/1/media/mojo/services/mojo_me...
media/mojo/services/mojo_media_client.h:62: scoped_ptr<PlatformMojoMediaClient>
impl_;
BTW, |impl| doesn't match the class name, maybe |platform_client_| if you want
to keep PlatformMojoMediaClient.

[alokp, 2015-12-16 22:57:30]

https://codereview.chromium.org/1529063004/diff/1/media/mojo/services/mojo_me...
File media/mojo/services/mojo_media_application.cc (right):

https://codereview.chromium.org/1529063004/diff/1/media/mojo/services/mojo_me...
media/mojo/services/mojo_media_application.cc:30:
media_client_(media_client.Pass()),
On 2015/12/16 21:30:31, xhwang wrote:
> replace Pass() with std::move

fancy! Done.

https://codereview.chromium.org/1529063004/diff/1/media/mojo/services/mojo_me...
media/mojo/services/mojo_media_application.cc:62: media_client_.get());
On 2015/12/16 21:30:31, xhwang wrote:
> ServiceFactoryImpl is owned by the pipe but media_client_ is owned by the
> MojoMediaApplication. How does the lifetime work here to ensure this pointer
is
> always valid?
> 
> Probably add a comment that since ServiceFactoryImpl owns a ref to the
> AppRefCount, this is safe etc.

A service cannot outlive the host application, right?

https://codereview.chromium.org/1529063004/diff/1/media/mojo/services/mojo_me...
File media/mojo/services/mojo_media_application.h (right):

https://codereview.chromium.org/1529063004/diff/1/media/mojo/services/mojo_me...
media/mojo/services/mojo_media_application.h:22: explicit
MojoMediaApplication(bool enable_logging,
On 2015/12/16 21:30:31, xhwang wrote:
> remove explicit

Done.

https://codereview.chromium.org/1529063004/diff/1/media/mojo/services/mojo_me...
media/mojo/services/mojo_media_application.h:37: scoped_ptr<MojoMediaClient>
media_client_;
On 2015/12/16 21:30:31, xhwang wrote:
> s/media_client_/mojo_media_client_ since we have a separate thing called
> MediaClient in media/

Done. Although this is a private member variable.

https://codereview.chromium.org/1529063004/diff/1/media/mojo/services/mojo_me...
File media/mojo/services/mojo_media_client.h (right):

https://codereview.chromium.org/1529063004/diff/1/media/mojo/services/mojo_me...
media/mojo/services/mojo_media_client.h:62: scoped_ptr<PlatformMojoMediaClient>
impl_;
On 2015/12/16 21:30:31, xhwang wrote:
> BTW, |impl| doesn't match the class name, maybe |platform_client_| if you want
> to keep PlatformMojoMediaClient.

Done.

https://codereview.chromium.org/1529063004/diff/1/media/mojo/services/mojo_me...
media/mojo/services/mojo_media_client.h:62: scoped_ptr<PlatformMojoMediaClient>
impl_;
On 2015/12/16 21:30:31, xhwang wrote:
> What's your plan about MojoMediaClient::Create() in the chromecast world? I
> never really liked the structure here where we have a different
> PlatformMojoMediaClient interface. 
> 
> With your changes, maybe we can just remove PlatformMojoMediaClient and let
each
> platform create it's MojoMediaClient directly?

I could make MojoMediaClient an abstract class and let each client implement
MojoMediaClient::Create similar to how they implement
CreatePlatformMojoMediaClient right now. wdyt?

[xhwang, 2015-12-16 23:43:36]

lgtm % nit

Thanks!

https://codereview.chromium.org/1529063004/diff/1/media/mojo/services/mojo_me...
File media/mojo/services/mojo_media_application.cc (right):

https://codereview.chromium.org/1529063004/diff/1/media/mojo/services/mojo_me...
media/mojo/services/mojo_media_application.cc:62: media_client_.get());
On 2015/12/16 22:57:29, alokp wrote:
> On 2015/12/16 21:30:31, xhwang wrote:
> > ServiceFactoryImpl is owned by the pipe but media_client_ is owned by the
> > MojoMediaApplication. How does the lifetime work here to ensure this pointer
> is
> > always valid?
> > 
> > Probably add a comment that since ServiceFactoryImpl owns a ref to the
> > AppRefCount, this is safe etc.
> 
> A service cannot outlive the host application, right?

Acknowledged.

https://codereview.chromium.org/1529063004/diff/1/media/mojo/services/mojo_me...
File media/mojo/services/mojo_media_client.h (right):

https://codereview.chromium.org/1529063004/diff/1/media/mojo/services/mojo_me...
media/mojo/services/mojo_media_client.h:62: scoped_ptr<PlatformMojoMediaClient>
impl_;
On 2015/12/16 22:57:30, alokp wrote:
> On 2015/12/16 21:30:31, xhwang wrote:
> > What's your plan about MojoMediaClient::Create() in the chromecast world? I
> > never really liked the structure here where we have a different
> > PlatformMojoMediaClient interface. 
> > 
> > With your changes, maybe we can just remove PlatformMojoMediaClient and let
> each
> > platform create it's MojoMediaClient directly?
> 
> I could make MojoMediaClient an abstract class and let each client implement
> MojoMediaClient::Create similar to how they implement
> CreatePlatformMojoMediaClient right now. wdyt?

Yes, that's what I meant. But feel free to do it in a separate CL :)

https://codereview.chromium.org/1529063004/diff/20001/media/mojo/services/moj...
File media/mojo/services/mojo_media_client.cc (right):

https://codereview.chromium.org/1529063004/diff/20001/media/mojo/services/moj...
media/mojo/services/mojo_media_client.cc:64: :
platform_client_(platform_client.Pass()) {
nit: std::move

[alokp, 2015-12-16 23:57:43]

https://codereview.chromium.org/1529063004/diff/20001/media/mojo/services/moj...
File media/mojo/services/mojo_media_client.cc (right):

https://codereview.chromium.org/1529063004/diff/20001/media/mojo/services/moj...
media/mojo/services/mojo_media_client.cc:64: :
platform_client_(platform_client.Pass()) {
On 2015/12/16 23:43:36, xhwang wrote:
> nit: std::move

Done.

[alokp, 2015-12-16 23:59:05]

The CQ bit was checked by alokp@chromium.org

[alokp, 2015-12-16 23:59:06]

The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org
Link to the patchset: https://codereview.chromium.org/1529063004/#ps40001
(title: "used std::move instead of Pass")

[commit-bot: I haz the power, 2015-12-17 00:03:20]

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

[commit-bot: I haz the power, 2015-12-17 00:37:41]

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

[commit-bot: I haz the power, 2015-12-17 00:37:42]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
  android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[alokp, 2015-12-17 01:39:39]

The CQ bit was checked by alokp@chromium.org

[alokp, 2015-12-17 01:39:40]

The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org
Link to the patchset: https://codereview.chromium.org/1529063004/#ps60001
(title: "fixed compile error")

[commit-bot: I haz the power, 2015-12-17 02:36:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1529063004/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1529063004/60001

[commit-bot: I haz the power, 2015-12-17 04:35:01]

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

[commit-bot: I haz the power, 2015-12-17 04:35:02]

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

[alokp1, 2015-12-17 18:17:17]

On 2015/12/17 04:35:02, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

Xiaohan: media_apptests were failing because AudioManager was being created
before AtExitManager. I added MojoMediaClient::Initialize to postpone it. PTAL.

[alokp1, 2015-12-17 18:19:50]

Description was changed from

==========
Pass MojoMediaClient instance to MojoMediaApplication constructor.
==========

to

==========
Pass MojoMediaClient instance to MojoMediaApplication constructor.
BUG=570508
==========

[xhwang, 2015-12-17 19:22:53]

lgtm++

[alokp1, 2015-12-17 19:54:09]

The CQ bit was checked by alokp@google.com

[commit-bot: I haz the power, 2015-12-17 19:56:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1529063004/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1529063004/100001

[commit-bot: I haz the power, 2015-12-17 21:13:52]

Description was changed from

==========
Pass MojoMediaClient instance to MojoMediaApplication constructor.
BUG=570508
==========

to

==========
Pass MojoMediaClient instance to MojoMediaApplication constructor.
BUG=570508
==========

[commit-bot: I haz the power, 2015-12-17 21:13:53]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-12-17 21:14:55]

Description was changed from

==========
Pass MojoMediaClient instance to MojoMediaApplication constructor.
BUG=570508
==========

to

==========
Pass MojoMediaClient instance to MojoMediaApplication constructor.
BUG=570508

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

[commit-bot: I haz the power, 2015-12-17 21:14:56]

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