Bug 1359808 - Don't do empty transactions for scroll updates if there are already pending transforms in the layer tree. r?mstange draft
authorKartikaya Gupta <kgupta@mozilla.com>
Wed, 12 Jul 2017 11:14:11 -0400
changeset 607626 83b63171892beb36af74fb4d5182972eecb659c2
parent 607503 09a4282d1172ac255038e7ccacfd772140b219e2
child 637092 4958959be3cba29f9daf971c8247afd07957cfd6
push id68050
push userkgupta@mozilla.com
push dateWed, 12 Jul 2017 15:15:05 +0000
reviewersmstange
bugs1359808
milestone56.0a1
Bug 1359808 - Don't do empty transactions for scroll updates if there are already pending transforms in the layer tree. r?mstange The pending transforms must have been computed using the older scroll offset values, which means that updating the scroll offsets without recomputing the transforms will make them wrong. If we do an empty transaction for the scroll offset updates, the transforms will not get computed. This patch catches this scenario and schedules a full paint instead of the empty transaction instead. The case where the scroll offset is modified *before* the transform is already handled by code in nsIFrame::TryUpdateTransformOnly. MozReview-Commit-ID: I5s5J7BS1ru
gfx/layers/Layers.cpp
gfx/layers/Layers.h
layout/generic/nsGfxScrollFrame.cpp
--- a/gfx/layers/Layers.cpp
+++ b/gfx/layers/Layers.cpp
@@ -2486,21 +2486,30 @@ LayerManager::ClearDisplayItemLayers()
 }
 
 /*static*/ bool
 LayerManager::IsLogEnabled()
 {
   return MOZ_LOG_TEST(GetLog(), LogLevel::Debug);
 }
 
-void
+bool
 LayerManager::SetPendingScrollUpdateForNextTransaction(FrameMetrics::ViewID aScrollId,
                                                        const ScrollUpdateInfo& aUpdateInfo)
 {
+  Layer* withPendingTransform = DepthFirstSearch<ForwardIterator>(GetRoot(),
+      [](Layer* aLayer) {
+        return aLayer->HasPendingTransform();
+      });
+  if (withPendingTransform) {
+    return false;
+  }
+
   mPendingScrollUpdates[aScrollId] = aUpdateInfo;
+  return true;
 }
 
 Maybe<ScrollUpdateInfo>
 LayerManager::GetPendingScrollInfoUpdate(FrameMetrics::ViewID aScrollId)
 {
   auto it = mPendingScrollUpdates.find(aScrollId);
   if (it != mPendingScrollUpdates.end()) {
     return Some(it->second);
--- a/gfx/layers/Layers.h
+++ b/gfx/layers/Layers.h
@@ -797,17 +797,17 @@ private:
   FramesTimingRecording mRecording;
 
 public:
   /*
    * Methods to store/get/clear a "pending scroll info update" object on a
    * per-scrollid basis. This is used for empty transactions that push over
    * scroll position updates to the APZ code.
    */
-  void SetPendingScrollUpdateForNextTransaction(FrameMetrics::ViewID aScrollId,
+  bool SetPendingScrollUpdateForNextTransaction(FrameMetrics::ViewID aScrollId,
                                                 const ScrollUpdateInfo& aUpdateInfo);
   Maybe<ScrollUpdateInfo> GetPendingScrollInfoUpdate(FrameMetrics::ViewID aScrollId);
   void ClearPendingScrollInfoUpdate();
 private:
   std::map<FrameMetrics::ViewID,ScrollUpdateInfo> mPendingScrollUpdates;
 
   // Display items are only valid during this transaction.
   // At the end of the transaction, we have to go and clear out
@@ -1397,16 +1397,18 @@ public:
   int32_t GetFixedPositionSides() { return mSimpleAttrs.FixedPositionSides(); }
   FrameMetrics::ViewID GetStickyScrollContainerId() { return mSimpleAttrs.StickyScrollContainerId(); }
   const LayerRect& GetStickyScrollRangeOuter() { return mSimpleAttrs.StickyScrollRangeOuter(); }
   const LayerRect& GetStickyScrollRangeInner() { return mSimpleAttrs.StickyScrollRangeInner(); }
   FrameMetrics::ViewID GetScrollbarTargetContainerId() { return mSimpleAttrs.ScrollbarTargetContainerId(); }
   const ScrollThumbData& GetScrollThumbData() const { return mSimpleAttrs.ThumbData(); }
   bool IsScrollbarContainer() { return mSimpleAttrs.IsScrollbarContainer(); }
   Layer* GetMaskLayer() const { return mMaskLayer; }
+  bool HasPendingTransform() const { return mPendingTransform; }
+
   void CheckCanary() const { mCanary.Check(); }
 
   // Ancestor mask layers are associated with FrameMetrics, but for simplicity
   // in maintaining the layer tree structure we attach them to the layer.
   size_t GetAncestorMaskLayerCount() const {
     return mAncestorMaskLayers.Length();
   }
   Layer* GetAncestorMaskLayerAt(size_t aIndex) const {
--- a/layout/generic/nsGfxScrollFrame.cpp
+++ b/layout/generic/nsGfxScrollFrame.cpp
@@ -2868,27 +2868,31 @@ ScrollFrameHelper::ScrollToImpl(nsPoint 
         if (LastScrollOrigin() == nsGkAtoms::apz) {
           schedulePaint = false;
           PAINT_SKIP_LOG("Skipping due to APZ scroll\n");
         } else if (mScrollableByAPZ) {
           nsIWidget* widget = presContext->GetNearestWidget();
           LayerManager* manager = widget ? widget->GetLayerManager() : nullptr;
           if (manager) {
             mozilla::layers::FrameMetrics::ViewID id;
-            DebugOnly<bool> success = nsLayoutUtils::FindIDFor(content, &id);
+            bool success = nsLayoutUtils::FindIDFor(content, &id);
             MOZ_ASSERT(success); // we have a displayport, we better have an ID
 
             // Schedule an empty transaction to carry over the scroll offset update,
             // instead of a full transaction. This empty transaction might still get
             // squashed into a full transaction if something happens to trigger one.
-            schedulePaint = false;
-            manager->SetPendingScrollUpdateForNextTransaction(id,
+            success = manager->SetPendingScrollUpdateForNextTransaction(id,
                 { mScrollGeneration, CSSPoint::FromAppUnits(GetScrollPosition()) });
-            mOuter->SchedulePaint(nsIFrame::PAINT_COMPOSITE_ONLY);
-            PAINT_SKIP_LOG("Skipping due to APZ-forwarded main-thread scroll\n");
+            if (success) {
+              schedulePaint = false;
+              mOuter->SchedulePaint(nsIFrame::PAINT_COMPOSITE_ONLY);
+              PAINT_SKIP_LOG("Skipping due to APZ-forwarded main-thread scroll\n");
+            } else {
+              PAINT_SKIP_LOG("Failed to set pending scroll update on layer manager\n");
+            }
           }
         }
       }
     }
   }
 
   if (schedulePaint) {
     mOuter->SchedulePaint();