Embed a mojo ApplicationManager in content/browser

Embed a mojo ApplicationManager in content/browser

This embeds mojo/shell's ApplicationManager in content/browser
and provides a way for arbitrary browser code to connect to
Mojo apps as if the browser itself were a Mojo app.

This is a basic implementation of Mojo app support which only
loads static apps either in the browser process or a (per-app)
utility process.

Future CLs will address connection to apps from arbitrary render
frames (i.e. connection requests which include the requestor's
origin) as well as refactoring the utility process code further
so that it serves strictly as a Mojo app runner.

BUG=492422
Committed: https://crrev.com/b814a5850da5aa473ad526eef4b41a82b05037a0
Cr-Commit-Position: refs/heads/master@{#332974}

[Ken Rockot(use gerrit already), 2015-05-27 00:12:21]

rockot@chromium.org changed reviewers:
+ jam@chromium.org

[Ken Rockot(use gerrit already), 2015-05-27 00:12:23]

Please take a look.

Aside from all the build file noise, the basic gist of this
patch, which follows the original proposal (linked in bug), is:

content/browser:
  * Add MojoShellContext to host an Application Manager.
    ContentBrowserClient can register URL->app mappings
    for in-process apps. Otherwise the shell context
    delegates to the utility process via ProcessControl.
  * Add MojoAppConnection to let browser code connect to
    apps by URL. mojo_shell_browsertest.cc uses this as
    an example.

content/public/common:
  * Add StaticMojoApplicationLoader and 
    ApplicationRegistry so app embedders can register their
    apps with Chrome via ContentBrowserClient and
    ContentUtilityClient.
  * Add ProcessControl interfaces for browser-to-child
    app management.

content/utility:
  * Add a ProcessControl impl to load apps.
  * Wire it to ServiceRegistry
  * Support app URL registration in ContentUtilityClient.

Note that I added a TestContentUtilityClient to content/test
so that browser test code has an accessible type it can
use to implement a utility client for in-process utility
threads.

https://codereview.chromium.org/1149833007/diff/1/content/browser/mojo_shell_...
File content/browser/mojo_shell_browsertest.cc (right):

https://codereview.chromium.org/1149833007/diff/1/content/browser/mojo_shell_...
content/browser/mojo_shell_browsertest.cc:151:
IN_PROC_BROWSER_TEST_F(MojoShellTest, TestUtilityConnection) {
I'm not terribly happy with the fact that this actually uses an in-process
utility thread rather than a real utility process, but I was having trouble
coming up with a good alternative (i.e., the utility process could just always
include a test app loader or something, but eh...)  Any ideas, or do you think
this is fine?

https://codereview.chromium.org/1149833007/diff/1/content/public/utility/BUIL...
File content/public/utility/BUILD.gn (right):

https://codereview.chromium.org/1149833007/diff/1/content/public/utility/BUIL...
content/public/utility/BUILD.gn:4: 
Hmm somehow missed updating this when I pulled the rest of the utility
gypi_to_gn stuff out of this CL.

https://codereview.chromium.org/1149833007/diff/1/mojo/mojo_services.gyp
File mojo/mojo_services.gyp (right):

https://codereview.chromium.org/1149833007/diff/1/mojo/mojo_services.gyp#newc...
mojo/mojo_services.gyp:32: 'target_name': 'network_service_bindings_lib',
Drive-by typo fix. It was referenced with '_lib' in mojo_shell.gyp, and _lib
makes more sense to me.

https://codereview.chromium.org/1149833007/diff/1/third_party/mojo/mojo_publi...
File third_party/mojo/mojo_public.gyp (right):

https://codereview.chromium.org/1149833007/diff/1/third_party/mojo/mojo_publi...
third_party/mojo/mojo_public.gyp:144: # GN version: //mojo/public/cpp/system
I don't really understand how we've managed to not be building this on gyp
builds....

[jam, 2015-05-27 16:05:36]

(just looked at browser for now)

https://codereview.chromium.org/1149833007/diff/1/content/browser/mojo_shell_...
File content/browser/mojo_shell_browsertest.cc (right):

https://codereview.chromium.org/1149833007/diff/1/content/browser/mojo_shell_...
content/browser/mojo_shell_browsertest.cc:151:
IN_PROC_BROWSER_TEST_F(MojoShellTest, TestUtilityConnection) {
On 2015/05/27 00:12:23, Ken Rockot wrote:
> I'm not terribly happy with the fact that this actually uses an in-process
> utility thread rather than a real utility process, but I was having trouble
> coming up with a good alternative (i.e., the utility process could just always
> include a test app loader or something, but eh...)  Any ideas, or do you think
> this is fine?

i think we want to test the production code path, which is multiprocess.

you could have content shell's contentutilityclient implementation always
register that test app

https://codereview.chromium.org/1149833007/diff/20001/content/DEPS
File content/DEPS (right):

https://codereview.chromium.org/1149833007/diff/20001/content/DEPS#newcode37
content/DEPS:37: "+mojo/shell",
these new two includes might be too broad. i.e. better to add them in subdirs
instead. i.e. will content/renderer access mojo/shell? that seems doubtful?

https://codereview.chromium.org/1149833007/diff/20001/content/browser/mojo_sh...
File content/browser/mojo_shell_context_impl.cc (right):

https://codereview.chromium.org/1149833007/diff/20001/content/browser/mojo_sh...
content/browser/mojo_shell_context_impl.cc:36: const char kBrowserAppUrl[] =
"system:content_browser";
i'm confused, how come this string is only referenced once?

https://codereview.chromium.org/1149833007/diff/20001/content/browser/mojo_sh...
content/browser/mojo_shell_context_impl.cc:47:
process_host->SetName(base::UTF8ToUTF16("Mojo Application"));
all the other process names are localized..

https://codereview.chromium.org/1149833007/diff/20001/content/browser/mojo_sh...
content/browser/mojo_shell_context_impl.cc:52: LOG(ERROR) << "Unable to start
utility process.";
do we really need all this error handling? i looked at StartMojoMode and the
methods it calls, and the only way it can fail is if CreateChannel fails which
has a CHECK in that case. we should make StartMojoMode not have a return code so
that callers don't have to write needless error handling

https://codereview.chromium.org/1149833007/diff/20001/content/browser/mojo_sh...
content/browser/mojo_shell_context_impl.cc:77:
BrowserThread::GetMessageLoopProxyForThread(BrowserThread::IO)
nit:BrowserThread::PostTask(BrowserThread::IO, FROM_HERE, ...

https://codereview.chromium.org/1149833007/diff/20001/content/browser/mojo_sh...
content/browser/mojo_shell_context_impl.cc:82: mojo::String::From(url.spec()),
application_request.Pass(),
nit: I think url.spec() is enough since mojo::String has a non-explicit
constructor

https://codereview.chromium.org/1149833007/diff/20001/content/browser/mojo_sh...
content/browser/mojo_shell_context_impl.cc:91: // Thread-safe proxy providing
global browser access to the shell context.
what does "global browser access" mean? do you mean access from all threads?

https://codereview.chromium.org/1149833007/diff/20001/content/browser/mojo_sh...
content/browser/mojo_shell_context_impl.cc:97: void
Reset(base::WeakPtr<MojoShellContextImpl> weak_shell_context);
nit: IMO this would be more readable if they were inlined, but up to you since
this is personal preference and not in the style guide

https://codereview.chromium.org/1149833007/diff/20001/content/browser/mojo_sh...
content/browser/mojo_shell_context_impl.cc:103: base::Lock lock_;
I don't understand why this locking is needed. i.e. the context is now created
early in browser startup, and lives till shutdown. so it seems keeping track of
the message loop and using it shouldn't need locks

https://codereview.chromium.org/1149833007/diff/20001/content/browser/mojo_sh...
File content/browser/mojo_shell_context_impl.h (right):

https://codereview.chromium.org/1149833007/diff/20001/content/browser/mojo_sh...
content/browser/mojo_shell_context_impl.h:18: // Implementation of the global
Mojo shell context in the browser.
it's not clear to a reader what "mojo shell context" means? can you perhaps
expand a little on what the responsibilities of this class are?

https://codereview.chromium.org/1149833007/diff/20001/content/public/browser/...
File content/public/browser/mojo_app_connection.h (right):

https://codereview.chromium.org/1149833007/diff/20001/content/public/browser/...
content/public/browser/mojo_app_connection.h:22: // applications through the
global Mojo shell. These objects are not thread-safe
nit: what does "global mojo shell" mean?

users of content don't need to know about details of implementation. they just
want to know how to connect to a mojo app

https://codereview.chromium.org/1149833007/diff/20001/content/public/browser/...
content/public/browser/mojo_app_connection.h:27: virtual ~MojoAppConnection() {}
nit: see content api wiki and other examples in content/public, we want to
reduce this as much as possible.

i.e. no need for constructor. also no need for disallow_copy_and_assign on a
non-copyable class (since pure virtual method)

https://codereview.chromium.org/1149833007/diff/20001/content/public/browser/...
File content/public/browser/mojo_shell_context.h (right):

https://codereview.chromium.org/1149833007/diff/20001/content/public/browser/...
content/public/browser/mojo_shell_context.h:18: class CONTENT_EXPORT
MojoShellContext {
why is this interface in public? i can't tell what will be used outside content
in the future.

(btw if you haven't seen it already,
https://www.chromium.org/developers/content-module/content-api is a good summary
of what goes in content/public)

https://codereview.chromium.org/1149833007/diff/20001/content/public/common/p...
File content/public/common/process_control.mojom (right):

https://codereview.chromium.org/1149833007/diff/20001/content/public/common/p...
content/public/common/process_control.mojom:1: // Copyright 2015 The Chromium
Authors. All rights reserved.
why is this in content/public?

[Ken Rockot(use gerrit already), 2015-05-27 19:36:17]

Thanks. I think I've addressed all the comments so far.

I moved the test app & service into content/public/test so both the shell
utility process and the browsertest can load it. The TestUtilityConnection test
now does real multiprocess.

Note that StaticApplicationLoader carries with it a dependency on mojo/shell,
otherwise that dependency could be limited to just content/browser. Probably OK
for now, but maybe ApplicationLoader could actually move into mojo/application.

https://codereview.chromium.org/1149833007/diff/20001/content/DEPS
File content/DEPS (right):

https://codereview.chromium.org/1149833007/diff/20001/content/DEPS#newcode37
content/DEPS:37: "+mojo/shell",
On 2015/05/27 16:05:35, jam wrote:
> these new two includes might be too broad. i.e. better to add them in subdirs
> instead. i.e. will content/renderer access mojo/shell? that seems doubtful?

Done.

https://codereview.chromium.org/1149833007/diff/20001/content/browser/mojo_sh...
File content/browser/mojo_shell_context_impl.cc (right):

https://codereview.chromium.org/1149833007/diff/20001/content/browser/mojo_sh...
content/browser/mojo_shell_context_impl.cc:36: const char kBrowserAppUrl[] =
"system:content_browser";
On 2015/05/27 16:05:36, jam wrote:
> i'm confused, how come this string is only referenced once?

It would make sense for this to be a public constant if we had a Mojo app that
wanted to know if it was talking to the Chrome browser process. Right now we
don't have that so I didn't bother making it public.

Also not really sure about the URL, btw. system:content_browser was sort of an
arbitrary choice because we need some URL.

https://codereview.chromium.org/1149833007/diff/20001/content/browser/mojo_sh...
content/browser/mojo_shell_context_impl.cc:47:
process_host->SetName(base::UTF8ToUTF16("Mojo Application"));
On 2015/05/27 16:05:36, jam wrote:
> all the other process names are localized.. 

It didn't seem worthwhile to localize a placeholder process name, given the
TODO.

Happy to do it anyway, but looking into now, where does one even introduce
localized strings for use in content/browser? It doesn't look like there's any
precedent for that. Any reason not to use content_strings?

https://codereview.chromium.org/1149833007/diff/20001/content/browser/mojo_sh...
content/browser/mojo_shell_context_impl.cc:52: LOG(ERROR) << "Unable to start
utility process.";
On 2015/05/27 16:05:36, jam wrote:
> do we really need all this error handling? i looked at StartMojoMode and the
> methods it calls, and the only way it can fail is if CreateChannel fails which
> has a CHECK in that case. we should make StartMojoMode not have a return code
so
> that callers don't have to write needless error handling

Ah, you're right. I removed the error checking. I will change StartMojoMode to
void in another CL.

https://codereview.chromium.org/1149833007/diff/20001/content/browser/mojo_sh...
content/browser/mojo_shell_context_impl.cc:77:
BrowserThread::GetMessageLoopProxyForThread(BrowserThread::IO)
On 2015/05/27 16:05:36, jam wrote:
> nit:BrowserThread::PostTask(BrowserThread::IO, FROM_HERE, ...

Done.

https://codereview.chromium.org/1149833007/diff/20001/content/browser/mojo_sh...
content/browser/mojo_shell_context_impl.cc:82: mojo::String::From(url.spec()),
application_request.Pass(),
On 2015/05/27 16:05:36, jam wrote:
> nit: I think url.spec() is enough since mojo::String has a non-explicit
> constructor

Done.

https://codereview.chromium.org/1149833007/diff/20001/content/browser/mojo_sh...
content/browser/mojo_shell_context_impl.cc:91: // Thread-safe proxy providing
global browser access to the shell context.
On 2015/05/27 16:05:36, jam wrote:
> what does "global browser access" mean? do you mean access from all threads?

Yes. Clarified.

https://codereview.chromium.org/1149833007/diff/20001/content/browser/mojo_sh...
content/browser/mojo_shell_context_impl.cc:97: void
Reset(base::WeakPtr<MojoShellContextImpl> weak_shell_context);
On 2015/05/27 16:05:36, jam wrote:
> nit: IMO this would be more readable if they were inlined, but up to you since
> this is personal preference and not in the style guide

Done.

https://codereview.chromium.org/1149833007/diff/20001/content/browser/mojo_sh...
content/browser/mojo_shell_context_impl.cc:103: base::Lock lock_;
On 2015/05/27 16:05:35, jam wrote:
> I don't understand why this locking is needed. i.e. the context is now created
> early in browser startup, and lives till shutdown. so it seems keeping track
of
> the message loop and using it shouldn't need locks

You're right, no need for the weakptr either. Though keeping track of the
TaskRunner still makes sense so we can post to the context from any thread.
Changed.

https://codereview.chromium.org/1149833007/diff/20001/content/browser/mojo_sh...
File content/browser/mojo_shell_context_impl.h (right):

https://codereview.chromium.org/1149833007/diff/20001/content/browser/mojo_sh...
content/browser/mojo_shell_context_impl.h:18: // Implementation of the global
Mojo shell context in the browser.
On 2015/05/27 16:05:36, jam wrote:
> it's not clear to a reader what "mojo shell context" means? can you perhaps
> expand a little on what the responsibilities of this class are?

Done.

https://codereview.chromium.org/1149833007/diff/20001/content/public/browser/...
File content/public/browser/mojo_app_connection.h (right):

https://codereview.chromium.org/1149833007/diff/20001/content/public/browser/...
content/public/browser/mojo_app_connection.h:22: // applications through the
global Mojo shell. These objects are not thread-safe
On 2015/05/27 16:05:36, jam wrote:
> nit: what does "global mojo shell" mean?
> 
> users of content don't need to know about details of implementation. they just
> want to know how to connect to a mojo app

Removed the extra details.

https://codereview.chromium.org/1149833007/diff/20001/content/public/browser/...
content/public/browser/mojo_app_connection.h:27: virtual ~MojoAppConnection() {}
On 2015/05/27 16:05:36, jam wrote:
> nit: see content api wiki and other examples in content/public, we want to
> reduce this as much as possible.
> 
> i.e. no need for constructor. also no need for disallow_copy_and_assign on a
> non-copyable class (since pure virtual method)

Done.

https://codereview.chromium.org/1149833007/diff/20001/content/public/browser/...
File content/public/browser/mojo_shell_context.h (right):

https://codereview.chromium.org/1149833007/diff/20001/content/public/browser/...
content/public/browser/mojo_shell_context.h:18: class CONTENT_EXPORT
MojoShellContext {
On 2015/05/27 16:05:36, jam wrote:
> why is this interface in public? i can't tell what will be used outside
content
> in the future.
> 
> (btw if you haven't seen it already,
> https://www.chromium.org/developers/content-module/content-api is a good
summary
> of what goes in content/public)

Doh. This is public because the first iteration of this patch had the embedder
providing an implementation. It is totally unnecessary now, so fixed.

https://codereview.chromium.org/1149833007/diff/20001/content/public/common/p...
File content/public/common/process_control.mojom (right):

https://codereview.chromium.org/1149833007/diff/20001/content/public/common/p...
content/public/common/process_control.mojom:1: // Copyright 2015 The Chromium
Authors. All rights reserved.
On 2015/05/27 16:05:36, jam wrote:
> why is this in content/public?

Same reason ShellContext was. My bad.

[jam, 2015-05-29 01:48:54]

https://codereview.chromium.org/1149833007/diff/190001/content/browser/mojo_a...
File content/browser/mojo_app_connection_impl.h (right):

https://codereview.chromium.org/1149833007/diff/190001/content/browser/mojo_a...
content/browser/mojo_app_connection_impl.h:1: // Copyright 2015 The Chromium
Authors. All rights reserved.
these new files in content/browser/ should be in content/browser/mojo

https://codereview.chromium.org/1149833007/diff/190001/content/browser/mojo_s...
File content/browser/mojo_shell_context.cc (right):

https://codereview.chromium.org/1149833007/diff/190001/content/browser/mojo_s...
content/browser/mojo_shell_context.cc:19: #include
"content/public/browser/utility_process_host_client.h"
i don't think this is needed

there are a lot of includes here, can you check which ones aren't needed?

https://codereview.chromium.org/1149833007/diff/190001/content/common/process...
File content/common/process_control.mojom (right):

https://codereview.chromium.org/1149833007/diff/190001/content/common/process...
content/common/process_control.mojom:11: NOT_FOUND,
why is this an enum instead of a boolean?

https://codereview.chromium.org/1149833007/diff/190001/content/public/browser...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/1149833007/diff/190001/content/public/browser...
content/public/browser/content_browser_client.h:584: virtual void
RegisterMojoApplications(ApplicationRegistry* registry) {}
why call the embedder and give it another interface so that it can call it back?
i.e. why not just give it here a vector of GURL/StaticAppFactory that it can
push_back?

https://codereview.chromium.org/1149833007/diff/190001/content/utility/utilit...
File content/utility/utility_process_control_impl.cc (right):

https://codereview.chromium.org/1149833007/diff/190001/content/utility/utilit...
content/utility/utility_process_control_impl.cc:19:
content::UtilityThread::Get()->ReleaseProcessIfNeeded();
nit: no "content::"

https://codereview.chromium.org/1149833007/diff/190001/mojo/shell/static_appl...
File mojo/shell/static_application_loader.cc (right):

https://codereview.chromium.org/1149833007/diff/190001/mojo/shell/static_appl...
mojo/shell/static_application_loader.cc:30: base::RunLoop run_loop;
this looks similar to the code in
mojo/application/public/cpp/lib/application_runner.cc, can they be shared? i.e.
sky just made fixes in the order of destruction in r331877

[Ken Rockot(use gerrit already), 2015-05-29 03:20:41]

https://codereview.chromium.org/1149833007/diff/190001/content/browser/mojo_a...
File content/browser/mojo_app_connection_impl.h (right):

https://codereview.chromium.org/1149833007/diff/190001/content/browser/mojo_a...
content/browser/mojo_app_connection_impl.h:1: // Copyright 2015 The Chromium
Authors. All rights reserved.
On 2015/05/29 01:48:54, jam wrote:
> these new files in content/browser/ should be in content/browser/mojo

Done.

https://codereview.chromium.org/1149833007/diff/190001/content/browser/mojo_s...
File content/browser/mojo_shell_context.cc (right):

https://codereview.chromium.org/1149833007/diff/190001/content/browser/mojo_s...
content/browser/mojo_shell_context.cc:19: #include
"content/public/browser/utility_process_host_client.h"
On 2015/05/29 01:48:54, jam wrote:
> i don't think this is needed
> 
> there are a lot of includes here, can you check which ones aren't needed?

Cleaned up some headers left behind, but this one is actually needed because
UtilityProcessHost::Create(nullptr, nullptr) implicitly constructs a
scoped_refptr<UtilityProcessHostClient> which needs the type information.

https://codereview.chromium.org/1149833007/diff/190001/content/common/process...
File content/common/process_control.mojom (right):

https://codereview.chromium.org/1149833007/diff/190001/content/common/process...
content/common/process_control.mojom:11: NOT_FOUND,
On 2015/05/29 01:48:54, jam wrote:
> why is this an enum instead of a boolean?

Done. It was an enum in anticipation of additional future failure modes (e.g.
when we support loading dynamic applications you might have an invalid binary), 
we can change it back to an enum when that happens.

https://codereview.chromium.org/1149833007/diff/190001/content/public/browser...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/1149833007/diff/190001/content/public/browser...
content/public/browser/content_browser_client.h:584: virtual void
RegisterMojoApplications(ApplicationRegistry* registry) {}
On 2015/05/29 01:48:54, jam wrote:
> why call the embedder and give it another interface so that it can call it
back?
> i.e. why not just give it here a vector of GURL/StaticAppFactory that it can
> push_back?

I think there are a few decent reasons:

 - I experimented with passing in a map and it adds lots of code,
   mostly because:
 - There's no need to allocate extra containers in some cases;
   MojoShellContext for example doesn't hold a map of its own, it
   just forwards URL/Loader pairs to ApplicationManager.
 - We eventually need to register more than just static factories
 - Things which hold URL/app mappings may want to hand out a
   registration interface to more than just Content*Client.

https://codereview.chromium.org/1149833007/diff/190001/content/utility/utilit...
File content/utility/utility_process_control_impl.cc (right):

https://codereview.chromium.org/1149833007/diff/190001/content/utility/utilit...
content/utility/utility_process_control_impl.cc:19:
content::UtilityThread::Get()->ReleaseProcessIfNeeded();
On 2015/05/29 01:48:54, jam wrote:
> nit: no "content::"

Done.

https://codereview.chromium.org/1149833007/diff/190001/mojo/shell/static_appl...
File mojo/shell/static_application_loader.cc (right):

https://codereview.chromium.org/1149833007/diff/190001/mojo/shell/static_appl...
mojo/shell/static_application_loader.cc:30: base::RunLoop run_loop;
On 2015/05/29 01:48:54, jam wrote:
> this looks similar to the code in
> mojo/application/public/cpp/lib/application_runner.cc, can they be shared?
i.e.
> sky just made fixes in the order of destruction in r331877

I don't really think ApplicationRunner can be shared with chrome (yet). It looks
like it expects to be on the main thread, and as such it constructs its own
MessageLoop, among other things. We're running on a new base::Thread here.

I'm pretty sure we CAN share ApplicationRunner in the utility process once we've
stripped it down to be nothing but an app runner, but we're not quite there yet.

[jam, 2015-05-29 15:11:50]

https://codereview.chromium.org/1149833007/diff/190001/content/public/browser...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/1149833007/diff/190001/content/public/browser...
content/public/browser/content_browser_client.h:584: virtual void
RegisterMojoApplications(ApplicationRegistry* registry) {}
On 2015/05/29 03:20:40, Ken Rockot wrote:
> On 2015/05/29 01:48:54, jam wrote:
> > why call the embedder and give it another interface so that it can call it
> back?
> > i.e. why not just give it here a vector of GURL/StaticAppFactory that it can
> > push_back?
> 
> I think there are a few decent reasons:
> 
>  - I experimented with passing in a map and it adds lots of code,
>    mostly because:
>  - There's no need to allocate extra containers in some cases;
>    MojoShellContext for example doesn't hold a map of its own, it
>    just forwards URL/Loader pairs to ApplicationManager.

I'm not sure I understand what you mean here.

are you saying that it's inefficient to copy the stl container class?

>  - We eventually need to register more than just static factories

why?

>  - Things which hold URL/app mappings may want to hand out a
>    registration interface to more than just Content*Client.

examples?

in general, for content api (per link in previous round of reviews) we try to be
as minimal as possible. what can be done with one method shouldn't be done with
a method and interface

https://codereview.chromium.org/1149833007/diff/190001/mojo/shell/static_appl...
File mojo/shell/static_application_loader.cc (right):

https://codereview.chromium.org/1149833007/diff/190001/mojo/shell/static_appl...
mojo/shell/static_application_loader.cc:30: base::RunLoop run_loop;
On 2015/05/29 03:20:40, Ken Rockot wrote:
> On 2015/05/29 01:48:54, jam wrote:
> > this looks similar to the code in
> > mojo/application/public/cpp/lib/application_runner.cc, can they be shared?
> i.e.
> > sky just made fixes in the order of destruction in r331877
> 
> I don't really think ApplicationRunner can be shared with chrome (yet). It
looks
> like it expects to be on the main thread, 

really? I don't see that, i.e.right now mandoline is single threaded and each
app runs on a different thread.

> and as such it constructs its own
> MessageLoop, among other things. We're running on a new base::Thread here.
> 
> I'm pretty sure we CAN share ApplicationRunner in the utility process once
we've
> stripped it down to be nothing but an app runner, but we're not quite there
yet.

[Ken Rockot(use gerrit already), 2015-05-29 15:34:55]

On 2015/05/29 15:11:50, jam wrote:
>
https://codereview.chromium.org/1149833007/diff/190001/content/public/browser...
> File content/public/browser/content_browser_client.h (right):
> 
>
https://codereview.chromium.org/1149833007/diff/190001/content/public/browser...
> content/public/browser/content_browser_client.h:584: virtual void
> RegisterMojoApplications(ApplicationRegistry* registry) {}
> On 2015/05/29 03:20:40, Ken Rockot wrote:
> > On 2015/05/29 01:48:54, jam wrote:
> > > why call the embedder and give it another interface so that it can call it
> > back?
> > > i.e. why not just give it here a vector of GURL/StaticAppFactory that it
can
> > > push_back?
> > 
> > I think there are a few decent reasons:
> > 
> >  - I experimented with passing in a map and it adds lots of code,
> >    mostly because:
> >  - There's no need to allocate extra containers in some cases;
> >    MojoShellContext for example doesn't hold a map of its own, it
> >    just forwards URL/Loader pairs to ApplicationManager.
> 
> I'm not sure I understand what you mean here.
> 
> are you saying that it's inefficient to copy the stl container class?
> 

Well, it's more allocation than necessary. That might not really matter.

> >  - We eventually need to register more than just static factories
> 
> why?

I was thinking we'd also need to register mojo binary paths for certain dynamic
loaders, but actually I guess signed packages with manifests can provide all the
information the shell will need.

> 
> >  - Things which hold URL/app mappings may want to hand out a
> >    registration interface to more than just Content*Client.
> 
> examples?

One example now is test support for MojoShellContext. Overriding the browser
client can't be done before MojoShellContext initialization. I guess we could
change where it's initialized, or just make the test support a little more
awkward (i.e. you provide a base::Callback<std::map<GURL,
base::Callback<scoped_ptr<mojo::ApplicationDelegate>()>>> instead of an
ApplicationRegistry*).

> 
> in general, for content api (per link in previous round of reviews) we try to
be
> as minimal as possible. what can be done with one method shouldn't be done
with
> a method and interface

OK. I'll upload a patch with this change.

> 
>
https://codereview.chromium.org/1149833007/diff/190001/mojo/shell/static_appl...
> File mojo/shell/static_application_loader.cc (right):
> 
>
https://codereview.chromium.org/1149833007/diff/190001/mojo/shell/static_appl...
> mojo/shell/static_application_loader.cc:30: base::RunLoop run_loop;
> On 2015/05/29 03:20:40, Ken Rockot wrote:
> > On 2015/05/29 01:48:54, jam wrote:
> > > this looks similar to the code in
> > > mojo/application/public/cpp/lib/application_runner.cc, can they be shared?
> > i.e.
> > > sky just made fixes in the order of destruction in r331877
> > 
> > I don't really think ApplicationRunner can be shared with chrome (yet). It
> looks
> > like it expects to be on the main thread, 
> 
> really? I don't see that, i.e.right now mandoline is single threaded and each
> app runs on a different thread.

Are we talking about the same thing?

https://chromium.googlesource.com/chromium/src/+/master/mojo/application/publ...

is creating its own MessageLoop.

> 
> > and as such it constructs its own
> > MessageLoop, among other things. We're running on a new base::Thread here.
> > 
> > I'm pretty sure we CAN share ApplicationRunner in the utility process once
> we've
> > stripped it down to be nothing but an app runner, but we're not quite there
> yet.

[Ken Rockot(use gerrit already), 2015-05-29 17:12:45]

Alright, ApplicationRegistry is gone. It's not so bad. I'm not crazy about
having redundant type aliases for a URL/factory mapping in different places, but
the aliases IMHO make the code more readable. I don't see a good place for a
shared definition.

[Ken Rockot(use gerrit already), 2015-05-29 17:54:24]

Oh, I think ApplicationRunner works for Mandoline because you're loading DSOs,
and they have their own base state. So base::MessageLoop::current is null in the
app's code, even though it's running on a thread that already has a messageloop.
Does that sound right?

[Ken Rockot(use gerrit already), 2015-06-02 21:10:45]

light ping

[jam, 2015-06-03 14:34:21]

sorry for delay, I missed the update. in the future please IM me if I'm not
responsive.

On 2015/05/29 15:34:55, Ken Rockot wrote:
> On 2015/05/29 15:11:50, jam wrote:
> >
>
https://codereview.chromium.org/1149833007/diff/190001/content/public/browser...
> > File content/public/browser/content_browser_client.h (right):
> > 
> >
>
https://codereview.chromium.org/1149833007/diff/190001/content/public/browser...
> > content/public/browser/content_browser_client.h:584: virtual void
> > RegisterMojoApplications(ApplicationRegistry* registry) {}
> > On 2015/05/29 03:20:40, Ken Rockot wrote:
> > > On 2015/05/29 01:48:54, jam wrote:
> > > > why call the embedder and give it another interface so that it can call
it
> > > back?
> > > > i.e. why not just give it here a vector of GURL/StaticAppFactory that it
> can
> > > > push_back?
> > > 
> > > I think there are a few decent reasons:
> > > 
> > >  - I experimented with passing in a map and it adds lots of code,
> > >    mostly because:
> > >  - There's no need to allocate extra containers in some cases;
> > >    MojoShellContext for example doesn't hold a map of its own, it
> > >    just forwards URL/Loader pairs to ApplicationManager.
> > 
> > I'm not sure I understand what you mean here.
> > 
> > are you saying that it's inefficient to copy the stl container class?
> > 
> 
> Well, it's more allocation than necessary. That might not really matter.
> 
> > >  - We eventually need to register more than just static factories
> > 
> > why?
> 
> I was thinking we'd also need to register mojo binary paths for certain
dynamic
> loaders, but actually I guess signed packages with manifests can provide all
the
> information the shell will need.
> 
> > 
> > >  - Things which hold URL/app mappings may want to hand out a
> > >    registration interface to more than just Content*Client.
> > 
> > examples?
> 
> One example now is test support for MojoShellContext. Overriding the browser
> client can't be done before MojoShellContext initialization. I guess we could
> change where it's initialized, or just make the test support a little more
> awkward (i.e. you provide a base::Callback<std::map<GURL,
> base::Callback<scoped_ptr<mojo::ApplicationDelegate>()>>> instead of an
> ApplicationRegistry*).
> 
> > 
> > in general, for content api (per link in previous round of reviews) we try
to
> be
> > as minimal as possible. what can be done with one method shouldn't be done
> with
> > a method and interface
> 
> OK. I'll upload a patch with this change.
> 
> > 
> >
>
https://codereview.chromium.org/1149833007/diff/190001/mojo/shell/static_appl...
> > File mojo/shell/static_application_loader.cc (right):
> > 
> >
>
https://codereview.chromium.org/1149833007/diff/190001/mojo/shell/static_appl...
> > mojo/shell/static_application_loader.cc:30: base::RunLoop run_loop;
> > On 2015/05/29 03:20:40, Ken Rockot wrote:
> > > On 2015/05/29 01:48:54, jam wrote:
> > > > this looks similar to the code in
> > > > mojo/application/public/cpp/lib/application_runner.cc, can they be
shared?
> > > i.e.
> > > > sky just made fixes in the order of destruction in r331877
> > > 
> > > I don't really think ApplicationRunner can be shared with chrome (yet). It
> > looks
> > > like it expects to be on the main thread, 
> > 
> > really? I don't see that, i.e.right now mandoline is single threaded and
each
> > app runs on a different thread.
> 
> Are we talking about the same thing?
> 
>
https://chromium.googlesource.com/chromium/src/+/master/mojo/application/publ...
> 
> is creating its own MessageLoop.

sorry I meant single process, and each app runs on a different thread.


> 
> > 
> > > and as such it constructs its own
> > > MessageLoop, among other things. We're running on a new base::Thread here.
> > > 
> > > I'm pretty sure we CAN share ApplicationRunner in the utility process once
> > we've
> > > stripped it down to be nothing but an app runner, but we're not quite
there
> > yet.

[jam, 2015-06-03 14:35:36]

On 2015/05/29 17:54:24, Ken Rockot wrote:
> Oh, I think ApplicationRunner works for Mandoline because you're loading DSOs,
> and they have their own base state. So base::MessageLoop::current is null in
the
> app's code, even though it's running on a thread that already has a
messageloop.
> Does that sound right?

ApplicationRunner is in the C++ helper library that is linked with the
application. So this is the message loop that the application sees.

[jam, 2015-06-03 14:50:21]

so to be clear i think this patch is almost done, other than the one point of
trying to combine the code which loads an app. perhaps it would be faster to
talk in person about how/if we can combine them?

https://codereview.chromium.org/1149833007/diff/220001/content/public/browser...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/1149833007/diff/220001/content/public/browser...
content/public/browser/content_browser_client.h:140: std::map<GURL,
base::Callback<scoped_ptr<mojo::ApplicationDelegate>()>>;
nit: by convention we put these typedefs right before a method in the content
api (since this i sa large file, and when looking at a method it's nice to see
the types there instead of scrolling back and forth)

[Ken Rockot(use gerrit already), 2015-06-03 17:02:38]

On 2015/06/03 14:35:36, jam wrote:
> On 2015/05/29 17:54:24, Ken Rockot wrote:
> > Oh, I think ApplicationRunner works for Mandoline because you're loading
DSOs,
> > and they have their own base state. So base::MessageLoop::current is null in
> the
> > app's code, even though it's running on a thread that already has a
> messageloop.
> > Does that sound right?
> 
> ApplicationRunner is in the C++ helper library that is linked with the
> application. So this is the message loop that the application sees.

Yeah sorry, I didn't explain the problem clearly.

The MessageLoop constructor DCHECKs:

  DCHECK(current() == this || !current());

and in Chrome on a base::Thread running statically linked application code, this
DCHECK will always fail because the thread already has a MessageLoop.

This doesn't fail in Mandoline because each application also links against its
own libbase, and from the application's perspective, MessageLoop::current() is
null until ApplicationRunner constructs its loop. But really the thread already
has a MessageLoop that the app can't see, and this works just fine in practice.

[jam, 2015-06-04 03:47:02]

On 2015/06/03 17:02:38, Ken Rockot wrote:
> On 2015/06/03 14:35:36, jam wrote:
> > On 2015/05/29 17:54:24, Ken Rockot wrote:
> > > Oh, I think ApplicationRunner works for Mandoline because you're loading
> DSOs,
> > > and they have their own base state. So base::MessageLoop::current is null
in
> > the
> > > app's code, even though it's running on a thread that already has a
> > messageloop.
> > > Does that sound right?
> > 
> > ApplicationRunner is in the C++ helper library that is linked with the
> > application. So this is the message loop that the application sees.
> 
> Yeah sorry, I didn't explain the problem clearly.
> 
> The MessageLoop constructor DCHECKs:
> 
>   DCHECK(current() == this || !current());
> 
> and in Chrome on a base::Thread running statically linked application code,
this
> DCHECK will always fail because the thread already has a MessageLoop.
> 
> This doesn't fail in Mandoline because each application also links against its
> own libbase, and from the application's perspective, MessageLoop::current() is
> null until ApplicationRunner constructs its loop. But really the thread
already
> has a MessageLoop that the app can't see, and this works just fine in
practice.

StaticApplicationLoader could use SimpleThread instead of Thread which doesn't
create a MessageLoop.

[jam, 2015-06-04 19:15:23]

lgtm

[Ken Rockot(use gerrit already), 2015-06-04 19:56:12]

tsepez, please take a look a look at content/common/process_control.mojom

[Ken Rockot(use gerrit already), 2015-06-04 19:56:29]

rockot@chromium.org changed reviewers:
+ tsepez@chromium.org

[Ken Rockot(use gerrit already), 2015-06-04 19:56:54]

(and now +tsepez for real)

tsepez, please take a look a look at content/common/process_control.mojom

[Tom Sepez, 2015-06-04 20:43:55]

https://codereview.chromium.org/1149833007/diff/250001/content/common/process...
File content/common/process_control.mojom (right):

https://codereview.chromium.org/1149833007/diff/250001/content/common/process...
content/common/process_control.mojom:10: LoadApplication(string url,
mojo.Application& request) => (bool success);
This is renderer -> browser, right?  If so, how do we know that the renderer has
permission to connect to the application given by |url|.

[Ken Rockot(use gerrit already), 2015-06-04 20:46:21]

Actually it's a request from browser to child

On Thu, Jun 4, 2015 at 1:43 PM, <tsepez@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/1149833007/diff/250001/content/common/process...
> File content/common/process_control.mojom (right):
>
>
>
https://codereview.chromium.org/1149833007/diff/250001/content/common/process...
> content/common/process_control.mojom:10: LoadApplication(string url,
> mojo.Application& request) => (bool success);
> This is renderer -> browser, right?  If so, how do we know that the
> renderer has permission to connect to the application given by |url|.
>
> https://codereview.chromium.org/1149833007/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Tom Sepez, 2015-06-04 20:56:02]

LGTM then.

[Ken Rockot(use gerrit already), 2015-06-04 22:53:12]

The CQ bit was checked by rockot@chromium.org

[Ken Rockot(use gerrit already), 2015-06-04 22:53:12]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
jam@chromium.org
Link to the patchset: https://codereview.chromium.org/1149833007/#ps310001
(title: " ")

[commit-bot: I haz the power, 2015-06-04 22:54:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1149833007/310001

[commit-bot: I haz the power, 2015-06-05 00:31:02]

Committed patchset #18 (id:310001)

[commit-bot: I haz the power, 2015-06-05 00:32:55]

Patchset 18 (id:??) landed as
https://crrev.com/b814a5850da5aa473ad526eef4b41a82b05037a0
Cr-Commit-Position: refs/heads/master@{#332974}

[Lei Zhang, 2015-08-25 03:16:03]

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

[Lei Zhang, 2015-08-25 03:16:04]

(random late drive-by)

https://codereview.chromium.org/1149833007/diff/310001/content/browser/mojo/m...
File content/browser/mojo/mojo_shell_context.cc (right):

https://codereview.chromium.org/1149833007/diff/310001/content/browser/mojo/m...
content/browser/mojo/mojo_shell_context.cc:36: const
MojoShellContext::StaticApplicationMap* g_applications_for_test;
Should this be explicitly initialized to nullptr?

https://codereview.chromium.org/1149833007/diff/310001/content/browser/mojo/m...
content/browser/mojo/mojo_shell_context.cc:43:
process_host->SetName(base::UTF8ToUTF16("Mojo Application"));
I'm assuming because this is in mojo_shell, it doesn't matter that this isn't
translated, since we are not shipping it to users?

https://codereview.chromium.org/1149833007/diff/310001/content/browser/mojo/m...
content/browser/mojo/mojo_shell_context.cc:83: Proxy(MojoShellContext*
shell_context)
nit: explicit

https://codereview.chromium.org/1149833007/diff/310001/content/browser/mojo/m...
content/browser/mojo/mojo_shell_context.cc:93: if (shell_context_)
Here you are checking for |shell_context_|, but in the else block, you pass it
as the object. Couldn't that crash if |shell_context_| is NULL?

[Ken Rockot(use gerrit already), 2015-08-25 03:54:52]

https://codereview.chromium.org/1149833007/diff/310001/content/browser/mojo/m...
File content/browser/mojo/mojo_shell_context.cc (right):

https://codereview.chromium.org/1149833007/diff/310001/content/browser/mojo/m...
content/browser/mojo/mojo_shell_context.cc:36: const
MojoShellContext::StaticApplicationMap* g_applications_for_test;
On 2015/08/25 03:16:03, Lei Zhang wrote:
> Should this be explicitly initialized to nullptr?

There's no need to do this as C++ guarantees zero-initialization for static
storage:

http://en.cppreference.com/w/cpp/language/zero_initialization

https://codereview.chromium.org/1149833007/diff/310001/content/browser/mojo/m...
content/browser/mojo/mojo_shell_context.cc:43:
process_host->SetName(base::UTF8ToUTF16("Mojo Application"));
On 2015/08/25 03:16:04, Lei Zhang wrote:
> I'm assuming because this is in mojo_shell, it doesn't matter that this isn't
> translated, since we are not shipping it to users?

This is not in Mojo shell. This code is shipping to users but there are no
production code paths which trigger it in practice yet. We do need to fix it.

https://codereview.chromium.org/1149833007/diff/310001/content/browser/mojo/m...
content/browser/mojo/mojo_shell_context.cc:83: Proxy(MojoShellContext*
shell_context)
On 2015/08/25 03:16:04, Lei Zhang wrote:
> nit: explicit

Acknowledged.

https://codereview.chromium.org/1149833007/diff/310001/content/browser/mojo/m...
content/browser/mojo/mojo_shell_context.cc:93: if (shell_context_)
On 2015/08/25 03:16:04, Lei Zhang wrote:
> Here you are checking for |shell_context_|, but in the else block, you pass it
> as the object. Couldn't that crash if |shell_context_| is NULL?

This is a pretty subtle thing, but I believe it's safe. shell_context_ lives on
task_runner_'s thread. If we're currently on that thread in this method, we can
test shell_context_ and call ConnectToApplicationOnOwnThread.

If we're not on task_runner_'s thread we post a task there to run the same
function. Because shell_context_'s lifetime is bound to that of task_runner_'s
MessageLoop, we know that if the task runs at all, the ShellContext is still
valid. Conversely if the ShellContext is no longer valid it means the
MessageLoop was destroyed and the task posted here will never run.

In fact I think the null test here could be replaced with a DCHECK.