mojo: allow BackgroundServiceLoader-loaded apps to Quit themselves.

mojo: allow BackgroundServiceLoader-loaded apps to Quit themselves.

You can't Quit() the MessageLoop of a base::Thread. This CL makes
BackgroundServiceLoader use DelegateSimpleThread and run a MessageLoop
instead of using base::Thread, so that applications can safely Quit()
their current MessageLoop as usual to signify that they are done.

This shouldn't be necessary for the desktop shell once services are moved
out (network, NVS), but we'll likely continue to use this loader on android.

Note: the quit_on_shutdown() method is being removed in https://codereview.chromium.org/394903005/

BUG=

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=287475

[tim (not reviewing), 2014-07-31 17:39:19]

(+ben fyi as we were chatting about this yesterday)

[sky, 2014-07-31 20:59:33]

https://codereview.chromium.org/437493002/diff/100001/mojo/service_manager/ba...
File mojo/service_manager/background_shell_service_loader.cc (right):

https://codereview.chromium.org/437493002/diff/100001/mojo/service_manager/ba...
mojo/service_manager/background_shell_service_loader.cc:15:
base::MessageLoop::current()->Quit();
Use a RunLoop.

https://codereview.chromium.org/437493002/diff/100001/mojo/service_manager/ba...
mojo/service_manager/background_shell_service_loader.cc:59:
message_loop_created_.Wait();
Why do we need to block here? Or for that matter start the thread?

https://codereview.chromium.org/437493002/diff/100001/mojo/service_manager/ba...
mojo/service_manager/background_shell_service_loader.cc:90:
scoped_ptr<base::MessageLoop> message_loop;
You should be able to use MessageLoop(type) here. All the subclasses are the
same size (there are compiler asserts to this effect).

https://codereview.chromium.org/437493002/diff/100001/mojo/service_manager/ba...
File mojo/service_manager/background_shell_service_loader.h (right):

https://codereview.chromium.org/437493002/diff/100001/mojo/service_manager/ba...
mojo/service_manager/background_shell_service_loader.h:40: private:
nit: newline between 39 and 40.

[tim (not reviewing), 2014-07-31 22:17:28]

https://codereview.chromium.org/437493002/diff/100001/mojo/service_manager/ba...
File mojo/service_manager/background_shell_service_loader.cc (right):

https://codereview.chromium.org/437493002/diff/100001/mojo/service_manager/ba...
mojo/service_manager/background_shell_service_loader.cc:15:
base::MessageLoop::current()->Quit();
On 2014/07/31 20:59:33, sky wrote:
> Use a RunLoop.

Done, assuming you meant Run() a RunLoop in BackgroundShellServiceLoader::Run()
from the spawned thread, stash a QuitClosure on the main thread that we Post to
the task_runner in the dtor...

https://codereview.chromium.org/437493002/diff/100001/mojo/service_manager/ba...
mojo/service_manager/background_shell_service_loader.cc:59:
message_loop_created_.Wait();
On 2014/07/31 20:59:33, sky wrote:
> Why do we need to block here? Or for that matter start the thread?

DelegateSimpleThread requires that it is both Started and Joined -- you can't
allocate one without Start()ing it. Once we start it, if we don't Wait then we
have to worry about the semi-initialized case all over the place, where we've
started the thread but haven't initialized task_runner_ and can't PostTask (for
another LoadService call, or in dtor). Blocking ensures anything can be safely
called and handled in a single way after construction.

I could make the DST member a scoped_ptr member, only allocate in LoadService,
and do the Wait() there. I could do-away with |loaded_service_| too.  Done.

https://codereview.chromium.org/437493002/diff/100001/mojo/service_manager/ba...
mojo/service_manager/background_shell_service_loader.cc:90:
scoped_ptr<base::MessageLoop> message_loop;
On 2014/07/31 20:59:33, sky wrote:
> You should be able to use MessageLoop(type) here. All the subclasses are the
> same size (there are compiler asserts to this effect).

Ah.. forgot about that.  Thanks!

https://codereview.chromium.org/437493002/diff/100001/mojo/service_manager/ba...
File mojo/service_manager/background_shell_service_loader.h (right):

https://codereview.chromium.org/437493002/diff/100001/mojo/service_manager/ba...
mojo/service_manager/background_shell_service_loader.h:40: private:
On 2014/07/31 20:59:33, sky wrote:
> nit: newline between 39 and 40.

Done.

[sky, 2014-08-04 18:56:25]

