Deprecate use of user gestures during scroll-related touch events

Deprecate use of user gestures during scroll-related touch events

See https://docs.google.com/document/d/1oF1T3O7_E4t1PYHV6gyCwHxOi3ystm0eSL5xZu7nvOg/edit#heading=h.qq59ev3u8fba

Intent to deprecate: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/2cvwbgZOZZs

This also contains a small tweak to the semantics of the TouchScrollStarted event to trigger it only on the first GestureScrollUpdate (so that it's not triggered when the page consumes all touchmove events).

BUG=611981
Committed: https://crrev.com/adbb8a0133bf42306ac4a533a5478529e5d87895
Cr-Commit-Position: refs/heads/master@{#396215}

[Rick Byers, 2016-05-19 20:16:54]

Description was changed from

==========
Deprecate use of user gestures during scroll-related touch events

See
https://docs.google.com/document/d/1oF1T3O7_E4t1PYHV6gyCwHxOi3ystm0eSL5xZu7nv...

BUG=611981
==========

to

==========
Deprecate use of user gestures during scroll-related touch events

See
https://docs.google.com/document/d/1oF1T3O7_E4t1PYHV6gyCwHxOi3ystm0eSL5xZu7nv...

This also contains a small tweak to the semantics of the TouchScrollStarted
event to trigger it only on the first GestureScrollUpdate (so that it's not
triggered when the page consumes all touchmove events).

BUG=611981
==========

[Rick Byers, 2016-05-19 20:16:54]

rbyers@chromium.org changed reviewers:
+ dtapuska@chromium.org

[Rick Byers, 2016-05-19 20:19:50]

rbyers@chromium.org changed reviewers:
+ mustaq@chromium.org

[Rick Byers, 2016-05-19 20:19:51]

Dave / Mustaq PTAL

The TouchScrollStarted change arguably could be split into a separate CL - let
me know if you'd prefer that.  It does subtly change the pointercancel behavior
(doesn't send pointercancel as long as the page is consuming touchmoves).  I
considered writing a test for that but it looks like we don't have any test that
covers the full behavior here.  Ideally I think this is the sort of thing we
should cover with an end-to-end test (perhaps even in web-platform-tests), but I
don't think Navid/Lan have the infrastructure for that quite ready yet.  So
perhaps just one more test to add to the list of pointer events tests we want to
write once we can automate them properly?

[Rick Byers, 2016-05-19 20:46:38]

Description was changed from

==========
Deprecate use of user gestures during scroll-related touch events

See
https://docs.google.com/document/d/1oF1T3O7_E4t1PYHV6gyCwHxOi3ystm0eSL5xZu7nv...

This also contains a small tweak to the semantics of the TouchScrollStarted
event to trigger it only on the first GestureScrollUpdate (so that it's not
triggered when the page consumes all touchmove events).

BUG=611981
==========

to

==========
Deprecate use of user gestures during scroll-related touch events

See
https://docs.google.com/document/d/1oF1T3O7_E4t1PYHV6gyCwHxOi3ystm0eSL5xZu7nv...

Intent to deprecate:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/2cvwbgZOZZs

This also contains a small tweak to the semantics of the TouchScrollStarted
event to trigger it only on the first GestureScrollUpdate (so that it's not
triggered when the page consumes all touchmove events).

BUG=611981
==========

[dtapuska, 2016-05-24 14:54:28]

lgtm % nits

https://codereview.chromium.org/1996143002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/input/TouchEventManager.cpp (right):

https://codereview.chromium.org/1996143002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/input/TouchEventManager.cpp:490:
CurrentEventHolder holder(m_currentEvent, event.type());
Can we not use base/auto_reset.h ? base/macros.h is already permitted.

https://codereview.chromium.org/1996143002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/input/TouchEventManager.h (right):

https://codereview.chromium.org/1996143002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/input/TouchEventManager.h:70: // Indicate that a
touchs scroll has started
touchs?

https://codereview.chromium.org/1996143002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/input/TouchEventManager.h:73: // Invoked when a
UserGestureIndicator corresponding to a touch event is utilized
Comments should terminate with periods :-) You seem to mix this between two
styles.

[mustaq, 2016-05-24 15:54:06]

LGTM but wanted to highlight the pointercancel delay in case it has some
consequences I don't see...

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

https://codereview.chromium.org/1996143002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/input/input_router_impl.cc:174:
touch_event_queue_.PrependTouchScrollNotification();
Originally when PrependTouchScrollNotification() was called, we used to have an
in-flight touchmove corresponding to the first touchmove beyond the slop region.
Now that the TSS event is delayed by one step, am I right that the pointercancel
event on a touch-action:none region will be seen after 2 touchmoves? What if a
2nd finger touchstart happens between the two touchmoves, so there will be no
GSU hence no TSS?

These seem insignificant but wanted to call out in case they may trigger some
observable regression somewhere.

https://codereview.chromium.org/1996143002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/input/TouchEventManager.cpp (right):

https://codereview.chromium.org/1996143002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/input/TouchEventManager.cpp:535: // These are
cases we'd like to migrate to not hold a user gesture.
The two comment lines seem to describe separate points. May be move the move
line to before |if|, like "Set the callback for the cases...".

[Rick Byers, 2016-05-24 20:29:21]

Thanks guys.

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

https://codereview.chromium.org/1996143002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/input/input_router_impl.cc:174:
touch_event_queue_.PrependTouchScrollNotification();
On 2016/05/24 15:54:05, mustaq wrote:
> Originally when PrependTouchScrollNotification() was called, we used to have
an
> in-flight touchmove corresponding to the first touchmove beyond the slop
region.
> Now that the TSS event is delayed by one step, am I right that the
pointercancel
> event on a touch-action:none region will be seen after 2 touchmoves? What if a
> 2nd finger touchstart happens between the two touchmoves, so there will be no
> GSU hence no TSS?
> 
> These seem insignificant but wanted to call out in case they may trigger some
> observable regression somewhere.

No, in the common case the first GestureScrollUpdate follows immediately the
GestureScrollBegin (both generated from the same touchmove event).  See
https://code.google.com/p/chromium/codesearch#chromium/src/ui/events/gesture_....
 So there's nothing that should ever happen between the GestureScrollBegin and
the first GestureScrollUpdate.

Also note that in the touch-action: none case there is no TSS / pointercancel
(because no GestureScrollBegin/Update is sent to the renderer at all - they're
entirely filtered out by the TouchActionGestureFilter).

https://codereview.chromium.org/1996143002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/input/TouchEventManager.cpp (right):

https://codereview.chromium.org/1996143002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/input/TouchEventManager.cpp:490:
CurrentEventHolder holder(m_currentEvent, event.type());
On 2016/05/24 14:54:27, dtapuska wrote:
> Can we not use base/auto_reset.h ? base/macros.h is already permitted.

macros.h is the only include from base we currently permit in Source/core and I
know the architecture team wants to be deliberate about adding more.

I agree that auto_reset should be trivial though (it doesn't pull in anything
else other than macros.h).  Added a TODO for now, but /cc haraken - I'm happy to
switch to AutoReset if he's OK with me adding it to Source/DEPS.

https://codereview.chromium.org/1996143002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/input/TouchEventManager.cpp:535: // These are
cases we'd like to migrate to not hold a user gesture.
On 2016/05/24 15:54:06, mustaq wrote:
> The two comment lines seem to describe separate points. May be move the move
> line to before |if|, like "Set the callback for the cases...".

Done.

https://codereview.chromium.org/1996143002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/input/TouchEventManager.h (right):

https://codereview.chromium.org/1996143002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/input/TouchEventManager.h:70: // Indicate that a
touchs scroll has started
On 2016/05/24 14:54:27, dtapuska wrote:
> touchs?

Done.

[Rick Byers, 2016-05-25 17:45:09]

The CQ bit was checked by rbyers@chromium.org

[Rick Byers, 2016-05-25 17:45:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtapuska@chromium.org, mustaq@chromium.org
Link to the patchset: https://codereview.chromium.org/1996143002/#ps40001
(title: "Fix test")

[commit-bot: I haz the power, 2016-05-25 17:45:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1996143002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1996143002/40001

[commit-bot: I haz the power, 2016-05-25 17:54:57]

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

[commit-bot: I haz the power, 2016-05-25 17:54:58]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Rick Byers, 2016-05-25 18:29:17]

rbyers@chromium.org changed reviewers:
+ tdresser@chromium.org

[Rick Byers, 2016-05-25 18:29:18]

+tdresser for OWNERS in content/browser/renderer_host/input/

TBR rkaplow for histograms.xml.  Robert, I assume it's silly to block on OWNERS
review of histograms.xml when all we're doing is adding more entries to a blink
UseCounter, right?  It's all script generated, nothing really to review I think?
 I'm happy to block if you prefer of course.

[rkaplow, 2016-05-25 18:30:49]

On 2016/05/25 18:29:18, Rick Byers wrote:
> +tdresser for OWNERS in content/browser/renderer_host/input/
> 
> TBR rkaplow for histograms.xml.  Robert, I assume it's silly to block on
OWNERS
> review of histograms.xml when all we're doing is adding more entries to a
blink
> UseCounter, right?  It's all script generated, nothing really to review I
think?
>  I'm happy to block if you prefer of course.

we tend to be pretty fast. But I don't really mind TBR in cases like this.

[rkaplow, 2016-05-25 18:30:56]

rkaplow@chromium.org changed reviewers:
+ rkaplow@chromium.org

[rkaplow, 2016-05-25 18:30:56]

lgtm

[Rick Byers, 2016-05-25 18:37:36]

On 2016/05/25 18:30:49, rkaplow wrote:
> On 2016/05/25 18:29:18, Rick Byers wrote:
> > +tdresser for OWNERS in content/browser/renderer_host/input/
> > 
> > TBR rkaplow for histograms.xml.  Robert, I assume it's silly to block on
> OWNERS
> > review of histograms.xml when all we're doing is adding more entries to a
> blink
> > UseCounter, right?  It's all script generated, nothing really to review I
> think?
> >  I'm happy to block if you prefer of course.
> 
> we tend to be pretty fast. But I don't really mind TBR in cases like this.

Indeed - 2 minute turn around, thanks! :-)

[tdresser, 2016-05-26 15:01:28]

content/browser/renderer_host/input/ LGTM

[Rick Byers, 2016-05-26 15:02:28]

The CQ bit was checked by rbyers@chromium.org

[commit-bot: I haz the power, 2016-05-26 15:02:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1996143002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1996143002/40001

[commit-bot: I haz the power, 2016-05-26 15:04:49]

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

[commit-bot: I haz the power, 2016-05-26 15:04:51]

Try jobs failed on following builders:
  ios-device on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[Rick Byers, 2016-05-26 15:16:27]

The CQ bit was checked by rbyers@chromium.org

[Rick Byers, 2016-05-26 15:16:28]

The patchset sent to the CQ was uploaded after l-g-t-m from mustaq@chromium.org,
rkaplow@chromium.org, tdresser@chromium.org, dtapuska@chromium.org
Link to the patchset: https://codereview.chromium.org/1996143002/#ps60001
(title: "Merge with trunk")

[commit-bot: I haz the power, 2016-05-26 15:16:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1996143002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1996143002/60001

[commit-bot: I haz the power, 2016-05-26 17:33:17]

Description was changed from

==========
Deprecate use of user gestures during scroll-related touch events

See
https://docs.google.com/document/d/1oF1T3O7_E4t1PYHV6gyCwHxOi3ystm0eSL5xZu7nv...

Intent to deprecate:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/2cvwbgZOZZs

This also contains a small tweak to the semantics of the TouchScrollStarted
event to trigger it only on the first GestureScrollUpdate (so that it's not
triggered when the page consumes all touchmove events).

BUG=611981
==========

to

==========
Deprecate use of user gestures during scroll-related touch events

See
https://docs.google.com/document/d/1oF1T3O7_E4t1PYHV6gyCwHxOi3ystm0eSL5xZu7nv...

Intent to deprecate:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/2cvwbgZOZZs

This also contains a small tweak to the semantics of the TouchScrollStarted
event to trigger it only on the first GestureScrollUpdate (so that it's not
triggered when the page consumes all touchmove events).

BUG=611981
==========

[commit-bot: I haz the power, 2016-05-26 17:33:19]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-05-26 17:34:29]

Description was changed from

==========
Deprecate use of user gestures during scroll-related touch events

See
https://docs.google.com/document/d/1oF1T3O7_E4t1PYHV6gyCwHxOi3ystm0eSL5xZu7nv...

Intent to deprecate:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/2cvwbgZOZZs

This also contains a small tweak to the semantics of the TouchScrollStarted
event to trigger it only on the first GestureScrollUpdate (so that it's not
triggered when the page consumes all touchmove events).

BUG=611981
==========

to

==========
Deprecate use of user gestures during scroll-related touch events

See
https://docs.google.com/document/d/1oF1T3O7_E4t1PYHV6gyCwHxOi3ystm0eSL5xZu7nv...

Intent to deprecate:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/2cvwbgZOZZs

This also contains a small tweak to the semantics of the TouchScrollStarted
event to trigger it only on the first GestureScrollUpdate (so that it's not
triggered when the page consumes all touchmove events).

BUG=611981

Committed: https://crrev.com/adbb8a0133bf42306ac4a533a5478529e5d87895
Cr-Commit-Position: refs/heads/master@{#396215}
==========

[commit-bot: I haz the power, 2016-05-26 17:34:30]

Patchset 4 (id:??) landed as
https://crrev.com/adbb8a0133bf42306ac4a533a5478529e5d87895
Cr-Commit-Position: refs/heads/master@{#396215}