[Android] Ensure slop-exceeding TouchMove's are sent to the renderer

[Android] Ensure slop-exceeding TouchMove's are sent to the renderer

Without floating point DIP coords in WebTouchPoint, it's possible that a
TouchMove physically outside the pixel slop region will be classified as
inside the DIP slop region. This leads to cases where a touch handler is
never offered a TouchMove before a ScrollBegin occurs.

Until the resolution of crbug.com/3336807, address this by conservatively
constructing the DIP slop region in the TouchEventQueue, ensuring that touch
handlers are offered at least one TouchMove before scrolling begins.

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

[jdduke (slow), 2014-02-21 20:13:29]

tdresser@: PTAL.  Sorry I didn't catch this the first time =/

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

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

https://codereview.chromium.org/172263007/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/input/touch_event_queue.cc:205: if ((position -
touch_sequence_start_position_).LengthSquared() >=
In my Aura fix I was trying to keep the strict inequality, but if we're willing
to increase the occurrence of multiple touchmoves than this is definitely
simpler (and probably solves the problem for Aura and Android with the same CL).
 We can wait for the unified GR to put this back to a strict inequality.

When there is more than one touchmove event before scrolling starts, how are the
dispositions handled?  Does consuming ANY of them prevent scrolling in both Aura
and Android?  If so (or at least if consuming the _first_ is guaranteed to
prevent scrolling) then I think this should be fine for 34.  In 35 hopefully
we'll have touch move absorption and this will be different (there is the
disposition of only the first that's really special - as required by the
TouchEvent spec).

[tdresser, 2014-02-21 21:44:49]

LGTM with nit.

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

https://codereview.chromium.org/172263007/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/input/touch_event_queue.cc:179: // TODO(jdduke):
Remove int cast on suppression length, crbug.com/3336807.
No such bug?

https://codereview.chromium.org/172263007/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/input/touch_event_queue.cc:205: if ((position -
touch_sequence_start_position_).LengthSquared() >=
On 2014/02/21 20:22:49, Rick Byers wrote:
> In my Aura fix I was trying to keep the strict inequality, but if we're
willing
> to increase the occurrence of multiple touchmoves than this is definitely
> simpler (and probably solves the problem for Aura and Android with the same
CL).
>  We can wait for the unified GR to put this back to a strict inequality.
> 
> When there is more than one touchmove event before scrolling starts, how are
the
> dispositions handled?  Does consuming ANY of them prevent scrolling in both
Aura
> and Android?  If so (or at least if consuming the _first_ is guaranteed to
> prevent scrolling) then I think this should be fine for 34.  In 35 hopefully
> we'll have touch move absorption and this will be different (there is the
> disposition of only the first that's really special - as required by the
> TouchEvent spec).

Consuming any touch move will prevent scrolling. (See
https://code.google.com/p/chromium/issues/detail?id=333947 for the Aura bug on
this).

[jdduke (slow), 2014-02-21 22:29:05]

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

https://codereview.chromium.org/172263007/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/input/touch_event_queue.cc:179: // TODO(jdduke):
Remove int cast on suppression length, crbug.com/3336807.
On 2014/02/21 21:44:50, tdresser wrote:
> No such bug?

Oops, good catch.

https://codereview.chromium.org/172263007/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/input/touch_event_queue.cc:205: if ((position -
touch_sequence_start_position_).LengthSquared() >=
On 2014/02/21 21:44:50, tdresser wrote:
> On 2014/02/21 20:22:49, Rick Byers wrote:
> > In my Aura fix I was trying to keep the strict inequality, but if we're
> willing
> > to increase the occurrence of multiple touchmoves than this is definitely
> > simpler (and probably solves the problem for Aura and Android with the same
> CL).
> >  We can wait for the unified GR to put this back to a strict inequality.
> > 
> > When there is more than one touchmove event before scrolling starts, how are
> the
> > dispositions handled?  Does consuming ANY of them prevent scrolling in both
> Aura
> > and Android?  If so (or at least if consuming the _first_ is guaranteed to
> > prevent scrolling) then I think this should be fine for 34.  In 35 hopefully
> > we'll have touch move absorption and this will be different (there is the
> > disposition of only the first that's really special - as required by the
> > TouchEvent spec).
> 
> Consuming any touch move will prevent scrolling. (See
> https://code.google.com/p/chromium/issues/detail?id=333947 for the Aura bug on
> this).

Yeah, with unified GR we can toggle the inequality quite easily.  

For now, the non strict inequality here means Android could potentially send
multiple touchmove's before the sequence turns into a scroll, but this will be
relatively infrequent.  There's really no way around this with integer DIP
coords.  In any case, it's better than occasionally never offering the touchmove
at all, and we can certainly tighten this up for M35.

[jdduke (slow), 2014-02-21 22:29:46]

The CQ bit was checked by jdduke@chromium.org

[commit-bot: I haz the power, 2014-02-21 22:32:10]

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

[commit-bot: I haz the power, 2014-02-22 00:00:37]

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

[commit-bot: I haz the power, 2014-02-22 00:00:37]

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

[jdduke (slow), 2014-02-22 00:07:45]

The CQ bit was checked by jdduke@chromium.org

[commit-bot: I haz the power, 2014-02-22 00:10:12]

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

[commit-bot: I haz the power, 2014-02-22 01:31:28]

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

[commit-bot: I haz the power, 2014-02-22 08:15:44]

Change committed as 252773