Fix WebView compositor input handling

Fix WebView compositor input handling

Recent changes to compositor input handling deprecated existing hooks for
Android WebView. This patch addresses these changes, allowing the browser
to perform synchronous, in-process input event handling when the UI and
compositor threads are merged. In particular, an InputHandlerManagerClient
allows the embedder to customize delivery of and response to InputEvents.

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

[jdduke (slow), 2013-05-23 19:08:29]

OK, this version compiles for WebView and Android, and Android runs like a
charm.  I have yet to run unit tests or anything, but I'd love some early
feedback.  Thanks.

In short, InputHandlerManager is no longer coupled to InputEventFilter.  Rather,
InputHandlerManager specifies a client interface, allowing the embedder to
customize respones to 1) The Addition/Removal of InputHandlers, and 2) The
result of InputEvent handling.  

WebView implements a client that allows it to forward InputEvents (in the
browser) to the InputHandlerManager, and return the response back to
ContentViewCore.   The default renderer implementation is still
InputEventFilter, which has  largely been left untouched exception some function
renaming and ownership relocation.  

Some of the code locations at the boundary (between ContentViewCore,
InProcessInputEventFilter and InProcessViewRenderer) are pretty liquid, and I'm
happy to shift things around as necessary.  The current blocking issue is that
InputEventAckState is not public, but is included by files in android_webview/.

[jdduke (slow), 2013-05-23 19:11:43]

> InputEventFilter, which has  largely been left untouched exception some
function
> renaming and ownership relocation.  

As we don't allow exceptions, that should be "excepting".

[jdduke (slow), 2013-05-23 19:19:32]

Also, I would love it if we could avoid making InputHandlerManagerClient public
at all. 

It's not clear to me how that might happen if it's to be a product of
ContentRendererClient, but if we used an approach similar to the creation of
SynchronousCompositorOutputSurface (behind
command_line.HasSwitch(switches::kEnableSynchronousRendererCompositor)) in
render_widget.cc, that might also work.

[jamesr, 2013-05-24 00:31:29]

I don't like putting the IHMClient interface in content/common/.  In a
multi-process embedding, this class *only* makes sense in the render process. 
In a single-process embedding, it doesn't matter where the header is.

Otherwise, squinting, this looks fairly reasonable although parts of the patch
are hard to read in this version since some things seem merged incorrectly.

https://codereview.chromium.org/15920002/diff/10001/content/renderer/gpu/inpu...
File content/renderer/gpu/input_event_filter.cc (right):

https://codereview.chromium.org/15920002/diff/10001/content/renderer/gpu/inpu...
content/renderer/gpu/input_event_filter.cc:14: #if defined(OS_ANDROID)
looks like you have a bad rebase here

[jdduke (slow), 2013-05-24 00:52:26]

OK, I'll move both IHMClient and SynchronousInputEventFilter out of
content/common and into content/renderer.  

Really all I need is an interface with a single method for consuming an
InputEvent and returning a disposition synchronously.  This will be implemented
by SynchronousInputEventFilter, and it is by that handle that it will be used in
any code within content/browser.  What would be the best place for this?
content/common/android? content/port? InputEventAckState is not public so that's
somewhat limiting...

https://codereview.chromium.org/15920002/diff/10001/content/renderer/gpu/inpu...
File content/renderer/gpu/input_event_filter.cc (right):

https://codereview.chromium.org/15920002/diff/10001/content/renderer/gpu/inpu...
content/renderer/gpu/input_event_filter.cc:14: #if defined(OS_ANDROID)
On 2013/05/24 00:31:29, jamesr wrote:
> looks like you have a bad rebase here

Hmm, I remember having a reason for moving this out of InputHandlerManager, but
it appears that was a bit premature.

[jamesr, 2013-05-24 00:54:10]

https://codereview.chromium.org/15920002/diff/14001/content/public/renderer/c...
File content/public/renderer/content_renderer_client.h (right):

https://codereview.chromium.org/15920002/diff/14001/content/public/renderer/c...
content/public/renderer/content_renderer_client.h:259:
SynchronousInputEventFilter* input_event_filter) {}
it doesn't make sense for a public/ interface to talk about a type that isn't in
the public/ interface. if you have this interface be public/, it'll probably
have something like a HandleInputEvent entry point, right? that'll need to
return something, probably the ACK state.  so i think having the ACK state be
public is OK

[jdduke (slow), 2013-05-24 03:50:18]

https://codereview.chromium.org/15920002/diff/14001/content/public/renderer/c...
File content/public/renderer/content_renderer_client.h (right):

https://codereview.chromium.org/15920002/diff/14001/content/public/renderer/c...
content/public/renderer/content_renderer_client.h:259:
SynchronousInputEventFilter* input_event_filter) {}
On 2013/05/24 00:54:10, jamesr wrote:
> it doesn't make sense for a public/ interface to talk about a type that isn't
in
> the public/ interface. if you have this interface be public/, it'll probably
> have something like a HandleInputEvent entry point, right? that'll need to
> return something, probably the ACK state.  so i think having the ACK state be
> public is OK

Perfect. The non-public ACK state is precisely what this was trying to work
around, in a rather nasty sort of way.  A simple public interface that can use
the ACK will do nicely.

