media: Quit MojoMediaApplication when no ServiceFactory service is bound.

media: Quit MojoMediaApplication when no ServiceFactory service is bound.

When no ServiceFactory service is bound, we'll quit the mojo media application
to release resources. In particular, when we host the app in the utility
process, this is needed so that the utility process can be killed when nobody
needs the app/services.

BUG=510657
TEST=Adds one unit test. Also manually tested when playing audio in the utility
process using MojoMediaApplication.

Committed: https://crrev.com/5001f4e987257e7a77147f0ed07ebc11215c3231
Cr-Commit-Position: refs/heads/master@{#340613}

[xhwang, 2015-07-22 21:25:57]

xhwang@chromium.org changed reviewers:
+ rockot@chromium.org, sky@chromium.org

[xhwang, 2015-07-22 21:25:57]

xhwang@chromium.org changed required reviewers:
+ rockot@chromium.org

[xhwang, 2015-07-22 21:25:58]

This is an updated version of https://codereview.chromium.org/1200323003/ where
the delayed app destruction part is dropped. After this, I'll investigate the
cost of restarting the utility process and reloading the media app (including
CDM) to see whether we still need delayed app destruction.

rockot: Please review everything.
sky: Please OWNERS review mojo/application/public/cpp/*
jam/dalecurtis: FYI

[Ken Rockot(use gerrit already), 2015-07-22 21:35:56]

lgtm

[sky, 2015-07-22 23:56:33]

https://codereview.chromium.org/1249643004/diff/1/mojo/application/public/cpp...
File mojo/application/public/cpp/application_impl.h (right):

https://codereview.chromium.org/1249643004/diff/1/mojo/application/public/cpp...
mojo/application/public/cpp/application_impl.h:83: // call to
ApplicationConnection::CloseConnection. If |terminate_cb| is
It's not when the app is terminated, but rather when there is a connection error
on the service provider. That said, I don't think this is the right place for
setting the error handler. I think it makes more sense on ApplicationConnection.
And confusingly we shouldn't use base types here. Use the one defined in
third_party/mojo/src/mojo/public/cpp/bindings/callback.h.

[xhwang, 2015-07-23 21:54:39]

comments addressed

[xhwang, 2015-07-23 21:57:15]

sky: PTAL again!

https://codereview.chromium.org/1249643004/diff/1/mojo/application/public/cpp...
File mojo/application/public/cpp/application_impl.h (right):

https://codereview.chromium.org/1249643004/diff/1/mojo/application/public/cpp...
mojo/application/public/cpp/application_impl.h:83: // call to
ApplicationConnection::CloseConnection. If |terminate_cb| is
On 2015/07/22 23:56:33, sky wrote:
> It's not when the app is terminated, but rather when there is a connection
error
> on the service provider. 

Done.

> That said, I don't think this is the right place for
> setting the error handler. I think it makes more sense on
ApplicationConnection.

Done.

> And confusingly we shouldn't use base types here. Use the one defined in
> third_party/mojo/src/mojo/public/cpp/bindings/callback.h.

Done.

https://codereview.chromium.org/1249643004/diff/20001/mojo/application/public...
File mojo/application/public/cpp/lib/service_registry.cc (right):

https://codereview.chromium.org/1249643004/diff/20001/mojo/application/public...
mojo/application/public/cpp/lib/service_registry.cc:50:
remote_service_provider_.set_connection_error_handler(closure);
Now I look at this more closely, does it make sense that when connection error
happens, we also call OnCloseConnection() so that this connection could be
destructed and removed from the ApplicationImpl? Currently when connection error
happens, the connection still sits in the ApplicaionImpl's ServiceRegistryList.

[sky, 2015-07-23 23:09:21]

https://codereview.chromium.org/1249643004/diff/20001/mojo/application/public...
File mojo/application/public/cpp/application_connection.h (right):

https://codereview.chromium.org/1249643004/diff/20001/mojo/application/public...
mojo/application/public/cpp/application_connection.h:105: virtual void
SetConnectionErrorHandlerForTest(const Closure& closure) = 0;
Why is this for test? It's a general need to know when the serviceprovider goes
away. Also, this needs to be named more specifically, eg
SetServiceProviderConnectionErrorHandler().

https://codereview.chromium.org/1249643004/diff/20001/mojo/application/public...
File mojo/application/public/cpp/lib/service_registry.cc (right):

https://codereview.chromium.org/1249643004/diff/20001/mojo/application/public...
mojo/application/public/cpp/lib/service_registry.cc:50:
remote_service_provider_.set_connection_error_handler(closure);
On 2015/07/23 21:57:15, xhwang wrote:
> Now I look at this more closely, does it make sense that when connection error
> happens, we also call OnCloseConnection() so that this connection could be
> destructed and removed from the ApplicationImpl? Currently when connection
error
> happens, the connection still sits in the ApplicaionImpl's
ServiceRegistryList.

I was thinking the same thing, but just because the SP goes away doesn't mean
other vended services have also gone away. Additionally there is no requirement
that an app even provide remote services. In other words, I think the consumer
of the connection shown control the lifetime.

[xhwang, 2015-07-23 23:35:22]

comments addressed

[xhwang, 2015-07-23 23:36:21]

sky: Thanks for the comments. PTAL again!

https://codereview.chromium.org/1249643004/diff/20001/mojo/application/public...
File mojo/application/public/cpp/application_connection.h (right):

https://codereview.chromium.org/1249643004/diff/20001/mojo/application/public...
mojo/application/public/cpp/application_connection.h:105: virtual void
SetConnectionErrorHandlerForTest(const Closure& closure) = 0;
On 2015/07/23 23:09:21, sky wrote:
> Why is this for test? It's a general need to know when the serviceprovider
goes
> away. Also, this needs to be named more specifically, eg
> SetServiceProviderConnectionErrorHandler().

Done.

https://codereview.chromium.org/1249643004/diff/20001/mojo/application/public...
File mojo/application/public/cpp/lib/service_registry.cc (right):

https://codereview.chromium.org/1249643004/diff/20001/mojo/application/public...
mojo/application/public/cpp/lib/service_registry.cc:50:
remote_service_provider_.set_connection_error_handler(closure);
On 2015/07/23 23:09:21, sky wrote:
> On 2015/07/23 21:57:15, xhwang wrote:
> > Now I look at this more closely, does it make sense that when connection
error
> > happens, we also call OnCloseConnection() so that this connection could be
> > destructed and removed from the ApplicationImpl? Currently when connection
> error
> > happens, the connection still sits in the ApplicaionImpl's
> ServiceRegistryList.
> 
> I was thinking the same thing, but just because the SP goes away doesn't mean
> other vended services have also gone away. Additionally there is no
requirement
> that an app even provide remote services. In other words, I think the consumer
> of the connection shown control the lifetime.

Acknowledged.

[sky, 2015-07-24 15:43:13]

sky@chromium.org changed reviewers:
+ ben@chromium.org

[sky, 2015-07-24 15:43:14]

LGTM: but wait for Ben. He was going with a slightly different name for the
ApplicationConnection function you're adding. His patch is here:
https://codereview.chromium.org/1244233002/ .

https://codereview.chromium.org/1249643004/diff/40001/mojo/application/public...
File mojo/application/public/cpp/application_connection.h (right):

https://codereview.chromium.org/1249643004/diff/40001/mojo/application/public...
mojo/application/public/cpp/application_connection.h:103: // Sets a connection
error handler for testing purpose. The |closure| will be
Again, this is not just for testing. Remove 'for testing purpose.'

https://codereview.chromium.org/1249643004/diff/40001/mojo/application/public...
mojo/application/public/cpp/application_connection.h:104: // called when an
connection error happens.
And be more specific. It's not a general connection error handler, it's when the
remote service provider has a connection error.

[xhwang, 2015-07-24 16:40:20]

comments addressed

[xhwang, 2015-07-24 16:40:54]

Updated. Waiting for Ben's comment...

https://codereview.chromium.org/1249643004/diff/40001/mojo/application/public...
File mojo/application/public/cpp/application_connection.h (right):

https://codereview.chromium.org/1249643004/diff/40001/mojo/application/public...
mojo/application/public/cpp/application_connection.h:103: // Sets a connection
error handler for testing purpose. The |closure| will be
On 2015/07/24 15:43:14, sky wrote:
> Again, this is not just for testing. Remove 'for testing purpose.'

Oops. Done.

https://codereview.chromium.org/1249643004/diff/40001/mojo/application/public...
mojo/application/public/cpp/application_connection.h:104: // called when an
connection error happens.
On 2015/07/24 15:43:14, sky wrote:
> And be more specific. It's not a general connection error handler, it's when
the
> remote service provider has a connection error.

Done.

[Ben Goodger (Google), 2015-07-24 17:33:43]

On 2015/07/24 16:40:54, xhwang wrote:
> Updated. Waiting for Ben's comment...
> 
>
https://codereview.chromium.org/1249643004/diff/40001/mojo/application/public...
> File mojo/application/public/cpp/application_connection.h (right):
> 
>
https://codereview.chromium.org/1249643004/diff/40001/mojo/application/public...
> mojo/application/public/cpp/application_connection.h:103: // Sets a connection
> error handler for testing purpose. The |closure| will be
> On 2015/07/24 15:43:14, sky wrote:
> > Again, this is not just for testing. Remove 'for testing purpose.'
> 
> Oops. Done.
> 
>
https://codereview.chromium.org/1249643004/diff/40001/mojo/application/public...
> mojo/application/public/cpp/application_connection.h:104: // called when an
> connection error happens.
> On 2015/07/24 15:43:14, sky wrote:
> > And be more specific. It's not a general connection error handler, it's when
> the
> > remote service provider has a connection error.
> 
> Done.

I have a CL in CQ now that adds a similar method to ApplicationConnection.

[xhwang, 2015-07-24 17:36:42]

On 2015/07/24 17:33:43, Ben Goodger (Google) wrote:
> On 2015/07/24 16:40:54, xhwang wrote:
> > Updated. Waiting for Ben's comment...
> > 
> >
>
https://codereview.chromium.org/1249643004/diff/40001/mojo/application/public...
> > File mojo/application/public/cpp/application_connection.h (right):
> > 
> >
>
https://codereview.chromium.org/1249643004/diff/40001/mojo/application/public...
> > mojo/application/public/cpp/application_connection.h:103: // Sets a
connection
> > error handler for testing purpose. The |closure| will be
> > On 2015/07/24 15:43:14, sky wrote:
> > > Again, this is not just for testing. Remove 'for testing purpose.'
> > 
> > Oops. Done.
> > 
> >
>
https://codereview.chromium.org/1249643004/diff/40001/mojo/application/public...
> > mojo/application/public/cpp/application_connection.h:104: // called when an
> > connection error happens.
> > On 2015/07/24 15:43:14, sky wrote:
> > > And be more specific. It's not a general connection error handler, it's
when
> > the
> > > remote service provider has a connection error.
> > 
> > Done.
> 
> I have a CL in CQ now that adds a similar method to ApplicationConnection.

Cool. I'll wait and then rebase.

[xhwang, 2015-07-25 08:11:18]

rebase only

[xhwang, 2015-07-25 08:12:42]

https://codereview.chromium.org/1249643004/diff/80001/mojo/shell/application_...
File mojo/shell/application_instance.cc (right):

https://codereview.chromium.org/1249643004/diff/80001/mojo/shell/application_...
mojo/shell/application_instance.cc:147: manager->ConnectToApplication(nullptr,
url.Pass(), std::string(),
This fixes the crash in my test. But I am not sure whether it's the correct fix.

[xhwang, 2015-07-27 22:21:35]

rebase only

[xhwang, 2015-07-27 22:26:19]

Rebased and now everything is working!

beng: FYI. I made some trivial fixes in mojo/shell/application_instance.cc.

[xhwang, 2015-07-27 22:41:00]

The CQ bit was checked by xhwang@chromium.org

[xhwang, 2015-07-27 22:41:00]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1249643004/#ps100001
(title: "rebase only")

[commit-bot: I haz the power, 2015-07-27 22:41:25]

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

[commit-bot: I haz the power, 2015-07-28 00:13:15]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-07-28 00:14:00]

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