Prevent partial touch sequences from reaching the renderer

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 5921521e8ce0b2115106fad05b9561420f0aba6f..edf029975aa0e02c6904dbb1e1ce99ce85d05024 100644
--- a/content/browser/renderer_host/input/touch_event_queue.cc
+++ b/content/browser/renderer_host/input/touch_event_queue.cc
@@ -80,8 +80,6 @@ class TouchEventQueue::TouchTimeoutHandler {
           SetPendingAckState(PENDING_ACK_CANCEL_EVENT);
           TouchEventWithLatencyInfo cancel_event =
               ObtainCancelEventForTouchEvent(timeout_event_);
-          touch_queue_->UpdateTouchAckStates(
-              cancel_event.event, kDefaultNotForwardedAck);
           touch_queue_->client_->SendTouchEventImmediately(cancel_event);
         } else {
           SetPendingAckState(PENDING_ACK_NONE);
@@ -241,9 +239,7 @@ TouchEventQueue::TouchEventQueue(TouchEventQueueClient* client)
     : client_(client),
       dispatching_touch_ack_(NULL),
       dispatching_touch_(false),
-      has_handlers_(false),
-      scroll_in_progress_(false),
-      renderer_is_consuming_touch_gesture_(false),
+      touch_handling_state_(TOUCH_HANDLING_STATE_DEFAULT),
       ack_timeout_enabled_(false) {
   DCHECK(client);
 }
@@ -256,7 +252,7 @@ TouchEventQueue::~TouchEventQueue() {
 void TouchEventQueue::QueueEvent(const TouchEventWithLatencyInfo& event) {
   // Optimization of the case without touch handlers.  Removing this path
   // yields identical results, but this avoids unnecessary allocations.
-  if (!has_handlers_) {
+  if (touch_handling_state_ == NO_TOUCH_HANDLER) {

[Rick Byers, 2014/01/21 22:00:25]

Shouldn't we be able to handle the NO_TOUCH_HANDLER_FOR_GESTURE state in the
same way?

[jdduke (slow), 2014/01/22 02:47:00]

Hmm, we'd have to check if it's a new touch sequence, but yeah we could do that.

     DCHECK(touch_queue_.empty());
     client_->OnTouchEventAck(event, kDefaultNotForwardedAck);
     return;
@@ -294,7 +290,7 @@ void TouchEventQueue::ProcessTouchAck(InputEventAckState ack_result,
     return;
 
   if (ack_result == INPUT_EVENT_ACK_STATE_CONSUMED)
-    renderer_is_consuming_touch_gesture_ = true;
+    touch_handling_state_ = RENDERER_CONSUMING_GESTURE;

[Rick Byers, 2014/01/21 22:00:25]

can we DCHECK that we're in the HAS_TOUCH_HANDLER state?  Eg. in my description
we're transitioning from FORWARD_TOUCHES_UNTIL_TIMEOUT to FORWARD_ALL_TOUCHES.

[jdduke (slow), 2014/01/22 02:47:00]

I'm trying to think if we could ever get an ack if there were no handler... At
the least, I should be checking |touch_filtering_state_ ==
FORWARD_TOUCHES_UNTIL_TIMEOUT|.

 
   const WebTouchEvent& acked_event =
       touch_queue_.front()->coalesced_event().event;
@@ -310,10 +306,6 @@ void TouchEventQueue::TryForwardNextEventToRenderer() {
   while (!touch_queue_.empty()) {
     const TouchEventWithLatencyInfo& touch =
         touch_queue_.front()->coalesced_event();
-    if (IsNewTouchGesture(touch.event)) {
-      touch_ack_states_.clear();
-      renderer_is_consuming_touch_gesture_ = false;
-    }
     if (ShouldForwardToRenderer(touch.event)) {
       ForwardToRenderer(touch);
       break;
@@ -325,13 +317,20 @@ void TouchEventQueue::TryForwardNextEventToRenderer() {
 void TouchEventQueue::ForwardToRenderer(
     const TouchEventWithLatencyInfo& touch) {
   DCHECK(!dispatching_touch_);
+  DCHECK_NE(touch_handling_state_, NO_TOUCH_HANDLER);
+
+  if (IsNewTouchGesture(touch.event)) {
+    touch_handling_state_ = HAS_TOUCH_HANDLER;
+    touch_ack_states_.clear();
+  }
+
   // A synchronous ack will reset |dispatching_touch_|, in which case
   // the touch timeout should not be started.
   base::AutoReset<bool> dispatching_touch(&dispatching_touch_, true);
   client_->SendTouchEventImmediately(touch);
   if (ack_timeout_enabled_ &&
       dispatching_touch_ &&
-      !renderer_is_consuming_touch_gesture_ &&
+      touch_handling_state_ != RENDERER_CONSUMING_GESTURE &&
       ShouldTouchTypeTriggerTimeout(touch.event.type)) {
     DCHECK(timeout_handler_);
     timeout_handler_->Start(touch);
@@ -340,59 +339,56 @@ void TouchEventQueue::ForwardToRenderer(
 
 void TouchEventQueue::OnGestureScrollEvent(
     const GestureEventWithLatencyInfo& gesture_event) {
-  blink::WebInputEvent::Type type = gesture_event.event.type;
-  if (type == blink::WebInputEvent::GestureScrollBegin) {
-    // We assume the scroll event are generated synchronously from
-    // dispatching a touch event ack, so that we can fake a cancel
-    // event that has the correct touch ids as the touch event that
-    // is being acked. If not, we don't do the touch-cancel optimization.
-    if (scroll_in_progress_ || !dispatching_touch_ack_)
-      return;
-    scroll_in_progress_ = true;
+  if (gesture_event.event.type != blink::WebInputEvent::GestureScrollBegin)
+    return;
 
-    // If we have a timeout event, a cancel has already been dispatched
-    // for the current touch stream.
-    if (HasTimeoutEvent())
-      return;
+  // We assume that scroll events are generated synchronously from
+  // dispatching a touch event ack. This allows us to generate a synthetic
+  // cancel event that has the same touch ids as the touch event that
+  // is being acked. Otherwise, we don't perform the touch-cancel optimization.
+  if (!dispatching_touch_ack_)
+    return;
 
-    // Fake a TouchCancel to cancel the touch points of the touch event
-    // that is currently being acked.
-    // Note: |dispatching_touch_ack_| is non-null when we reach here, meaning we
-    // are in the scope of PopTouchEventToClient() and that no touch event
-    // in the queue is waiting for ack from renderer. So we can just insert
-    // the touch cancel at the beginning of the queue.
-    touch_queue_.push_front(new CoalescedWebTouchEvent(
-        ObtainCancelEventForTouchEvent(
-            dispatching_touch_ack_->coalesced_event()), true));
-  } else if (type == blink::WebInputEvent::GestureScrollEnd ||
-             type == blink::WebInputEvent::GestureFlingStart) {
-    scroll_in_progress_ = false;
-  }
+  if (touch_handling_state_ == NO_TOUCH_HANDLER_FOR_GESTURE)
+    return;
+
+  touch_handling_state_ = NO_TOUCH_HANDLER_FOR_GESTURE;
+
+  // If we have a timeout event, a cancel has already been dispatched
+  // for the current touch stream.
+  if (HasTimeoutEvent())
+    return;
+
+  // Fake a TouchCancel to cancel the touch points of the touch event
+  // that is currently being acked.
+  // Note: |dispatching_touch_ack_| is non-null when we reach here, meaning we
+  // are in the scope of PopTouchEventToClient() and that no touch event
+  // in the queue is waiting for ack from renderer. So we can just insert
+  // the touch cancel at the beginning of the queue.
+  touch_queue_.push_front(new CoalescedWebTouchEvent(
+      ObtainCancelEventForTouchEvent(
+          dispatching_touch_ack_->coalesced_event()), true));
 }
 
 void TouchEventQueue::OnHasTouchEventHandlers(bool has_handlers) {
   DCHECK(!dispatching_touch_ack_);
   DCHECK(!dispatching_touch_);
-  if (has_handlers_ == has_handlers)
-    return;
-
-  has_handlers_ = has_handlers;
 
-  if (!has_handlers_) {
-    // TODO(jdduke): Synthesize a TouchCancel if necessary to update Blink touch
-    // state tracking.
+  if (has_handlers) {
+    if (touch_handling_state_ == NO_TOUCH_HANDLER) {
+      // If no touch handler was previously registered, ensure that we don't
+      // send a partial gesture to the renderer.
+      DCHECK(touch_queue_.empty());
+      touch_handling_state_ = NO_TOUCH_HANDLER_FOR_GESTURE;

[Rick Byers, 2014/01/21 22:00:25]

Your comment for this state says that no events are sent for the duration of the
sequence.  Won't this cause exactly the 'partial gesture' state you're trying to
avoid?  Do we need a new state for sending all events in the current sequence
and then reverting to NO_TOUCH_HANDLER at the end of the sequence?

[jdduke (slow), 2014/01/22 02:47:00]

Sorry, by "partial gesture" I really mean "partial touch sequence".  I'll update
the verbiage.

+    }
+  } else {
+    touch_handling_state_ = NO_TOUCH_HANDLER;
     if (timeout_handler_)
       timeout_handler_->Reset();
     if (!touch_queue_.empty())
       ProcessTouchAck(kDefaultNotForwardedAck, ui::LatencyInfo());
     // As there is no touch handler, ack'ing the event should flush the queue.
     DCHECK(touch_queue_.empty());
-  } else {
-    DCHECK(touch_queue_.empty());
-    // Prevent a partial sequence from being sent to the renderer.
-    TouchPointAckStates::iterator ack_it = touch_ack_states_.begin();
-    for (; ack_it != touch_ack_states_.end(); ++ack_it)
-      ack_it->second = kDefaultNotForwardedAck;
   }
 }
 
@@ -436,6 +432,9 @@ TouchEventQueue::GetLatestEventForTesting() const {
 void TouchEventQueue::FlushQueue() {
   DCHECK(!dispatching_touch_ack_);
   DCHECK(!dispatching_touch_);
+  DCHECK_NE(RENDERER_CONSUMING_GESTURE, touch_handling_state_);
+  if (touch_handling_state_ != NO_TOUCH_HANDLER)
+    touch_handling_state_ = NO_TOUCH_HANDLER_FOR_GESTURE;
   while (!touch_queue_.empty())
     PopTouchEventToClient(kDefaultNotForwardedAck, ui::LatencyInfo());
 }
@@ -470,12 +469,16 @@ bool TouchEventQueue::ShouldForwardToRenderer(
   if (HasTimeoutEvent())
     return false;
 
-  if (!has_handlers_)
+  if (touch_handling_state_ == NO_TOUCH_HANDLER)
     return false;
 
-  if (scroll_in_progress_ &&
-      event.type != blink::WebInputEvent::TouchCancel)
+  if (touch_handling_state_ == NO_TOUCH_HANDLER_FOR_GESTURE) {
+    if (IsNewTouchGesture(event))
+      return true;
+    if (event.type == WebInputEvent::TouchCancel)
+      return true;
     return false;
+  }
 
   // Touch press events should always be forwarded to the renderer.
   if (event.type == WebInputEvent::TouchStart)