Always reset touch action at the beginning of a new gesture sequence

Always reset touch action at the beginning of a new gesture sequence

The InputRouter now tells the TouchActionFilter when a new gesture
sequence is starting, and the allowed_touch_action_ is set to auto.

BUG=344536
TESTS=TouchActionFilterTest.TouchActionResetsOnNewTouchActionSequence

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

[tdresser, 2014-02-18 21:23:09]

rbyers@, can you take a first look at this?

[Rick Byers, 2014-02-18 22:01:15]

https://codereview.chromium.org/171283002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/input/input_router_impl.cc (right):

https://codereview.chromium.org/171283002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/input/input_router_impl.cc:262: if
(WebInputEventTraits::IsNewTouchSequence(touch_event.event))
Doesn't this need to go in SendTouchEventImmediately instead?  If the renderer
is busy and we're queuing up multiple touch sequences in the TEQ, then we may
still be processing gestures associated with the _previous_ sequence.

Can you look into adding a unit test for this sort of timing issue?  I think
it's probably the most subtle/brittle aspect of touch-action behavior.  Perhaps
an input-router unit test with a mock TEQ but real TouchActionFilter?

https://codereview.chromium.org/171283002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/input/touch_action_filter.cc (right):

https://codereview.chromium.org/171283002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/input/touch_action_filter.cc:117:
allowed_touch_action_ = TOUCH_ACTION_AUTO;
I think you can DCHECK that our other state is consistent with the idea that
we're starting a new gesture sequence (i.e. !drop_scroll_gesture_events and
!drop_pinch_gesture_events), right?  Delayed taps is, as we've discussed,
trickier and probably subtly broken today.

https://codereview.chromium.org/171283002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/input/touch_action_filter.h (right):

https://codereview.chromium.org/171283002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/input/touch_action_filter.h:34: void
StartNewTouchActionSequence();
Add comment please, including mentioning the relative order with other things
(critically - that this is called BEFORE the first touchstart is sent to the
renderer).

I think "TouchActionSequence" doesn't have a well defined meaning.  How about
"OnStartNewTouchSequence"?

Have you thought about the potential for other races here in the context of
eager GR?  I think it's OK, but we should add a comment somewhere explaining why
the start of a new touch sequence is guaranteed to be the start of a new gesture
sequence (maybe even name this function 'OnStartNewGestureSequence' to make it
clear?).

https://codereview.chromium.org/171283002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/input/touch_action_filter_unittest.cc
(right):

https://codereview.chromium.org/171283002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/input/touch_action_filter_unittest.cc:61: // After
the end of a gesture the state is reset.
update comment

https://codereview.chromium.org/171283002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/input/touch_action_filter_unittest.cc:114: //
After a fling the state is reset.
update comment (maybe even remove the test since it's explicit now and really
covered by your new test?)

https://codereview.chromium.org/171283002/diff/1/content/common/input/web_inp...
File content/common/input/web_input_event_traits.cc (right):

https://codereview.chromium.org/171283002/diff/1/content/common/input/web_inp...
content/common/input/web_input_event_traits.cc:345: if (!event.touchesLength)
This case should never happen, right?  Maybe DHCECK (and leave the return false
in if you're worried about it)?

[tdresser, 2014-02-20 18:37:56]

https://codereview.chromium.org/171283002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/input/input_router_impl.cc (right):

https://codereview.chromium.org/171283002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/input/input_router_impl.cc:262: if
(WebInputEventTraits::IsNewTouchSequence(touch_event.event))
On 2014/02/18 22:01:15, Rick Byers wrote:
> Doesn't this need to go in SendTouchEventImmediately instead?  If the renderer
> is busy and we're queuing up multiple touch sequences in the TEQ, then we may
> still be processing gestures associated with the _previous_ sequence.
> 
> Can you look into adding a unit test for this sort of timing issue?  I think
> it's probably the most subtle/brittle aspect of touch-action behavior. 
Perhaps
> an input-router unit test with a mock TEQ but real TouchActionFilter?

Done.

https://codereview.chromium.org/171283002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/input/touch_action_filter.cc (right):

https://codereview.chromium.org/171283002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/input/touch_action_filter.cc:117:
allowed_touch_action_ = TOUCH_ACTION_AUTO;
On 2014/02/18 22:01:15, Rick Byers wrote:
> I think you can DCHECK that our other state is consistent with the idea that
> we're starting a new gesture sequence (i.e. !drop_scroll_gesture_events and
> !drop_pinch_gesture_events), right?  Delayed taps is, as we've discussed,
> trickier and probably subtly broken today.

