Allow repeated handler removal/addition with the TouchEventQueue

content/browser/renderer_host/input/touch_event_queue.cc

Index: content/browser/renderer_host/input/touch_event_queue.cc
diff --git a/content/browser/renderer_host/input/touch_event_queue.cc b/content/browser/renderer_host/input/touch_event_queue.cc
index d98655a1668f58f6aea392e15ad41341997c943d..a62cf737fd0a09b46df9b4c40efd0312010e561b 100644
--- a/content/browser/renderer_host/input/touch_event_queue.cc
+++ b/content/browser/renderer_host/input/touch_event_queue.cc
@@ -69,15 +69,28 @@ class TouchEventQueue::TouchTimeoutHandler {
         timeout_delay_(timeout_delay),
         pending_ack_state_(PENDING_ACK_NONE),
         timeout_monitor_(base::Bind(&TouchTimeoutHandler::OnTimeOut,
-                                    base::Unretained(this))) {
+                                    base::Unretained(this))),
+        enabled_(true),
+        enabled_for_current_sequence_(false) {
     DCHECK(timeout_delay != base::TimeDelta());
   }
 
   ~TouchTimeoutHandler() {}
 
-  void Start(const TouchEventWithLatencyInfo& event) {
+  void StartIfNecessary(const TouchEventWithLatencyInfo& event) {
     DCHECK_EQ(pending_ack_state_, PENDING_ACK_NONE);
-    DCHECK(ShouldTouchTriggerTimeout(event.event));
+    if (!enabled_)
+      return;
+
+    if (!ShouldTouchTriggerTimeout(event.event))
+      return;
+
+    if (WebTouchEventTraits::IsTouchSequenceStart(event.event))
+      enabled_for_current_sequence_ = true;
+
+    if (!enabled_for_current_sequence_)
+      return;
+
     timeout_event_ = event;
     timeout_monitor_.Restart(timeout_delay_);
   }
@@ -85,6 +98,8 @@ class TouchEventQueue::TouchTimeoutHandler {
   bool ConfirmTouchEvent(InputEventAckState ack_result) {
     switch (pending_ack_state_) {
       case PENDING_ACK_NONE:
+        if (ack_result == INPUT_EVENT_ACK_STATE_CONSUMED)
+          enabled_for_current_sequence_ = false;
         timeout_monitor_.Stop();
         return false;
       case PENDING_ACK_ORIGINAL_EVENT:
@@ -95,7 +110,8 @@ class TouchEventQueue::TouchTimeoutHandler {
           touch_queue_->SendTouchEventImmediately(cancel_event);
         } else {
           SetPendingAckState(PENDING_ACK_NONE);
-          touch_queue_->UpdateTouchAckStates(timeout_event_.event, ack_result);
+          touch_queue_->UpdateTouchConsumerStates(timeout_event_.event,
+                                                  ack_result);
         }
         return true;
       case PENDING_ACK_CANCEL_EVENT:
@@ -109,19 +125,29 @@ class TouchEventQueue::TouchTimeoutHandler {
     return HasTimeoutEvent();
   }
 
-  bool IsTimeoutTimerRunning() const {
-    return timeout_monitor_.IsRunning();
-  }
+  void SetEnabled(bool enabled) {
+    if (enabled_ == enabled)
+      return;
 
-  void Reset() {
-    pending_ack_state_ = PENDING_ACK_NONE;
-    timeout_monitor_.Stop();
-  }
+    enabled_ = enabled;
 
-  void set_timeout_delay(base::TimeDelta timeout_delay) {
-    timeout_delay_ = timeout_delay;
+    if (enabled_)
+      return;
+
+    enabled_for_current_sequence_ = false;
+    // Only reset the |timeout_handler_| if the timer is running and has not
+    // yet timed out. This ensures that an already timed out sequence is
+    // properly flushed by the handler.
+    if (IsTimeoutTimerRunning()) {
+      pending_ack_state_ = PENDING_ACK_NONE;
+      timeout_monitor_.Stop();
+    }
   }
 
+  bool IsTimeoutTimerRunning() const { return timeout_monitor_.IsRunning(); }
+
+  bool enabled() const { return enabled_; }
+
  private:
   enum PendingAckState {
     PENDING_ACK_NONE,
@@ -184,6 +210,9 @@ class TouchEventQueue::TouchTimeoutHandler {
 
   // Provides timeout-based callback behavior.
   TimeoutMonitor timeout_monitor_;
+
+  bool enabled_;
+  bool enabled_for_current_sequence_;
 };
 
 // Provides touchmove slop suppression for a single touch that remains within
@@ -343,8 +372,7 @@ TouchEventQueue::TouchEventQueue(TouchEventQueueClient* client,
     : client_(client),
       dispatching_touch_ack_(NULL),
       dispatching_touch_(false),
-      touch_filtering_state_(TOUCH_FILTERING_STATE_DEFAULT),
-      ack_timeout_enabled_(config.touch_ack_timeout_supported),
+      has_handlers_(true),
       touchmove_slop_suppressor_(new TouchMoveSlopSuppressor(
           config.touchmove_slop_suppression_length_dips)),
       send_touch_events_async_(false),
@@ -352,7 +380,7 @@ TouchEventQueue::TouchEventQueue(TouchEventQueueClient* client,
       last_sent_touch_timestamp_sec_(0),
       touch_scrolling_mode_(config.touch_scrolling_mode) {
   DCHECK(client);
-  if (ack_timeout_enabled_) {
+  if (config.touch_ack_timeout_supported) {
     timeout_handler_.reset(
         new TouchTimeoutHandler(this, config.touch_ack_timeout_delay));
   }
@@ -412,11 +440,6 @@ void TouchEventQueue::ProcessTouchAck(InputEventAckState ack_result,
   if (touch_queue_.empty())
     return;
 
-  if (ack_result == INPUT_EVENT_ACK_STATE_CONSUMED &&
-      touch_filtering_state_ == FORWARD_TOUCHES_UNTIL_TIMEOUT) {
-    touch_filtering_state_ = FORWARD_ALL_TOUCHES;
-  }
-
   PopTouchEventToClient(ack_result, latency_info);
   TryForwardNextEventToRenderer();
 }
@@ -447,20 +470,8 @@ void TouchEventQueue::ForwardNextEventToRenderer() {
 
   DCHECK(!empty());
   DCHECK(!dispatching_touch_);
-  DCHECK_NE(touch_filtering_state_, DROP_ALL_TOUCHES);
   TouchEventWithLatencyInfo touch = touch_queue_.front()->coalesced_event();
 
-  if (WebTouchEventTraits::IsTouchSequenceStart(touch.event)) {
-    touch_filtering_state_ =
-        ack_timeout_enabled_ ? FORWARD_TOUCHES_UNTIL_TIMEOUT
-                             : FORWARD_ALL_TOUCHES;
-    touch_ack_states_.clear();
-    send_touch_events_async_ = false;
-    pending_async_touchmove_.reset();
-    touch_sequence_start_position_ =
-        gfx::PointF(touch.event.touches[0].position);
-  }
-
   if (send_touch_events_async_ &&
       touch.event.type == WebInputEvent::TouchMove) {
     // Throttling touchmove's in a continuous touchmove stream while scrolling
@@ -526,36 +537,22 @@ void TouchEventQueue::ForwardNextEventToRenderer() {
   // the touch timeout should not be started.
   base::AutoReset<bool> dispatching_touch(&dispatching_touch_, true);
   SendTouchEventImmediately(touch);
-  if (dispatching_touch_ &&
-      touch_filtering_state_ == FORWARD_TOUCHES_UNTIL_TIMEOUT &&
-      ShouldTouchTriggerTimeout(touch.event)) {
-    DCHECK(timeout_handler_);
-    timeout_handler_->Start(touch);
-  }
+  if (dispatching_touch_ && timeout_handler_)
+    timeout_handler_->StartIfNecessary(touch);
 }
 
 void TouchEventQueue::OnGestureScrollEvent(
     const GestureEventWithLatencyInfo& gesture_event) {
   if (gesture_event.event.type == blink::WebInputEvent::GestureScrollBegin) {
-    if (touch_filtering_state_ != DROP_ALL_TOUCHES &&
-        touch_filtering_state_ != DROP_TOUCHES_IN_SEQUENCE) {
-      DCHECK(!touchmove_slop_suppressor_->suppressing_touchmoves())

[Rick Byers, 2014/09/19 17:54:06]

We added this DCHECK because we had at least one nasty bug where we'd never
offer the page a touchmove before scrolling.  Can't we still check that?

[jdduke (slow), 2014/09/22 16:30:14]

Hmm, you're right, I thought it would be tricky but I think we can just check
the consumer state.

-          << "The renderer should be offered a touchmove before scrolling "
-             "begins";
-    }
-
     if (touch_scrolling_mode_ == TOUCH_SCROLLING_MODE_ASYNC_TOUCHMOVE &&
-        touch_filtering_state_ != DROP_ALL_TOUCHES &&
-        touch_filtering_state_ != DROP_TOUCHES_IN_SEQUENCE &&
-        (touch_ack_states_.empty() ||
-         AllTouchAckStatesHaveState(
-             INPUT_EVENT_ACK_STATE_NO_CONSUMER_EXISTS))) {
+        !drop_remaining_touches_in_sequence_ &&
+        touch_consumer_states_.is_empty()) {
       // If no touch points have a consumer, prevent all subsequent touch events
       // received during the scroll from reaching the renderer. This ensures
       // that the first touchstart the renderer sees in any given sequence can
       // always be preventDefault'ed (cancelable == true).
       // TODO(jdduke): Revisit if touchstarts during scroll are made cancelable.
-      touch_filtering_state_ = DROP_TOUCHES_IN_SEQUENCE;
+      drop_remaining_touches_in_sequence_ = true;
     }
 
     if (touch_scrolling_mode_ == TOUCH_SCROLLING_MODE_ASYNC_TOUCHMOVE) {
@@ -582,10 +579,10 @@ void TouchEventQueue::OnGestureScrollEvent(
   if (!dispatching_touch_ack_)
     return;
 
-  if (touch_filtering_state_ == DROP_TOUCHES_IN_SEQUENCE)
+  if (drop_remaining_touches_in_sequence_)
     return;
 
-  touch_filtering_state_ = DROP_TOUCHES_IN_SEQUENCE;
+  drop_remaining_touches_in_sequence_ = true;
 
   // Fake a TouchCancel to cancel the touch points of the touch event
   // that is currently being acked.
@@ -621,22 +618,7 @@ void TouchEventQueue::OnGestureEventAck(
 void TouchEventQueue::OnHasTouchEventHandlers(bool has_handlers) {
   DCHECK(!dispatching_touch_ack_);
   DCHECK(!dispatching_touch_);
-
-  if (has_handlers) {
-    if (touch_filtering_state_ == DROP_ALL_TOUCHES) {
-      // If no touch handler was previously registered, ensure that we don't
-      // send a partial touch sequence to the renderer.
-      touch_filtering_state_ = DROP_TOUCHES_IN_SEQUENCE;
-    }
-  } else {
-    // TODO(jdduke): Synthesize a TouchCancel if necessary to update Blink touch
-    // state tracking and/or touch-action filtering (e.g., if the touch handler
-    // was removed mid-sequence), crbug.com/375940.
-    touch_filtering_state_ = DROP_ALL_TOUCHES;
-    pending_async_touchmove_.reset();
-    if (timeout_handler_)
-      timeout_handler_->Reset();
-  }
+  has_handlers_ = has_handlers;
 }
 
 bool TouchEventQueue::IsPendingAckTouchStart() const {
@@ -650,25 +632,12 @@ bool TouchEventQueue::IsPendingAckTouchStart() const {
 }
 
 void TouchEventQueue::SetAckTimeoutEnabled(bool enabled) {
-  // The timeout handler is valid only if explicitly supported in the config.
-  if (!timeout_handler_)
-    return;
-
-  if (ack_timeout_enabled_ == enabled)
-    return;
-
-  ack_timeout_enabled_ = enabled;
-
-  if (enabled)
-    return;
+  if (timeout_handler_)
+    timeout_handler_->SetEnabled(enabled);
+}
 
-  if (touch_filtering_state_ == FORWARD_TOUCHES_UNTIL_TIMEOUT)
-    touch_filtering_state_ = FORWARD_ALL_TOUCHES;
-  // Only reset the |timeout_handler_| if the timer is running and has not yet
-  // timed out. This ensures that an already timed out sequence is properly
-  // flushed by the handler.
-  if (timeout_handler_ && timeout_handler_->IsTimeoutTimerRunning())
-    timeout_handler_->Reset();
+bool TouchEventQueue::IsAckTimeoutEnabled() const {
+  return timeout_handler_ && timeout_handler_->enabled();
 }
 
 bool TouchEventQueue::HasPendingAsyncTouchMoveForTesting() const {
@@ -688,8 +657,7 @@ void TouchEventQueue::FlushQueue() {
   DCHECK(!dispatching_touch_ack_);
   DCHECK(!dispatching_touch_);
   pending_async_touchmove_.reset();
-  if (touch_filtering_state_ != DROP_ALL_TOUCHES)
-    touch_filtering_state_ = DROP_TOUCHES_IN_SEQUENCE;
+  drop_remaining_touches_in_sequence_ = true;
   while (!touch_queue_.empty())
     PopTouchEventToClient(INPUT_EVENT_ACK_STATE_NO_CONSUMER_EXISTS);
 }
@@ -711,7 +679,7 @@ void TouchEventQueue::AckTouchEventToClient(
     scoped_ptr<CoalescedWebTouchEvent> acked_event) {
   DCHECK(acked_event);
   DCHECK(!dispatching_touch_ack_);
-  UpdateTouchAckStates(acked_event->coalesced_event().event, ack_result);
+  UpdateTouchConsumerStates(acked_event->coalesced_event().event, ack_result);
 
   // Note that acking the touch-event may result in multiple gestures being sent
   // to the renderer, or touch-events being queued.
@@ -750,19 +718,25 @@ TouchEventQueue::FilterBeforeForwarding(const WebTouchEvent& event) {
   if (touchmove_slop_suppressor_->FilterEvent(event))
     return ACK_WITH_NOT_CONSUMED;
 
-  if (touch_filtering_state_ == DROP_ALL_TOUCHES)
-    return ACK_WITH_NO_CONSUMER_EXISTS;
+  if (WebTouchEventTraits::IsTouchSequenceStart(event)) {
+    touch_consumer_states_.clear();
+    send_touch_events_async_ = false;
+    pending_async_touchmove_.reset();
+    touch_sequence_start_position_ = gfx::PointF(event.touches[0].position);
+    drop_remaining_touches_in_sequence_ = false;
+    if (!has_handlers_) {
+      drop_remaining_touches_in_sequence_ = true;
+      return ACK_WITH_NO_CONSUMER_EXISTS;
+    }
+  }
 
-  if (touch_filtering_state_ == DROP_TOUCHES_IN_SEQUENCE &&
+  if (drop_remaining_touches_in_sequence_ &&
       event.type != WebInputEvent::TouchCancel) {
-    if (WebTouchEventTraits::IsTouchSequenceStart(event))
-      return FORWARD_TO_RENDERER;
     return ACK_WITH_NO_CONSUMER_EXISTS;
   }
 
-  // Touch press events should always be forwarded to the renderer.
   if (event.type == WebInputEvent::TouchStart)
-    return FORWARD_TO_RENDERER;
+    return has_handlers_ ? FORWARD_TO_RENDERER : ACK_WITH_NO_CONSUMER_EXISTS;
 
   for (unsigned int i = 0; i < event.touchesLength; ++i) {
     const WebTouchPoint& point = event.touches[i];
@@ -770,55 +744,42 @@ TouchEventQueue::FilterBeforeForwarding(const WebTouchEvent& event) {
     if (point.state == WebTouchPoint::StateStationary)
       continue;
 
-    if (touch_ack_states_.count(point.id) > 0) {
-      if (touch_ack_states_.find(point.id)->second !=
-          INPUT_EVENT_ACK_STATE_NO_CONSUMER_EXISTS)
-        return FORWARD_TO_RENDERER;
-    } else {
-      // If the ACK status of a point is unknown, then the event should be
-      // forwarded to the renderer.
+    DCHECK_GE(point.id, 0);
+    DCHECK_LT(point.id, 32);
+    if (touch_consumer_states_.has_bit(point.id))
       return FORWARD_TO_RENDERER;
-    }
   }
 
   return ACK_WITH_NO_CONSUMER_EXISTS;
 }
 
-void TouchEventQueue::UpdateTouchAckStates(const WebTouchEvent& event,
-                                           InputEventAckState ack_result) {
+void TouchEventQueue::UpdateTouchConsumerStates(const WebTouchEvent& event,
+                                                InputEventAckState ack_result) {
   // Update the ACK status for each touch point in the ACKed event.
   if (event.type == WebInputEvent::TouchEnd ||
       event.type == WebInputEvent::TouchCancel) {
     // The points have been released. Erase the ACK states.
     for (unsigned i = 0; i < event.touchesLength; ++i) {
       const WebTouchPoint& point = event.touches[i];
+      DCHECK_GE(point.id, 0);

[Rick Byers, 2014/09/19 17:54:06]

I'd prefer if the DCHECK_LT was in the bitset implementation instead of
sprinkled at every callsite.

Perhaps we should have signed overloads that do the DCHECK_GT too (or maybe
WebTouchPoint.id should really be unsigned), or if you want to just check that
here it's fine with me.

[jdduke (slow), 2014/09/22 16:30:14]

That's much better, thanks.

+      DCHECK_LT(point.id, 32);
       if (point.state == WebTouchPoint::StateReleased ||
           point.state == WebTouchPoint::StateCancelled)
-        touch_ack_states_.erase(point.id);
+        touch_consumer_states_.clear_bit(point.id);
     }
   } else if (event.type == WebInputEvent::TouchStart) {
     for (unsigned i = 0; i < event.touchesLength; ++i) {
       const WebTouchPoint& point = event.touches[i];
-      if (point.state == WebTouchPoint::StatePressed)
-        touch_ack_states_[point.id] = ack_result;
+      DCHECK_GE(point.id, 0);
+      DCHECK_LT(point.id, 32);
+      if (point.state == WebTouchPoint::StatePressed) {
+        if (ack_result != INPUT_EVENT_ACK_STATE_NO_CONSUMER_EXISTS)
+          touch_consumer_states_.mark_bit(point.id);
+        else
+          touch_consumer_states_.clear_bit(point.id);
+      }
     }
   }
 }
 
-bool TouchEventQueue::AllTouchAckStatesHaveState(
-    InputEventAckState ack_state) const {
-  if (touch_ack_states_.empty())
-    return false;
-
-  for (TouchPointAckStates::const_iterator iter = touch_ack_states_.begin(),
-                                           end = touch_ack_states_.end();
-       iter != end;
-       ++iter) {
-    if (iter->second != ack_state)
-      return false;
-  }
-
-  return true;
-}
-
 }  // namespace content