Simplify the ApplicationLoader interface in preparation for changes.

Simplify the ApplicationLoader interface in preparation for changes.

The ApplicationLoader interface has become very complicated because it
provides a way for clients of ApplicationManager to do two different
things in response to a connection request.

1. Bind to an Application implementation.

2. Delegate back to ApplicationManager with details about a
   ContentHandler to use instead.

ApplicationLoader was originally designed to do only #1. I extended it
to do #2 in the misguided thought that it would be nice for
ApplicationLoader implementations to be able to provide content for a
ContentHandler as well actual applications.

However, this second use case was never needed and adds lots of
complexity.

I think it will simplify things dramatically to move URL fetching and
ContentHandler handling into application manager directly. If we ever
need to generalize that behavior, we can create a separate interface
from ApplicationLoader for it at that time.

This CL is a first step that is pretty mechanical in nature, but I
think is still an improvement. Suggest reviewing in the following
order:

- application_loader.*
- application_manager.*
- everything else

The next change will break apart DynamicApplicationLoader into pieces
which will be used from within ApplicationManager.

R=davemoore@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/bfc1118ae801b4142f4027cd3cb1ab079c325f66

[Aaron Boodman, 2015-02-18 19:30:11]

Patchset #1 (id:1) has been deleted

[Aaron Boodman, 2015-02-18 19:30:20]

aa@chromium.org changed reviewers:
+ davemoore@chromium.org

[qsr, 2015-02-19 09:25:00]

qsr@chromium.org changed reviewers:
+ qsr@chromium.org

[qsr, 2015-02-19 09:25:01]

https://codereview.chromium.org/930243006/diff/80001/shell/application_manage...
File shell/application_manager/application_loader.h (right):

https://codereview.chromium.org/930243006/diff/80001/shell/application_manage...
shell/application_manager/application_loader.h:45: class
MOJO_APPLICATION_MANAGER_EXPORT DynamicApplicationLoader {
As this is temporary, do you want to rename this or the other? Having 2
DynamicApplicationLoader makes it a little bit harder to follow which is which.
I guess calling the other DynamicApplicationLoaderImpl might help. WDYT?

Also, can we get rid of all the _EXPORT?

https://codereview.chromium.org/930243006/diff/80001/shell/application_manage...
shell/application_manager/application_loader.h:47: // Returns a callback that
will should never be called.
s/will //

https://codereview.chromium.org/930243006/diff/80001/shell/application_manage...
File shell/application_manager/application_manager.cc (right):

https://codereview.chromium.org/930243006/diff/80001/shell/application_manage...
shell/application_manager/application_manager.cc:147: services.Pass(),
exposed_services.Pass()).Pass(),
Is the last .Pass() necessary?

https://codereview.chromium.org/930243006/diff/80001/shell/context.cc
File shell/context.cc (right):

https://codereview.chromium.org/930243006/diff/80001/shell/context.cc#newcode261
shell/context.cc:261: new DynamicApplicationLoader(this, runner_factory.Pass());
Maybe:

scoped_ptr<DynamicApplicationLoader> dynamic_application_loader (new
DynamicApplicationLoader(this, runner_factory.Pass()));

and pass it line 263?

[DaveMoore, 2015-02-19 18:58:31]

https://codereview.chromium.org/930243006/diff/80001/shell/application_manage...
File shell/application_manager/application_loader.h (right):

https://codereview.chromium.org/930243006/diff/80001/shell/application_manage...
shell/application_manager/application_loader.h:45: class
MOJO_APPLICATION_MANAGER_EXPORT DynamicApplicationLoader {
On 2015/02/19 09:25:01, qsr wrote:
> As this is temporary, do you want to rename this or the other? Having 2
> DynamicApplicationLoader makes it a little bit harder to follow which is
which.
> I guess calling the other DynamicApplicationLoaderImpl might help. WDYT?
> 
> Also, can we get rid of all the _EXPORT?

Maybe the word "Dynamic" is all wrong. What we're really loading is a
NativeApplication (in our .gn files we call them mojo_native_application). Could
we move towards that sort of naming?

[Aaron Boodman, 2015-02-19 20:38:47]

https://codereview.chromium.org/930243006/diff/80001/shell/application_manage...
File shell/application_manager/application_loader.h (right):

https://codereview.chromium.org/930243006/diff/80001/shell/application_manage...
shell/application_manager/application_loader.h:45: class
MOJO_APPLICATION_MANAGER_EXPORT DynamicApplicationLoader {
On 2015/02/19 18:58:31, DaveMoore wrote:
> On 2015/02/19 09:25:01, qsr wrote:
> > As this is temporary, do you want to rename this or the other? Having 2
> > DynamicApplicationLoader makes it a little bit harder to follow which is
> which.
> > I guess calling the other DynamicApplicationLoaderImpl might help. WDYT?
> > 
> > Also, can we get rid of all the _EXPORT?
> 
> Maybe the word "Dynamic" is all wrong. What we're really loading is a
> NativeApplication (in our .gn files we call them mojo_native_application).
Could
> we move towards that sort of naming?

Done.

https://codereview.chromium.org/930243006/diff/80001/shell/application_manage...
shell/application_manager/application_loader.h:45: class
MOJO_APPLICATION_MANAGER_EXPORT DynamicApplicationLoader {
On 2015/02/19 09:25:01, qsr wrote:
> As this is temporary, do you want to rename this or the other? Having 2
> DynamicApplicationLoader makes it a little bit harder to follow which is
which.
> I guess calling the other DynamicApplicationLoaderImpl might help. WDYT?
> 
> Also, can we get rid of all the _EXPORT?

Done.

https://codereview.chromium.org/930243006/diff/80001/shell/application_manage...
shell/application_manager/application_loader.h:47: // Returns a callback that
will should never be called.
On 2015/02/19 09:25:01, qsr wrote:
> s/will //

Done.

https://codereview.chromium.org/930243006/diff/80001/shell/application_manage...
File shell/application_manager/application_manager.cc (right):

https://codereview.chromium.org/930243006/diff/80001/shell/application_manage...
shell/application_manager/application_manager.cc:147: services.Pass(),
exposed_services.Pass()).Pass(),
On 2015/02/19 09:25:01, qsr wrote:
> Is the last .Pass() necessary?

Done.

https://codereview.chromium.org/930243006/diff/80001/shell/context.cc
File shell/context.cc (right):

https://codereview.chromium.org/930243006/diff/80001/shell/context.cc#newcode261
shell/context.cc:261: new DynamicApplicationLoader(this, runner_factory.Pass());
On 2015/02/19 09:25:01, qsr wrote:
> Maybe:
> 
> scoped_ptr<DynamicApplicationLoader> dynamic_application_loader (new
> DynamicApplicationLoader(this, runner_factory.Pass()));
> 
> and pass it line 263?

Done.

[DaveMoore, 2015-02-19 20:42:04]

lgtm

[Aaron Boodman, 2015-02-19 20:42:27]

Committed patchset #5 (id:100001) manually as
bfc1118ae801b4142f4027cd3cb1ab079c325f66 (presubmit successful).