mustash: Implement basic input event routing in renderer

mustash: Implement basic input event routing in renderer

This CL does the following:

1. It introduces CompositorMusConnection. The connection to
the Mus Window server is now managed on the compositor
thread. RenderWidgetMusConnection is the corresponding main
thread object. CompositorMusConnection deals with
postTasking information RenderWidgetMusConnection wants to
the main thread and passing information from
RenderWidgetMusConnection to the compositor thread.

2. Input events are routed directly to InputHandlerManager
on the compositor thread. Unconsumed input events make
their way to RenderWidgetMusConnection where they're
currently dropped on the floor.

TEST=./out/mandoline_Debug/mojo_runner mojo:example_main --use-mus-in-renderer --use-zero-copy
BUG=551250
Committed: https://crrev.com/2e7bf2f61d4d5082769314c90ad557bdf330791c
Cr-Commit-Position: refs/heads/master@{#363030}

[Fady Samuel, 2015-12-01 22:43:27]

fsamuel@chromium.org changed reviewers:
+ sadrul@chromium.org

[Fady Samuel, 2015-12-01 22:43:27]

This is a bit hacky at the moment. Could you please give me some early feedback?
Thanks! I think we need coalescing and manual event ACK.

[Fady Samuel, 2015-12-01 23:40:45]

Description was changed from

==========
mustash: enable GPU Compositing in renderer.

RenderWidgetMusConnection now lives on the Compositor thread.

Input events are plumbed directly into InputEventManager.

This is a bit broken. Smooth scrolling works but click doesn't.

TEST=./out/mandoline_Debug/mojo_runner mojo:example_main --use-mus-in-renderer
--use-zero-copy
BUG=

patch from issue 1472063007 at patchset 90001
(http://crrev.com/1472063007#ps90001)
==========

to

==========
mustash: Implement basic input event routing in renderer

This CL does the following:

1. It introduces CompositorMusConnection. The connection to the Mus Window
server is now managed on the compositor thread. RenderWidgetMusConnection is the
corresponding main thread object. CompositorMusConnection deals with postTasking
information RnederWidgetMusConnection wants to the main thread and passing
information from RenderWidgetMusConnection to the compositor thread. 

2. Input events are routed directly to InputHandlerManager on the compositor
thread. Unconsumed input events make their way to RenderWidgetMusConnection
where they're currently dropped on the floor.

TEST=./out/mandoline_Debug/mojo_runner mojo:example_main --use-mus-in-renderer
--use-zero-copy
BUG=551250
==========

[Fady Samuel, 2015-12-01 23:41:19]

PTAL Sadrul. I think this is ready for a proper review. I've just removed code
that does routing of input to RenderWidget for now. I'll revisit that over a
series of CLs.

[Fady Samuel, 2015-12-01 23:44:41]

Description was changed from

==========
mustash: Implement basic input event routing in renderer

This CL does the following:

1. It introduces CompositorMusConnection. The connection to the Mus Window
server is now managed on the compositor thread. RenderWidgetMusConnection is the
corresponding main thread object. CompositorMusConnection deals with postTasking
information RnederWidgetMusConnection wants to the main thread and passing
information from RenderWidgetMusConnection to the compositor thread. 

2. Input events are routed directly to InputHandlerManager on the compositor
thread. Unconsumed input events make their way to RenderWidgetMusConnection
where they're currently dropped on the floor.

TEST=./out/mandoline_Debug/mojo_runner mojo:example_main --use-mus-in-renderer
--use-zero-copy
BUG=551250
==========

to

==========
mustash: Implement basic input event routing in renderer

This CL does the following:

1. It introduces CompositorMusConnection. The connection to 
the Mus Window server is now managed on the compositor 
thread. RenderWidgetMusConnection is the corresponding main 
thread object. CompositorMusConnection deals with 
postTasking information RnederWidgetMusConnection wants to 
the main thread and passing information from 
RenderWidgetMusConnection to the compositor thread. 

2. Input events are routed directly to InputHandlerManager 
on the compositor thread. Unconsumed input events make 
their way to RenderWidgetMusConnection where they're 
currently dropped on the floor.

TEST=./out/mandoline_Debug/mojo_runner mojo:example_main --use-mus-in-renderer
--use-zero-copy
BUG=551250
==========

[sadrul, 2015-12-02 18:20:58]

Should there be a content/renderer/mus/ for the mus-specific files?

https://codereview.chromium.org/1484013003/diff/180001/content/renderer/blink...
File content/renderer/blink_input_events_type_converters.cc (right):

https://codereview.chromium.org/1484013003/diff/180001/content/renderer/blink...
content/renderer/blink_input_events_type_converters.cc:208: }
Oh, we should probably return a nullptr here, instead of falling back down to
attempting to create a wheel event.

https://codereview.chromium.org/1484013003/diff/180001/content/renderer/blink...
File content/renderer/blink_input_events_type_converters.h (right):

https://codereview.chromium.org/1484013003/diff/180001/content/renderer/blink...
content/renderer/blink_input_events_type_converters.h:25: #endif  //
CONTENT_RENDERER_BLINK_INPUT_EVENTS_TYPE_CONVERTERS_H_
Maybe this should move to mojo/converters/blink/?

https://codereview.chromium.org/1484013003/diff/180001/content/renderer/compo...
File content/renderer/compositor_mus_connection.cc (right):

https://codereview.chromium.org/1484013003/diff/180001/content/renderer/compo...
content/renderer/compositor_mus_connection.cc:40: this,
base::Passed(surface_binding.Pass())));
Can a WindowSurfaceBinding be safely moved to a different thread?

