[headless] Make browser service manifest overlay configurable.

[headless] Make browser service manifest overlay configurable.

Headless embedders need to specify mojo services that they
expose to the renderer. This patch lets them list the services
in HeadlessBrowser::Options and generates the corresponding
browser manifest overlay in HeadlessContentBrowserClient.

BUG=546953
Committed: https://crrev.com/9ff09983783153837ac7337de33077c6ff2f1ba1
Cr-Commit-Position: refs/heads/master@{#432151}

[Eric Seckler, 2016-11-09 18:00:23]

eseckler@chromium.org changed reviewers:
+ skyostil@chromium.org

[Eric Seckler, 2016-11-09 18:00:24]

Quick draft trying to make the overlay json resource configurable. Sadly, this
doesn't actually work, because the resource pak that contains the resource can
only be loaded after ContentMain() is called (b/c the ResourceBundle isn't
initialized until HeadlessContentMainDelegate::PreSanboxStartup), but the
service manager is initialized during main thread startup.

Sami, can you think of a solution to this? I guess the alternative is to
programmatically fill the overlay? -- In which case I'll contact the service
folks about moving catalog::Entry (the class that handles serialization of the
manifest) to an accessible place...

wdyt?

[Sami, 2016-11-09 18:35:52]

How about instead of specifying the resource file we ask the embedder to list
the services (which they already do?) and we can generate the overlay json based
on that whenever we need it?

(Left a couple of other comments too before I noticed this was a draft :)

https://codereview.chromium.org/2489673005/diff/1/headless/lib/browser/devtoo...
File headless/lib/browser/devtools_api/client_api_generator.py (right):

https://codereview.chromium.org/2489673005/diff/1/headless/lib/browser/devtoo...
headless/lib/browser/devtools_api/client_api_generator.py:25:
os.path.join(module_path, os.pardir, os.pardir, os.pardir, os.pardir,
I guess your other patch accidentally ended up here too?

https://codereview.chromium.org/2489673005/diff/1/headless/public/headless_br...
File headless/public/headless_browser.h (right):

https://codereview.chromium.org/2489673005/diff/1/headless/public/headless_br...
headless/public/headless_browser.h:121: int mojo_service_manifest_resource_id;
Looks uninitialized?

[Eric Seckler, 2016-11-10 09:58:50]

On 2016/11/09 18:35:52, Sami wrote:
> How about instead of specifying the resource file we ask the embedder to list
> the services (which they already do?) and we can generate the overlay json
based
> on that whenever we need it?

I think we'd have to have the embedder list the services again, since they're
currently specified per WebContents and we need them before browser
initialization.
Will sketch that approach using catalog::Entry and then ping ben@ about it.

Btw, I tried specifying the manifest resource first b/c I have the feeling that
people will eventually want to do security reviews for manifests. Hope they'll
be fine with us building this up more-or-less dynamically for headless... :)

[Eric Seckler, 2016-11-10 12:36:21]

Description was changed from

==========
[headless] Make service manifest.json overlay configurable.

BUG=
==========

to

==========
[headless] Make service manifest.json overlay configurable.

BUG=546953
==========

[Eric Seckler, 2016-11-10 14:01:33]

Description was changed from

==========
[headless] Make service manifest.json overlay configurable.

BUG=546953
==========

to

==========
[headless] Make browser service manifest overlay configurable.

Headless embedders need to specify mojo services that they
expose to the renderer. This patch lets them list the services
in HeadlessBrowser::Options and generates the corresponding
browser manifest overlay in HeadlessContentBrowserClient.

BUG=546953
==========

[Eric Seckler, 2016-11-10 14:21:34]

eseckler@chromium.org changed reviewers:
+ ben@chromium.org

[Eric Seckler, 2016-11-10 14:21:35]

+ben@: Can you advise us whether it's okay to depend on catalog::Entry, or if we
need to do something about that? Thanks! :)

Context:

Our headless embedders expose some embedder-specific mojo services to the
renderer, which somehow have to make it into the browser service manifest. This
patch lets the embedders list the names of their services and generates the
corresponding browser manifest overlay in HeadlessContentBrowserClient.

We also looked into letting the embedder provide the overlay file themselves
(patchset 1), but I think we'd prefer generating its contents. Not sure how you
feel about either of these options? Let us know if you already had something
else in mind for situations like ours.

