Stricter user gestures for touch - measure and warn

Stricter user gestures for touch - measure and warn

Add UserGestureIndicator infrastructure for being notified when the state of a UserGestureIndicator is used.  Use this infrastructure to get an idea of the potential impact of the "stricter user gestures for touch" intervention, and warn developers about it.

See https://docs.google.com/document/d/1oF1T3O7_E4t1PYHV6gyCwHxOi3ystm0eSL5xZu7nvOg/edit#

BUG=582140
Committed: https://crrev.com/fb38d26a84cfab930dfb53a2f338534aa0ef3100
Cr-Commit-Position: refs/heads/master@{#385826}

[jochen (gone - plz use gerrit), 2016-03-15 20:01:05]

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

[jochen (gone - plz use gerrit), 2016-03-15 20:01:06]

generally looks good

https://codereview.chromium.org/1799253002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebUserGestureIndicator.cpp (right):

https://codereview.chromium.org/1799253002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebUserGestureIndicator.cpp:40: return
UserGestureIndicator::processingUserGestureUntracked();
ppapi calls this API to do some checks that "count", in
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/p...

[Rick Byers, 2016-03-16 18:24:44]

https://codereview.chromium.org/1799253002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebUserGestureIndicator.cpp (right):

https://codereview.chromium.org/1799253002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebUserGestureIndicator.cpp:40: return
UserGestureIndicator::processingUserGestureUntracked();
On 2016/03/15 20:01:06, jochen wrote:
> ppapi calls this API to do some checks that "count", in
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/p...

So do you think it's worth adding tracked/untracked variants of the
WebUserGestureIndicator API for that?  There's still a bunch of deferred cases
like the setTimeout handling which (unless you feel strongly) I wasn't planning
on trying to handle (since that would require moving the callback to the
GestureToken which would be more complex).

[jochen (gone - plz use gerrit), 2016-03-17 19:58:47]

On 2016/03/16 at 18:24:44, rbyers wrote:
>
https://codereview.chromium.org/1799253002/diff/1/third_party/WebKit/Source/w...
> File third_party/WebKit/Source/web/WebUserGestureIndicator.cpp (right):
> 
>
https://codereview.chromium.org/1799253002/diff/1/third_party/WebKit/Source/w...
> third_party/WebKit/Source/web/WebUserGestureIndicator.cpp:40: return
UserGestureIndicator::processingUserGestureUntracked();
> On 2016/03/15 20:01:06, jochen wrote:
> > ppapi calls this API to do some checks that "count", in
> >
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/p...
> 
> So do you think it's worth adding tracked/untracked variants of the
WebUserGestureIndicator API for that?  There's still a bunch of deferred cases
like the setTimeout handling which (unless you feel strongly) I wasn't planning
on trying to handle (since that would require moving the callback to the
GestureToken which would be more complex).

depends on how much coverage you want to have in the end.

Since you want to intervene on popups, having the warning shown when trying to
open a window sounds useful?

[Rick Byers, 2016-04-04 17:24:53]

Description was changed from

==========
WIP: Preparations for stricter user gestures for touch

Very much still work-in-progress, not ready for full review.

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

BUG=582140
==========

to

==========
Stricter user gestures for touch - measure and warn

Add UserGestureIndicator infrastructure for being notified when the state of a
UserGestureIndicator is used.  Use this infrastructure to get an idea of the
potential impact of the "stricter user gestures for touch" intervention, and
warn developers about it.

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

BUG=582140
==========

[Rick Byers, 2016-04-04 17:24:53]

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

[Rick Byers, 2016-04-04 20:31:12]

On 2016/03/17 19:58:47, jochen wrote:
> On 2016/03/16 at 18:24:44, rbyers wrote:
> >
>
https://codereview.chromium.org/1799253002/diff/1/third_party/WebKit/Source/w...
> > File third_party/WebKit/Source/web/WebUserGestureIndicator.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1799253002/diff/1/third_party/WebKit/Source/w...
> > third_party/WebKit/Source/web/WebUserGestureIndicator.cpp:40: return
> UserGestureIndicator::processingUserGestureUntracked();
> > On 2016/03/15 20:01:06, jochen wrote:
> > > ppapi calls this API to do some checks that "count", in
> > >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/p...
> > 
> > So do you think it's worth adding tracked/untracked variants of the
> WebUserGestureIndicator API for that?  There's still a bunch of deferred cases
> like the setTimeout handling which (unless you feel strongly) I wasn't
planning
> on trying to handle (since that would require moving the callback to the
> GestureToken which would be more complex).
> 
> depends on how much coverage you want to have in the end.
> 
> Since you want to intervene on popups, having the warning shown when trying to
> open a window sounds useful?

I've done some more digging and manual testing and this actually looks like it
should be ok.  All the window opening cases I can see go down the consumeGesture
path and so trigger the measurement/warning.  I'll do a bit more exhaustive
testing on device, but can you take another look?

+dtapuska for EventHandler.*
jochen for everything else (UserGestureIndicator plumbing).

[dtapuska, 2016-04-06 16:26:57]

lgtm % an additional comment for clarity.

https://codereview.chromium.org/1799253002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1799253002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:3848: // This is a
touchend corresponding to a tap, definitely a user gesture.
I think this might need a comment that the callback is explicitly not set as to
avoid reporting UMA metrics in this code path.

[Rick Byers, 2016-04-06 16:31:28]

On 2016/04/06 16:26:57, dtapuska wrote:
> lgtm % an additional comment for clarity.
> 
>
https://codereview.chromium.org/1799253002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/input/EventHandler.cpp (right):
> 
>
https://codereview.chromium.org/1799253002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/input/EventHandler.cpp:3848: // This is a
> touchend corresponding to a tap, definitely a user gesture.
> I think this might need a comment that the callback is explicitly not set as
to
> avoid reporting UMA metrics in this code path.

Done thanks.

[Rick Byers, 2016-04-06 16:31:58]

+rkaplow for metrics OWNERS

[Rick Byers, 2016-04-06 16:32:12]

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

[rkaplow, 2016-04-06 19:53:58]

lgtm

histograms lgtm

[Rick Byers, 2016-04-07 01:40:47]

On 2016/04/04 20:31:12, Rick Byers wrote:
> On 2016/03/17 19:58:47, jochen wrote:
> > On 2016/03/16 at 18:24:44, rbyers wrote:
> > >
> >
>
https://codereview.chromium.org/1799253002/diff/1/third_party/WebKit/Source/w...
> > > File third_party/WebKit/Source/web/WebUserGestureIndicator.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1799253002/diff/1/third_party/WebKit/Source/w...
> > > third_party/WebKit/Source/web/WebUserGestureIndicator.cpp:40: return
> > UserGestureIndicator::processingUserGestureUntracked();
> > > On 2016/03/15 20:01:06, jochen wrote:
> > > > ppapi calls this API to do some checks that "count", in
> > > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/p...
> > > 
> > > So do you think it's worth adding tracked/untracked variants of the
> > WebUserGestureIndicator API for that?  There's still a bunch of deferred
cases
> > like the setTimeout handling which (unless you feel strongly) I wasn't
> planning
> > on trying to handle (since that would require moving the callback to the
> > GestureToken which would be more complex).
> > 
> > depends on how much coverage you want to have in the end.
> > 
> > Since you want to intervene on popups, having the warning shown when trying
to
> > open a window sounds useful?
> 
> I've done some more digging and manual testing and this actually looks like it
> should be ok.  All the window opening cases I can see go down the
consumeGesture
> path and so trigger the measurement/warning.  I'll do a bit more exhaustive
> testing on device, but can you take another look?

I've done more exhaustive testing on device and I'm happy with this.  However I
realized the way I had the code was confusing and brittle
("processingUserGesture" triggered the Use callback in blink but not outside,
etc.).  In particular I'm nervous about people accidentally adding new code that
triggers the callback in scenarios where they're not necessarily depending on
the value.  I'd rather err on the side of missing coverage of some cases (as is
already the case) than risk false-positives.

