Suppress touchscreen tap immediately after a GestureFlingCancel

content/browser/renderer_host/gesture_event_filter.cc

Index: content/browser/renderer_host/gesture_event_filter.cc
diff --git a/content/browser/renderer_host/gesture_event_filter.cc b/content/browser/renderer_host/gesture_event_filter.cc
index e5fc3edce90e0dcd6ab37758bb9594e8c946b7c8..02ed0c31cfdceae33470d2c4be1a953caeaee534 100644
--- a/content/browser/renderer_host/gesture_event_filter.cc
+++ b/content/browser/renderer_host/gesture_event_filter.cc
@@ -8,6 +8,7 @@
 #include "base/string_number_conversions.h"
 #include "content/browser/renderer_host/render_widget_host_impl.h"
 #include "content/browser/renderer_host/touchpad_tap_suppression_controller.h"
+#include "content/browser/renderer_host/touchscreen_tap_suppression_controller.h"
 #include "content/public/common/content_switches.h"
 
 using WebKit::WebGestureEvent;
@@ -55,7 +56,7 @@ static int GetTapDownDeferralTimeMs() {
                  switches::kTapDownDeferralTimeMs);
   return tap_down_deferral_time_window;
 }
-} // namespace
+}  // namespace
 
 GestureEventFilter::GestureEventFilter(RenderWidgetHostImpl* rwhv)
      : render_widget_host_(rwhv),
@@ -63,7 +64,10 @@ GestureEventFilter::GestureEventFilter(RenderWidgetHostImpl* rwhv)
        scrolling_in_progress_(false),
        ignore_next_ack_(false),
        combined_scroll_pinch_(gfx::Transform()),
-       tap_suppression_controller_(new TouchpadTapSuppressionController(rwhv)),
+       touchpad_tap_suppression_controller_(
+           new TouchpadTapSuppressionController(rwhv)),
+       touchscreen_tap_suppression_controller_(
+           new TouchscreenTapSuppressionController(this)),
        maximum_tap_gap_time_ms_(GetTapDownDeferralTimeMs()),
        debounce_interval_time_ms_(kDebouncingIntervalTimeMs) {
 }
