FrameMojoShell provides services to mojo apps.

FrameMojoShell provides services to mojo apps.

BUG=479935
TEST=Manually tested by connecting PlatformVerification service from
MojoCdmService.

Committed: https://crrev.com/7be3bba3cba700c96d7d9bb940c8d57604720ceb
Cr-Commit-Position: refs/heads/master@{#337953}

[xhwang, 2015-06-29 20:25:51]

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

[xhwang, 2015-06-29 20:25:51]

Now a mojo app running in the browser/utility process can use services provided
in the browser (RFHI).

PTAL!

https://codereview.chromium.org/1209283003/diff/1/content/browser/frame_host/...
File content/browser/frame_host/frame_mojo_shell.h (right):

https://codereview.chromium.org/1209283003/diff/1/content/browser/frame_host/...
content/browser/frame_host/frame_mojo_shell.h:42:
scoped_ptr<ServiceRegistryImpl> service_registry_;
I am debating between doing this and using the service registry in RFHI
directly. If latter, the mojo app running in the utility process will be able to
use all services RFHI provides (the same set of services provided to the render
frame in the render process). It seems a good idea for code reuse, but I am not
sure whether we are over-exposing services. For example, the service runnin in
the utility process can in turn ask for a Shell service...

[Ken Rockot(use gerrit already), 2015-07-06 18:58:00]

https://codereview.chromium.org/1209283003/diff/1/content/browser/frame_host/...
File content/browser/frame_host/frame_mojo_shell.cc (right):

https://codereview.chromium.org/1209283003/diff/1/content/browser/frame_host/...
content/browser/frame_host/frame_mojo_shell.cc:20: // A ServiceProvider
implementation based on ServiceRegistryImpl. Strongly bound
I think this class might be unnecessary if FrameMojoShell hosts a
ServiceProviderImpl (see other comments).

https://codereview.chromium.org/1209283003/diff/1/content/browser/frame_host/...
content/browser/frame_host/frame_mojo_shell.cc:64: new
ProxyServiceProvider(GetServiceRegistry(), GetProxy(&frame_services));
Though it may be true in practice (and I may be missing something), I don't see
any reason why we can assume this won't outlive the frame host which owns the
ServiceRegistry, and that could be a problem.

If you go with the ServiceProviderImpl suggestion outlined in other comments, it
doesn't matter though, because FrameMojoHost can instead just own a
WeakBindingSet<mojo::ServiceProvider> and add new bindings to its
ServiceProviderImpl here.

https://codereview.chromium.org/1209283003/diff/1/content/browser/frame_host/...
File content/browser/frame_host/frame_mojo_shell.h (right):

https://codereview.chromium.org/1209283003/diff/1/content/browser/frame_host/...
content/browser/frame_host/frame_mojo_shell.h:42:
scoped_ptr<ServiceRegistryImpl> service_registry_;
On 2015/06/29 20:25:51, xhwang wrote:
> I am debating between doing this and using the service registry in RFHI
> directly. If latter, the mojo app running in the utility process will be able
to
> use all services RFHI provides (the same set of services provided to the
render
> frame in the render process). It seems a good idea for code reuse, but I am
not
> sure whether we are over-exposing services. For example, the service runnin in
> the utility process can in turn ask for a Shell service...

What you're doing here seems right to me. We should be explicit about what
services render frames expose to other apps.

Eventually a renderer will be just another OOP Mojo app rendering web contents.
We can think of this as the set of services that would be exposed by that app.

This is a good interim solution which mocks mojo shell integration but avoids
the massive amount of effort it would take to make the renderer a "real" Mojo
app.

https://codereview.chromium.org/1209283003/diff/1/content/browser/frame_host/...
content/browser/frame_host/frame_mojo_shell.h:43: 
Actually I think ServiceRegistry is overkill here since there's no use for the
remote service bits which comprise half of its interface.

Can you get away with making this a ServiceProviderImpl instead?
(//mojo/application/public/cpp/service_provider_impl.h). And of course
GetServiceRegistry would change to GetServiceProvider, and
OverrideFrameMojoShellServices have to take a ServiceProviderImpl* rather than a
ServiceRegistry*.

This should be OK (I think...) because if a renderer-side service needs to
access remote services it should be using C2A rather than some one-off
ServiceRegistry instance.

https://codereview.chromium.org/1209283003/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_impl.cc (left):

https://codereview.chromium.org/1209283003/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:1579:
GetContentClient()->browser()->OverrideRenderFrameMojoServices(
So how does this get called now?

https://codereview.chromium.org/1209283003/diff/1/content/browser/mojo/mojo_s...
File content/browser/mojo/mojo_shell_context.cc (right):

https://codereview.chromium.org/1209283003/diff/1/content/browser/mojo/mojo_s...
content/browser/mojo/mojo_shell_context.cc:96:
shell_context_->ConnectToApplicationOnOwnThread(
nit: this is my fault, but please add braces around the if body since it's > 1
line.

[xhwang, 2015-07-07 19:21:05]

I replaced ProxyServiceProvider with WeakBindingSet. PTAL.

Also, sorry for the rebase noise.

https://codereview.chromium.org/1209283003/diff/1/content/browser/frame_host/...
File content/browser/frame_host/frame_mojo_shell.cc (right):

https://codereview.chromium.org/1209283003/diff/1/content/browser/frame_host/...
content/browser/frame_host/frame_mojo_shell.cc:20: // A ServiceProvider
implementation based on ServiceRegistryImpl. Strongly bound
On 2015/07/06 18:58:00, Ken Rockot wrote:
> I think this class might be unnecessary if FrameMojoShell hosts a
> ServiceProviderImpl (see other comments).

Removed in favor of WeakBindingSet.

https://codereview.chromium.org/1209283003/diff/1/content/browser/frame_host/...
content/browser/frame_host/frame_mojo_shell.cc:64: new
ProxyServiceProvider(GetServiceRegistry(), GetProxy(&frame_services));
On 2015/07/06 18:58:00, Ken Rockot wrote:
> Though it may be true in practice (and I may be missing something), I don't
see
> any reason why we can assume this won't outlive the frame host which owns the
> ServiceRegistry, and that could be a problem.
> 
> If you go with the ServiceProviderImpl suggestion outlined in other comments,
it
> doesn't matter though, because FrameMojoHost can instead just own a
> WeakBindingSet<mojo::ServiceProvider> and add new bindings to its
> ServiceProviderImpl here.

Use SRI + WBS now.

https://codereview.chromium.org/1209283003/diff/1/content/browser/frame_host/...
File content/browser/frame_host/frame_mojo_shell.h (right):

https://codereview.chromium.org/1209283003/diff/1/content/browser/frame_host/...
content/browser/frame_host/frame_mojo_shell.h:42:
scoped_ptr<ServiceRegistryImpl> service_registry_;
On 2015/07/06 18:58:00, Ken Rockot wrote:
> On 2015/06/29 20:25:51, xhwang wrote:
> > I am debating between doing this and using the service registry in RFHI
> > directly. If latter, the mojo app running in the utility process will be
able
> to
> > use all services RFHI provides (the same set of services provided to the
> render
> > frame in the render process). It seems a good idea for code reuse, but I am
> not
> > sure whether we are over-exposing services. For example, the service runnin
in
> > the utility process can in turn ask for a Shell service...
> 
> What you're doing here seems right to me. We should be explicit about what
> services render frames expose to other apps.
> 
> Eventually a renderer will be just another OOP Mojo app rendering web
contents.
> We can think of this as the set of services that would be exposed by that app.
> 
> This is a good interim solution which mocks mojo shell integration but avoids
> the massive amount of effort it would take to make the renderer a "real" Mojo
> app.

Acknowledged.

https://codereview.chromium.org/1209283003/diff/1/content/browser/frame_host/...
content/browser/frame_host/frame_mojo_shell.h:43: 
On 2015/07/06 18:58:00, Ken Rockot wrote:
> Actually I think ServiceRegistry is overkill here since there's no use for the
> remote service bits which comprise half of its interface.
> 
> Can you get away with making this a ServiceProviderImpl instead?
> (//mojo/application/public/cpp/service_provider_impl.h). And of course
> GetServiceRegistry would change to GetServiceProvider, and
> OverrideFrameMojoShellServices have to take a ServiceProviderImpl* rather than
a
> ServiceRegistry*.
> 
> This should be OK (I think...) because if a renderer-side service needs to
> access remote services it should be using C2A rather than some one-off
> ServiceRegistry instance.

As discussed offline, the current ServiceProviderImpl is pretty hard to use as
is. I could add a content::ServiceProviderImpl but that needs to be in
content/public/common, which isn't trivial. So  we'll keep using the SRI for
now.

Also, I can use WeakBindingSet here so we don't need ProxyServiceProvider
anymore.

https://codereview.chromium.org/1209283003/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_impl.cc (left):

https://codereview.chromium.org/1209283003/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:1579:
GetContentClient()->browser()->OverrideRenderFrameMojoServices(
On 2015/07/06 18:58:00, Ken Rockot wrote:
> So how does this get called now?

This call is renamed to OverrideFrameMojoShellServices and is called in
FrameMojoShell now. In the future we may need OverrideRenderFrameMojoServices
again and we can add it back then.

https://codereview.chromium.org/1209283003/diff/1/content/browser/mojo/mojo_s...
File content/browser/mojo/mojo_shell_context.cc (right):

https://codereview.chromium.org/1209283003/diff/1/content/browser/mojo/mojo_s...
content/browser/mojo/mojo_shell_context.cc:96:
shell_context_->ConnectToApplicationOnOwnThread(
On 2015/07/06 18:58:00, Ken Rockot wrote:
> nit: this is my fault, but please add braces around the if body since it's > 1
> line.

Done.

[Ken Rockot(use gerrit already), 2015-07-07 19:25:46]

lgtm

https://codereview.chromium.org/1209283003/diff/20001/content/browser/frame_h...
File content/browser/frame_host/frame_mojo_shell.cc (right):

https://codereview.chromium.org/1209283003/diff/20001/content/browser/frame_h...
content/browser/frame_host/frame_mojo_shell.cc:13: #include
"third_party/mojo/src/mojo/public/cpp/bindings/strong_binding.h"
nit: can remove this now

[xhwang, 2015-07-07 19:31:41]

https://codereview.chromium.org/1209283003/diff/20001/content/browser/frame_h...
File content/browser/frame_host/frame_mojo_shell.cc (right):

https://codereview.chromium.org/1209283003/diff/20001/content/browser/frame_h...
content/browser/frame_host/frame_mojo_shell.cc:13: #include
"third_party/mojo/src/mojo/public/cpp/bindings/strong_binding.h"
On 2015/07/07 19:25:46, Ken Rockot wrote:
> nit: can remove this now

Done.

[xhwang, 2015-07-07 19:32:06]

jam: Please OWNERs review!

[jam, 2015-07-08 21:17:01]

lgtm

[xhwang, 2015-07-08 21:26:00]

The CQ bit was checked by xhwang@chromium.org

[xhwang, 2015-07-08 21:26:00]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org
Link to the patchset: https://codereview.chromium.org/1209283003/#ps40001
(title: "address comments")

[xhwang, 2015-07-08 21:26:20]

The CQ bit was unchecked by xhwang@chromium.org

[xhwang, 2015-07-08 22:14:43]

rebase only

[xhwang, 2015-07-08 22:27:50]

The CQ bit was checked by xhwang@chromium.org

[xhwang, 2015-07-08 22:27:51]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
jam@chromium.org
Link to the patchset: https://codereview.chromium.org/1209283003/#ps60001
(title: "rebase only")

[commit-bot: I haz the power, 2015-07-08 22:30:12]

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

[commit-bot: I haz the power, 2015-07-08 23:55:39]

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

[commit-bot: I haz the power, 2015-07-08 23:55:41]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

(exceeded global retry quota)

[xhwang, 2015-07-09 00:17:34]

The CQ bit was checked by xhwang@chromium.org

[commit-bot: I haz the power, 2015-07-09 00:18:12]

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

[commit-bot: I haz the power, 2015-07-09 00:34:34]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-07-09 00:35:28]

Patchset 4 (id:??) landed as
https://crrev.com/7be3bba3cba700c96d7d9bb940c8d57604720ceb
Cr-Commit-Position: refs/heads/master@{#337953}