Adds plumbing to pass WebUI mojo::Handle from browser to renderer

Adds plumbing to pass WebUI mojo::Handle from browser to renderer

See the test for details on how it'll all work out in practice.

BUG=none
TEST=none
R=darin@chromium.org, tsepez@chromium.org

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

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

[sky, 2014-03-20 22:27:53]

darin: all of it
tsepez: mojo_messages (it was missing an include) as well as generate direction.

Sorry for the large patch. I could have split up, but I really wanted a test and
the easiest way to do that was to get it all there. Unfortunately the test can't
be enabled yet since we don't have a way to share gen file for tests. *SIGH*

[Tom Sepez, 2014-03-20 22:43:58]

Messages themselves are LGTM.  The question I had is once the browser gets a
Mojo message, how does it find the corresponding RVH?

[sky, 2014-03-20 22:56:49]

Currently there are no messages from the renderer to the browser
(RenderProcessHostMojo has no functions). The only new message this
has is one from the browser to the renderer, and it uses the existing
routing id to look up the corresponding RV (see RenderProcessMojo in
content/common/mojo/render_process.mojom).

  -Scott

On Thu, Mar 20, 2014 at 3:43 PM,  <tsepez@chromium.org> wrote:
> Messages themselves are LGTM.  The question I had is once the browser gets a
> Mojo message, how does it find the corresponding RVH?
>
> https://codereview.chromium.org/206923002/

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

[darin (slow to review), 2014-03-21 00:04:51]

first pass...

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

https://codereview.chromium.org/206923002/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.h:246: void
SetWebUIHandle(int32 view_routing_id, mojo::ScopedHandle handle);
nit: should this use ScopedMessagePipeHandle instead?

https://codereview.chromium.org/206923002/diff/40001/content/browser/renderer...
File content/browser/renderer_host/render_process_host_mojo_impl.cc (right):

https://codereview.chromium.org/206923002/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_process_host_mojo_impl.cc:65:
SetWebUIHandle(handle->view_routing_id, handle->handle.Pass());
nit: it might be nice to assert that pending_handle_ is indeed null after
calling SetWebUIHandle here.

https://codereview.chromium.org/206923002/diff/40001/content/browser/renderer...
File content/browser/renderer_host/render_process_host_mojo_impl.h (right):

https://codereview.chromium.org/206923002/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_process_host_mojo_impl.h:18: //
RenderProcessHostMojoImpl implements RenderProcessHostMojo. Additionally
nit: this first sentence can be deleted as it is pretty clear from the code.

https://codereview.chromium.org/206923002/diff/40001/content/browser/renderer...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/206923002/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_view_host_impl.cc:739: void
RenderViewHostImpl::SetBrowserWebUIHandle(mojo::ScopedHandle handle) {
why is this method named SetBrowserWebUIHandle whereas all of the others are
just named SetWebUIHandle?

https://codereview.chromium.org/206923002/diff/40001/content/browser/webui/we...
File content/browser/webui/web_ui_mojo_browsertest.cc (right):

https://codereview.chromium.org/206923002/diff/40001/content/browser/webui/we...
content/browser/webui/web_ui_mojo_browsertest.cc:1: // Copyright (c) 2014 The
Chromium Authors. All rights reserved.
nit: no "(c)"

https://codereview.chromium.org/206923002/diff/40001/content/browser/webui/we...
content/browser/webui/web_ui_mojo_browsertest.cc:41: // TODO: this is a hack.
TODO(sky) ? :)

https://codereview.chromium.org/206923002/diff/40001/content/common/mojo/mojo...
File content/common/mojo/mojo_channel_init.cc (right):

https://codereview.chromium.org/206923002/diff/40001/content/common/mojo/mojo...
content/common/mojo/mojo_channel_init.cc:42: static
base::LazyInstance<Initializer>::Leaky initializer =
I don't know if it really matters, but I think one reason for putting this
static at file scope instead of function scope is to avoid the need for the
compiler to lazily construct this class. the compiler will insert another static
boolean that it uses to record whether or not it needs to initialize this
static. (it might be worth looking at generated asm to verify this.)

https://codereview.chromium.org/206923002/diff/40001/content/common/mojo/rend...
File content/common/mojo/render_process.mojom (right):

https://codereview.chromium.org/206923002/diff/40001/content/common/mojo/rend...
content/common/mojo/render_process.mojom:13: SetBrowserWebUIHandle(int32
view_routing_id, handle browser_web_ui_handle);
nit: maybe |handle| -> |handle<message_pipe>| ?

hmm, it is not so obvious to me why the routing ID is needed here.

https://codereview.chromium.org/206923002/diff/40001/content/renderer/mojo/mo...
File content/renderer/mojo/mojo_render_process_observer.cc (right):

https://codereview.chromium.org/206923002/diff/40001/content/renderer/mojo/mo...
content/renderer/mojo/mojo_render_process_observer.cc:52: return;
should there be an assertion here if the handle is not valid?

https://codereview.chromium.org/206923002/diff/40001/content/renderer/mojo/mo...
content/renderer/mojo/mojo_render_process_observer.cc:60: void
MojoRenderProcessObserver::SetBrowserWebUIHandle(
I see why the view ID is needed now. Hmm, OK. Long term, we'll probably have a
mojo MessagePipe per RenderView (or per RenderFrame), and in that world, we'd be
passing this Handle over that message pipe.

https://codereview.chromium.org/206923002/diff/40001/content/test/data/web_ui...
File content/test/data/web_ui_mojo.js (right):

https://codereview.chromium.org/206923002/diff/40001/content/test/data/web_ui...
content/test/data/web_ui_mojo.js:17: RendererTargetTest.prototype.test =
function() {
it looks like these few lines are repeated below.

https://codereview.chromium.org/206923002/diff/40001/mojo/public/bindings/js/...
File mojo/public/bindings/js/defaults.h (right):

https://codereview.chromium.org/206923002/diff/40001/mojo/public/bindings/js/...
mojo/public/bindings/js/defaults.h:5: #ifndef
MOJO_PUBLIC_BINDINGS_JS_DEFAULTS_H_
nit: not sure I get the name "defaults.h" here. maybe "constants.h" would be
better?

[sky, 2014-03-21 03:19:15]

Uploaded new patch.

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

https://codereview.chromium.org/206923002/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.h:246: void
SetWebUIHandle(int32 view_routing_id, mojo::ScopedHandle handle);
On 2014/03/21 00:04:52, darin wrote:
> nit: should this use ScopedMessagePipeHandle instead?

Done.

https://codereview.chromium.org/206923002/diff/40001/content/browser/renderer...
File content/browser/renderer_host/render_process_host_mojo_impl.cc (right):

https://codereview.chromium.org/206923002/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_process_host_mojo_impl.cc:65:
SetWebUIHandle(handle->view_routing_id, handle->handle.Pass());
On 2014/03/21 00:04:52, darin wrote:
> nit: it might be nice to assert that pending_handle_ is indeed null after
> calling SetWebUIHandle here.

It's possible that it isn't. Say if channel initialization fails.

https://codereview.chromium.org/206923002/diff/40001/content/browser/renderer...
File content/browser/renderer_host/render_process_host_mojo_impl.h (right):

https://codereview.chromium.org/206923002/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_process_host_mojo_impl.h:18: //
RenderProcessHostMojoImpl implements RenderProcessHostMojo. Additionally
On 2014/03/21 00:04:52, darin wrote:
> nit: this first sentence can be deleted as it is pretty clear from the code.

Done.

https://codereview.chromium.org/206923002/diff/40001/content/browser/renderer...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/206923002/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_view_host_impl.cc:739: void
RenderViewHostImpl::SetBrowserWebUIHandle(mojo::ScopedHandle handle) {
On 2014/03/21 00:04:52, darin wrote:
> why is this method named SetBrowserWebUIHandle whereas all of the others are
> just named SetWebUIHandle?

I failed at keeping this consistent. I went with SetWebUIHandle every where.

https://codereview.chromium.org/206923002/diff/40001/content/browser/webui/we...
File content/browser/webui/web_ui_mojo_browsertest.cc (right):

https://codereview.chromium.org/206923002/diff/40001/content/browser/webui/we...
content/browser/webui/web_ui_mojo_browsertest.cc:1: // Copyright (c) 2014 The
Chromium Authors. All rights reserved.
On 2014/03/21 00:04:52, darin wrote:
> nit: no "(c)"

Done.

https://codereview.chromium.org/206923002/diff/40001/content/browser/webui/we...
content/browser/webui/web_ui_mojo_browsertest.cc:41: // TODO: this is a hack.
On 2014/03/21 00:04:52, darin wrote:
> TODO(sky) ? :)

I ended up removing the comment. This seems like the only way to accomplish what
I need.

https://codereview.chromium.org/206923002/diff/40001/content/common/mojo/mojo...
File content/common/mojo/mojo_channel_init.cc (right):

https://codereview.chromium.org/206923002/diff/40001/content/common/mojo/mojo...
content/common/mojo/mojo_channel_init.cc:42: static
base::LazyInstance<Initializer>::Leaky initializer =
On 2014/03/21 00:04:52, darin wrote:
> I don't know if it really matters, but I think one reason for putting this
> static at file scope instead of function scope is to avoid the need for the
> compiler to lazily construct this class. the compiler will insert another
static
> boolean that it uses to record whether or not it needs to initialize this
> static. (it might be worth looking at generated asm to verify this.)

This is all temporary anyway as Dave is going to move initialization out of this
class and do it explicitly during startup (he tried but had to revert his
patch).

https://codereview.chromium.org/206923002/diff/40001/content/common/mojo/rend...
File content/common/mojo/render_process.mojom (right):

https://codereview.chromium.org/206923002/diff/40001/content/common/mojo/rend...
content/common/mojo/render_process.mojom:13: SetBrowserWebUIHandle(int32
view_routing_id, handle browser_web_ui_handle);
On 2014/03/21 00:04:52, darin wrote:
> nit: maybe |handle| -> |handle<message_pipe>| ?
> 
> hmm, it is not so obvious to me why the routing ID is needed here.

Done.

https://codereview.chromium.org/206923002/diff/40001/content/renderer/mojo/mo...
File content/renderer/mojo/mojo_render_process_observer.cc (right):

https://codereview.chromium.org/206923002/diff/40001/content/renderer/mojo/mo...
content/renderer/mojo/mojo_render_process_observer.cc:52: return;
On 2014/03/21 00:04:52, darin wrote:
> should there be an assertion here if the handle is not valid?

I think you're suggesting to make this a DCHECK, right? It's possible
initialization could fail though for valid reasons, right? In which case we
don't want to crash.

https://codereview.chromium.org/206923002/diff/40001/content/renderer/mojo/mo...
content/renderer/mojo/mojo_render_process_observer.cc:60: void
MojoRenderProcessObserver::SetBrowserWebUIHandle(
On 2014/03/21 00:04:52, darin wrote:
> I see why the view ID is needed now. Hmm, OK. Long term, we'll probably have a
> mojo MessagePipe per RenderView (or per RenderFrame), and in that world, we'd
be
> passing this Handle over that message pipe.

I agree. Long term it seems like we'll want an interface per RenderView, but I
was hesitant to plumb all of that right now.

https://codereview.chromium.org/206923002/diff/40001/content/test/data/web_ui...
File content/test/data/web_ui_mojo.js (right):

https://codereview.chromium.org/206923002/diff/40001/content/test/data/web_ui...
content/test/data/web_ui_mojo.js:17: RendererTargetTest.prototype.test =
function() {
On 2014/03/21 00:04:52, darin wrote:
> it looks like these few lines are repeated below.

Good catch. The case is slightly different and this one (lowercase) is the one
that can be nuked.

https://codereview.chromium.org/206923002/diff/40001/mojo/public/bindings/js/...
File mojo/public/bindings/js/defaults.h (right):

https://codereview.chromium.org/206923002/diff/40001/mojo/public/bindings/js/...
mojo/public/bindings/js/defaults.h:5: #ifndef
MOJO_PUBLIC_BINDINGS_JS_DEFAULTS_H_
On 2014/03/21 00:04:52, darin wrote:
> nit: not sure I get the name "defaults.h" here. maybe "constants.h" would be
> better?

Done.

[darin (slow to review), 2014-03-21 21:49:21]

On 2014/03/21 03:19:15, sky wrote:
...
>
https://codereview.chromium.org/206923002/diff/40001/content/browser/renderer...
> File content/browser/renderer_host/render_process_host_mojo_impl.cc (right):
> 
>
https://codereview.chromium.org/206923002/diff/40001/content/browser/renderer...
> content/browser/renderer_host/render_process_host_mojo_impl.cc:65:
> SetWebUIHandle(handle->view_routing_id, handle->handle.Pass());
> On 2014/03/21 00:04:52, darin wrote:
> > nit: it might be nice to assert that pending_handle_ is indeed null after
> > calling SetWebUIHandle here.
> 
> It's possible that it isn't. Say if channel initialization fails.

OK


>
https://codereview.chromium.org/206923002/diff/40001/content/browser/renderer...
> File content/browser/renderer_host/render_view_host_impl.cc (right):
> 
>
https://codereview.chromium.org/206923002/diff/40001/content/browser/renderer...
> content/browser/renderer_host/render_view_host_impl.cc:739: void
> RenderViewHostImpl::SetBrowserWebUIHandle(mojo::ScopedHandle handle) {
> On 2014/03/21 00:04:52, darin wrote:
> > why is this method named SetBrowserWebUIHandle whereas all of the others are
> > just named SetWebUIHandle?
> 
> I failed at keeping this consistent. I went with SetWebUIHandle every where.

OK


>
https://codereview.chromium.org/206923002/diff/40001/content/browser/webui/we...
> content/browser/webui/web_ui_mojo_browsertest.cc:41: // TODO: this is a hack.
> On 2014/03/21 00:04:52, darin wrote:
> > TODO(sky) ? :)
> 
> I ended up removing the comment. This seems like the only way to accomplish
what
> I need.

OK


>
https://codereview.chromium.org/206923002/diff/40001/content/common/mojo/mojo...
> File content/common/mojo/mojo_channel_init.cc (right):
> 
>
https://codereview.chromium.org/206923002/diff/40001/content/common/mojo/mojo...
> content/common/mojo/mojo_channel_init.cc:42: static
> base::LazyInstance<Initializer>::Leaky initializer =
> On 2014/03/21 00:04:52, darin wrote:
> > I don't know if it really matters, but I think one reason for putting this
> > static at file scope instead of function scope is to avoid the need for the
> > compiler to lazily construct this class. the compiler will insert another
> static
> > boolean that it uses to record whether or not it needs to initialize this
> > static. (it might be worth looking at generated asm to verify this.)
> 
> This is all temporary anyway as Dave is going to move initialization out of
this
> class and do it explicitly during startup (he tried but had to revert his
> patch).

OK


>
https://codereview.chromium.org/206923002/diff/40001/content/renderer/mojo/mo...
> File content/renderer/mojo/mojo_render_process_observer.cc (right):
> 
>
https://codereview.chromium.org/206923002/diff/40001/content/renderer/mojo/mo...
> content/renderer/mojo/mojo_render_process_observer.cc:52: return;
> On 2014/03/21 00:04:52, darin wrote:
> > should there be an assertion here if the handle is not valid?
> 
> I think you're suggesting to make this a DCHECK, right? It's possible
> initialization could fail though for valid reasons, right? In which case we
> don't want to crash.

OK


>
https://codereview.chromium.org/206923002/diff/40001/content/renderer/mojo/mo...
> content/renderer/mojo/mojo_render_process_observer.cc:60: void
> MojoRenderProcessObserver::SetBrowserWebUIHandle(
> On 2014/03/21 00:04:52, darin wrote:
> > I see why the view ID is needed now. Hmm, OK. Long term, we'll probably have
a
> > mojo MessagePipe per RenderView (or per RenderFrame), and in that world,
we'd
> be
> > passing this Handle over that message pipe.
> 
> I agree. Long term it seems like we'll want an interface per RenderView, but I
> was hesitant to plumb all of that right now.

Yeah, makes sense.


>
https://codereview.chromium.org/206923002/diff/40001/content/test/data/web_ui...
> File content/test/data/web_ui_mojo.js (right):
> 
>
https://codereview.chromium.org/206923002/diff/40001/content/test/data/web_ui...
> content/test/data/web_ui_mojo.js:17: RendererTargetTest.prototype.test =
> function() {
> On 2014/03/21 00:04:52, darin wrote:
> > it looks like these few lines are repeated below.
> 
> Good catch. The case is slightly different and this one (lowercase) is the one
> that can be nuked.

OK


LGTM

[sky, 2014-03-21 21:49:58]

The CQ bit was checked by sky@chromium.org

[commit-bot: I haz the power, 2014-03-21 21:50:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sky@chromium.org/206923002/60001

[commit-bot: I haz the power, 2014-03-21 22:29:55]

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

[commit-bot: I haz the power, 2014-03-21 22:29:56]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_clang_dbg

[sky, 2014-03-21 22:56:06]

The CQ bit was checked by sky@chromium.org

[commit-bot: I haz the power, 2014-03-21 22:56:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sky@chromium.org/206923002/80001

[commit-bot: I haz the power, 2014-03-22 03:25:27]

Change committed as 258763

[sky, 2014-03-25 22:16:19]

As Darin/Mark/myself figured out the reason for the sometimes failure is that
gyp may incorrectly cache the result of a command. All the gypi for the mojom
files includes:

  '<!(python <(DEPTH)/build/inverse_depth.py <(DEPTH))',

this means that if the result ends up being cached we'll generate the bindings
to the wrong directory.

For the time being I'm explicitly setting mojom_base_output_dir in content. When
Mark fixes the gyp side this won't be necessary.

I'm also including two additional changes here:
1. The js was not right for creating the bindings. I needed to specify the
BrowserTarget, not the renderer target.
2. I removed DISABLED from the test. The test now runs if the generated file
exists. If the generated file doesn't exist I assume running on an isolate and
pass. This can be removed once support is added for copying gen files to
isolates.

Diff between 6 and latest to see real changes.
I'm going to reland shortly.

[sky, 2014-03-26 16:45:24]

Committed patchset #15 manually as r259605 (presubmit successful).