[Ben Goodger (Google), 2016-11-10 15:03:24]

On 2016/11/10 14:21:35, Eric Seckler wrote:
> +ben@: Can you advise us whether it's okay to depend on catalog::Entry, or if
we
> need to do something about that? Thanks! :)
> 
> Context:
> 
> Our headless embedders expose some embedder-specific mojo services to the
> renderer, which somehow have to make it into the browser service manifest.
This
> patch lets the embedders list the names of their services and generates the
> corresponding browser manifest overlay in HeadlessContentBrowserClient.
> 
> We also looked into letting the embedder provide the overlay file themselves
> (patchset 1), but I think we'd prefer generating its contents. Not sure how
you
> feel about either of these options? Let us know if you already had something
> else in mind for situations like ours.

1. So if they're expecting to provide these interfaces to renderers the empty
InterfaceProviderSpec you are specifying here won't be sufficient.
2. I don't want to leak Catalog implementation details.

I think the most practical solution here is to have them supply overlays for
their services, since they'll need to express the complete
InterfaceProviderSpec.

[Ben Goodger (Google), 2016-11-10 15:03:43]

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

[Ben Goodger (Google), 2016-11-10 15:03:57]

+ken fyi

[Eric Seckler, 2016-11-10 15:22:13]

Thanks, Ben!

On 2016/11/10 15:03:24, Ben Goodger (Google) wrote:
> On 2016/11/10 14:21:35, Eric Seckler wrote:
> > +ben@: Can you advise us whether it's okay to depend on catalog::Entry, or
if
> we
> > need to do something about that? Thanks! :)
> > 
> > Context:
> > 
> > Our headless embedders expose some embedder-specific mojo services to the
> > renderer, which somehow have to make it into the browser service manifest.
> This
> > patch lets the embedders list the names of their services and generates the
> > corresponding browser manifest overlay in HeadlessContentBrowserClient.
> > 
> > We also looked into letting the embedder provide the overlay file themselves
> > (patchset 1), but I think we'd prefer generating its contents. Not sure how
> you
> > feel about either of these options? Let us know if you already had something
> > else in mind for situations like ours.
> 
> 1. So if they're expecting to provide these interfaces to renderers the empty
> InterfaceProviderSpec you are specifying here won't be sufficient.
The generated spec includes the additional entries for the "provides" section,
do we need more than that? (This seems to work.)

> 2. I don't want to leak Catalog implementation details.
Sounds reasonable. I guess we could build the DictionaryValue ourselves, but
that's not really the point... :)

> I think the most practical solution here is to have them supply overlays for
> their services, since they'll need to express the complete
> InterfaceProviderSpec.
We ran into an initialization order issue with the first approach (embedder
resources are intialized after the ServiceManagerContext is created).
Sami, could we specify the embedder paks in HeadlessBrowser::Options as well,
and let the HeadlessContentMainDelegate load them for the embedder?

[Eric Seckler, 2016-11-11 15:24:51]

After a chat with Sami, we agreed that we'd like an API that avoids exposing
content details to headless embedders (i.e. manifest structure). We're now using
a template manifest value that we fill with service names, instead of the
catalog::Entry.

Sami, PTAL :)

[Sami, 2016-11-11 17:41:16]

Thanks, I think this is a pretty tidy way of handling this.

https://codereview.chromium.org/2489673005/diff/40001/headless/lib/browser/he...
File headless/lib/browser/headless_content_browser_client.cc (right):

https://codereview.chromium.org/2489673005/diff/40001/headless/lib/browser/he...
headless/lib/browser/headless_content_browser_client.cc:30: }  // namspace
namespace

https://codereview.chromium.org/2489673005/diff/40001/headless/public/headles...
File headless/public/headless_browser.h (right):

https://codereview.chromium.org/2489673005/diff/40001/headless/public/headles...
headless/public/headless_browser.h:121: std::set<std::string>
mojo_service_names;
Any reason for this not to be an unordered_set?

[Eric Seckler, 2016-11-14 10:23:14]

https://codereview.chromium.org/2489673005/diff/40001/headless/lib/browser/he...
File headless/lib/browser/headless_content_browser_client.cc (right):

https://codereview.chromium.org/2489673005/diff/40001/headless/lib/browser/he...
headless/lib/browser/headless_content_browser_client.cc:30: }  // namspace
On 2016/11/11 17:41:15, Sami wrote:
> namespace

