UserGestureIndicator is a mess. Clean it up.

UserGestureIndicator is a mess. Clean it up.

Currently are two constructors for a UserGestureIndicator, one that takes a
ProcessingUserGestureState enum value and one that takes a
PassRefPtr<UserGestureToken>.

There are three ways to construct a UserGestureIndicator that does nothing:
1. Pass a nullptr to the PassRefPtr<UserGestureToken> constructor.
2. Pass PossiblyProcessingUserGesture to the enum constructor.
3. Pass DefinitelyNotProcessingUserGesture when not alredy processing a user
    gesture (this is in fact the only way it is currently used).
(2) and (3) are only used in one place each, so remove those enum values and use
the other constructor with a nullptr or simply don't construct a
UserGestureIndicator.

UserGestureToken is a pure virtual interface for reasons that aren't clear.
Collapse its implementation in to the base class.

With 2 ProcessingUserGestureState values removed, the only purpose of this enum is
during UserGestureToken initialization: determining whether it definitely starts
with m_consumableGestures == 1, or whether it starts at 1 only if there are no
other UserGestureTokens on the stack. Move this initialization logic in to the
UserGestureToken constructor, generally simplify UserGestureToken's surface, and
remove the enum-based UserGestureIndicator constructor. Callers are now expected
to pass in a PassRefPtr<UserGestureToken> on UserGestureIndicator initialization.
UserGestureToken::create() now takes an enum similar to the old
ProcessingUserGestureState, with values equivalent to
DefinitelyProcessingNewUserGesture and DefinitelyProcessingUserGesture.

BUG=

Committed: https://crrev.com/046115dc4c7cff68b07ad14f426388d86463aa22
Cr-Commit-Position: refs/heads/master@{#424552}

[Nate Chapin, 2016-10-07 19:41:50]

The CQ bit was checked by japhet@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-07 19:42:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-07 19:45:21]

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

[commit-bot: I haz the power, 2016-10-07 19:45:22]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Nate Chapin, 2016-10-07 19:59:56]

The CQ bit was checked by japhet@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-07 20:00:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-07 20:02:59]

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

[commit-bot: I haz the power, 2016-10-07 20:03:00]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Nate Chapin, 2016-10-10 18:07:53]

The CQ bit was checked by japhet@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-10 18:08:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Nate Chapin, 2016-10-10 21:15:34]

The CQ bit was checked by japhet@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-10 21:16:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Nate Chapin, 2016-10-10 21:37:56]

Description was changed from

==========
UserGestureIndicator is a mess. Clean it up.

BUG=
==========

to

==========
UserGestureIndicator is a mess. Clean it up.

Currently are two constructors for a UserGestureIndicator,
one that takes a ProcessingUserGestureState state and one
that takes a PassRefPtr<UserGestureToken>. Both take an
optional UserGestureUtilizedCallback*.

There are three ways to construct a UserGestureIndicator
that does nothing: (1) Pass a nullptr to the
PassRefPtr<UserGestureToken> constructor, (2) Pass 
PossiblyProcessingUserGesture to the enum constructor, or
(3) Pass DefinitelyNotProcessingUserGesture when not
processing a user gesture (this is in fact the only way
DefinitelyNotProcessingUserGesture is used). (2) and (3)
are only used in one place each, so remove those enum
values and use the other constructor with a nullptr or
simply don't construct a UserGestureIndicator.

BUG=
==========

[Nate Chapin, 2016-10-10 21:42:28]

Description was changed from

==========
UserGestureIndicator is a mess. Clean it up.

Currently are two constructors for a UserGestureIndicator,
one that takes a ProcessingUserGestureState state and one
that takes a PassRefPtr<UserGestureToken>. Both take an
optional UserGestureUtilizedCallback*.

There are three ways to construct a UserGestureIndicator
that does nothing: (1) Pass a nullptr to the
PassRefPtr<UserGestureToken> constructor, (2) Pass 
PossiblyProcessingUserGesture to the enum constructor, or
(3) Pass DefinitelyNotProcessingUserGesture when not
processing a user gesture (this is in fact the only way
DefinitelyNotProcessingUserGesture is used). (2) and (3)
are only used in one place each, so remove those enum
values and use the other constructor with a nullptr or
simply don't construct a UserGestureIndicator.

BUG=
==========

to

==========
UserGestureIndicator is a mess. Clean it up.

Currently are two constructors for a UserGestureIndicator, one that takes a
ProcessingUserGestureState state and one that takes a
PassRefPtr<UserGestureToken>. Both take an optional
UserGestureUtilizedCallback*.

There are three ways to construct a UserGestureIndicator that does nothing:
(1) Pass a nullptr to the PassRefPtr<UserGestureToken> constructor,
(2) Pass PossiblyProcessingUserGesture to the enum constructor, or
(3) Pass DefinitelyNotProcessingUserGesture when not processing a user gesture
    (this is in fact the only way DefinitelyNotProcessingUserGesture is used).
(2) and (3) are only used in one place each, so remove those enum values and use
the other constructor with a nullptr or simply don't construct a
UserGestureIndicator.

BUG=
==========

[Nate Chapin, 2016-10-10 21:42:48]

Description was changed from

==========
UserGestureIndicator is a mess. Clean it up.