[jdduke (slow), 2013-05-24 17:28:08]

OK, sorry, this patch has become rather unwieldy.   I'm thinking of splitting
the movement of input_event_ack_state.h to content/public into a separate patch?

[jamesr, 2013-05-24 23:46:16]

I think splitting the public API changes out would be a good idea.  I'm not
sure, for example, why this enum was in content/port/ or how that's supposed to
be related to content/public/

https://codereview.chromium.org/15920002/diff/22001/android_webview/browser/i...
File android_webview/browser/in_process_renderer/in_process_view_renderer.cc
(right):

https://codereview.chromium.org/15920002/diff/22001/android_webview/browser/i...
android_webview/browser/in_process_renderer/in_process_view_renderer.cc:190:
base::Unretained(this)));
what prevents this callback from being invoked after |this| has gone away?

[jdduke (slow), 2013-05-25 01:22:11]

OK, moving InputEventAckState visibility change to crrev.com/15720007.  I'll
rebase this patch when that one lands, and run through some testing.  Thanks.

https://codereview.chromium.org/15920002/diff/22001/android_webview/browser/i...
File android_webview/browser/in_process_renderer/in_process_view_renderer.cc
(right):

https://codereview.chromium.org/15920002/diff/22001/android_webview/browser/i...
android_webview/browser/in_process_renderer/in_process_view_renderer.cc:190:
base::Unretained(this)));
On 2013/05/24 23:46:16, jamesr wrote:
> what prevents this callback from being invoked after |this| has gone away?

The assumption (which I should probably specify in the ContentViewCore method
contract) is that a call to ContentViewCore::SetInputEventFilterCallback() with
a null callback will clear any that have been previously supplied.  When |this|
is destroyed, we call SetContents(NULL), which should clear any outstanding
callback on a valid content_view_core_.  Of course, this assumes
content_view_core_ is a valid handle by such time.

[joth, 2013-05-30 01:15:02]

https://codereview.chromium.org/15920002/diff/22001/content/public/renderer/c...
File content/public/renderer/content_renderer_client.h (left):

https://codereview.chromium.org/15920002/diff/22001/content/public/renderer/c...
content/public/renderer/content_renderer_client.h:254: virtual bool
ShouldCreateCompositorInputHandler() const;
note I'm blitzing the three webview-y methods here in
https://codereview.chromium.org/15851006/diff/7001/content/public/renderer/co...

I think this is fine for your next version of this patch: in RenderThreadImpl
you can just have a look at SynchronousCompositorFactory::HasInstance() and if
non-null do the logic to wire it up using direct call path. No
ContentRendererClient hooks needed :-)

[jdduke (slow), 2013-05-31 17:04:59]

OK, PTAL; I think this is ready for closer inspection.

SynchronousCompositorFactory now owns the SynchronousInputEventFilter, which is
little more than a bridge to the InputHandlerManager.  RWHVA can access this via
the SynchronousCompositorImpl.  

Eventually, I'm hoping we can tweak
InputHandlerManager/InputHandlerManagerClient such that 
SynchronousInputEventFilter can go away completely. That would be part of a
separate refactoring.

[mkosiba (inactive), 2013-05-31 17:31:08]

I hit:
F/chromium( 5910): [FATAL:input_handler_manager.cc(27)] Check failed: !client_
|| !client.
when trying this locally.

[jdduke (slow), 2013-05-31 17:40:09]

On 2013/05/31 17:31:08, mkosiba wrote:
> I hit:
> F/chromium( 5910): [FATAL:input_handler_manager.cc(27)] Check failed: !client_
> || !client.
> when trying this locally.

I guess it would help to test this first :) I'll take a look.

[jdduke (slow), 2013-05-31 17:41:35]

On 2013/05/31 17:40:09, jdduke wrote:
> On 2013/05/31 17:31:08, mkosiba wrote:
> > I hit:
> > F/chromium( 5910): [FATAL:input_handler_manager.cc(27)] Check failed:
!client_
> > || !client.
> > when trying this locally.
> 
> I guess it would help to test this first :) I'll take a look.

It would also help to NULL init member pointers... =/

[joth, 2013-05-31 18:05:07]

good... this is much nicer now it can be done fully inside content/ without more
warts on the public API. Bunch of detailed questions below, but the overall
structure works for me.
Thanks!

https://codereview.chromium.org/15920002/diff/37001/content/browser/android/i...
File content/browser/android/in_process/synchronous_compositor_impl.h (right):

https://codereview.chromium.org/15920002/diff/37001/content/browser/android/i...
content/browser/android/in_process/synchronous_compositor_impl.h:51:
InputEventAckState HandleInputEvent(const WebKit::WebInputEvent& input_event);
nit: move to line 33 .

https://codereview.chromium.org/15920002/diff/37001/content/renderer/android/...
File content/renderer/android/synchronous_input_event_filter.cc (right):

