Disable the touch ack timeout for touch-action:none

Disable the touch ack timeout for touch-action:none

When the touch-action for the current touch sequence is updated, also update
whether the touch ack time is disabled based on the allowed touch-action. This
provides a straightforward knob for developers to disable the timeout on desktop
sites.

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

[jdduke (slow), 2014-03-05 17:53:22]

rbyers@: PTAL.

[Rick Byers, 2014-03-05 18:31:06]

Thanks Jared!  This looks pretty good but I don't think it quite has the
behavior you intend.

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

https://codereview.chromium.org/183923034/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/input/input_router_impl.cc:561: 
Note that this is only called for a new touch sequence when that sequence should
have a touch-action other than auto.  In your current implementation this means
that once a user touches a touch-action: none region, it'll disable the timeout
for that touch sequence and all future touch sequences (unless they touch
something with a touch-action other than auto or none).

You could update the timeout on the start of each touch sequence, but perhaps we
should consider going a little further here (since we likely won't have
touch-action hit testing on the impl thread any time soon).  If the user just
touched something with touch-action: none (indicating the page is consuming
events) then it seems likely that the page will want to consume events for the
next touch too.  So perhaps we SHOULD leave the timeout disabled for at _least_
one more touch sequence.  touch-action is also a strong signal that the page has
been designed with mobile scenarios in mind, so this could be a good heuristic
similar to the mobile viewport (although I guess you could say the same about a
touchstart handler that consumes the touch events).

https://codereview.chromium.org/183923034/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/input/input_router_impl.cc:787: 
I suggest adding a comment to each line justifying the reasoning.  Eg. for
TOUCH_ACTION_NONE it's something like "scrolling is disabled for this touch
sequence, no benefit to the timeout and receiving events is likely critical to
the app".

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

https://codereview.chromium.org/183923034/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/input/input_router_impl_unittest.cc:1037: 
I don't think we ever explicitly set the touch-action to AUTO (it's implicit at
the start of a sequence).  So a more realistic test would 1) check the state for
a new touch without setting the touch-action, and 2) set the touch-action to
something else like PAN_Y and verify that the timeout is still enabled.

[jdduke (slow), 2014-03-05 19:20:39]

On 2014/03/05 18:31:06, Rick Byers wrote:
> Thanks Jared!  This looks pretty good but I don't think it quite has the
> behavior you intend.
> 
>
https://codereview.chromium.org/183923034/diff/1/content/browser/renderer_hos...
> File content/browser/renderer_host/input/input_router_impl.cc (right):
> 
>
https://codereview.chromium.org/183923034/diff/1/content/browser/renderer_hos...
> content/browser/renderer_host/input/input_router_impl.cc:561: 
> Note that this is only called for a new touch sequence when that sequence
should
> have a touch-action other than auto.  In your current implementation this
means
> that once a user touches a touch-action: none region, it'll disable the
timeout
> for that touch sequence and all future touch sequences (unless they touch
> something with a touch-action other than auto or none).
> 
> You could update the timeout on the start of each touch sequence, but perhaps
we
> should consider going a little further here (since we likely won't have
> touch-action hit testing on the impl thread any time soon).  If the user just
> touched something with touch-action: none (indicating the page is consuming
> events) then it seems likely that the page will want to consume events for the
> next touch too.  So perhaps we SHOULD leave the timeout disabled for at
_least_
> one more touch sequence.  touch-action is also a strong signal that the page
has
> been designed with mobile scenarios in mind, so this could be a good heuristic
> similar to the mobile viewport (although I guess you could say the same about
a
> touchstart handler that consumes the touch events).

I see, of course.  The code will update the timeout now when the touch action is
reset.  The side-effect here is that the TouchStart of a touch sequence
following a TOUCH_ACTION_NONE will *never* activate the timeout, as the timeout
is enabled just *after* the touch has passed through the TouchEventQueue.  

The redeeming side-effect here is that the second touch sequence has a clean
opportunity to repeat the TOUCH_ACTION_NONE (during its TouchStart) without
suffering from the timeout! So I think it's a worthwhile tradeoff for the time
being.

