Bug 1468545 - Ensure the drag metrics gets to APZ before the target APZC for inactive scrollframes. r=botond
authorKartikaya Gupta <kgupta@mozilla.com>
Fri, 15 Jun 2018 18:13:47 -0400
changeset 479482 1deb0d199abe9460a4882e855d14e8fb700774a5
parent 479481 d8d2085c29c2c915d0c78465159aca43c7810d51
child 479483 ebbe89ecd4ff6b3322cb402b5cfa55aac88ed80f
push id1757
push userffxbld-merge
push dateFri, 24 Aug 2018 17:02:43 +0000
treeherdermozilla-release@736023aebdb1 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbotond
bugs1468545
milestone62.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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
dom/ipc/TabChild.cpp
gfx/layers/apz/util/APZCCallbackHelper.cpp
gfx/layers/apz/util/APZCCallbackHelper.h
widget/nsBaseWidget.cpp
--- a/dom/ipc/TabChild.cpp
+++ b/dom/ipc/TabChild.cpp
@@ -1713,36 +1713,43 @@ TabChild::HandleRealMouseButtonEvent(con
                                      const uint64_t& aInputBlockId)
 {
   // Mouse events like eMouseEnterIntoWidget, that are created in the parent
   // process EventStateManager code, have an input block id which they get from
   // the InputAPZContext in the parent process stack. However, they did not
   // 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<DisplayportSetListener> postLayerization;
   if (aInputBlockId && aEvent.mFlags.mHandledByAPZ) {
     nsCOMPtr<nsIDocument> 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;
   APZCCallbackHelper::ApplyCallbackTransform(localEvent, aGuid,
       mPuppetWidget->GetDefaultScale());
   DispatchWidgetEventViaAPZ(localEvent);
 
   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
 TabChild::RecvNormalPriorityRealMouseButtonEvent(
   const WidgetMouseEvent& aEvent,
   const ScrollableLayerGuid& aGuid,
   const uint64_t& aInputBlockId)
 {
@@ -1813,18 +1820,22 @@ TabChild::MaybeDispatchCoalescedWheelEve
 void
 TabChild::DispatchWheelEvent(const WidgetWheelEvent& aEvent,
                                   const ScrollableLayerGuid& aGuid,
                                   const uint64_t& aInputBlockId)
 {
   WidgetWheelEvent localEvent(aEvent);
   if (aInputBlockId && aEvent.mFlags.mHandledByAPZ) {
     nsCOMPtr<nsIDocument> document(GetDocument());
-    APZCCallbackHelper::SendSetTargetAPZCNotification(
-      mPuppetWidget, document, aEvent, aGuid, aInputBlockId);
+    UniquePtr<DisplayportSetListener> postLayerization =
+        APZCCallbackHelper::SendSetTargetAPZCNotification(
+            mPuppetWidget, document, aEvent, aGuid, aInputBlockId);
+    if (postLayerization && postLayerization->Register()) {
+      Unused << postLayerization.release();
+    }
   }
 
   localEvent.mWidget = mPuppetWidget;
   APZCCallbackHelper::ApplyCallbackTransform(localEvent, aGuid,
                                              mPuppetWidget->GetDefaultScale());
   DispatchWidgetEventViaAPZ(localEvent);
 
   if (localEvent.mCanTriggerSwipe) {
@@ -1889,19 +1900,22 @@ TabChild::RecvRealTouchEvent(const Widge
 
   if (localEvent.mMessage == eTouchStart && AsyncPanZoomEnabled()) {
     nsCOMPtr<nsIDocument> document = GetDocument();
     if (gfxPrefs::TouchActionEnabled()) {
       APZCCallbackHelper::SendSetAllowedTouchBehaviorNotification(
         mPuppetWidget, document, localEvent, aInputBlockId,
         mSetAllowedTouchBehaviorCallback);
     }
-    APZCCallbackHelper::SendSetTargetAPZCNotification(mPuppetWidget, document,
-                                                      localEvent, aGuid,
-                                                      aInputBlockId);
+    UniquePtr<DisplayportSetListener> postLayerization =
+        APZCCallbackHelper::SendSetTargetAPZCNotification(
+            mPuppetWidget, document, localEvent, aGuid, aInputBlockId);
+    if (postLayerization && postLayerization->Register()) {
+      Unused << postLayerization.release();
+    }
   }
 
   // Dispatch event to content (potentially a long-running operation)
   nsEventStatus status = DispatchWidgetEventViaAPZ(localEvent);
 
   if (!AsyncPanZoomEnabled()) {
     // We shouldn't have any e10s platforms that have touch events enabled
     // without APZ.
--- a/gfx/layers/apz/util/APZCCallbackHelper.cpp
+++ b/gfx/layers/apz/util/APZCCallbackHelper.cpp
@@ -717,97 +717,82 @@ SendLayersDependentApzcTargetConfirmatio
   LayerTransactionChild* shadow = lm->AsShadowForwarder()->GetShadowManager();
   if (!shadow) {
     return;
   }
 
   shadow->SendSetConfirmedTargetAPZC(aInputBlockId, aTargets);
 }
 
-class DisplayportSetListener : public nsAPostRefreshObserver {
-public:
-  DisplayportSetListener(nsIPresShell* aPresShell,
-                         const uint64_t& aInputBlockId,
-                         const nsTArray<ScrollableLayerGuid>& 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;
-    }
+DisplayportSetListener::DisplayportSetListener(nsIWidget* aWidget,
+                                               nsIPresShell* aPresShell,
+                                               const uint64_t& aInputBlockId,
+                                               const nsTArray<ScrollableLayerGuid>& aTargets)
+  : mWidget(aWidget)
+  , mPresShell(aPresShell)
+  , mInputBlockId(aInputBlockId)
+  , mTargets(aTargets)
+{
+}
 
-    delete this;
-  }
-
-private:
-  RefPtr<nsIPresShell> mPresShell;
-  uint64_t mInputBlockId;
-  nsTArray<ScrollableLayerGuid> 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<ScrollableLayerGuid>& aTargets,
-                                    bool aWaitForRefresh)
+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<DisplayportSetListener>
 APZCCallbackHelper::SendSetTargetAPZCNotification(nsIWidget* aWidget,
                                                   nsIDocument* aDocument,
                                                   const WidgetGUIEvent& aEvent,
                                                   const ScrollableLayerGuid& aGuid,
                                                   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
     // input block. If we activated a scroll frame for this input block,
     // sending another target APZC confirmation would be harmful, as it might
     // 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()) {
     if (nsIFrame* rootFrame = shell->GetRootFrame()) {
       rootFrame = UpdateRootFrameForTouchTargetDocument(rootFrame);
 
       bool waitForRefresh = false;
       nsTArray<ScrollableLayerGuid> targets;
@@ -822,28 +807,26 @@ APZCCallbackHelper::SendSetTargetAPZCNot
             rootFrame, wheelEvent->mRefPoint, &targets);
       } else if (const WidgetMouseEvent* mouseEvent = aEvent.AsMouseEvent()) {
         waitForRefresh = PrepareForSetTargetAPZCNotification(aWidget, aGuid,
             rootFrame, mouseEvent->mRefPoint, &targets);
       }
       // 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<DisplayportSetListener>(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
 APZCCallbackHelper::SendSetAllowedTouchBehaviorNotification(
         nsIWidget* aWidget,
         nsIDocument* aDocument,
         const WidgetTouchEvent& aEvent,
         uint64_t aInputBlockId,
--- a/gfx/layers/apz/util/APZCCallbackHelper.h
+++ b/gfx/layers/apz/util/APZCCallbackHelper.h
@@ -6,16 +6,17 @@
 #ifndef mozilla_layers_APZCCallbackHelper_h
 #define mozilla_layers_APZCCallbackHelper_h
 
 #include "FrameMetrics.h"
 #include "InputData.h"
 #include "mozilla/EventForwards.h"
 #include "mozilla/layers/APZUtils.h"
 #include "nsIDOMWindowUtils.h"
+#include "nsRefreshDriver.h"
 
 #include <functional>
 
 class nsIContent;
 class nsIDocument;
 class nsIPresShell;
 class nsIScrollableFrame;
 class nsIWidget;
@@ -23,16 +24,35 @@ template<class T> struct already_AddRefe
 template<class T> class nsCOMPtr;
 
 namespace mozilla {
 namespace layers {
 
 typedef std::function<void(uint64_t, const nsTArray<TouchBehaviorFlags>&)>
         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<ScrollableLayerGuid>& aTargets);
+  virtual ~DisplayportSetListener();
+  bool Register();
+  void DidRefresh() override;
+
+private:
+  RefPtr<nsIWidget> mWidget;
+  RefPtr<nsIPresShell> mPresShell;
+  uint64_t mInputBlockId;
+  nsTArray<ScrollableLayerGuid> 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-
    not-quite-the-same ways, this utility class provides some helpful methods
    to hold code that can be shared across the different platform implementations.
  */
 class APZCCallbackHelper
 {
@@ -131,30 +151,34 @@ public:
                                    Modifiers aModifiers,
                                    int32_t aClickCount,
                                    nsIWidget* aWidget);
 
     /* Perform hit-testing on the touch points of |aEvent| to determine
      * 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<DisplayportSetListener> 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. */
     static void SendSetAllowedTouchBehaviorNotification(nsIWidget* aWidget,
                                                         nsIDocument* aDocument,
                                                         const WidgetTouchEvent& aEvent,
                                                         uint64_t aInputBlockId,
                                                         const SetAllowedTouchBehaviorCallback& aCallback);
--- a/widget/nsBaseWidget.cpp
+++ b/widget/nsBaseWidget.cpp
@@ -1078,44 +1078,46 @@ nsBaseWidget::ProcessUntransformedAPZEve
   // ways that we don't want (i.e. touch points can get stripped out).
   nsEventStatus status;
   UniquePtr<WidgetEvent> original(aEvent->Duplicate());
   DispatchEvent(aEvent, status);
 
   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<DisplayportSetListener> postLayerization;
     if (WidgetTouchEvent* touchEvent = aEvent->AsTouchEvent()) {
       if (touchEvent->mMessage == eTouchStart) {
         if (gfxPrefs::TouchActionEnabled()) {
           APZCCallbackHelper::SendSetAllowedTouchBehaviorNotification(this,
               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());
       }
       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;
 }
 
 class DispatchWheelEventOnMainThread : public Runnable
 {
 public: