From 8fbc33c36e764f3e4c8ec4c210638985c97cfab7 Mon Sep 17 00:00:00 2001 From: Kartikaya Gupta Date: Mon, 10 Feb 2014 17:34:24 -0500 Subject: [PATCH] Bug 965381 - Delay the single tap notification until after the touchdown is handled. r=daleharvey --- gfx/layers/ipc/AsyncPanZoomController.cpp | 16 ++++++- .../gtest/TestAsyncPanZoomController.cpp | 47 ++++++++++++++----- 2 files changed, 49 insertions(+), 14 deletions(-) diff --git a/gfx/layers/ipc/AsyncPanZoomController.cpp b/gfx/layers/ipc/AsyncPanZoomController.cpp index ea29bf7b66ea..36a59e4f612b 100644 --- a/gfx/layers/ipc/AsyncPanZoomController.cpp +++ b/gfx/layers/ipc/AsyncPanZoomController.cpp @@ -963,7 +963,15 @@ nsEventStatus AsyncPanZoomController::OnSingleTapUp(const TapGestureInput& aEven int32_t modifiers = WidgetModifiersToDOMModifiers(aEvent.modifiers); CSSIntPoint geckoScreenPoint; if (ConvertToGecko(aEvent.mPoint, &geckoScreenPoint)) { - controller->HandleSingleTap(geckoScreenPoint, modifiers, GetGuid()); + // Because this may be being running as part of APZCTreeManager::ReceiveInputEvent, + // calling controller->HandleSingleTap directly might mean that content receives + // the single tap message before the corresponding touch-up. To avoid that we + // schedule the singletap message to run on the next spin of the event loop. + // See bug 965381 for the issue this was causing. + controller->PostDelayedTask( + NewRunnableMethod(controller.get(), &GeckoContentController::HandleSingleTap, + geckoScreenPoint, modifiers, GetGuid()), + 0); return nsEventStatus_eConsumeNoDefault; } } @@ -977,7 +985,11 @@ nsEventStatus AsyncPanZoomController::OnSingleTapConfirmed(const TapGestureInput int32_t modifiers = WidgetModifiersToDOMModifiers(aEvent.modifiers); CSSIntPoint geckoScreenPoint; if (ConvertToGecko(aEvent.mPoint, &geckoScreenPoint)) { - controller->HandleSingleTap(geckoScreenPoint, modifiers, GetGuid()); + // See comment in OnSingleTapUp as to why we do this in PostDelayedTask. + controller->PostDelayedTask( + NewRunnableMethod(controller.get(), &GeckoContentController::HandleSingleTap, + geckoScreenPoint, modifiers, GetGuid()), + 0); return nsEventStatus_eConsumeNoDefault; } } diff --git a/gfx/tests/gtest/TestAsyncPanZoomController.cpp b/gfx/tests/gtest/TestAsyncPanZoomController.cpp index 932b9414d4c7..c72a3f4296d2 100644 --- a/gfx/tests/gtest/TestAsyncPanZoomController.cpp +++ b/gfx/tests/gtest/TestAsyncPanZoomController.cpp @@ -43,11 +43,26 @@ class MockContentControllerDelayed : public MockContentController { public: void PostDelayedTask(Task* aTask, int aDelayMs) { + // Ensure we're not clobbering an existing task + EXPECT_TRUE(nullptr == mCurrentTask); mCurrentTask = aTask; } - Task* GetDelayedTask() { - return mCurrentTask; + void CheckHasDelayedTask() { + EXPECT_TRUE(nullptr != mCurrentTask); + } + + // Note that deleting mCurrentTask is important in order to + // release the reference to the callee object. Without this + // that object might be leaked. This is also why we don't + // expose mCurrentTask to any users of MockContentControllerDelayed. + void RunDelayedTask() { + // Running mCurrentTask may call PostDelayedTask, so we should + // keep a local copy of mCurrentTask and operate on that + Task* local = mCurrentTask; + mCurrentTask = nullptr; + local->Run(); + delete local; } private: @@ -587,7 +602,7 @@ TEST(AsyncPanZoomController, OverScrollPanning) { } TEST(AsyncPanZoomController, ShortPress) { - nsRefPtr mcc = new NiceMock(); + nsRefPtr mcc = new NiceMock(); nsRefPtr tm = new TestAPZCTreeManager(); nsRefPtr apzc = new TestAsyncPanZoomController( 0, mcc, tm, AsyncPanZoomController::USE_GESTURE_DETECTOR); @@ -596,17 +611,22 @@ TEST(AsyncPanZoomController, ShortPress) { apzc->NotifyLayersUpdated(TestFrameMetrics(), true); apzc->UpdateZoomConstraints(ZoomConstraints(false, CSSToScreenScale(1.0), CSSToScreenScale(1.0))); - EXPECT_CALL(*mcc, HandleSingleTap(CSSIntPoint(10, 10), 0, apzc->GetGuid())).Times(1); - int time = 0; nsEventStatus status = ApzcTap(apzc, 10, 10, time, 100); EXPECT_EQ(nsEventStatus_eIgnore, status); + // This verifies that the single tap notification is sent after the + // touchdown is fully processed. The ordering here is important. + mcc->CheckHasDelayedTask(); + + EXPECT_CALL(*mcc, HandleSingleTap(CSSIntPoint(10, 10), 0, apzc->GetGuid())).Times(1); + mcc->RunDelayedTask(); + apzc->Destroy(); } TEST(AsyncPanZoomController, MediumPress) { - nsRefPtr mcc = new NiceMock(); + nsRefPtr mcc = new NiceMock(); nsRefPtr tm = new TestAPZCTreeManager(); nsRefPtr apzc = new TestAsyncPanZoomController( 0, mcc, tm, AsyncPanZoomController::USE_GESTURE_DETECTOR); @@ -615,12 +635,17 @@ TEST(AsyncPanZoomController, MediumPress) { apzc->NotifyLayersUpdated(TestFrameMetrics(), true); apzc->UpdateZoomConstraints(ZoomConstraints(false, CSSToScreenScale(1.0), CSSToScreenScale(1.0))); - EXPECT_CALL(*mcc, HandleSingleTap(CSSIntPoint(10, 10), 0, apzc->GetGuid())).Times(1); - int time = 0; nsEventStatus status = ApzcTap(apzc, 10, 10, time, 400); EXPECT_EQ(nsEventStatus_eIgnore, status); + // This verifies that the single tap notification is sent after the + // touchdown is fully processed. The ordering here is important. + mcc->CheckHasDelayedTask(); + + EXPECT_CALL(*mcc, HandleSingleTap(CSSIntPoint(10, 10), 0, apzc->GetGuid())).Times(1); + mcc->RunDelayedTask(); + apzc->Destroy(); } @@ -639,13 +664,11 @@ TEST(AsyncPanZoomController, LongPress) { nsEventStatus status = ApzcDown(apzc, 10, 10, time); EXPECT_EQ(nsEventStatus_eConsumeNoDefault, status); - Task* t = mcc->GetDelayedTask(); - - EXPECT_TRUE(nullptr != t); + mcc->CheckHasDelayedTask(); EXPECT_CALL(*mcc, HandleLongTap(CSSIntPoint(10, 10), 0, apzc->GetGuid())).Times(1); // Manually invoke the longpress while the touch is currently down. - t->Run(); + mcc->RunDelayedTask(); time += 1000;