Suppress sending mousedown / mouseup when in fling

Suppress sending mousedown / mouseup when in fling

Tapping on the touchpad during a fling should stop the scroll, but
should not generate mousedown and mouseup events.
TapSuppressionController is used to suppress mousedown / mouseup events
in this case.

Also, added some tests for TapSuppressionController to content unit tests.

BUG=152287
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=171338

[mohsen, 2012-11-07 16:51:53]

Please review!

[rjkroege, 2012-11-07 19:04:14]

On 2012/11/07 16:51:53, mohsen wrote:
> Please review!

looks reasonable. please add some tests.

[mohsen, 2012-11-15 23:38:14]

On 2012/11/07 19:04:14, rjkroege wrote:
> On 2012/11/07 16:51:53, mohsen wrote:
> > Please review!
> 
> looks reasonable. please add some tests.

I have added some unit tests. I have also removed redundant calls to
scoped_ptr::get() from my previous patch.

[rjkroege, 2012-11-16 16:33:55]

this is looking really good. some minor changes I think and and it will be good
to commit.

https://codereview.chromium.org/11361150/diff/3001/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_unittest.cc (right):

https://codereview.chromium.org/11361150/diff/3001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_unittest.cc:13: #include
"content/browser/renderer_host/tap_suppression_controller.h"
these need to be in alphabetical order.

https://codereview.chromium.org/11361150/diff/3001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_unittest.cc:1389: // Test
TapSuppressionController - GestureFlingCancel Ack comes before
comments need to be complete sentences. with periods and stuff.

https://codereview.chromium.org/11361150/diff/3001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_unittest.cc:1432: // MouseDown
- Should be suppressed
please check if the test sets the tap cancel window so that the test is not
fragile in the presence of configuration changes.

https://codereview.chromium.org/11361150/diff/3001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_unittest.cc:1442: // MouseUp -
Should be suppressed
there are two cases: mouseup in the time interval and mouseup after the time
interval. It is excellent that you have tested both.

https://codereview.chromium.org/11361150/diff/3001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_unittest.cc:1644:
TimeDelta::FromMilliseconds(500));
you need to set time window parameters and adjust these contstants
appropriately. Delays of this size (.5 s) will appreciably slow the builders.

https://codereview.chromium.org/11361150/diff/3001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_unittest.cc:1731: // Test
TapSuppressionController - GestureFlingCancel Ack comes after MouseDown
this comment is the same as the above. but the test is different. the comment
should reflect  this

[mohsen, 2012-11-19 19:30:27]

I have replied to reviews and added some questions, too. Take a look please.

https://codereview.chromium.org/11361150/diff/3001/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_unittest.cc (right):

https://codereview.chromium.org/11361150/diff/3001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_unittest.cc:13: #include
"content/browser/renderer_host/tap_suppression_controller.h"
On 2012/11/16 16:33:57, rjkroege wrote:
> these need to be in alphabetical order.
I have put the header I included in the right place, I think. Do I need to
correct the order of headers from previous commits?

https://codereview.chromium.org/11361150/diff/3001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_unittest.cc:78: };
Since TapSuppressionController::State enum was not accessible to test codes, I
made a public copy of it here. Is this the right way to solve the problem?

https://codereview.chromium.org/11361150/diff/3001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_unittest.cc:1389: // Test
TapSuppressionController - GestureFlingCancel Ack comes before
On 2012/11/16 16:33:57, rjkroege wrote:
> comments need to be complete sentences. with periods and stuff.

Done.

https://codereview.chromium.org/11361150/diff/3001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_unittest.cc:1432: // MouseDown
- Should be suppressed
On 2012/11/16 16:33:57, rjkroege wrote:
> please check if the test sets the tap cancel window so that the test is not
> fragile in the presence of configuration changes.
I don't get this part. Could you please explain more? What should I check for?
What configuration changes do you mean?

https://codereview.chromium.org/11361150/diff/3001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_unittest.cc:1455:
TEST_F(RenderWidgetHostTest, GFCAckBeforeMouseInsufficientlyLateMouseUp) {
Having a delay between MouseDown and MouseUp has an effect on the behavior of
TSC. In the previous test, I've put no delay. In the next test, I've put a long
enough delay so that the two events are not considered a single tap. In this
event I have put a delay, but not long enough to prevent the two events forming
a single tap. So, this case has the same effect as having no delay. Is this a
valuable test or not, since it is somehow like the previous test? Specially
considering that eventually we will make delays in TSC configurable to set them
to small values before each test (see reviews under line 1644).

https://codereview.chromium.org/11361150/diff/3001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_unittest.cc:1644:
TimeDelta::FromMilliseconds(500));
On 2012/11/16 16:33:57, rjkroege wrote:
> you need to set time window parameters and adjust these contstants
> appropriately. Delays of this size (.5 s) will appreciably slow the builders.
These parameters are not configurable by now. For now, I will add a TODO to make
them configurable later and adjust them here for tests.

