Prevent partial touch sequences from reaching the renderer

Prevent partial touch sequences from reaching the renderer

Consolidate the various state-related members of TouchEventQueue, instead
relying on but one state variable to determine touch dispatch. In addition,
support touch forwarding suppression per-touch sequence. This prevents cases
where a partial touch sequence might be forwarded, e.g., if a touch handler is
added in the middle of a gesture, or a scroll-ending event is received in the
middle of a gesture.

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

[jdduke (slow), 2014-01-17 02:20:23]

tdresser@: PTAL.  This completes the transfer of touch disposition handling
logic from ContentViewGestureHandler.  Hopefully these are the final bits we
need here for Android to switch to eager GR.

[tdresser, 2014-01-17 14:33:40]

https://codereview.chromium.org/130993003/diff/60001/content/browser/renderer...
File content/browser/renderer_host/input/touch_event_queue.cc (right):

https://codereview.chromium.org/130993003/diff/60001/content/browser/renderer...
content/browser/renderer_host/input/touch_event_queue.cc:345: // We assume the
scroll event are generated synchronously from
While you're here, wouldn't hurt to fix "scroll event are" -> "scroll events
are".

https://codereview.chromium.org/130993003/diff/60001/content/browser/renderer...
File content/browser/renderer_host/input/touch_event_queue_unittest.cc (right):