https://codereview.chromium.org/437493002/diff/100001/mojo/service_manager/ba...
File mojo/service_manager/background_shell_service_loader.cc (right):

https://codereview.chromium.org/437493002/diff/100001/mojo/service_manager/ba...
mojo/service_manager/background_shell_service_loader.cc:15:
base::MessageLoop::current()->Quit();
On 2014/07/31 22:17:28, timsteele wrote:
> On 2014/07/31 20:59:33, sky wrote:
> > Use a RunLoop.
> 
> Done, assuming you meant Run() a RunLoop in
BackgroundShellServiceLoader::Run()
> from the spawned thread, stash a QuitClosure on the main thread that we Post
to
> the task_runner in the dtor...

I was actually think you use the base::RunLoop you have on the stack at line 88
a member and Quit() it.

[tim (not reviewing), 2014-08-04 19:51:29]

https://codereview.chromium.org/437493002/diff/100001/mojo/service_manager/ba...
File mojo/service_manager/background_shell_service_loader.cc (right):

https://codereview.chromium.org/437493002/diff/100001/mojo/service_manager/ba...
mojo/service_manager/background_shell_service_loader.cc:15:
base::MessageLoop::current()->Quit();
On 2014/08/04 18:56:25, sky wrote:
> On 2014/07/31 22:17:28, timsteele wrote:
> > On 2014/07/31 20:59:33, sky wrote:
> > > Use a RunLoop.
> > 
> > Done, assuming you meant Run() a RunLoop in
> BackgroundShellServiceLoader::Run()
> > from the spawned thread, stash a QuitClosure on the main thread that we Post
> to
> > the task_runner in the dtor...
> 
> I was actually think you use the base::RunLoop you have on the stack at line
88
> a member and Quit() it.

I see. It'd have to be heap allocated pointer in that case, since ServiceLoader
is constructed on the shell thread.  I was trying to keep as many vars as
possible that live on the background thread scoped to Run(). In particular the
MessageLoop and RunLoop so we don't have to manage and poke at them from
multiple threads...

[sky, 2014-08-04 20:35:09]

All the various MessageLoop quit functions are deprecated though. Seems best to
post a task to the background thread that quits the runloop that was created.

[tim (not reviewing), 2014-08-04 20:43:55]

On 2014/08/04 20:35:09, sky wrote:
> All the various MessageLoop quit functions are deprecated though. Seems best
to
> post a task to the background thread that quits the runloop that was created.

Right, that's what the latest patchset is doing though. I keep a pointer to a
TaskRunner for the background thread and post the non-deprecated RunLoop
QuitClosure to it. (but yes, my original patchset was using deprecated methods.)

[sky, 2014-08-04 21:49:17]

My mistake, I look at the code and read 'loop.QuitClosure' to be the message
loop, which isn't the case. LGTM

[tim (not reviewing), 2014-08-04 21:57:12]

The CQ bit was checked by tim@chromium.org

[commit-bot: I haz the power, 2014-08-04 21:58:03]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tim@chromium.org/437493002/140001

[commit-bot: I haz the power, 2014-08-04 22:07:31]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tri...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-04 22:09:36]

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

[commit-bot: I haz the power, 2014-08-04 22:09:37]

Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[tim (not reviewing), 2014-08-04 22:12:32]

The CQ bit was checked by tim@chromium.org

[commit-bot: I haz the power, 2014-08-04 22:15:22]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tim@chromium.org/437493002/160001

[commit-bot: I haz the power, 2014-08-05 00:09:58]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/37249)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)

[commit-bot: I haz the power, 2014-08-05 00:14:59]

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

[commit-bot: I haz the power, 2014-08-05 00:15:00]

Try jobs failed on following builders:
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[tim (not reviewing), 2014-08-05 00:17:43]

The CQ bit was checked by tim@chromium.org

[commit-bot: I haz the power, 2014-08-05 00:20:07]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tim@chromium.org/437493002/160001

[commit-bot: I haz the power, 2014-08-05 00:52:23]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[commit-bot: I haz the power, 2014-08-05 01:02:08]

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

[commit-bot: I haz the power, 2014-08-05 01:02:09]

Try jobs failed on following builders:
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[tim (not reviewing), 2014-08-05 06:27:33]

The CQ bit was checked by tim@chromium.org

[commit-bot: I haz the power, 2014-08-05 06:28:50]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tim@chromium.org/437493002/160001

[commit-bot: I haz the power, 2014-08-05 07:34:02]

Change committed as 287475