Move Mojo channel initialization closer to IPC::Channel setup

Move Mojo channel initialization closer to IPC::Channel setup

This CL introduces two new content classes:
 - MojoApplicationHost encapsulates what's needed to host a Mojo App using Chrome IPC to bootstrap.
 - MojoApplication represents what's needed to be a Mojo App using Chrome IPC to bootstrap.

The RenderProcess and RenderProcessHost interfaces are replaced with WebUISetup and WebUISetupClient interfaces. This way the interface is more specific to the service of setting up WebUI.

WebUISetupClient is empty and uninteresting. RenderProcessHostImpl no longer deals with WebUI setup. That is all done directly by RenderViewHostImpl by talking to the WebUISetup service.

Service names get defined in content/common/mojo/mojo_service_names.{h,cc}.

R=sky@chromium.org, tsepez@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=265693

[darin (slow to review), 2014-04-16 05:46:18]

This is a big change to the way we have had things setup in content/ for Mojo.
Let me know what you think. Maybe some things should be different. I'm curious
what you think of the I-prefix, etc.

[sky, 2014-04-16 17:18:10]

Generally I like it!

https://codereview.chromium.org/236813002/diff/80001/content/browser/mojo/moj...
File content/browser/mojo/mojo_application_host.cc (right):

https://codereview.chromium.org/236813002/diff/80001/content/browser/mojo/moj...
content/browser/mojo/mojo_application_host.cc:53: base::PlatformFile client_file
=
DCHECK(client_handle_.is_valid()) ?