To this end I've cleaned things up by giving UserGestureIndicator 3 distinct
operations:
 - processingUserGesture - strictly non-side-effecting, like before
 - utilizeUserGesture - invoke the callback
 - consumeUserGesture - invoke the callback and prevent it from being used again
(like before)

I've gone through and changed processingUserGesture() to utilizeUserGesture()
where it's appropriate and possible without non-trivial architectural changes.

dtapuska: jochen is travelling and AFAICT he's the only one with experience with
this infrastructure (other than me).  Can you please review the whole patch in
his absence?  I'd like to land this for M51.  

jochen: after you're available I'm happy to apply any feedback you have in a
follow-up patch.

[jochen (gone - plz use gerrit), 2016-04-07 03:37:56]

lgtm

https://codereview.chromium.org/1799253002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/UserGestureIndicator.h (right):

https://codereview.chromium.org/1799253002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/UserGestureIndicator.h:67:
UserGestureUtilizedCallback() { }
we don't need a ctor for an interface, no?

https://codereview.chromium.org/1799253002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/UserGestureIndicator.h:68: virtual
~UserGestureUtilizedCallback() { }
= default; ?

https://codereview.chromium.org/1799253002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/UserGestureIndicator.h:101: explicit
UserGestureIndicator(ProcessingUserGestureState, UserGestureUtilizedCallback*
usageCallback = 0);
omit "usageCallback" (also next line)