Done.

https://codereview.chromium.org/2489673005/diff/40001/headless/public/headles...
File headless/public/headless_browser.h (right):

https://codereview.chromium.org/2489673005/diff/40001/headless/public/headles...
headless/public/headless_browser.h:121: std::set<std::string>
mojo_service_names;
On 2016/11/11 17:41:15, Sami wrote:
> Any reason for this not to be an unordered_set?

Nope, leftover from the previous approach. Done!

[Sami, 2016-11-14 21:22:37]

lgtm, thanks for fixing this!

https://codereview.chromium.org/2489673005/diff/60001/headless/lib/browser/he...
File headless/lib/browser/headless_content_browser_client.cc (right):

https://codereview.chromium.org/2489673005/diff/60001/headless/lib/browser/he...
headless/lib/browser/headless_content_browser_client.cc:8: #include <set>
Needed?

https://codereview.chromium.org/2489673005/diff/60001/headless/public/headles...
File headless/public/headless_browser.h (right):

https://codereview.chromium.org/2489673005/diff/60001/headless/public/headles...
headless/public/headless_browser.h:9: #include <set>
unordered_set

[Eric Seckler, 2016-11-15 08:46:17]

The CQ bit was checked by eseckler@chromium.org

[Eric Seckler, 2016-11-15 08:46:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
skyostil@chromium.org
Link to the patchset: https://codereview.chromium.org/2489673005/#ps80001
(title: "fix includes.")

[commit-bot: I haz the power, 2016-11-15 08:46:30]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Eric Seckler, 2016-11-15 08:46:58]

https://codereview.chromium.org/2489673005/diff/60001/headless/lib/browser/he...
File headless/lib/browser/headless_content_browser_client.cc (right):

https://codereview.chromium.org/2489673005/diff/60001/headless/lib/browser/he...
headless/lib/browser/headless_content_browser_client.cc:8: #include <set>
On 2016/11/14 21:22:37, Sami (slow -- traveling) wrote:
> Needed?

changed to unordered_set. we're iterating over one below, so I'd say it's needed
as per IWYU.

https://codereview.chromium.org/2489673005/diff/60001/headless/public/headles...
File headless/public/headless_browser.h (right):

https://codereview.chromium.org/2489673005/diff/60001/headless/public/headles...
headless/public/headless_browser.h:9: #include <set>
On 2016/11/14 21:22:37, Sami (slow -- traveling) wrote:
> unordered_set

Done.

[commit-bot: I haz the power, 2016-11-15 09:18:50]

Description was changed from

==========
[headless] Make browser service manifest overlay configurable.

Headless embedders need to specify mojo services that they
expose to the renderer. This patch lets them list the services
in HeadlessBrowser::Options and generates the corresponding
browser manifest overlay in HeadlessContentBrowserClient.

BUG=546953
==========

to

==========
[headless] Make browser service manifest overlay configurable.

Headless embedders need to specify mojo services that they
expose to the renderer. This patch lets them list the services
in HeadlessBrowser::Options and generates the corresponding
browser manifest overlay in HeadlessContentBrowserClient.

BUG=546953
==========

[commit-bot: I haz the power, 2016-11-15 09:18:52]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-11-15 09:21:07]

Description was changed from

==========
[headless] Make browser service manifest overlay configurable.

Headless embedders need to specify mojo services that they
expose to the renderer. This patch lets them list the services
in HeadlessBrowser::Options and generates the corresponding
browser manifest overlay in HeadlessContentBrowserClient.

BUG=546953
==========

to

==========
[headless] Make browser service manifest overlay configurable.

Headless embedders need to specify mojo services that they
expose to the renderer. This patch lets them list the services
in HeadlessBrowser::Options and generates the corresponding
browser manifest overlay in HeadlessContentBrowserClient.

BUG=546953

Committed: https://crrev.com/9ff09983783153837ac7337de33077c6ff2f1ba1
Cr-Commit-Position: refs/heads/master@{#432151}
==========

[commit-bot: I haz the power, 2016-11-15 09:21:09]

Patchset 5 (id:??) landed as
https://crrev.com/9ff09983783153837ac7337de33077c6ff2f1ba1
Cr-Commit-Position: refs/heads/master@{#432151}