Currently are two constructors for a UserGestureIndicator, one that takes a
ProcessingUserGestureState state and one that takes a
PassRefPtr<UserGestureToken>. Both take an optional
UserGestureUtilizedCallback*.

There are three ways to construct a UserGestureIndicator that does nothing:
(1) Pass a nullptr to the PassRefPtr<UserGestureToken> constructor,
(2) Pass PossiblyProcessingUserGesture to the enum constructor, or
(3) Pass DefinitelyNotProcessingUserGesture when not processing a user gesture
    (this is in fact the only way DefinitelyNotProcessingUserGesture is used).
(2) and (3) are only used in one place each, so remove those enum values and use
the other constructor with a nullptr or simply don't construct a
UserGestureIndicator.

BUG=
==========

to

==========
UserGestureIndicator is a mess. Clean it up.

Currently are two constructors for a UserGestureIndicator, one that takes a
ProcessingUserGestureState state and one that takes a
PassRefPtr<UserGestureToken>. Both take an optional
UserGestureUtilizedCallback*.

There are three ways to construct a UserGestureIndicator that does nothing:
(1) Pass a nullptr to the PassRefPtr<UserGestureToken> constructor,
(2) Pass PossiblyProcessingUserGesture to the enum constructor, or
(3) Pass DefinitelyNotProcessingUserGesture when not processing a user gesture
    (this is in fact the only way DefinitelyNotProcessingUserGesture is used).
(2) and (3) are only used in one place each, so remove those enum values and use
the other constructor with a nullptr or simply don't construct a
UserGestureIndicator.

BUG=
==========

[Nate Chapin, 2016-10-10 21:43:00]

Description was changed from

==========
UserGestureIndicator is a mess. Clean it up.

Currently are two constructors for a UserGestureIndicator, one that takes a
ProcessingUserGestureState state and one that takes a
PassRefPtr<UserGestureToken>. Both take an optional
UserGestureUtilizedCallback*.

There are three ways to construct a UserGestureIndicator that does nothing:
(1) Pass a nullptr to the PassRefPtr<UserGestureToken> constructor,
(2) Pass PossiblyProcessingUserGesture to the enum constructor, or
(3) Pass DefinitelyNotProcessingUserGesture when not processing a user gesture
    (this is in fact the only way DefinitelyNotProcessingUserGesture is used).
(2) and (3) are only used in one place each, so remove those enum values and use
the other constructor with a nullptr or simply don't construct a
UserGestureIndicator.

BUG=
==========

to

==========
UserGestureIndicator is a mess. Clean it up.

Currently are two constructors for a UserGestureIndicator, one that takes a
ProcessingUserGestureState state and one that takes a
PassRefPtr<UserGestureToken>. Both take an optional
UserGestureUtilizedCallback*.

There are three ways to construct a UserGestureIndicator that does nothing:
(1) Pass a nullptr to the PassRefPtr<UserGestureToken> constructor,
(2) Pass PossiblyProcessingUserGesture to the enum constructor, or
(3) Pass DefinitelyNotProcessingUserGesture when not processing a user gesture
    (this is in fact the only way DefinitelyNotProcessingUserGesture is used).
(2) and (3) are only used in one place each, so remove those enum values and use
the other constructor with a nullptr or simply don't construct a
UserGestureIndicator. testtesttest

BUG=
==========

[Nate Chapin, 2016-10-10 21:43:14]

Description was changed from

==========
UserGestureIndicator is a mess. Clean it up.

Currently are two constructors for a UserGestureIndicator, one that takes a
ProcessingUserGestureState state and one that takes a
PassRefPtr<UserGestureToken>. Both take an optional
UserGestureUtilizedCallback*.

There are three ways to construct a UserGestureIndicator that does nothing:
(1) Pass a nullptr to the PassRefPtr<UserGestureToken> constructor,
(2) Pass PossiblyProcessingUserGesture to the enum constructor, or
(3) Pass DefinitelyNotProcessingUserGesture when not processing a user gesture
    (this is in fact the only way DefinitelyNotProcessingUserGesture is used).
(2) and (3) are only used in one place each, so remove those enum values and use
the other constructor with a nullptr or simply don't construct a
UserGestureIndicator. testtesttest

BUG=
==========

to

==========
UserGestureIndicator is a mess. Clean it up.

Currently are two constructors for a UserGestureIndicator, one that takes a
ProcessingUserGestureState state and one that takes a
PassRefPtr<UserGestureToken>. Both take an optional
UserGestureUtilizedCallback*.

There are three ways to construct a UserGestureIndicator that does nothing:
(1) Pass a nullptr to the PassRefPtr<UserGestureToken> constructor,
(2) Pass PossiblyProcessingUserGesture to the enum constructor, or
(3) Pass DefinitelyNotProcessingUserGesture when not processing a user gesture
    (this is in fact the only way DefinitelyNotProcessingUserGesture is used).
(2) and (3) are only used in one place each, so remove those enum values and use
the other constructor with a nullptr or simply don't construct a
UserGestureIndicator.

BUG=
==========

[Nate Chapin, 2016-10-10 21:51:24]

Description was changed from

==========
UserGestureIndicator is a mess. Clean it up.

Currently are two constructors for a UserGestureIndicator, one that takes a
ProcessingUserGestureState state and one that takes a
PassRefPtr<UserGestureToken>. Both take an optional
UserGestureUtilizedCallback*.

