[Android] Perform eager gesture recognition on MotionEvents

[Android] Perform eager gesture recognition on MotionEvents

ContentViewGestureHandler conditionally forwards MotionEvent's to the Android
gesture detection pipeline, depending on the current touch stream handling
disposition.  However, this approach is fragile, as the detection pipeline
expects a consistent touch stream for each sequence.  Instead, forward all
MotionEvent's immediately for gesture detection, conditionally dispatching the
generated gestures depending on the touch stream ack dispositions.

BUG=295075, 334040, 327444, 240550, 234516
NOTRY=true

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

[jdduke (slow), 2014-01-13 22:36:58]

mkosiba@: My chief worry with this change is how it might affect WebView's hooks
for listening to the gesture stream.  I *think* I've rewired the pipeline
properly so that WebView will not regress, but I'd appreciate it if you could
look at the key changes to android_webview/ and ContentViewCore.  This patch is
very much a rough draft, but any feedback would be great.

tdresser@: Did you have any thoughts on the basic structure of
TouchToGestureQueue? I'll do another cleanup pass later, but if you had any
questions or concerns I'm all ears.  

The current Web-facing change is that a gesture sequence can be
preventDefault'ed by a TouchMove within the slop region.  Also, double-tap
related events will not be prevented if only the first tap in the sequence is
pd'ed, but I think that's less of an issue.

[jdduke (slow), 2014-01-13 22:40:55]

On 2014/01/13 22:36:58, jdduke wrote:
> mkosiba@: My chief worry with this change is how it might affect WebView's
hooks
> for listening to the gesture stream.  I *think* I've rewired the pipeline
> properly so that WebView will not regress, but I'd appreciate it if you could
> look at the key changes to android_webview/ and ContentViewCore.  This patch
is
> very much a rough draft, but any feedback would be great.
> 
> tdresser@: Did you have any thoughts on the basic structure of
> TouchToGestureQueue? I'll do another cleanup pass later, but if you had any
> questions or concerns I'm all ears.  
> 
> The current Web-facing change is that a gesture sequence can be
> preventDefault'ed by a TouchMove within the slop region.  Also, double-tap
> related events will not be prevented if only the first tap in the sequence is
> pd'ed, but I think that's less of an issue.

One note: I have yet to modify the ContentViewGestureHandlerTest accordingly
(this should mostly be code removal), and the TouchToGestureQueue tests need
some more fleshing out as well.

[jdduke (slow), 2014-01-13 23:58:56]

https://codereview.chromium.org/120513005/diff/520001/content/browser/rendere...
File content/browser/renderer_host/input/touch_to_gesture_queue.h (right):