Done. I DCHECK(!drop_current_tap_ending_event) when we get a
WebInputEvent::GestureTapDown, so we at least have some coverage on that.

https://codereview.chromium.org/171283002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/input/touch_action_filter.h (right):

https://codereview.chromium.org/171283002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/input/touch_action_filter.h:34: void
StartNewTouchActionSequence();
On 2014/02/18 22:01:15, Rick Byers wrote:
> Add comment please, including mentioning the relative order with other things
> (critically - that this is called BEFORE the first touchstart is sent to the
> renderer).
> 
> I think "TouchActionSequence" doesn't have a well defined meaning.  How about
> "OnStartNewTouchSequence"?
> 
> Have you thought about the potential for other races here in the context of
> eager GR?  I think it's OK, but we should add a comment somewhere explaining
why
> the start of a new touch sequence is guaranteed to be the start of a new
gesture
> sequence (maybe even name this function 'OnStartNewGestureSequence' to make it
> clear?).

Done.

https://codereview.chromium.org/171283002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/input/touch_action_filter_unittest.cc
(right):

https://codereview.chromium.org/171283002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/input/touch_action_filter_unittest.cc:61: // After
the end of a gesture the state is reset.
On 2014/02/18 22:01:15, Rick Byers wrote:
> update comment

Done.

https://codereview.chromium.org/171283002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/input/touch_action_filter_unittest.cc:114: //
After a fling the state is reset.
On 2014/02/18 22:01:15, Rick Byers wrote:
> update comment (maybe even remove the test since it's explicit now and really
> covered by your new test?)

I think the test still provides some value - I've removed the bit that was
testing the state reset though. Done.

https://codereview.chromium.org/171283002/diff/1/content/common/input/web_inp...
File content/common/input/web_input_event_traits.cc (right):