There are three ways to construct a UserGestureIndicator that does nothing:
(1) Pass a nullptr to the PassRefPtr<UserGestureToken> constructor,
(2) Pass PossiblyProcessingUserGesture to the enum constructor, or
(3) Pass DefinitelyNotProcessingUserGesture when not processing a user gesture
    (this is in fact the only way DefinitelyNotProcessingUserGesture is used).
(2) and (3) are only used in one place each, so remove those enum values and use
the other constructor with a nullptr or simply don't construct a
UserGestureIndicator.

BUG=
==========

to

==========
UserGestureIndicator is a mess. Clean it up.

Currently are two constructors for a UserGestureIndicator, one that takes a
ProcessingUserGestureState state and one that takes a
PassRefPtr<UserGestureToken>. Both take an optional
UserGestureUtilizedCallback*.

There are three ways to construct a UserGestureIndicator that does nothing:
(1) Pass a nullptr to the PassRefPtr<UserGestureToken> constructor,
(2) Pass PossiblyProcessingUserGesture to the enum constructor, or
(3) Pass DefinitelyNotProcessingUserGesture when not processing a user gesture
    (this is in fact the only way DefinitelyNotProcessingUserGesture is used).
(2) and (3) are only used in one place each, so remove those enum values and use
the other constructor with a nullptr or simply don't construct a
UserGestureIndicator.

UserGestureToken is a pure virtual intreface for reasons that aren't clear.
Collapse its implementation in to the base class.

With 2 ProcessingUserGestureState values removed, the only purpose of this enum
is
during UserGestureToken initialization: determining whether it definitely starts
with m_consumableGestures == 1, or whether it starts at 1 only if there are no
other UserGestureTokens on the stack. Move this initialization logic in to the
UserGestureToken constructor, generally simplify UserGestureToken's surface, and
remove the enum-based UserGestureIndicator constructor. Callers are now expected
to pass in a PassRefPtr<UserGestureToken> on UserGestureIndicator
initialization.
UserGestureToken::create() now takes an enum similar to the old
ProcessingUserGestureState, with values equivalent to
DefinitelyProcessingNewUserGesture and DefinitelyProcessingUserGesture.




BUG=
==========

[Nate Chapin, 2016-10-10 22:15:05]

Description was changed from

==========
UserGestureIndicator is a mess. Clean it up.

Currently are two constructors for a UserGestureIndicator, one that takes a
ProcessingUserGestureState state and one that takes a
PassRefPtr<UserGestureToken>. Both take an optional
UserGestureUtilizedCallback*.

There are three ways to construct a UserGestureIndicator that does nothing:
(1) Pass a nullptr to the PassRefPtr<UserGestureToken> constructor,
(2) Pass PossiblyProcessingUserGesture to the enum constructor, or
(3) Pass DefinitelyNotProcessingUserGesture when not processing a user gesture
    (this is in fact the only way DefinitelyNotProcessingUserGesture is used).
(2) and (3) are only used in one place each, so remove those enum values and use
the other constructor with a nullptr or simply don't construct a
UserGestureIndicator.

UserGestureToken is a pure virtual intreface for reasons that aren't clear.
Collapse its implementation in to the base class.

With 2 ProcessingUserGestureState values removed, the only purpose of this enum
is
during UserGestureToken initialization: determining whether it definitely starts
with m_consumableGestures == 1, or whether it starts at 1 only if there are no
other UserGestureTokens on the stack. Move this initialization logic in to the
UserGestureToken constructor, generally simplify UserGestureToken's surface, and
remove the enum-based UserGestureIndicator constructor. Callers are now expected
to pass in a PassRefPtr<UserGestureToken> on UserGestureIndicator
initialization.
UserGestureToken::create() now takes an enum similar to the old
ProcessingUserGestureState, with values equivalent to
DefinitelyProcessingNewUserGesture and DefinitelyProcessingUserGesture.




BUG=
==========

to

==========
UserGestureIndicator is a mess. Clean it up.

Currently are two constructors for a UserGestureIndicator, one that takes a
ProcessingUserGestureState enum value and one that takes a
PassRefPtr<UserGestureToken>.

There are three ways to construct a UserGestureIndicator that does nothing:
1. Pass a nullptr to the PassRefPtr<UserGestureToken> constructor.
2. Pass PossiblyProcessingUserGesture to the enum constructor.
3. Pass DefinitelyNotProcessingUserGesture when not alredy processing a user
    gesture (this is in fact the only way it is currently used).
(2) and (3) are only used in one place each, so remove those enum values and use
the other constructor with a nullptr or simply don't construct a
UserGestureIndicator.

UserGestureToken is a pure virtual interface for reasons that aren't clear.
Collapse its implementation in to the base class.

With 2 ProcessingUserGestureState values removed, the only purpose of this enum
is
during UserGestureToken initialization: determining whether it definitely starts
with m_consumableGestures == 1, or whether it starts at 1 only if there are no
other UserGestureTokens on the stack. Move this initialization logic in to the
UserGestureToken constructor, generally simplify UserGestureToken's surface, and
remove the enum-based UserGestureIndicator constructor. Callers are now expected
to pass in a PassRefPtr<UserGestureToken> on UserGestureIndicator
initialization.
UserGestureToken::create() now takes an enum similar to the old
ProcessingUserGestureState, with values equivalent to
DefinitelyProcessingNewUserGesture and DefinitelyProcessingUserGesture.