@@ -88,13 +92,16 @@ bool GestureEventFilter::ShouldDiscardFlingCancelEvent(
 
 bool GestureEventFilter::ShouldForwardForBounceReduction(
     const WebGestureEvent& gesture_event) {
+  if (debounce_interval_time_ms_ ==  0)
+    return true;
   switch (gesture_event.type) {
     case WebInputEvent::GestureScrollUpdate:
       if (!scrolling_in_progress_) {
-        debounce_deferring_timer_.Start(FROM_HERE,
-              base::TimeDelta::FromMilliseconds(debounce_interval_time_ms_),
-              this,
-              &GestureEventFilter::SendScrollEndingEventsNow);
+        debounce_deferring_timer_.Start(
+            FROM_HERE,
+            base::TimeDelta::FromMilliseconds(debounce_interval_time_ms_),
+            this,
+            &GestureEventFilter::SendScrollEndingEventsNow);
       } else {
         // Extend the bounce interval.
         debounce_deferring_timer_.Reset();
@@ -121,38 +128,63 @@ bool GestureEventFilter::ShouldForwardForBounceReduction(
 
 // NOTE: The filters are applied successively. This simplifies the change.
 bool GestureEventFilter::ShouldForward(const WebGestureEvent& gesture_event) {
-  // Discard a zero-velocity fling start from the trackpad.
-  if (gesture_event.type == WebInputEvent::GestureFlingStart &&
-      gesture_event.sourceDevice == WebGestureEvent::Touchpad &&
-      gesture_event.data.flingStart.velocityX == 0 &&
-      gesture_event.data.flingStart.velocityY == 0) {
-    return false;
-  }
+  return ShouldForwardForZeroVelocityFlingStart(gesture_event) &&
+      ShouldForwardForBounceReduction(gesture_event) &&

[rjkroege, 2013/03/07 22:56:21]

afaik: touchpad events shouldn't be debounced. are they?

[mohsen, 2013/03/08 00:14:58]

I don't think so. Only when we are scrolling using GestureScrollUpdates, events
are debounced. I think touchpad scrolls use MouseWheel events, so no debouncing
should happen?

[rjkroege, 2013/03/14 19:48:06]

yes. you're right.

+      ShouldForwardForGFCFiltering(gesture_event) &&
+      ShouldForwardForTapSuppression(gesture_event) &&
+      ShouldForwardForTapDeferral(gesture_event) &&
+      ShouldForwardForCoalescing(gesture_event);
+}
+
+bool GestureEventFilter::ShouldForwardForZeroVelocityFlingStart(
+    const WebGestureEvent& gesture_event) {
+  return gesture_event.type != WebInputEvent::GestureFlingStart ||
+      gesture_event.sourceDevice != WebGestureEvent::Touchpad ||
+      gesture_event.data.flingStart.velocityX != 0 ||
+      gesture_event.data.flingStart.velocityY != 0;
+}
 
-  if (debounce_interval_time_ms_ ==  0 ||
-      ShouldForwardForBounceReduction(gesture_event))
-    return ShouldForwardForTapDeferral(gesture_event);
+bool GestureEventFilter::ShouldForwardForGFCFiltering(
+    const WebGestureEvent& gesture_event) {
+  return gesture_event.type != WebInputEvent::GestureFlingCancel ||
+      !ShouldDiscardFlingCancelEvent(gesture_event);
+}
+
+bool GestureEventFilter::ShouldForwardForTapSuppression(
+    const WebGestureEvent& gesture_event) {
+  switch (gesture_event.type) {
+    case WebInputEvent::GestureFlingCancel:
+      if (gesture_event.sourceDevice == WebGestureEvent::Touchscreen)
+        touchscreen_tap_suppression_controller_->GestureFlingCancel();
+      else
+        touchpad_tap_suppression_controller_->GestureFlingCancel();
+      return true;
+    case WebInputEvent::GestureTapDown:
+      return !touchscreen_tap_suppression_controller_->
+          ShouldDeferGestureTapDown(gesture_event);
+    case WebInputEvent::GestureTapCancel:
+      return !touchscreen_tap_suppression_controller_->
+          ShouldSuppressGestureTapCancel();
+    case WebInputEvent::GestureTap:
+      return !touchscreen_tap_suppression_controller_->
+          ShouldSuppressGestureTap();
+    default:
+      return true;
+  }
+  NOTREACHED();
   return false;
 }
 
-// TODO(rjkroege): separate touchpad and touchscreen events.
 bool GestureEventFilter::ShouldForwardForTapDeferral(

[mohsen, 2013/03/07 22:31:41]

ShouldForwardForTapDeferral() is divided into 3 serial filters: 1.
ShouldForwardForGFCFiltering(), 2. ShouldForwardForTapDeferral() and 3.
ShouldForwardForCoalescing(). 1 and 2 are separated to add tap suppression
between them. 2 and 3 are separate to add more readability and also ease reuse
of 3 alone in other parts of code.

     const WebGestureEvent& gesture_event) {
   switch (gesture_event.type) {
-    case WebInputEvent::GestureFlingCancel:
-      if (!ShouldDiscardFlingCancelEvent(gesture_event)) {
-        coalesced_gesture_events_.push_back(gesture_event);
-        fling_in_progress_ = false;
-        tap_suppression_controller_->GestureFlingCancel();
-        return ShouldHandleEventNow();
-      }
-      return false;
     case WebInputEvent::GestureTapDown:
       // GestureTapDown is always paired with either a Tap, or TapCancel, so it
       // should be impossible to have more than one outstanding at a time.
       DCHECK_EQ(deferred_tap_down_event_.type, WebInputEvent::Undefined);
       deferred_tap_down_event_ = gesture_event;
-      send_gtd_timer_.Start(FROM_HERE,
+      send_gtd_timer_.Start(
+          FROM_HERE,
           base::TimeDelta::FromMilliseconds(maximum_tap_gap_time_ms_),
           this,
           &GestureEventFilter::SendGestureTapDownNow);
@@ -161,8 +193,7 @@ bool GestureEventFilter::ShouldForwardForTapDeferral(
       if (deferred_tap_down_event_.type == WebInputEvent::Undefined) {
         // The TapDown has already been put in the queue, must send the
         // corresponding TapCancel as well.
-        coalesced_gesture_events_.push_back(gesture_event);
-        return ShouldHandleEventNow();
+        return true;
       }
       // Cancelling a deferred TapDown, just drop them on the floor.
       send_gtd_timer_.Stop();
@@ -171,35 +202,42 @@ bool GestureEventFilter::ShouldForwardForTapDeferral(
     case WebInputEvent::GestureTap:
       send_gtd_timer_.Stop();
       if (deferred_tap_down_event_.type != WebInputEvent::Undefined) {
-        coalesced_gesture_events_.push_back(deferred_tap_down_event_);
-        if (ShouldHandleEventNow())
-          render_widget_host_->ForwardGestureEventImmediately(
-              deferred_tap_down_event_);
+        ForwardGestureEventSkipDeferral(deferred_tap_down_event_);
         deferred_tap_down_event_.type = WebInputEvent::Undefined;
-        coalesced_gesture_events_.push_back(gesture_event);
-        return false;
       }
-      coalesced_gesture_events_.push_back(gesture_event);
-      return ShouldHandleEventNow();
+      return true;
     case WebInputEvent::GestureFlingStart:
-      fling_in_progress_ = true;
     case WebInputEvent::GestureScrollBegin:
     case WebInputEvent::GesturePinchBegin:
       send_gtd_timer_.Stop();
       deferred_tap_down_event_.type = WebInputEvent::Undefined;
-      coalesced_gesture_events_.push_back(gesture_event);
-      return ShouldHandleEventNow();
+      return true;
+    default:
+      return true;
+  }
+
+  NOTREACHED();
+  return true;
+}
+
+bool GestureEventFilter::ShouldForwardForCoalescing(
+    const WebGestureEvent& gesture_event) {
+  switch (gesture_event.type) {
+    case WebInputEvent::GestureFlingCancel:
+      fling_in_progress_ = false;
+      break;
+    case WebInputEvent::GestureFlingStart:
+      fling_in_progress_ = true;
+      break;
     case WebInputEvent::GesturePinchUpdate:
     case WebInputEvent::GestureScrollUpdate:
       MergeOrInsertScrollAndPinchEvent(gesture_event);
       return ShouldHandleEventNow();
     default:
-      coalesced_gesture_events_.push_back(gesture_event);
-      return ShouldHandleEventNow();
+      break;
   }
-
-  NOTREACHED();
-  return false;
+  coalesced_gesture_events_.push_back(gesture_event);
+  return ShouldHandleEventNow();
 }
 
 void GestureEventFilter::Reset() {
@@ -221,10 +259,17 @@ void GestureEventFilter::ProcessGestureAck(bool processed, int type) {
     return;
   }
   DCHECK_EQ(coalesced_gesture_events_.front().type, type);
+  if (type == WebInputEvent::GestureFlingCancel) {

[rjkroege, 2013/03/07 22:56:21]

this is not how I wanted to see the code evolve. I would wanted to see touch pad
handling removed from this and the handling and touchpad stuff using the
touchpad_tap_suppression_controller and not pass through the gesture event
filter.

Going forward, it will be easier to maintain this code if touchpad and
touchscreen handling code can be written about and reasoned in isolation.

[mohsen, 2013/03/08 00:14:58]

I can move touchpad TSC out of GEF back to RenderWidgetHostImpl. But, I think it
is better to be done in another CL. Maybe it is better to add a TODO and do it
after this CL lands?

+    if (coalesced_gesture_events_.front().sourceDevice ==
+        WebGestureEvent::Touchscreen)
+      touchscreen_tap_suppression_controller_->GestureFlingCancelAck(processed);
+    else
+      touchpad_tap_suppression_controller_->GestureFlingCancelAck(processed);
+  }
   coalesced_gesture_events_.pop_front();
-  if (type == WebInputEvent::GestureFlingCancel)
-    tap_suppression_controller_->GestureFlingCancelAck(processed);
-  if (!coalesced_gesture_events_.empty() && !ignore_next_ack_) {
+  if (ignore_next_ack_) {
+    ignore_next_ack_ = false;
+  } else if (!coalesced_gesture_events_.empty()) {
     const WebGestureEvent& next_gesture_event =
         coalesced_gesture_events_.front();
     render_widget_host_->ForwardGestureEventImmediately(next_gesture_event);
@@ -241,13 +286,12 @@ void GestureEventFilter::ProcessGestureAck(bool processed, int type) {
         combined_scroll_pinch_ = gfx::Transform();
       }
     }
-  } else if (ignore_next_ack_)
-    ignore_next_ack_ = false;
+  }
 }
 
 TouchpadTapSuppressionController*
-    GestureEventFilter::GetTapSuppressionController() {
-  return tap_suppression_controller_.get();
+    GestureEventFilter::GetTouchpadTapSuppressionController() {
+  return touchpad_tap_suppression_controller_.get();
 }
 
 bool GestureEventFilter::HasQueuedGestureEvents() const {
@@ -271,16 +315,24 @@ bool GestureEventFilter::ShouldHandleEventNow() {
   return coalesced_gesture_events_.size() == 1;
 }
 
+void GestureEventFilter::ForwardGestureEventForDeferral(
+    const WebGestureEvent& gesture_event) {
+  if (ShouldForwardForTapDeferral(gesture_event))
+    ForwardGestureEventSkipDeferral(gesture_event);
+}
+
+void GestureEventFilter::ForwardGestureEventSkipDeferral(
+    const WebGestureEvent& gesture_event) {
+  if (ShouldForwardForCoalescing(gesture_event))
+      render_widget_host_->ForwardGestureEventImmediately(gesture_event);
+}
+
 void GestureEventFilter::SendGestureTapDownNow() {
   // We must not have already sent the deferred TapDown (if we did, we would
   // have stopped the timer, which prevents this task from running - even if
   // it's time had already elapsed).
   DCHECK_EQ(deferred_tap_down_event_.type, WebInputEvent::GestureTapDown);
-  coalesced_gesture_events_.push_back(deferred_tap_down_event_);
-  if (ShouldHandleEventNow()) {
-      render_widget_host_->ForwardGestureEventImmediately(
-          deferred_tap_down_event_);
-  }
+  ForwardGestureEventSkipDeferral(deferred_tap_down_event_);
   deferred_tap_down_event_.type = WebInputEvent::Undefined;
 }
 
@@ -289,7 +341,10 @@ void GestureEventFilter::SendScrollEndingEventsNow() {
   for (GestureEventQueue::iterator it =
       debouncing_deferral_queue_.begin();
       it != debouncing_deferral_queue_.end(); it++) {
-    if (ShouldForwardForTapDeferral(*it)) {
+    if (ShouldForwardForGFCFiltering(*it) &&
+        ShouldForwardForTapSuppression(*it) &&
+        ShouldForwardForTapDeferral(*it) &&
+        ShouldForwardForCoalescing(*it)) {
       render_widget_host_->ForwardGestureEventImmediately(*it);
     }
   }
@@ -303,18 +358,18 @@ void GestureEventFilter::MergeOrInsertScrollAndPinchEvent(
     return;
   }
   WebGestureEvent* last_event = &coalesced_gesture_events_.back();
-  if (coalesced_gesture_events_.size() > 1 &&
-      gesture_event.type == WebInputEvent::GestureScrollUpdate &&
-          last_event->type == WebInputEvent::GestureScrollUpdate &&
-              last_event->modifiers == gesture_event.modifiers) {
+  if (gesture_event.type == WebInputEvent::GestureScrollUpdate &&
+      last_event->type == WebInputEvent::GestureScrollUpdate &&
+      last_event->modifiers == gesture_event.modifiers) {
     last_event->data.scrollUpdate.deltaX +=
         gesture_event.data.scrollUpdate.deltaX;
     last_event->data.scrollUpdate.deltaY +=
         gesture_event.data.scrollUpdate.deltaY;
     return;
-  } else if (coalesced_gesture_events_.size() < 3 ||
+  }
+  if (coalesced_gesture_events_.size() == 2 ||
       (coalesced_gesture_events_.size() == 3 && ignore_next_ack_) ||
-      !ShouldTryMerging(gesture_event,*last_event)) {
+      !ShouldTryMerging(gesture_event, *last_event)) {
     coalesced_gesture_events_.push_back(gesture_event);
     return;
   }
@@ -378,4 +433,4 @@ gfx::Transform GestureEventFilter::GetTransformForEvent(
   }
   return gesture_transform;
 }
-} // namespace content
+}  // namespace content