mojo: Add ExistingThreadLoader.

mojo: Add ExistingThreadLoader.

BUG=
TEST=

[xhwang, 2016-04-05 07:10:31]

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

[xhwang, 2016-04-05 07:10:31]

This is a prototype CL for discussion. After having done this I feel we should
at least merge CurrentThreadLoader and ExistingThreadLoader.

The test I did is in https://chromiumcodereview.appspot.com/1857103003/

[alokp, 2016-04-05 15:33:56]

Looks pretty good. I do agree that StaticLoader and CurrentThreadLoader can be
merged with this one into a single Loader implementation.

One of the tricky things I discovered when using an existing thread was that the
usage of mojo::MessageLoopRefFactory, which seems to be implemented on the
assumption that the mojo application has a dedicated message loop. I am not sure
how the clients can use MessageLoopRefFactory when loading apps on  an existing
thread shared with other code.

https://chromiumcodereview.appspot.com/1856313002/diff/1/content/common/mojo/...
File content/common/mojo/existing_thread_loader.cc (right):

https://chromiumcodereview.appspot.com/1856313002/diff/1/content/common/mojo/...
content/common/mojo/existing_thread_loader.cc:34: factory_ =
ApplicationFactory();
what is the need for clearing the fatory_?

[Ken Rockot(use gerrit already), 2016-04-06 14:41:56]

On 2016/04/05 at 15:33:56, alokp wrote:
> Looks pretty good. I do agree that StaticLoader and CurrentThreadLoader can be
merged with this one into a single Loader implementation.

I would agree with this too, but I don't see the point in doing that work if
we're going to delete all of them soon.

> 
> One of the tricky things I discovered when using an existing thread was that
the usage of mojo::MessageLoopRefFactory, which seems to be implemented on the
assumption that the mojo application has a dedicated message loop. I am not sure
how the clients can use MessageLoopRefFactory when loading apps on  an existing
thread shared with other code.

They shouldn't use this if they don't own the MessageLoop. "Quitting" an app
really just means disconnecting its ShellClient and/or Connector pipes.

> 
>
https://chromiumcodereview.appspot.com/1856313002/diff/1/content/common/mojo/...
> File content/common/mojo/existing_thread_loader.cc (right):
> 
>
https://chromiumcodereview.appspot.com/1856313002/diff/1/content/common/mojo/...
> content/common/mojo/existing_thread_loader.cc:34: factory_ =
ApplicationFactory();
> what is the need for clearing the fatory_?

I think this is just cargo-culted from CurrentThreadLoader to prevent the
factory from being run more than once. Not sure it's desirable, and in any case
it should just be factory_.Reset().

[alokp, 2016-04-26 18:47:46]

Ken recently added EmbeddedApplicationRunner to accept an existing task runner,
which eliminates the need for this patch:
https://code.google.com/p/chromium/codesearch#chromium/src/content/common/moj...

However, it seems it is still not possible to specify the application task
runner when registering a mojo app using
ContentBrowserClient::RegisterInProcessMojoApplications.

Ken: Am I missing something?

[Ken Rockot(use gerrit already), 2016-04-26 19:16:51]

On 2016/04/26 at 18:47:46, alokp wrote:
> Ken recently added EmbeddedApplicationRunner to accept an existing task
runner, which eliminates the need for this patch:
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/common/moj...
> 
> However, it seems it is still not possible to specify the application task
runner when registering a mojo app using
ContentBrowserClient::RegisterInProcessMojoApplications.
> 
> Ken: Am I missing something?

I didn't plumb that option through to the ContentBrowserClient interface because
it wasn't needed for anything we have today. Seems reasonable to modify the
interface accordingly if needed. Maybe StaticMojoApplicationMap could hold an
EmbeddedApplicationRunner factory instead of a ShellClient factory, or (my
preference) hold a TaskRunner in addition to its ShellClient factory.

[alokp, 2016-04-27 16:44:02]

On 2016/04/26 19:16:51, Ken Rockot wrote:
> On 2016/04/26 at 18:47:46, alokp wrote:
> > Ken recently added EmbeddedApplicationRunner to accept an existing task
> runner, which eliminates the need for this patch:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/common/moj...
> > 
> > However, it seems it is still not possible to specify the application task
> runner when registering a mojo app using
> ContentBrowserClient::RegisterInProcessMojoApplications.
> > 
> > Ken: Am I missing something?
> 
> I didn't plumb that option through to the ContentBrowserClient interface
because
> it wasn't needed for anything we have today. Seems reasonable to modify the
> interface accordingly if needed. Maybe StaticMojoApplicationMap could hold an
> EmbeddedApplicationRunner factory instead of a ShellClient factory, or (my
> preference) hold a TaskRunner in addition to its ShellClient factory.

OK. I will look into adding the task runner to StaticMojoApplicationMap. Thanks
for your response.

[xhwang, 2016-12-02 17:34:43]

Description was changed from

==========
mojo: Add ExistingThreadLoader.

BUG=
TEST=
==========

to

==========
mojo: Add ExistingThreadLoader.

BUG=
TEST=
==========