BUG=
==========

[Nate Chapin, 2016-10-10 22:22:45]

japhet@chromium.org changed reviewers:
+ esprehn@chromium.org, jochen@chromium.org, ojan@chromium.org,
rbyers@chromium.org

[Nate Chapin, 2016-10-10 22:22:46]

I took a look at UserGestureIndicator's implementation as a part of
https://codereview.chromium.org/2392773002. There's quite a bit of complexity
that appears unnecessary.

rbyers/jochen: PTAL
esprehn/ojan: FYI

[Nate Chapin, 2016-10-10 22:26:46]

https://codereview.chromium.org/2401123002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/UserGestureIndicator.cpp (left):

https://codereview.chromium.org/2401123002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/UserGestureIndicator.cpp:125:
UserGestureIndicator* UserGestureIndicator::s_topmostIndicator = 0;
Fun fact! s_topmostIndicator was actually the bottom of the stack of indicators
:)

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

https://codereview.chromium.org/2401123002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/UserGestureIndicator.cpp:57:
other->m_timestamp = WTF::currentTime();
I kept the old behavior here when a UserGestureToken is put on the stack:
transfer at most one user gesture, and update the timestamp to currentTime(), as
opposed to, e.g., the max of the two tokens' timestamps. I have no idea whether
these behaviors are correct or vestigial.

[commit-bot: I haz the power, 2016-10-10 22:57:49]

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

[commit-bot: I haz the power, 2016-10-10 22:57:50]

Dry run: This issue passed the CQ dry run.

[esprehn, 2016-10-10 23:52:07]

lgtm, this is amazing!

[esprehn, 2016-10-10 23:52:21]

Description was changed from

==========
UserGestureIndicator is a mess. Clean it up.

Currently are two constructors for a UserGestureIndicator, one that takes a
ProcessingUserGestureState enum value and one that takes a
PassRefPtr<UserGestureToken>.

There are three ways to construct a UserGestureIndicator that does nothing:
1. Pass a nullptr to the PassRefPtr<UserGestureToken> constructor.
2. Pass PossiblyProcessingUserGesture to the enum constructor.
3. Pass DefinitelyNotProcessingUserGesture when not alredy processing a user
    gesture (this is in fact the only way it is currently used).
(2) and (3) are only used in one place each, so remove those enum values and use
the other constructor with a nullptr or simply don't construct a
UserGestureIndicator.

UserGestureToken is a pure virtual interface for reasons that aren't clear.
Collapse its implementation in to the base class.

With 2 ProcessingUserGestureState values removed, the only purpose of this enum
is
during UserGestureToken initialization: determining whether it definitely starts
with m_consumableGestures == 1, or whether it starts at 1 only if there are no
other UserGestureTokens on the stack. Move this initialization logic in to the
UserGestureToken constructor, generally simplify UserGestureToken's surface, and
remove the enum-based UserGestureIndicator constructor. Callers are now expected
to pass in a PassRefPtr<UserGestureToken> on UserGestureIndicator
initialization.
UserGestureToken::create() now takes an enum similar to the old
ProcessingUserGestureState, with values equivalent to
DefinitelyProcessingNewUserGesture and DefinitelyProcessingUserGesture.




BUG=
==========

to

==========
UserGestureIndicator is a mess. Clean it up.

Currently are two constructors for a UserGestureIndicator, one that takes a
ProcessingUserGestureState enum value and one that takes a
PassRefPtr<UserGestureToken>.

There are three ways to construct a UserGestureIndicator that does nothing:
1. Pass a nullptr to the PassRefPtr<UserGestureToken> constructor.
2. Pass PossiblyProcessingUserGesture to the enum constructor.
3. Pass DefinitelyNotProcessingUserGesture when not alredy processing a user
    gesture (this is in fact the only way it is currently used).
(2) and (3) are only used in one place each, so remove those enum values and use
the other constructor with a nullptr or simply don't construct a
UserGestureIndicator.

UserGestureToken is a pure virtual interface for reasons that aren't clear.
Collapse its implementation in to the base class.

With 2 ProcessingUserGestureState values removed, the only purpose of this enum
is
during UserGestureToken initialization: determining whether it definitely starts
with m_consumableGestures == 1, or whether it starts at 1 only if there are no
other UserGestureTokens on the stack. Move this initialization logic in to the
UserGestureToken constructor, generally simplify UserGestureToken's surface, and
remove the enum-based UserGestureIndicator constructor. Callers are now expected
to pass in a PassRefPtr<UserGestureToken> on UserGestureIndicator
initialization.
UserGestureToken::create() now takes an enum similar to the old
ProcessingUserGestureState, with values equivalent to
DefinitelyProcessingNewUserGesture and DefinitelyProcessingUserGesture.

BUG=
==========

[ojan, 2016-10-11 03:29:10]

<3

[dtapuska, 2016-10-11 14:00:02]

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

[dtapuska, 2016-10-11 14:00:03]

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