https://codereview.chromium.org/1484013003/diff/180001/content/renderer/compo...
content/renderer/compositor_mus_connection.cc:84: ack.Run();
This could directly call OnWindowInputEventAckOnMainThread(), or just
compositor_task_runner_->PostTask(), right? (and |ack| would be the callback to
run on compositor-thread)?

https://codereview.chromium.org/1484013003/diff/180001/content/renderer/compo...
content/renderer/compositor_mus_connection.cc:122: base::Closure ack =
base::Bind(&base::DoNothing);
Add a TODO(sad) here to do something more useful once we can do async acks.

https://codereview.chromium.org/1484013003/diff/180001/content/renderer/rende...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/1484013003/diff/180001/content/renderer/rende...
content/renderer/render_view_impl.cc:221: using blink::WebApplicationCacheHost;
The new includes probably aren't necessary?

https://codereview.chromium.org/1484013003/diff/180001/content/renderer/rende...
File content/renderer/render_widget_mus_connection.cc (right):

https://codereview.chromium.org/1484013003/diff/180001/content/renderer/rende...
content/renderer/render_widget_mus_connection.cc:48: mus::mojom::GpuPtr
gpu_service;
Is there a reason to remove the DCHECK()?

https://codereview.chromium.org/1484013003/diff/180001/content/renderer/rende...
content/renderer/render_widget_mus_connection.cc:61: return surface;
Should RenderWidgetMusConnection also check that it's being used in the right
thread, like CompositorMusConnection?

https://codereview.chromium.org/1484013003/diff/180001/content/renderer/rende...
File content/renderer/render_widget_window_tree_client_factory.cc (right):

https://codereview.chromium.org/1484013003/diff/180001/content/renderer/rende...
content/renderer/render_widget_window_tree_client_factory.cc:16: #include
"mojo/application/public/cpp/interface_factory.h"
These includes probably aren't necessary?

[Fady Samuel, 2015-12-02 21:30:28]

PTAL Sadrul! If you're happy with this I'll pass it on to Ben.

I want to do the refactor in a separate patch to avoid confusing the changes
happening here.

https://codereview.chromium.org/1484013003/diff/180001/content/renderer/blink...
File content/renderer/blink_input_events_type_converters.cc (right):

https://codereview.chromium.org/1484013003/diff/180001/content/renderer/blink...
content/renderer/blink_input_events_type_converters.cc:208: }
On 2015/12/02 18:20:57, sadrul wrote:
> Oh, we should probably return a nullptr here, instead of falling back down to
> attempting to create a wheel event.

Done.

https://codereview.chromium.org/1484013003/diff/180001/content/renderer/blink...
File content/renderer/blink_input_events_type_converters.h (right):

https://codereview.chromium.org/1484013003/diff/180001/content/renderer/blink...
content/renderer/blink_input_events_type_converters.h:25: #endif  //
CONTENT_RENDERER_BLINK_INPUT_EVENTS_TYPE_CONVERTERS_H_
On 2015/12/02 18:20:57, sadrul wrote:
> Maybe this should move to mojo/converters/blink/?

Done.

https://codereview.chromium.org/1484013003/diff/180001/content/renderer/compo...
File content/renderer/compositor_mus_connection.cc (right):

https://codereview.chromium.org/1484013003/diff/180001/content/renderer/compo...
content/renderer/compositor_mus_connection.cc:40: this,
base::Passed(surface_binding.Pass())));
On 2015/12/02 18:20:57, sadrul wrote:
> Can a WindowSurfaceBinding be safely moved to a different thread?

Technically yes, but I'll make that more explicit in a separate patch.

https://codereview.chromium.org/1484013003/diff/180001/content/renderer/compo...
content/renderer/compositor_mus_connection.cc:84: ack.Run();
On 2015/12/02 18:20:57, sadrul wrote:
> This could directly call OnWindowInputEventAckOnMainThread(), or just
> compositor_task_runner_->PostTask(), right? (and |ack| would be the callback
to
> run on compositor-thread)?

Well, this ACK was for handling by RenderWidgetMusConnection. The idea is
RenderWidgetMusCOnnection doesn't need to know about what the right thread is to
do this, and just calls it when it's ready and it happens on the right thread.

https://codereview.chromium.org/1484013003/diff/180001/content/renderer/compo...
content/renderer/compositor_mus_connection.cc:122: base::Closure ack =
base::Bind(&base::DoNothing);
On 2015/12/02 18:20:57, sadrul wrote:
> Add a TODO(sad) here to do something more useful once we can do async acks.

Done.

https://codereview.chromium.org/1484013003/diff/180001/content/renderer/rende...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/1484013003/diff/180001/content/renderer/rende...
content/renderer/render_view_impl.cc:221: using blink::WebApplicationCacheHost;
On 2015/12/02 18:20:57, sadrul wrote:
> The new includes probably aren't necessary?

Reverted.

https://codereview.chromium.org/1484013003/diff/180001/content/renderer/rende...
File content/renderer/render_widget_mus_connection.cc (right):

https://codereview.chromium.org/1484013003/diff/180001/content/renderer/rende...
content/renderer/render_widget_mus_connection.cc:48: mus::mojom::GpuPtr
gpu_service;
On 2015/12/02 18:20:57, sadrul wrote:
> Is there a reason to remove the DCHECK()?

No, restored.

https://codereview.chromium.org/1484013003/diff/180001/content/renderer/rende...
content/renderer/render_widget_mus_connection.cc:61: return surface;
On 2015/12/02 18:20:57, sadrul wrote:
> Should RenderWidgetMusConnection also check that it's being used in the right
> thread, like CompositorMusConnection?

Sure. That's a good idea. I added a ThreadChecker that way all relevant
functions are on the same thread.

https://codereview.chromium.org/1484013003/diff/180001/content/renderer/rende...
File content/renderer/render_widget_window_tree_client_factory.cc (right):

https://codereview.chromium.org/1484013003/diff/180001/content/renderer/rende...
content/renderer/render_widget_window_tree_client_factory.cc:16: #include
"mojo/application/public/cpp/interface_factory.h"
On 2015/12/02 18:20:57, sadrul wrote:
> These includes probably aren't necessary?

Removed.

[sadrul, 2015-12-03 00:36:53]

lgtm

https://codereview.chromium.org/1484013003/diff/200001/mojo/converters/blink/...
File mojo/converters/blink/mojo_blink_export.h (right):