https://codereview.chromium.org/236813002/diff/80001/content/browser/mojo/moj...
content/browser/mojo/mojo_application_host.cc:55: return sender->Send(new
MojoMsg_Activate(
Do we need to do this when running single-process?

https://codereview.chromium.org/236813002/diff/80001/content/browser/mojo/moj...
File content/browser/mojo/mojo_application_host.h (right):

https://codereview.chromium.org/236813002/diff/80001/content/browser/mojo/moj...
content/browser/mojo/mojo_application_host.h:20: class MojoApplicationHost {
Add description.

https://codereview.chromium.org/236813002/diff/80001/content/browser/mojo/moj...
content/browser/mojo/mojo_application_host.h:37: };
DISALLOW...

https://codereview.chromium.org/236813002/diff/80001/content/browser/renderer...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/236813002/diff/80001/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.cc:554: InitializeMojo();
We don't need to wait for process to launch?

https://codereview.chromium.org/236813002/diff/80001/content/renderer/mojo/mo...
File content/renderer/mojo/mojo_application.h (right):

https://codereview.chromium.org/236813002/diff/80001/content/renderer/mojo/mo...
content/renderer/mojo/mojo_application.h:19: class MojoApplication {
Description?

https://codereview.chromium.org/236813002/diff/80001/content/renderer/mojo/mo...
content/renderer/mojo/mojo_application.h:33: mojo::ShellClient* shell_client_;
Document ownership.

https://codereview.chromium.org/236813002/diff/80001/content/renderer/mojo/mo...
content/renderer/mojo/mojo_application.h:34: };
DISALLOW...

[DaveMoore, 2014-04-16 23:08:36]

https://codereview.chromium.org/236813002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://codereview.chromium.org/236813002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_process_host_impl.h:78: : public
IRenderProcessHost,
Using IRenderProcessHost makes this read really poorly. You have three classes
here, FooImpl, IFoo and Foo. There's no clear meaning to what each of them are.

https://codereview.chromium.org/236813002/diff/80001/content/browser/renderer...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://codereview.chromium.org/236813002/diff/80001/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.h:78: class
CONTENT_EXPORT RenderProcessHostImpl
This seems bad. You have FooImpl, IFoo and Foo all in the same place. I think
the IRenderProcessHost needs to be more descriptive about exactly what part of
the interface it's declaring.

[darin (slow to review), 2014-04-16 23:26:04]

Thanks for the feedback!

https://codereview.chromium.org/236813002/diff/80001/content/browser/mojo/moj...
File content/browser/mojo/mojo_application_host.cc (right):

https://codereview.chromium.org/236813002/diff/80001/content/browser/mojo/moj...
content/browser/mojo/mojo_application_host.cc:53: base::PlatformFile client_file
=
On 2014/04/16 17:18:10, sky wrote:
> DCHECK(client_handle_.is_valid()) ?

Good idea.

https://codereview.chromium.org/236813002/diff/80001/content/browser/mojo/moj...
content/browser/mojo/mojo_application_host.cc:55: return sender->Send(new
MojoMsg_Activate(
On 2014/04/16 17:18:10, sky wrote:
> Do we need to do this when running single-process?

Good question. I thought we would still want to establish the Mojo channel. I'll
test it in single process mode :-)

https://codereview.chromium.org/236813002/diff/80001/content/browser/mojo/moj...
File content/browser/mojo/mojo_application_host.h (right):

https://codereview.chromium.org/236813002/diff/80001/content/browser/mojo/moj...
content/browser/mojo/mojo_application_host.h:20: class MojoApplicationHost {
On 2014/04/16 17:18:10, sky wrote:
> Add description.

Will do.

https://codereview.chromium.org/236813002/diff/80001/content/browser/mojo/moj...
content/browser/mojo/mojo_application_host.h:37: };
On 2014/04/16 17:18:10, sky wrote:
> DISALLOW...

Will do.

https://codereview.chromium.org/236813002/diff/80001/content/browser/renderer...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/236813002/diff/80001/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.cc:554: InitializeMojo();
On 2014/04/16 17:18:10, sky wrote:
> We don't need to wait for process to launch?

No, we don't. This is all just setting up the channels and pipes locally. We can
start writing to them even if there is no consumer yet. (The EndToEnd test
passes!)

https://codereview.chromium.org/236813002/diff/80001/content/browser/renderer...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://codereview.chromium.org/236813002/diff/80001/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.h:78: class
CONTENT_EXPORT RenderProcessHostImpl
On 2014/04/16 23:08:36, DaveMoore wrote:
> This seems bad. You have FooImpl, IFoo and Foo all in the same place. I think
> the IRenderProcessHost needs to be more descriptive about exactly what part of
> the interface it's declaring.

Yeah, I wanted to see what this would feel like. Do you have any suggestions on
alternate names?

The concerns I have:

1- I'm not a big fan of adding "Mojo" into the name as that seems like something
we shouldn't have to think about. These are just interfaces to be implemented.

2- While this interface currently has very few methods, I expect it to grow to
have all of the methods that represent the "render process host" from the point
of view of the "render process". Given that, the name should probably include
the term "RenderProcessHost" (and "RenderProcess" for the other end).

https://codereview.chromium.org/236813002/diff/80001/content/renderer/mojo/mo...
File content/renderer/mojo/mojo_application.h (right):

https://codereview.chromium.org/236813002/diff/80001/content/renderer/mojo/mo...
content/renderer/mojo/mojo_application.h:19: class MojoApplication {
On 2014/04/16 17:18:10, sky wrote:
> Description?

Will add.

https://codereview.chromium.org/236813002/diff/80001/content/renderer/mojo/mo...
content/renderer/mojo/mojo_application.h:33: mojo::ShellClient* shell_client_;
On 2014/04/16 17:18:10, sky wrote:
> Document ownership.

Good idea.

https://codereview.chromium.org/236813002/diff/80001/content/renderer/mojo/mo...
content/renderer/mojo/mojo_application.h:34: };
On 2014/04/16 17:18:10, sky wrote:
> DISALLOW...

Thanks.

[DaveMoore, 2014-04-17 17:56:09]

https://codereview.chromium.org/236813002/diff/80001/content/browser/renderer...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://codereview.chromium.org/236813002/diff/80001/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.h:78: class
CONTENT_EXPORT RenderProcessHostImpl
So what will determine what's in the new interface as opposed to the old one? At
the end will the existing RenderProcessHost interface go away?

On 2014/04/16 23:26:04, darin wrote:
> On 2014/04/16 23:08:36, DaveMoore wrote:
> > This seems bad. You have FooImpl, IFoo and Foo all in the same place. I
think
> > the IRenderProcessHost needs to be more descriptive about exactly what part
of
> > the interface it's declaring.
> 
> Yeah, I wanted to see what this would feel like. Do you have any suggestions
on
> alternate names?
> 
> The concerns I have:
> 
> 1- I'm not a big fan of adding "Mojo" into the name as that seems like
something
> we shouldn't have to think about. These are just interfaces to be implemented.
> 
> 2- While this interface currently has very few methods, I expect it to grow to
> have all of the methods that represent the "render process host" from the
point
> of view of the "render process". Given that, the name should probably include
> the term "RenderProcessHost" (and "RenderProcess" for the other end).

[darin (slow to review), 2014-04-22 06:45:36]

OK, please take another look. This is a bit different now.

The biggest change is essentially this:
render_process.mojom -> web_ui_setup.mojom

[koz (OOO until 15th September), 2014-04-22 07:09:58]

https://codereview.chromium.org/236813002/diff/80001/content/browser/renderer...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://codereview.chromium.org/236813002/diff/80001/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.h:78: class
CONTENT_EXPORT RenderProcessHostImpl
On 2014/04/17 17:56:09, DaveMoore wrote:
> So what will determine what's in the new interface as opposed to the old one?
At
> the end will the existing RenderProcessHost interface go away?
> 
> On 2014/04/16 23:26:04, darin wrote:
> > On 2014/04/16 23:08:36, DaveMoore wrote:
> > > This seems bad. You have FooImpl, IFoo and Foo all in the same place. I
> think
> > > the IRenderProcessHost needs to be more descriptive about exactly what
part
> of
> > > the interface it's declaring.
> > 
> > Yeah, I wanted to see what this would feel like. Do you have any suggestions
> on
> > alternate names?
> > 
> > The concerns I have:
> > 
> > 1- I'm not a big fan of adding "Mojo" into the name as that seems like
> something
> > we shouldn't have to think about. These are just interfaces to be
implemented.
> > 
> > 2- While this interface currently has very few methods, I expect it to grow
to
> > have all of the methods that represent the "render process host" from the
> point
> > of view of the "render process". Given that, the name should probably
include
> > the term "RenderProcessHost" (and "RenderProcess" for the other end).
> 

It's true that the mojo interfaces are just regular interfaces, but they have a
specific use in mind, that is, to receive messages from a mojo peer, which in
practice makes them quite special (they use special mojo types, can be invoked
cross-process and cross-privilege barrier...) For this reason IFoo and Foo are
semantically different, so it makes sense to distinguish them.

I think FooMojo reads a bit funny, because to me it looks like the Mojo
implementation of Foo. IFoo makes sense to me, but I think it's perhaps too
general because the I prefix typically just says it's an interface, not a
special "between-peer", mojo-ey interface.

How about mojom::Foo? I find having #include "foo.mojom.h" instead of #include
"foo.h" very useful, and I think it would be equally helpful to be able to see
which interfaces are mojo ones just by their name. Using a namespace instead of
a Mojom prefix or suffix has the benefit that we don't have to mangle the names
we use in .mojom files.

https://codereview.chromium.org/236813002/diff/200001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/236813002/diff/200001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.cc:705:
web_ui_setup->SetWebUIHandle(GetRoutingID(), handle.Pass());
Rather than pushing handles across to the client, should the client not
Connect() to services it needs by itself? E.g.

interface RenderProcessHost {
  ConnectToView(int32 view_routing_id, string service_name, handle<message_pipe>
view_service);
}

or

interface RenderProcessHost {
  ConnectToView(int32 view_routing_id, handle<message_pipe> view_shell);
}

On the host side we could poke service loaders into the appropriate places to
handle client Connect() calls as we determine what the client has permission to
access.

[sky, 2014-04-22 15:52:14]

https://codereview.chromium.org/236813002/diff/200001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://codereview.chromium.org/236813002/diff/200001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.h:248: void
ConnectTo(const char* service_name,
Is service_name always going to be well known that a char* is good here? Would a
const std::string& be better?

https://codereview.chromium.org/236813002/diff/200001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/236813002/diff/200001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.cc:705:
web_ui_setup->SetWebUIHandle(GetRoutingID(), handle.Pass());
On 2014/04/22 07:09:59, koz wrote:
> Rather than pushing handles across to the client, should the client not
> Connect() to services it needs by itself? E.g.
> 
> interface RenderProcessHost {
>   ConnectToView(int32 view_routing_id, string service_name,
handle<message_pipe>
> view_service);
> }
> 
> or
> 
> interface RenderProcessHost {
>   ConnectToView(int32 view_routing_id, handle<message_pipe> view_shell);
> }
> 
> On the host side we could poke service loaders into the appropriate places to
> handle client Connect() calls as we determine what the client has permission
to
> access.

This model (pushing to the client) matches how WebUI is typically structured.
See https://codereview.chromium.org/222543004 for flow of typical webui page.

https://codereview.chromium.org/236813002/diff/200001/content/child/child_thr...
File content/child/child_thread.cc (right):

https://codereview.chromium.org/236813002/diff/200001/content/child/child_thr...
content/child/child_thread.cc:357: // By default, we handle no services.
If we get here does it mean someone is sending the wrong thing to a thread?
Should we take more evasive action?

https://codereview.chromium.org/236813002/diff/200001/content/renderer/render...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/236813002/diff/200001/content/renderer/render...
content/renderer/render_thread_impl.cc:1212: }
DCHECK or something if we hit the else?

https://codereview.chromium.org/236813002/diff/200001/content/renderer/web_ui...
File content/renderer/web_ui_setup_impl.cc (right):

https://codereview.chromium.org/236813002/diff/200001/content/renderer/web_ui...
content/renderer/web_ui_setup_impl.cc:14: new WebUISetupImpl(handle.Pass());
Document lifetime. In particular how this is destroyed.

https://codereview.chromium.org/236813002/diff/200001/content/renderer/web_ui...
File content/renderer/web_ui_setup_impl.h (right):

https://codereview.chromium.org/236813002/diff/200001/content/renderer/web_ui...
content/renderer/web_ui_setup_impl.h:33: };
DISALLOW_...

https://codereview.chromium.org/236813002/diff/200001/mojo/common/channel_init.h
File mojo/common/channel_init.h (right):

https://codereview.chromium.org/236813002/diff/200001/mojo/common/channel_ini...
mojo/common/channel_init.h:26: // ChannelInit handle creation (and destruction)
of the mojo channel. It is
Is there a reason you don't like naming this MojoChannelInit? ChannelInit seems
awfully vague, especially when the description says 'mojo channel'.

[darin (slow to review), 2014-04-22 17:16:43]

On 2014/04/22 15:52:14, sky wrote:
>
https://codereview.chromium.org/236813002/diff/200001/content/browser/rendere...
> File content/browser/renderer_host/render_process_host_impl.h (right):
> 
>
https://codereview.chromium.org/236813002/diff/200001/content/browser/rendere...
> content/browser/renderer_host/render_process_host_impl.h:248: void
> ConnectTo(const char* service_name,
> Is service_name always going to be well known that a char* is good here? Would
a
> const std::string& be better?
> 
>
https://codereview.chromium.org/236813002/diff/200001/content/browser/rendere...
> File content/browser/renderer_host/render_view_host_impl.cc (right):
> 
>
https://codereview.chromium.org/236813002/diff/200001/content/browser/rendere...
> content/browser/renderer_host/render_view_host_impl.cc:705:
> web_ui_setup->SetWebUIHandle(GetRoutingID(), handle.Pass());
> On 2014/04/22 07:09:59, koz wrote:
> > Rather than pushing handles across to the client, should the client not
> > Connect() to services it needs by itself? E.g.
> > 
> > interface RenderProcessHost {
> >   ConnectToView(int32 view_routing_id, string service_name,
> handle<message_pipe>
> > view_service);
> > }
> > 
> > or
> > 
> > interface RenderProcessHost {
> >   ConnectToView(int32 view_routing_id, handle<message_pipe> view_shell);
> > }
> > 
> > On the host side we could poke service loaders into the appropriate places
to
> > handle client Connect() calls as we determine what the client has permission
> to
> > access.
> 
> This model (pushing to the client) matches how WebUI is typically structured.
> See https://codereview.chromium.org/222543004 for flow of typical webui page.
> 
>
https://codereview.chromium.org/236813002/diff/200001/content/child/child_thr...
> File content/child/child_thread.cc (right):
> 
>
https://codereview.chromium.org/236813002/diff/200001/content/child/child_thr...
> content/child/child_thread.cc:357: // By default, we handle no services.
> If we get here does it mean someone is sending the wrong thing to a thread?
> Should we take more evasive action?
> 
>
https://codereview.chromium.org/236813002/diff/200001/content/renderer/render...
> File content/renderer/render_thread_impl.cc (right):
> 
>
https://codereview.chromium.org/236813002/diff/200001/content/renderer/render...
> content/renderer/render_thread_impl.cc:1212: }
> DCHECK or something if we hit the else?
> 
>
https://codereview.chromium.org/236813002/diff/200001/content/renderer/web_ui...
> File content/renderer/web_ui_setup_impl.cc (right):
> 
>
https://codereview.chromium.org/236813002/diff/200001/content/renderer/web_ui...
> content/renderer/web_ui_setup_impl.cc:14: new WebUISetupImpl(handle.Pass());
> Document lifetime. In particular how this is destroyed.
> 
>
https://codereview.chromium.org/236813002/diff/200001/content/renderer/web_ui...
> File content/renderer/web_ui_setup_impl.h (right):
> 
>
https://codereview.chromium.org/236813002/diff/200001/content/renderer/web_ui...
> content/renderer/web_ui_setup_impl.h:33: };
> DISALLOW_...
> 
>
https://codereview.chromium.org/236813002/diff/200001/mojo/common/channel_init.h
> File mojo/common/channel_init.h (right):
> 
>
https://codereview.chromium.org/236813002/diff/200001/mojo/common/channel_ini...
> mojo/common/channel_init.h:26: // ChannelInit handle creation (and
destruction)
> of the mojo channel. It is
> Is there a reason you don't like naming this MojoChannelInit? ChannelInit
seems
> awfully vague, especially when the description says 'mojo channel'.

Oh, it was just that the Mojo-prefix seemed redundant given that this lives
in the mojo namespace. I should have called that out in the CL description.
It is mojo::common::MojoChannelInit, so not as bad as mojo::MojoChannelInit.
I also wonder why this doesn't live in mojo/embedder/.

[darin (slow to review), 2014-04-22 17:28:17]

On 2014/04/22 15:52:14, sky wrote:
>
https://codereview.chromium.org/236813002/diff/200001/content/browser/rendere...
> File content/browser/renderer_host/render_process_host_impl.h (right):
> 
>
https://codereview.chromium.org/236813002/diff/200001/content/browser/rendere...
> content/browser/renderer_host/render_process_host_impl.h:248: void
> ConnectTo(const char* service_name,
> Is service_name always going to be well known that a char* is good here? Would
a
> const std::string& be better?

Yeah, they are likely to always be pre-defined constants. I changed it to
use base::StringPiece.


>
https://codereview.chromium.org/236813002/diff/200001/content/child/child_thr...
> content/child/child_thread.cc:357: // By default, we handle no services.
> If we get here does it mean someone is sending the wrong thing to a thread?
> Should we take more evasive action?

Hmm, yeah... come to think of it, a NOTREACHED() assertion would
probably be helpful.


>
https://codereview.chromium.org/236813002/diff/200001/content/renderer/render...
> File content/renderer/render_thread_impl.cc (right):
> 
>
https://codereview.chromium.org/236813002/diff/200001/content/renderer/render...
> content/renderer/render_thread_impl.cc:1212: }
> DCHECK or something if we hit the else?

Good idea.


>
https://codereview.chromium.org/236813002/diff/200001/content/renderer/web_ui...
> File content/renderer/web_ui_setup_impl.cc (right):
> 
>
https://codereview.chromium.org/236813002/diff/200001/content/renderer/web_ui...
> content/renderer/web_ui_setup_impl.cc:14: new WebUISetupImpl(handle.Pass());
> Document lifetime. In particular how this is destroyed.

Done. The idea here was to bind the object lifetime to the pipe. That
seems like a pattern we may frequently apply.


>
https://codereview.chromium.org/236813002/diff/200001/content/renderer/web_ui...
> File content/renderer/web_ui_setup_impl.h (right):
> 
>
https://codereview.chromium.org/236813002/diff/200001/content/renderer/web_ui...
> content/renderer/web_ui_setup_impl.h:33: };
> DISALLOW_...

Thanks!

[darin (slow to review), 2014-04-22 17:32:15]

On 2014/04/22 07:09:58, koz wrote:
>
https://codereview.chromium.org/236813002/diff/80001/content/browser/renderer...
> File content/browser/renderer_host/render_process_host_impl.h (right):
> 
>
https://codereview.chromium.org/236813002/diff/80001/content/browser/renderer...
> content/browser/renderer_host/render_process_host_impl.h:78: class
> CONTENT_EXPORT RenderProcessHostImpl
> On 2014/04/17 17:56:09, DaveMoore wrote:
> > So what will determine what's in the new interface as opposed to the old
one?
> At
> > the end will the existing RenderProcessHost interface go away?
> > 
> > On 2014/04/16 23:26:04, darin wrote:
> > > On 2014/04/16 23:08:36, DaveMoore wrote:
> > > > This seems bad. You have FooImpl, IFoo and Foo all in the same place. I
> > think
> > > > the IRenderProcessHost needs to be more descriptive about exactly what
> part
> > of
> > > > the interface it's declaring.
> > > 
> > > Yeah, I wanted to see what this would feel like. Do you have any
suggestions
> > on
> > > alternate names?
> > > 
> > > The concerns I have:
> > > 
> > > 1- I'm not a big fan of adding "Mojo" into the name as that seems like
> > something
> > > we shouldn't have to think about. These are just interfaces to be
> implemented.
> > > 
> > > 2- While this interface currently has very few methods, I expect it to
grow
> to
> > > have all of the methods that represent the "render process host" from the
> > point
> > > of view of the "render process". Given that, the name should probably
> include
> > > the term "RenderProcessHost" (and "RenderProcess" for the other end).
> > 

This is probably good fodder for a mojo-dev@ thread :)


> It's true that the mojo interfaces are just regular interfaces, but they have
a
> specific use in mind, that is, to receive messages from a mojo peer, which in
> practice makes them quite special (they use special mojo types, can be invoked
> cross-process and cross-privilege barrier...) For this reason IFoo and Foo are
> semantically different, so it makes sense to distinguish them.
> 
> I think FooMojo reads a bit funny, because to me it looks like the Mojo
> implementation of Foo. IFoo makes sense to me, but I think it's perhaps too
> general because the I prefix typically just says it's an interface, not a
> special "between-peer", mojo-ey interface.
> 
> How about mojom::Foo? I find having #include "foo.mojom.h" instead of #include
> "foo.h" very useful, and I think it would be equally helpful to be able to see
> which interfaces are mojo ones just by their name. Using a namespace instead
of
> a Mojom prefix or suffix has the benefit that we don't have to mangle the
names
> we use in .mojom files.

We actually discussed the nested mojom:: namespace idea a bunch. I think
it has some nice properties. Imagine content::mojom::Foo, etc.


>
https://codereview.chromium.org/236813002/diff/200001/content/browser/rendere...
> File content/browser/renderer_host/render_view_host_impl.cc (right):
> 
>
https://codereview.chromium.org/236813002/diff/200001/content/browser/rendere...
> content/browser/renderer_host/render_view_host_impl.cc:705:
> web_ui_setup->SetWebUIHandle(GetRoutingID(), handle.Pass());
> Rather than pushing handles across to the client, should the client not
> Connect() to services it needs by itself? E.g.

Both ways work. In this case, we've optimized for keeping the browser WebUI
setup code simpler. It passes a capability to the web page. We could certainly
re-cast it to work as you suggest. I think we probably need to get a bit more
implementation experience with Mojo before the right patterns fully emerge.

[sky, 2014-04-22 18:15:03]

LGTM

[darin (slow to review), 2014-04-22 21:02:19]

Hi Tom,

Could you please review this from a security point of view. The IPC message
change is really just a renaming. The browser process still lazily sets up Mojo,
only in response to a request to setup WebUI.

[Tom Sepez, 2014-04-22 21:26:18]

I think this is OK in that the main risk is having an "inert" mojo::ShellClient
in every renderer, which doesn't get contacted. I'm now going to take a quick
look a ShellClient, which I've not poked at so far.

https://codereview.chromium.org/236813002/diff/250001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/236813002/diff/250001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:2043: #if 0
nit: Add comment about "un-commenting this will allow mojo calls from all
renderers."

https://codereview.chromium.org/236813002/diff/250001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:2134: if
(!mojo_application_host_->did_activate())
nit: Seems like a shame to activate this before we know whether the service name
is valid or not.

https://codereview.chromium.org/236813002/diff/250001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:2134: if
(!mojo_application_host_->did_activate())
nit: seems like a shame to activate this before we know if the service_name is
valid.

https://codereview.chromium.org/236813002/diff/250001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/236813002/diff/250001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.cc:702: 
I imagined seeing a call to Activate() here?

https://codereview.chromium.org/236813002/diff/250001/content/child/child_thr...
File content/child/child_thread.cc (right):

https://codereview.chromium.org/236813002/diff/250001/content/child/child_thr...
content/child/child_thread.cc:422: if
(mojo_application_->OnMessageReceived(msg))
nit: maybe this goes last since it is so rarely called.

[Tom Sepez, 2014-04-22 21:32:29]

This isn't yet pulling in any of the dynamic library loader stuff into the
browser, right?

[darin (slow to review), 2014-04-22 22:50:22]

On 2014/04/22 21:32:29, Tom Sepez wrote:
> This isn't yet pulling in any of the dynamic library loader stuff into the
> browser, right?

Right

[darin (slow to review), 2014-04-22 22:56:39]

On 2014/04/22 21:26:18, Tom Sepez wrote:
> I think this is OK in that the main risk is having an "inert"
mojo::ShellClient
> in every renderer, which doesn't get contacted. I'm now going to take a quick
> look a ShellClient, which I've not poked at so far.

Right. It should be harmless.


>
https://codereview.chromium.org/236813002/diff/250001/content/browser/rendere...
> File content/browser/renderer_host/render_process_host_impl.cc (right):
> 
>
https://codereview.chromium.org/236813002/diff/250001/content/browser/rendere...
> content/browser/renderer_host/render_process_host_impl.cc:2043: #if 0
> nit: Add comment about "un-commenting this will allow mojo calls from all
> renderers."

Good idea.

 
>
https://codereview.chromium.org/236813002/diff/250001/content/browser/rendere...
> content/browser/renderer_host/render_process_host_impl.cc:2134: if
> (!mojo_application_host_->did_activate())
> nit: Seems like a shame to activate this before we know whether the service
name
> is valid or not.

Eventually we will call Activate() much earlier and eliminate the
did_activate() check. It doesn't really depend on whether the
service is available or not.


>
https://codereview.chromium.org/236813002/diff/250001/content/browser/rendere...
> File content/browser/renderer_host/render_view_host_impl.cc (right):
> 
>
https://codereview.chromium.org/236813002/diff/250001/content/browser/rendere...
> content/browser/renderer_host/render_view_host_impl.cc:702: 
> I imagined seeing a call to Activate() here?

It is inside the call to RenderProcessHostImpl::ConnectTo.


>
https://codereview.chromium.org/236813002/diff/250001/content/child/child_thr...
> File content/child/child_thread.cc (right):
> 
>
https://codereview.chromium.org/236813002/diff/250001/content/child/child_thr...
> content/child/child_thread.cc:422: if
> (mojo_application_->OnMessageReceived(msg))
> nit: maybe this goes last since it is so rarely called.

Hmm, yeah. I don't think it matters much. My goal was to let
Mojo see things before anything else. Eventually the rest of
this function will get deleted in favor of Mojo messages.

Thanks for the review!

[Tom Sepez, 2014-04-23 01:30:06]

On 2014/04/22 22:56:39, darin wrote:
> On 2014/04/22 21:26:18, Tom Sepez wrote:
> > I think this is OK in that the main risk is having an "inert"
> mojo::ShellClient
> > in every renderer, which doesn't get contacted. I'm now going to take a
quick
> > look a ShellClient, which I've not poked at so far.
> 
> Right. It should be harmless.
> 
> 
> >
>
https://codereview.chromium.org/236813002/diff/250001/content/browser/rendere...
> > File content/browser/renderer_host/render_process_host_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/236813002/diff/250001/content/browser/rendere...
> > content/browser/renderer_host/render_process_host_impl.cc:2043: #if 0
> > nit: Add comment about "un-commenting this will allow mojo calls from all
> > renderers."
> 
> Good idea.
> 
>  
> >
>
https://codereview.chromium.org/236813002/diff/250001/content/browser/rendere...
> > content/browser/renderer_host/render_process_host_impl.cc:2134: if
> > (!mojo_application_host_->did_activate())
> > nit: Seems like a shame to activate this before we know whether the service
> name
> > is valid or not.
> 
> Eventually we will call Activate() much earlier and eliminate the
> did_activate() check. It doesn't really depend on whether the
> service is available or not.
> 
> 
> >
>
https://codereview.chromium.org/236813002/diff/250001/content/browser/rendere...
> > File content/browser/renderer_host/render_view_host_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/236813002/diff/250001/content/browser/rendere...
> > content/browser/renderer_host/render_view_host_impl.cc:702: 
> > I imagined seeing a call to Activate() here?
> 
> It is inside the call to RenderProcessHostImpl::ConnectTo.
> 
> 
> >
>
https://codereview.chromium.org/236813002/diff/250001/content/child/child_thr...
> > File content/child/child_thread.cc (right):
> > 
> >
>
https://codereview.chromium.org/236813002/diff/250001/content/child/child_thr...
> > content/child/child_thread.cc:422: if
> > (mojo_application_->OnMessageReceived(msg))
> > nit: maybe this goes last since it is so rarely called.
> 
> Hmm, yeah. I don't think it matters much. My goal was to let
> Mojo see things before anything else. Eventually the rest of
> this function will get deleted in favor of Mojo messages.
> 
> Thanks for the review!

Lgtm.

[darin (slow to review), 2014-04-23 18:36:58]

Committed patchset #16 manually as r265693 (presubmit successful).

[brianderson, 2014-04-23 19:40:40]

On 2014/04/23 18:36:58, darin wrote:
> Committed patchset #16 manually as r265693 (presubmit successful).

We suspect this might have caused some iOS build bot runhooks failures. Any
objections to reverting this?

[Vadim Sh., 2014-04-23 20:58:25]

I think this CL also caused errors in interactive_ui_tests on Linux. See
http://build.chromium.org/p/chromium.linux/builders/Linux%20Tests%20%28dbg%29...:

AppListServiceInteractiveTest.ShowAndDismiss (run #1):
[ RUN      ] AppListServiceInteractiveTest.ShowAndDismiss
Xlib:  extension "RANDR" missing on display ":9".
[9488:9488:0423/130618:INFO:audio_manager_pulse.cc(256)] Failed to connect to
the context.  Error: Connection refused
Xlib:  extension "RANDR" missing on display ":9".
[9488:9488:0423/130618:WARNING:password_store_factory.cc(213)] Using basic
(unencrypted) store for password storage. See
http://code.google.com/p/chromium/wiki/LinuxPasswordStorage for more information
about password storage options.
[9488:9515:0423/130620:ERROR:raw_channel_posix.cc(115)] read: Connection reset
by peer
[9488:9515:0423/130620:ERROR:channel.cc(289)] RawChannel fatal error (type 1)
[9488:9515:0423/130620:FATAL:raw_channel_posix.cc(273)] Check failed:
!read_watcher_.get() || pending_read_.
#0 0x7f0589087dd9 base::debug::StackTrace::StackTrace()
#1 0x7f05891016ad logging::LogMessage::~LogMessage()
#2 0x7f05736621bf mojo::system::(anonymous
namespace)::RawChannelPosix::OnFileCanReadWithoutBlocking()
#3 0x7f0589050a2e
base::MessagePumpLibevent::FileDescriptorWatcher::OnFileCanReadWithoutBlocking()
#4 0x7f05890521cf base::MessagePumpLibevent::OnLibeventNotification()
#5 0x7f0589218a5c event_process_active
#6 0x7f0589218d86 event_base_loop
#7 0x7f0589051995 base::MessagePumpLibevent::Run()
#8 0x7f0589121c33 base::MessageLoop::RunHandler()
#9 0x7f058917ae34 base::RunLoop::Run()
#10 0x7f05891213a2 base::MessageLoop::Run()
#11 0x7f05891d5d82 base::Thread::Run()
#12 0x7f057b9d81b9 content::BrowserThreadImpl::IOThreadRun()
#13 0x7f057b9d836a content::BrowserThreadImpl::Run()
#14 0x7f05891d5fd2 base::Thread::ThreadMain()
#15 0x7f05891c3b21 base::(anonymous namespace)::ThreadFunc()
#16 0x7f0578cf7e9a start_thread
#17 0x7f05765e63fd clone

After the revert the bot cycled green.

[darin (slow to review), 2014-04-23 21:37:29]

On 2014/04/23 20:58:25, Vadim Sh. wrote:
> I think this CL also caused errors in interactive_ui_tests on Linux. See
>
http://build.chromium.org/p/chromium.linux/builders/Linux%20Tests%20%28dbg%29...:
> 
> AppListServiceInteractiveTest.ShowAndDismiss (run #1):
> [ RUN      ] AppListServiceInteractiveTest.ShowAndDismiss
> Xlib:  extension "RANDR" missing on display ":9".
> [9488:9488:0423/130618:INFO:audio_manager_pulse.cc(256)] Failed to connect to
> the context.  Error: Connection refused
> Xlib:  extension "RANDR" missing on display ":9".
> [9488:9488:0423/130618:WARNING:password_store_factory.cc(213)] Using basic
> (unencrypted) store for password storage. See
> http://code.google.com/p/chromium/wiki/LinuxPasswordStorage for more
information
> about password storage options.
> [9488:9515:0423/130620:ERROR:raw_channel_posix.cc(115)] read: Connection reset
> by peer
> [9488:9515:0423/130620:ERROR:channel.cc(289)] RawChannel fatal error (type 1)
> [9488:9515:0423/130620:FATAL:raw_channel_posix.cc(273)] Check failed:
> !read_watcher_.get() || pending_read_.
> #0 0x7f0589087dd9 base::debug::StackTrace::StackTrace()
> #1 0x7f05891016ad logging::LogMessage::~LogMessage()
> #2 0x7f05736621bf mojo::system::(anonymous
> namespace)::RawChannelPosix::OnFileCanReadWithoutBlocking()
> #3 0x7f0589050a2e
>
base::MessagePumpLibevent::FileDescriptorWatcher::OnFileCanReadWithoutBlocking()
> #4 0x7f05890521cf base::MessagePumpLibevent::OnLibeventNotification()
> #5 0x7f0589218a5c event_process_active
> #6 0x7f0589218d86 event_base_loop
> #7 0x7f0589051995 base::MessagePumpLibevent::Run()
> #8 0x7f0589121c33 base::MessageLoop::RunHandler()
> #9 0x7f058917ae34 base::RunLoop::Run()
> #10 0x7f05891213a2 base::MessageLoop::Run()
> #11 0x7f05891d5d82 base::Thread::Run()
> #12 0x7f057b9d81b9 content::BrowserThreadImpl::IOThreadRun()
> #13 0x7f057b9d836a content::BrowserThreadImpl::Run()
> #14 0x7f05891d5fd2 base::Thread::ThreadMain()
> #15 0x7f05891c3b21 base::(anonymous namespace)::ThreadFunc()
> #16 0x7f0578cf7e9a start_thread
> #17 0x7f05765e63fd clone
> 
> After the revert the bot cycled green.

We don't believe that failure is specific to this CL. For this
error, we should probably just disable the tests.

However, given the iOS issue, maybe revert is the best answer.
I don't understand the runhooks issue there.