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 98227a96d90e662a4bae83f4b067648c6802d971..0f70a7f4b189d1eb410aa015ef82426184a0bbf4 100644
--- a/content/browser/renderer_host/gesture_event_filter.cc
+++ b/content/browser/renderer_host/gesture_event_filter.cc
@@ -7,7 +7,8 @@
 #include "base/command_line.h"
 #include "base/string_number_conversions.h"
 #include "content/browser/renderer_host/render_widget_host_impl.h"
-#include "content/browser/renderer_host/tap_suppression_controller.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;
@@ -61,7 +62,10 @@ GestureEventFilter::GestureEventFilter(RenderWidgetHostImpl* rwhv)
      : render_widget_host_(rwhv),
        fling_in_progress_(false),
        scrolling_in_progress_(false),
-       tap_suppression_controller_(new TapSuppressionController(rwhv)),
+       touchpad_tap_suppression_controller_(
+           new TouchpadTapSuppressionController(rwhv)),
+       touchscreen_tap_suppression_controller_(
+           new TouchscreenTapSuppressionController(rwhv, this)),
        maximum_tap_gap_time_ms_(GetTapDownDeferralTimeMs()),
        debounce_interval_time_ms_(kDebouncingIntervalTimeMs) {
 }
@@ -129,7 +133,44 @@ bool GestureEventFilter::ShouldForward(const WebGestureEvent& gesture_event) {
 
   if (debounce_interval_time_ms_ ==  0 ||
       ShouldForwardForBounceReduction(gesture_event))
-    return ShouldForwardForTapDeferral(gesture_event);
+    return ShouldForwardForGFCFiltering(gesture_event) &&
+        ShouldForwardForTapSuppression(gesture_event) &&
+        ShouldForwardForTapDeferral(gesture_event);
+  return false;
+}
+
+bool GestureEventFilter::ShouldForwardForGFCFiltering(
+    const WebGestureEvent& gesture_event) {

[mohsen, 2013/02/04 15:09:05]

I have extracted GFC filtering into this method to be able to put tap
suppression between this filter and tap deferral.

[rjkroege, 2013/02/05 16:00:07]

sgtm

+  return gesture_event.type != WebInputEvent::GestureFlingCancel ||
+      !ShouldDiscardFlingCancelEvent(gesture_event);
+}
+
+bool GestureEventFilter::ShouldForwardForTapSuppression(

[rjkroege, 2013/02/05 16:00:07]

say what it does?

[mohsen, 2013/02/06 16:13:06]

There is a comment in .h file above this method. It runs the sub-filter for
suppressing taps immediately after a GestureFlingCancel.

+    const WebGestureEvent& gesture_event) {
+  switch (gesture_event.type) {
+    case WebInputEvent::GestureFlingCancel:
+      if (gesture_event.sourceDevice == WebGestureEvent::Touchscreen)
+        touchscreen_tap_suppression_controller_->GestureFlingCancel(
+            gesture_event.timeStampSeconds);
+      else
+        touchpad_tap_suppression_controller_->GestureFlingCancel(
+            gesture_event.timeStampSeconds);
+      last_gfc_source_device_ = gesture_event.sourceDevice;
+      return true;
+    case WebInputEvent::GestureTapDown:
+      return !touchscreen_tap_suppression_controller_->

[rjkroege, 2013/02/05 16:00:07]

I am sad that ShouldForwardForTapSuppression inverts the general convention.

We have methods that are "should forward" and "should defer". This is somewhat
icky. Add a TODO to get around to fixing this.

[mohsen, 2013/02/06 16:13:06]

What convention does it invert? There were some ShouldForwardForX methods for
each sub-filter X. I have added this method in the same style. BTW, I agree that
these ShouldForward/ShouldDiscard methods that negate each others return value
are confusing. A TODO is added in the .h file.

+          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;
 }
 
@@ -138,14 +179,9 @@ bool GestureEventFilter::ShouldForwardForTapDeferral(
     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(
-            gesture_event.timeStampSeconds);
-        return ShouldHandleEventNow();
-      }
-      return false;
+      coalesced_gesture_events_.push_back(gesture_event);
+      fling_in_progress_ = false;
+      return ShouldHandleEventNow();
     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.
@@ -170,10 +206,7 @@ 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_);
+        TryForwardGestureEvent(deferred_tap_down_event_);
         deferred_tap_down_event_.type = WebInputEvent::Undefined;
         coalesced_gesture_events_.push_back(gesture_event);
         return false;