https://codereview.chromium.org/171283002/diff/1/content/common/input/web_inp...
content/common/input/web_input_event_traits.cc:345: if (!event.touchesLength)
On 2014/02/18 22:01:15, Rick Byers wrote:
> This case should never happen, right?  Maybe DHCECK (and leave the return
false
> in if you're worried about it)?

Done.

[Rick Byers, 2014-02-21 03:06:46]

lgtm

[tdresser, 2014-02-21 14:20:52]

Jared, can you take a look?

[jdduke (slow), 2014-02-21 15:45:32]

https://codereview.chromium.org/171283002/diff/240001/content/browser/rendere...
File content/browser/renderer_host/input/input_router_impl.cc (right):

https://codereview.chromium.org/171283002/diff/240001/content/browser/rendere...
content/browser/renderer_host/input/input_router_impl.cc:289: if
(WebInputEventTraits::IsNewTouchSequence(touch_event.event))
Hmm, what if the touch handler is removed during a touch sequence?  On the next
touch sequence, we won't forward any touches, but we'll still have the touch
action from the previous sequence?

https://codereview.chromium.org/171283002/diff/240001/content/common/input/we...
File content/common/input/web_input_event_traits.h (right):

https://codereview.chromium.org/171283002/diff/240001/content/common/input/we...
content/common/input/web_input_event_traits.h:26: static bool
IsNewTouchSequence(const blink::WebTouchEvent& event);
I wonder if it's worth having a WebTouchEventTraits in
content/renderer_host/input where we can add perhaps a few additional queries
(IsTouchSequenceEnd()? IsValidTouchEvent()?).  We could move it to common should
the need arise to use it elsewhere?

Also, we should be consistent with the GestureEventPacket naming here, e.g.,
TouchSequenceBegin, or NewTouchSequence, but not both... Perhaps a better name
is |TouchSequence{Start,End}| to parallel Touch{Start,End}.

[jdduke (slow), 2014-02-21 15:50:09]

https://codereview.chromium.org/171283002/diff/240001/content/browser/rendere...
File content/browser/renderer_host/input/input_router_impl.cc (right):

https://codereview.chromium.org/171283002/diff/240001/content/browser/rendere...
content/browser/renderer_host/input/input_router_impl.cc:289: if
(WebInputEventTraits::IsNewTouchSequence(touch_event.event))
On 2014/02/21 15:45:33, jdduke wrote:
> Hmm, what if the touch handler is removed during a touch sequence?  On the
next
> touch sequence, we won't forward any touches, but we'll still have the touch
> action from the previous sequence?

Hmm, we should probably have |OnHasTouchEventHandlers()| update
TouchActionFilter in some fashion regardless of this change?

[tdresser, 2014-02-21 19:14:03]

https://codereview.chromium.org/171283002/diff/240001/content/browser/rendere...
File content/browser/renderer_host/input/input_router_impl.cc (right):

https://codereview.chromium.org/171283002/diff/240001/content/browser/rendere...
content/browser/renderer_host/input/input_router_impl.cc:289: if
(WebInputEventTraits::IsNewTouchSequence(touch_event.event))
On 2014/02/21 15:50:10, jdduke wrote:
> On 2014/02/21 15:45:33, jdduke wrote:
> > Hmm, what if the touch handler is removed during a touch sequence?  On the
> next
> > touch sequence, we won't forward any touches, but we'll still have the touch
> > action from the previous sequence?
> 
> Hmm, we should probably have |OnHasTouchEventHandlers()| update
> TouchActionFilter in some fashion regardless of this change?

In theory OnHasTouchHandlers should be completely irrelevant to touch-action.
See crbug.com/329566 for context.

https://codereview.chromium.org/171283002/diff/240001/content/common/input/we...
File content/common/input/web_input_event_traits.h (right):

https://codereview.chromium.org/171283002/diff/240001/content/common/input/we...
content/common/input/web_input_event_traits.h:26: static bool
IsNewTouchSequence(const blink::WebTouchEvent& event);
On 2014/02/21 15:45:33, jdduke wrote:
> I wonder if it's worth having a WebTouchEventTraits in
> content/renderer_host/input where we can add perhaps a few additional queries
> (IsTouchSequenceEnd()? IsValidTouchEvent()?).  We could move it to common
should
> the need arise to use it elsewhere?
> 
> Also, we should be consistent with the GestureEventPacket naming here, e.g.,
> TouchSequenceBegin, or NewTouchSequence, but not both... Perhaps a better name
> is |TouchSequence{Start,End}| to parallel Touch{Start,End}.

Done.

[Rick Byers, 2014-02-21 19:52:13]

https://codereview.chromium.org/171283002/diff/240001/content/browser/rendere...
File content/browser/renderer_host/input/input_router_impl.cc (right):

https://codereview.chromium.org/171283002/diff/240001/content/browser/rendere...
content/browser/renderer_host/input/input_router_impl.cc:289: if
(WebInputEventTraits::IsNewTouchSequence(touch_event.event))
On 2014/02/21 19:14:03, tdresser wrote:
> On 2014/02/21 15:50:10, jdduke wrote:
> > On 2014/02/21 15:45:33, jdduke wrote:
> > > Hmm, what if the touch handler is removed during a touch sequence?  On the
> > next
> > > touch sequence, we won't forward any touches, but we'll still have the
touch
> > > action from the previous sequence?
> > 
> > Hmm, we should probably have |OnHasTouchEventHandlers()| update
> > TouchActionFilter in some fashion regardless of this change?
> 
> In theory OnHasTouchHandlers should be completely irrelevant to touch-action.
> See crbug.com/329566 for context. 

I think Jared is right here - good catch!  The problem is that if we reset the
touch action on the START of a touch sequence, then anything that filters out
touch sequences (like TouchEventQueue does in various ways) can cause us to miss
this start.  crbug.com/329566 is about compositor touch hit testing (where we
have a fast path on the impl thread).  Jared is talking about the (older) fast
path where we avoid sending touch events to the renderer entirely when we know
there are no touch handlers at all.  TouchEventQueue should be free to drop
touch events for other reasons too (thought I don't think it drops touchstart
for any other case today).

Perhaps a better fix is to reset the touch action on touch sequence end (since
if SetTouchAction was called, then a touchstart reached the renderer, and so
_some_ ending event for the sequence must as well).  We'll have to be careful
not to end pre-maturely on synthetic touchcancel cases.  This may prove
problematic too.

Or maybe we really do need to plumb GestureEnd (for the last finger) through (as
recently came up in one of Zeeshan's patches too).  Having a 'all fingers have
left the screen' gesture event could be usefulll...

Alternately hooking just OnHasTouchEventHandlers as Jared suggests is workable
as well, but feels like a hack that could be brittle.

[Rick Byers, 2014-02-21 19:52:46]

On 2014/02/21 19:52:13, Rick Byers wrote:
>
https://codereview.chromium.org/171283002/diff/240001/content/browser/rendere...
> File content/browser/renderer_host/input/input_router_impl.cc (right):
> 
>
https://codereview.chromium.org/171283002/diff/240001/content/browser/rendere...
> content/browser/renderer_host/input/input_router_impl.cc:289: if
> (WebInputEventTraits::IsNewTouchSequence(touch_event.event))
> On 2014/02/21 19:14:03, tdresser wrote:
> > On 2014/02/21 15:50:10, jdduke wrote:
> > > On 2014/02/21 15:45:33, jdduke wrote:
> > > > Hmm, what if the touch handler is removed during a touch sequence?  On
the
> > > next
> > > > touch sequence, we won't forward any touches, but we'll still have the
> touch
> > > > action from the previous sequence?
> > > 
> > > Hmm, we should probably have |OnHasTouchEventHandlers()| update
> > > TouchActionFilter in some fashion regardless of this change?
> > 
> > In theory OnHasTouchHandlers should be completely irrelevant to
touch-action.
> > See crbug.com/329566 for context. 
> 
> I think Jared is right here - good catch!  The problem is that if we reset the
> touch action on the START of a touch sequence, then anything that filters out
> touch sequences (like TouchEventQueue does in various ways) can cause us to
miss
> this start.  crbug.com/329566 is about compositor touch hit testing (where we
> have a fast path on the impl thread).  Jared is talking about the (older) fast
> path where we avoid sending touch events to the renderer entirely when we know
> there are no touch handlers at all.  TouchEventQueue should be free to drop
> touch events for other reasons too (thought I don't think it drops touchstart
> for any other case today).
> 
> Perhaps a better fix is to reset the touch action on touch sequence end (since
> if SetTouchAction was called, then a touchstart reached the renderer, and so
> _some_ ending event for the sequence must as well).  We'll have to be careful
> not to end pre-maturely on synthetic touchcancel cases.  This may prove
> problematic too.
> 
> Or maybe we really do need to plumb GestureEnd (for the last finger) through
(as
> recently came up in one of Zeeshan's patches too).  Having a 'all fingers have
> left the screen' gesture event could be usefulll...
> 
> Alternately hooking just OnHasTouchEventHandlers as Jared suggests is workable
> as well, but feels like a hack that could be brittle.

Sorry I didn't catch this sooner by the way - I know I suggested using touch
sequence start.  I should have anticipated this issue.

[jdduke (slow), 2014-02-21 20:03:48]

On 2014/02/21 19:52:46, Rick Byers wrote:
> On 2014/02/21 19:52:13, Rick Byers wrote:
> > Perhaps a better fix is to reset the touch action on touch sequence end
(since
> > if SetTouchAction was called, then a touchstart reached the renderer, and so
> > _some_ ending event for the sequence must as well).  We'll have to be
careful
> > not to end pre-maturely on synthetic touchcancel cases.  This may prove
> > problematic too.
> > 
> > Or maybe we really do need to plumb GestureEnd (for the last finger) through
> (as
> > recently came up in one of Zeeshan's patches too).  Having a 'all fingers
have
> > left the screen' gesture event could be usefulll...

Yeah, we probably shouldn't assume that this is the only case where the
TouchEventQueue might drop a TouchSequenceStart.

The TEQ does guarantee we'll get an ack for all touch events, though, so I
wonder if it's sufficient to also listen to a NO_CONSUMER_EXISTS for
TouchSequenceStart.  That's starting to get messy though.

It's a shame our event ACK message payloads aren't more flexible... I'd love to
plumb things like overscroll deltas and/or touchaction responses there.

[Rick Byers, 2014-02-21 20:13:22]

On 2014/02/21 20:03:48, jdduke wrote:
> On 2014/02/21 19:52:46, Rick Byers wrote:
> > On 2014/02/21 19:52:13, Rick Byers wrote:
> > > Perhaps a better fix is to reset the touch action on touch sequence end
> (since
> > > if SetTouchAction was called, then a touchstart reached the renderer, and
so
> > > _some_ ending event for the sequence must as well).  We'll have to be
> careful
> > > not to end pre-maturely on synthetic touchcancel cases.  This may prove
> > > problematic too.
> > > 
> > > Or maybe we really do need to plumb GestureEnd (for the last finger)
through
> > (as
> > > recently came up in one of Zeeshan's patches too).  Having a 'all fingers
> have
> > > left the screen' gesture event could be usefulll...
> 
> Yeah, we probably shouldn't assume that this is the only case where the
> TouchEventQueue might drop a TouchSequenceStart.
> 
> The TEQ does guarantee we'll get an ack for all touch events, though, so I
> wonder if it's sufficient to also listen to a NO_CONSUMER_EXISTS for
> TouchSequenceStart.  That's starting to get messy though.

Oh that's a good point - resetting at either 'SendImmediately' or
'NO_CONSUMER_EXISTS' ACK would cover all the cases and avoid the brittleness I'm
worried about.  Perhaps not quite as elegant as a GestureEnding event, but
pretty simple.  Tim, please explore the options here and let us know what you
prefer (and, sigh, add another test case for this scenario of course <grin>).

> It's a shame our event ACK message payloads aren't more flexible... I'd love
to
> plumb things like overscroll deltas and/or touchaction responses there.

I don't see any reason we can't make them more flexible - it's up to us.  I
considered extending the ACK for touch-action, but decided against it because in
the future I expect it to be important for the impl thread to set the touch
action while the touch event is still handled on the main thread (it's the only
way I see to achieve the zero-main-thread-blocking scroll option I think we all
agree we eventually want).  So for touch-action, ACKing the input event, and
informing the browser of the touch-action are inherently disconnected...

[tdresser, 2014-02-25 16:44:50]

I think the simple option makes the most sense for now. On an ack indicating
that no consumer exists, we start a new touch sequence.

[Rick Byers, 2014-02-25 23:03:36]

https://codereview.chromium.org/171283002/diff/500001/content/browser/rendere...
File content/browser/renderer_host/input/input_router_impl.cc (right):

https://codereview.chromium.org/171283002/diff/500001/content/browser/rendere...
content/browser/renderer_host/input/input_router_impl.cc:342:
touch_action_filter_.OnTouchSequenceStart();
I think we want to do this for TouchStart events only.  I don't think it happens
today, but in theory we could send NO_CONSUMER_EXISTS for the move/end events,
even if there's a touchstart handler.  The rationale here (which you should
probably add a comment for) should be something like "We normally watch for
touchstart events being sent to the renderer to indicate a new touch sequence,
but in some cases we may filter out sending the touchstart - so catch those
here".

Note that in the CC hit testing case you'll be double-resetting (once when the
touchstart is sent, and once when the ACK comes back), but I think that's fine
for now.  We'll probably need to revisit this when we implement touch-action
support in the CC (since then we could get a NO_CONSUMER_EXISTS ACK AND still
have a touch-action other than auto).

https://codereview.chromium.org/171283002/diff/500001/content/browser/rendere...
File content/browser/renderer_host/input/input_router_impl_unittest.cc (right):

https://codereview.chromium.org/171283002/diff/500001/content/browser/rendere...
content/browser/renderer_host/input/input_router_impl_unittest.cc:1041:
SendInputEventACK(WebInputEvent::TouchMove,
You should probably make this a valid sequence: send the move/end with
ACK_STATE_CONSUMED, then send a new touchstart with NO_CONSUMER_EXISTS, right?

[tdresser, 2014-02-26 14:04:48]

https://codereview.chromium.org/171283002/diff/500001/content/browser/rendere...
File content/browser/renderer_host/input/input_router_impl.cc (right):

https://codereview.chromium.org/171283002/diff/500001/content/browser/rendere...
content/browser/renderer_host/input/input_router_impl.cc:342:
touch_action_filter_.OnTouchSequenceStart();
On 2014/02/25 23:03:37, Rick Byers wrote:
> I think we want to do this for TouchStart events only.  I don't think it
happens
> today, but in theory we could send NO_CONSUMER_EXISTS for the move/end events,
> even if there's a touchstart handler.  The rationale here (which you should
> probably add a comment for) should be something like "We normally watch for
> touchstart events being sent to the renderer to indicate a new touch sequence,
> but in some cases we may filter out sending the touchstart - so catch those
> here".
> 
> Note that in the CC hit testing case you'll be double-resetting (once when the
> touchstart is sent, and once when the ACK comes back), but I think that's fine
> for now.  We'll probably need to revisit this when we implement touch-action
> support in the CC (since then we could get a NO_CONSUMER_EXISTS ACK AND still
> have a touch-action other than auto).

Done.

https://codereview.chromium.org/171283002/diff/500001/content/browser/rendere...
File content/browser/renderer_host/input/input_router_impl_unittest.cc (right):

https://codereview.chromium.org/171283002/diff/500001/content/browser/rendere...
content/browser/renderer_host/input/input_router_impl_unittest.cc:1041:
SendInputEventACK(WebInputEvent::TouchMove,
On 2014/02/25 23:03:37, Rick Byers wrote:
> You should probably make this a valid sequence: send the move/end with
> ACK_STATE_CONSUMED, then send a new touchstart with NO_CONSUMER_EXISTS, right?

Done.

[Rick Byers, 2014-02-26 14:32:32]

lgtm with nit

https://codereview.chromium.org/171283002/diff/500001/content/browser/rendere...
File content/browser/renderer_host/input/input_router_impl.cc (right):

https://codereview.chromium.org/171283002/diff/500001/content/browser/rendere...
content/browser/renderer_host/input/input_router_impl.cc:342:
touch_action_filter_.OnTouchSequenceStart();
On 2014/02/26 14:04:49, tdresser wrote:
> On 2014/02/25 23:03:37, Rick Byers wrote:
> > I think we want to do this for TouchStart events only.  I don't think it
> happens
> > today, but in theory we could send NO_CONSUMER_EXISTS for the move/end
events,
> > even if there's a touchstart handler.  The rationale here (which you should
> > probably add a comment for) should be something like "We normally watch for

nit: since the assumptions here may change in the future, please add a comment
explaining the current rationale.

> > touchstart events being sent to the renderer to indicate a new touch
sequence,
> > but in some cases we may filter out sending the touchstart - so catch those
> > here".
> > 
> > Note that in the CC hit testing case you'll be double-resetting (once when
the
> > touchstart is sent, and once when the ACK comes back), but I think that's
fine
> > for now.  We'll probably need to revisit this when we implement touch-action
> > support in the CC (since then we could get a NO_CONSUMER_EXISTS ACK AND
still
> > have a touch-action other than auto).
> 
> Done.

[tdresser, 2014-02-26 15:16:13]

https://codereview.chromium.org/171283002/diff/500001/content/browser/rendere...
File content/browser/renderer_host/input/input_router_impl.cc (right):

https://codereview.chromium.org/171283002/diff/500001/content/browser/rendere...
content/browser/renderer_host/input/input_router_impl.cc:342:
touch_action_filter_.OnTouchSequenceStart();
On 2014/02/26 14:32:33, Rick Byers wrote:
> On 2014/02/26 14:04:49, tdresser wrote:
> > On 2014/02/25 23:03:37, Rick Byers wrote:
> > > I think we want to do this for TouchStart events only.  I don't think it
> > happens
> > > today, but in theory we could send NO_CONSUMER_EXISTS for the move/end
> events,
> > > even if there's a touchstart handler.  The rationale here (which you
should
> > > probably add a comment for) should be something like "We normally watch
for
> 
> nit: since the assumptions here may change in the future, please add a comment
> explaining the current rationale.
> 
> > > touchstart events being sent to the renderer to indicate a new touch
> sequence,
> > > but in some cases we may filter out sending the touchstart - so catch
those
> > > here".
> > > 
> > > Note that in the CC hit testing case you'll be double-resetting (once when
> the
> > > touchstart is sent, and once when the ACK comes back), but I think that's
> fine
> > > for now.  We'll probably need to revisit this when we implement
touch-action
> > > support in the CC (since then we could get a NO_CONSUMER_EXISTS ACK AND
> still
> > > have a touch-action other than auto).
> > 
> > Done.
> 