https://codereview.chromium.org/15920002/diff/37001/content/renderer/android/...
content/renderer/android/synchronous_input_event_filter.cc:42: handler_ =
handler;
this is called on main thread, but HandleInputEvent() above is on compositor
thread. DO we need a lock around hander_ or is there something to guarantee this
does not race? (I don't think there is, ya know)

also..... devil's advocate, what happens if DidBind is called a second time
(with null handler) exactly while the compositor thread is in the middle of a
call to handler_.Run() above? If we use a lock, we dead lock. Without, we are
calling into an object that is going to become invalid while we're calling it.
(Can we simplify a log but just discount the binding ever going away? Make it
official that the manager and its client must both live forever and never get
unbound, and be done? question for the cc folks)

https://codereview.chromium.org/15920002/diff/37001/content/renderer/android/...
File content/renderer/android/synchronous_input_event_filter.h (right):

https://codereview.chromium.org/15920002/diff/37001/content/renderer/android/...
content/renderer/android/synchronous_input_event_filter.h:27: // It is up to the
user to ensure thread-safety.
user means caller, or sub-classer?

https://codereview.chromium.org/15920002/diff/37001/content/renderer/android/...
content/renderer/android/synchronous_input_event_filter.h:28: class
SynchronousInputEventFilter : public InputHandlerManagerClient {
feels like this whole class could now live in
content/browser/android/in_process? It's basically an implementation detail of
SyncCompositorImpl. The important interface is input_handler_manager_client.h --
we'd need to open up DEPS a bit to let in_process/... see all of content/render,
but that seems fine to me.

WDYT?

https://codereview.chromium.org/15920002/diff/37001/content/renderer/android/...
content/renderer/android/synchronous_input_event_filter.h:49: InputEventAckState
event_result_;
To make this super-clear, you _could_ turn this into a InputEventAckState* and
have it only be non-NULL during the  HandleInputEvent() call (pointing to a
stack var).
I'm fine either way.

https://codereview.chromium.org/15920002/diff/37001/content/renderer/gpu/inpu...
File content/renderer/gpu/input_event_filter.cc (right):

https://codereview.chromium.org/15920002/diff/37001/content/renderer/gpu/inpu...
content/renderer/gpu/input_event_filter.cc:9: #include
"content/common/child_thread.h"
why?

https://codereview.chromium.org/15920002/diff/37001/content/renderer/gpu/inpu...
File content/renderer/gpu/input_handler_manager.cc (right):

https://codereview.chromium.org/15920002/diff/37001/content/renderer/gpu/inpu...
content/renderer/gpu/input_handler_manager.cc:23:
InputHandlerManager::~InputHandlerManager() {
should this unbind Handler() instance from the client? (As the client has to
outline the manager)
i.e. call BindToClient(NULL);

https://codereview.chromium.org/15920002/diff/37001/content/renderer/gpu/inpu...
content/renderer/gpu/input_handler_manager.cc:35: base::Unretained(this)));
nit: intent to next open-paren

https://codereview.chromium.org/15920002/diff/37001/content/renderer/gpu/inpu...
content/renderer/gpu/input_handler_manager.cc:52: base::Unretained(this),
Any idea why the Unretained() is safe here? (I see this pre-dates your change
anyway)

I guess the answer  is 'in practice the InputHandlerManager lasts as long as the
process, so its moot'

https://codereview.chromium.org/15920002/diff/37001/content/renderer/gpu/inpu...
File content/renderer/gpu/input_handler_manager_client.h (right):

https://codereview.chromium.org/15920002/diff/37001/content/renderer/gpu/inpu...
content/renderer/gpu/input_handler_manager_client.h:25: // the next call to
DidBind. The client should only makes calls to |handler|
ah! DidBind() will be called multiple times, and with a null Handler() in the
going-away case.

I wonder is something more direct like SetBoundHanlder() would be clearer in
this case?

whatever, add a comment below that it will be called multiple times

https://codereview.chromium.org/15920002/diff/37001/content/renderer/gpu/inpu...
content/renderer/gpu/input_handler_manager_client.h:32: virtual void
DidAddInputHandler(int routing_id) = 0;
It's be useful to repeat the contract on when and why each of these should be
called (or, link to the class that defines the protocol).

I'd find it handy to have the OnXxxThread called out in the function names, but
that maybe over-verbose for anyone semi-familiar with this code.

[jdduke (slow), 2013-05-31 20:48:24]

https://codereview.chromium.org/15920002/diff/37001/content/browser/android/i...
File content/browser/android/in_process/synchronous_compositor_impl.h (right):

https://codereview.chromium.org/15920002/diff/37001/content/browser/android/i...
content/browser/android/in_process/synchronous_compositor_impl.h:51:
InputEventAckState HandleInputEvent(const WebKit::WebInputEvent& input_event);
On 2013/05/31 18:05:08, joth wrote:
> nit: move to line 33 .

Done.

https://codereview.chromium.org/15920002/diff/37001/content/renderer/android/...
File content/renderer/android/synchronous_input_event_filter.cc (right):

https://codereview.chromium.org/15920002/diff/37001/content/renderer/android/...
content/renderer/android/synchronous_input_event_filter.cc:42: handler_ =
handler;
On 2013/05/31 18:05:08, joth wrote:
> this is called on main thread, but HandleInputEvent() above is on compositor
> thread. DO we need a lock around hander_ or is there something to guarantee
this
> does not race? (I don't think there is, ya know)
> 
> also..... devil's advocate, what happens if DidBind is called a second time
> (with null handler) exactly while the compositor thread is in the middle of a
> call to handler_.Run() above? If we use a lock, we dead lock. Without, we are
> calling into an object that is going to become invalid while we're calling it.
> (Can we simplify a log but just discount the binding ever going away? Make it
> official that the manager and its client must both live forever and never get
> unbound, and be done? question for the cc folks)

Adding a PostTask for assigning the handler.  This handles initialization, but
not shutdown, which should be sufficient?

https://codereview.chromium.org/15920002/diff/37001/content/renderer/android/...
File content/renderer/android/synchronous_input_event_filter.h (right):

https://codereview.chromium.org/15920002/diff/37001/content/renderer/android/...
content/renderer/android/synchronous_input_event_filter.h:27: // It is up to the
user to ensure thread-safety.
On 2013/05/31 18:05:08, joth wrote:
> user means caller, or sub-classer?
> 
Caller. I'll make that clear.

https://codereview.chromium.org/15920002/diff/37001/content/renderer/android/...
content/renderer/android/synchronous_input_event_filter.h:28: class
SynchronousInputEventFilter : public InputHandlerManagerClient {
On 2013/05/31 18:05:08, joth wrote:
> feels like this whole class could now live in
> content/browser/android/in_process? It's basically an implementation detail of
> SyncCompositorImpl. The important interface is input_handler_manager_client.h
--
> we'd need to open up DEPS a bit to let in_process/... see all of
content/render,
> but that seems fine to me.
> 
> WDYT?

Hmm, I put it here because it has a content/renderer dependency
(InputHandlerManagerClient), but it looks like we don't have that restriction
for content/browser/android/in_process members, so I'd be happy for it to live
there. Done.

https://codereview.chromium.org/15920002/diff/37001/content/renderer/android/...
content/renderer/android/synchronous_input_event_filter.h:49: InputEventAckState
event_result_;
On 2013/05/31 18:05:08, joth wrote:
> To make this super-clear, you _could_ turn this into a InputEventAckState* and
> have it only be non-NULL during the  HandleInputEvent() call (pointing to a
> stack var).
> I'm fine either way.

Done.

https://codereview.chromium.org/15920002/diff/37001/content/renderer/gpu/inpu...
File content/renderer/gpu/input_event_filter.cc (right):

https://codereview.chromium.org/15920002/diff/37001/content/renderer/gpu/inpu...
content/renderer/gpu/input_event_filter.cc:9: #include
"content/common/child_thread.h"
On 2013/05/31 18:05:08, joth wrote:
> why?

From a bygone age.  Fixed.

https://codereview.chromium.org/15920002/diff/37001/content/renderer/gpu/inpu...
File content/renderer/gpu/input_handler_manager.cc (right):

https://codereview.chromium.org/15920002/diff/37001/content/renderer/gpu/inpu...
content/renderer/gpu/input_handler_manager.cc:23:
InputHandlerManager::~InputHandlerManager() {
On 2013/05/31 18:05:08, joth wrote:
> should this unbind Handler() instance from the client? (As the client has to
> outline the manager)
> i.e. call BindToClient(NULL);

You're absolutely right, I had that in and must have reverted in all the
shuffle.  Good catch!

https://codereview.chromium.org/15920002/diff/37001/content/renderer/gpu/inpu...
content/renderer/gpu/input_handler_manager.cc:35: base::Unretained(this)));
On 2013/05/31 18:05:08, joth wrote:
> nit: intent to next open-paren

Done.

https://codereview.chromium.org/15920002/diff/37001/content/renderer/gpu/inpu...
content/renderer/gpu/input_handler_manager.cc:52: base::Unretained(this),
On 2013/05/31 18:05:08, joth wrote:
> Any idea why the Unretained() is safe here? (I see this pre-dates your change
> anyway)
> 
> I guess the answer  is 'in practice the InputHandlerManager lasts as long as
the
> process, so its moot'

Right.

https://codereview.chromium.org/15920002/diff/37001/content/renderer/gpu/inpu...
File content/renderer/gpu/input_handler_manager_client.h (right):

https://codereview.chromium.org/15920002/diff/37001/content/renderer/gpu/inpu...
content/renderer/gpu/input_handler_manager_client.h:25: // the next call to
DidBind. The client should only makes calls to |handler|
On 2013/05/31 18:05:08, joth wrote:
> ah! DidBind() will be called multiple times, and with a null Handler() in the
> going-away case.
> 
> I wonder is something more direct like SetBoundHanlder() would be clearer in
> this case?
> 
> whatever, add a comment below that it will be called multiple times

Yeah, I'm open to changing this.  I had it as SetHandler() for a while. 

Eventually I'd like the Did/DidNotHandle callbacks to go away completely, and we
simply get the handled event disposition from the Handler.

https://codereview.chromium.org/15920002/diff/37001/content/renderer/gpu/inpu...
content/renderer/gpu/input_handler_manager_client.h:32: virtual void
DidAddInputHandler(int routing_id) = 0;
On 2013/05/31 18:05:08, joth wrote:
> It's be useful to repeat the contract on when and why each of these should be
> called (or, link to the class that defines the protocol).
> 
> I'd find it handy to have the OnXxxThread called out in the function names,
but
> that maybe over-verbose for anyone semi-familiar with this code.

Yeah.  I really think these two callback methods should disappear. I have a
patch with such a change; for now they simply retain the method names from
before.

[joth, 2013-05-31 20:57:47]

PS3 lgtm

https://codereview.chromium.org/15920002/diff/53001/content/browser/android/i...
File content/browser/android/in_process/DEPS (right):

https://codereview.chromium.org/15920002/diff/53001/content/browser/android/i...
content/browser/android/in_process/DEPS:3: "+content/public/renderer/android",
could you nix /android here too, for consistency?

https://codereview.chromium.org/15920002/diff/53001/content/browser/android/i...
File content/browser/android/in_process/synchronous_input_event_filter.h
(right):

https://codereview.chromium.org/15920002/diff/53001/content/browser/android/i...
content/browser/android/in_process/synchronous_input_event_filter.h:1: //
Copyright (c) 2011 The Chromium Authors. All rights reserved.
2013
(cl upload doesn't warn? shrug)

[joth, 2013-05-31 20:58:10]

On 2013/05/31 20:57:47, joth wrote:
> PS3 lgtm
> 
of course I meant PS 13 

>
https://codereview.chromium.org/15920002/diff/53001/content/browser/android/i...
> File content/browser/android/in_process/DEPS (right):
> 
>
https://codereview.chromium.org/15920002/diff/53001/content/browser/android/i...
> content/browser/android/in_process/DEPS:3: "+content/public/renderer/android",
> could you nix /android here too, for consistency?
> 
>
https://codereview.chromium.org/15920002/diff/53001/content/browser/android/i...
> File content/browser/android/in_process/synchronous_input_event_filter.h
> (right):
> 
>
https://codereview.chromium.org/15920002/diff/53001/content/browser/android/i...
> content/browser/android/in_process/synchronous_input_event_filter.h:1: //
> Copyright (c) 2011 The Chromium Authors. All rights reserved.
> 2013
> (cl upload doesn't warn? shrug)

[jamesr, 2013-06-04 06:51:11]

https://codereview.chromium.org/15920002/diff/53001/content/renderer/gpu/inpu...
File content/renderer/gpu/input_handler_manager_client.h (right):

https://codereview.chromium.org/15920002/diff/53001/content/renderer/gpu/inpu...
content/renderer/gpu/input_handler_manager_client.h:27: typedef
base::Callback<void(int /*routing_id*/,
i think using a return value here instead of Did(Not)HandleInputEvent would make
things a lot clearer

[jdduke (slow), 2013-06-04 15:46:23]

Alright, I think this is in good enough shape for @mkosiba's needs. I'm planning
to run some tests locally and make sure things work as expected.

https://codereview.chromium.org/15920002/diff/53001/content/browser/android/i...
File content/browser/android/in_process/DEPS (right):

https://codereview.chromium.org/15920002/diff/53001/content/browser/android/i...
content/browser/android/in_process/DEPS:3: "+content/public/renderer/android",
On 2013/05/31 20:57:47, joth wrote:
> could you nix /android here too, for consistency?

Done.

https://codereview.chromium.org/15920002/diff/53001/content/browser/android/i...
File content/browser/android/in_process/synchronous_input_event_filter.h
(right):

https://codereview.chromium.org/15920002/diff/53001/content/browser/android/i...
content/browser/android/in_process/synchronous_input_event_filter.h:1: //
Copyright (c) 2011 The Chromium Authors. All rights reserved.
On 2013/05/31 20:57:47, joth wrote:
> 2013
> (cl upload doesn't warn? shrug)

Done.

https://codereview.chromium.org/15920002/diff/53001/content/renderer/gpu/inpu...
File content/renderer/gpu/input_handler_manager_client.h (right):

https://codereview.chromium.org/15920002/diff/53001/content/renderer/gpu/inpu...
content/renderer/gpu/input_handler_manager_client.h:27: typedef
base::Callback<void(int /*routing_id*/,
On 2013/06/04 06:51:11, jamesr wrote:
> i think using a return value here instead of Did(Not)HandleInputEvent would
make
> things a lot clearer

Yeah, just rebased with the new patch that does just that :)  

It largely renders this class redundant, but I believe mkosiba still needs the
"DidAddInputHandler" callbacks for compositor wiring.

[jdduke (slow), 2013-06-04 19:21:14]

@aelias: Could you take a look at
content/browser/renderer_host/render_widget_host_view_android.cc? Thanks.

[joth, 2013-06-04 19:25:05]

rebase onto the other patch lgtm too.

yes - we need some notion of DidAddInputHandler() on the
InputHandlerManagerClient so we have a place to hookup the Root layer scroll
delegate too.

Thanks!

https://codereview.chromium.org/15920002/diff/67036/content/browser/android/i...
File content/browser/android/in_process/synchronous_input_event_filter.h
(right):

https://codereview.chromium.org/15920002/diff/67036/content/browser/android/i...
content/browser/android/in_process/synchronous_input_event_filter.h:9: #include
"base/callback.h"
nit: unneeded include

https://codereview.chromium.org/15920002/diff/67036/content/renderer/android/...
File content/renderer/android/synchronous_compositor_factory.h (right):

https://codereview.chromium.org/15920002/diff/67036/content/renderer/android/...
content/renderer/android/synchronous_compositor_factory.h:38: virtual
InputHandlerManagerClient* GetInputHandlerManagerClient() = 0;
mention that ownership is not returned.

[jdduke (slow), 2013-06-04 19:44:17]

https://codereview.chromium.org/15920002/diff/67036/content/browser/android/i...
File content/browser/android/in_process/synchronous_input_event_filter.h
(right):

https://codereview.chromium.org/15920002/diff/67036/content/browser/android/i...
content/browser/android/in_process/synchronous_input_event_filter.h:9: #include
"base/callback.h"
On 2013/06/04 19:25:05, joth wrote:
> nit: unneeded include

I thought so too, but it's still necessary; input_handler_manager_client.h only
includes callback_forward.h.

https://codereview.chromium.org/15920002/diff/67036/content/renderer/android/...
File content/renderer/android/synchronous_compositor_factory.h (right):

https://codereview.chromium.org/15920002/diff/67036/content/renderer/android/...
content/renderer/android/synchronous_compositor_factory.h:38: virtual
InputHandlerManagerClient* GetInputHandlerManagerClient() = 0;
On 2013/06/04 19:25:05, joth wrote:
> mention that ownership is not returned.

Done.

[joth, 2013-06-04 19:50:07]

https://codereview.chromium.org/15920002/diff/67036/content/browser/android/i...
File content/browser/android/in_process/synchronous_input_event_filter.h
(right):

https://codereview.chromium.org/15920002/diff/67036/content/browser/android/i...
content/browser/android/in_process/synchronous_input_event_filter.h:9: #include
"base/callback.h"
On 2013/06/04 19:44:17, jdduke wrote:
> On 2013/06/04 19:25:05, joth wrote:
> > nit: unneeded include
> 
> I thought so too, but it's still necessary; input_handler_manager_client.h
only
> includes callback_forward.h.

Ah, OK yes.
In that case you can remove the repeated include of it from
synchronous_input_event_filter.cc, but that's of less consequence either way.

[mkosiba (inactive), 2013-06-04 20:55:34]

LGTM. I'm assuming the DidAddInputHandler callback is going to be a separate CL?

[jdduke (slow), 2013-06-04 21:12:38]

On 2013/06/04 20:55:34, mkosiba wrote:
> LGTM. I'm assuming the DidAddInputHandler callback is going to be a separate
CL?

Yeah, I'm not yet sure how/where you want the cc::InputHandler, so I was unsure
how to structure that interface.  It should be pretty easy to extend the 
"InputHandlerManagerClient::DidAddInputHandler" callback to take a weak pointer
to the cc::InputHandler.

[aelias_OOO_until_Jul13, 2013-06-05 03:41:29]

render_widget_host_view_android lgtm

[jamesr, 2013-06-05 03:57:16]

https://codereview.chromium.org/15920002/diff/79001/content/renderer/gpu/inpu...
File content/renderer/gpu/input_event_filter_unittest.cc (right):

https://codereview.chromium.org/15920002/diff/79001/content/renderer/gpu/inpu...
content/renderer/gpu/input_event_filter_unittest.cc:49: const WebInputEvent&
event) {
what was wrong with const * ?

https://codereview.chromium.org/15920002/diff/79001/content/renderer/gpu/inpu...
content/renderer/gpu/input_event_filter_unittest.cc:52:
records_.push_back(Record(&event));
this makes the const& switch look silly

https://codereview.chromium.org/15920002/diff/79001/content/renderer/gpu/inpu...
File content/renderer/gpu/input_handler_manager_client.h (right):

https://codereview.chromium.org/15920002/diff/79001/content/renderer/gpu/inpu...
content/renderer/gpu/input_handler_manager_client.h:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
2013

https://codereview.chromium.org/15920002/diff/79001/content/renderer/gpu/inpu...
content/renderer/gpu/input_handler_manager_client.h:23: // The client should
only makes calls to |handler| on the compositing thread.
it's normally called the compositor thread, not compositing

[joth, 2013-06-05 04:45:57]

On 4 June 2013 20:57, <jamesr@chromium.org> wrote:

>
> https://codereview.chromium.**org/15920002/diff/79001/**
>
content/renderer/gpu/input_**event_filter_unittest.cc<https://codereview.chromium.org/15920002/diff/79001/content/renderer/gpu/input_event_filter_unittest.cc>
> File content/renderer/gpu/input_**event_filter_unittest.cc (right):
>
> https://codereview.chromium.**org/15920002/diff/79001/**
>
content/renderer/gpu/input_**event_filter_unittest.cc#**newcode49<https://codereview.chromium.org/15920002/diff/79001/content/renderer/gpu/input_event_filter_unittest.cc#newcode49>
> content/renderer/gpu/input_**event_filter_unittest.cc:49: const
> WebInputEvent& event) {
> what was wrong with const * ?
>
>
There is a weak pref for const& in the style guide:

"Input parameters are usually values or const references, while output and
input/output parameters will be non-const pointers. "



> https://codereview.chromium.**org/15920002/diff/79001/**
>
content/renderer/gpu/input_**event_filter_unittest.cc#**newcode52<https://codereview.chromium.org/15920002/diff/79001/content/renderer/gpu/input_event_filter_unittest.cc#newcode52>
> content/renderer/gpu/input_**event_filter_unittest.cc:52:
> records_.push_back(Record(&**event));
> this makes the const& switch look silly
>

The change proliferates: make Record c'tor take const& too... ?   It's
internals are sufficiently special that an extra & won't look at all out of
place ;)



>
> https://codereview.chromium.**org/15920002/diff/79001/**
>
content/renderer/gpu/input_**handler_manager_client.h<https://codereview.chromium.org/15920002/diff/79001/content/renderer/gpu/input_handler_manager_client.h>
> File content/renderer/gpu/input_**handler_manager_client.h (right):
>
> https://codereview.chromium.**org/15920002/diff/79001/**
>
content/renderer/gpu/input_**handler_manager_client.h#**newcode1<https://codereview.chromium.org/15920002/diff/79001/content/renderer/gpu/input_handler_manager_client.h#newcode1>
> content/renderer/gpu/input_**handler_manager_client.h:1: // Copyright (c)
> 2012 The Chromium Authors. All rights reserved.
> 2013
>
> https://codereview.chromium.**org/15920002/diff/79001/**
>
content/renderer/gpu/input_**handler_manager_client.h#**newcode23<https://codereview.chromium.org/15920002/diff/79001/content/renderer/gpu/input_handler_manager_client.h#newcode23>
> content/renderer/gpu/input_**handler_manager_client.h:23: // The client
> should only makes calls to |handler| on the compositing thread.
> it's normally called the compositor thread, not compositing
>
>
https://codereview.chromium.**org/15920002/<https://codereview.chromium.org/1...
>

[jamesr, 2013-06-05 05:38:42]

On 2013/06/05 04:45:57, joth wrote:
> On 4 June 2013 20:57, <mailto:jamesr@chromium.org> wrote:
> 
> >
> > https://codereview.chromium.**org/15920002/diff/79001/**
> >
>
content/renderer/gpu/input_**event_filter_unittest.cc<https://codereview.chromium.org/15920002/diff/79001/content/renderer/gpu/input_event_filter_unittest.cc>
> > File content/renderer/gpu/input_**event_filter_unittest.cc (right):
> >
> > https://codereview.chromium.**org/15920002/diff/79001/**
> >
>
content/renderer/gpu/input_**event_filter_unittest.cc#**newcode49<https://codereview.chromium.org/15920002/diff/79001/content/renderer/gpu/input_event_filter_unittest.cc#newcode49>
> > content/renderer/gpu/input_**event_filter_unittest.cc:49: const
> > WebInputEvent& event) {
> > what was wrong with const * ?
> >
> >
> There is a weak pref for const& in the style guide:

No, there's no stated preference.

> 
> "Input parameters are usually values or const references, while output and
> input/output parameters will be non-const pointers. "

see "usually"  There's nothing wrong with having other types where they make
sense.  In this case, the type's a pointer and I think it makes more sense since
it's actually a pointer into the IPC::Message.

Please change it back.

[mkosiba (inactive), 2013-06-05 16:04:21]

On 2013/06/04 21:12:38, jdduke wrote:
> On 2013/06/04 20:55:34, mkosiba wrote:
> > LGTM. I'm assuming the DidAddInputHandler callback is going to be a separate
> CL?
> 
> Yeah, I'm not yet sure how/where you want the cc::InputHandler, so I was
unsure
> how to structure that interface.  It should be pretty easy to extend the 
> "InputHandlerManagerClient::DidAddInputHandler" callback to take a weak
pointer
> to the cc::InputHandler.


I'd need to access cc::InputHandler somewhere that also has access to
SynchronousCompositorImpl. Ideally SynchronousCompositorImpl would hold on to a
weak pointer to cc::InputHandler, that would make sending the 'Java changed the
scroll offset' notification to CC simpler.

Extending InputHandlerManagerClient::DidAddInputHandler SGTM, I can easily
incorporate
that into my local CL to see if it works ;)

[jdduke (slow), 2013-06-05 18:46:30]

https://codereview.chromium.org/15920002/diff/79001/content/renderer/gpu/inpu...
File content/renderer/gpu/input_event_filter_unittest.cc (right):

https://codereview.chromium.org/15920002/diff/79001/content/renderer/gpu/inpu...
content/renderer/gpu/input_event_filter_unittest.cc:49: const WebInputEvent&
event) {
On 2013/06/05 03:57:17, jamesr wrote:
> what was wrong with const * ?

Oh, nothing really.  I changed the signature for the Handler supplied to
InputHandlerManagerClient (InputEventFilter) to take a const ref. Before,
InputHandlerManager::HandleInputEvent never actually checked the WebInputEvent
pointer before doing a deref, so I thought it might make it a bit more clear
that only non-null objects were acceptable?

I'll revert and change the Handler signature back to properly reflect that.

https://codereview.chromium.org/15920002/diff/79001/content/renderer/gpu/inpu...
content/renderer/gpu/input_event_filter_unittest.cc:52:
records_.push_back(Record(&event));
On 2013/06/05 03:57:17, jamesr wrote:
> this makes the const& switch look silly

Yeah, see above.

https://codereview.chromium.org/15920002/diff/79001/content/renderer/gpu/inpu...
File content/renderer/gpu/input_handler_manager_client.h (right):

https://codereview.chromium.org/15920002/diff/79001/content/renderer/gpu/inpu...
content/renderer/gpu/input_handler_manager_client.h:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
On 2013/06/05 03:57:17, jamesr wrote:
> 2013

Done.

https://codereview.chromium.org/15920002/diff/79001/content/renderer/gpu/inpu...
content/renderer/gpu/input_handler_manager_client.h:23: // The client should
only makes calls to |handler| on the compositing thread.
On 2013/06/05 03:57:17, jamesr wrote:
> it's normally called the compositor thread, not compositing

Done.

[jamesr, 2013-06-05 19:07:36]

lgtm

https://codereview.chromium.org/15920002/diff/95001/content/renderer/gpu/inpu...
File content/renderer/gpu/input_event_filter.cc (right):

https://codereview.chromium.org/15920002/diff/95001/content/renderer/gpu/inpu...
content/renderer/gpu/input_event_filter.cc:11: #include "ipc/ipc_sync_channel.h"
is this #include used? remove if not

[joth, 2013-06-05 19:13:35]

On 2013/06/05 05:38:42, jamesr wrote:
> On 2013/06/05 04:45:57, joth wrote:
> > On 4 June 2013 20:57, <mailto:jamesr@chromium.org> wrote:
> > 
> > >
> > > https://codereview.chromium.**org/15920002/diff/79001/**
> > >
> >
>
content/renderer/gpu/input_**event_filter_unittest.cc<https://codereview.chromium.org/15920002/diff/79001/content/renderer/gpu/input_event_filter_unittest.cc>
> > > File content/renderer/gpu/input_**event_filter_unittest.cc (right):
> > >
> > > https://codereview.chromium.**org/15920002/diff/79001/**
> > >
> >
>
content/renderer/gpu/input_**event_filter_unittest.cc#**newcode49<https://codereview.chromium.org/15920002/diff/79001/content/renderer/gpu/input_event_filter_unittest.cc#newcode49>
> > > content/renderer/gpu/input_**event_filter_unittest.cc:49: const
> > > WebInputEvent& event) {
> > > what was wrong with const * ?
> > >
> > >
> > There is a weak pref for const& in the style guide:
> 
> No, there's no stated preference.
> 
> > 
> > "Input parameters are usually values or const references, while output and
> > input/output parameters will be non-const pointers. "
> 
> see "usually"  There's nothing wrong with having other types where they make
> sense.  In this case, the type's a pointer and I think it makes more sense
since
> it's actually a pointer into the IPC::Message.
> 

Oh, right. So does InputHandlerManager::HandleInputEvent() need to know or care
that the  struct might be stored inside an IPC message? 

ISTM the main things it need to know are:  a) that it will never be passed NULL
(else it's going to fault, as it currently stands), and b) that the referenced
WebInputEvent will valid at least for the duration of the call.    const& seems
to convey both these very well.



But, whatever. I don't care enough either way to hold up this patch more over
it.

[jdduke (slow), 2013-06-05 19:30:24]

https://codereview.chromium.org/15920002/diff/95001/content/renderer/gpu/inpu...
File content/renderer/gpu/input_event_filter.cc (right):

https://codereview.chromium.org/15920002/diff/95001/content/renderer/gpu/inpu...
content/renderer/gpu/input_event_filter.cc:11: #include "ipc/ipc_sync_channel.h"
On 2013/06/05 19:07:37, jamesr wrote:
> is this #include used? remove if not

Yeah I don't know why I added that.  Fixed.

[commit-bot: I haz the power, 2013-06-05 20:35:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jdduke@chromium.org/15920002/106001

[commit-bot: I haz the power, 2013-06-05 20:45:49]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[jdduke (slow), 2013-06-05 20:49:43]

@jam: Could you take a look at content/content_browser.gypi and
content/content_renderer.gypi ? Thanks.

[jam, 2013-06-06 16:41:19]

On 2013/06/05 20:49:43, jdduke wrote:
> @jam: Could you take a look at content/content_browser.gypi and
> content/content_renderer.gypi ? Thanks.

lgtm. feel free to TBR simple changes to gypi like this in the future

[commit-bot: I haz the power, 2013-06-06 16:44:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jdduke@chromium.org/15920002/121001

[commit-bot: I haz the power, 2013-06-06 19:10:39]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-06-06 19:50:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jdduke@chromium.org/15920002/143001

[commit-bot: I haz the power, 2013-06-07 08:48:04]

The commit queue went berserk retrying too often for a
seemingly flaky test on builder android_dbg:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[commit-bot: I haz the power, 2013-06-07 15:39:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jdduke@chromium.org/15920002/167001

[commit-bot: I haz the power, 2013-06-07 17:53:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jdduke@chromium.org/15920002/182001

[commit-bot: I haz the power, 2013-06-07 21:24:10]

Change committed as 204933