Load application manifests from resources

Load application manifests from resources

- Renames content's builtin apps to content_browser and content_renderer
- Supports loading manifests from resources instead of files
- Derives chrome's manifests from content's without duplicating metadata
- Avoids all filesystem lookups for existing apps in content today
- Ensures all content embedders have a working shell connection with capabilities

BUG=594419
Committed: https://crrev.com/00668f9780a1b1c6c1b1a1f2457418d0aecf9bb5
Cr-Commit-Position: refs/heads/master@{#385823}

[Ken Rockot(use gerrit already), 2016-03-24 16:28:57]

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

[Ken Rockot(use gerrit already), 2016-03-24 16:28:57]

I want to kill Loader, and the first step is turning the browser into a proper
package/ShellClientFactory. We need manifests to be loaded in production to
support this, and the cleanest way to do that right now is to stuff them in the
browser resource bundle.

This also removes all the recently added "chrome"ish references from content in
favor of client interface delegation.

John could you look at content bits, and Ben at the CL overall?

[Ken Rockot(use gerrit already), 2016-03-24 16:30:11]

Hmm... I see catalog has changed again already so I probably need to fix this.

[Ken Rockot(use gerrit already), 2016-03-24 16:46:52]

On 2016/03/24 at 16:30:11, Ken Rockot wrote:
> Hmm... I see catalog has changed again already so I probably need to fix this.

(fixed)

[jam, 2016-03-24 17:09:10]

my one question is how does this change affect non-chrome embedders of content?
i.e. as we add new apps that are required for operation of content (like mus),
shouldn't this be registered at the content layer instead of chrome? should
there be a way for content to register its own manifest, and chrome layer can
add on top of that?

https://codereview.chromium.org/1828733004/diff/40001/chrome/app/mojo/chrome_...
File chrome/app/mojo/chrome_manifest.json (right):

https://codereview.chromium.org/1828733004/diff/40001/chrome/app/mojo/chrome_...
chrome/app/mojo/chrome_manifest.json:4: "display_name": "Chrome",
why isn't this Chrome Browser, to be consistent with "Chrome Renderer" for the
renderer process? ditto for filename

the reason i bring this up is it's a bit confusing what "chrome" refers to. i.e.
is it services in all processes? i realize it's not, but "chrome" is generic and
doesn't answer that

https://codereview.chromium.org/1828733004/diff/40001/content/browser/mojo/mo...
File content/browser/mojo/mojo_child_connection.cc (right):

https://codereview.chromium.org/1828733004/diff/40001/content/browser/mojo/mo...
content/browser/mojo/mojo_child_connection.cc:18: #include
"content/public/common/content_client.h"
nit: by convention we don't include this when we include content_foo_client,
since they always include content_client.h

https://codereview.chromium.org/1828733004/diff/40001/content/browser/mojo/mo...
content/browser/mojo/mojo_child_connection.cc:20: #include
"content/public/common/process_type.h"
nit: no point in including this, since by definition content_browser_client.h
includes it as it's used as a parameter

[Ken Rockot(use gerrit already), 2016-03-24 17:33:24]

I can think of two potential solutions for the layering issue. Either one would
probably be more appropriate as a follow-up CL as needed:

1. content defines a mojo:content_browser app and embedders can add this to
their exe package if they want to expose it. If we end up wanting renderers to
be packages too, we can do the same thing with a mojo:content_renderer and
exe:chrome_renderer.

2. we define a new way to aggregate manifest metadata that makes sense for
layered applications like this. i.e. content would define a partial manifest and
provide its own resolution logic (delegating the rest to the browser client as
this CL does), and we could express a way to embed the partial manifest in
exe:chrome's manifest at build time.

WDYT?

https://codereview.chromium.org/1828733004/diff/40001/chrome/app/mojo/chrome_...
File chrome/app/mojo/chrome_manifest.json (right):

https://codereview.chromium.org/1828733004/diff/40001/chrome/app/mojo/chrome_...
chrome/app/mojo/chrome_manifest.json:4: "display_name": "Chrome",
On 2016/03/24 at 17:09:10, jam wrote:
> why isn't this Chrome Browser, to be consistent with "Chrome Renderer" for the
renderer process? ditto for filename
> 
> the reason i bring this up is it's a bit confusing what "chrome" refers to.
i.e. is it services in all processes? i realize it's not, but "chrome" is
generic and doesn't answer that

Well, it's not just the browser. The idea is to treat Chrome as an app package,
so its generated manifest will eventually aggregate other application manifests.
This will teach the shell to use exe:chrome's ShellClientFactory when resolving
packaged app names.

As one example I was going to write a follow-up CL which adds a manifest for
mojo:profile and includes it in exe:chrome's package. That would let us get rid
of the Loader used for mojo:profile now. Other loaders would follow.

I don't really care what we name this, but I think just "Chrome" is good enough
for now.

https://codereview.chromium.org/1828733004/diff/40001/content/browser/mojo/mo...
File content/browser/mojo/mojo_child_connection.cc (right):

https://codereview.chromium.org/1828733004/diff/40001/content/browser/mojo/mo...
content/browser/mojo/mojo_child_connection.cc:18: #include
"content/public/common/content_client.h"
On 2016/03/24 at 17:09:10, jam wrote:
> nit: by convention we don't include this when we include content_foo_client,
since they always include content_client.h

done

https://codereview.chromium.org/1828733004/diff/40001/content/browser/mojo/mo...
content/browser/mojo/mojo_child_connection.cc:20: #include
"content/public/common/process_type.h"
On 2016/03/24 at 17:09:10, jam wrote:
> nit: no point in including this, since by definition content_browser_client.h
includes it as it's used as a parameter

done, though i'm not sure i agree with this policy. my interpretation of IWYU is
that it doesn't really matter if it's included by content_browser_client.h or
that it's only used as a parameter for something defined there.

[Ben Goodger (Google), 2016-03-24 22:53:54]

https://codereview.chromium.org/1828733004/diff/20001/content/browser/mojo/mo...
File content/browser/mojo/mojo_child_connection.cc (right):

https://codereview.chromium.org/1828733004/diff/20001/content/browser/mojo/mo...
content/browser/mojo/mojo_child_connection.cc:108:
GetContentClient()->browser()->GetMojoApplicationNameForProcess(
instead of doing this, I think we should probably call all chrome processes
exe:chrome and use the instance field instead. you can see me autogen it in the
target construction below %d_%d...

https://codereview.chromium.org/1828733004/diff/20001/mojo/services/catalog/c...
File mojo/services/catalog/catalog.h (right):

https://codereview.chromium.org/1828733004/diff/20001/mojo/services/catalog/c...
mojo/services/catalog/catalog.h:30: class Delegate {
top level class, ManifestProvider.

[Ken Rockot(use gerrit already), 2016-03-24 22:57:25]

On 2016/03/24 at 22:53:54, ben wrote:
>
https://codereview.chromium.org/1828733004/diff/20001/content/browser/mojo/mo...
> File content/browser/mojo/mojo_child_connection.cc (right):
> 
>
https://codereview.chromium.org/1828733004/diff/20001/content/browser/mojo/mo...
> content/browser/mojo/mojo_child_connection.cc:108:
GetContentClient()->browser()->GetMojoApplicationNameForProcess(
> instead of doing this, I think we should probably call all chrome processes
exe:chrome and use the instance field instead. you can see me autogen it in the
target construction below %d_%d...

I don't follow. Shouldn't different process types have different capabilities
and therefore different app identities?

> 
>
https://codereview.chromium.org/1828733004/diff/20001/mojo/services/catalog/c...
> File mojo/services/catalog/catalog.h (right):
> 
>
https://codereview.chromium.org/1828733004/diff/20001/mojo/services/catalog/c...
> mojo/services/catalog/catalog.h:30: class Delegate {
> top level class, ManifestProvider.

[Ken Rockot(use gerrit already), 2016-03-30 02:46:44]

Description was changed from

==========
Switch Chrome to use Mojo manifests from resources

- Delegates Mojo application naming for the browser and child
  processes to ContentBrowserClient
- Moves Chrome manifests from content/browser to chrome/app
- Bakes Chrome manifests into browser resource bundle
- Adds a manifest resolution delegate interface to Catalog
- Makes MojoShellContext delegate builtin manifest resolution
  to ContentBrowserClient
- Adds builtin resolvers for exe:chrome, exe:chrome_renderer,
  and mojo:catalog to ChromeContentBrowserClient to avoid
  filesystem lookups for builtin applications at runtime.

BUG=594419
==========

to

==========
Load application manifests from resources

- Renames content's builtin apps to content_browser and content_renderer
- Supports loading manifests from resources instead of files
- Adds a content client interface for overriding app names and manifests
- Derives chrome's manifests from content's without duplicating metadata
- Avoids all filesystem lookups for existing apps in Chrome today
- Ensures all content embedders have a working shell connection with
capabilities

BUG=594419
==========

[Ken Rockot(use gerrit already), 2016-03-30 02:52:13]

Updated the CL to reflect the proposal in the doc where chrome derives its
manifests from content's. In the case where content is built by a different
embedder it will now default to shell connections as exe:content_browser and
exe:content_renderer with appropriate capabilities so other content embedders
get basic Mojo shell integration for free.

Ben, regarding your earlier comment, I think different child process types still
need different 
application names because they have different capabilities.

The base_manifest stuff feels a little hackish, but it works.

John, despite our earlier conversation I'd still like to try this approach. We
have to do all the work to merge the manifests at build-time anyway. If you
really don't want to delegate the entire manifest to the embedder we can always
add code in this CL to do some kind of runtime delegation in the embedded shell
case. LMKWYT.

[Ben Goodger (Google), 2016-03-30 22:27:50]

Looking good... just a couple of questions

https://codereview.chromium.org/1828733004/diff/80001/content/browser/mojo/mo...
File content/browser/mojo/mojo_child_connection.cc (right):

https://codereview.chromium.org/1828733004/diff/80001/content/browser/mojo/mo...
content/browser/mojo/mojo_child_connection.cc:109: return pipe_token;
what happens in this case? how does the caller react?

https://codereview.chromium.org/1828733004/diff/80001/content/browser/mojo/mo...
File content/browser/mojo/mojo_shell_context.cc (right):

https://codereview.chromium.org/1828733004/diff/80001/content/browser/mojo/mo...
content/browser/mojo/mojo_shell_context.cc:179: } else if (name ==
"exe:content_browser") {
this isn't affected if the embedder provides a different value in
GetPackagedApplicationName is it?

https://codereview.chromium.org/1828733004/diff/80001/content/public/browser/...
File content/public/browser/content_browser_client.cc (right):

https://codereview.chromium.org/1828733004/diff/80001/content/public/browser/...
content/public/browser/content_browser_client.cc:388: return
"exe:content_browser";
it'd be nice eventually if these strings wound up in constants to reduce the
possibility of weird runtime errors.

[Ken Rockot(use gerrit already), 2016-03-31 18:55:49]

https://codereview.chromium.org/1828733004/diff/80001/content/browser/mojo/mo...
File content/browser/mojo/mojo_child_connection.cc (right):

https://codereview.chromium.org/1828733004/diff/80001/content/browser/mojo/mo...
content/browser/mojo/mojo_child_connection.cc:109: return pipe_token;
On 2016/03/30 at 22:27:50, Ben Goodger (Google) wrote:
> what happens in this case? how does the caller react?

The caller doesn't know about any failures here, it just passes the token along
to the child. When the child establishes its end of the pipe it'll detect peer
closure since the parent end will have been closed.

This means a little unnecessary work happens in the failure cases, but it also
means less error handling code. We could make this return the empty string
instead. It would probably be easier for readers to understand.

https://codereview.chromium.org/1828733004/diff/80001/content/browser/mojo/mo...
File content/browser/mojo/mojo_shell_context.cc (right):

https://codereview.chromium.org/1828733004/diff/80001/content/browser/mojo/mo...
content/browser/mojo/mojo_shell_context.cc:179: } else if (name ==
"exe:content_browser") {
On 2016/03/30 at 22:27:50, Ben Goodger (Google) wrote:
> this isn't affected if the embedder provides a different value in
GetPackagedApplicationName is it?

If the embedder provides a different app name, they're also responsible for
providing the associated manifest if necessary.

https://codereview.chromium.org/1828733004/diff/80001/content/public/browser/...
File content/public/browser/content_browser_client.cc (right):

https://codereview.chromium.org/1828733004/diff/80001/content/public/browser/...
content/public/browser/content_browser_client.cc:388: return
"exe:content_browser";
On 2016/03/30 at 22:27:50, Ben Goodger (Google) wrote:
> it'd be nice eventually if these strings wound up in constants to reduce the
possibility of weird runtime errors.

Done. I only added constants for the content names. We should do it for
exe:chrome too, and probably common mojo: names, but maybe later. There are
quite a few scattered references to the others.

[Ben Goodger (Google), 2016-03-31 19:06:23]

lgtm

[jam, 2016-04-06 22:41:01]

lgtm

[Ken Rockot(use gerrit already), 2016-04-07 01:38:27]

Description was changed from

==========
Load application manifests from resources

- Renames content's builtin apps to content_browser and content_renderer
- Supports loading manifests from resources instead of files
- Adds a content client interface for overriding app names and manifests
- Derives chrome's manifests from content's without duplicating metadata
- Avoids all filesystem lookups for existing apps in Chrome today
- Ensures all content embedders have a working shell connection with
capabilities

BUG=594419
==========

to

==========
Load application manifests from resources

- Renames content's builtin apps to content_browser and content_renderer
- Supports loading manifests from resources instead of files
- Derives chrome's manifests from content's without duplicating metadata
- Avoids all filesystem lookups for existing apps in content today
- Ensures all content embedders have a working shell connection with
capabilities

BUG=594419
==========

[Ken Rockot(use gerrit already), 2016-04-07 01:38:44]

The CQ bit was checked by rockot@chromium.org

[Ken Rockot(use gerrit already), 2016-04-07 01:38:45]

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

[commit-bot: I haz the power, 2016-04-07 01:39:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1828733004/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1828733004/240001

[commit-bot: I haz the power, 2016-04-07 03:00:08]

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

[commit-bot: I haz the power, 2016-04-07 03:00:09]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Ken Rockot(use gerrit already), 2016-04-07 05:38:35]

The CQ bit was checked by rockot@chromium.org

[commit-bot: I haz the power, 2016-04-07 05:38:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1828733004/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1828733004/240001

[commit-bot: I haz the power, 2016-04-07 08:47:05]

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

[commit-bot: I haz the power, 2016-04-07 08:47:07]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ken Rockot(use gerrit already), 2016-04-07 17:43:50]

The CQ bit was checked by rockot@chromium.org

[Ken Rockot(use gerrit already), 2016-04-07 17:43:50]

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

[commit-bot: I haz the power, 2016-04-07 17:44:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1828733004/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1828733004/280001

[commit-bot: I haz the power, 2016-04-07 18:56:42]

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2016-04-07 18:58:06]

Description was changed from

==========
Load application manifests from resources

- Renames content's builtin apps to content_browser and content_renderer
- Supports loading manifests from resources instead of files
- Derives chrome's manifests from content's without duplicating metadata
- Avoids all filesystem lookups for existing apps in content today
- Ensures all content embedders have a working shell connection with
capabilities

BUG=594419
==========

to

==========
Load application manifests from resources

- Renames content's builtin apps to content_browser and content_renderer
- Supports loading manifests from resources instead of files
- Derives chrome's manifests from content's without duplicating metadata
- Avoids all filesystem lookups for existing apps in content today
- Ensures all content embedders have a working shell connection with
capabilities

BUG=594419

Committed: https://crrev.com/00668f9780a1b1c6c1b1a1f2457418d0aecf9bb5
Cr-Commit-Position: refs/heads/master@{#385823}
==========

[commit-bot: I haz the power, 2016-04-07 18:58:08]

Patchset 15 (id:??) landed as
https://crrev.com/00668f9780a1b1c6c1b1a1f2457418d0aecf9bb5
Cr-Commit-Position: refs/heads/master@{#385823}