https://codereview.chromium.org/2401123002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/UserGestureIndicator.cpp:57:
other->m_timestamp = WTF::currentTime();
On 2016/10/10 22:26:46, Nate Chapin wrote:
> I kept the old behavior here when a UserGestureToken is put on the stack:
> transfer at most one user gesture, and update the timestamp to currentTime(),
as
> opposed to, e.g., the max of the two tokens' timestamps. I have no idea
whether
> these behaviors are correct or vestigial.

Why are we using currentTime and not monotonic time?

[Nate Chapin, 2016-10-11 16:43:28]

On 2016/10/11 at 14:00:03, dtapuska wrote:
>
https://codereview.chromium.org/2401123002/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/platform/UserGestureIndicator.cpp (right):
> 
>
https://codereview.chromium.org/2401123002/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/UserGestureIndicator.cpp:57:
other->m_timestamp = WTF::currentTime();
> On 2016/10/10 22:26:46, Nate Chapin wrote:
> > I kept the old behavior here when a UserGestureToken is put on the stack:
> > transfer at most one user gesture, and update the timestamp to
currentTime(), as
> > opposed to, e.g., the max of the two tokens' timestamps. I have no idea
whether
> > these behaviors are correct or vestigial.
> 
> Why are we using currentTime and not monotonic time?

I have no idea! Legacy?

I work on time-related things fairly infrequently; what's the rule on when to
use currentTime vs. monotonicallyIncreasingTime?

[dtapuska, 2016-10-11 17:03:44]

On 2016/10/11 16:43:28, Nate Chapin wrote:
> On 2016/10/11 at 14:00:03, dtapuska wrote:
> >
>
https://codereview.chromium.org/2401123002/diff/80001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/platform/UserGestureIndicator.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2401123002/diff/80001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/platform/UserGestureIndicator.cpp:57:
> other->m_timestamp = WTF::currentTime();
> > On 2016/10/10 22:26:46, Nate Chapin wrote:
> > > I kept the old behavior here when a UserGestureToken is put on the stack:
> > > transfer at most one user gesture, and update the timestamp to
> currentTime(), as
> > > opposed to, e.g., the max of the two tokens' timestamps. I have no idea
> whether
> > > these behaviors are correct or vestigial.
> > 
> > Why are we using currentTime and not monotonic time?
> 
> I have no idea! Legacy?
> 
> I work on time-related things fairly infrequently; what's the rule on when to
> use currentTime vs. monotonicallyIncreasingTime?

currentTime can be adjusted based on the UTC offset. So you really shouldn't be
using that. monotonic has no utc offset.

[jochen (gone - plz use gerrit), 2016-10-11 17:06:00]

https://codereview.chromium.org/2401123002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/UserGestureIndicator.cpp (left):

https://codereview.chromium.org/2401123002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/UserGestureIndicator.cpp:125:
UserGestureIndicator* UserGestureIndicator::s_topmostIndicator = 0;
On 2016/10/10 at 22:26:46, Nate Chapin wrote:
> Fun fact! s_topmostIndicator was actually the bottom of the stack of
indicators :)

well, on this stack, you push to the top...

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

https://codereview.chromium.org/2401123002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/UserGestureIndicator.cpp:57:
other->m_timestamp = WTF::currentTime();
On 2016/10/10 at 22:26:46, Nate Chapin wrote:
> I kept the old behavior here when a UserGestureToken is put on the stack:
transfer at most one user gesture, and update the timestamp to currentTime(), as
opposed to, e.g., the max of the two tokens' timestamps. I have no idea whether
these behaviors are correct or vestigial.

at the time I wrote the transfer of tokens, Adam and me hoped that it would
never happen that we add two tokens here - even for showModalDialog there was
some kind of separation token you could put on the stack to prevent them from
adding up.

However, we decided that transferring one token is better than crashing and
probably fine.

The idea of the timestamp is to prevent sites from somehow storing a gesture
indefinitely long. If you are in the regular 1s case for setTimeout for example,
your timeout handler should still have a reasonable chance to run, so we just
reset the time here.

[Nate Chapin, 2016-10-11 17:27:00]

https://codereview.chromium.org/2401123002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/UserGestureIndicator.cpp (left):

https://codereview.chromium.org/2401123002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/UserGestureIndicator.cpp:125:
UserGestureIndicator* UserGestureIndicator::s_topmostIndicator = 0;
On 2016/10/11 at 17:06:00, jochen wrote:
> On 2016/10/10 at 22:26:46, Nate Chapin wrote:
> > Fun fact! s_topmostIndicator was actually the bottom of the stack of
indicators :)
> 
> well, on this stack, you push to the top...

Of course. But s_topmostIndicator was initialized to the first
UserGestureIndicator put on the stack and explicitly not updated for subsequent
pushes.

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

https://codereview.chromium.org/2401123002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/UserGestureIndicator.cpp:57:
other->m_timestamp = WTF::currentTime();
On 2016/10/11 at 17:06:00, jochen wrote:
> On 2016/10/10 at 22:26:46, Nate Chapin wrote:
> > I kept the old behavior here when a UserGestureToken is put on the stack:
transfer at most one user gesture, and update the timestamp to currentTime(), as
opposed to, e.g., the max of the two tokens' timestamps. I have no idea whether
these behaviors are correct or vestigial.
> 
> at the time I wrote the transfer of tokens, Adam and me hoped that it would
never happen that we add two tokens here - even for showModalDialog there was
some kind of separation token you could put on the stack to prevent them from
adding up.
> 
> However, we decided that transferring one token is better than crashing and
probably fine.
> 
> The idea of the timestamp is to prevent sites from somehow storing a gesture
indefinitely long. If you are in the regular 1s case for setTimeout for example,
your timeout handler should still have a reasonable chance to run, so we just
reset the time here.

