Get rid of KeepAlive. Quit shell when all urls run directly by Context are closed.

Get rid of KeepAlive.  Quit shell  when all urls run directly by Context are closed.

BUG=

Committed: https://crrev.com/9567e664440a3d550d3e5819dbeca544953f7e7f
Cr-Commit-Position: refs/heads/master@{#291723}

[DaveMoore, 2014-08-22 22:13:33]

Cleanup

[DaveMoore, 2014-08-22 22:15:46]

This also ensures that all threads associated with running DSOs are joined
before the ApplicationManager is destroyed (and therefore before the shell
exits). This provides a much more consistent environment for tests.

[tim (not reviewing), 2014-08-22 22:24:21]

Cool!  Will take a look asap. One off-the-cuff comment below.

https://codereview.chromium.org/491443005/diff/20001/mojo/shell/shell_test_ba...
File mojo/shell/shell_test_base.cc (left):

https://codereview.chromium.org/491443005/diff/20001/mojo/shell/shell_test_ba...
mojo/shell/shell_test_base.cc:40: test_server_->base_url());
This was the key to using NetworkService rather than loading local libraries. 
The test probably still passes without it, but I don't think it's taking the
desired path.

[DaveMoore, 2014-08-22 22:48:00]

Bring back ConnectToServiceViaNetwork

[DaveMoore, 2014-08-22 22:48:35]

https://codereview.chromium.org/491443005/diff/20001/mojo/shell/shell_test_ba...
File mojo/shell/shell_test_base.cc (left):

https://codereview.chromium.org/491443005/diff/20001/mojo/shell/shell_test_ba...
mojo/shell/shell_test_base.cc:40: test_server_->base_url());
Whoops. I changed that accidentally. It's back now.

On 2014/08/22 22:24:21, timsteele wrote:
> This was the key to using NetworkService rather than loading local libraries. 
> The test probably still passes without it, but I don't think it's taking the
> desired path.

[tim (not reviewing), 2014-08-23 00:14:27]

looks nice.

https://codereview.chromium.org/491443005/diff/40001/mojo/application_manager...
File mojo/application_manager/application_manager.h (right):

https://codereview.chromium.org/491443005/diff/40001/mojo/application_manager...
mojo/application_manager/application_manager.h:25: virtual void
OnServiceError(const GURL& url) = 0;
Please add a comment describing what a service error means here. Also, should
this be OnApplicationError?

https://codereview.chromium.org/491443005/diff/40001/mojo/application_manager...
mojo/application_manager/application_manager.h:26: };
Should have a virtual ~Delegate... (protected: if you like).

https://codereview.chromium.org/491443005/diff/40001/mojo/application_manager...
mojo/application_manager/application_manager.h:78: void set_delegate(Delegate*
delegate) { delegate_ = delegate; }
Do we need to support the NULL-delegate case or could this be a constructor
parameter?

https://codereview.chromium.org/491443005/diff/40001/mojo/shell/context.h
File mojo/shell/context.h (right):

https://codereview.chromium.org/491443005/diff/40001/mojo/shell/context.h#new...
mojo/shell/context.h:39: const GURL& application_url,
nit - indent two more spaces.

https://codereview.chromium.org/491443005/diff/40001/mojo/shell/dynamic_appli...
File mojo/shell/dynamic_application_loader.cc (right):

https://codereview.chromium.org/491443005/diff/40001/mojo/shell/dynamic_appli...
mojo/shell/dynamic_application_loader.cc:151: base::Unretained(runner.get()),
Hm. So if I have this right, a DynamicServiceRunner could be destroyed either a)
by OnRunLibraryComplete or b) by ~DynamicApplicationLoader. If we use
ApplicationManager::TerminateShellConnections when shutting down though,
shouldn't it be possible to say that the Runners are *always* destroyed by
OnRunLibraryComplete after the library exits (because Terminate "forces" the
libraries to exit the standard way)?

In that case, maybe you could you use base::Passed(runner) here so that
OnRunLibraryComplete accepts a scoped_ptr<DynamicServiceRunner> and avoid the
need for |runners_| altogether?

https://codereview.chromium.org/491443005/diff/40001/mojo/shell/dynamic_appli...
File mojo/shell/dynamic_application_loader.h (right):

https://codereview.chromium.org/491443005/diff/40001/mojo/shell/dynamic_appli...
mojo/shell/dynamic_application_loader.h:59: void
RunLibraryComplete(DynamicServiceRunner* runner,
nit: OnRunLibraryComplete

[DaveMoore, 2014-08-24 22:30:49]

Review nits

[DaveMoore, 2014-08-24 22:31:11]

https://codereview.chromium.org/491443005/diff/40001/mojo/application_manager...
File mojo/application_manager/application_manager.h (right):

https://codereview.chromium.org/491443005/diff/40001/mojo/application_manager...
mojo/application_manager/application_manager.h:25: virtual void
OnServiceError(const GURL& url) = 0;
On 2014/08/23 00:14:26, timsteele wrote:
> Please add a comment describing what a service error means here. Also, should
> this be OnApplicationError?

Done and Done.

https://codereview.chromium.org/491443005/diff/40001/mojo/application_manager...
mojo/application_manager/application_manager.h:26: };
On 2014/08/23 00:14:27, timsteele wrote:
> Should have a virtual ~Delegate... (protected: if you like).