@@ -200,6 +233,13 @@ bool GestureEventFilter::ShouldForwardForTapDeferral(
   return false;
 }
 
+void GestureEventFilter::TryForwardGestureEvent(
+    const WebGestureEvent& gesture_event) {
+  coalesced_gesture_events_.push_back(gesture_event);
+  if (ShouldHandleEventNow())
+      render_widget_host_->ForwardGestureEventImmediately(gesture_event);
+}
+
 void GestureEventFilter::Reset() {
   fling_in_progress_ = false;
   scrolling_in_progress_ = false;
@@ -218,16 +258,20 @@ void GestureEventFilter::ProcessGestureAck(bool processed, int type) {
   }
   DCHECK_EQ(coalesced_gesture_events_.front().type, type);
   coalesced_gesture_events_.pop_front();
-  if (type == WebInputEvent::GestureFlingCancel)
-    tap_suppression_controller_->GestureFlingCancelAck(processed);
+  if (type == WebInputEvent::GestureFlingCancel) {
+    if (last_gfc_source_device_ == WebGestureEvent::Touchscreen)

[rjkroege, 2013/02/05 16:00:07]

I suspect that this might not be robust unless the GFC filtering has ensured
that there is only one GFC in flight. it would be good to test for this.

[mohsen, 2013/02/06 16:13:06]

Since GEF fowards events one by one, i.e. waits for first event's ack and then
sends the next event, we are always waiting for ack of the event in front of the
queue. So, there should be no problem?
(Also, note that I have replaced last_gfc_source_device_ with
coalesced_gesture_events_.front().sourceDevice)

+      touchscreen_tap_suppression_controller_->GestureFlingCancelAck(processed);
+    else
+      touchpad_tap_suppression_controller_->GestureFlingCancelAck(processed);
+  }
   if (!coalesced_gesture_events_.empty()) {
     WebGestureEvent next_gesture_event = coalesced_gesture_events_.front();
     render_widget_host_->ForwardGestureEventImmediately(next_gesture_event);
   }
 }
 
-TapSuppressionController*  GestureEventFilter::GetTapSuppressionController() {
-  return tap_suppression_controller_.get();
+TouchpadTapSuppressionController* GestureEventFilter::GetTouchpadTapSuppressionController() {

[mohsen, 2013/02/04 15:09:05]

Does this line need to be broken?

[rjkroege, 2013/02/05 16:00:07]

yes

[mohsen, 2013/02/06 16:13:06]

Done.

+  return touchpad_tap_suppression_controller_.get();
 }
 
 bool GestureEventFilter::HasQueuedGestureEvents() const {
@@ -248,16 +292,23 @@ bool GestureEventFilter::ShouldHandleEventNow() {
   return coalesced_gesture_events_.size() == 1;
 }
 
+void GestureEventFilter::ForwardGestureEventForDeferral(
+    const WebGestureEvent& gesture_event) {
+  if (ShouldForwardForTapDeferral(gesture_event))
+    TryForwardGestureEvent(gesture_event);
+}
+
+void GestureEventFilter::ForwardGestureEventSkipDeferral(
+    const WebGestureEvent& gesture_event) {
+  TryForwardGestureEvent(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_);
-  }
+  TryForwardGestureEvent(deferred_tap_down_event_);
   deferred_tap_down_event_.type = WebInputEvent::Undefined;
 }
 
@@ -266,7 +317,9 @@ 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)) {
       render_widget_host_->ForwardGestureEventImmediately(*it);
     }
   }
@@ -295,4 +348,4 @@ void GestureEventFilter::MergeOrInsertScrollEvent(
   }
 }
 
-} // namespace content
+}  // namespace content