https://codereview.chromium.org/1484013003/diff/200001/mojo/converters/blink/...
mojo/converters/blink/mojo_blink_export.h:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
2015

[Fady Samuel, 2015-12-03 00:42:35]

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

[Fady Samuel, 2015-12-03 00:42:35]

+ben@ for OWNER approval.

https://codereview.chromium.org/1484013003/diff/200001/mojo/converters/blink/...
File mojo/converters/blink/mojo_blink_export.h (right):

https://codereview.chromium.org/1484013003/diff/200001/mojo/converters/blink/...
mojo/converters/blink/mojo_blink_export.h:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
On 2015/12/03 00:36:53, sadrul wrote:
> 2015

Done.

[Ben Goodger (Google), 2015-12-03 06:35:53]

https://codereview.chromium.org/1484013003/diff/220001/content/renderer/compo...
File content/renderer/compositor_mus_connection.cc (right):

https://codereview.chromium.org/1484013003/diff/220001/content/renderer/compo...
content/renderer/compositor_mus_connection.cc:31: this,
base::Passed(request.PassMessagePipe())));
is there no typesafe way to pass an interface request to another thread, a la
InterfacePtr::PassInterface?

https://codereview.chromium.org/1484013003/diff/220001/content/renderer/compo...
content/renderer/compositor_mus_connection.cc:129:
&CompositorMusConnection::OnWindowInputEventAckOnMainThread, this, ack);
why does it have to bounce thru main thread like this?

https://codereview.chromium.org/1484013003/diff/220001/content/renderer/rende...
File content/renderer/render_widget_mus_connection.cc (right):

https://codereview.chromium.org/1484013003/diff/220001/content/renderer/rende...
content/renderer/render_widget_mus_connection.cc:104: // 1. The Mus client lib
support manual event ACKing.
supports

https://codereview.chromium.org/1484013003/diff/220001/mojo/converters/blink/...
File mojo/converters/blink/BUILD.gn (right):

https://codereview.chromium.org/1484013003/diff/220001/mojo/converters/blink/...
mojo/converters/blink/BUILD.gn:14: defines = [ "MOJO_BLINK_IMPLEMENTATION" ]
MOJO_CONVERTERS_BLINK_IMPLEMENTATION is a bit more specific

[Fady Samuel, 2015-12-03 16:42:12]

PTAL Ben!

https://codereview.chromium.org/1484013003/diff/220001/content/renderer/compo...
File content/renderer/compositor_mus_connection.cc (right):

https://codereview.chromium.org/1484013003/diff/220001/content/renderer/compo...
content/renderer/compositor_mus_connection.cc:31: this,
base::Passed(request.PassMessagePipe())));
On 2015/12/03 06:35:53, Ben Goodger (Google) wrote:
> is there no typesafe way to pass an interface request to another thread, a la
> InterfacePtr::PassInterface?

Nope, I don't see a way to do this unfortunately. Once that's added, I'm happy
to update this code.

https://codereview.chromium.org/1484013003/diff/220001/content/renderer/compo...
content/renderer/compositor_mus_connection.cc:129:
&CompositorMusConnection::OnWindowInputEventAckOnMainThread, this, ack);
On 2015/12/03 06:35:53, Ben Goodger (Google) wrote:
> why does it have to bounce thru main thread like this?

Once Sadrul completes manual/explicit event ACKing, the way this would work is
the compositor thread gets first crack at the event. If the compositor knows how
to process the event then it ACKs immediately. If it doesn't then we pass the
ACK over to the main thread and ACK there once the main thread does its work. I
plan to pass this closure to render_widget.cc. It will be an opaque closure. For
Chrome w/o Mus, this closure will perform an IPC back to the browser process.
For Chrome in Mus, it will go back to
CompositorMusConnection::OnWindowInputEventAckOnMainThread and then bounce back
to the compositor thread.

We need to do this on the compositor thread because the ACK will ultimately go
to the client lib which lives on the compositor thread now (and the client lib
is not thread-safe).

https://codereview.chromium.org/1484013003/diff/220001/content/renderer/rende...
File content/renderer/render_widget_mus_connection.cc (right):