https://codereview.chromium.org/130993003/diff/60001/content/browser/renderer...
content/browser/renderer_host/input/touch_event_queue_unittest.cc:153: bool
WillForwardTouchEvents() {
Something's wrong here...

[jdduke (slow), 2014-01-17 18:29:07]

https://codereview.chromium.org/130993003/diff/60001/content/browser/renderer...
File content/browser/renderer_host/input/touch_event_queue.cc (right):

https://codereview.chromium.org/130993003/diff/60001/content/browser/renderer...
content/browser/renderer_host/input/touch_event_queue.cc:345: // We assume the
scroll event are generated synchronously from
On 2014/01/17 14:33:40, tdresser wrote:
> While you're here, wouldn't hurt to fix "scroll event are" -> "scroll events
> are".

Done.

https://codereview.chromium.org/130993003/diff/60001/content/browser/renderer...
File content/browser/renderer_host/input/touch_event_queue_unittest.cc (right):

https://codereview.chromium.org/130993003/diff/60001/content/browser/renderer...
content/browser/renderer_host/input/touch_event_queue_unittest.cc:153: bool
WillForwardTouchEvents() {
On 2014/01/17 14:33:40, tdresser wrote:
> Something's wrong here...

lol, yeah this whole method needs to die... We should be testing visible state,
not internal bookkeeping.  I'll make the necessary changes.

[Rick Byers, 2014-01-21 22:00:24]

https://codereview.chromium.org/130993003/diff/220001/content/browser/rendere...
File content/browser/renderer_host/input/touch_event_queue.cc (right):

https://codereview.chromium.org/130993003/diff/220001/content/browser/rendere...
content/browser/renderer_host/input/touch_event_queue.cc:255: if
(touch_handling_state_ == NO_TOUCH_HANDLER) {
Shouldn't we be able to handle the NO_TOUCH_HANDLER_FOR_GESTURE state in the
same way?

https://codereview.chromium.org/130993003/diff/220001/content/browser/rendere...
content/browser/renderer_host/input/touch_event_queue.cc:293:
touch_handling_state_ = RENDERER_CONSUMING_GESTURE;
can we DCHECK that we're in the HAS_TOUCH_HANDLER state?  Eg. in my description
we're transitioning from FORWARD_TOUCHES_UNTIL_TIMEOUT to FORWARD_ALL_TOUCHES.

https://codereview.chromium.org/130993003/diff/220001/content/browser/rendere...
content/browser/renderer_host/input/touch_event_queue.cc:382:
touch_handling_state_ = NO_TOUCH_HANDLER_FOR_GESTURE;
Your comment for this state says that no events are sent for the duration of the
sequence.  Won't this cause exactly the 'partial gesture' state you're trying to
avoid?  Do we need a new state for sending all events in the current sequence
and then reverting to NO_TOUCH_HANDLER at the end of the sequence?

https://codereview.chromium.org/130993003/diff/220001/content/browser/rendere...
File content/browser/renderer_host/input/touch_event_queue.h (right):

https://codereview.chromium.org/130993003/diff/220001/content/browser/rendere...
content/browser/renderer_host/input/touch_event_queue.h:136: // Determines touch
event forwarding decisions.
I like collapsing the bits into a single enum!  I think we should try to be
clear on TEQ's role here though.  In particular, I think of it as part of the
low-level of the platform, below gestures which are at a higher conceptual
level.  So I don't think we should be even mentioning "gesture" here.

Can we think of this more as a touch event filter optimization?  Eg. something
like (and this is probably mostly naming changes rather than semantic changes):

// Determines which touch events we can safely avoid sending
// to the renderer as a performance optimization.
enum TouchFilteringState {
  FORWARD_ALL_TOUCHES,    // Don't filter at all - the default
  FORWARD_TOUCHES_UNTIL_TIMEOUT,  // Don't filter unless we get an ACK timeout
  DROP_TOUCHES_IN_SEQUENCE,  // Filter all events until all active touch points
are released, then revert to sending everything.
  DROP_ALL_TOUCHES,     // Filter everything - eg. no touch handlers
}

In particular, this feels less coupled to the idea that a gesture is either
consumed or not (just thinking of how I'd change this for my experimental touch
scroll modes - https://codereview.chromium.org/140253005/, one of which we'll
probably land eventually).

[jdduke (slow), 2014-01-22 02:46:59]

Thanks for review.  The more input from those that will be needing to tweak this
in the future, the better :)

https://codereview.chromium.org/130993003/diff/220001/content/browser/rendere...
File content/browser/renderer_host/input/touch_event_queue.cc (right):

https://codereview.chromium.org/130993003/diff/220001/content/browser/rendere...
content/browser/renderer_host/input/touch_event_queue.cc:255: if
(touch_handling_state_ == NO_TOUCH_HANDLER) {
On 2014/01/21 22:00:25, Rick Byers wrote:
> Shouldn't we be able to handle the NO_TOUCH_HANDLER_FOR_GESTURE state in the
> same way?

Hmm, we'd have to check if it's a new touch sequence, but yeah we could do that.

https://codereview.chromium.org/130993003/diff/220001/content/browser/rendere...
content/browser/renderer_host/input/touch_event_queue.cc:293:
touch_handling_state_ = RENDERER_CONSUMING_GESTURE;
On 2014/01/21 22:00:25, Rick Byers wrote:
> can we DCHECK that we're in the HAS_TOUCH_HANDLER state?  Eg. in my
description
> we're transitioning from FORWARD_TOUCHES_UNTIL_TIMEOUT to FORWARD_ALL_TOUCHES.

I'm trying to think if we could ever get an ack if there were no handler... At
the least, I should be checking |touch_filtering_state_ ==
FORWARD_TOUCHES_UNTIL_TIMEOUT|.

https://codereview.chromium.org/130993003/diff/220001/content/browser/rendere...
content/browser/renderer_host/input/touch_event_queue.cc:382:
touch_handling_state_ = NO_TOUCH_HANDLER_FOR_GESTURE;
On 2014/01/21 22:00:25, Rick Byers wrote:
> Your comment for this state says that no events are sent for the duration of
the
> sequence.  Won't this cause exactly the 'partial gesture' state you're trying
to
> avoid?  Do we need a new state for sending all events in the current sequence
> and then reverting to NO_TOUCH_HANDLER at the end of the sequence?

Sorry, by "partial gesture" I really mean "partial touch sequence".  I'll update
the verbiage.

https://codereview.chromium.org/130993003/diff/220001/content/browser/rendere...
File content/browser/renderer_host/input/touch_event_queue.h (right):

https://codereview.chromium.org/130993003/diff/220001/content/browser/rendere...
content/browser/renderer_host/input/touch_event_queue.h:136: // Determines touch
event forwarding decisions.
On 2014/01/21 22:00:25, Rick Byers wrote:
> I like collapsing the bits into a single enum!  I think we should try to be
> clear on TEQ's role here though.  In particular, I think of it as part of the
> low-level of the platform, below gestures which are at a higher conceptual
> level.  So I don't think we should be even mentioning "gesture" here.
> 
> Can we think of this more as a touch event filter optimization?  Eg. something
> like (and this is probably mostly naming changes rather than semantic
changes):
> 
> // Determines which touch events we can safely avoid sending
> // to the renderer as a performance optimization.
> enum TouchFilteringState {
>   FORWARD_ALL_TOUCHES,    // Don't filter at all - the default
>   FORWARD_TOUCHES_UNTIL_TIMEOUT,  // Don't filter unless we get an ACK timeout
>   DROP_TOUCHES_IN_SEQUENCE,  // Filter all events until all active touch
points
> are released, then revert to sending everything.
>   DROP_ALL_TOUCHES,     // Filter everything - eg. no touch handlers
> }
> 
> In particular, this feels less coupled to the idea that a gesture is either
> consumed or not (just thinking of how I'd change this for my experimental
touch
> scroll modes - https://codereview.chromium.org/140253005/, one of which we'll
> probably land eventually).

That sounds just fine :) And I agree, using the term gesture here just leads to
confusion.  Too much borrowing of terms from ContentViewGestureHandler...

[jdduke (slow), 2014-01-23 16:35:41]

rbyers@: Anything else you'd like to see in this patch? If you think this kind
of transformation will make experimentation more difficult in the future, then
I'd be happy to scrap this entirely.

[tdresser, 2014-01-23 18:26:17]

LGTM

[Rick Byers, 2014-01-23 18:40:13]

I think this is a good change overall, thanks Jared!
LGTM.

Do you think I should try to land https://codereview.chromium.org/140253005/ to
make it easier for others to experiment with it and avoid endless rebaselining
headaches?

https://codereview.chromium.org/130993003/diff/330002/content/browser/rendere...
File content/browser/renderer_host/input/touch_event_queue.cc (right):

https://codereview.chromium.org/130993003/diff/330002/content/browser/rendere...
content/browser/renderer_host/input/touch_event_queue.cc:383:
touch_filtering_state_ = DROP_ALL_TOUCHES;
I had to think about this for a minute to convince myself that you don't need
the "TODO send touchcancel" anymore.  Can you add a comment saying (I think)
something like: Since there are no handlers, there's no need to send any
remaining events in the current sequence (they'd just be ignored anyway).

I'm actually not 100% convinced that this is OK - eg. somethin in blink may
expect consistent begin/end pairs, but I'm not aware of anything offhand and I
think the risk is low.

[jdduke (slow), 2014-01-23 22:10:44]

On 2014/01/23 18:40:13, Rick Byers wrote:
> I think this is a good change overall, thanks Jared!
> LGTM.
> 
> Do you think I should try to land https://codereview.chromium.org/140253005/
to
> make it easier for others to experiment with it and avoid endless rebaselining
> headaches?
> 

It's up to you.  If you think those flags will be useful to other developers, it
could be worth it.

[jdduke (slow), 2014-01-24 05:29:21]

https://codereview.chromium.org/130993003/diff/330002/content/browser/rendere...
File content/browser/renderer_host/input/touch_event_queue.cc (right):

https://codereview.chromium.org/130993003/diff/330002/content/browser/rendere...
content/browser/renderer_host/input/touch_event_queue.cc:383:
touch_filtering_state_ = DROP_ALL_TOUCHES;
On 2014/01/23 18:40:14, Rick Byers wrote:
> I had to think about this for a minute to convince myself that you don't need
> the "TODO send touchcancel" anymore.  Can you add a comment saying (I think)
> something like: Since there are no handlers, there's no need to send any
> remaining events in the current sequence (they'd just be ignored anyway).
> 
> I'm actually not 100% convinced that this is OK - eg. somethin in blink may
> expect consistent begin/end pairs, but I'm not aware of anything offhand and I
> think the risk is low.

Ahh, I actually never intended to remove that TODO.  I'll put it back in.

[commit-bot: I haz the power, 2014-01-24 05:32:04]

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

[commit-bot: I haz the power, 2014-01-24 09:26:58]

Retried try job too often on win_rel for step(s) browser_tests,
browser_tests_swarm
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2014-01-24 13:04:22]

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

[commit-bot: I haz the power, 2014-01-24 15:21:48]

Change committed as 246893