https://codereview.chromium.org/11361150/diff/3001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_unittest.cc:1731: // Test
TapSuppressionController - GestureFlingCancel Ack comes after MouseDown
On 2012/11/16 16:33:57, rjkroege wrote:
> this comment is the same as the above. but the test is different. the comment
> should reflect  this
Something was messed up in comments. Corrected.

[rjkroege, 2012-11-19 22:18:01]

https://codereview.chromium.org/11361150/diff/3001/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_unittest.cc (right):

https://codereview.chromium.org/11361150/diff/3001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_unittest.cc:78: };
On 2012/11/19 19:30:27, mohsen wrote:
> Since TapSuppressionController::State enum was not accessible to test codes, I
> made a public copy of it here. Is this the right way to solve the problem?

This is fine.

https://codereview.chromium.org/11361150/diff/3001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_unittest.cc:1432: // MouseDown
- Should be suppressed
On 2012/11/19 19:30:27, mohsen wrote:
> On 2012/11/16 16:33:57, rjkroege wrote:
> > please check if the test sets the tap cancel window so that the test is not
> > fragile in the presence of configuration changes.
> I don't get this part. Could you please explain more? What should I check for?
> What configuration changes do you mean?

someday, tsc times (kMaxiumTapGapTimeMs, kMaxiumCancelToDownTimeMs) will be
configurable. because you are assuming these values, this test is a) brittle and
b) takes too long to run.

So: modify TSC to make these values configurable in some fashion and set them to
explicit (smaller) values in the test.

https://codereview.chromium.org/11361150/diff/3001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_unittest.cc:1455:
TEST_F(RenderWidgetHostTest, GFCAckBeforeMouseInsufficientlyLateMouseUp) {
On 2012/11/19 19:30:27, mohsen wrote:
> Having a delay between MouseDown and MouseUp has an effect on the behavior of
> TSC. In the previous test, I've put no delay. In the next test, I've put a
long
> enough delay so that the two events are not considered a single tap. In this
> event I have put a delay, but not long enough to prevent the two events
forming
> a single tap. So, this case has the same effect as having no delay. Is this a
> valuable test or not, since it is somehow like the previous test? Specially
> considering that eventually we will make delays in TSC configurable to set
them
> to small values before each test (see reviews under line 1644).

you need this test (but adjusted) for configurable times.

https://codereview.chromium.org/11361150/diff/3001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_unittest.cc:1793: // Test
TapSuppressionController - GestureFlingCancel Ack comes after MouseDonw
MouseDone

https://codereview.chromium.org/11361150/diff/3001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_unittest.cc:1794: // and there
is a long delay between the Ack and MouseUp
period at the end.

[mohsen, 2012-11-19 22:51:12]

https://codereview.chromium.org/11361150/diff/3001/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_unittest.cc (right):

https://codereview.chromium.org/11361150/diff/3001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_unittest.cc:1432: // MouseDown
- Should be suppressed
On 2012/11/19 22:18:01, rjkroege wrote:
> On 2012/11/19 19:30:27, mohsen wrote:
> > On 2012/11/16 16:33:57, rjkroege wrote:
> > > please check if the test sets the tap cancel window so that the test is
not
> > > fragile in the presence of configuration changes.
> > I don't get this part. Could you please explain more? What should I check
for?
> > What configuration changes do you mean?
> 
> someday, tsc times (kMaxiumTapGapTimeMs, kMaxiumCancelToDownTimeMs) will be
> configurable. because you are assuming these values, this test is a) brittle
and
> b) takes too long to run.
> 
> So: modify TSC to make these values configurable in some fashion and set them
to
> explicit (smaller) values in the test.

So, this is the same point you mentioned under line 1644? If yes, a TODO has
been added to make values configurable.