https://codereview.chromium.org/1484013003/diff/220001/content/renderer/rende...
content/renderer/render_widget_mus_connection.cc:104: // 1. The Mus client lib
support manual event ACKing.
On 2015/12/03 06:35:53, Ben Goodger (Google) wrote:
> supports

Done.

https://codereview.chromium.org/1484013003/diff/220001/mojo/converters/blink/...
File mojo/converters/blink/BUILD.gn (right):

https://codereview.chromium.org/1484013003/diff/220001/mojo/converters/blink/...
mojo/converters/blink/BUILD.gn:14: defines = [ "MOJO_BLINK_IMPLEMENTATION" ]
On 2015/12/03 06:35:53, Ben Goodger (Google) wrote:
> MOJO_CONVERTERS_BLINK_IMPLEMENTATION is a bit more specific

Done.

[Fady Samuel, 2015-12-03 16:42:22]

Description was changed from

==========
mustash: Implement basic input event routing in renderer

This CL does the following:

1. It introduces CompositorMusConnection. The connection to 
the Mus Window server is now managed on the compositor 
thread. RenderWidgetMusConnection is the corresponding main 
thread object. CompositorMusConnection deals with 
postTasking information RnederWidgetMusConnection wants to 
the main thread and passing information from 
RenderWidgetMusConnection to the compositor thread. 

2. Input events are routed directly to InputHandlerManager 
on the compositor thread. Unconsumed input events make 
their way to RenderWidgetMusConnection where they're 
currently dropped on the floor.

TEST=./out/mandoline_Debug/mojo_runner mojo:example_main --use-mus-in-renderer
--use-zero-copy
BUG=551250
==========

to

==========
mustash: Implement basic input event routing in renderer

This CL does the following:

1. It introduces CompositorMusConnection. The connection to 
the Mus Window server is now managed on the compositor 
thread. RenderWidgetMusConnection is the corresponding main 
thread object. CompositorMusConnection deals with 
postTasking information RenderWidgetMusConnection wants to 
the main thread and passing information from 
RenderWidgetMusConnection to the compositor thread. 

2. Input events are routed directly to InputHandlerManager 
on the compositor thread. Unconsumed input events make 
their way to RenderWidgetMusConnection where they're 
currently dropped on the floor.

TEST=./out/mandoline_Debug/mojo_runner mojo:example_main --use-mus-in-renderer
--use-zero-copy
BUG=551250
==========

[Ben Goodger (Google), 2015-12-03 16:55:43]

On 2015/12/03 16:42:12, Fady Samuel wrote:
> We need to do this on the compositor thread because the ACK will ultimately go
> to the client lib which lives on the compositor thread now (and the client lib
> is not thread-safe).
IC. Can you add a TODO in the code on the main thread that just posts it back.
It's confusing as is.

lgtm with that.

[Fady Samuel, 2015-12-03 17:38:56]

On 2015/12/03 16:55:43, Ben Goodger (Google) wrote:
> On 2015/12/03 16:42:12, Fady Samuel wrote:
> > We need to do this on the compositor thread because the ACK will ultimately
go
> > to the client lib which lives on the compositor thread now (and the client
lib
> > is not thread-safe).
> IC. Can you add a TODO in the code on the main thread that just posts it back.
> It's confusing as is.
> 
> lgtm with that.

Done! Thanks! I also fixed the InterfaceRequest passing after chatting with
Yuzhu. I was confusing InterfaceRequest with mojo::Binding which is not
thread-safe. Also moved from .Pass() to std::move as the rest of Chromium is
moving in that direction (no pun intended).

[Fady Samuel, 2015-12-03 17:39:00]

The CQ bit was checked by fsamuel@chromium.org

