SyntheticGestureTarget implementation for injecting synthetic input events

SyntheticGestureTarget implementation for injecting synthetic input events.

This will be used from SyntheticGesture implementations such as SyntheticSmoothScrollGesture(New) to inject input events in a Android/aura specific way. The patch also includes fallback implementation in SyntheticGetsureTargetBase which passes the input events to RWH.

BUG=297960
BUG=306459
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=234354

[kouhei (in TOK), 2013-10-09 07:02:14]

jdduke, dominikg: PTAL. I tested this working by:
tools/perf/run_measurement smoothness tools/perf/page_sets/top_25.json

[kouhei (in TOK), 2013-10-09 07:03:48]

On 2013/10/09 07:02:14, kouhei wrote:
> jdduke, dominikg: PTAL. I tested this working by:
> tools/perf/run_measurement smoothness tools/perf/page_sets/top_25.json

This includes http://crrev.com/25098004 changes as this CL relies on it. Sorry
for this being a bit hard to read.

[Dominik Grewe, 2013-10-10 14:01:44]

Thanks Kouhei. Just a few nits and comments.

https://codereview.chromium.org/26664002/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/input/touch_scroll_smooth_gesture.h (right):

https://codereview.chromium.org/26664002/diff/1/content/browser/renderer_host...
content/browser/renderer_host/input/touch_scroll_smooth_gesture.h:15: 
Is this just for testing or do you want to keep this? Once the new
implementation is ready this will probably have to change again.

https://codereview.chromium.org/26664002/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/26664002/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_widget_host_view_android.cc:618: // FIXME:
move this to RenderWidgetHostBase
This method will actually be removed completely once the new implementation of
synthetic gestures is complete.

https://codereview.chromium.org/26664002/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/synthetic_touch_event_android.cc (right):

https://codereview.chromium.org/26664002/diff/1/content/browser/renderer_host...
content/browser/renderer_host/synthetic_touch_event_android.cc:60: reset();
If I understand correctly you're trying to make the Java side kind of stateless,
i.e. the state across the entire gesture (savedDownTime_) is only stored on the
C++ side. That's why you reset the Java object each time, right? (and to reset
mPointerCount).

I'm wondering if the overhead of all the JNI calls is worth that. If you didn't
reset mDownTime on the Java side you wouldn't have to set it again each time.
But maybe the overhead is negligible. What do you think?

https://codereview.chromium.org/26664002/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/synthetic_touch_event_android.h (right):

https://codereview.chromium.org/26664002/diff/1/content/browser/renderer_host...
content/browser/renderer_host/synthetic_touch_event_android.h:45: int
savedDownTime_;
Should this be saved_down_time_? Or do we have other style guides when JNI is
involved?

https://codereview.chromium.org/26664002/diff/1/content/content_browser.gypi
File content/content_browser.gypi (right):

https://codereview.chromium.org/26664002/diff/1/content/content_browser.gypi#...
content/content_browser.gypi:837:
'browser/renderer_host/input/touch_scroll_smooth_gesture.h',
nit: Keep alphabetic order. Move below 'touch_event_queue.h'.

https://codereview.chromium.org/26664002/diff/1/content/content_browser.gypi#...
content/content_browser.gypi:1030:
'browser/renderer_host/synthetic_touch_event_android.h',
nit: Same here. Actually all the synthetic_* files should be below
'socket_stream_host.h'.

https://codereview.chromium.org/26664002/diff/1/content/public/android/java/s...
File
content/public/android/java/src/org/chromium/content/browser/SyntheticTouchEvent.java
(right):

https://codereview.chromium.org/26664002/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/SyntheticTouchEvent.java:61:
coords.y = y;
In GenericTouchGesture.java we're also setting coords.pressure to 1.0f. IIRC
that was required in versions <= JB to make sure the touch is properly
registered.

[kouhei (in TOK), 2013-10-15 02:10:01]

Thanks for your comments!

https://codereview.chromium.org/26664002/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/input/touch_scroll_smooth_gesture.h (right):

https://codereview.chromium.org/26664002/diff/1/content/browser/renderer_host...
content/browser/renderer_host/input/touch_scroll_smooth_gesture.h:15: 
On 2013/10/10 14:01:45, Dominik Grewe wrote:
> Is this just for testing or do you want to keep this? Once the new
> implementation is ready this will probably have to change again.

This is for testing, so I will not commit TouchSmoothScrollGesture in this CL.

https://codereview.chromium.org/26664002/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/26664002/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_widget_host_view_android.cc:618: // FIXME:
move this to RenderWidgetHostBase
On 2013/10/10 14:01:45, Dominik Grewe wrote:
> This method will actually be removed completely once the new implementation of
> synthetic gestures is complete.

Ack. I'll remove this in commit CL.

https://codereview.chromium.org/26664002/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/synthetic_touch_event_android.cc (right):

https://codereview.chromium.org/26664002/diff/1/content/browser/renderer_host...
content/browser/renderer_host/synthetic_touch_event_android.cc:60: reset();
On 2013/10/10 14:01:45, Dominik Grewe wrote:
> If I understand correctly you're trying to make the Java side kind of
stateless,
> i.e. the state across the entire gesture (savedDownTime_) is only stored on
the
> C++ side. That's why you reset the Java object each time, right? (and to reset
> mPointerCount).
> 
> I'm wondering if the overhead of all the JNI calls is worth that. If you
didn't
> reset mDownTime on the Java side you wouldn't have to set it again each time.
> But maybe the overhead is negligible. What do you think?

I think the cost of reset() here is negligible, as we are already doing JNI call
N_pointers+2 times. If we got rid of reset(), this will still JNI call
N_pointers+1 times.

https://codereview.chromium.org/26664002/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/synthetic_touch_event_android.h (right):

https://codereview.chromium.org/26664002/diff/1/content/browser/renderer_host...
content/browser/renderer_host/synthetic_touch_event_android.h:45: int
savedDownTime_;
On 2013/10/10 14:01:45, Dominik Grewe wrote:
> Should this be saved_down_time_? Or do we have other style guides when JNI is
> involved?

Done. I just got confused switching back and forth between C++/Java.

https://codereview.chromium.org/26664002/diff/1/content/content_browser.gypi
File content/content_browser.gypi (right):

https://codereview.chromium.org/26664002/diff/1/content/content_browser.gypi#...
content/content_browser.gypi:837:
'browser/renderer_host/input/touch_scroll_smooth_gesture.h',
On 2013/10/10 14:01:45, Dominik Grewe wrote:
> nit: Keep alphabetic order. Move below 'touch_event_queue.h'.

Done.

https://codereview.chromium.org/26664002/diff/1/content/content_browser.gypi#...
content/content_browser.gypi:1030:
'browser/renderer_host/synthetic_touch_event_android.h',
On 2013/10/10 14:01:45, Dominik Grewe wrote:
> nit: Same here. Actually all the synthetic_* files should be below
> 'socket_stream_host.h'.

Done.

https://codereview.chromium.org/26664002/diff/1/content/public/android/java/s...
File
content/public/android/java/src/org/chromium/content/browser/SyntheticTouchEvent.java
(right):

https://codereview.chromium.org/26664002/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/SyntheticTouchEvent.java:61:
coords.y = y;
On 2013/10/10 14:01:45, Dominik Grewe wrote:
> In GenericTouchGesture.java we're also setting coords.pressure to 1.0f. IIRC
> that was required in versions <= JB to make sure the touch is properly
> registered.

Done.

[Dominik Grewe, 2013-10-15 09:10:55]

https://codereview.chromium.org/26664002/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/synthetic_touch_event_android.cc (right):