https://codereview.chromium.org/11361150/diff/3001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_unittest.cc:1455:
TEST_F(RenderWidgetHostTest, GFCAckBeforeMouseInsufficientlyLateMouseUp) {
On 2012/11/19 22:18:01, rjkroege wrote:
> On 2012/11/19 19:30:27, mohsen wrote:
> > Having a delay between MouseDown and MouseUp has an effect on the behavior
of
> > TSC. In the previous test, I've put no delay. In the next test, I've put a
> long
> > enough delay so that the two events are not considered a single tap. In this
> > event I have put a delay, but not long enough to prevent the two events
> forming
> > a single tap. So, this case has the same effect as having no delay. Is this
a
> > valuable test or not, since it is somehow like the previous test? Specially
> > considering that eventually we will make delays in TSC configurable to set
> them
> > to small values before each test (see reviews under line 1644).
> 
> you need this test (but adjusted) for configurable times.

So, I need to add similar tests in other places we have delays?

https://codereview.chromium.org/11361150/diff/3001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_unittest.cc:1793: // Test
TapSuppressionController - GestureFlingCancel Ack comes after MouseDonw
On 2012/11/19 22:18:01, rjkroege wrote:
> MouseDone

Corrected (MouseDown, in fact).

https://codereview.chromium.org/11361150/diff/3001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_unittest.cc:1794: // and there
is a long delay between the Ack and MouseUp
On 2012/11/19 22:18:01, rjkroege wrote:
> period at the end.

All comments rewritten in full sentence form.

[rjkroege, 2012-11-19 23:33:04]

lgtm. 

On 2012/11/19 22:51:12, mohsen wrote:
>
https://codereview.chromium.org/11361150/diff/3001/content/browser/renderer_h...
> File content/browser/renderer_host/render_widget_host_unittest.cc (right):
> 
>
https://codereview.chromium.org/11361150/diff/3001/content/browser/renderer_h...
> content/browser/renderer_host/render_widget_host_unittest.cc:1432: //
MouseDown
> - Should be suppressed
> On 2012/11/19 22:18:01, rjkroege wrote:
> > On 2012/11/19 19:30:27, mohsen wrote:
> > > On 2012/11/16 16:33:57, rjkroege wrote:
> > > > please check if the test sets the tap cancel window so that the test is
> not
> > > > fragile in the presence of configuration changes.
> > > I don't get this part. Could you please explain more? What should I check
> for?
> > > What configuration changes do you mean?
> > 
> > someday, tsc times (kMaxiumTapGapTimeMs, kMaxiumCancelToDownTimeMs) will be
> > configurable. because you are assuming these values, this test is a) brittle
> and
> > b) takes too long to run.
> > 
> > So: modify TSC to make these values configurable in some fashion and set
them
> to
> > explicit (smaller) values in the test.
> 
> So, this is the same point you mentioned under line 1644? If yes, a TODO has
> been added to make values configurable.

file a bug for this and assign it to yourself. And CC me.