[Fady Samuel, 2015-12-03 17:39:01]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org,
ben@chromium.org
Link to the patchset: https://codereview.chromium.org/1484013003/#ps260001
(title: "Added comment, use InterfaceRequest to pass across threads and use
std::move instead of .Pass()")

[commit-bot: I haz the power, 2015-12-03 17:40:02]

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

[commit-bot: I haz the power, 2015-12-03 17:50:18]

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

[commit-bot: I haz the power, 2015-12-03 17:50:19]

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

[Fady Samuel, 2015-12-03 18:30:31]

The CQ bit was checked by fsamuel@chromium.org

[Fady Samuel, 2015-12-03 18:30:31]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org,
ben@chromium.org
Link to the patchset: https://codereview.chromium.org/1484013003/#ps280001
(title: "Fixed dependency for unit test")

[commit-bot: I haz the power, 2015-12-03 18:31:16]

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

[commit-bot: I haz the power, 2015-12-03 19:15:53]

Description was changed from

==========
mustash: Implement basic input event routing in renderer

This CL does the following:

1. It introduces CompositorMusConnection. The connection to 
the Mus Window server is now managed on the compositor 
thread. RenderWidgetMusConnection is the corresponding main 
thread object. CompositorMusConnection deals with 
postTasking information RenderWidgetMusConnection wants to 
the main thread and passing information from 
RenderWidgetMusConnection to the compositor thread. 

2. Input events are routed directly to InputHandlerManager 
on the compositor thread. Unconsumed input events make 
their way to RenderWidgetMusConnection where they're 
currently dropped on the floor.

TEST=./out/mandoline_Debug/mojo_runner mojo:example_main --use-mus-in-renderer
--use-zero-copy
BUG=551250
==========

to

==========
mustash: Implement basic input event routing in renderer

This CL does the following:

1. It introduces CompositorMusConnection. The connection to 
the Mus Window server is now managed on the compositor 
thread. RenderWidgetMusConnection is the corresponding main 
thread object. CompositorMusConnection deals with 
postTasking information RenderWidgetMusConnection wants to 
the main thread and passing information from 
RenderWidgetMusConnection to the compositor thread. 

2. Input events are routed directly to InputHandlerManager 
on the compositor thread. Unconsumed input events make 
their way to RenderWidgetMusConnection where they're 
currently dropped on the floor.

TEST=./out/mandoline_Debug/mojo_runner mojo:example_main --use-mus-in-renderer
--use-zero-copy
BUG=551250
==========

[commit-bot: I haz the power, 2015-12-03 19:15:54]

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2015-12-03 19:16:51]

Description was changed from

==========
mustash: Implement basic input event routing in renderer

This CL does the following:

1. It introduces CompositorMusConnection. The connection to 
the Mus Window server is now managed on the compositor 
thread. RenderWidgetMusConnection is the corresponding main 
thread object. CompositorMusConnection deals with 
postTasking information RenderWidgetMusConnection wants to 
the main thread and passing information from 
RenderWidgetMusConnection to the compositor thread. 

2. Input events are routed directly to InputHandlerManager 
on the compositor thread. Unconsumed input events make 
their way to RenderWidgetMusConnection where they're 
currently dropped on the floor.

TEST=./out/mandoline_Debug/mojo_runner mojo:example_main --use-mus-in-renderer
--use-zero-copy
BUG=551250
==========

to

==========
mustash: Implement basic input event routing in renderer

This CL does the following:

1. It introduces CompositorMusConnection. The connection to
the Mus Window server is now managed on the compositor
thread. RenderWidgetMusConnection is the corresponding main
thread object. CompositorMusConnection deals with
postTasking information RenderWidgetMusConnection wants to
the main thread and passing information from
RenderWidgetMusConnection to the compositor thread.

2. Input events are routed directly to InputHandlerManager
on the compositor thread. Unconsumed input events make
their way to RenderWidgetMusConnection where they're
currently dropped on the floor.

TEST=./out/mandoline_Debug/mojo_runner mojo:example_main --use-mus-in-renderer
--use-zero-copy
BUG=551250

Committed: https://crrev.com/2e7bf2f61d4d5082769314c90ad557bdf330791c
Cr-Commit-Position: refs/heads/master@{#363030}
==========

[commit-bot: I haz the power, 2015-12-03 19:16:52]

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