https://codereview.chromium.org/26664002/diff/1/content/browser/renderer_host...
content/browser/renderer_host/synthetic_touch_event_android.cc:60: reset();
On 2013/10/15 02:10:02, kouhei wrote:
> On 2013/10/10 14:01:45, Dominik Grewe wrote:
> > If I understand correctly you're trying to make the Java side kind of
> stateless,
> > i.e. the state across the entire gesture (savedDownTime_) is only stored on
> the
> > C++ side. That's why you reset the Java object each time, right? (and to
reset
> > mPointerCount).
> > 
> > I'm wondering if the overhead of all the JNI calls is worth that. If you
> didn't
> > reset mDownTime on the Java side you wouldn't have to set it again each
time.
> > But maybe the overhead is negligible. What do you think?
> 
> I think the cost of reset() here is negligible, as we are already doing JNI
call
> N_pointers+2 times. If we got rid of reset(), this will still JNI call
> N_pointers+1 times.

You would also save the call to SetDownTime() on each call. Given that
N_pointers is likely to be small (1 or 2) you could cut about half the JNI calls
on average. But like I said, I don't know if it's worth it overall.

https://codereview.chromium.org/26664002/diff/11001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/26664002/diff/11001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_android.cc:1012: if (!
synthetic_touch_event_)
Does this work with scoped_ptr<> or should it be
if (!synthetic_touch_event_.get()) ?

Also, remove space between '!' and 'synthetic_touch_event_'.

[kouhei (in TOK), 2013-10-16 04:45:37]

This time I tried stateful SyntheticTouchEvent.java. I'm not sure which is
better. I'm ok with both.

https://codereview.chromium.org/26664002/diff/11001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/26664002/diff/11001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_android.cc:1012: if (!
synthetic_touch_event_)
On 2013/10/15 09:10:55, Dominik Grewe wrote:
> Does this work with scoped_ptr<> or should it be
> if (!synthetic_touch_event_.get()) ?

This should be OK.
https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/scoped...

> Also, remove space between '!' and 'synthetic_touch_event_'.
Done.

[Dominik Grewe, 2013-10-16 09:22:10]

I slightly prefer this version, because it seems less complex. But I'm okay with
either.

https://codereview.chromium.org/26664002/diff/21001/content/browser/renderer_...
File content/browser/renderer_host/synthetic_touch_event_android.cc (right):

https://codereview.chromium.org/26664002/diff/21001/content/browser/renderer_...
content/browser/renderer_host/synthetic_touch_event_android.cc:28: CHECK(!
java_touch_event_.is_null());
Nit: Remove whitespace between unary operator and its argument.

https://codereview.chromium.org/26664002/diff/21001/content/browser/renderer_...
content/browser/renderer_host/synthetic_touch_event_android.cc:66: for (unsigned
i = 0; i < num_touches; ++ i) {
Nit: Same here, remove whitespace between '++' and 'i'.

[jdduke (slow), 2013-10-16 15:05:24]

https://codereview.chromium.org/26664002/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/synthetic_touch_event_android.cc (right):

https://codereview.chromium.org/26664002/diff/1/content/browser/renderer_host...
content/browser/renderer_host/synthetic_touch_event_android.cc:60: reset();
On 2013/10/15 09:10:55, Dominik Grewe wrote:
> On 2013/10/15 02:10:02, kouhei wrote:
> > On 2013/10/10 14:01:45, Dominik Grewe wrote:
> > > If I understand correctly you're trying to make the Java side kind of
> > stateless,
> > > i.e. the state across the entire gesture (savedDownTime_) is only stored
on
> > the
> > > C++ side. That's why you reset the Java object each time, right? (and to
> reset
> > > mPointerCount).
> > > 
> > > I'm wondering if the overhead of all the JNI calls is worth that. If you
> > didn't
> > > reset mDownTime on the Java side you wouldn't have to set it again each
> time.
> > > But maybe the overhead is negligible. What do you think?
> > 
> > I think the cost of reset() here is negligible, as we are already doing JNI
> call
> > N_pointers+2 times. If we got rid of reset(), this will still JNI call
> > N_pointers+1 times.
> 
> You would also save the call to SetDownTime() on each call. Given that
> N_pointers is likely to be small (1 or 2) you could cut about half the JNI
calls
> on average. But like I said, I don't know if it's worth it overall.

While JNI calls are "slow", that is only in the relative sense; the overhead is
still in the order of just tens of nanoseconds. So, unless they're being called
thousands of times a frame, we should not be optimizing JNI calls here.  We
should do whatever is simplest.

[kouhei (in TOK), 2013-10-17 05:44:12]

https://codereview.chromium.org/26664002/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/synthetic_touch_event_android.cc (right):

https://codereview.chromium.org/26664002/diff/1/content/browser/renderer_host...
content/browser/renderer_host/synthetic_touch_event_android.cc:60: reset();
On 2013/10/16 15:05:25, jdduke wrote:
> On 2013/10/15 09:10:55, Dominik Grewe wrote:
> > On 2013/10/15 02:10:02, kouhei wrote:
> > > On 2013/10/10 14:01:45, Dominik Grewe wrote:
> > > > If I understand correctly you're trying to make the Java side kind of
> > > stateless,
> > > > i.e. the state across the entire gesture (savedDownTime_) is only stored
> on
> > > the
> > > > C++ side. That's why you reset the Java object each time, right? (and to
> > reset
> > > > mPointerCount).
> > > > 
> > > > I'm wondering if the overhead of all the JNI calls is worth that. If you
> > > didn't
> > > > reset mDownTime on the Java side you wouldn't have to set it again each
> > time.
> > > > But maybe the overhead is negligible. What do you think?
> > > 
> > > I think the cost of reset() here is negligible, as we are already doing
JNI
> > call
> > > N_pointers+2 times. If we got rid of reset(), this will still JNI call
> > > N_pointers+1 times.
> > 
> > You would also save the call to SetDownTime() on each call. Given that
> > N_pointers is likely to be small (1 or 2) you could cut about half the JNI
> calls
> > on average. But like I said, I don't know if it's worth it overall.
> 
> While JNI calls are "slow", that is only in the relative sense; the overhead
is
> still in the order of just tens of nanoseconds. So, unless they're being
called
> thousands of times a frame, we should not be optimizing JNI calls here.  We
> should do whatever is simplest.

Thanks for your comments! I think I can go with the stateful version with less
JNI calls, it wasn't confusing as I first thought.

https://codereview.chromium.org/26664002/diff/21001/content/browser/renderer_...
File content/browser/renderer_host/synthetic_touch_event_android.cc (right):

https://codereview.chromium.org/26664002/diff/21001/content/browser/renderer_...
content/browser/renderer_host/synthetic_touch_event_android.cc:28: CHECK(!
java_touch_event_.is_null());
On 2013/10/16 09:22:10, Dominik Grewe wrote:
> Nit: Remove whitespace between unary operator and its argument.

Done.