Whoops, sorry.
Done.

[jdduke (slow), 2014-02-26 15:40:55]

https://codereview.chromium.org/171283002/diff/560001/content/browser/rendere...
File content/browser/renderer_host/input/input_router_impl.cc (right):

https://codereview.chromium.org/171283002/diff/560001/content/browser/rendere...
content/browser/renderer_host/input/input_router_impl.cc:343: if
(event.event.type == WebInputEvent::TouchStart &&
Shouldn't this be |IsTouchSequenceStart()|?

https://codereview.chromium.org/171283002/diff/560001/content/browser/rendere...
content/browser/renderer_host/input/input_router_impl.cc:345:
touch_action_filter_.OnTouchSequenceStart();
Can we rename |OnTouchSequenceStart()| to something like |ResetTouchAction()|?
That way it's immediately obvious what this call is doing, and we don't have to
hijack the method for a case like this.

[tdresser, 2014-02-26 19:31:26]

https://codereview.chromium.org/171283002/diff/560001/content/browser/rendere...
File content/browser/renderer_host/input/input_router_impl.cc (right):

https://codereview.chromium.org/171283002/diff/560001/content/browser/rendere...
content/browser/renderer_host/input/input_router_impl.cc:343: if
(event.event.type == WebInputEvent::TouchStart &&
On 2014/02/26 15:40:55, jdduke wrote:
> Shouldn't this be |IsTouchSequenceStart()|?

Done.

https://codereview.chromium.org/171283002/diff/560001/content/browser/rendere...
content/browser/renderer_host/input/input_router_impl.cc:345:
touch_action_filter_.OnTouchSequenceStart();
On 2014/02/26 15:40:55, jdduke wrote:
> Can we rename |OnTouchSequenceStart()| to something like |ResetTouchAction()|?
> That way it's immediately obvious what this call is doing, and we don't have
to
> hijack the method for a case like this.

Done.

[jdduke (slow), 2014-02-26 19:39:42]

lgtm with nit.

https://codereview.chromium.org/171283002/diff/580001/content/browser/rendere...
File content/browser/renderer_host/input/touch_action_filter_unittest.cc
(right):

https://codereview.chromium.org/171283002/diff/580001/content/browser/rendere...
content/browser/renderer_host/input/touch_action_filter_unittest.cc:557:
TEST(TouchActionFilterTest, TouchActionResetsResetTouchAction) {
Nit: Maybe rename this |TouchActionResetsOnResetTouchAction|?

[tdresser, 2014-02-26 20:14:35]

https://codereview.chromium.org/171283002/diff/580001/content/browser/rendere...
File content/browser/renderer_host/input/touch_action_filter_unittest.cc
(right):

https://codereview.chromium.org/171283002/diff/580001/content/browser/rendere...
content/browser/renderer_host/input/touch_action_filter_unittest.cc:557:
TEST(TouchActionFilterTest, TouchActionResetsResetTouchAction) {
On 2014/02/26 19:39:43, jdduke wrote:
> Nit: Maybe rename this |TouchActionResetsOnResetTouchAction|?

Gah, sloppy find/replace.
Thanks.
Done.

[tdresser, 2014-02-26 20:14:42]

The CQ bit was checked by tdresser@chromium.org

[commit-bot: I haz the power, 2014-02-26 20:21:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tdresser@chromium.org/171283002/600001

[commit-bot: I haz the power, 2014-02-26 21:14:03]

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

[commit-bot: I haz the power, 2014-02-26 21:14:04]

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

[tdresser, 2014-02-27 14:24:23]

The CQ bit was checked by tdresser@chromium.org

[commit-bot: I haz the power, 2014-02-27 14:25:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tdresser@chromium.org/171283002/600001

[commit-bot: I haz the power, 2014-02-27 16:28:04]

Change committed as 253841