https://codereview.chromium.org/120513005/diff/520001/content/browser/rendere...
content/browser/renderer_host/input/touch_to_gesture_queue.h:38: struct
TouchToGesture {
These data structures aren't very optimized.  I'll likely have a fixed size
array for gestures per touch, and reserve a certain size upon creation of the
|TouchToGesture.sequence| array.

[Rick Byers, 2014-01-14 00:23:16]

Awesome!

https://codereview.chromium.org/120513005/diff/520001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/120513005/diff/520001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1812:
//if (!hasWindowFocus) {
what's this about?

[jdduke (slow), 2014-01-14 00:39:17]

https://codereview.chromium.org/120513005/diff/520001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/120513005/diff/520001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1812:
//if (!hasWindowFocus) {
On 2014/01/14 00:23:16, Rick Byers wrote:
> what's this about?

Ah, yeah, good catch. I disabled this during testing, but I'll need to come up
with an appropriate solution for the current approach, perhaps synthesizing a
cancellation event.

[tdresser, 2014-01-14 16:25:29]

Mainly nits and some naming suggestions.

The biggest thing I'm not sure of is whether or not it makes sense to dispatch
touch events before passing them to the TTGQ, or not.

I'm leaning towards dispatching touch events beforehand, so you don't need to
bother keeping track of them in the TTGQ.

https://codereview.chromium.org/120513005/diff/520001/content/browser/android...
File content/browser/android/content_view_core_impl.h (right):

https://codereview.chromium.org/120513005/diff/520001/content/browser/android...
content/browser/android/content_view_core_impl.h:338: // TouchToGestureQueue
implementation.
TouchToGestureQueue -> TouchToGestureQueueClient

https://codereview.chromium.org/120513005/diff/520001/content/browser/rendere...
File content/browser/renderer_host/input/touch_to_gesture_queue.cc (right):

https://codereview.chromium.org/120513005/diff/520001/content/browser/rendere...
content/browser/renderer_host/input/touch_to_gesture_queue.cc:60: bool
IsTimeout(const WebGestureEvent& event) {
Perhaps IsTimeout -> IsTriggeredByTimeout?

https://codereview.chromium.org/120513005/diff/520001/content/browser/rendere...
content/browser/renderer_host/input/touch_to_gesture_queue.cc:72:
InputEventAckState Intersection(InputEventAckState ack_old,
The word "Intersection" applied to an enum could be interpreted a couple
different ways. Perhaps |LeastConsumed| would be clearer? Any other ideas?

https://codereview.chromium.org/120513005/diff/520001/content/browser/rendere...
content/browser/renderer_host/input/touch_to_gesture_queue.cc:169: void
TouchToGestureQueue::ForwardGestureEvent(const WebGestureEvent& event) {
I don't think this is any more readable than a bigger switch statement which
explicitly tests all the event types. Having all those three line functions
feels a little unnecessary. I'm fine with this approach too though.

https://codereview.chromium.org/120513005/diff/520001/content/browser/rendere...
File content/browser/renderer_host/input/touch_to_gesture_queue.h (right):

https://codereview.chromium.org/120513005/diff/520001/content/browser/rendere...
content/browser/renderer_host/input/touch_to_gesture_queue.h:19: virtual void
ForwardTouchEvent(const blink::WebTouchEvent&) = 0;
I think it may be cleaner to do the touch event dispatch outside of the
TouchToGestureQueue. If you dispatch the touch events before passing the touch
events to the TTGQ, you don't need to store the touch events in the TTGQ at all.

I guess there's also an advantage in unifying the flow of touch and gesture
events, so I'm open to either solution.

https://codereview.chromium.org/120513005/diff/520001/content/browser/rendere...
content/browser/renderer_host/input/touch_to_gesture_queue.h:23: class
CONTENT_EXPORT TouchToGestureQueue {
The names TouchToGesture, TouchToGestureSequence, and TouchToGestureSequence,
are a little confusing.

I think they might be a little clearer as TouchToGestures,
TouchToGesturesSequence, and TouchToGesturesQueue, though those aren't fantastic
names.

If we decide to stop storing touch events, this naming might get a bit simpler
too.

https://codereview.chromium.org/120513005/diff/520001/content/browser/rendere...
content/browser/renderer_host/input/touch_to_gesture_queue.h:28: //
TODO(jdduke): Refactor to take a touch event with a vector of gestures.
Is it reasonable to perform this refactor before this lands? It would make
interaction with the TouchToGestureQueue a lot clearer.

https://codereview.chromium.org/120513005/diff/520001/content/browser/rendere...
File content/browser/renderer_host/input/touch_to_gesture_queue_unittest.cc
(right):

https://codereview.chromium.org/120513005/diff/520001/content/browser/rendere...
content/browser/renderer_host/input/touch_to_gesture_queue_unittest.cc:234: //
An unconsumed touch's gesture should not be sent.
I think these comments are incorrect?

https://codereview.chromium.org/120513005/diff/520001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/120513005/diff/520001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1365:
private boolean filterGesture(int type, int x, int y) {
Can we find a better name for this? It isn't clear what the return value
indicates, and most of what we're doing is updating state, not filtering.

I haven't thought of a name I really like, but perhaps something along the lines
of |processGesture|, |handleGesture|, or |updateForGesture|?

https://codereview.chromium.org/120513005/diff/520001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java
(right):

https://codereview.chromium.org/120513005/diff/520001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:186:

Looks like we normally omit this space (122 occurrences without vs 23
occurrences with).

[mkosiba (inactive), 2014-01-14 16:49:36]

> mkosiba@: My chief worry with this change is how it might affect WebView's
hooks
> for listening to the gesture stream.  I *think* I've rewired the pipeline
> properly so that WebView will not regress, but I'd appreciate it if you could
> look at the key changes to android_webview/ and ContentViewCore.  This patch
is
> very much a rough draft, but any feedback would be great.

nothing obvious stands out. I tried compiling this but couldn't so can't do a
manual
check. Once you have a more polished version of the patch ping me and I'll test
it.

[jdduke (slow), 2014-01-14 23:24:03]

https://codereview.chromium.org/120513005/diff/520001/content/browser/android...
File content/browser/android/content_view_core_impl.h (right):

https://codereview.chromium.org/120513005/diff/520001/content/browser/android...
content/browser/android/content_view_core_impl.h:338: // TouchToGestureQueue
implementation.
On 2014/01/14 16:25:30, tdresser wrote:
> TouchToGestureQueue -> TouchToGestureQueueClient

Done.

https://codereview.chromium.org/120513005/diff/520001/content/browser/rendere...
File content/browser/renderer_host/input/touch_to_gesture_queue.cc (right):

https://codereview.chromium.org/120513005/diff/520001/content/browser/rendere...
content/browser/renderer_host/input/touch_to_gesture_queue.cc:60: bool
IsTimeout(const WebGestureEvent& event) {
On 2014/01/14 16:25:30, tdresser wrote:
> Perhaps IsTimeout -> IsTriggeredByTimeout?

Done.

https://codereview.chromium.org/120513005/diff/520001/content/browser/rendere...
content/browser/renderer_host/input/touch_to_gesture_queue.cc:72:
InputEventAckState Intersection(InputEventAckState ack_old,
On 2014/01/14 16:25:30, tdresser wrote:
> The word "Intersection" applied to an enum could be interpreted a couple
> different ways. Perhaps |LeastConsumed| would be clearer? Any other ideas?

Hmm, I'll noodle on this.

https://codereview.chromium.org/120513005/diff/520001/content/browser/rendere...
content/browser/renderer_host/input/touch_to_gesture_queue.cc:169: void
TouchToGestureQueue::ForwardGestureEvent(const WebGestureEvent& event) {
On 2014/01/14 16:25:30, tdresser wrote:
> I don't think this is any more readable than a bigger switch statement which
> explicitly tests all the event types. Having all those three line functions
> feels a little unnecessary. I'm fine with this approach too though.

You're totally right, this originally started with just the tap queries, but a
switch is better.

https://codereview.chromium.org/120513005/diff/520001/content/browser/rendere...
File content/browser/renderer_host/input/touch_to_gesture_queue.h (right):

https://codereview.chromium.org/120513005/diff/520001/content/browser/rendere...
content/browser/renderer_host/input/touch_to_gesture_queue.h:19: virtual void
ForwardTouchEvent(const blink::WebTouchEvent&) = 0;
On 2014/01/14 16:25:30, tdresser wrote:
> I think it may be cleaner to do the touch event dispatch outside of the
> TouchToGestureQueue. If you dispatch the touch events before passing the touch
> events to the TTGQ, you don't need to store the touch events in the TTGQ at
all.
> 
> I guess there's also an advantage in unifying the flow of touch and gesture
> events, so I'm open to either solution.

The main reason I did it this way was because of the
|OnTouchEventHandling{Begin,End]| methods, requiring that we delay sending the
touch until we've assembled all of its gestures (as the touch ack may be
synchronous).

If we can all agree that PD'ed touchmove's within the touch slop region will
prevent both tapping and scrolling, then I think we can go ahead with forwarding
the touches immediately.

https://codereview.chromium.org/120513005/diff/520001/content/browser/rendere...
content/browser/renderer_host/input/touch_to_gesture_queue.h:23: class
CONTENT_EXPORT TouchToGestureQueue {
On 2014/01/14 16:25:30, tdresser wrote:
> The names TouchToGesture, TouchToGestureSequence, and TouchToGestureSequence,
> are a little confusing.
> 
> I think they might be a little clearer as TouchToGestures,
> TouchToGesturesSequence, and TouchToGesturesQueue, though those aren't
fantastic
> names.
> 
> If we decide to stop storing touch events, this naming might get a bit simpler
> too.
> 
>  

Let's just call it GestureEventQueue? To match TouchEventQueue?

https://codereview.chromium.org/120513005/diff/520001/content/browser/rendere...
content/browser/renderer_host/input/touch_to_gesture_queue.h:28: //
TODO(jdduke): Refactor to take a touch event with a vector of gestures.
On 2014/01/14 16:25:30, tdresser wrote:
> Is it reasonable to perform this refactor before this lands? It would make
> interaction with the TouchToGestureQueue a lot clearer.

We could.  With the way gestures are piped here (via callbacks), we'll need to
do the gesture "collecting" somewhere, but that could be offloaded to a wrapper
or even just the ContentViewCoreImpl for now, no need to clutter the interface
for something the caller should be doing.

https://codereview.chromium.org/120513005/diff/520001/content/browser/rendere...
File content/browser/renderer_host/input/touch_to_gesture_queue_unittest.cc
(right):

https://codereview.chromium.org/120513005/diff/520001/content/browser/rendere...
content/browser/renderer_host/input/touch_to_gesture_queue_unittest.cc:234: //
An unconsumed touch's gesture should not be sent.
On 2014/01/14 16:25:30, tdresser wrote:
> I think these comments are incorrect?

Done.

https://codereview.chromium.org/120513005/diff/520001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java
(right):

https://codereview.chromium.org/120513005/diff/520001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:186:

On 2014/01/14 16:25:30, tdresser wrote:
> Looks like we normally omit this space (122 occurrences without vs 23
> occurrences with).

Oops, yeah accidental addition, thanks.

[tdresser, 2014-01-15 13:43:07]

https://codereview.chromium.org/120513005/diff/520001/content/browser/rendere...
File content/browser/renderer_host/input/touch_to_gesture_queue.h (right):

https://codereview.chromium.org/120513005/diff/520001/content/browser/rendere...
content/browser/renderer_host/input/touch_to_gesture_queue.h:19: virtual void
ForwardTouchEvent(const blink::WebTouchEvent&) = 0;
On 2014/01/14 23:24:03, jdduke wrote:
> On 2014/01/14 16:25:30, tdresser wrote:
> > I think it may be cleaner to do the touch event dispatch outside of the
> > TouchToGestureQueue. If you dispatch the touch events before passing the
touch
> > events to the TTGQ, you don't need to store the touch events in the TTGQ at
> all.
> > 
> > I guess there's also an advantage in unifying the flow of touch and gesture
> > events, so I'm open to either solution.
> 
> The main reason I did it this way was because of the
> |OnTouchEventHandling{Begin,End]| methods, requiring that we delay sending the
> touch until we've assembled all of its gestures (as the touch ack may be
> synchronous).
> 
> If we can all agree that PD'ed touchmove's within the touch slop region will
> prevent both tapping and scrolling, then I think we can go ahead with
forwarding
> the touches immediately.

I was under the impression that this solution would already only work if PD'ed
touchmoves within the touch slop region would prevent tapping and scrolling. Is
that incorrect?

https://codereview.chromium.org/120513005/diff/520001/content/browser/rendere...
content/browser/renderer_host/input/touch_to_gesture_queue.h:23: class
CONTENT_EXPORT TouchToGestureQueue {
On 2014/01/14 23:24:03, jdduke wrote:
> On 2014/01/14 16:25:30, tdresser wrote:
> > The names TouchToGesture, TouchToGestureSequence, and
TouchToGestureSequence,
> > are a little confusing.
> > 
> > I think they might be a little clearer as TouchToGestures,
> > TouchToGesturesSequence, and TouchToGesturesQueue, though those aren't
> fantastic
> > names.
> > 
> > If we decide to stop storing touch events, this naming might get a bit
simpler
> > too.
> > 
> >  
> 
> Let's just call it GestureEventQueue? To match TouchEventQueue?

I like that name. I do think that TouchToGesture -> TouchToGestures would also
clarify matters.

https://codereview.chromium.org/120513005/diff/520001/content/browser/rendere...
content/browser/renderer_host/input/touch_to_gesture_queue.h:28: //
TODO(jdduke): Refactor to take a touch event with a vector of gestures.
On 2014/01/14 23:24:03, jdduke wrote:
> On 2014/01/14 16:25:30, tdresser wrote:
> > Is it reasonable to perform this refactor before this lands? It would make
> > interaction with the TouchToGestureQueue a lot clearer.
> 
> We could.  With the way gestures are piped here (via callbacks), we'll need to
> do the gesture "collecting" somewhere, but that could be offloaded to a
wrapper
> or even just the ContentViewCoreImpl for now, no need to clutter the interface
> for something the caller should be doing.

Aggregating the gestures in CVCI sounds like a good plan to me.

[jdduke (slow), 2014-01-15 20:36:35]

OK, did some massive cleanup and refactoring.  This should compile, though I
have yet to update the tests to reflect the latest changes.  That will come
next.  Sorry if you had any compilation problems.

https://codereview.chromium.org/120513005/diff/520001/content/browser/rendere...
File content/browser/renderer_host/input/touch_to_gesture_queue.h (right):

https://codereview.chromium.org/120513005/diff/520001/content/browser/rendere...
content/browser/renderer_host/input/touch_to_gesture_queue.h:19: virtual void
ForwardTouchEvent(const blink::WebTouchEvent&) = 0;
On 2014/01/15 13:43:08, tdresser wrote:
> On 2014/01/14 23:24:03, jdduke wrote:
> > On 2014/01/14 16:25:30, tdresser wrote:
> > > I think it may be cleaner to do the touch event dispatch outside of the
> > > TouchToGestureQueue. If you dispatch the touch events before passing the
> touch
> > > events to the TTGQ, you don't need to store the touch events in the TTGQ
at
> > all.
> > > 
> > > I guess there's also an advantage in unifying the flow of touch and
gesture
> > > events, so I'm open to either solution.
> > 
> > The main reason I did it this way was because of the
> > |OnTouchEventHandling{Begin,End]| methods, requiring that we delay sending
the
> > touch until we've assembled all of its gestures (as the touch ack may be
> > synchronous).
> > 
> > If we can all agree that PD'ed touchmove's within the touch slop region will
> > prevent both tapping and scrolling, then I think we can go ahead with
> forwarding
> > the touches immediately.
> 
> I was under the impression that this solution would already only work if PD'ed
> touchmoves within the touch slop region would prevent tapping and scrolling.
Is
> that incorrect?

In it's current form, that's very much correct.  We'd have to make some
(non-trivial) changes to make it support touch slop suppression (in short, we'd
have to hold touch events until the initial TouchStart ack is received, getting
us into the business of touch queueing which I'd prefer to avoid).   However,
per our discussion, I'm inclined not to support that at all, unless we find in
the future that we absolutely need it.

https://codereview.chromium.org/120513005/diff/520001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/120513005/diff/520001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1365:
private boolean filterGesture(int type, int x, int y) {
On 2014/01/14 16:25:30, tdresser wrote:
> Can we find a better name for this? It isn't clear what the return value
> indicates, and most of what we're doing is updating state, not filtering.
> 
> I haven't thought of a name I really like, but perhaps something along the
lines
> of |processGesture|, |handleGesture|, or |updateForGesture|?

Sure.  I guess the common pattern on Android is |onFooEvent()|, where the
boolean return value indicates that it was handled.  I suggest we do the same,
|onGestureEvent()|, or perhaps |onForwardingGestureEvent()|.

[tdresser, 2014-01-16 14:31:45]

https://codereview.chromium.org/120513005/diff/820001/content/browser/android...
File content/browser/android/content_view_core_impl.h (right):

https://codereview.chromium.org/120513005/diff/820001/content/browser/android...
content/browser/android/content_view_core_impl.h:401:
scoped_ptr<GestureEventQueue> gesture_event_queue_;
Why not just allocate this on the stack?

https://codereview.chromium.org/120513005/diff/820001/content/browser/rendere...
File content/browser/renderer_host/input/gesture_event_packet.cc (right):

https://codereview.chromium.org/120513005/diff/820001/content/browser/rendere...
content/browser/renderer_host/input/gesture_event_packet.cc:18: bool
IsTouchSequenceBegin(const WebTouchEvent& event) {
nit: "IsTouchSequenceStart" probably makes more sense.

https://codereview.chromium.org/120513005/diff/820001/content/browser/rendere...
content/browser/renderer_host/input/gesture_event_packet.cc:49: // TODO(jdduke):
Use GestureTapDown as a new sequence marker?
Agreed, that sounds simpler.

https://codereview.chromium.org/120513005/diff/820001/content/browser/rendere...
content/browser/renderer_host/input/gesture_event_packet.cc:73:
DCHECK_LT(gesture_count_, kMaxGesturesPerTouch);
Is it reasonable to make this a CHECK_LT? I'm pretty likely to add a new gesture
that bumps us over 5 gestures per touch, test on device in release mode, and get
extremely confused.

https://codereview.chromium.org/120513005/diff/820001/content/browser/rendere...
File content/browser/renderer_host/input/gesture_event_queue.cc (right):

https://codereview.chromium.org/120513005/diff/820001/content/browser/rendere...
content/browser/renderer_host/input/gesture_event_queue.cc:19:
InputEventAckState TransitionToMostRestrictiveState(
TransitionToMostRestrictiveState implies that this mutates something. No
"transition" is taking place.
You could actually mutate ack_state_ in here, in which case the name would make
sense.

Otherwise I think naming it GetMostRestrictiveState or MostRestrictiveState
would be reasonable.

https://codereview.chromium.org/120513005/diff/820001/content/browser/rendere...
content/browser/renderer_host/input/gesture_event_queue.cc:71: case
GestureEventPacket::SYNTHETIC:
Couldn't this violate some ordering constraints?

https://codereview.chromium.org/120513005/diff/820001/content/browser/rendere...
content/browser/renderer_host/input/gesture_event_queue.cc:81: // until the ack
in the event that the sequence receives a timeout event.
This is the only place where you're transitioning to the next touch sequence,
isn't it? I'm confused by the comment.

https://codereview.chromium.org/120513005/diff/820001/content/browser/rendere...
content/browser/renderer_host/input/gesture_event_queue.cc:163: //
Timeout-derived gestures should have beeen forwarded or dropped
beeen -> been

[mkosiba (inactive), 2014-01-16 15:08:06]

did some quick testing and one weird thing I observed is that when I add touch
handlers that prevent everything ( http://jsbin.com/uGoqoBi/5/ ) I'm still able
to perform one fling/scroll on the red inner div. This repros both with
android_webview_shell and content_shell_apk

[jdduke (slow), 2014-01-16 15:45:42]

On 2014/01/16 15:08:06, mkosiba wrote:
> did some quick testing and one weird thing I observed is that when I add touch
> handlers that prevent everything ( http://jsbin.com/uGoqoBi/5/ ) I'm still
able
> to perform one fling/scroll on the red inner div. This repros both with
> android_webview_shell and content_shell_apk

OK, thanks for the repro, I'll dig into it.  My main concern was simply that
WebView was getting the right event bits/updates for the types that it was
interested in.  I'll do some more rigorous integration/unit testing over the
next several days.

[jdduke (slow), 2014-01-16 17:17:52]

https://codereview.chromium.org/120513005/diff/820001/content/browser/android...
File content/browser/android/content_view_core_impl.h (right):

https://codereview.chromium.org/120513005/diff/820001/content/browser/android...
content/browser/android/content_view_core_impl.h:401:
scoped_ptr<GestureEventQueue> gesture_event_queue_;
On 2014/01/16 14:31:45, tdresser wrote:
> Why not just allocate this on the stack?

I had this behind a flag before, so there's no longer any need... though it will
never be on the stack proper, as CVCI is always heap allocated.  I'll change.

https://codereview.chromium.org/120513005/diff/820001/content/browser/rendere...
File content/browser/renderer_host/input/gesture_event_packet.cc (right):

https://codereview.chromium.org/120513005/diff/820001/content/browser/rendere...
content/browser/renderer_host/input/gesture_event_packet.cc:18: bool
IsTouchSequenceBegin(const WebTouchEvent& event) {
On 2014/01/16 14:31:45, tdresser wrote:
> nit: "IsTouchSequenceStart" probably makes more sense.

Done.

https://codereview.chromium.org/120513005/diff/820001/content/browser/rendere...
content/browser/renderer_host/input/gesture_event_packet.cc:73:
DCHECK_LT(gesture_count_, kMaxGesturesPerTouch);
On 2014/01/16 14:31:45, tdresser wrote:
> Is it reasonable to make this a CHECK_LT? I'm pretty likely to add a new
gesture
> that bumps us over 5 gestures per touch, test on device in release mode, and
get
> extremely confused.

Yeah.

https://codereview.chromium.org/120513005/diff/820001/content/browser/rendere...
File content/browser/renderer_host/input/gesture_event_queue.cc (right):

https://codereview.chromium.org/120513005/diff/820001/content/browser/rendere...
content/browser/renderer_host/input/gesture_event_queue.cc:19:
InputEventAckState TransitionToMostRestrictiveState(
On 2014/01/16 14:31:45, tdresser wrote:
> TransitionToMostRestrictiveState implies that this mutates something. No
> "transition" is taking place.
> You could actually mutate ack_state_ in here, in which case the name would
make
> sense.
> 
> Otherwise I think naming it GetMostRestrictiveState or MostRestrictiveState
> would be reasonable.

Yeah, except the order matters (i.e., we never go from a restrictive state to a
less restrictive state).  I agree this isn't the best wording, but I'd like the
name to indicate that arg order is important.

https://codereview.chromium.org/120513005/diff/820001/content/browser/rendere...
content/browser/renderer_host/input/gesture_event_queue.cc:71: case
GestureEventPacket::SYNTHETIC:
On 2014/01/16 14:31:45, tdresser wrote:
> Couldn't this violate some ordering constraints?

Yeah, though on Android that constraint is already violated (e.g., synthetic
gestures sent down while the CVGH is awaiting a touch ack will already be
processed out-of-order).

One thought is to simply intersperse synthetic packets with an existing
sequence; they'd be processed in-order, but they'd simply ignore the disposition
of the current sequence and always be sent. wdyt?

https://codereview.chromium.org/120513005/diff/820001/content/browser/rendere...
content/browser/renderer_host/input/gesture_event_queue.cc:81: // until the ack
in the event that the sequence receives a timeout event.
On 2014/01/16 14:31:45, tdresser wrote:
> This is the only place where you're transitioning to the next touch sequence,
> isn't it? I'm confused by the comment.

It's a terrible comment.  I was thinking of adding a TOUCH_END marker so that
we'd pop a sequence then, though in-practice the result should be the same. 
Regardless, it's a bad comment.

https://codereview.chromium.org/120513005/diff/820001/content/browser/rendere...
content/browser/renderer_host/input/gesture_event_queue.cc:163: //
Timeout-derived gestures should have beeen forwarded or dropped
On 2014/01/16 14:31:45, tdresser wrote:
> beeen -> been

Done.

https://codereview.chromium.org/120513005/diff/820001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java
(right):

https://codereview.chromium.org/120513005/diff/820001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:911:
private void reportDoubleTap() {
I'm inclined to remove all double tap reporting in this patch, otherwise it will
have to be completely reworked...  rbyers@: Do we still need/want this?

[Rick Byers, 2014-01-16 17:46:18]

https://codereview.chromium.org/120513005/diff/820001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java
(right):

https://codereview.chromium.org/120513005/diff/820001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:911:
private void reportDoubleTap() {
On 2014/01/16 17:17:53, jdduke wrote:
> I'm inclined to remove all double tap reporting in this patch, otherwise it
will
> have to be completely reworked...  rbyers@: Do we still need/want this?

Hmm, I'd prefer to keep it until we know that we've got the data we need (it was
added in M33, and M33 isn't yet available to users).  Hopefully we'll get the
data we need from M33, but we might find we need to tweak our collection.  How
hard is it to rework it to fit with eager GR?

[jdduke (slow), 2014-01-16 17:51:45]

On 2014/01/16 17:46:18, Rick Byers wrote:
>
https://codereview.chromium.org/120513005/diff/820001/content/public/android/...
> File
>
content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java
> (right):
> 
>
https://codereview.chromium.org/120513005/diff/820001/content/public/android/...
>
content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:911:
> private void reportDoubleTap() {
> On 2014/01/16 17:17:53, jdduke wrote:
> > I'm inclined to remove all double tap reporting in this patch, otherwise it
> will
> > have to be completely reworked...  rbyers@: Do we still need/want this?
> 
> Hmm, I'd prefer to keep it until we know that we've got the data we need (it
was
> added in M33, and M33 isn't yet available to users).  Hopefully we'll get the
> data we need from M33, but we might find we need to tweak our collection.  How
> hard is it to rework it to fit with eager GR?

Probably not too difficult, I think I'd just move all the code to
ContentViewCore.

[tdresser, 2014-01-16 18:28:29]

https://codereview.chromium.org/120513005/diff/820001/content/browser/rendere...
File content/browser/renderer_host/input/gesture_event_queue.cc (right):

https://codereview.chromium.org/120513005/diff/820001/content/browser/rendere...
content/browser/renderer_host/input/gesture_event_queue.cc:71: case
GestureEventPacket::SYNTHETIC:
On 2014/01/16 17:17:53, jdduke wrote:
> On 2014/01/16 14:31:45, tdresser wrote:
> > Couldn't this violate some ordering constraints?
> 
> Yeah, though on Android that constraint is already violated (e.g., synthetic
> gestures sent down while the CVGH is awaiting a touch ack will already be
> processed out-of-order).
> 
> One thought is to simply intersperse synthetic packets with an existing
> sequence; they'd be processed in-order, but they'd simply ignore the
disposition
> of the current sequence and always be sent. wdyt? 

SGTM.

[jdduke (slow), 2014-01-16 20:02:15]

On 2014/01/16 15:08:06, mkosiba wrote:
> did some quick testing and one weird thing I observed is that when I add touch
> handlers that prevent everything ( http://jsbin.com/uGoqoBi/5/ ) I'm still
able
> to perform one fling/scroll on the red inner div. This repros both with
> android_webview_shell and content_shell_apk

Hmm, are you able to repro in a regular ToT build? I did a bunch of debugging,
and was only able to repro if I tried to scroll before the page had finished
loading.  In that case, I'm seeing a NO_CONSUMER ack for the TouchStart, so I
wonder if this may be a touch handler registration issue?

[jdduke (slow), 2014-01-16 21:31:44]

I fleshed out the unit tests a bit more, and did a fun little hack job on
ContentViewGestureHandlerTest. Some of the removed tests I'd like to port to
either TouchEventQueueTest or GestureEventQueueTest, as I think they may hit
cases not currently covered by existing tests.

[mkosiba (inactive), 2014-01-17 10:18:44]

On 2014/01/16 20:02:15, jdduke wrote:
> On 2014/01/16 15:08:06, mkosiba wrote:
> > did some quick testing and one weird thing I observed is that when I add
touch
> > handlers that prevent everything ( http://jsbin.com/uGoqoBi/5/ ) I'm still
> able
> > to perform one fling/scroll on the red inner div. This repros both with
> > android_webview_shell and content_shell_apk
> 
> Hmm, are you able to repro in a regular ToT build? I did a bunch of debugging,
> and was only able to repro if I tried to scroll before the page had finished
> loading.  In that case, I'm seeing a NO_CONSUMER ack for the TouchStart, so I
> wonder if this may be a touch handler registration issue?

yeath, this was a regular ToT build. I thought it was a registration issue but
I could repro this after waiting ~10 seconds after the content had loaded.

[tdresser, 2014-01-17 15:24:16]

https://codereview.chromium.org/120513005/diff/1000001/content/browser/render...
File content/browser/renderer_host/input/gesture_event_queue.h (right):

https://codereview.chromium.org/120513005/diff/1000001/content/browser/render...
content/browser/renderer_host/input/gesture_event_queue.h:44: typedef
base::Callback<void(const GestureEventPacket& packet)> PacketSender;
I'm not super clear why you're adding a layer of abstraction here. Can you
comment this?

https://codereview.chromium.org/120513005/diff/1000001/content/browser/render...
File content/browser/renderer_host/input/gesture_event_queue_unittest.cc
(right):

https://codereview.chromium.org/120513005/diff/1000001/content/browser/render...
content/browser/renderer_host/input/gesture_event_queue_unittest.cc:198: //
Event if the subsequent touch had no consumer, continue dropping gestures.
You mention touches having no consumer fairly frequently in comments, but never
actually pass INPUT_EVENT_ACK_STATE_NO_CONSUMER_EXISTS. Do you just mean that
the touch wasn't consumed?

https://codereview.chromium.org/120513005/diff/1000001/content/browser/render...
content/browser/renderer_host/input/gesture_event_queue_unittest.cc:222:
SendTouchEventACK(INPUT_EVENT_ACK_STATE_NOT_CONSUMED);
Based on the comment, shouldn't this be INPUT_EVENT_ACK_STATE_CONSUMED?

https://codereview.chromium.org/120513005/diff/1000001/content/browser/render...
content/browser/renderer_host/input/gesture_event_queue_unittest.cc:243: // The
first gesture sequence should not be allowed.
"The first gesture sequence should not be allowed." ->
"The second gesture sequence should be allowed."

https://codereview.chromium.org/120513005/diff/1000001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java
(right):

https://codereview.chromium.org/120513005/diff/1000001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:817:
/**
Remove accidental space.

[jdduke (slow), 2014-01-17 19:38:03]

Thanks for review tdresser@.

I've fleshed out the GestureEventQueue unit tests a bit more.  Next I'll be
tackling cleanup for ContentViewGestureHandlerTest.

https://codereview.chromium.org/120513005/diff/1000001/content/browser/render...
File content/browser/renderer_host/input/gesture_event_queue.h (right):

https://codereview.chromium.org/120513005/diff/1000001/content/browser/render...
content/browser/renderer_host/input/gesture_event_queue.h:44: typedef
base::Callback<void(const GestureEventPacket& packet)> PacketSender;
On 2014/01/17 15:24:17, tdresser wrote:
> I'm not super clear why you're adding a layer of abstraction here. Can you
> comment this?

Well, I'd like the GestureSequence to perform dispatch of packets, as there may
be multiple per touch ack, e.g., for queued synthetic or timeout packets. There
are several ways to do this, my initial one had GEQ implement a simple
forwarding interface. I could also have the GestureSequence return a vector of
packets, but I'd prefer not to dynamically allocate anything.

https://codereview.chromium.org/120513005/diff/1000001/content/browser/render...
File content/browser/renderer_host/input/gesture_event_queue_unittest.cc
(right):

https://codereview.chromium.org/120513005/diff/1000001/content/browser/render...
content/browser/renderer_host/input/gesture_event_queue_unittest.cc:198: //
Event if the subsequent touch had no consumer, continue dropping gestures.
On 2014/01/17 15:24:17, tdresser wrote:
> You mention touches having no consumer fairly frequently in comments, but
never
> actually pass INPUT_EVENT_ACK_STATE_NO_CONSUMER_EXISTS. Do you just mean that
> the touch wasn't consumed?

Yeah, I'll tighten up the verbiage here (though I also should add some
NO_CONSUMER_EXISTS coverage).

https://codereview.chromium.org/120513005/diff/1000001/content/browser/render...
content/browser/renderer_host/input/gesture_event_queue_unittest.cc:222:
SendTouchEventACK(INPUT_EVENT_ACK_STATE_NOT_CONSUMED);
On 2014/01/17 15:24:17, tdresser wrote:
> Based on the comment, shouldn't this be INPUT_EVENT_ACK_STATE_CONSUMED?

Done.

https://codereview.chromium.org/120513005/diff/1000001/content/browser/render...
content/browser/renderer_host/input/gesture_event_queue_unittest.cc:243: // The
first gesture sequence should not be allowed.
On 2014/01/17 15:24:17, tdresser wrote:
> "The first gesture sequence should not be allowed." ->
> "The second gesture sequence should be allowed."

Done.

https://codereview.chromium.org/120513005/diff/1000001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java
(right):

https://codereview.chromium.org/120513005/diff/1000001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:817:
/**
On 2014/01/17 15:24:17, tdresser wrote:
> Remove accidental space.

I'm not seeing the space :(

[tdresser, 2014-01-17 19:43:33]

https://codereview.chromium.org/120513005/diff/1000001/content/browser/render...
File content/browser/renderer_host/input/gesture_event_queue.h (right):

https://codereview.chromium.org/120513005/diff/1000001/content/browser/render...
content/browser/renderer_host/input/gesture_event_queue.h:44: typedef
base::Callback<void(const GestureEventPacket& packet)> PacketSender;
On 2014/01/17 19:38:04, jdduke wrote:
> On 2014/01/17 15:24:17, tdresser wrote:
> > I'm not super clear why you're adding a layer of abstraction here. Can you
> > comment this?
> 
> Well, I'd like the GestureSequence to perform dispatch of packets, as there
may
> be multiple per touch ack, e.g., for queued synthetic or timeout packets.
There
> are several ways to do this, my initial one had GEQ implement a simple
> forwarding interface. I could also have the GestureSequence return a vector of
> packets, but I'd prefer not to dynamically allocate anything.

Sounds reasonable.

https://codereview.chromium.org/120513005/diff/1000001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java
(right):

https://codereview.chromium.org/120513005/diff/1000001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:817:
/**
On 2014/01/17 19:38:04, jdduke wrote:
> On 2014/01/17 15:24:17, tdresser wrote:
> > Remove accidental space.
> 
> I'm not seeing the space :(

Whoops, I was confused by looking at "delta from patch set" diffs which included
changes other than yours...

[jdduke (slow), 2014-01-18 16:36:00]

https://codereview.chromium.org/120513005/diff/1420001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/120513005/diff/1420001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1376:
private boolean onForwardingGestureEvent(int type, int x, int y) {
TODO: Fix |type|, as it's completely bogus.  WebInputEvent::Type does NOT align
with ContentViewGestureHandler.Type, so we need a way to map between the two.

https://codereview.chromium.org/120513005/diff/1420001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2010:
if (type != ContentViewGestureHandler.GESTURE_SINGLE_TAP_CONFIRMED
TODO(jdduke): These types are all wrong.  We need to convert back from
WebInputEvent::Type, though I fear that's lossy for SINGLE_TAP_CONFIRMED from
GestureTap.

[Rick Byers, 2014-01-21 20:10:09]

A few higher-level comments

https://codereview.chromium.org/120513005/diff/1420001/content/browser/render...
File content/browser/renderer_host/input/gesture_event_queue.cc (right):

https://codereview.chromium.org/120513005/diff/1420001/content/browser/render...
content/browser/renderer_host/input/gesture_event_queue.cc:50: case
GestureEventPacket::TOUCH_TIMEOUT:
This special handling for TOUCH_TIMEOUT and SYNTHETIC seems somewhat redundant
with the logic in OnTouchEventAck.  Can we unify these somehow to simplify
things?  Eg. are you just trying to ensure these get sent without needing a
touch ack?  Maybe I just don't fully understand why these cases need to be
handled in a fundamentally different fashion.  Overall this makes what I naively
expected to be relatively simple logic around gesture event dispatch rather more
complicated.

https://codereview.chromium.org/120513005/diff/1420001/content/browser/render...
content/browser/renderer_host/input/gesture_event_queue.cc:61: // Handle the
synthetic packet immediately if no other packets pending.
Can you talk a bit about why synthetic gestures are handled differently?  Are
these for gestures created without any corresponding touch events?  Where do we
still do that (Dominik's synthetic gesture framework seems superior in most
cases IMHO)?

https://codereview.chromium.org/120513005/diff/1420001/content/browser/render...
content/browser/renderer_host/input/gesture_event_queue.cc:189: ack_state_ =
GetMostRestrictiveState(old_ack_state, new_ack_state);
Couldn't this just be a single bit "is gesture allowed", rather than trying to
track some (imho awkward) notion of 'most restrictive ack state'?

https://codereview.chromium.org/120513005/diff/1420001/content/browser/render...
File content/browser/renderer_host/input/gesture_event_queue.h (right):

https://codereview.chromium.org/120513005/diff/1420001/content/browser/render...
content/browser/renderer_host/input/gesture_event_queue.h:28: class
CONTENT_EXPORT GestureEventQueue {
I think we need either more discussion of the design here, or a link to the
design doc (not sure if you guys are still maintaining the one Tim started
with).  I may be slow (or short on caffine right now), but a bunch of the
details/motivation here are non-obvious to me (and eager GR was my idea to begin
with <grin>).  I'll add a few comments throughout.

https://codereview.chromium.org/120513005/diff/1420001/content/browser/render...
content/browser/renderer_host/input/gesture_event_queue.h:67: InputEventAckState
ack_state_;
maybe call this 'aggregate_ack_state' or something?  To me it's not really an
individual "ack state" at all anymore, but some representation of all the acks
that have been received for this sequence.  Eg. I believe I'll need to modify
this to track a bit more state for some stuff I'm doing.

[jdduke (slow), 2014-01-21 20:52:36]

Thanks for review!

https://codereview.chromium.org/120513005/diff/1420001/content/browser/render...
File content/browser/renderer_host/input/gesture_event_queue.cc (right):

https://codereview.chromium.org/120513005/diff/1420001/content/browser/render...
content/browser/renderer_host/input/gesture_event_queue.cc:50: case
GestureEventPacket::TOUCH_TIMEOUT:
On 2014/01/21 20:10:10, Rick Byers wrote:
> This special handling for TOUCH_TIMEOUT and SYNTHETIC seems somewhat redundant
> with the logic in OnTouchEventAck.  Can we unify these somehow to simplify
> things?  Eg. are you just trying to ensure these get sent without needing a
> touch ack?  Maybe I just don't fully understand why these cases need to be
> handled in a fundamentally different fashion.  Overall this makes what I
naively
> expected to be relatively simple logic around gesture event dispatch rather
more
> complicated.

It's not redundant, unfortunately.   Synthetic packets will always be sent, but
they will be dispatched in the order they were received with respect to other
packets.  Timeout packets will only be sent if the current sequence is
"allowed".  I should make that clear in the comments, thanks.

https://codereview.chromium.org/120513005/diff/1420001/content/browser/render...
content/browser/renderer_host/input/gesture_event_queue.cc:61: // Handle the
synthetic packet immediately if no other packets pending.
On 2014/01/21 20:10:10, Rick Byers wrote:
> Can you talk a bit about why synthetic gestures are handled differently?  Are
> these for gestures created without any corresponding touch events?  Where do
we
> still do that (Dominik's synthetic gesture framework seems superior in most
> cases IMHO)?

This is done for things like WebView's zoom controls (the +/- overlay).

https://codereview.chromium.org/120513005/diff/1420001/content/browser/render...
content/browser/renderer_host/input/gesture_event_queue.cc:189: ack_state_ =
GetMostRestrictiveState(old_ack_state, new_ack_state);
On 2014/01/21 20:10:10, Rick Byers wrote:
> Couldn't this just be a single bit "is gesture allowed", rather than trying to
> track some (imho awkward) notion of 'most restrictive ack state'?

It is awkward, and I'm not opposed to using a unique state tracking bit for
this.  However, I think we need more than a binary state (for handling the
initial transition, so a simple enum should be sufficient here.

https://codereview.chromium.org/120513005/diff/1420001/content/browser/render...
File content/browser/renderer_host/input/gesture_event_queue.h (right):

https://codereview.chromium.org/120513005/diff/1420001/content/browser/render...
content/browser/renderer_host/input/gesture_event_queue.h:28: class
CONTENT_EXPORT GestureEventQueue {
On 2014/01/21 20:10:10, Rick Byers wrote:
> I think we need either more discussion of the design here, or a link to the
> design doc (not sure if you guys are still maintaining the one Tim started
> with).  I may be slow (or short on caffine right now), but a bunch of the
> details/motivation here are non-obvious to me (and eager GR was my idea to
begin
> with <grin>).  I'll add a few comments throughout.

aelias@ and I talked about an eager GR a *long* time ago.  The chief motivation
then, for Android, was to prevent gesture detection issues that inevitably arise
from either partial or delayed touch event streams (e.g., a misfiring ShowPress,
or a disappearing TouchEnd).  

Added benefits include complete decoupling of detection from the final output
(making it easier to experiment, and simplifying gesture detection), and a
consistent way of enforcing whether a particular gesture should or should not be
forwarded.

I think there's a lot of room for refinement here, but this implements fully the
current Android touch handling logic, and can certainly be extended for more
nuanced behavior.

https://codereview.chromium.org/120513005/diff/1420001/content/browser/render...
content/browser/renderer_host/input/gesture_event_queue.h:67: InputEventAckState
ack_state_;
On 2014/01/21 20:10:10, Rick Byers wrote:
> maybe call this 'aggregate_ack_state' or something?  To me it's not really an
> individual "ack state" at all anymore, but some representation of all the acks
> that have been received for this sequence.  Eg. I believe I'll need to modify
> this to track a bit more state for some stuff I'm doing.

As noted below, using the ack here is just bad on my part.  I'll switch to a
more appropriate (and localized) state variable.

https://codereview.chromium.org/120513005/diff/1420001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java
(right):

https://codereview.chromium.org/120513005/diff/1420001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:357:
if (isDistanceBetweenDownAndUpTooLong(e.getRawX(), e.getRawY())) {
TODO: As a follow-up, all of this tap bookkeeping can be greatly simplified.

[jdduke (slow), 2014-01-22 18:46:34]

I'd like to start final review for this, please let me know if you have any
questions or the review burden is too great (sorry!)

mkosiba@: android_webview/
skyostil@: content/browser/android and content/public/android
tdresser@: content/browser/renderer_host/input
aelias@: content/browser/renderer_host/render_widget_host*
rbyers@: UMA changes in ContentViewGestureHandler/ContentViewCore, and any
additional input on content/browser/renderer_host/input

Thanks!

[tdresser, 2014-01-23 16:32:42]

I haven't looked at unit tests yet this pass.

I'm seeing some buggy behaviour though, with touch cancelling not happening
consistently.

To repro:
1. Visit jsbin.com/uniLISo/1?quiet=1
2. Press and move your finger quickly
     - As expected, the touch count goes to 1, and then 0, 
       as the touch is cancelled
3. Press and move your finger slowly
     - You expect the count to decrease to 0, but it stays                   
       at 1.

https://codereview.chromium.org/120513005/diff/1670001/content/browser/render...
File content/browser/renderer_host/input/gesture_event_packet.cc (right):

https://codereview.chromium.org/120513005/diff/1670001/content/browser/render...
content/browser/renderer_host/input/gesture_event_packet.cc:8: #include
"content/common/input/web_input_event_traits.h"
Remove unused includes.

https://codereview.chromium.org/120513005/diff/1670001/content/browser/render...
File content/browser/renderer_host/input/gesture_event_packet.h (right):

https://codereview.chromium.org/120513005/diff/1670001/content/browser/render...
content/browser/renderer_host/input/gesture_event_packet.h:41: enum {
kMaxGesturesPerTouch = 5 };
Using an enum for a single constant isn't very common (~64 occurrences, compared
to ~8000 for const int). I'd prefer a const int, unless there's a specific
reason you've used an enum here.

https://codereview.chromium.org/120513005/diff/1670001/content/browser/render...
File content/browser/renderer_host/input/gesture_event_queue.cc (right):

https://codereview.chromium.org/120513005/diff/1670001/content/browser/render...
content/browser/renderer_host/input/gesture_event_queue.cc:60: case
GestureEventPacket::SYNTHETIC:
If this is only for things like WebView's zoom controls, do we care about out of
order dispatch? I think it might be safe to always dispatch events like that
immediately. (Sorry I've been switching back and forth on this). Are there any
synthetic events which would cause issues if they were dispatched out of order?

https://codereview.chromium.org/120513005/diff/1670001/content/browser/render...
content/browser/renderer_host/input/gesture_event_queue.cc:74: void
GestureEventQueue::OnTouchEventAck(InputEventAckState ack_state) {
A DCHECK(!sequences.empty()) might be good here.

https://codereview.chromium.org/120513005/diff/1670001/content/browser/render...
content/browser/renderer_host/input/gesture_event_queue.cc:80: 
A DCHECK(!sequences.empty()) might be good here too.

https://codereview.chromium.org/120513005/diff/1670001/content/browser/render...
content/browser/renderer_host/input/gesture_event_queue.cc:148: 
Perhaps a comment separating GestureEventQueue methods from
GestureEventQueue::GestureSequence methods?

https://codereview.chromium.org/120513005/diff/1670001/content/browser/render...
content/browser/renderer_host/input/gesture_event_queue.cc:171: // |NO_CONSUMER|
should only be effective when the *first* touch is ack'ed.
If we get an INPUT_EVENT_ACK_STATE_NO_CONSUMER_EXISTS, shouldn't all future acks
in the sequence be INPUT_EVENT_ACK_STATE_NOT_CONSUMED?

https://codereview.chromium.org/120513005/diff/1670001/content/browser/render...
File content/browser/renderer_host/input/gesture_event_queue.h (right):

https://codereview.chromium.org/120513005/diff/1670001/content/browser/render...
content/browser/renderer_host/input/gesture_event_queue.h:27: // generating
touch sequence.  Synthetic gestures are dispatched immediately.
Synthetic gestures are currently not always dispatched immediately (but see
comments on gesture_event_queue.cc).

https://codereview.chromium.org/120513005/diff/1670001/content/browser/render...
content/browser/renderer_host/input/gesture_event_queue.h:61:
std::deque<GestureEventPacket> sequence_;
This should just be an std::queue, shouldn't it? Using the more restrictive type
clarifies the purpose of the object.

https://codereview.chromium.org/120513005/diff/1670001/content/browser/render...
content/browser/renderer_host/input/gesture_event_queue.h:80:
std::deque<GestureSequence> sequences_;
std::queue?

[Rick Byers, 2014-01-23 20:45:27]

> rbyers@: UMA changes in ContentViewGestureHandler/ContentViewCore, 

This LGTM

> and any additional input on content/browser/renderer_host/input

See comments, mostly nits at this stage.

https://codereview.chromium.org/120513005/diff/1420001/content/browser/render...
File content/browser/renderer_host/input/gesture_event_queue.cc (right):

https://codereview.chromium.org/120513005/diff/1420001/content/browser/render...
content/browser/renderer_host/input/gesture_event_queue.cc:50: case
GestureEventPacket::TOUCH_TIMEOUT:
On 2014/01/21 20:52:38, jdduke wrote:
> On 2014/01/21 20:10:10, Rick Byers wrote:
> > This special handling for TOUCH_TIMEOUT and SYNTHETIC seems somewhat
redundant
> > with the logic in OnTouchEventAck.  Can we unify these somehow to simplify
> > things?  Eg. are you just trying to ensure these get sent without needing a
> > touch ack?  Maybe I just don't fully understand why these cases need to be
> > handled in a fundamentally different fashion.  Overall this makes what I
> naively
> > expected to be relatively simple logic around gesture event dispatch rather
> more
> > complicated.
> 
> It's not redundant, unfortunately.   Synthetic packets will always be sent,
but
> they will be dispatched in the order they were received with respect to other
> packets.  Timeout packets will only be sent if the current sequence is
> "allowed".  I should make that clear in the comments, thanks.

Sorry, what I meant was that we've got two separate places that need to know
about handling synthetic and timeout packets specially in this regard.  One in
OnGestureEventPacket when they're received, and one in
GestureSequence::OnTouchEventAck when they happen to be the next thing in the
queue.  Can we put some of this logic in a common place and call it from both
locations.  Some sort of MaybeHandlePacketNow(GestureEventPacket) function or
something?  It would return true if the packet was special in some way and so
handled immediately.  Then I think we might be able to have a single switch on
gesture_source() statement in this file instead of two different ones.  It
doesn't look like there's perfect overlap though and I don't fully understand
some of the cases here, so perhaps this would actually be more awkward than
leaving it as is.  Your call.

https://codereview.chromium.org/120513005/diff/1420001/content/browser/render...
content/browser/renderer_host/input/gesture_event_queue.cc:61: // Handle the
synthetic packet immediately if no other packets pending.
On 2014/01/21 20:52:38, jdduke wrote:
> On 2014/01/21 20:10:10, Rick Byers wrote:
> > Can you talk a bit about why synthetic gestures are handled differently? 
Are
> > these for gestures created without any corresponding touch events?  Where do
> we
> > still do that (Dominik's synthetic gesture framework seems superior in most
> > cases IMHO)?
> 
> This is done for things like WebView's zoom controls (the +/- overlay).

Ah, thanks!  I had forgotten we used synthetic gestures in production like that.

https://codereview.chromium.org/120513005/diff/1420001/content/browser/render...
content/browser/renderer_host/input/gesture_event_queue.cc:189: ack_state_ =
GetMostRestrictiveState(old_ack_state, new_ack_state);
On 2014/01/21 20:52:38, jdduke wrote:
> On 2014/01/21 20:10:10, Rick Byers wrote:
> > Couldn't this just be a single bit "is gesture allowed", rather than trying
to
> > track some (imho awkward) notion of 'most restrictive ack state'?
> 
> It is awkward, and I'm not opposed to using a unique state tracking bit for
> this.  However, I think we need more than a binary state (for handling the
> initial transition, so a simple enum should be sufficient here.

Cool, I like this better (has a nice symmetry with the new TouchEventQueue
filtering state too).  Thanks.

https://codereview.chromium.org/120513005/diff/1670001/content/browser/render...
File content/browser/renderer_host/input/gesture_event_packet.cc (right):

https://codereview.chromium.org/120513005/diff/1670001/content/browser/render...
content/browser/renderer_host/input/gesture_event_packet.cc:32: // TODO(jdduke):
Use GestureTapDown as a new sequence marker?
I think clearing using 'period when no touches are on the target window' as the
precise delimitation between sequences seems right to me.  This is what's
important to the user, and gestures are a higher level semantic above it (eg.
conceivably we could decide not to send GTD if we saw two fingers go down at
once in the same input frame, or that some other gesture event could come BEFORE
a GTD in a sequence).  So my vote is probably for removing this TODO and leaving
as-is.

https://codereview.chromium.org/120513005/diff/1670001/content/browser/render...
File content/browser/renderer_host/input/gesture_event_queue.h (right):

https://codereview.chromium.org/120513005/diff/1670001/content/browser/render...
content/browser/renderer_host/input/gesture_event_queue.h:63: PENDING,
Please add a brief comment describing the semantics of each of these sates.  In
particular, the likely distinction between ALWAYS_ALLOWED and ALLOWED isn't
obvious to me (before looking at the code that implements them).

https://codereview.chromium.org/120513005/diff/1670001/content/browser/render...
content/browser/renderer_host/input/gesture_event_queue.h:81: bool
outstanding_tap_;
Can you add a comment for these two making it clear that they're about what's
outstanding on the output side of this buffer, not the input side?  To me
'outstanding' by itself ambiguous in this regard.

https://codereview.chromium.org/120513005/diff/1670001/content/browser/render...
File content/browser/renderer_host/input/gesture_event_queue_unittest.cc
(right):

https://codereview.chromium.org/120513005/diff/1670001/content/browser/render...
content/browser/renderer_host/input/gesture_event_queue_unittest.cc:308:
EXPECT_EQ(WebInputEvent::GestureScrollBegin, sent_gesture().type);
The other expected gesture is a TapCancel, but that's not actually validated
anywhere right?

I think it's worth while to extend the test infrastructure to be able to easily
EXPECT that the gestures are a particular sequence.  The simplest way to do this
is to have a function that just returns the list of gesture types received as a
string, and then EXPECT_EQ on that.  Or you could go more fancy and build an
EXPECT_TYPE_LIST macro which takes a var-arg number of input event types or
something.  Tim did something like this a long time ago in some new Aura GR unit
tests (that ended up getting forgotten after his intership ended), and it turned
out really nice.

[tdresser, 2014-01-23 21:42:16]

https://codereview.chromium.org/120513005/diff/1670001/content/browser/render...
File content/browser/renderer_host/input/gesture_event_queue_unittest.cc
(right):

https://codereview.chromium.org/120513005/diff/1670001/content/browser/render...
content/browser/renderer_host/input/gesture_event_queue_unittest.cc:308:
EXPECT_EQ(WebInputEvent::GestureScrollBegin, sent_gesture().type);
On 2014/01/23 20:45:29, Rick Byers wrote:
> The other expected gesture is a TapCancel, but that's not actually validated
> anywhere right?
> 
> I think it's worth while to extend the test infrastructure to be able to
easily
> EXPECT that the gestures are a particular sequence.  The simplest way to do
this
> is to have a function that just returns the list of gesture types received as
a
> string, and then EXPECT_EQ on that.  Or you could go more fancy and build an
> EXPECT_TYPE_LIST macro which takes a var-arg number of input event types or
> something.  Tim did something like this a long time ago in some new Aura GR
unit
> tests (that ended up getting forgotten after his intership ended), and it
turned
> out really nice.  

Since gmock isn't widely accepted in chromium, I think the right thing to do
here is to define a custom gtest matcher.

Something along the lines of this:
http://stackoverflow.com/questions/10060110/how-does-gtest-compare-the-values...
(though perhaps with size as an additional parameter, instead of relying on
crazy templating)

[jdduke (slow), 2014-01-23 22:52:32]

https://codereview.chromium.org/120513005/diff/1670001/content/browser/render...
File content/browser/renderer_host/input/gesture_event_packet.cc (right):

https://codereview.chromium.org/120513005/diff/1670001/content/browser/render...
content/browser/renderer_host/input/gesture_event_packet.cc:8: #include
"content/common/input/web_input_event_traits.h"
On 2014/01/23 16:32:43, tdresser wrote:
> Remove unused includes.

Done.

https://codereview.chromium.org/120513005/diff/1670001/content/browser/render...
File content/browser/renderer_host/input/gesture_event_packet.h (right):

https://codereview.chromium.org/120513005/diff/1670001/content/browser/render...
content/browser/renderer_host/input/gesture_event_packet.h:41: enum {
kMaxGesturesPerTouch = 5 };
On 2014/01/23 16:32:43, tdresser wrote:
> Using an enum for a single constant isn't very common (~64 occurrences,
compared
> to ~8000 for const int). I'd prefer a const int, unless there's a specific
> reason you've used an enum here.

I think the enum might be a bit more clear here as it's used for the member
array length.

https://codereview.chromium.org/120513005/diff/1670001/content/browser/render...
File content/browser/renderer_host/input/gesture_event_queue.cc (right):

https://codereview.chromium.org/120513005/diff/1670001/content/browser/render...
content/browser/renderer_host/input/gesture_event_queue.cc:60: case
GestureEventPacket::SYNTHETIC:
On 2014/01/23 16:32:43, tdresser wrote:
> If this is only for things like WebView's zoom controls, do we care about out
of
> order dispatch? I think it might be safe to always dispatch events like that
> immediately. (Sorry I've been switching back and forth on this). Are there any
> synthetic events which would cause issues if they were dispatched out of
order?

Honestly I have no idea.  I don't think it hurts to dispatch them in-order, but
there are probably scenarios under both approaches that could confuse the
gesture stream.  I just don't know if any of them happen in practice.

https://codereview.chromium.org/120513005/diff/1670001/content/browser/render...
content/browser/renderer_host/input/gesture_event_queue.cc:74: void
GestureEventQueue::OnTouchEventAck(InputEventAckState ack_state) {
On 2014/01/23 16:32:43, tdresser wrote:
> A DCHECK(!sequences.empty()) might be good here.

Sure. The |Head()| call performs that check, but I can be more explicit if you
like, same below, your call.

https://codereview.chromium.org/120513005/diff/1670001/content/browser/render...
content/browser/renderer_host/input/gesture_event_queue.cc:148: 
On 2014/01/23 16:32:43, tdresser wrote:
> Perhaps a comment separating GestureEventQueue methods from
> GestureEventQueue::GestureSequence methods?

Done.

https://codereview.chromium.org/120513005/diff/1670001/content/browser/render...
content/browser/renderer_host/input/gesture_event_queue.cc:171: // |NO_CONSUMER|
should only be effective when the *first* touch is ack'ed.
On 2014/01/23 16:32:43, tdresser wrote:
> If we get an INPUT_EVENT_ACK_STATE_NO_CONSUMER_EXISTS, shouldn't all future
acks
> in the sequence be INPUT_EVENT_ACK_STATE_NOT_CONSUMED?

If the primary pointer press was NOT_CONSUMED, but remains stationary and the
secondary pointer press is NO_CONSUMER, there's still the possibility that the
primary pointer will then move and be CONSUMED. This was first reported in
crbug/135102.

https://codereview.chromium.org/120513005/diff/1670001/content/browser/render...
File content/browser/renderer_host/input/gesture_event_queue.h (right):

https://codereview.chromium.org/120513005/diff/1670001/content/browser/render...
content/browser/renderer_host/input/gesture_event_queue.h:27: // generating
touch sequence.  Synthetic gestures are dispatched immediately.
On 2014/01/23 16:32:43, tdresser wrote:
> Synthetic gestures are currently not always dispatched immediately (but see
> comments on gesture_event_queue.cc).

Good catch.

https://codereview.chromium.org/120513005/diff/1670001/content/browser/render...
content/browser/renderer_host/input/gesture_event_queue.h:61:
std::deque<GestureEventPacket> sequence_;
On 2014/01/23 16:32:43, tdresser wrote:
> This should just be an std::queue, shouldn't it? Using the more restrictive
type
> clarifies the purpose of the object.

Done.

https://codereview.chromium.org/120513005/diff/1670001/content/browser/render...
content/browser/renderer_host/input/gesture_event_queue.h:80:
std::deque<GestureSequence> sequences_;
On 2014/01/23 16:32:43, tdresser wrote:
> std::queue?

Done.

[jdduke (slow), 2014-01-24 05:34:39]

https://codereview.chromium.org/120513005/diff/1670001/content/browser/render...
File content/browser/renderer_host/input/gesture_event_packet.cc (right):

https://codereview.chromium.org/120513005/diff/1670001/content/browser/render...
content/browser/renderer_host/input/gesture_event_packet.cc:32: // TODO(jdduke):
Use GestureTapDown as a new sequence marker?
On 2014/01/23 20:45:29, Rick Byers wrote:
> I think clearing using 'period when no touches are on the target window' as
the
> precise delimitation between sequences seems right to me.  This is what's
> important to the user, and gestures are a higher level semantic above it (eg.
> conceivably we could decide not to send GTD if we saw two fingers go down at
> once in the same input frame, or that some other gesture event could come
BEFORE
> a GTD in a sequence).  So my vote is probably for removing this TODO and
leaving
> as-is.

Done.

https://codereview.chromium.org/120513005/diff/1670001/content/browser/render...
File content/browser/renderer_host/input/gesture_event_queue.cc (right):

https://codereview.chromium.org/120513005/diff/1670001/content/browser/render...
content/browser/renderer_host/input/gesture_event_queue.cc:80: 
On 2014/01/23 16:32:43, tdresser wrote:
> A DCHECK(!sequences.empty()) might be good here too.

Done.

https://codereview.chromium.org/120513005/diff/1670001/content/browser/render...
File content/browser/renderer_host/input/gesture_event_queue.h (right):

https://codereview.chromium.org/120513005/diff/1670001/content/browser/render...
content/browser/renderer_host/input/gesture_event_queue.h:63: PENDING,
On 2014/01/23 20:45:29, Rick Byers wrote:
> Please add a brief comment describing the semantics of each of these sates. 
In
> particular, the likely distinction between ALWAYS_ALLOWED and ALLOWED isn't
> obvious to me (before looking at the code that implements them).

Done.

https://codereview.chromium.org/120513005/diff/1670001/content/browser/render...
content/browser/renderer_host/input/gesture_event_queue.h:81: bool
outstanding_tap_;
On 2014/01/23 20:45:29, Rick Byers wrote:
> Can you add a comment for these two making it clear that they're about what's
> outstanding on the output side of this buffer, not the input side?  To me
> 'outstanding' by itself ambiguous in this regard.

bool fantastic_fling_? ;) Done.

https://codereview.chromium.org/120513005/diff/1670001/content/browser/render...
File content/browser/renderer_host/input/gesture_event_queue_unittest.cc
(right):

https://codereview.chromium.org/120513005/diff/1670001/content/browser/render...
content/browser/renderer_host/input/gesture_event_queue_unittest.cc:308:
EXPECT_EQ(WebInputEvent::GestureScrollBegin, sent_gesture().type);
On 2014/01/23 20:45:29, Rick Byers wrote:
> The other expected gesture is a TapCancel, but that's not actually validated
> anywhere right?
> 
> I think it's worth while to extend the test infrastructure to be able to
easily
> EXPECT that the gestures are a particular sequence.  The simplest way to do
this
> is to have a function that just returns the list of gesture types received as
a
> string, and then EXPECT_EQ on that.  Or you could go more fancy and build an
> EXPECT_TYPE_LIST macro which takes a var-arg number of input event types or
> something.  Tim did something like this a long time ago in some new Aura GR
unit
> tests (that ended up getting forgotten after his intership ended), and it
turned
> out really nice.  

Yeah, I should do this right.  Good catch.

[aelias_OOO_until_Jul13, 2014-01-24 05:35:53]

lgtm modulo minor comments below.

https://codereview.chromium.org/120513005/diff/1800002/content/browser/render...
File content/browser/renderer_host/input/gesture_event_packet.h (right):

https://codereview.chromium.org/120513005/diff/1800002/content/browser/render...
content/browser/renderer_host/input/gesture_event_packet.h:22:
GESTURE_SOURCE_DEFAULT = TOUCH
I'd prefer to get rid of this and the default constructor.  If the default
constructor is truly needed for some technical reasons, I'd prefer using an
"INVALID" entry in the enum instead.

https://codereview.chromium.org/120513005/diff/1800002/content/browser/render...
File content/browser/renderer_host/input/gesture_event_queue.cc (right):

https://codereview.chromium.org/120513005/diff/1800002/content/browser/render...
content/browser/renderer_host/input/gesture_event_queue.cc:62: // Handle the
synthetic packet immediately if no other packets pending.
Looks like this was already discussed to some degree, but I'm also a bit
suspicious of the different handling for synthetic input events.  It seems like
test stuff leaking into production code and it might affect perf
characteristics.  Is there some way to use GestureEventPacket::TOUCH for these
and maybe change the synthetic gesture code to be more similar to a normal
sequence?

[tdresser, 2014-01-24 13:47:29]

I'll want to do some additional manual testing once the current issue with
touches not being cancelled is sorted out. Other than that, I'm happy with this.

https://codereview.chromium.org/120513005/diff/1670001/content/browser/render...
File content/browser/renderer_host/input/gesture_event_queue.cc (right):

https://codereview.chromium.org/120513005/diff/1670001/content/browser/render...
content/browser/renderer_host/input/gesture_event_queue.cc:74: void
GestureEventQueue::OnTouchEventAck(InputEventAckState ack_state) {
On 2014/01/23 22:52:34, jdduke wrote:
> On 2014/01/23 16:32:43, tdresser wrote:
> > A DCHECK(!sequences.empty()) might be good here.
> 
> Sure. The |Head()| call performs that check, but I can be more explicit if you
> like, same below, your call.

Oh, good point. In that case I have no strong feeling either way.

https://codereview.chromium.org/120513005/diff/1670001/content/browser/render...
content/browser/renderer_host/input/gesture_event_queue.cc:171: // |NO_CONSUMER|
should only be effective when the *first* touch is ack'ed.
On 2014/01/23 22:52:34, jdduke wrote:
> On 2014/01/23 16:32:43, tdresser wrote:
> > If we get an INPUT_EVENT_ACK_STATE_NO_CONSUMER_EXISTS, shouldn't all future
> acks
> > in the sequence be INPUT_EVENT_ACK_STATE_NOT_CONSUMED?
> 
> If the primary pointer press was NOT_CONSUMED, but remains stationary and the
> secondary pointer press is NO_CONSUMER, there's still the possibility that the
> primary pointer will then move and be CONSUMED. This was first reported in
> crbug/135102.

That's unfortunate. Looks good then.

https://codereview.chromium.org/120513005/diff/1890001/content/browser/render...
File content/browser/renderer_host/input/gesture_event_queue_unittest.cc
(right):

https://codereview.chromium.org/120513005/diff/1890001/content/browser/render...
content/browser/renderer_host/input/gesture_event_queue_unittest.cc:6: #include
"base/logging.h"
Remove unnecessary includes.

https://codereview.chromium.org/120513005/diff/1890001/content/browser/render...
content/browser/renderer_host/input/gesture_event_queue_unittest.cc:150: void
MoveTouchPoints(int index0, int x0, int y0, int index1, int x1, int y1) {
Unused, and probably unnecessary.

https://codereview.chromium.org/120513005/diff/1890001/content/browser/render...
content/browser/renderer_host/input/gesture_event_queue_unittest.cc:182: void
PushGestureImpl(const WebGestureEvent& gesture) {
Do we need a separate method for this?

https://codereview.chromium.org/120513005/diff/1890001/content/browser/render...
content/browser/renderer_host/input/gesture_event_queue_unittest.cc:247: //
Event if the subsequent touch is not consumed, continue dropping gestures.
Event if -> Even if?

https://codereview.chromium.org/120513005/diff/1890001/content/browser/render...
content/browser/renderer_host/input/gesture_event_queue_unittest.cc:253: //
Event if the subsequent touch had no consumer, continue dropping gestures.
As above.

[jdduke (slow), 2014-01-24 18:17:34]

https://codereview.chromium.org/120513005/diff/1800002/content/browser/render...
File content/browser/renderer_host/input/gesture_event_packet.h (right):

https://codereview.chromium.org/120513005/diff/1800002/content/browser/render...
content/browser/renderer_host/input/gesture_event_packet.h:22:
GESTURE_SOURCE_DEFAULT = TOUCH
On 2014/01/24 05:35:54, aelias wrote:
> I'd prefer to get rid of this and the default constructor.  If the default
> constructor is truly needed for some technical reasons, I'd prefer using an
> "INVALID" entry in the enum instead.

Done.

https://codereview.chromium.org/120513005/diff/1800002/content/browser/render...
File content/browser/renderer_host/input/gesture_event_queue.cc (right):

https://codereview.chromium.org/120513005/diff/1800002/content/browser/render...
content/browser/renderer_host/input/gesture_event_queue.cc:62: // Handle the
synthetic packet immediately if no other packets pending.
On 2014/01/24 05:35:54, aelias wrote:
> Looks like this was already discussed to some degree, but I'm also a bit
> suspicious of the different handling for synthetic input events.  It seems
like
> test stuff leaking into production code and it might affect perf
> characteristics.  Is there some way to use GestureEventPacket::TOUCH for these
> and maybe change the synthetic gesture code to be more similar to a normal
> sequence?

Yeah, it was a mistake to include synthetic gestures here... the burden should
remain on the caller to inject synthetic gestures in the proper sequence
relative to a touch stream, and so shall it remain.

https://codereview.chromium.org/120513005/diff/1890001/content/browser/render...
File content/browser/renderer_host/input/gesture_event_queue_unittest.cc
(right):

https://codereview.chromium.org/120513005/diff/1890001/content/browser/render...
content/browser/renderer_host/input/gesture_event_queue_unittest.cc:6: #include
"base/logging.h"
On 2014/01/24 13:47:30, tdresser wrote:
> Remove unnecessary includes.

Done.

https://codereview.chromium.org/120513005/diff/1890001/content/browser/render...
content/browser/renderer_host/input/gesture_event_queue_unittest.cc:150: void
MoveTouchPoints(int index0, int x0, int y0, int index1, int x1, int y1) {
On 2014/01/24 13:47:30, tdresser wrote:
> Unused, and probably unnecessary.

Done.

https://codereview.chromium.org/120513005/diff/1890001/content/browser/render...
content/browser/renderer_host/input/gesture_event_queue_unittest.cc:182: void
PushGestureImpl(const WebGestureEvent& gesture) {
On 2014/01/24 13:47:30, tdresser wrote:
> Do we need a separate method for this?

Absolutely not.

https://codereview.chromium.org/120513005/diff/1890001/content/browser/render...
content/browser/renderer_host/input/gesture_event_queue_unittest.cc:247: //
Event if the subsequent touch is not consumed, continue dropping gestures.
On 2014/01/24 13:47:30, tdresser wrote:
> Event if -> Even if?

Done.

https://codereview.chromium.org/120513005/diff/1890001/content/browser/render...
content/browser/renderer_host/input/gesture_event_queue_unittest.cc:253: //
Event if the subsequent touch had no consumer, continue dropping gestures.
On 2014/01/24 13:47:30, tdresser wrote:
> As above.

Done.

[tdresser, 2014-01-24 19:30:14]

LGTM

https://codereview.chromium.org/120513005/diff/1980001/content/browser/render...
File content/browser/renderer_host/input/gesture_event_queue_unittest.cc
(right):

https://codereview.chromium.org/120513005/diff/1980001/content/browser/render...
content/browser/renderer_host/input/gesture_event_queue_unittest.cc:447: }
I'd like to see a test for the scroll after longpress case.

[jdduke (slow), 2014-01-24 23:00:55]

avi@: owner review for content/port/browser/render_widget_host_view_port.h?
Thanks.

https://codereview.chromium.org/120513005/diff/1980001/content/browser/render...
File content/browser/renderer_host/input/gesture_event_queue_unittest.cc
(right):

https://codereview.chromium.org/120513005/diff/1980001/content/browser/render...
content/browser/renderer_host/input/gesture_event_queue_unittest.cc:447: }
On 2014/01/24 19:30:15, tdresser wrote:
> I'd like to see a test for the scroll after longpress case.

Sure.  I added a specific test for this case in ContentViewGestureHandlerTest,
but I think it would be a good addition to have a TOUCH after TOUCH_TIMEOUT
packet test.

[Avi (use Gerrit), 2014-01-25 02:16:41]

content/port/browser/render_widget_host_view_port.h LGTM

[Rick Byers, 2014-01-25 14:00:40]

content/browser/renderer_host/input LGTM

https://codereview.chromium.org/120513005/diff/1670001/content/browser/render...
File content/browser/renderer_host/input/gesture_event_queue_unittest.cc
(right):

https://codereview.chromium.org/120513005/diff/1670001/content/browser/render...
content/browser/renderer_host/input/gesture_event_queue_unittest.cc:308:
EXPECT_EQ(WebInputEvent::GestureScrollBegin, sent_gesture().type);
On 2014/01/24 05:34:40, jdduke wrote:
> On 2014/01/23 20:45:29, Rick Byers wrote:
> > The other expected gesture is a TapCancel, but that's not actually validated
> > anywhere right?
> > 
> > I think it's worth while to extend the test infrastructure to be able to
> easily
> > EXPECT that the gestures are a particular sequence.  The simplest way to do
> this
> > is to have a function that just returns the list of gesture types received
as
> a
> > string, and then EXPECT_EQ on that.  Or you could go more fancy and build an
> > EXPECT_TYPE_LIST macro which takes a var-arg number of input event types or
> > something.  Tim did something like this a long time ago in some new Aura GR
> unit
> > tests (that ended up getting forgotten after his intership ended), and it
> turned
> > out really nice.  
> 
> Yeah, I should do this right.  Good catch.

Thanks for putting the extra time into this, I like it a LOT better now!

[jdduke (slow), 2014-01-28 00:34:07]

mkosiba@: Review for android_webview/? Thanks.

[mkosiba (inactive), 2014-01-28 23:47:01]

lgtm

[jdduke (slow), 2014-01-29 16:42:58]

tedchoc@: Could you take a look at ContentViewCore.java? I'd love another set of
eyes on it... Thank you sir.

[Ted C, 2014-01-30 02:29:47]

lgtm w/ nits...(scariest change I've seen in a long time)

https://codereview.chromium.org/120513005/diff/2420001/content/browser/androi...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/120513005/diff/2420001/content/browser/androi...
content/browser/android/content_view_core_impl.cc:398: env, j_obj.obj(),
should only be indented 4 right?

https://codereview.chromium.org/120513005/diff/2420001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/120513005/diff/2420001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:486:
static final int INPUT_EVENT_ACK_STATE_UNKNOWN = 0;
Any chance we could convert the shared constants to use the template files to
generate java constants and c++ constants instead of having to manually keep
them in sync?

i.e.
./content/public/android/java/src/org/chromium/content/common/ResultCodes.template

using ../build/android/java_cpp_template.gypi

https://codereview.chromium.org/120513005/diff/2420001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1988:
&& type != ContentViewGestureHandler.GESTURE_LONG_TAP) return;
if the conditional and statement don't all fit on one line, then braces are
needed

https://codereview.chromium.org/120513005/diff/2420001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java
(right):

https://codereview.chromium.org/120513005/diff/2420001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:149:
static final int GESTURE_SHOW_PRESS = 0;
same comment about sharing constants as my previous one

https://codereview.chromium.org/120513005/diff/2420001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:582:
private boolean ignoreDetectorEvents() {
why not just use getPermanentlyIgnoreDetectorEvents() above?

https://codereview.chromium.org/120513005/diff/2420001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:589:
//if (Build.VERSION.SDK_INT < Build.VERSION_CODES.KITKAT) return;
Doesn't this need to be uncommented?

https://codereview.chromium.org/120513005/diff/2420001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:826:
// TODO: Need to deal with multi-touch transition
TODO(jdduke):

https://codereview.chromium.org/120513005/diff/2420001/content/public/android...
File
content/public/android/javatests/src/org/chromium/content/browser/ContentViewGestureHandlerTest.java
(right):

https://codereview.chromium.org/120513005/diff/2420001/content/public/android...
content/public/android/javatests/src/org/chromium/content/browser/ContentViewGestureHandlerTest.java:97:
if (type == ContentViewGestureHandler.GESTURE_SCROLL_START)
since not all fit on one line, need to add braces

[jdduke (slow), 2014-01-30 02:38:07]

On 2014/01/30 02:29:47, Ted C wrote:
> lgtm w/ nits...(scariest change I've seen in a long time)

I'll take that as a compliment ;) (but seriously, this patch was one of those
"down the rabbit hole" untanglings, and I'll be happy to wash my hands of it...)

I'll address nits tomorrow, thanks for review.

[jdduke (slow), 2014-01-30 18:46:22]

https://codereview.chromium.org/120513005/diff/2420001/content/browser/androi...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/120513005/diff/2420001/content/browser/androi...
content/browser/android/content_view_core_impl.cc:398: env, j_obj.obj(),
On 2014/01/30 02:29:48, Ted C wrote:
> should only be indented 4 right?

Done.

https://codereview.chromium.org/120513005/diff/2420001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/120513005/diff/2420001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:486:
static final int INPUT_EVENT_ACK_STATE_UNKNOWN = 0;
On 2014/01/30 02:29:48, Ted C wrote:
> Any chance we could convert the shared constants to use the template files to
> generate java constants and c++ constants instead of having to manually keep
> them in sync?
> 
> i.e.
>
./content/public/android/java/src/org/chromium/content/common/ResultCodes.template
> 
> using ../build/android/java_cpp_template.gypi

Hmm, we may not actually need them, I've reworked those bits somewhat.

https://codereview.chromium.org/120513005/diff/2420001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1988:
&& type != ContentViewGestureHandler.GESTURE_LONG_TAP) return;
On 2014/01/30 02:29:48, Ted C wrote:
> if the conditional and statement don't all fit on one line, then braces are
> needed

Done.

https://codereview.chromium.org/120513005/diff/2420001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java
(right):

https://codereview.chromium.org/120513005/diff/2420001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:149:
static final int GESTURE_SHOW_PRESS = 0;
On 2014/01/30 02:29:48, Ted C wrote:
> same comment about sharing constants as my previous one

Yeah, this is a bit more difficult as blink owns the enum. However, I suppose we
could register the constants, as is done for TouchPoint.java? I'll file a bug
and address that in a follow-up patch.

https://codereview.chromium.org/120513005/diff/2420001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:582:
private boolean ignoreDetectorEvents() {
On 2014/01/30 02:29:48, Ted C wrote:
> why not just use getPermanentlyIgnoreDetectorEvents() above?

Good point, we no longer have the "temporarily ignore" value so there's no need
for this.

https://codereview.chromium.org/120513005/diff/2420001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:589:
//if (Build.VERSION.SDK_INT < Build.VERSION_CODES.KITKAT) return;
On 2014/01/30 02:29:48, Ted C wrote:
> Doesn't this need to be uncommented?

Oops, yes! Good catch, I had disabled for testing...

https://codereview.chromium.org/120513005/diff/2420001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:826:
// TODO: Need to deal with multi-touch transition
On 2014/01/30 02:29:48, Ted C wrote:
> TODO(jdduke): 

Done.

https://codereview.chromium.org/120513005/diff/2420001/content/public/android...
File
content/public/android/javatests/src/org/chromium/content/browser/ContentViewGestureHandlerTest.java
(right):

https://codereview.chromium.org/120513005/diff/2420001/content/public/android...
content/public/android/javatests/src/org/chromium/content/browser/ContentViewGestureHandlerTest.java:97:
if (type == ContentViewGestureHandler.GESTURE_SCROLL_START)
On 2014/01/30 02:29:48, Ted C wrote:
> since not all fit on one line, need to add braces

Done.

[commit-bot: I haz the power, 2014-01-30 18:47:37]

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

[commit-bot: I haz the power, 2014-01-30 19:13:32]

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

[commit-bot: I haz the power, 2014-01-30 19:13:33]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-01-30 19:33:45]

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

[commit-bot: I haz the power, 2014-01-30 19:51:28]

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

[commit-bot: I haz the power, 2014-01-30 19:51:28]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-01-30 20:07:36]

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

[commit-bot: I haz the power, 2014-01-30 20:23:31]

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

[commit-bot: I haz the power, 2014-01-30 20:23:41]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-01-30 22:44:58]

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

[commit-bot: I haz the power, 2014-01-30 22:48:53]

Change committed as 248066

[commit-bot: I haz the power, 2014-01-30 22:49:02]

CQ bit was unchecked on CL. Ignoring.