https://codereview.chromium.org/26664002/diff/21001/content/browser/renderer_...
content/browser/renderer_host/synthetic_touch_event_android.cc:66: for (unsigned
i = 0; i < num_touches; ++ i) {
On 2013/10/16 09:22:10, Dominik Grewe wrote:
> Nit: Same here, remove whitespace between '++' and 'i'.

Done.

[Dominik Grewe, 2013-10-21 17:26:44]

LGTM. Jared, what do you think?

https://codereview.chromium.org/26664002/diff/27001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/26664002/diff/27001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_android.cc:1022:
NOTREACHED(); // FIXME: implement other types (MouseWheel?)
For Android it should be fine to only have touch support for now I guess.

[jdduke (slow), 2013-10-21 19:30:39]

https://codereview.chromium.org/26664002/diff/27001/content/browser/renderer_...
File content/browser/renderer_host/synthetic_touch_event_android.h (right):

https://codereview.chromium.org/26664002/diff/27001/content/browser/renderer_...
content/browser/renderer_host/synthetic_touch_event_android.h:5: #ifndef
CONTENT_BROWSER_RENDERER_HOST_SYNTHETIC_TOUCH_EVENT_ANDROID_H_
Can we move this to renderer_host/input?

[jdduke (slow), 2013-10-21 23:05:44]

https://codereview.chromium.org/26664002/diff/27001/content/browser/renderer_...
File content/browser/renderer_host/synthetic_touch_event_android.h (right):

https://codereview.chromium.org/26664002/diff/27001/content/browser/renderer_...
content/browser/renderer_host/synthetic_touch_event_android.h:12: 

Nit: Remove lines before/after "class WebTouchEvent", and also the trailing " 
// namespace WebKit".

https://codereview.chromium.org/26664002/diff/27001/content/browser/renderer_...
content/browser/renderer_host/synthetic_touch_event_android.h:21: class
SyntheticTouchEventAndroid {

I wonder if we want to add something else to the name of this class, as it's not
really an event.  What about SyntheticTouchEventHandler(Android)?

https://codereview.chromium.org/26664002/diff/27001/content/browser/renderer_...
content/browser/renderer_host/synthetic_touch_event_android.h:35: void
SetPointer(int index, int x, int y, int id);

Nit: Please make SetPointer/Inject private, and the Action enum (unless it's
needed elsewhere?)

https://codereview.chromium.org/26664002/diff/27001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/SyntheticTouchEvent.java
(right):

https://codereview.chromium.org/26664002/diff/27001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/SyntheticTouchEvent.java:72:
mContentViewCore.onTouchEvent(event);
Maybe add a helper method to call mContentViewCore.onTouchEvent(event) +
event.recycle()? injectAndRecycleEvent(event)?

[jdduke (slow), 2013-10-22 21:47:08]

kouhei@: With RWHVPort::CreateSyntheticGestureTarget(), we could probably just
make SyntheticTouchEventAndroid implement SyntheticGestureTarget directly.

[kouhei (in TOK), 2013-10-25 15:10:17]

This is a wip patch. I plan to finish it early next week. This also includes
draft of Syn*TargetBase.

> kouhei@: With RWHVPort::CreateSyntheticGestureTarget(), we could probably just
> make SyntheticTouchEventAndroid implement SyntheticGestureTarget directly.
Done.

https://codereview.chromium.org/26664002/diff/27001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/26664002/diff/27001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_android.cc:1022:
NOTREACHED(); // FIXME: implement other types (MouseWheel?)
On 2013/10/21 17:26:44, Dominik Grewe wrote:
> For Android it should be fine to only have touch support for now I guess.
Got it.

https://codereview.chromium.org/26664002/diff/27001/content/browser/renderer_...
File content/browser/renderer_host/synthetic_touch_event_android.h (right):

https://codereview.chromium.org/26664002/diff/27001/content/browser/renderer_...
content/browser/renderer_host/synthetic_touch_event_android.h:5: #ifndef
CONTENT_BROWSER_RENDERER_HOST_SYNTHETIC_TOUCH_EVENT_ANDROID_H_
On 2013/10/21 19:30:40, jdduke wrote:
> Can we move this to renderer_host/input?

Done.

https://codereview.chromium.org/26664002/diff/27001/content/browser/renderer_...
content/browser/renderer_host/synthetic_touch_event_android.h:12: 
On 2013/10/21 23:05:45, jdduke wrote:
> 
> Nit: Remove lines before/after "class WebTouchEvent", and also the trailing " 
> // namespace WebKit".

Done.

https://codereview.chromium.org/26664002/diff/27001/content/browser/renderer_...
content/browser/renderer_host/synthetic_touch_event_android.h:35: void
SetPointer(int index, int x, int y, int id);
On 2013/10/21 23:05:45, jdduke wrote:
> 
> Nit: Please make SetPointer/Inject private, and the Action enum (unless it's
> needed elsewhere?)

Done.

[Dominik Grewe, 2013-10-25 16:10:11]

Looking good. Thanks Kouhei!

https://codereview.chromium.org/26664002/diff/93001/content/browser/renderer_...
File content/browser/renderer_host/input/synthetic_gesture_target_base.cc
(right):

https://codereview.chromium.org/26664002/diff/93001/content/browser/renderer_...
content/browser/renderer_host/input/synthetic_gesture_target_base.cc:55: return
SyntheticGestureParams::TOUCH_INPUT;
I'd say we use mouse input as the default on base, as this will mainly be for
desktop afaict.

https://codereview.chromium.org/26664002/diff/93001/content/browser/renderer_...
File content/browser/renderer_host/input/synthetic_gesture_target_base.h
(right):

https://codereview.chromium.org/26664002/diff/93001/content/browser/renderer_...
content/browser/renderer_host/input/synthetic_gesture_target_base.h:33: virtual
void QueueInputEventToPlatform(const InputEvent& event) OVERRIDE;
I'm wondering if it's better to override this method in the sub-classes rather
than rely on this one. If, for example, on Android we don't support mouse
events, do we want to fall back to Base or do we want to fail? I thought the
latter, that's why we have SupportsSyntheticGestureSourceType(), but I guess
both options have their advantages.

Also, in Base there's no need to distinguish between different types of events
if we simply forward them as-is to RWH.

https://codereview.chromium.org/26664002/diff/93001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_base.cc (right):

https://codereview.chromium.org/26664002/diff/93001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_base.cc:533:
NOTIMPLEMENTED();
Here's where you would return SyntheticGestureTargetBase for platforms that
don't have their own implementation.

https://codereview.chromium.org/26664002/diff/93001/content/content_browser.gypi
File content/content_browser.gypi (right):

https://codereview.chromium.org/26664002/diff/93001/content/content_browser.g...
content/content_browser.gypi:833:
'browser/renderer_host/input/synthetic_gesture_target_base.cc',
'.cc' before '.h'

[kouhei (in TOK), 2013-10-28 03:03:16]

Thanks for your comments. Merging this with aura CL.

https://codereview.chromium.org/26664002/diff/93001/content/browser/renderer_...
File content/browser/renderer_host/input/synthetic_gesture_target_base.cc
(right):

https://codereview.chromium.org/26664002/diff/93001/content/browser/renderer_...
content/browser/renderer_host/input/synthetic_gesture_target_base.cc:55: return
SyntheticGestureParams::TOUCH_INPUT;
On 2013/10/25 16:10:11, Dominik Grewe wrote:
> I'd say we use mouse input as the default on base, as this will mainly be for
> desktop afaict.

Done.

https://codereview.chromium.org/26664002/diff/93001/content/browser/renderer_...
File content/browser/renderer_host/input/synthetic_gesture_target_base.h
(right):

https://codereview.chromium.org/26664002/diff/93001/content/browser/renderer_...
content/browser/renderer_host/input/synthetic_gesture_target_base.h:33: virtual
void QueueInputEventToPlatform(const InputEvent& event) OVERRIDE;
On 2013/10/25 16:10:11, Dominik Grewe wrote:
> I'm wondering if it's better to override this method in the sub-classes rather
> than rely on this one. If, for example, on Android we don't support mouse
> events, do we want to fall back to Base or do we want to fail? I thought the
> latter, that's why we have SupportsSyntheticGestureSourceType(), but I guess
> both options have their advantages.
I'm not sure here. I added DCHECKs for unsupported GestureSourceType calls.

> Also, in Base there's no need to distinguish between different types of events
> if we simply forward them as-is to RWH.
This is true in future, but currently we need to use separate forward method per
type. (e.g. Forward{MouseWheel,Touch}EventWithLatencyInfo)

https://codereview.chromium.org/26664002/diff/93001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_base.cc (right):

https://codereview.chromium.org/26664002/diff/93001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_base.cc:533:
NOTIMPLEMENTED();
On 2013/10/25 16:10:11, Dominik Grewe wrote:
> Here's where you would return SyntheticGestureTargetBase for platforms that
> don't have their own implementation.

Done.

https://codereview.chromium.org/26664002/diff/93001/content/content_browser.gypi
File content/content_browser.gypi (right):

https://codereview.chromium.org/26664002/diff/93001/content/content_browser.g...
content/content_browser.gypi:833:
'browser/renderer_host/input/synthetic_gesture_target_base.cc',
On 2013/10/25 16:10:11, Dominik Grewe wrote:
> '.cc' before '.h'

Done.

[Dominik Grewe, 2013-10-28 10:11:59]

Thanks Kouhei. A few comments below, otherwise it's looking pretty good afaict.

https://codereview.chromium.org/26664002/diff/713001/content/browser/renderer...
File content/browser/renderer_host/input/synthetic_gesture_target_base.cc
(right):

https://codereview.chromium.org/26664002/diff/713001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_gesture_target_base.cc:23: const
int kSyntheticGestureMessageIntervalMs = 7;
I think we should change this to 16, so we're roughly sending one event per
frame. I remember having problems on Android with this value (the current
implementations on Android completely bypasses this mechanims).

Also, rename the constant so it's more in line with the method name
GetSyntheticGestureUpdateRate().

https://codereview.chromium.org/26664002/diff/713001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_gesture_target_base.cc:48:
QueueWebMouseWheelEventToPlatform(*web_wheel, event.latency_info);
MOUSE_INPUT isn't necessarily a WebMouseWheelEvent, it could also be a
WebMouseEvent.

https://codereview.chromium.org/26664002/diff/713001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_gesture_target_base.cc:86: return
gesture_source_type == SyntheticGestureParams::MOUSE_INPUT;
Base also supports touch.

https://codereview.chromium.org/26664002/diff/713001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/26664002/diff/713001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_aura.cc:93: using
WebKit::WebInputEvent;
I don't think this is needed anymore.

[jdduke (slow), 2013-10-28 15:59:46]

https://codereview.chromium.org/26664002/diff/93001/content/browser/renderer_...
File content/browser/renderer_host/input/synthetic_gesture_target_android.cc
(right):

https://codereview.chromium.org/26664002/diff/93001/content/browser/renderer_...
content/browser/renderer_host/input/synthetic_gesture_target_android.cc:53:
SyntheticGestureTargetAndroid::Action action =
Rather than storing |env_|, just attach here and hand it to Touch{SetPointer,
Inject}.

https://codereview.chromium.org/26664002/diff/93001/content/browser/renderer_...
File content/browser/renderer_host/input/synthetic_gesture_target_android.h
(right):

https://codereview.chromium.org/26664002/diff/93001/content/browser/renderer_...
content/browser/renderer_host/input/synthetic_gesture_target_android.h:19:
SyntheticGestureTargetAndroid(RenderWidgetHostViewAndroid* render_view);
What if we have as an argument here |java_touch_event|?

https://codereview.chromium.org/26664002/diff/93001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_android.h (right):

https://codereview.chromium.org/26664002/diff/93001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_android.h:248:
ContentViewCoreImpl* content_view_core() { return content_view_core_; }
I think we don't need this if we just give SyntheticGestureTargetAndroid the
"SyntheticTouchEvent" java handle on construction.

https://codereview.chromium.org/26664002/diff/93001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/26664002/diff/93001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:93: using
WebKit::WebInputEvent;
Nit: I don't think you need this using decl now?

https://codereview.chromium.org/26664002/diff/93001/content/port/browser/rend...
File content/port/browser/render_widget_host_view_port.h (right):

https://codereview.chromium.org/26664002/diff/93001/content/port/browser/rend...
content/port/browser/render_widget_host_view_port.h:303: virtual
SyntheticGestureTarget* CreateSyntheticGestureTarget() = 0;
Could we return a scoped_ptr?

https://codereview.chromium.org/26664002/diff/713001/content/browser/renderer...
File content/browser/renderer_host/input/synthetic_gesture_target_aura.h
(right):

https://codereview.chromium.org/26664002/diff/713001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_gesture_target_aura.h:18: class
SyntheticGestureTargetAura : public SyntheticGestureTargetBase {
Is it necessary for SyntheticGestureTarget{Aura,Android} to inherit from Base?
It looks like we override most of the functionality, in which case I think this
would all be simpler if we have SyntheticGestureTarget{Aura,Android,Default},
and they don't share any implementation details.

[Dominik Grewe, 2013-10-28 16:14:30]

https://codereview.chromium.org/26664002/diff/713001/content/browser/renderer...
File content/browser/renderer_host/input/synthetic_gesture_target_aura.h
(right):

https://codereview.chromium.org/26664002/diff/713001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_gesture_target_aura.h:18: class
SyntheticGestureTargetAura : public SyntheticGestureTargetBase {
On 2013/10/28 15:59:47, jdduke wrote:
> Is it necessary for SyntheticGestureTarget{Aura,Android} to inherit from Base?
> It looks like we override most of the functionality, in which case I think
this
> would all be simpler if we have SyntheticGestureTarget{Aura,Android,Default},
> and they don't share any implementation details. 

There is some common functionality: GetSyntheticGestureUpdateRate() and
OnSyntheticGestureCompleted(). But yes, it's limited.

[kouhei (in TOK), 2013-10-29 03:30:41]

Updated patch.

https://codereview.chromium.org/26664002/diff/713001/content/browser/renderer...
File content/browser/renderer_host/input/synthetic_gesture_target_base.cc
(right):

https://codereview.chromium.org/26664002/diff/713001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_gesture_target_base.cc:23: const
int kSyntheticGestureMessageIntervalMs = 7;
On 2013/10/28 10:12:00, Dominik Grewe wrote:
> I think we should change this to 16, so we're roughly sending one event per
> frame. I remember having problems on Android with this value (the current
> implementations on Android completely bypasses this mechanims).
> 
> Also, rename the constant so it's more in line with the method name
> GetSyntheticGestureUpdateRate().

Done.

https://codereview.chromium.org/26664002/diff/713001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_gesture_target_base.cc:48:
QueueWebMouseWheelEventToPlatform(*web_wheel, event.latency_info);
On 2013/10/28 10:12:00, Dominik Grewe wrote:
> MOUSE_INPUT isn't necessarily a WebMouseWheelEvent, it could also be a
> WebMouseEvent.

Done.

https://codereview.chromium.org/26664002/diff/713001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_gesture_target_base.cc:86: return
gesture_source_type == SyntheticGestureParams::MOUSE_INPUT;
On 2013/10/28 10:12:00, Dominik Grewe wrote:
> Base also supports touch.

Done.

https://codereview.chromium.org/26664002/diff/713001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/26664002/diff/713001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_aura.cc:93: using
WebKit::WebInputEvent;
On 2013/10/28 10:12:00, Dominik Grewe wrote:
> I don't think this is needed anymore.

Done.

[Dominik Grewe, 2013-10-29 09:12:02]

Thanks Kouhei.
Are you planning to land the Android and Aura patches separately or is it all
going to be in this one?

[Dominik Grewe, 2013-10-29 10:18:06]

Also, did you see Jared's comments? He added some to patch set 5 so maybe you've
overlooked them.

[kouhei (in TOK), 2013-10-29 16:22:53]

> Are you planning to land the Android and Aura patches separately or is it all
> going to be in this one?
I think we can land them both with this CL. I can separate them again before
owner reviews if it is more convinient to you.

On 2013/10/29 10:18:06, Dominik Grewe wrote:
> Also, did you see Jared's comments? He added some to patch set 5 so maybe
you've
> overlooked them.

Sorry I missed it. I'll fix it tomorrow (12h later).

[Dominik Grewe, 2013-10-29 16:29:44]

On 2013/10/29 16:22:53, kouhei wrote:
> > Are you planning to land the Android and Aura patches separately or is it
all
> > going to be in this one?
> I think we can land them both with this CL. I can separate them again before
> owner reviews if it is more convenient to you.

That's fine with me. Just make sure you change the description of the patch and
get Sadrul to review the Aura bits.

> 
> On 2013/10/29 10:18:06, Dominik Grewe wrote:
> > Also, did you see Jared's comments? He added some to patch set 5 so maybe
> you've
> > overlooked them.
> 
> Sorry I missed it. I'll fix it tomorrow (12h later).

Great, thanks!

[kouhei (in TOK), 2013-10-30 03:18:34]

https://codereview.chromium.org/26664002/diff/93001/content/browser/renderer_...
File content/browser/renderer_host/input/synthetic_gesture_target_android.cc
(right):

https://codereview.chromium.org/26664002/diff/93001/content/browser/renderer_...
content/browser/renderer_host/input/synthetic_gesture_target_android.cc:53:
SyntheticGestureTargetAndroid::Action action =
On 2013/10/28 15:59:47, jdduke wrote:
> Rather than storing |env_|, just attach here and hand it to Touch{SetPointer,
> Inject}.

Done.

https://codereview.chromium.org/26664002/diff/93001/content/browser/renderer_...
File content/browser/renderer_host/input/synthetic_gesture_target_android.h
(right):

https://codereview.chromium.org/26664002/diff/93001/content/browser/renderer_...
content/browser/renderer_host/input/synthetic_gesture_target_android.h:19:
SyntheticGestureTargetAndroid(RenderWidgetHostViewAndroid* render_view);
On 2013/10/28 15:59:47, jdduke wrote:
> What if we have as an argument here |java_touch_event|?

Done.

https://codereview.chromium.org/26664002/diff/93001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_android.h (right):

https://codereview.chromium.org/26664002/diff/93001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_android.h:248:
ContentViewCoreImpl* content_view_core() { return content_view_core_; }
On 2013/10/28 15:59:47, jdduke wrote:
> I think we don't need this if we just give SyntheticGestureTargetAndroid the
> "SyntheticTouchEvent" java handle on construction.

Done.

https://codereview.chromium.org/26664002/diff/93001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/26664002/diff/93001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:93: using
WebKit::WebInputEvent;
On 2013/10/28 15:59:47, jdduke wrote:
> Nit: I don't think you need this using decl now?

Done.

[kouhei (in TOK), 2013-10-30 03:25:57]

sadrul, aelias: Would you take a look? I merged changes from
http://crrev.com/25098004 to this CL.

[jdduke (slow), 2013-10-30 19:02:42]

https://codereview.chromium.org/26664002/diff/993001/content/browser/renderer...
File content/browser/renderer_host/input/synthetic_gesture_target_base.cc
(right):

https://codereview.chromium.org/26664002/diff/993001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_gesture_target_base.cc:88:
SyntheticGestureNew::Result result) {

Did we need to do something here? Send an ack to the renderer? Or maybe that
comes later when it's all wired up?

https://codereview.chromium.org/26664002/diff/993001/content/browser/renderer...
File content/browser/renderer_host/input/synthetic_gesture_target_base.h
(right):

https://codereview.chromium.org/26664002/diff/993001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_gesture_target_base.h:27: explicit
SyntheticGestureTargetBase(RenderWidgetHostView* render_view);

Are there issues with holding a host view reference in the host widget?  Or will
the controller be destroyed if the view goes away?

https://codereview.chromium.org/26664002/diff/993001/content/port/browser/ren...
File content/port/browser/render_widget_host_view_port.h (right):

https://codereview.chromium.org/26664002/diff/993001/content/port/browser/ren...
content/port/browser/render_widget_host_view_port.h:302: virtual
SyntheticGestureTarget* CreateSyntheticGestureTarget() = 0;

Would it be possible to return a scoped_ptr?

[Dominik Grewe, 2013-10-30 19:11:58]

https://codereview.chromium.org/26664002/diff/993001/content/browser/renderer...
File content/browser/renderer_host/input/synthetic_gesture_target_base.cc
(right):

https://codereview.chromium.org/26664002/diff/993001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_gesture_target_base.cc:88:
SyntheticGestureNew::Result result) {
On 2013/10/30 19:02:43, jdduke wrote:
> 
> Did we need to do something here? Send an ack to the renderer? Or maybe that
> comes later when it's all wired up?

Yes, we don't have the message types yet. We'll add it once it's all being wired
up.

https://codereview.chromium.org/26664002/diff/993001/content/browser/renderer...
File content/browser/renderer_host/input/synthetic_gesture_target_base.h
(right):

https://codereview.chromium.org/26664002/diff/993001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_gesture_target_base.h:27: explicit
SyntheticGestureTargetBase(RenderWidgetHostView* render_view);
On 2013/10/30 19:02:43, jdduke wrote:
> 
> Are there issues with holding a host view reference in the host widget?  Or
will
> the controller be destroyed if the view goes away?

The controller (currently) lives in RWHI so is independent of the view as far as
I understand. Should we have RWHI instead? Then we can call
RenderWidgetHostImpl::GetView() whenever needed.

[jdduke (slow), 2013-10-31 00:52:50]

https://codereview.chromium.org/26664002/diff/993001/content/browser/renderer...
File content/browser/renderer_host/input/synthetic_gesture_target_base.h
(right):

https://codereview.chromium.org/26664002/diff/993001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_gesture_target_base.h:27: explicit
SyntheticGestureTargetBase(RenderWidgetHostView* render_view);
On 2013/10/30 19:11:59, Dominik Grewe wrote:
> On 2013/10/30 19:02:43, jdduke wrote:
> > 
> > Are there issues with holding a host view reference in the host widget?  Or
> will
> > the controller be destroyed if the view goes away?
> 
> The controller (currently) lives in RWHI so is independent of the view as far
as
> I understand. Should we have RWHI instead? Then we can call
> RenderWidgetHostImpl::GetView() whenever needed.

Either will probably be fine, though I'm not really an authority on View vs
Widget lifetimes so maybe somebody else can speak to that.

[aelias_OOO_until_Jul13, 2013-10-31 01:47:47]

https://codereview.chromium.org/26664002/diff/993001/content/browser/renderer...
File content/browser/renderer_host/input/synthetic_gesture_target_android.cc
(right):

https://codereview.chromium.org/26664002/diff/993001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_gesture_target_android.cc:14: bool
g_jni_initialized = false;
This isn't the usual way to do this.  Could you add this to the list in
content/browser/android/browser_jni_registrar.cc instead?

https://codereview.chromium.org/26664002/diff/993001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_gesture_target_android.cc:31:
CHECK(!java_touch_event_.is_null());
I don't see a strong reason for a CHECK here, please use DCHECK.

https://codereview.chromium.org/26664002/diff/993001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_gesture_target_android.cc:42: 
nit: extra newline

https://codereview.chromium.org/26664002/diff/993001/content/browser/renderer...
File content/browser/renderer_host/input/synthetic_gesture_target_android.h
(right):

https://codereview.chromium.org/26664002/diff/993001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_gesture_target_android.h:21:
base::android::ScopedJavaLocalRef<jobject> java_touch_event);
I'm confused as to why this is taking a single touch event in the constructor. 
Isn't this class to set up a target that will receive a stream of many touch
events?

It looks like you're reusing the same touch event over and over.  But this isn't
what happens in reality and Android code may depend or eventually come to depend
on the fact that each touch event is different.  I think this should create a
different Java touch event object for each, and hold a Java local ref not a
global one.

https://codereview.chromium.org/26664002/diff/993001/content/browser/renderer...
File content/browser/renderer_host/input/synthetic_gesture_target_aura.cc
(right):

https://codereview.chromium.org/26664002/diff/993001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_gesture_target_aura.cc:28:
reinterpret_cast<aura::RootWindow*>(window->GetRootWindow());
Use static_cast for downcasts.  reinterpret_cast is only for crazy dangerous
casts.

https://codereview.chromium.org/26664002/diff/993001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_gesture_target_aura.cc:39: const
unsigned num_touches = touch_with_latency.event.touchesLength;
size_t is preferred to unsigned for counts.

https://codereview.chromium.org/26664002/diff/993001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_gesture_target_aura.cc:52: return;
Should this ever happen?  How about DCHECKing it instead?

https://codereview.chromium.org/26664002/diff/993001/content/browser/renderer...
File content/browser/renderer_host/input/synthetic_gesture_target_aura.h
(right):

https://codereview.chromium.org/26664002/diff/993001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_gesture_target_aura.h:36: return
reinterpret_cast<RenderWidgetHostViewAura*>(render_view());
static_cast

https://codereview.chromium.org/26664002/diff/993001/content/browser/renderer...
File content/browser/renderer_host/input/synthetic_gesture_target_base.cc
(right):

https://codereview.chromium.org/26664002/diff/993001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_gesture_target_base.cc:61: void
SyntheticGestureTargetBase::QueueWebTouchEventToPlatform(
Are these default implementations ever going to be used in practice?  Can we
make these abstract methods and then have NOTREACHED in the Android
implementations of the mouse events instead?

https://codereview.chromium.org/26664002/diff/993001/content/browser/renderer...
File content/browser/renderer_host/input/synthetic_gesture_target_base.h
(right):

https://codereview.chromium.org/26664002/diff/993001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_gesture_target_base.h:27: explicit
SyntheticGestureTargetBase(RenderWidgetHostView* render_view);
Based on the comment in render_widget_host_impl.h (near view_), RWHI can outlive
RWHV.  I agree it seems safer to hold a pointer back to RWHI.

https://codereview.chromium.org/26664002/diff/993001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_gesture_target_base.h:48: virtual
base::TimeDelta GetSyntheticGestureUpdateRate() const OVERRIDE;
Why should the target decide this rate?  Can we make the constant live in the
telemetry code instead?

https://codereview.chromium.org/26664002/diff/993001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_gesture_target_base.h:51:
GetDefaultSyntheticGestureSourceType() const OVERRIDE;
Likewise, can telemetry decide this without asking the target?

https://codereview.chromium.org/26664002/diff/993001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_gesture_target_base.h:52: virtual
bool SupportsSyntheticGestureSourceType(
Can we also remove this and just mandate that all targets must support all
types?

[aelias_OOO_until_Jul13, 2013-10-31 01:51:40]

https://codereview.chromium.org/26664002/diff/993001/content/browser/renderer...
File content/browser/renderer_host/input/synthetic_gesture_target_base.h
(right):

https://codereview.chromium.org/26664002/diff/993001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_gesture_target_base.h:52: virtual
bool SupportsSyntheticGestureSourceType(
On 2013/10/31 01:47:48, aelias wrote:
> Can we also remove this and just mandate that all targets must support all
> types?

Sorry, leftover comment before I noticed the Android implementation doesn't
support mouse events.  We don't have to add mouse event support to Android (even
though mice are actually supported on Android), although I'm still not sure this
method is useful for callers in practice for anything beyond DCHECKs.

[kouhei (in TOK), 2013-10-31 04:26:30]

Thanks for your comments!

https://codereview.chromium.org/26664002/diff/993001/content/browser/renderer...
File content/browser/renderer_host/input/synthetic_gesture_target_android.cc
(right):

https://codereview.chromium.org/26664002/diff/993001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_gesture_target_android.cc:14: bool
g_jni_initialized = false;
On 2013/10/31 01:47:48, aelias wrote:
> This isn't the usual way to do this.  Could you add this to the list in
> content/browser/android/browser_jni_registrar.cc instead?

Done.

https://codereview.chromium.org/26664002/diff/993001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_gesture_target_android.cc:31:
CHECK(!java_touch_event_.is_null());
On 2013/10/31 01:47:48, aelias wrote:
> I don't see a strong reason for a CHECK here, please use DCHECK.

Done.

https://codereview.chromium.org/26664002/diff/993001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_gesture_target_android.cc:42: 
On 2013/10/31 01:47:48, aelias wrote:
> nit: extra newline

Done.

https://codereview.chromium.org/26664002/diff/993001/content/browser/renderer...
File content/browser/renderer_host/input/synthetic_gesture_target_android.h
(right):

https://codereview.chromium.org/26664002/diff/993001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_gesture_target_android.h:21:
base::android::ScopedJavaLocalRef<jobject> java_touch_event);
On 2013/10/31 01:47:48, aelias wrote:
> I'm confused as to why this is taking a single touch event in the constructor.

> Isn't this class to set up a target that will receive a stream of many touch
> events?
> 
> It looks like you're reusing the same touch event over and over.  But this
isn't
> what happens in reality and Android code may depend or eventually come to
depend
> on the fact that each touch event is different.  I think this should create a
> different Java touch event object for each, and hold a Java local ref not a
> global one.
java_touch_event is a SyntheticTouchEvent instance. SyntheticTouchEvent is a
factory for MotionEvent added in this CL. We are using new MotionEvent per touch
event.

https://codereview.chromium.org/26664002/diff/993001/content/browser/renderer...
File content/browser/renderer_host/input/synthetic_gesture_target_aura.cc
(right):

https://codereview.chromium.org/26664002/diff/993001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_gesture_target_aura.cc:28:
reinterpret_cast<aura::RootWindow*>(window->GetRootWindow());
On 2013/10/31 01:47:48, aelias wrote:
> Use static_cast for downcasts.  reinterpret_cast is only for crazy dangerous
> casts.

Done.

https://codereview.chromium.org/26664002/diff/993001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_gesture_target_aura.cc:39: const
unsigned num_touches = touch_with_latency.event.touchesLength;
On 2013/10/31 01:47:48, aelias wrote:
> size_t is preferred to unsigned for counts.

Done.

https://codereview.chromium.org/26664002/diff/993001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_gesture_target_aura.cc:52: return;
On 2013/10/31 01:47:48, aelias wrote:
> Should this ever happen?  How about DCHECKing it instead?

Done.

https://codereview.chromium.org/26664002/diff/993001/content/browser/renderer...
File content/browser/renderer_host/input/synthetic_gesture_target_aura.h
(right):

https://codereview.chromium.org/26664002/diff/993001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_gesture_target_aura.h:36: return
reinterpret_cast<RenderWidgetHostViewAura*>(render_view());
On 2013/10/31 01:47:48, aelias wrote:
> static_cast
Ack. This code was deleted.

https://codereview.chromium.org/26664002/diff/993001/content/browser/renderer...
File content/browser/renderer_host/input/synthetic_gesture_target_base.cc
(right):

https://codereview.chromium.org/26664002/diff/993001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_gesture_target_base.cc:61: void
SyntheticGestureTargetBase::QueueWebTouchEventToPlatform(
On 2013/10/31 01:47:48, aelias wrote:
> Are these default implementations ever going to be used in practice?  Can we
> make these abstract methods and then have NOTREACHED in the Android
> implementations of the mouse events instead?

These will be used on non-Android/aura platforms as a fallback implementation.

https://codereview.chromium.org/26664002/diff/993001/content/browser/renderer...
File content/browser/renderer_host/input/synthetic_gesture_target_base.h
(right):

https://codereview.chromium.org/26664002/diff/993001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_gesture_target_base.h:27: explicit
SyntheticGestureTargetBase(RenderWidgetHostView* render_view);
On 2013/10/31 01:47:48, aelias wrote:
> Based on the comment in render_widget_host_impl.h (near view_), RWHI can
outlive
> RWHV.  I agree it seems safer to hold a pointer back to RWHI.

Done.

[aelias_OOO_until_Jul13, 2013-10-31 08:52:42]

lgtm modulo naming comment below.

https://codereview.chromium.org/26664002/diff/993001/content/browser/renderer...
File content/browser/renderer_host/input/synthetic_gesture_target_android.h
(right):

https://codereview.chromium.org/26664002/diff/993001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_gesture_target_android.h:21:
base::android::ScopedJavaLocalRef<jobject> java_touch_event);
On 2013/10/31 04:26:36, kouhei wrote:
> On 2013/10/31 01:47:48, aelias wrote:
> > I'm confused as to why this is taking a single touch event in the
constructor.
> 
> > Isn't this class to set up a target that will receive a stream of many touch
> > events?
> > 
> > It looks like you're reusing the same touch event over and over.  But this
> isn't
> > what happens in reality and Android code may depend or eventually come to
> depend
> > on the fact that each touch event is different.  I think this should create
a
> > different Java touch event object for each, and hold a Java local ref not a
> > global one.
> java_touch_event is a SyntheticTouchEvent instance. SyntheticTouchEvent is a
> factory for MotionEvent added in this CL. We are using new MotionEvent per
touch
> event.

OK.  Can you please rename it to TouchEventSynthesizer then?

[Dominik Grewe, 2013-11-01 16:28:37]

The gestures are working with device independent pixels (DIPs). On Android, they
need to be converted to physical pixels (see comment below) before sending them
to the platform. Is this also true for Aura?

https://codereview.chromium.org/26664002/diff/1183001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/SyntheticTouchEvent.java
(right):

https://codereview.chromium.org/26664002/diff/1183001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/SyntheticTouchEvent.java:44:
coords.y = y;
Convert coordinates from device independent pixels (DIPs) to device (or
physical) pixels by multiplying with
|mContentViewCore.getRenderCoordinates().getDeviceScaleFactor()|.

[sadrul, 2013-11-04 19:26:40]

https://codereview.chromium.org/26664002/diff/1183001/content/browser/rendere...
File content/browser/renderer_host/input/synthetic_gesture_target_aura.cc
(right):

https://codereview.chromium.org/26664002/diff/1183001/content/browser/rendere...
content/browser/renderer_host/input/synthetic_gesture_target_aura.cc:13: using
WebKit::WebTouchEvent;
a new line after the #include block

https://codereview.chromium.org/26664002/diff/1183001/content/browser/rendere...
content/browser/renderer_host/input/synthetic_gesture_target_aura.cc:44:
root_window->ConvertPointToHost(&position);
Use aura::client::ScreenPositionClient::ConvertPointToScreen() instead.

https://codereview.chromium.org/26664002/diff/1183001/content/browser/rendere...
content/browser/renderer_host/input/synthetic_gesture_target_aura.cc:50: bool
conversionSucceed = MakeUITouchEventsFromWebTouchEvents(
conversion_success

https://codereview.chromium.org/26664002/diff/1183001/content/browser/rendere...
content/browser/renderer_host/input/synthetic_gesture_target_aura.cc:56:
root_window_host_delegate->OnHostTouchEvent(*iter);
If Queue*<> expects events to be dispatched asynchronously, then this is wrong.
You should use PostTask here instead. Please clarify this in the Queue*
documentations.

https://codereview.chromium.org/26664002/diff/1183001/content/browser/rendere...
content/browser/renderer_host/input/synthetic_gesture_target_aura.cc:62: return
SyntheticGestureParams::TOUCH_INPUT;
I am not so sure if TOUCH_ is the default for aura.

https://codereview.chromium.org/26664002/diff/1183001/content/browser/rendere...
content/browser/renderer_host/input/synthetic_gesture_target_aura.cc:67: return
gesture_source_type == SyntheticGestureParams::TOUCH_INPUT
|| at the end of the first line

https://codereview.chromium.org/26664002/diff/1183001/content/browser/rendere...
File content/browser/renderer_host/input/synthetic_gesture_target_base.h
(right):

https://codereview.chromium.org/26664002/diff/1183001/content/browser/rendere...
content/browser/renderer_host/input/synthetic_gesture_target_base.h:51:
GetDefaultSyntheticGestureSourceType() const OVERRIDE;
Please document these methods in the interface.

[kouhei (in TOK), 2013-11-05 01:32:24]

Thanks for your feedback!

https://codereview.chromium.org/26664002/diff/993001/content/browser/renderer...
File content/browser/renderer_host/input/synthetic_gesture_target_android.h
(right):

https://codereview.chromium.org/26664002/diff/993001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_gesture_target_android.h:21:
base::android::ScopedJavaLocalRef<jobject> java_touch_event);
On 2013/10/31 08:52:42, aelias wrote:
> On 2013/10/31 04:26:36, kouhei wrote:
> > On 2013/10/31 01:47:48, aelias wrote:
> > > I'm confused as to why this is taking a single touch event in the
> constructor.
> > 
> > > Isn't this class to set up a target that will receive a stream of many
touch
> > > events?
> > > 
> > > It looks like you're reusing the same touch event over and over.  But this
> > isn't
> > > what happens in reality and Android code may depend or eventually come to
> > depend
> > > on the fact that each touch event is different.  I think this should
create
> a
> > > different Java touch event object for each, and hold a Java local ref not
a
> > > global one.
> > java_touch_event is a SyntheticTouchEvent instance. SyntheticTouchEvent is a
> > factory for MotionEvent added in this CL. We are using new MotionEvent per
> touch
> > event.
> 
> OK.  Can you please rename it to TouchEventSynthesizer then?

Done.

https://codereview.chromium.org/26664002/diff/1183001/content/browser/rendere...
File content/browser/renderer_host/input/synthetic_gesture_target_aura.cc
(right):

https://codereview.chromium.org/26664002/diff/1183001/content/browser/rendere...
content/browser/renderer_host/input/synthetic_gesture_target_aura.cc:13: using
WebKit::WebTouchEvent;
On 2013/11/04 19:26:42, sadrul wrote:
> a new line after the #include block

Done.

https://codereview.chromium.org/26664002/diff/1183001/content/browser/rendere...
content/browser/renderer_host/input/synthetic_gesture_target_aura.cc:44:
root_window->ConvertPointToHost(&position);
On 2013/11/04 19:26:42, sadrul wrote:
> Use aura::client::ScreenPositionClient::ConvertPointToScreen() instead.

Done.

https://codereview.chromium.org/26664002/diff/1183001/content/browser/rendere...
content/browser/renderer_host/input/synthetic_gesture_target_aura.cc:50: bool
conversionSucceed = MakeUITouchEventsFromWebTouchEvents(
On 2013/11/04 19:26:42, sadrul wrote:
> conversion_success

Done.

https://codereview.chromium.org/26664002/diff/1183001/content/browser/rendere...
content/browser/renderer_host/input/synthetic_gesture_target_aura.cc:56:
root_window_host_delegate->OnHostTouchEvent(*iter);
On 2013/11/04 19:26:42, sadrul wrote:
> If Queue*<> expects events to be dispatched asynchronously, then this is
wrong.
> You should use PostTask here instead. Please clarify this in the Queue*
> documentations.

IIUC, QueueWebTouchEventToPlatform can either asynchronously/synchronously
dispatch events.
domonikg: Am I getting this correct?

https://codereview.chromium.org/26664002/diff/1183001/content/browser/rendere...
content/browser/renderer_host/input/synthetic_gesture_target_aura.cc:62: return
SyntheticGestureParams::TOUCH_INPUT;
On 2013/11/04 19:26:42, sadrul wrote:
> I am not so sure if TOUCH_ is the default for aura.

Ack. I'll fix this in new CL once I have aura-native impl for
QueueWebMouseWheelEventToPlatform.

https://codereview.chromium.org/26664002/diff/1183001/content/browser/rendere...
content/browser/renderer_host/input/synthetic_gesture_target_aura.cc:67: return
gesture_source_type == SyntheticGestureParams::TOUCH_INPUT
On 2013/11/04 19:26:42, sadrul wrote:
> || at the end of the first line

Done.

https://codereview.chromium.org/26664002/diff/1183001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/SyntheticTouchEvent.java
(right):

https://codereview.chromium.org/26664002/diff/1183001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/SyntheticTouchEvent.java:44:
coords.y = y;
On 2013/11/01 16:28:38, Dominik Grewe wrote:
> Convert coordinates from device independent pixels (DIPs) to device (or
> physical) pixels by multiplying with
> |mContentViewCore.getRenderCoordinates().getDeviceScaleFactor()|.

Done.

[Dominik Grewe, 2013-11-05 10:32:47]

Thanks Kouhei.

https://codereview.chromium.org/26664002/diff/1183001/content/browser/rendere...
File content/browser/renderer_host/input/synthetic_gesture_target_aura.cc
(right):

https://codereview.chromium.org/26664002/diff/1183001/content/browser/rendere...
content/browser/renderer_host/input/synthetic_gesture_target_aura.cc:56:
root_window_host_delegate->OnHostTouchEvent(*iter);
On 2013/11/05 01:32:26, kouhei wrote:
> On 2013/11/04 19:26:42, sadrul wrote:
> > If Queue*<> expects events to be dispatched asynchronously, then this is
> wrong.
> > You should use PostTask here instead. Please clarify this in the Queue*
> > documentations.
> 
> IIUC, QueueWebTouchEventToPlatform can either asynchronously/synchronously
> dispatch events.
> domonikg: Am I getting this correct?

Good question. Afaict synchronous dispatch is fine, that's what we're doing on
Android and also in the current implementation of smooth scroll on Aura.

https://codereview.chromium.org/26664002/diff/1183001/content/browser/rendere...
File content/browser/renderer_host/input/synthetic_gesture_target_base.h
(right):

https://codereview.chromium.org/26664002/diff/1183001/content/browser/rendere...
content/browser/renderer_host/input/synthetic_gesture_target_base.h:51:
GetDefaultSyntheticGestureSourceType() const OVERRIDE;
On 2013/11/04 19:26:42, sadrul wrote:
> Please document these methods in the interface.

Is it okay if we did this in a follow-up CL? We'll be making some minor changes
to that interface anyway.

https://codereview.chromium.org/26664002/diff/1373001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/TouchEventSynthesizer.java
(right):

https://codereview.chromium.org/26664002/diff/1373001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/TouchEventSynthesizer.java:42:
// Convert coordinates from device independent pixels to device dependent pixels
Actually it's density independent pixels, not device independent pixels (see
http://trac.webkit.org/wiki/ScalesAndZooms). Sorry, my mistake.

[sadrul, 2013-11-05 17:53:23]

lgtm

https://codereview.chromium.org/26664002/diff/1183001/content/browser/rendere...
File content/browser/renderer_host/input/synthetic_gesture_target_aura.cc
(right):

https://codereview.chromium.org/26664002/diff/1183001/content/browser/rendere...
content/browser/renderer_host/input/synthetic_gesture_target_aura.cc:56:
root_window_host_delegate->OnHostTouchEvent(*iter);
On 2013/11/05 10:32:48, Dominik Grewe wrote:
> On 2013/11/05 01:32:26, kouhei wrote:
> > On 2013/11/04 19:26:42, sadrul wrote:
> > > If Queue*<> expects events to be dispatched asynchronously, then this is
> > wrong.
> > > You should use PostTask here instead. Please clarify this in the Queue*
> > > documentations.
> > 
> > IIUC, QueueWebTouchEventToPlatform can either asynchronously/synchronously
> > dispatch events.
> > domonikg: Am I getting this correct?
> 
> Good question. Afaict synchronous dispatch is fine, that's what we're doing on
> Android and also in the current implementation of smooth scroll on Aura.

OK. Perhaps 'Dispatch' would be a better name. 'Queue' in my mind implies async
dispatch.

https://codereview.chromium.org/26664002/diff/1183001/content/browser/rendere...
content/browser/renderer_host/input/synthetic_gesture_target_aura.cc:62: return
SyntheticGestureParams::TOUCH_INPUT;
On 2013/11/05 01:32:26, kouhei wrote:
> On 2013/11/04 19:26:42, sadrul wrote:
> > I am not so sure if TOUCH_ is the default for aura.
> 
> Ack. I'll fix this in new CL once I have aura-native impl for
> QueueWebMouseWheelEventToPlatform.

OK. Please add a TODO (and reference the bug# if there is one).

https://codereview.chromium.org/26664002/diff/1183001/content/browser/rendere...
File content/browser/renderer_host/input/synthetic_gesture_target_base.h
(right):

https://codereview.chromium.org/26664002/diff/1183001/content/browser/rendere...
content/browser/renderer_host/input/synthetic_gesture_target_base.h:51:
GetDefaultSyntheticGestureSourceType() const OVERRIDE;
On 2013/11/05 10:32:48, Dominik Grewe wrote:
> On 2013/11/04 19:26:42, sadrul wrote:
> > Please document these methods in the interface.
> 
> Is it okay if we did this in a follow-up CL? We'll be making some minor
changes
> to that interface anyway.

Sure.

https://codereview.chromium.org/26664002/diff/1373001/content/browser/rendere...
File content/browser/renderer_host/input/synthetic_gesture_target_base.cc
(right):

https://codereview.chromium.org/26664002/diff/1373001/content/browser/rendere...
content/browser/renderer_host/input/synthetic_gesture_target_base.cc:60:
NOTREACHED();
Add {}

[kouhei (in TOK), 2013-11-06 03:24:21]

Thanks for your comments.

jam@chromium.org: Would you OWNER review changes in content/ interfaces and gypi
files? Thanks.

https://codereview.chromium.org/26664002/diff/1183001/content/browser/rendere...
File content/browser/renderer_host/input/synthetic_gesture_target_aura.cc
(right):

https://codereview.chromium.org/26664002/diff/1183001/content/browser/rendere...
content/browser/renderer_host/input/synthetic_gesture_target_aura.cc:56:
root_window_host_delegate->OnHostTouchEvent(*iter);
On 2013/11/05 17:53:25, sadrul wrote:
> On 2013/11/05 10:32:48, Dominik Grewe wrote:
> > On 2013/11/05 01:32:26, kouhei wrote:
> > > On 2013/11/04 19:26:42, sadrul wrote:
> > > > If Queue*<> expects events to be dispatched asynchronously, then this is
> > > wrong.
> > > > You should use PostTask here instead. Please clarify this in the Queue*
> > > > documentations.
> > > 
> > > IIUC, QueueWebTouchEventToPlatform can either asynchronously/synchronously
> > > dispatch events.
> > > domonikg: Am I getting this correct?
> > 
> > Good question. Afaict synchronous dispatch is fine, that's what we're doing
on
> > Android and also in the current implementation of smooth scroll on Aura.
> 
> OK. Perhaps 'Dispatch' would be a better name. 'Queue' in my mind implies
async
> dispatch.

To be addressed in the later CL.

https://codereview.chromium.org/26664002/diff/1183001/content/browser/rendere...
content/browser/renderer_host/input/synthetic_gesture_target_aura.cc:62: return
SyntheticGestureParams::TOUCH_INPUT;
On 2013/11/05 17:53:25, sadrul wrote:
> On 2013/11/05 01:32:26, kouhei wrote:
> > On 2013/11/04 19:26:42, sadrul wrote:
> > > I am not so sure if TOUCH_ is the default for aura.
> > 
> > Ack. I'll fix this in new CL once I have aura-native impl for
> > QueueWebMouseWheelEventToPlatform.
> 
> OK. Please add a TODO (and reference the bug# if there is one).

Done.

https://codereview.chromium.org/26664002/diff/1373001/content/browser/rendere...
File content/browser/renderer_host/input/synthetic_gesture_target_base.cc
(right):

https://codereview.chromium.org/26664002/diff/1373001/content/browser/rendere...
content/browser/renderer_host/input/synthetic_gesture_target_base.cc:60:
NOTREACHED();
On 2013/11/05 17:53:25, sadrul wrote:
> Add {}

Done.

https://codereview.chromium.org/26664002/diff/1373001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/TouchEventSynthesizer.java
(right):

https://codereview.chromium.org/26664002/diff/1373001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/TouchEventSynthesizer.java:42:
// Convert coordinates from device independent pixels to device dependent pixels
On 2013/11/05 10:32:48, Dominik Grewe wrote:
> Actually it's density independent pixels, not device independent pixels (see
> http://trac.webkit.org/wiki/ScalesAndZooms). Sorry, my mistake.

Done.

[jam, 2013-11-06 17:47:24]

lgtm

https://codereview.chromium.org/26664002/diff/1423001/content/port/browser/re...
File content/port/browser/render_widget_host_view_port.h (right):

https://codereview.chromium.org/26664002/diff/1423001/content/port/browser/re...
content/port/browser/render_widget_host_view_port.h:303: virtual
scoped_ptr<SyntheticGestureTarget> CreateSyntheticGestureTarget() = 0;
nit: please add a comment

[kouhei (in TOK), 2013-11-11 03:49:29]

Thanks for review!

[commit-bot: I haz the power, 2013-11-11 03:49:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kouhei@chromium.org/26664002/1483001

[commit-bot: I haz the power, 2013-11-11 04:19:36]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
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-11-11 08:14:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kouhei@chromium.org/26664002/1763001

[commit-bot: I haz the power, 2013-11-12 01:08:20]

Change committed as 234354