>
https://codereview.chromium.org/183923034/diff/1/content/browser/renderer_hos...
> content/browser/renderer_host/input/input_router_impl.cc:787: 
> I suggest adding a comment to each line justifying the reasoning.  Eg. for
> TOUCH_ACTION_NONE it's something like "scrolling is disabled for this touch
> sequence, no benefit to the timeout and receiving events is likely critical to
> the app".

Done.

>
https://codereview.chromium.org/183923034/diff/1/content/browser/renderer_hos...
> File content/browser/renderer_host/input/input_router_impl_unittest.cc
(right):
> 
>
https://codereview.chromium.org/183923034/diff/1/content/browser/renderer_hos...
> content/browser/renderer_host/input/input_router_impl_unittest.cc:1037: 
> I don't think we ever explicitly set the touch-action to AUTO (it's implicit
at
> the start of a sequence).  So a more realistic test would 1) check the state
for
> a new touch without setting the touch-action, and 2) set the touch-action to
> something else like PAN_Y and verify that the timeout is still enabled.

Done.

[Rick Byers, 2014-03-05 21:24:22]

LGTM modulo an additional test (if it's not too hard).

https://codereview.chromium.org/183923034/diff/70001/content/browser/renderer...
File content/browser/renderer_host/input/input_router_impl.cc (right):

https://codereview.chromium.org/183923034/diff/70001/content/browser/renderer...
content/browser/renderer_host/input/input_router_impl.cc:793: // little purpose.
 It's also a strong signal that touch handling is critical
nit: I believe chromium checkstyle will complain about the double-space
following a period (at least it ALWAYS does for me - drives me nuts!).  Are you
running presubmit with the style checks?

https://codereview.chromium.org/183923034/diff/70001/content/browser/renderer...
File content/browser/renderer_host/input/input_router_impl_unittest.cc (right):

https://codereview.chromium.org/183923034/diff/70001/content/browser/renderer...
content/browser/renderer_host/input/input_router_impl_unittest.cc:1039: //
meaningful effect until the following touch event is sent).
We both agree we like this property.  Perhaps make it explicit by finding a way
to test it (and maybe add a comment in the implementation saying it's
intentional)?  At least until we have impl thread touch-action hit testing, I
think it's a property worth preserving...

[jdduke (slow), 2014-03-06 00:15:24]

https://codereview.chromium.org/183923034/diff/70001/content/browser/renderer...
File content/browser/renderer_host/input/input_router_impl.cc (right):

https://codereview.chromium.org/183923034/diff/70001/content/browser/renderer...
content/browser/renderer_host/input/input_router_impl.cc:793: // little purpose.
 It's also a strong signal that touch handling is critical
On 2014/03/05 21:24:23, Rick Byers wrote:
> nit: I believe chromium checkstyle will complain about the double-space
> following a period (at least it ALWAYS does for me - drives me nuts!).  Are
you
> running presubmit with the style checks?

Done.

https://codereview.chromium.org/183923034/diff/70001/content/browser/renderer...
File content/browser/renderer_host/input/input_router_impl_unittest.cc (right):

https://codereview.chromium.org/183923034/diff/70001/content/browser/renderer...
content/browser/renderer_host/input/input_router_impl_unittest.cc:1039: //
meaningful effect until the following touch event is sent).
On 2014/03/05 21:24:23, Rick Byers wrote:
> We both agree we like this property.  Perhaps make it explicit by finding a
way
> to test it (and maybe add a comment in the implementation saying it's
> intentional)?  At least until we have impl thread touch-action hit testing, I
> think it's a property worth preserving...

Definitely, added a test.

[jdduke (slow), 2014-03-06 00:15:31]

The CQ bit was checked by jdduke@chromium.org

[commit-bot: I haz the power, 2014-03-06 00:26:02]

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

[commit-bot: I haz the power, 2014-03-06 02:52:04]

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

[commit-bot: I haz the power, 2014-03-06 03:16:47]

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

[commit-bot: I haz the power, 2014-03-06 10:48:14]

Change committed as 255304