[Rick Byers, 2016-04-07 17:51:31]

Thanks Jochen!

https://codereview.chromium.org/1799253002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/UserGestureIndicator.h (right):

https://codereview.chromium.org/1799253002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/UserGestureIndicator.h:67:
UserGestureUtilizedCallback() { }
On 2016/04/07 03:37:56, jochen - slow wrote:
> we don't need a ctor for an interface, no?

Yep, we can use the implicit default ctor by also removing the
WTF_MAKE_NONCOPYABLE (which shouldn't matter for an interface).

https://codereview.chromium.org/1799253002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/UserGestureIndicator.h:68: virtual
~UserGestureUtilizedCallback() { }
On 2016/04/07 03:37:56, jochen - slow wrote:
> = default; ?

Done.

https://codereview.chromium.org/1799253002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/UserGestureIndicator.h:101: explicit
UserGestureIndicator(ProcessingUserGestureState, UserGestureUtilizedCallback*
usageCallback = 0);
On 2016/04/07 03:37:56, jochen - slow wrote:
> omit "usageCallback" (also next line)

Done.

[Rick Byers, 2016-04-07 17:51:38]

The CQ bit was checked by rbyers@chromium.org

[Rick Byers, 2016-04-07 17:51:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtapuska@chromium.org, rkaplow@chromium.org, jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/1799253002/#ps100001
(title: "Tweaks")

[commit-bot: I haz the power, 2016-04-07 17:52:11]

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

[commit-bot: I haz the power, 2016-04-07 19:08:35]

Description was changed from

==========
Stricter user gestures for touch - measure and warn

Add UserGestureIndicator infrastructure for being notified when the state of a
UserGestureIndicator is used.  Use this infrastructure to get an idea of the
potential impact of the "stricter user gestures for touch" intervention, and
warn developers about it.

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

BUG=582140
==========

to

==========
Stricter user gestures for touch - measure and warn

Add UserGestureIndicator infrastructure for being notified when the state of a
UserGestureIndicator is used.  Use this infrastructure to get an idea of the
potential impact of the "stricter user gestures for touch" intervention, and
warn developers about it.

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

BUG=582140
==========

[commit-bot: I haz the power, 2016-04-07 19:08:38]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-04-07 19:15:00]

Description was changed from

==========
Stricter user gestures for touch - measure and warn

Add UserGestureIndicator infrastructure for being notified when the state of a
UserGestureIndicator is used.  Use this infrastructure to get an idea of the
potential impact of the "stricter user gestures for touch" intervention, and
warn developers about it.

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

BUG=582140
==========

to

==========
Stricter user gestures for touch - measure and warn

Add UserGestureIndicator infrastructure for being notified when the state of a
UserGestureIndicator is used.  Use this infrastructure to get an idea of the
potential impact of the "stricter user gestures for touch" intervention, and
warn developers about it.

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

BUG=582140

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

[commit-bot: I haz the power, 2016-04-07 19:15:02]

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