From ac8ae1c793e2cff8f0a79bc8c41043c1db0765f4 Mon Sep 17 00:00:00 2001 From: Kartikaya Gupta Date: Fri, 15 Jun 2018 18:13:47 -0400 Subject: [PATCH] Bug 1468545 - Ensure the drag metrics gets to APZ before the target APZC for inactive scrollframes. r=botond In the case where an inactive scrollframe's scrollbar gets dragged, the main thread layerizes the scrollframe and dispatches both a SetTargetAPZC message and a AsyncDragMetrics message using post-refresh observers. However, the post-refresh observers are registered such that the SetTargetAPZC message gets sent first, and APZ will start processing the drag block immediately upon receipt of that event. This means that the APZC might not have the correct drag metrics when it processes those input events. For correct behaviour, we want the AsyncDragMetrics message to reach APZ first in this scenario, and this patch accomplishes this by allowing the post-refresh observers to be registered in the opposite order. MozReview-Commit-ID: 6LzyYYG1t6F --HG-- extra : rebase_source : 2e3ebaa4fd776c8ad17e8225d70ec4a18eee67e0 --- dom/ipc/TabChild.cpp | 36 ++++-- gfx/layers/apz/util/APZCCallbackHelper.cpp | 125 +++++++++------------ gfx/layers/apz/util/APZCCallbackHelper.h | 48 ++++++-- widget/nsBaseWidget.cpp | 12 +- 4 files changed, 122 insertions(+), 99 deletions(-) diff --git a/dom/ipc/TabChild.cpp b/dom/ipc/TabChild.cpp index 127e2afe17a9..c276ae99ff90 100644 --- a/dom/ipc/TabChild.cpp +++ b/dom/ipc/TabChild.cpp @@ -1718,16 +1718,14 @@ TabChild::HandleRealMouseButtonEvent(const WidgetMouseEvent& aEvent, // actually go through the APZ code and so their mHandledByAPZ flag is false. // Since thos events didn't go through APZ, we don't need to send // notifications for them. - bool pendingLayerization = false; + UniquePtr postLayerization; if (aInputBlockId && aEvent.mFlags.mHandledByAPZ) { nsCOMPtr document(GetDocument()); - pendingLayerization = - APZCCallbackHelper::SendSetTargetAPZCNotification(mPuppetWidget, document, - aEvent, aGuid, - aInputBlockId); + postLayerization = APZCCallbackHelper::SendSetTargetAPZCNotification( + mPuppetWidget, document, aEvent, aGuid, aInputBlockId); } - InputAPZContext context(aGuid, aInputBlockId, nsEventStatus_eIgnore, pendingLayerization); + InputAPZContext context(aGuid, aInputBlockId, nsEventStatus_eIgnore, postLayerization != nullptr); WidgetMouseEvent localEvent(aEvent); localEvent.mWidget = mPuppetWidget; @@ -1738,6 +1736,15 @@ TabChild::HandleRealMouseButtonEvent(const WidgetMouseEvent& aEvent, if (aInputBlockId && aEvent.mFlags.mHandledByAPZ) { mAPZEventState->ProcessMouseEvent(aEvent, aGuid, aInputBlockId); } + + // Do this after the DispatchWidgetEventViaAPZ call above, so that if the + // mouse event triggered a post-refresh AsyncDragMetrics message to be sent + // to APZ (from scrollbar dragging in nsSliderFrame), then that will reach + // APZ before the SetTargetAPZC message. This ensures the drag input block + // gets the drag metrics before handling the input events. + if (postLayerization && postLayerization->Register()) { + Unused << postLayerization.release(); + } } mozilla::ipc::IPCResult @@ -1818,8 +1825,12 @@ TabChild::DispatchWheelEvent(const WidgetWheelEvent& aEvent, WidgetWheelEvent localEvent(aEvent); if (aInputBlockId && aEvent.mFlags.mHandledByAPZ) { nsCOMPtr document(GetDocument()); - APZCCallbackHelper::SendSetTargetAPZCNotification( - mPuppetWidget, document, aEvent, aGuid, aInputBlockId); + UniquePtr postLayerization = + APZCCallbackHelper::SendSetTargetAPZCNotification( + mPuppetWidget, document, aEvent, aGuid, aInputBlockId); + if (postLayerization && postLayerization->Register()) { + Unused << postLayerization.release(); + } } localEvent.mWidget = mPuppetWidget; @@ -1894,9 +1905,12 @@ TabChild::RecvRealTouchEvent(const WidgetTouchEvent& aEvent, mPuppetWidget, document, localEvent, aInputBlockId, mSetAllowedTouchBehaviorCallback); } - APZCCallbackHelper::SendSetTargetAPZCNotification(mPuppetWidget, document, - localEvent, aGuid, - aInputBlockId); + UniquePtr postLayerization = + APZCCallbackHelper::SendSetTargetAPZCNotification( + mPuppetWidget, document, localEvent, aGuid, aInputBlockId); + if (postLayerization && postLayerization->Register()) { + Unused << postLayerization.release(); + } } // Dispatch event to content (potentially a long-running operation) diff --git a/gfx/layers/apz/util/APZCCallbackHelper.cpp b/gfx/layers/apz/util/APZCCallbackHelper.cpp index bd2549c8ea3d..ca56bf26f9f4 100644 --- a/gfx/layers/apz/util/APZCCallbackHelper.cpp +++ b/gfx/layers/apz/util/APZCCallbackHelper.cpp @@ -722,70 +722,55 @@ SendLayersDependentApzcTargetConfirmation(nsIPresShell* aShell, uint64_t aInputB shadow->SendSetConfirmedTargetAPZC(aInputBlockId, aTargets); } -class DisplayportSetListener : public nsAPostRefreshObserver { -public: - DisplayportSetListener(nsIPresShell* aPresShell, - const uint64_t& aInputBlockId, - const nsTArray& aTargets) - : mPresShell(aPresShell) - , mInputBlockId(aInputBlockId) - , mTargets(aTargets) - { - } - - virtual ~DisplayportSetListener() - { - } - - void DidRefresh() override { - if (!mPresShell) { - MOZ_ASSERT_UNREACHABLE("Post-refresh observer fired again after failed attempt at unregistering it"); - return; - } - - APZCCH_LOG("Got refresh, sending target APZCs for input block %" PRIu64 "\n", mInputBlockId); - SendLayersDependentApzcTargetConfirmation(mPresShell, mInputBlockId, std::move(mTargets)); - - if (!mPresShell->RemovePostRefreshObserver(this)) { - MOZ_ASSERT_UNREACHABLE("Unable to unregister post-refresh observer! Leaking it instead of leaving garbage registered"); - // Graceful handling, just in case... - mPresShell = nullptr; - return; - } - - delete this; - } - -private: - RefPtr mPresShell; - uint64_t mInputBlockId; - nsTArray mTargets; -}; - -// Sends a SetTarget notification for APZC, given one or more previous -// calls to PrepareForAPZCSetTargetNotification(). -static void -SendSetTargetAPZCNotificationHelper(nsIWidget* aWidget, - nsIPresShell* aShell, - const uint64_t& aInputBlockId, - const nsTArray& aTargets, - bool aWaitForRefresh) +DisplayportSetListener::DisplayportSetListener(nsIWidget* aWidget, + nsIPresShell* aPresShell, + const uint64_t& aInputBlockId, + const nsTArray& aTargets) + : mWidget(aWidget) + , mPresShell(aPresShell) + , mInputBlockId(aInputBlockId) + , mTargets(aTargets) +{ +} + +DisplayportSetListener:: ~DisplayportSetListener() { - bool waitForRefresh = aWaitForRefresh; - if (waitForRefresh) { - APZCCH_LOG("At least one target got a new displayport, need to wait for refresh\n"); - waitForRefresh = aShell->AddPostRefreshObserver( - new DisplayportSetListener(aShell, aInputBlockId, std::move(aTargets))); - } - if (!waitForRefresh) { - APZCCH_LOG("Sending target APZCs for input block %" PRIu64 "\n", aInputBlockId); - aWidget->SetConfirmedTargetAPZC(aInputBlockId, aTargets); - } else { - APZCCH_LOG("Successfully registered post-refresh observer\n"); - } } bool +DisplayportSetListener::Register() +{ + if (mPresShell->AddPostRefreshObserver(this)) { + APZCCH_LOG("Successfully registered post-refresh observer\n"); + return true; + } + // In case of failure just send the notification right away + APZCCH_LOG("Sending target APZCs for input block %" PRIu64 "\n", mInputBlockId); + mWidget->SetConfirmedTargetAPZC(mInputBlockId, mTargets); + return false; +} + +void +DisplayportSetListener::DidRefresh() { + if (!mPresShell) { + MOZ_ASSERT_UNREACHABLE("Post-refresh observer fired again after failed attempt at unregistering it"); + return; + } + + APZCCH_LOG("Got refresh, sending target APZCs for input block %" PRIu64 "\n", mInputBlockId); + SendLayersDependentApzcTargetConfirmation(mPresShell, mInputBlockId, std::move(mTargets)); + + if (!mPresShell->RemovePostRefreshObserver(this)) { + MOZ_ASSERT_UNREACHABLE("Unable to unregister post-refresh observer! Leaking it instead of leaving garbage registered"); + // Graceful handling, just in case... + mPresShell = nullptr; + return; + } + + delete this; +} + +UniquePtr APZCCallbackHelper::SendSetTargetAPZCNotification(nsIWidget* aWidget, nsIDocument* aDocument, const WidgetGUIEvent& aEvent, @@ -793,7 +778,7 @@ APZCCallbackHelper::SendSetTargetAPZCNotification(nsIWidget* aWidget, uint64_t aInputBlockId) { if (!aWidget || !aDocument) { - return false; + return nullptr; } if (aInputBlockId == sLastTargetAPZCNotificationInputBlock) { // We have already confirmed the target APZC for a previous event of this @@ -802,7 +787,7 @@ APZCCallbackHelper::SendSetTargetAPZCNotification(nsIWidget* aWidget, // race the original confirmation (which needs to go through a layers // transaction). APZCCH_LOG("Not resending target APZC confirmation for input block %" PRIu64 "\n", aInputBlockId); - return false; + return nullptr; } sLastTargetAPZCNotificationInputBlock = aInputBlockId; if (nsIPresShell* shell = aDocument->GetShell()) { @@ -827,18 +812,16 @@ APZCCallbackHelper::SendSetTargetAPZCNotification(nsIWidget* aWidget, // TODO: Do other types of events need to be handled? if (!targets.IsEmpty()) { - SendSetTargetAPZCNotificationHelper( - aWidget, - shell, - aInputBlockId, - std::move(targets), - waitForRefresh); + if (waitForRefresh) { + APZCCH_LOG("At least one target got a new displayport, need to wait for refresh\n"); + return MakeUnique(aWidget, shell, aInputBlockId, std::move(targets)); + } + APZCCH_LOG("Sending target APZCs for input block %" PRIu64 "\n", aInputBlockId); + aWidget->SetConfirmedTargetAPZC(aInputBlockId, targets); } - - return waitForRefresh; } } - return false; + return nullptr; } void diff --git a/gfx/layers/apz/util/APZCCallbackHelper.h b/gfx/layers/apz/util/APZCCallbackHelper.h index 8e43a34f7a97..30befef7b923 100644 --- a/gfx/layers/apz/util/APZCCallbackHelper.h +++ b/gfx/layers/apz/util/APZCCallbackHelper.h @@ -11,6 +11,7 @@ #include "mozilla/EventForwards.h" #include "mozilla/layers/APZUtils.h" #include "nsIDOMWindowUtils.h" +#include "nsRefreshDriver.h" #include @@ -28,6 +29,25 @@ namespace layers { typedef std::function&)> SetAllowedTouchBehaviorCallback; +/* Refer to documentation on SendSetTargetAPZCNotification for this class */ +class DisplayportSetListener : public nsAPostRefreshObserver +{ +public: + DisplayportSetListener(nsIWidget* aWidget, + nsIPresShell* aPresShell, + const uint64_t& aInputBlockId, + const nsTArray& aTargets); + virtual ~DisplayportSetListener(); + bool Register(); + void DidRefresh() override; + +private: + RefPtr mWidget; + RefPtr mPresShell; + uint64_t mInputBlockId; + nsTArray mTargets; +}; + /* This class contains some helper methods that facilitate implementing the GeckoContentController callback interface required by the AsyncPanZoomController. Since different platforms need to implement this interface in similar-but- @@ -136,20 +156,24 @@ public: * which scrollable frames they target. If any of these frames don't have * a displayport, set one. * - * If any displayports need to be set, the actual notification to APZ is - * sent to the compositor, which will then post a message back to APZ's - * controller thread. Otherwise, the provided widget's SetConfirmedTargetAPZC - * method is invoked immediately. + * If any displayports need to be set, this function returns a heap-allocated + * object. The caller is responsible for calling Register() on that object, + * and release()'ing the UniquePtr if that Register() call returns true. + * The object registers itself as a post-refresh observer on the presShell + * and ensures that notifications get sent to APZ correctly after the + * refresh. * - * Returns true if any displayports need to be set. (A caller may be - * interested to know this, because they may need to delay certain actions - * until after the displayport comes into effect.) + * Having the caller manage this object is desirable in case they want to + * (a) know about the fact that a displayport needs to be set, and + * (b) register a post-refresh observer of their own that will run in + * a defined ordering relative to the APZ messages. */ - static bool SendSetTargetAPZCNotification(nsIWidget* aWidget, - nsIDocument* aDocument, - const WidgetGUIEvent& aEvent, - const ScrollableLayerGuid& aGuid, - uint64_t aInputBlockId); + static UniquePtr SendSetTargetAPZCNotification( + nsIWidget* aWidget, + nsIDocument* aDocument, + const WidgetGUIEvent& aEvent, + const ScrollableLayerGuid& aGuid, + uint64_t aInputBlockId); /* Figure out the allowed touch behaviors of each touch point in |aEvent| * and send that information to the provided callback. */ diff --git a/widget/nsBaseWidget.cpp b/widget/nsBaseWidget.cpp index a2ba5df5ed10..dd3dda46c7ad 100644 --- a/widget/nsBaseWidget.cpp +++ b/widget/nsBaseWidget.cpp @@ -1083,8 +1083,7 @@ nsBaseWidget::ProcessUntransformedAPZEvent(WidgetInputEvent* aEvent, if (mAPZC && !InputAPZContext::WasRoutedToChildProcess() && aInputBlockId) { // EventStateManager did not route the event into the child process. // It's safe to communicate to APZ that the event has been processed. - // TODO: Eventually we'll be able to move the SendSetTargetAPZCNotification - // call into APZEventState::Process*Event() as well. + UniquePtr postLayerization; if (WidgetTouchEvent* touchEvent = aEvent->AsTouchEvent()) { if (touchEvent->mMessage == eTouchStart) { if (gfxPrefs::TouchActionEnabled()) { @@ -1092,14 +1091,14 @@ nsBaseWidget::ProcessUntransformedAPZEvent(WidgetInputEvent* aEvent, GetDocument(), *(original->AsTouchEvent()), aInputBlockId, mSetAllowedTouchBehaviorCallback); } - APZCCallbackHelper::SendSetTargetAPZCNotification(this, GetDocument(), + postLayerization = APZCCallbackHelper::SendSetTargetAPZCNotification(this, GetDocument(), *(original->AsTouchEvent()), aGuid, aInputBlockId); } mAPZEventState->ProcessTouchEvent(*touchEvent, aGuid, aInputBlockId, aApzResponse, status); } else if (WidgetWheelEvent* wheelEvent = aEvent->AsWheelEvent()) { MOZ_ASSERT(wheelEvent->mFlags.mHandledByAPZ); - APZCCallbackHelper::SendSetTargetAPZCNotification(this, GetDocument(), + postLayerization = APZCCallbackHelper::SendSetTargetAPZCNotification(this, GetDocument(), *(original->AsWheelEvent()), aGuid, aInputBlockId); if (wheelEvent->mCanTriggerSwipe) { ReportSwipeStarted(aInputBlockId, wheelEvent->TriggersSwipe()); @@ -1107,10 +1106,13 @@ nsBaseWidget::ProcessUntransformedAPZEvent(WidgetInputEvent* aEvent, mAPZEventState->ProcessWheelEvent(*wheelEvent, aGuid, aInputBlockId); } else if (WidgetMouseEvent* mouseEvent = aEvent->AsMouseEvent()) { MOZ_ASSERT(mouseEvent->mFlags.mHandledByAPZ); - APZCCallbackHelper::SendSetTargetAPZCNotification(this, GetDocument(), + postLayerization = APZCCallbackHelper::SendSetTargetAPZCNotification(this, GetDocument(), *(original->AsMouseEvent()), aGuid, aInputBlockId); mAPZEventState->ProcessMouseEvent(*mouseEvent, aGuid, aInputBlockId); } + if (postLayerization && postLayerization->Register()) { + Unused << postLayerization.release(); + } } return status;