> 
>
https://codereview.chromium.org/11361150/diff/3001/content/browser/renderer_h...
> content/browser/renderer_host/render_widget_host_unittest.cc:1455:
> TEST_F(RenderWidgetHostTest, GFCAckBeforeMouseInsufficientlyLateMouseUp) {
> On 2012/11/19 22:18:01, rjkroege wrote:
> > On 2012/11/19 19:30:27, mohsen wrote:
> > > Having a delay between MouseDown and MouseUp has an effect on the behavior
> of
> > > TSC. In the previous test, I've put no delay. In the next test, I've put a
> > long
> > > enough delay so that the two events are not considered a single tap. In
this
> > > event I have put a delay, but not long enough to prevent the two events
> > forming
> > > a single tap. So, this case has the same effect as having no delay. Is
this
> a
> > > valuable test or not, since it is somehow like the previous test?
Specially
> > > considering that eventually we will make delays in TSC configurable to set
> > them
> > > to small values before each test (see reviews under line 1644).
> > 
> > you need this test (but adjusted) for configurable times.
> 
> So, I need to add similar tests in other places we have delays?
> 
>
https://codereview.chromium.org/11361150/diff/3001/content/browser/renderer_h...
> content/browser/renderer_host/render_widget_host_unittest.cc:1793: // Test
> TapSuppressionController - GestureFlingCancel Ack comes after MouseDonw
> On 2012/11/19 22:18:01, rjkroege wrote:
> > MouseDone
> 
> Corrected (MouseDown, in fact).
> 
>
https://codereview.chromium.org/11361150/diff/3001/content/browser/renderer_h...
> content/browser/renderer_host/render_widget_host_unittest.cc:1794: // and
there
> is a long delay between the Ack and MouseUp
> On 2012/11/19 22:18:01, rjkroege wrote:
> > period at the end.
> 
> All comments rewritten in full sentence form.

[commit-bot: I haz the power, 2012-11-20 15:58:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mohsen@chromium.org/11361150/10002

[commit-bot: I haz the power, 2012-11-20 15:59:03]

Failed to apply patch for
content/browser/renderer_host/render_widget_host_unittest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file content/browser/renderer_host/render_widget_host_unittest.cc
  Hunk #1 FAILED at 10.
  Hunk #2 succeeded at 117 with fuzz 2 (offset 49 lines).
  Hunk #3 succeeded at 163 with fuzz 1 (offset 55 lines).
  Hunk #4 succeeded at 518 (offset 75 lines).
  Hunk #5 succeeded at 1520 (offset 135 lines).
  1 out of 5 hunks FAILED -- saving rejects to file
content/browser/renderer_host/render_widget_host_unittest.cc.rej

Patch:       content/browser/renderer_host/render_widget_host_unittest.cc
Index: content/browser/renderer_host/render_widget_host_unittest.cc
diff --git a/content/browser/renderer_host/render_widget_host_unittest.cc
b/content/browser/renderer_host/render_widget_host_unittest.cc
index
ce518c5c09eaba2a0c44bb486406bd40bdfae555..b00185b09d1990b5a59699440430b10df38e9bb9
100644
--- a/content/browser/renderer_host/render_widget_host_unittest.cc
+++ b/content/browser/renderer_host/render_widget_host_unittest.cc
@@ -10,6 +10,7 @@
 #include "content/browser/renderer_host/backing_store.h"
 #include "content/browser/renderer_host/render_widget_host_delegate.h"
 #include "content/browser/renderer_host/gesture_event_filter.h"
+#include "content/browser/renderer_host/tap_suppression_controller.h"
 #include "content/browser/renderer_host/test_render_view_host.h"
 #include "content/browser/renderer_host/touch_event_queue.h"
 #include "content/common/view_messages.h"
@@ -68,6 +69,14 @@ class MockRenderWidgetHost : public RenderWidgetHostImpl {
   using RenderWidgetHostImpl::gesture_event_filter_;
   using RenderWidgetHostImpl::touch_event_queue_;
 
+  enum TapSuppressionState {
+    TSC_NOTHING = TapSuppressionController::NOTHING,
+    TSC_GFC_IN_PROGRESS = TapSuppressionController::GFC_IN_PROGRESS,
+    TSC_MD_STASHED = TapSuppressionController::MD_STASHED,
+    TSC_LAST_CANCEL_STOPPED_FLING =
+      TapSuppressionController::LAST_CANCEL_STOPPED_FLING,
+  };
+
   bool unresponsive_timer_fired() const {
     return unresponsive_timer_fired_;
   }
@@ -100,6 +109,11 @@ class MockRenderWidgetHost : public RenderWidgetHostImpl {
     return gesture_event_filter_->fling_in_progress_;
   }
 
+  TapSuppressionState TapSuppressionControllerState() {
+    return static_cast<TapSuppressionState>(
+        gesture_event_filter_->tap_suppression_controller_->state_);
+  }
+
   void set_maximum_tap_gap_time_ms(int delay_ms) {
     gesture_event_filter_->maximum_tap_gap_time_ms_ = delay_ms;
   }
@@ -430,6 +444,12 @@ class RenderWidgetHostTest : public testing::Test {
     host_->ForwardKeyboardEvent(key_event);
   }
 
+  void SimulateMouseEvent(WebInputEvent::Type type) {
+    WebMouseWheelEvent mouse_event;
+    mouse_event.type = type;
+    host_->ForwardMouseEvent(mouse_event);
+  }
+
   void SimulateWheelEvent(float dX, float dY, int modifiers) {
     WebMouseWheelEvent wheel_event;
     wheel_event.type = WebInputEvent::MouseWheel;
@@ -1366,6 +1386,652 @@ TEST_F(RenderWidgetHostTest,
DebounceDropsDeferredEvents) {
   }
 }
 
+// Test TapSuppressionController for when GestureFlingCancel Ack comes before
+// MouseDown and everything happens without any delays.
+TEST_F(RenderWidgetHostTest, GFCAckBeforeMouseFast) {
+  process_->sink().ClearMessages();
+
+  // Send GestureFlingStart.
+  SimulateGestureFlingStartEvent(0, -10);
+  EXPECT_EQ(1U, process_->sink().message_count());
+  EXPECT_EQ(1U, host_->GestureEventLastQueueEventSize());
+  EXPECT_EQ(WebInputEvent::GestureFlingStart,
+            host_->GestureEventLastQueueEvent().type);
+  EXPECT_EQ(MockRenderWidgetHost::TSC_NOTHING,
+            host_->TapSuppressionControllerState());
+  EXPECT_TRUE(host_->FlingInProgress());
+
+  // Send GestureFlingStart Ack.
+  SendInputEventACK(WebInputEvent::GestureFlingStart, true);
+  MessageLoop::current()->RunUntilIdle();
+  EXPECT_EQ(1U, process_->sink().message_count());
+  EXPECT_EQ(0U, host_->GestureEventLastQueueEventSize());
+  EXPECT_EQ(MockRenderWidgetHost::TSC_NOTHING,
+            host_->TapSuppressionControllerState());
+  EXPECT_TRUE(host_->FlingInProgress());
+
+  // Send GestureFlingCancel.
+  SimulateGestureEvent(WebInputEvent::GestureFlingCancel);
+  EXPECT_EQ(2U, process_->sink().message_count());
+  EXPECT_EQ(1U, host_->GestureEventLastQueueEventSize());
+  EXPECT_EQ(WebInputEvent::GestureFlingCancel,
+            host_->GestureEventLastQueueEvent().type);
+  EXPECT_EQ(MockRenderWidgetHost::TSC_GFC_IN_PROGRESS,
+            host_->TapSuppressionControllerState());
+  EXPECT_FALSE(host_->FlingInProgress());
+
+  // Send GestureFlingCancel Ack.
+  SendInputEventACK(WebInputEvent::GestureFlingCancel, true);
+  MessageLoop::current()->RunUntilIdle();
+  EXPECT_EQ(2U, process_->sink().message_count());
+  EXPECT_EQ(0U, host_->GestureEventLastQueueEventSize());
+  EXPECT_EQ(MockRenderWidgetHost::TSC_LAST_CANCEL_STOPPED_FLING,
+            host_->TapSuppressionControllerState());
+  EXPECT_FALSE(host_->FlingInProgress());
+
+  // Send MouseDown. This MouseDown should be suppressed.
+  SimulateMouseEvent(WebInputEvent::MouseDown);
+  EXPECT_EQ(2U, process_->sink().message_count());
+  EXPECT_EQ(0U, host_->GestureEventLastQueueEventSize());
+  EXPECT_EQ(WebInputEvent::GestureFlingCancel,
+            host_->GestureEventLastQueueEvent().type);
+  EXPECT_EQ(MockRenderWidgetHost::TSC_MD_STASHED,
+            host_->TapSuppressionControllerState());
+  EXPECT_FALSE(host_->FlingInProgress());
+
+  // Send MouseUp. This MouseUp should be suppressed.
+  SimulateMouseEvent(WebInputEvent::MouseUp);
+  EXPECT_EQ(2U, process_->sink().message_count());
+  EXPECT_EQ(0U, host_->GestureEventLastQueueEventSize());
+  EXPECT_EQ(WebInputEvent::GestureFlingCancel,
+             host_->GestureEventLastQueueEvent().type);
+  EXPECT_EQ(MockRenderWidgetHost::TSC_NOTHING,
+            host_->TapSuppressionControllerState());
+  EXPECT_FALSE(host_->FlingInProgress());
+}
+
+// Test TapSuppressionController for when GestureFlingCancel Ack comes before
+// MouseDown, but there is a small delay between MouseDown and MouseUp.
+TEST_F(RenderWidgetHostTest, GFCAckBeforeMouseInsufficientlyLateMouseUp) {
+  process_->sink().ClearMessages();
+
+  // Send GestureFlingStart.
+  SimulateGestureEvent(WebInputEvent::GestureFlingStart);
+  EXPECT_EQ(1U, process_->sink().message_count());
+  EXPECT_EQ(1U, host_->GestureEventLastQueueEventSize());
+  EXPECT_EQ(WebInputEvent::GestureFlingStart,
+            host_->GestureEventLastQueueEvent().type);
+  EXPECT_EQ(MockRenderWidgetHost::TSC_NOTHING,
+            host_->TapSuppressionControllerState());
+  EXPECT_TRUE(host_->FlingInProgress());
+
+  // Send GestureFlingStart Ack.
+  SendInputEventACK(WebInputEvent::GestureFlingStart, true);
+  MessageLoop::current()->RunUntilIdle();
+  EXPECT_EQ(1U, process_->sink().message_count());
+  EXPECT_EQ(0U, host_->GestureEventLastQueueEventSize());
+  EXPECT_EQ(MockRenderWidgetHost::TSC_NOTHING,
+            host_->TapSuppressionControllerState());
+  EXPECT_TRUE(host_->FlingInProgress());
+
+  // Send GestureFlingCancel.
+  SimulateGestureEvent(WebInputEvent::GestureFlingCancel);
+  EXPECT_EQ(2U, process_->sink().message_count());
+  EXPECT_EQ(1U, host_->GestureEventLastQueueEventSize());
+  EXPECT_EQ(WebInputEvent::GestureFlingCancel,
+            host_->GestureEventLastQueueEvent().type);
+  EXPECT_EQ(MockRenderWidgetHost::TSC_GFC_IN_PROGRESS,
+            host_->TapSuppressionControllerState());
+  EXPECT_FALSE(host_->FlingInProgress());
+
+  // Send GestureFlingCancel Ack.
+  SendInputEventACK(WebInputEvent::GestureFlingCancel, true);
+  MessageLoop::current()->RunUntilIdle();
+  EXPECT_EQ(2U, process_->sink().message_count());
+  EXPECT_EQ(0U, host_->GestureEventLastQueueEventSize());
+  EXPECT_EQ(MockRenderWidgetHost::TSC_LAST_CANCEL_STOPPED_FLING,
+            host_->TapSuppressionControllerState());
+  EXPECT_FALSE(host_->FlingInProgress());
+
+  // Send MouseDown. This MouseDown should be suppressed.
+  SimulateMouseEvent(WebInputEvent::MouseDown);
+  EXPECT_EQ(2U, process_->sink().message_count());
+  EXPECT_EQ(0U, host_->GestureEventLastQueueEventSize());
+  EXPECT_EQ(MockRenderWidgetHost::TSC_MD_STASHED,
+            host_->TapSuppressionControllerState());
+  EXPECT_FALSE(host_->FlingInProgress());
+
+  // Wait less than allowed delay between MouseDown and MouseUp, so they are
+  // still considered a tap.
+  // TODO(mohsen): The amounts used for delays are too much and will slow down
+  // the tests. A better way is to reduce the allowed delays in TSC before each
+  // test. So, they should be made accessible and configurable.
+  MessageLoop::current()->PostDelayedTask(FROM_HERE,
+                                          MessageLoop::QuitClosure(),
+                                          TimeDelta::FromMilliseconds(100));
+  MessageLoop::current()->Run();
+  EXPECT_EQ(2U, process_->sink().message_count());
+  EXPECT_EQ(0U, host_->GestureEventLastQueueEventSize());
+  EXPECT_EQ(MockRenderWidgetHost::TSC_MD_STASHED,
+            host_->TapSuppressionControllerState());
+  EXPECT_FALSE(host_->FlingInProgress());
+
+  // Send MouseUp. This MouseUp should be suppressed.
+  SimulateMouseEvent(WebInputEvent::MouseUp);
+  EXPECT_EQ(2U, process_->sink().message_count());
+  EXPECT_EQ(0U, host_->GestureEventLastQueueEventSize());
+  EXPECT_EQ(MockRenderWidgetHost::TSC_NOTHING,
+            host_->TapSuppressionControllerState());
+  EXPECT_FALSE(host_->FlingInProgress());
+}
+
+// Test TapSuppressionController for when GestureFlingCancel Ack comes before
+// MouseDown, but there is a long delay between MouseDown and MouseUp.
+TEST_F(RenderWidgetHostTest, GFCAckBeforeMouseSufficientlyLateMouseUp) {
+  process_->sink().ClearMessages();
+
+  // Send GestureFlingStart.
+  SimulateGestureEvent(WebInputEvent::GestureFlingStart);
+  EXPECT_EQ(1U, process_->sink().message_count());
+  EXPECT_EQ(…
(message too large)

[commit-bot: I haz the power, 2012-11-20 16:18:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mohsen@chromium.org/11361150/6003

[commit-bot: I haz the power, 2012-11-20 17:29:13]

Retried try job too often for step(s) content_unittests

[commit-bot: I haz the power, 2012-11-21 04:44:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mohsen@chromium.org/11361150/5009

[commit-bot: I haz the power, 2012-11-21 05:44:38]

Retried try job too often for step(s) content_unittests

[mohsen, 2012-11-21 18:34:53]

https://codereview.chromium.org/11361150/diff/18006/content/browser/renderer_...
File content/browser/renderer_host/tap_suppression_controller_aura.cc (left):

https://codereview.chromium.org/11361150/diff/18006/content/browser/renderer_...
content/browser/renderer_host/tap_suppression_controller_aura.cc:136: state_ =
NOTHING;
If MouseDown is forwarded before setting the state_ to NOTHING, the
ShouldDeferMouseDown() method will be called while state_ is still set to
MD_STASHED, which is not desirable.

https://codereview.chromium.org/11361150/diff/18006/content/browser/renderer_...
content/browser/renderer_host/tap_suppression_controller_aura.cc:168: state_ =
NOTHING;
The same as note under line 136.

[rjkroege, 2012-11-21 20:41:17]

https://codereview.chromium.org/11361150/diff/18006/content/browser/renderer_...
File content/browser/renderer_host/tap_suppression_controller_aura.cc (left):

https://codereview.chromium.org/11361150/diff/18006/content/browser/renderer_...
content/browser/renderer_host/tap_suppression_controller_aura.cc:136: state_ =
NOTHING;
On 2012/11/21 18:34:54, mohsen wrote:
> If MouseDown is forwarded before setting the state_ to NOTHING, the
> ShouldDeferMouseDown() method will be called while state_ is still set to
> MD_STASHED, which is not desirable.

this is indeed an issue. TSC is not tested or intended to be recursive. I would
imagine that there might be other bugs of this kind in the future. I think it
would be better to add a RenderWidgetHostImpl::ForwardMouseEventImmediately
instead?

[mohsen, 2012-11-21 22:11:32]

https://codereview.chromium.org/11361150/diff/18006/content/browser/renderer_...
File content/browser/renderer_host/tap_suppression_controller_aura.cc (left):

https://codereview.chromium.org/11361150/diff/18006/content/browser/renderer_...
content/browser/renderer_host/tap_suppression_controller_aura.cc:136: state_ =
NOTHING;
On 2012/11/21 20:41:18, rjkroege wrote:
> On 2012/11/21 18:34:54, mohsen wrote:
> > If MouseDown is forwarded before setting the state_ to NOTHING, the
> > ShouldDeferMouseDown() method will be called while state_ is still set to
> > MD_STASHED, which is not desirable.
> 
> this is indeed an issue. TSC is not tested or intended to be recursive. I
would
> imagine that there might be other bugs of this kind in the future. I think it
> would be better to add a RenderWidgetHostImpl::ForwardMouseEventImmediately
> instead?

I agree. The only problem is that most of the code for these two functions
(immediate and non-immediate) would be common. Maybe, we had better factor out
those common parts into another function or call the immediate one from inside
the non-immediate one, if it decides to forward the event. What do you think?

(BTW, I think the best solution is to somehow have a one-way relationship
between RWHI/GEF and TSC, i.e. only RWHI/GEF know of TSC.)

[rjkroege, 2012-11-21 22:15:23]

On 2012/11/21 22:11:32, mohsen wrote:
>
https://codereview.chromium.org/11361150/diff/18006/content/browser/renderer_...
> File content/browser/renderer_host/tap_suppression_controller_aura.cc (left):
> 
>
https://codereview.chromium.org/11361150/diff/18006/content/browser/renderer_...
> content/browser/renderer_host/tap_suppression_controller_aura.cc:136: state_ =
> NOTHING;
> On 2012/11/21 20:41:18, rjkroege wrote:
> > On 2012/11/21 18:34:54, mohsen wrote:
> > > If MouseDown is forwarded before setting the state_ to NOTHING, the
> > > ShouldDeferMouseDown() method will be called while state_ is still set to
> > > MD_STASHED, which is not desirable.
> > 
> > this is indeed an issue. TSC is not tested or intended to be recursive. I
> would
> > imagine that there might be other bugs of this kind in the future. I think
it
> > would be better to add a RenderWidgetHostImpl::ForwardMouseEventImmediately
> > instead?
> 
> I agree. The only problem is that most of the code for these two functions
> (immediate and non-immediate) would be common. Maybe, we had better factor out
> those common parts into another function or call the immediate one from inside
> the non-immediate one, if it decides to forward the event. What do you think?
> 
> (BTW, I think the best solution is to somehow have a one-way relationship
> between RWHI/GEF and TSC, i.e. only RWHI/GEF know of TSC.)

the whole entanglement of gef and tsc was a mistake for touchpad. my badness.
maybe it needs to be split out. tap suppression for touch screens can be
implemented separately.

[rjkroege, 2012-11-26 17:53:40]

lgtm

https://codereview.chromium.org/11361150/diff/14010/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_unittest.cc (right):

https://codereview.chromium.org/11361150/diff/14010/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_unittest.cc:1588: #endif  //
defined(USE_AURA)
nit: you could put the whole test block inside a single ifdef

[commit-bot: I haz the power, 2012-11-26 18:33:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mohsen@chromium.org/11361150/8005

[commit-bot: I haz the power, 2012-11-26 19:03:39]

Retried try job too often for step(s) browser_tests

[commit-bot: I haz the power, 2012-11-27 00:05:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mohsen@chromium.org/11361150/8005

[commit-bot: I haz the power, 2012-11-28 16:57:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mohsen@chromium.org/11361150/8005

[commit-bot: I haz the power, 2012-11-28 17:26:11]

Change committed as 169979

[jam, 2012-11-28 20:41:24]

On 2012/11/28 17:26:11, I haz the power (commit-bot) wrote:
> Change committed as 169979

reverting because RenderWidgetHostTest.GFCAckBeforeMouseFast crashes on win
aura. sorry (working on getting win aura added to try and commit queue asap)

[rjkroege, 2012-12-03 19:24:58]

i don't understand why change 10 was needed?

[mohsen, 2012-12-03 20:24:11]

On 2012/12/03 19:24:58, rjkroege wrote:
> i don't understand why change 10 was needed?

That was the reason for win_aura failure. When an stl::deque (which type of the
event queue in GestureEventFilter) is empty, calling back() method on it causes
an assertion failure in MSVC.

[rjkroege, 2012-12-04 15:30:59]

On 2012/12/03 20:24:11, mohsen wrote:
> On 2012/12/03 19:24:58, rjkroege wrote:
> > i don't understand why change 10 was needed?
> 
> That was the reason for win_aura failure. When an stl::deque (which type of
the
> event queue in GestureEventFilter) is empty, calling back() method on it
causes
> an assertion failure in MSVC.

ah.

But you have also weakened the test. I think you need to still verify that that
event that had been in the deque is of the correct type yes?

[mohsen, 2012-12-04 15:52:35]

On 2012/12/04 15:30:59, rjkroege wrote:
> On 2012/12/03 20:24:11, mohsen wrote:
> > On 2012/12/03 19:24:58, rjkroege wrote:
> > > i don't understand why change 10 was needed?
> > 
> > That was the reason for win_aura failure. When an stl::deque (which type of
> the
> > event queue in GestureEventFilter) is empty, calling back() method on it
> causes
> > an assertion failure in MSVC.
> 
> ah.
> 
> But you have also weakened the test. I think you need to still verify that
that
> event that had been in the deque is of the correct type yes?

Since I check contents of the queue in every step, I think what you want is
satisfied, i.e., in each step the event that had been in the queue is verified
in the previous step. By the way, in the two cases that I removed checks in
change 10, there had been nothing in the queue.

[rjkroege, 2012-12-04 21:02:44]

On 2012/12/04 15:52:35, mohsen wrote:
> On 2012/12/04 15:30:59, rjkroege wrote:
> > On 2012/12/03 20:24:11, mohsen wrote:
> > > On 2012/12/03 19:24:58, rjkroege wrote:
> > > > i don't understand why change 10 was needed?
> > > 
> > > That was the reason for win_aura failure. When an stl::deque (which type
of
> > the
> > > event queue in GestureEventFilter) is empty, calling back() method on it
> > causes
> > > an assertion failure in MSVC.
> > 
> > ah.
> > 
> > But you have also weakened the test. I think you need to still verify that
> that
> > event that had been in the deque is of the correct type yes?
> 
> Since I check contents of the queue in every step, I think what you want is
> satisfied, i.e., in each step the event that had been in the queue is verified
> in the previous step. By the way, in the two cases that I removed checks in
> change 10, there had been nothing in the queue.

lgtm

[commit-bot: I haz the power, 2012-12-05 19:39:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mohsen@chromium.org/11361150/12018

[commit-bot: I haz the power, 2012-12-05 19:39:38]

Presubmit check for 11361150-12018 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
The CL description appears to end with a committed link. If this issue
has been previously used for a commit, please make a new issue number
by typing "git cl issue 0" and reuploading your CL.

Presubmit checks took 3.8s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[commit-bot: I haz the power, 2012-12-05 19:46:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mohsen@chromium.org/11361150/12018

[commit-bot: I haz the power, 2012-12-05 19:46:19]

Presubmit check for 11361150-12018 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
The CL description appears to end with a committed link. If this issue
has been previously used for a commit, please make a new issue number
by typing "git cl issue 0" and reuploading your CL.

Presubmit checks took 1.7s to calculate.

[commit-bot: I haz the power, 2012-12-05 19:50:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mohsen@chromium.org/11361150/12018

[commit-bot: I haz the power, 2012-12-05 22:41:07]

Change committed as 171338