UserGestureIndicator is a mess. Clean it up.

third_party/WebKit/Source/platform/UserGestureIndicator.cpp

Index: third_party/WebKit/Source/platform/UserGestureIndicator.cpp
diff --git a/third_party/WebKit/Source/platform/UserGestureIndicator.cpp b/third_party/WebKit/Source/platform/UserGestureIndicator.cpp
index 44106f2f820caf1fa01074ae1339b9771ff659ab..7518cf726fa416964210912d3c7a31a47be129e5 100644
--- a/third_party/WebKit/Source/platform/UserGestureIndicator.cpp
+++ b/third_party/WebKit/Source/platform/UserGestureIndicator.cpp
@@ -30,180 +30,93 @@
 
 namespace blink {
 
-namespace {
-
 // User gestures timeout in 1 second.
 const double userGestureTimeout = 1.0;
 
 // For out of process tokens we allow a 10 second delay.
 const double userGestureOutOfProcessTimeout = 10.0;
 
-class GestureToken final : public UserGestureToken {
-  WTF_MAKE_NONCOPYABLE(GestureToken);
-
- public:
-  static PassRefPtr<UserGestureToken> create() {
-    return adoptRef(new GestureToken);
-  }
-
-  ~GestureToken() final {}
-
-  bool hasGestures() const override {
-    // Do not enforce timeouts for gestures which spawned javascript prompts or
-    // debugger pause.
-    if (m_consumableGestures < 1 ||
-        (!m_javascriptPrompt && !m_pauseInDebugger &&
-         hasTimedOut(m_outOfProcess ? userGestureOutOfProcessTimeout
-                                    : userGestureTimeout)))
-      return false;
-    return true;
-  }
-
-  void addGesture() {
+UserGestureToken::UserGestureToken(Status status)
+    : m_consumableGestures(0),
+      m_timestamp(WTF::currentTime()),
+      m_timeoutPolicy(Default),
+      m_usageCallback(nullptr) {
+  if (status == NewGesture || !UserGestureIndicator::currentToken())
     m_consumableGestures++;
-    m_timestamp = WTF::currentTime();
-  }
-
-  void resetTimestamp() { m_timestamp = WTF::currentTime(); }
-
-  bool consumeGesture() {
-    if (!m_consumableGestures)
-      return false;
-    m_consumableGestures--;
-    return true;
-  }
-
-  void setOutOfProcess() final {
-    if (!hasTimedOut(userGestureTimeout) && hasGestures())
-      m_outOfProcess = true;
-  }
-
-  void setJavascriptPrompt() final {
-    if (!hasTimedOut(userGestureTimeout) && hasGestures())
-      m_javascriptPrompt = true;
-  }
-
-  void setPauseInDebugger() final {
-    if (!hasTimedOut(userGestureTimeout) && hasGestures())
-      m_pauseInDebugger = true;
-  }
-
- private:
-  GestureToken()
-      : m_consumableGestures(0),
-        m_timestamp(0),
-        m_outOfProcess(false),
-        m_javascriptPrompt(false),
-        m_pauseInDebugger(false) {}
-
-  bool hasTimedOut(double timeout) const {
-    return WTF::currentTime() - m_timestamp > timeout;
-  }
-
-  size_t m_consumableGestures;
-  double m_timestamp;
-  bool m_outOfProcess;
-  bool m_javascriptPrompt;
-  bool m_pauseInDebugger;
-};
-
-bool isDefinite(ProcessingUserGestureState state) {
-  return state == DefinitelyProcessingNewUserGesture ||
-         state == DefinitelyProcessingUserGesture ||
-         state == DefinitelyNotProcessingUserGesture;
 }
 
-bool isDefiniteUserGesture(ProcessingUserGestureState state) {
-  return state == DefinitelyProcessingNewUserGesture ||
-         state == DefinitelyProcessingUserGesture;
+bool UserGestureToken::hasGestures() {
+  return m_consumableGestures && !hasTimedOut();
 }
 
-}  // namespace
+void UserGestureToken::transferGestureTo(UserGestureToken* other) {
+  if (!hasGestures())
+    return;
+  m_consumableGestures--;
+  other->m_consumableGestures++;
+  other->m_timestamp = WTF::currentTime();

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

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.

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

Why are we using currentTime and not monotonic time?

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

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]

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

+}
 
-ProcessingUserGestureState UserGestureIndicator::s_state =
-    DefinitelyNotProcessingUserGesture;
-UserGestureIndicator* UserGestureIndicator::s_topmostIndicator = 0;

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

Fun fact! s_topmostIndicator was actually the bottom of the stack of indicators
:)

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

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

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

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

-bool UserGestureIndicator::s_processedUserGestureSinceLoad = false;
+bool UserGestureToken::consumeGesture() {
+  if (!m_consumableGestures)
+    return false;
+  m_consumableGestures--;
+  return true;
+}
 
-UserGestureIndicator::UserGestureIndicator(
-    ProcessingUserGestureState state,
-    UserGestureUtilizedCallback* usageCallback)
-    : m_previousState(DefinitelyNotProcessingUserGesture),
-      m_usageCallback(usageCallback) {
-  // Silently ignore UserGestureIndicators on non-main threads.
-  if (!isMainThread())
-    return;
+void UserGestureToken::setTimeoutPolicy(TimeoutPolicy policy) {
+  if (!hasTimedOut() && hasGestures() && policy > m_timeoutPolicy)
+    m_timeoutPolicy = policy;
+}
 
-  m_previousState = s_state;
+bool UserGestureToken::hasTimedOut() const {
+  if (m_timeoutPolicy == HasPaused)
+    return false;
+  double timeout = m_timeoutPolicy == OutOfProcess
+                       ? userGestureOutOfProcessTimeout
+                       : userGestureTimeout;
+  return WTF::currentTime() - m_timestamp > timeout;
+}
 
-  // We overwrite s_state only if the caller is definite about the gesture
-  // state.
-  if (isDefinite(state)) {
-    if (!s_topmostIndicator) {
-      s_topmostIndicator = this;
-      m_token = GestureToken::create();
-    } else {
-      m_token = currentToken();
-    }
-    s_state = state;
-  }
+void UserGestureToken::setUserGestureUtilizedCallback(
+    UserGestureUtilizedCallback* callback) {
+  m_usageCallback = callback;
+}
 
-  if (state == DefinitelyProcessingNewUserGesture) {
-    static_cast<GestureToken*>(m_token.get())->addGesture();
-    s_processedUserGestureSinceLoad = true;
-  } else if (state == DefinitelyProcessingUserGesture &&
-             s_topmostIndicator == this) {
-    static_cast<GestureToken*>(m_token.get())->addGesture();
-    s_processedUserGestureSinceLoad = true;
+void UserGestureToken::userGestureUtilized() {
+  if (m_usageCallback) {
+    m_usageCallback->userGestureUtilized();
+    m_usageCallback = nullptr;
   }
-  ASSERT(isDefinite(s_state));
 }
 
-UserGestureIndicator::UserGestureIndicator(
-    PassRefPtr<UserGestureToken> token,
-    UserGestureUtilizedCallback* usageCallback)
-    : m_previousState(DefinitelyNotProcessingUserGesture),
-      m_usageCallback(usageCallback) {
+UserGestureToken* UserGestureIndicator::s_rootToken = nullptr;
+bool UserGestureIndicator::s_processedUserGestureSinceLoad = false;
+
+UserGestureIndicator::UserGestureIndicator(PassRefPtr<UserGestureToken> token)
+    : m_token(token) {
   // Silently ignore UserGestureIndicators on non-main threads.
-  if (!isMainThread())
+  if (!isMainThread() || !m_token)
     return;
 
-  m_previousState = s_state;
-
-  if (token) {
-    static_cast<GestureToken*>(token.get())->resetTimestamp();
-    if (!s_topmostIndicator) {
-      s_topmostIndicator = this;
-      m_token = token;
-    } else {
-      m_token = currentToken();
-      if (static_cast<GestureToken*>(token.get())->hasGestures()) {
-        static_cast<GestureToken*>(m_token.get())->addGesture();
-        static_cast<GestureToken*>(token.get())->consumeGesture();
-      }
-    }
-    s_state = DefinitelyProcessingUserGesture;
-  }
-
-  ASSERT(isDefinite(s_state));
+  if (!s_rootToken)
+    s_rootToken = m_token.get();
+  else
+    m_token->transferGestureTo(s_rootToken);
+  s_processedUserGestureSinceLoad = true;
 }
 
 UserGestureIndicator::~UserGestureIndicator() {
-  if (!isMainThread())
-    return;
-  s_state = m_previousState;
-  if (s_topmostIndicator == this)
-    s_topmostIndicator = nullptr;
-  ASSERT(isDefinite(s_state));
+  if (isMainThread() && m_token && m_token == s_rootToken) {

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

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 :-(

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

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?

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

Done (3).

+    s_rootToken->setUserGestureUtilizedCallback(nullptr);
+    s_rootToken = nullptr;
+  }
 }
 
 // static
 bool UserGestureIndicator::utilizeUserGesture() {
   if (UserGestureIndicator::processingUserGesture()) {
-    if (s_topmostIndicator->m_usageCallback) {
-      s_topmostIndicator->m_usageCallback->userGestureUtilized();
-      s_topmostIndicator->m_usageCallback = nullptr;
-    }
+    s_rootToken->userGestureUtilized();
     return true;
   }
   return false;
@@ -212,8 +125,7 @@ bool UserGestureIndicator::utilizeUserGesture() {
 bool UserGestureIndicator::processingUserGesture() {
   if (auto* token = currentToken()) {
     ASSERT(isMainThread());
-    return isDefiniteUserGesture(s_state) &&
-           static_cast<GestureToken*>(token)->hasGestures();
+    return token->hasGestures();
   }
 
   return false;
@@ -223,11 +135,8 @@ bool UserGestureIndicator::processingUserGesture() {
 bool UserGestureIndicator::consumeUserGesture() {
   if (auto* token = currentToken()) {
     ASSERT(isMainThread());
-    if (static_cast<GestureToken*>(token)->consumeGesture()) {
-      if (s_topmostIndicator->m_usageCallback) {
-        s_topmostIndicator->m_usageCallback->userGestureUtilized();
-        s_topmostIndicator->m_usageCallback = nullptr;
-      }
+    if (token->consumeGesture()) {
+      token->userGestureUtilized();
       return true;
     }
   }
@@ -236,9 +145,9 @@ bool UserGestureIndicator::consumeUserGesture() {
 
 // static
 UserGestureToken* UserGestureIndicator::currentToken() {
-  if (!isMainThread() || !s_topmostIndicator)
+  if (!isMainThread() || !s_rootToken)
     return nullptr;
-  return s_topmostIndicator->m_token.get();
+  return s_rootToken;
 }
 
 // static