Ok, all that makes sense. I'm going to change the timestamp to
monotonicallyIncreasingTime() before landing, though, per dtapuska.

[Rick Byers, 2016-10-11 18:00:19]

This is a great simplification, thanks!  Just one non-trivial concern over
callback lifetime semantics:


I haven't finished looking over all the changes in usage but wanted to send this
before my next batch of meetings...

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

https://codereview.chromium.org/2401123002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/UserGestureIndicator.cpp:110: if
(isMainThread() && m_token && m_token == s_rootToken) {
The callback lifetime semantics seem tricky.  I had put it on
UserGestureIndicator because there was no good way to guarantee that the
callback would be alive beyond the lifetime of the UserGestureIndicator.  Moving
it to the token might be OK, but at a minimum you should document what the
lifetime semantics are and how you're enforcing it.  Eg. in the m_token !=
s_rootToken case, should we still do a
m_token->setUserGestureUtilizedCallback(nullptr)?

There's likely not great test coverage for this :-(

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

https://codereview.chromium.org/2401123002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/UserGestureIndicator.h:52: class
PLATFORM_EXPORT UserGestureToken : public RefCounted<UserGestureToken> {
nit: since you're overhauling this, can you please add a comment briefly
describing the semantics of a UserGestureToken?

https://codereview.chromium.org/2401123002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/UserGestureIndicator.h:69: void
setUserGestureUtilizedCallback(UserGestureUtilizedCallback*);
Add a comment describing the lifetime semantics for the callback (see below).

https://codereview.chromium.org/2401123002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/UserGestureIndicatorTest.cpp (left):

https://codereview.chromium.org/2401123002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/UserGestureIndicatorTest.cpp:157: // The
callback isn't invoked outside the scope of the UGI
With the separation from the token, this is probably the test which should
validate the callback lifetime semantics.  I probably should have written this
test using a nested UGI, then it would have failed as a result of this
refactoring.

[Nate Chapin, 2016-10-11 18:18:42]

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

https://codereview.chromium.org/2401123002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/UserGestureIndicator.cpp:110: if
(isMainThread() && m_token && m_token == s_rootToken) {
On 2016/10/11 at 18:00:19, Rick Byers wrote:
> The callback lifetime semantics seem tricky.  I had put it on
UserGestureIndicator because there was no good way to guarantee that the
callback would be alive beyond the lifetime of the UserGestureIndicator.  Moving
it to the token might be OK, but at a minimum you should document what the
lifetime semantics are and how you're enforcing it.  Eg. in the m_token !=
s_rootToken case, should we still do a
m_token->setUserGestureUtilizedCallback(nullptr)?
> 
> There's likely not great test coverage for this :-(

Given how UserGestureUtilizedCallback are currently used and the changes I made
in TouchEventManager, it is guaranteed that the UserGestureUtilizedCallback is
added to s_rootToken. However, that's probably not future-proof.

Potential solutions:
1. Move the callback back to UserGestureIndicator.
2. Always clear the callback when a token's UserGestureIndicator destructs.
3. CHECK(callback == nullptr || this == s_rootToken) in
setUserGestureUtilizedCallback.

I think my preference is 3 > 2 > 1, since you won't get the callback if you put
it on a non-root token anyway. WDYT?

[Rick Byers, 2016-10-11 19:05:01]

On 2016/10/11 18:18:42, Nate Chapin wrote:
>
https://codereview.chromium.org/2401123002/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/platform/UserGestureIndicator.cpp (right):
> 
>
https://codereview.chromium.org/2401123002/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/UserGestureIndicator.cpp:110: if
> (isMainThread() && m_token && m_token == s_rootToken) {
> On 2016/10/11 at 18:00:19, Rick Byers wrote:
> > The callback lifetime semantics seem tricky.  I had put it on
> UserGestureIndicator because there was no good way to guarantee that the
> callback would be alive beyond the lifetime of the UserGestureIndicator. 
Moving
> it to the token might be OK, but at a minimum you should document what the
> lifetime semantics are and how you're enforcing it.  Eg. in the m_token !=
> s_rootToken case, should we still do a
> m_token->setUserGestureUtilizedCallback(nullptr)?
> > 
> > There's likely not great test coverage for this :-(
> 
> Given how UserGestureUtilizedCallback are currently used and the changes I
made
> in TouchEventManager, it is guaranteed that the UserGestureUtilizedCallback is
> added to s_rootToken. However, that's probably not future-proof.
> 
> Potential solutions:
> 1. Move the callback back to UserGestureIndicator.
> 2. Always clear the callback when a token's UserGestureIndicator destructs.
> 3. CHECK(callback == nullptr || this == s_rootToken) in
> setUserGestureUtilizedCallback.
> 
> I think my preference is 3 > 2 > 1, since you won't get the callback if you
put
> it on a non-root token anyway. WDYT?

Sounds good to me - especially if you also add some comments making the lifetime
requirements clear, and update the test to verify that the callback is indeed
cleared when the root indicator gets destructed.

[Nate Chapin, 2016-10-11 19:17:50]

The CQ bit was checked by japhet@chromium.org to run a CQ dry run

[Nate Chapin, 2016-10-11 19:17:55]

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

https://codereview.chromium.org/2401123002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/UserGestureIndicator.cpp:110: if
(isMainThread() && m_token && m_token == s_rootToken) {
On 2016/10/11 at 18:18:42, Nate Chapin wrote:
> On 2016/10/11 at 18:00:19, Rick Byers wrote:
> > The callback lifetime semantics seem tricky.  I had put it on
UserGestureIndicator because there was no good way to guarantee that the
callback would be alive beyond the lifetime of the UserGestureIndicator.  Moving
it to the token might be OK, but at a minimum you should document what the
lifetime semantics are and how you're enforcing it.  Eg. in the m_token !=
s_rootToken case, should we still do a
m_token->setUserGestureUtilizedCallback(nullptr)?
> > 
> > There's likely not great test coverage for this :-(
> 
> Given how UserGestureUtilizedCallback are currently used and the changes I
made in TouchEventManager, it is guaranteed that the UserGestureUtilizedCallback
is added to s_rootToken. However, that's probably not future-proof.
> 
> Potential solutions:
> 1. Move the callback back to UserGestureIndicator.
> 2. Always clear the callback when a token's UserGestureIndicator destructs.
> 3. CHECK(callback == nullptr || this == s_rootToken) in
setUserGestureUtilizedCallback.
> 
> I think my preference is 3 > 2 > 1, since you won't get the callback if you
put it on a non-root token anyway. WDYT?

Done (3).

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

https://codereview.chromium.org/2401123002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/UserGestureIndicator.h:52: class
PLATFORM_EXPORT UserGestureToken : public RefCounted<UserGestureToken> {
On 2016/10/11 at 18:00:19, Rick Byers wrote:
> nit: since you're overhauling this, can you please add a comment briefly
describing the semantics of a UserGestureToken?

Done.

https://codereview.chromium.org/2401123002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/UserGestureIndicator.h:69: void
setUserGestureUtilizedCallback(UserGestureUtilizedCallback*);
On 2016/10/11 at 18:00:19, Rick Byers wrote:
> Add a comment describing the lifetime semantics for the callback (see below).

Done.

https://codereview.chromium.org/2401123002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/UserGestureIndicatorTest.cpp (left):

https://codereview.chromium.org/2401123002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/UserGestureIndicatorTest.cpp:157: // The
callback isn't invoked outside the scope of the UGI
On 2016/10/11 at 18:00:19, Rick Byers wrote:
> With the separation from the token, this is probably the test which should
validate the callback lifetime semantics.  I probably should have written this
test using a nested UGI, then it would have failed as a result of this
refactoring.

Done (with a unique_ptr instead of with nested scopes).

[commit-bot: I haz the power, 2016-10-11 19:18:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Rick Byers, 2016-10-11 19:28:12]

LGTM with one possible cleanup nit.

Thank you!

https://codereview.chromium.org/2401123002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/TouchEventManager.cpp (right):

https://codereview.chromium.org/2401123002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/TouchEventManager.cpp:527: gestureIndicator
= wrapUnique(
I think you can simplify this further now to eliminate the unique_ptr
indirection, right?  Something like:

if (isTap || isSameOrigin) {
  if (!m_touchSequenceUserGestureToken)
    m_touchSequenceUserGestureToken = UserGestureToken::create();
  // These are ...
  ...
}
// If we have a token, active it for the duration of dispatch
UserGestureIndicator holder(m_touchSequenceUserGestureToken)

[Nate Chapin, 2016-10-11 19:41:30]

On 2016/10/11 at 19:28:12, rbyers wrote:
> LGTM with one possible cleanup nit.
> 
> Thank you!
> 
>
https://codereview.chromium.org/2401123002/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/input/TouchEventManager.cpp (right):
> 
>
https://codereview.chromium.org/2401123002/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/input/TouchEventManager.cpp:527:
gestureIndicator = wrapUnique(
> I think you can simplify this further now to eliminate the unique_ptr
indirection, right?  Something like:
> 
> if (isTap || isSameOrigin) {
>   if (!m_touchSequenceUserGestureToken)
>     m_touchSequenceUserGestureToken = UserGestureToken::create();
>   // These are ...
>   ...
> }
> // If we have a token, active it for the duration of dispatch
> UserGestureIndicator holder(m_touchSequenceUserGestureToken)

If both m_touchSequenceUserGestureToken and UserGestureToken::currentToken() are
non-null at function start, this snippet will change
m_touchSequenceUserGestureToken's value at function end, from currentToken() to
m_touchSequenceUserGestureToken.

Also, we need to create the UserGestureIndicator before calling
setUserGestureUtilizedCallback(), due to that CHECK I added.

[Rick Byers, 2016-10-11 19:54:12]

On 2016/10/11 19:41:30, Nate Chapin wrote:
> On 2016/10/11 at 19:28:12, rbyers wrote:
> > LGTM with one possible cleanup nit.
> > 
> > Thank you!
> > 
> >
>
https://codereview.chromium.org/2401123002/diff/80001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/input/TouchEventManager.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2401123002/diff/80001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/input/TouchEventManager.cpp:527:
> gestureIndicator = wrapUnique(
> > I think you can simplify this further now to eliminate the unique_ptr
> indirection, right?  Something like:
> > 
> > if (isTap || isSameOrigin) {
> >   if (!m_touchSequenceUserGestureToken)
> >     m_touchSequenceUserGestureToken = UserGestureToken::create();
> >   // These are ...
> >   ...
> > }
> > // If we have a token, active it for the duration of dispatch
> > UserGestureIndicator holder(m_touchSequenceUserGestureToken)
> 
> If both m_touchSequenceUserGestureToken and UserGestureToken::currentToken()
are
> non-null at function start, this snippet will change
> m_touchSequenceUserGestureToken's value at function end, from currentToken()
to
> m_touchSequenceUserGestureToken.
> 
> Also, we need to create the UserGestureIndicator before calling
> setUserGestureUtilizedCallback(), due to that CHECK I added.

Of course - ok.

I guess the simplest way to eliminate the need for the unique_ptr indirection
would be to add a method to UserGestureIndicator like 'takeToken' so the
indicator could start out without a token then conditionally get one attached to
it.  But it's minor - relying on a unique_ptr indirection isn't the end of the
world.

Still LGTM.

[commit-bot: I haz the power, 2016-10-11 21:00:58]

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

[commit-bot: I haz the power, 2016-10-11 21:00:59]

Dry run: This issue passed the CQ dry run.

[Nate Chapin, 2016-10-11 21:13:25]

The CQ bit was checked by japhet@chromium.org

[Nate Chapin, 2016-10-11 21:13:25]

The patchset sent to the CQ was uploaded after l-g-t-m from esprehn@chromium.org
Link to the patchset: https://codereview.chromium.org/2401123002/#ps100001
(title: "Callback cleanup, comments")

[commit-bot: I haz the power, 2016-10-11 21:13:52]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-11 21:36:57]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-10-11 21:39:37]

Description was changed from

==========
UserGestureIndicator is a mess. Clean it up.

Currently are two constructors for a UserGestureIndicator, one that takes a
ProcessingUserGestureState enum value and one that takes a
PassRefPtr<UserGestureToken>.

There are three ways to construct a UserGestureIndicator that does nothing:
1. Pass a nullptr to the PassRefPtr<UserGestureToken> constructor.
2. Pass PossiblyProcessingUserGesture to the enum constructor.
3. Pass DefinitelyNotProcessingUserGesture when not alredy processing a user
    gesture (this is in fact the only way it is currently used).
(2) and (3) are only used in one place each, so remove those enum values and use
the other constructor with a nullptr or simply don't construct a
UserGestureIndicator.

UserGestureToken is a pure virtual interface for reasons that aren't clear.
Collapse its implementation in to the base class.

With 2 ProcessingUserGestureState values removed, the only purpose of this enum
is
during UserGestureToken initialization: determining whether it definitely starts
with m_consumableGestures == 1, or whether it starts at 1 only if there are no
other UserGestureTokens on the stack. Move this initialization logic in to the
UserGestureToken constructor, generally simplify UserGestureToken's surface, and
remove the enum-based UserGestureIndicator constructor. Callers are now expected
to pass in a PassRefPtr<UserGestureToken> on UserGestureIndicator
initialization.
UserGestureToken::create() now takes an enum similar to the old
ProcessingUserGestureState, with values equivalent to
DefinitelyProcessingNewUserGesture and DefinitelyProcessingUserGesture.

BUG=
==========

to

==========
UserGestureIndicator is a mess. Clean it up.

Currently are two constructors for a UserGestureIndicator, one that takes a
ProcessingUserGestureState enum value and one that takes a
PassRefPtr<UserGestureToken>.

There are three ways to construct a UserGestureIndicator that does nothing:
1. Pass a nullptr to the PassRefPtr<UserGestureToken> constructor.
2. Pass PossiblyProcessingUserGesture to the enum constructor.
3. Pass DefinitelyNotProcessingUserGesture when not alredy processing a user
    gesture (this is in fact the only way it is currently used).
(2) and (3) are only used in one place each, so remove those enum values and use
the other constructor with a nullptr or simply don't construct a
UserGestureIndicator.

UserGestureToken is a pure virtual interface for reasons that aren't clear.
Collapse its implementation in to the base class.

With 2 ProcessingUserGestureState values removed, the only purpose of this enum
is
during UserGestureToken initialization: determining whether it definitely starts
with m_consumableGestures == 1, or whether it starts at 1 only if there are no
other UserGestureTokens on the stack. Move this initialization logic in to the
UserGestureToken constructor, generally simplify UserGestureToken's surface, and
remove the enum-based UserGestureIndicator constructor. Callers are now expected
to pass in a PassRefPtr<UserGestureToken> on UserGestureIndicator
initialization.
UserGestureToken::create() now takes an enum similar to the old
ProcessingUserGestureState, with values equivalent to
DefinitelyProcessingNewUserGesture and DefinitelyProcessingUserGesture.

BUG=

Committed: https://crrev.com/046115dc4c7cff68b07ad14f426388d86463aa22
Cr-Commit-Position: refs/heads/master@{#424552}
==========

[commit-bot: I haz the power, 2016-10-11 21:39:40]

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