Done.

https://codereview.chromium.org/491443005/diff/40001/mojo/application_manager...
mojo/application_manager/application_manager.h:78: void set_delegate(Delegate*
delegate) { delegate_ = delegate; }
I don't think so. It's up to the creator of the ApplicationManager to decide if
they want to respond to
errors or not.
On 2014/08/23 00:14:27, timsteele wrote:
> Do we need to support the NULL-delegate case or could this be a constructor
> parameter?

https://codereview.chromium.org/491443005/diff/40001/mojo/shell/context.h
File mojo/shell/context.h (right):

https://codereview.chromium.org/491443005/diff/40001/mojo/shell/context.h#new...
mojo/shell/context.h:39: const GURL& application_url,
On 2014/08/23 00:14:27, timsteele wrote:
> nit - indent two more spaces.

Done.

https://codereview.chromium.org/491443005/diff/40001/mojo/shell/dynamic_appli...
File mojo/shell/dynamic_application_loader.cc (right):

https://codereview.chromium.org/491443005/diff/40001/mojo/shell/dynamic_appli...
mojo/shell/dynamic_application_loader.cc:151: base::Unretained(runner.get()),
We could if we made TerminateShellConnections block on the Joins(). I thought it
was cleaner to have the DAL own the DSR reference and be the only thing that
deleted it (through the scoped_ptr).

On 2014/08/23 00:14:27, timsteele wrote:
> Hm. So if I have this right, a DynamicServiceRunner could be destroyed either
a)
> by OnRunLibraryComplete or b) by ~DynamicApplicationLoader. If we use
> ApplicationManager::TerminateShellConnections when shutting down though,
> shouldn't it be possible to say that the Runners are *always* destroyed by
> OnRunLibraryComplete after the library exits (because Terminate "forces" the
> libraries to exit the standard way)?
> 
> In that case, maybe you could you use base::Passed(runner) here so that
> OnRunLibraryComplete accepts a scoped_ptr<DynamicServiceRunner> and avoid the
> need for |runners_| altogether?

https://codereview.chromium.org/491443005/diff/40001/mojo/shell/dynamic_appli...
File mojo/shell/dynamic_application_loader.h (right):

https://codereview.chromium.org/491443005/diff/40001/mojo/shell/dynamic_appli...
mojo/shell/dynamic_application_loader.h:59: void
RunLibraryComplete(DynamicServiceRunner* runner,
On 2014/08/23 00:14:27, timsteele wrote:
> nit: OnRunLibraryComplete

Done.

[tim (not reviewing), 2014-08-25 16:56:47]

LGTM assuming what I said below makes sense.

https://codereview.chromium.org/491443005/diff/40001/mojo/shell/dynamic_appli...
File mojo/shell/dynamic_application_loader.cc (right):

https://codereview.chromium.org/491443005/diff/40001/mojo/shell/dynamic_appli...
mojo/shell/dynamic_application_loader.cc:151: base::Unretained(runner.get()),
Your patch makes it so we won't Quit the shell MessageLoop until no apps remain,
so that's an implicit wait right?  I guess the issue is the other apps that
you're not tracking with app_urls in the context?

On 2014/08/24 22:31:11, DaveMoore wrote:
> We could if we made TerminateShellConnections block on the Joins(). I thought
it
> was cleaner to have the DAL own the DSR reference and be the only thing that
> deleted it (through the scoped_ptr).
> 
> On 2014/08/23 00:14:27, timsteele wrote:
> > Hm. So if I have this right, a DynamicServiceRunner could be destroyed
either
> a)
> > by OnRunLibraryComplete or b) by ~DynamicApplicationLoader. If we use
> > ApplicationManager::TerminateShellConnections when shutting down though,
> > shouldn't it be possible to say that the Runners are *always* destroyed by
> > OnRunLibraryComplete after the library exits (because Terminate "forces" the
> > libraries to exit the standard way)?
> > 
> > In that case, maybe you could you use base::Passed(runner) here so that
> > OnRunLibraryComplete accepts a scoped_ptr<DynamicServiceRunner> and avoid
the
> > need for |runners_| altogether?
>

[DaveMoore, 2014-08-25 16:58:08]

The CQ bit was checked by davemoore@chromium.org

[commit-bot: I haz the power, 2014-08-25 16:58:33]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/davemoore@chromium.org/491443005/60001

[commit-bot: I haz the power, 2014-08-25 17:52:53]

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

[commit-bot: I haz the power, 2014-08-25 18:34:13]

Committed patchset #4 (60001) as be3787d50f2ab74e1767462ea01cceb470b08549

[DaveMoore, 2014-09-02 22:00:48]

Break out both NVS and VM

[DaveMoore, 2014-09-02 22:13:44]

Remove workaround for thunks

[commit-bot: I haz the power, 2014-09-10 02:36:03]

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