Don't treat first touch move differently from future touch moves

Don't treat first move touch differently from future touch moves.

See http://lists.w3.org/Archives/Public/public-touchevents/2014Apr/0005.html for details.

BUG=354460
TEST=TouchEventQueueTest.TouchAbsorptionWithConsumedFirstMove, TouchEventQueueTest.TouchCancelOnScroll

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

[tdresser, 2014-04-14 17:58:02]

jdduke, PTAL.

[jdduke (slow), 2014-04-14 18:11:45]

On 2014/04/14 17:58:02, tdresser wrote:
> jdduke, PTAL.

We should probably wait on this until it has been discussed by the group,
particularly now that the state of touchmove absorption is in flux.

[tdresser, 2014-04-22 19:26:43]

On 2014/04/14 18:11:45, jdduke wrote:
> On 2014/04/14 17:58:02, tdresser wrote:
> > jdduke, PTAL.
> 
> We should probably wait on this until it has been discussed by the group,
> particularly now that the state of touchmove absorption is in flux.

As our plan for touch-absorption has solidified and this has now been discussed
publicly
(http://lists.w3.org/Archives/Public/public-touchevents/2014Apr/0005.html), I
think it's reasonable that we push forward with this.

[jdduke (slow), 2014-04-22 19:30:54]

On 2014/04/22 19:26:43, tdresser wrote:
> On 2014/04/14 18:11:45, jdduke wrote:
> > On 2014/04/14 17:58:02, tdresser wrote:
> > > jdduke, PTAL.
> > 
> > We should probably wait on this until it has been discussed by the group,
> > particularly now that the state of touchmove absorption is in flux.
> 
> As our plan for touch-absorption has solidified and this has now been
discussed
> publicly
> (http://lists.w3.org/Archives/Public/public-touchevents/2014Apr/0005.html), I
> think it's reasonable that we push forward with this.

I thought we agreed in the meeting this morning that we would try to get some
additional discussion/feedback on this before proceeding?

I am curious, what part of pull-to-refresh is required by this change? It seems
like the effect should only preventDefault a touchmove *after* the effect has
started and has some "slack" that it needs to deal with before handing off the
scroll baton.  With async touchmove, you'll always get the overscrolling
touchmove's and there's no need to preventDefault them.

[tdresser, 2014-04-22 19:49:26]

On 2014/04/22 19:30:54, jdduke wrote:
> On 2014/04/22 19:26:43, tdresser wrote:
> > On 2014/04/14 18:11:45, jdduke wrote:
> > > On 2014/04/14 17:58:02, tdresser wrote:
> > > > jdduke, PTAL.
> > > 
> > > We should probably wait on this until it has been discussed by the group,
> > > particularly now that the state of touchmove absorption is in flux.
> > 
> > As our plan for touch-absorption has solidified and this has now been
> discussed
> > publicly
> > (http://lists.w3.org/Archives/Public/public-touchevents/2014Apr/0005.html),
I
> > think it's reasonable that we push forward with this.
> 
> I thought we agreed in the meeting this morning that we would try to get some
> additional discussion/feedback on this before proceeding?
> 
> I am curious, what part of pull-to-refresh is required by this change? It
seems
> like the effect should only preventDefault a touchmove *after* the effect has
> started and has some "slack" that it needs to deal with before handing off the
> scroll baton.  With async touchmove, you'll always get the overscrolling
> touchmove's and there's no need to preventDefault them.

rbyers@ and I figure it's reasonable to push forward with this in parallel with
the discussion in the touch events community group.

This is necessary for pull to refresh when the user starts a fresh scroll while
the P2R header is already displayed.
The issue occurs in the following scenario:

1. Drag down the P2R header
2. Release, so the content begins to refresh, with the header still displayed.
3. With a new touch, scroll downwards.

If the first touchmove event of that scroll is consumed, then we won't be able
to transition into impl thread scrolling.

If the first touchmove event of that scroll is not consumed, then we may not get
any further touchmove events, meaning that we can't drive the required P2R
functionality this scroll.

With the proposed changes to touch absorption, there is a reasonable chance that
not consuming the first touch event would work, as most of the time we'll get
another touch move event which we could preventDefault. If, within a single
frame, the finger moves outside the region within which we send touchmoves, we
will run into issues.

[jdduke (slow), 2014-04-22 20:03:19]

On 2014/04/22 19:49:26, tdresser wrote:
> On 2014/04/22 19:30:54, jdduke wrote:
> > On 2014/04/22 19:26:43, tdresser wrote:
> > > On 2014/04/14 18:11:45, jdduke wrote:
> > > > On 2014/04/14 17:58:02, tdresser wrote:
> > > > > jdduke, PTAL.
> > > > 
> > > > We should probably wait on this until it has been discussed by the
group,
> > > > particularly now that the state of touchmove absorption is in flux.
> > > 
> > > As our plan for touch-absorption has solidified and this has now been
> > discussed
> > > publicly
> > >
(http://lists.w3.org/Archives/Public/public-touchevents/2014Apr/0005.html),
> I
> > > think it's reasonable that we push forward with this.
> > 
> > I thought we agreed in the meeting this morning that we would try to get
some
> > additional discussion/feedback on this before proceeding?
> > 
> > I am curious, what part of pull-to-refresh is required by this change? It
> seems
> > like the effect should only preventDefault a touchmove *after* the effect
has
> > started and has some "slack" that it needs to deal with before handing off
the
> > scroll baton.  With async touchmove, you'll always get the overscrolling
> > touchmove's and there's no need to preventDefault them.
> 
> rbyers@ and I figure it's reasonable to push forward with this in parallel
with
> the discussion in the touch events community group.
> 
> This is necessary for pull to refresh when the user starts a fresh scroll
while
> the P2R header is already displayed.
> The issue occurs in the following scenario:
> 
> 1. Drag down the P2R header
> 2. Release, so the content begins to refresh, with the header still displayed.
> 3. With a new touch, scroll downwards.
> 
> If the first touchmove event of that scroll is consumed, then we won't be able
> to transition into impl thread scrolling.
> 
> If the first touchmove event of that scroll is not consumed, then we may not
get
> any further touchmove events, meaning that we can't drive the required P2R
> functionality this scroll.
> 
> With the proposed changes to touch absorption, there is a reasonable chance
that
> not consuming the first touch event would work, as most of the time we'll get
> another touch move event which we could preventDefault. If, within a single
> frame, the finger moves outside the region within which we send touchmoves, we
> will run into issues.

If a user has lifted their finger, do we really want the refresh effect to
remain sticky if the user is scrolling "out" and "away" from the effect? In my
mind, the effect should simply dissipate if the user has lifted their finger and
tries to resume scrolling.

[tdresser, 2014-04-22 20:09:12]

On 2014/04/22 20:03:19, jdduke wrote:
> On 2014/04/22 19:49:26, tdresser wrote:
> > On 2014/04/22 19:30:54, jdduke wrote:
> > > On 2014/04/22 19:26:43, tdresser wrote:
> > > > On 2014/04/14 18:11:45, jdduke wrote:
> > > > > On 2014/04/14 17:58:02, tdresser wrote:
> > > > > > jdduke, PTAL.
> > > > > 
> > > > > We should probably wait on this until it has been discussed by the
> group,
> > > > > particularly now that the state of touchmove absorption is in flux.
> > > > 
> > > > As our plan for touch-absorption has solidified and this has now been
> > > discussed
> > > > publicly
> > > >
> (http://lists.w3.org/Archives/Public/public-touchevents/2014Apr/0005.html),
> > I
> > > > think it's reasonable that we push forward with this.
> > > 
> > > I thought we agreed in the meeting this morning that we would try to get
> some
> > > additional discussion/feedback on this before proceeding?
> > > 
> > > I am curious, what part of pull-to-refresh is required by this change? It
> > seems
> > > like the effect should only preventDefault a touchmove *after* the effect
> has
> > > started and has some "slack" that it needs to deal with before handing off
> the
> > > scroll baton.  With async touchmove, you'll always get the overscrolling
> > > touchmove's and there's no need to preventDefault them.
> > 
> > rbyers@ and I figure it's reasonable to push forward with this in parallel
> with
> > the discussion in the touch events community group.
> > 
> > This is necessary for pull to refresh when the user starts a fresh scroll
> while
> > the P2R header is already displayed.
> > The issue occurs in the following scenario:
> > 
> > 1. Drag down the P2R header
> > 2. Release, so the content begins to refresh, with the header still
displayed.
> > 3. With a new touch, scroll downwards.
> > 
> > If the first touchmove event of that scroll is consumed, then we won't be
able
> > to transition into impl thread scrolling.
> > 
> > If the first touchmove event of that scroll is not consumed, then we may not
> get
> > any further touchmove events, meaning that we can't drive the required P2R
> > functionality this scroll.
> > 
> > With the proposed changes to touch absorption, there is a reasonable chance
> that
> > not consuming the first touch event would work, as most of the time we'll
get
> > another touch move event which we could preventDefault. If, within a single
> > frame, the finger moves outside the region within which we send touchmoves,
we
> > will run into issues.
> 
> If a user has lifted their finger, do we really want the refresh effect to
> remain sticky if the user is scrolling "out" and "away" from the effect? In my
> mind, the effect should simply dissipate if the user has lifted their finger
and
> tries to resume scrolling.

Suppose the user scrolls up, but not quite far enough to trigger a refresh, and
then releases.
As the header bounces back down (before it's entirely hidden), the user puts
their finger down and begins another scroll. 

I think we want to continue the scroll from the point where the user's finger
went down, and I think it makes sense to keep this consistent in all states of
the widget.

[Rick Byers, 2014-04-22 20:30:39]

On 2014/04/22 20:09:12, tdresser wrote:
> On 2014/04/22 20:03:19, jdduke wrote:
> > On 2014/04/22 19:49:26, tdresser wrote:
> > > On 2014/04/22 19:30:54, jdduke wrote:
> > > > On 2014/04/22 19:26:43, tdresser wrote:
> > > > > On 2014/04/14 18:11:45, jdduke wrote:
> > > > > > On 2014/04/14 17:58:02, tdresser wrote:
> > > > > > > jdduke, PTAL.
> > > > > > 
> > > > > > We should probably wait on this until it has been discussed by the
> > group,
> > > > > > particularly now that the state of touchmove absorption is in flux.
> > > > > 
> > > > > As our plan for touch-absorption has solidified and this has now been
> > > > discussed
> > > > > publicly
> > > > >
> > (http://lists.w3.org/Archives/Public/public-touchevents/2014Apr/0005.html),
> > > I
> > > > > think it's reasonable that we push forward with this.
> > > > 
> > > > I thought we agreed in the meeting this morning that we would try to get
> > some
> > > > additional discussion/feedback on this before proceeding?
> > > > 
> > > > I am curious, what part of pull-to-refresh is required by this change?
It
> > > seems
> > > > like the effect should only preventDefault a touchmove *after* the
effect
> > has
> > > > started and has some "slack" that it needs to deal with before handing
off
> > the
> > > > scroll baton.  With async touchmove, you'll always get the overscrolling
> > > > touchmove's and there's no need to preventDefault them.
> > > 
> > > rbyers@ and I figure it's reasonable to push forward with this in parallel
> > with
> > > the discussion in the touch events community group.
> > > 
> > > This is necessary for pull to refresh when the user starts a fresh scroll
> > while
> > > the P2R header is already displayed.
> > > The issue occurs in the following scenario:
> > > 
> > > 1. Drag down the P2R header
> > > 2. Release, so the content begins to refresh, with the header still
> displayed.
> > > 3. With a new touch, scroll downwards.
> > > 
> > > If the first touchmove event of that scroll is consumed, then we won't be
> able
> > > to transition into impl thread scrolling.
> > > 
> > > If the first touchmove event of that scroll is not consumed, then we may
not
> > get
> > > any further touchmove events, meaning that we can't drive the required P2R
> > > functionality this scroll.
> > > 
> > > With the proposed changes to touch absorption, there is a reasonable
chance
> > that
> > > not consuming the first touch event would work, as most of the time we'll
> get
> > > another touch move event which we could preventDefault. If, within a
single
> > > frame, the finger moves outside the region within which we send
touchmoves,
> we
> > > will run into issues.
> > 
> > If a user has lifted their finger, do we really want the refresh effect to
> > remain sticky if the user is scrolling "out" and "away" from the effect? In
my
> > mind, the effect should simply dissipate if the user has lifted their finger
> and
> > tries to resume scrolling.
> 
> Suppose the user scrolls up, but not quite far enough to trigger a refresh,
and
> then releases.
> As the header bounces back down (before it's entirely hidden), the user puts
> their finger down and begins another scroll. 
> 
> I think we want to continue the scroll from the point where the user's finger
> went down, and I think it makes sense to keep this consistent in all states of
> the widget.

Isn't this also an issue in the more common case where the content is at
scrollTop=0 and the user pulls down a little then reverses and tries to scroll
the page?  They absolutely must be able to scroll in that case, seems like it
could happen a bunch accidentally or intentionally.

[tdresser, 2014-04-22 20:36:25]

On 2014/04/22 20:30:39, Rick Byers wrote:
> On 2014/04/22 20:09:12, tdresser wrote:
> > On 2014/04/22 20:03:19, jdduke wrote:
> > > On 2014/04/22 19:49:26, tdresser wrote:
> > > > On 2014/04/22 19:30:54, jdduke wrote:
> > > > > On 2014/04/22 19:26:43, tdresser wrote:
> > > > > > On 2014/04/14 18:11:45, jdduke wrote:
> > > > > > > On 2014/04/14 17:58:02, tdresser wrote:
> > > > > > > > jdduke, PTAL.
> > > > > > > 
> > > > > > > We should probably wait on this until it has been discussed by the
> > > group,
> > > > > > > particularly now that the state of touchmove absorption is in
flux.
> > > > > > 
> > > > > > As our plan for touch-absorption has solidified and this has now
been
> > > > > discussed
> > > > > > publicly
> > > > > >
> > >
(http://lists.w3.org/Archives/Public/public-touchevents/2014Apr/0005.html),
> > > > I
> > > > > > think it's reasonable that we push forward with this.
> > > > > 
> > > > > I thought we agreed in the meeting this morning that we would try to
get
> > > some
> > > > > additional discussion/feedback on this before proceeding?
> > > > > 
> > > > > I am curious, what part of pull-to-refresh is required by this change?
> It
> > > > seems
> > > > > like the effect should only preventDefault a touchmove *after* the
> effect
> > > has
> > > > > started and has some "slack" that it needs to deal with before handing
> off
> > > the
> > > > > scroll baton.  With async touchmove, you'll always get the
overscrolling
> > > > > touchmove's and there's no need to preventDefault them.
> > > > 
> > > > rbyers@ and I figure it's reasonable to push forward with this in
parallel
> > > with
> > > > the discussion in the touch events community group.
> > > > 
> > > > This is necessary for pull to refresh when the user starts a fresh
scroll
> > > while
> > > > the P2R header is already displayed.
> > > > The issue occurs in the following scenario:
> > > > 
> > > > 1. Drag down the P2R header
> > > > 2. Release, so the content begins to refresh, with the header still
> > displayed.
> > > > 3. With a new touch, scroll downwards.
> > > > 
> > > > If the first touchmove event of that scroll is consumed, then we won't
be
> > able
> > > > to transition into impl thread scrolling.
> > > > 
> > > > If the first touchmove event of that scroll is not consumed, then we may
> not
> > > get
> > > > any further touchmove events, meaning that we can't drive the required
P2R
> > > > functionality this scroll.
> > > > 
> > > > With the proposed changes to touch absorption, there is a reasonable
> chance
> > > that
> > > > not consuming the first touch event would work, as most of the time
we'll
> > get
> > > > another touch move event which we could preventDefault. If, within a
> single
> > > > frame, the finger moves outside the region within which we send
> touchmoves,
> > we
> > > > will run into issues.
> > > 
> > > If a user has lifted their finger, do we really want the refresh effect to
> > > remain sticky if the user is scrolling "out" and "away" from the effect?
In
> my
> > > mind, the effect should simply dissipate if the user has lifted their
finger
> > and
> > > tries to resume scrolling.
> > 
> > Suppose the user scrolls up, but not quite far enough to trigger a refresh,
> and
> > then releases.
> > As the header bounces back down (before it's entirely hidden), the user puts
> > their finger down and begins another scroll. 
> > 
> > I think we want to continue the scroll from the point where the user's
finger
> > went down, and I think it makes sense to keep this consistent in all states
of
> > the widget.
> 
> Isn't this also an issue in the more common case where the content is at
> scrollTop=0 and the user pulls down a little then reverses and tries to scroll
> the page?  They absolutely must be able to scroll in that case, seems like it
> could happen a bunch accidentally or intentionally.

In the case you describe, you can 
- Not preventDefault the first touchmove
- PreventDefault the second touchmove, which you'll get since you're in
overscroll.

[jdduke (slow), 2014-04-22 20:45:58]

I guess my primary concern here is shipping both this and the async touchmove
change in the same release, as both are likely to have compat issues.

[Rick Byers, 2014-04-24 17:49:05]

> > Isn't this also an issue in the more common case where the content is at
> > scrollTop=0 and the user pulls down a little then reverses and tries to
scroll
> > the page?  They absolutely must be able to scroll in that case, seems like
it
> > could happen a bunch accidentally or intentionally.
> 
> In the case you describe, you can 
> - Not preventDefault the first touchmove
> - PreventDefault the second touchmove, which you'll get since you're in
> overscroll.

Good point.  In theory you might get only a single move, but it should be very
rare in practice.

On 2014/04/22 20:45:58, jdduke wrote:
> I guess my primary concern here is shipping both this and the async touchmove
> change in the same release, as both are likely to have compat issues.

If we're really talking just about edge cases in pull-to-refresh, then I suggest
we start for now with special-casing the first move (never consuming it) and see
how often we notice a glitch in practice.  Chances are that'll take us to after
the branch point, then we can make the change in 37.  I agree it's probably
better to minimize the risk to the async touchmove change.

[jdduke (slow), 2014-05-01 15:53:21]

https://codereview.chromium.org/228973003/diff/60001/ui/events/gesture_detect...
File ui/events/gesture_detection/touch_disposition_gesture_filter.cc (right):

https://codereview.chromium.org/228973003/diff/60001/ui/events/gesture_detect...
ui/events/gesture_detection/touch_disposition_gesture_filter.cc:80: return
Info(RT_START);
Hmm, but even with this change, the first touchmove remains special because
preventDefault'ing it will prevent the GestureScrollBegin, which in turn will
prevent all subsequent GestureScrollUpdate's?

[jdduke (slow), 2014-05-01 15:53:57]

https://codereview.chromium.org/228973003/diff/60001/ui/events/gesture_detect...
File ui/events/gesture_detection/touch_disposition_gesture_filter.cc (right):

https://codereview.chromium.org/228973003/diff/60001/ui/events/gesture_detect...
ui/events/gesture_detection/touch_disposition_gesture_filter.cc:80: return
Info(RT_START);
On 2014/05/01 15:53:22, jdduke wrote:
> Hmm, but even with this change, the first touchmove remains special because
> preventDefault'ing it will prevent the GestureScrollBegin, which in turn will
> prevent all subsequent GestureScrollUpdate's?

Nevermind, looks like we don't attach RT_CURRENT to this event.

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

If we still want to do this, I'd support landing either ASAP (to shake out
potential issues well before M37 branch), or waiting until Aura adopts the new
GR, thus preserving touch handling parity across platforms.  I'm leaning towards
the latter, but I'm sympathetic to the former (and could be convinced...).

[tdresser, 2014-05-22 12:16:34]

On 2014/05/20 20:36:21, jdduke wrote:
> If we still want to do this, I'd support landing either ASAP (to shake out
> potential issues well before M37 branch), or waiting until Aura adopts the new
> GR, thus preserving touch handling parity across platforms.  I'm leaning
towards
> the latter, but I'm sympathetic to the former (and could be convinced...).

I agree that the latter is the correct approach.

[tdresser, 2014-06-13 13:41:57]

Jared, do you think it makes sense to land this now, or should we hold off until
the unified gesture detector has been through a few canary builds?

[jdduke (slow), 2014-06-13 14:09:54]

On 2014/06/13 13:41:57, tdresser wrote:
> Jared, do you think it makes sense to land this now, or should we hold off
until
> the unified gesture detector has been through a few canary builds?

How necessary is this for M37? I'm a little wary of pushing it through a week
before the branch point, but I'd be all for landing this immediately after
branch.  That gives Android canary/dev builds a bit longer to bake with the
change before it goes to Beta (sadly, Android's Canary/Dev channels are woefully
underrepresented as they insist on keeping them internal =/)

[tdresser, 2014-06-13 14:16:01]

On 2014/06/13 14:09:54, jdduke wrote:
> On 2014/06/13 13:41:57, tdresser wrote:
> > Jared, do you think it makes sense to land this now, or should we hold off
> until
> > the unified gesture detector has been through a few canary builds?
> 
> How necessary is this for M37? I'm a little wary of pushing it through a week
> before the branch point, but I'd be all for landing this immediately after
> branch.  That gives Android canary/dev builds a bit longer to bake with the
> change before it goes to Beta (sadly, Android's Canary/Dev channels are
woefully
> underrepresented as they insist on keeping them internal =/)

SGTM.

[tdresser, 2014-07-02 17:27:57]

On 2014/06/13 14:16:01, tdresser wrote:
> On 2014/06/13 14:09:54, jdduke wrote:
> > On 2014/06/13 13:41:57, tdresser wrote:
> > > Jared, do you think it makes sense to land this now, or should we hold off
> > until
> > > the unified gesture detector has been through a few canary builds?
> > 
> > How necessary is this for M37? I'm a little wary of pushing it through a
week
> > before the branch point, but I'd be all for landing this immediately after
> > branch.  That gives Android canary/dev builds a bit longer to bake with the
> > change before it goes to Beta (sadly, Android's Canary/Dev channels are
> woefully
> > underrepresented as they insist on keeping them internal =/)
> 
> SGTM.

Jared, can you take a look, now that we're passed the branch?

[jdduke (slow), 2014-07-02 17:46:37]

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

https://codereview.chromium.org/228973003/diff/120001/content/browser/rendere...
content/browser/renderer_host/input/touch_event_queue.cc:577:
gesture_event.event.type != blink::WebInputEvent::GestureScrollBegin) {
Hmm, I would think we also want the "first allowed scroll update" logic to apply
to the TOUCH_CANCEL mode, no?  Otherwise, if the page preventDefaults the first
touchmove, we'd still get a GSB which would cancel the touch sequence?

It might be easier if we do the GSB and GSU checks at the very beginning of this
function, and treat the first GSU like we do the first GSB (returning early
otherwise).

[tdresser, 2014-07-04 18:28:17]

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

https://codereview.chromium.org/228973003/diff/120001/content/browser/rendere...
content/browser/renderer_host/input/touch_event_queue.cc:577:
gesture_event.event.type != blink::WebInputEvent::GestureScrollBegin) {
On 2014/07/02 17:46:37, jdduke wrote:
> Hmm, I would think we also want the "first allowed scroll update" logic to
apply
> to the TOUCH_CANCEL mode, no?  Otherwise, if the page preventDefaults the
first
> touchmove, we'd still get a GSB which would cancel the touch sequence?
> 
> It might be easier if we do the GSB and GSU checks at the very beginning of
this
> function, and treat the first GSU like we do the first GSB (returning early
> otherwise).

Done.

[jdduke (slow), 2014-07-07 23:09:14]

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

https://codereview.chromium.org/228973003/diff/140001/content/browser/rendere...
content/browser/renderer_host/input/touch_event_queue.cc:563: if
(gesture_event.event.type != blink::WebInputEvent::GestureScrollUpdate)
Hmm, maybe change this to read:

if (gesture_event.event.type != blink::WebInputEvent::GestureScrollUpdate ||
    seen_scroll_update_this_sequence_)
  return;

seen_scroll_update_this_sequence_ = true;
...

https://codereview.chromium.org/228973003/diff/140001/content/browser/rendere...
File content/browser/renderer_host/input/touch_event_queue_unittest.cc (left):

https://codereview.chromium.org/228973003/diff/140001/content/browser/rendere...
content/browser/renderer_host/input/touch_event_queue_unittest.cc:894:
SendGestureEvent(WebInputEvent::GestureScrollUpdate);
Why remove this?

https://codereview.chromium.org/228973003/diff/140001/content/browser/rendere...
File content/browser/renderer_host/input/touch_event_queue_unittest.cc (right):

https://codereview.chromium.org/228973003/diff/140001/content/browser/rendere...
content/browser/renderer_host/input/touch_event_queue_unittest.cc:2076: //
scroll if we receive unconsumed touchmove events.
Maybe change the comment to say:

// Even if the first touchmove event was consumed, subsequently unconsumed
touchmove
// events should be allowed to trigger scrolling.

and then simulate 1) a consumed touchmove followed by 2) an unconsumed touchmove
that generates a GSUt?

[tdresser, 2014-07-08 15:56:41]

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

https://codereview.chromium.org/228973003/diff/140001/content/browser/rendere...
content/browser/renderer_host/input/touch_event_queue.cc:563: if
(gesture_event.event.type != blink::WebInputEvent::GestureScrollUpdate)
On 2014/07/07 23:09:14, jdduke wrote:
> Hmm, maybe change this to read:
> 
> if (gesture_event.event.type != blink::WebInputEvent::GestureScrollUpdate ||
>     seen_scroll_update_this_sequence_)
>   return;
> 
> seen_scroll_update_this_sequence_ = true;
> ...

Good catch, thanks.

https://codereview.chromium.org/228973003/diff/140001/content/browser/rendere...
File content/browser/renderer_host/input/touch_event_queue_unittest.cc (left):

https://codereview.chromium.org/228973003/diff/140001/content/browser/rendere...
content/browser/renderer_host/input/touch_event_queue_unittest.cc:894:
SendGestureEvent(WebInputEvent::GestureScrollUpdate);
On 2014/07/07 23:09:14, jdduke wrote:
> Why remove this?

I was thinking of it more as moving it upwards. Re-added.

https://codereview.chromium.org/228973003/diff/140001/content/browser/rendere...
File content/browser/renderer_host/input/touch_event_queue_unittest.cc (right):

https://codereview.chromium.org/228973003/diff/140001/content/browser/rendere...
content/browser/renderer_host/input/touch_event_queue_unittest.cc:2076: //
scroll if we receive unconsumed touchmove events.
On 2014/07/07 23:09:14, jdduke wrote:
> Maybe change the comment to say:
> 
> // Even if the first touchmove event was consumed, subsequently unconsumed
> touchmove
> // events should be allowed to trigger scrolling.
> 
> and then simulate 1) a consumed touchmove followed by 2) an unconsumed
touchmove
> that generates a GSUt?

Done.

[jdduke (slow), 2014-07-08 16:30:18]

lgtm, maybe add a few comments in the description about why this change is
desirable.

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

https://codereview.chromium.org/228973003/diff/160001/content/browser/rendere...
content/browser/renderer_host/input/touch_event_queue.cc:560: }
Could we return here (the end of the == GestureScrollBegin scope)? I know it's
not necessary but I always feel better about the early return :)

[tdresser, 2014-07-08 18:10:36]

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

https://codereview.chromium.org/228973003/diff/160001/content/browser/rendere...
content/browser/renderer_host/input/touch_event_queue.cc:560: }
On 2014/07/08 16:30:18, jdduke wrote:
> Could we return here (the end of the == GestureScrollBegin scope)? I know it's
> not necessary but I always feel better about the early return :)

Fine with me. Done.

[tdresser, 2014-07-08 18:11:25]

+sadrul for gesture_recognizer_unittest.cc

[sadrul, 2014-07-10 15:26:11]

The test change LGTM.

But should the aura-only GR be also updated to follow this new behaviour? (in
case we need to switch back to it)

[tdresser, 2014-07-11 13:55:39]

The CQ bit was checked by tdresser@chromium.org

[commit-bot: I haz the power, 2014-07-11 13:56:06]

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

[commit-bot: I haz the power, 2014-07-11 15:47:45]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-07-11 17:38:13]

Change committed as 282642

[tdresser, 2014-07-14 20:35:33]

A revert of this CL has been created in
https://codereview.chromium.org/394523004/ by tdresser@chromium.org.

The reason for reverting is: This never set |seen_scroll_update_this_sequence_|
to true, resulting in:
https://code.google.com/p/chromium/issues/detail?id=393695

The fact that no tests caught this issue is pretty scary; we'll need some
beefier tests as well as the trivial fix..