From 746fe9865af03036c679c5f61d9e7c5b4f08bfbc Mon Sep 17 00:00:00 2001
From: Miko Mynttinen <mikokm@gmail.com>
Date: Fri, 16 Jul 2021 10:55:03 +0000
Subject: [PATCH] Bug 1720711 - Remove most nsDisplayItem::GetAbove() calls
 r=mattwoodrow

Replaces direct linked list traversals with iterators.

Differential Revision: https://phabricator.services.mozilla.com/D119985
---
 gfx/layers/wr/WebRenderCommandBuilder.cpp     | 56 +++++++++---------
 layout/base/PresShell.cpp                     |  4 +-
 layout/base/nsLayoutDebugger.cpp              |  2 +-
 layout/base/nsLayoutUtils.cpp                 |  2 +-
 layout/generic/nsGfxScrollFrame.cpp           |  8 +--
 layout/generic/nsIFrame.cpp                   |  2 +-
 .../painting/RetainedDisplayListBuilder.cpp   |  5 +-
 layout/painting/nsDisplayList.cpp             | 30 ++++++----
 layout/painting/nsDisplayList.h               | 57 +++++++++++++++----
 9 files changed, 103 insertions(+), 63 deletions(-)

diff --git a/gfx/layers/wr/WebRenderCommandBuilder.cpp b/gfx/layers/wr/WebRenderCommandBuilder.cpp
index 08cf08993f75..666cf2c7cf44 100644
--- a/gfx/layers/wr/WebRenderCommandBuilder.cpp
+++ b/gfx/layers/wr/WebRenderCommandBuilder.cpp
@@ -646,8 +646,8 @@ struct DIGroup {
     // Reset mHitInfo, it will get updated inside PaintItemRange
     mHitInfo = CompositorHitTestInvisibleToHit;
 
-    PaintItemRange(aGrouper, aStartItem, aEndItem, context, recorder,
-                   rootManager, aResources);
+    PaintItemRange(aGrouper, nsDisplayList::Range(aStartItem, aEndItem),
+                   context, recorder, rootManager, aResources);
 
     // XXX: set this correctly perhaps using
     // aItem->GetOpaqueRegion(aDisplayListBuilder, &snapped).
@@ -743,14 +743,16 @@ struct DIGroup {
                        wr::AsImageKey(*mKey));
   }
 
-  void PaintItemRange(Grouper* aGrouper, nsDisplayItem* aStartItem,
-                      nsDisplayItem* aEndItem, gfxContext* aContext,
+  void PaintItemRange(Grouper* aGrouper, nsDisplayList::Iterator aIter,
+                      gfxContext* aContext,
                       WebRenderDrawEventRecorder* aRecorder,
                       RenderRootStateManager* aRootManager,
                       wr::IpcResourceUpdateQueue& aResources) {
     LayerIntSize size = mVisibleRect.Size();
-    for (nsDisplayItem* item = aStartItem; item != aEndItem;
-         item = item->GetAbove()) {
+    while (aIter.HasNext()) {
+      nsDisplayItem* item = aIter.GetNext();
+      MOZ_ASSERT(item);
+
       BlobItemData* data = GetBlobItemData(item);
       IntRect bounds = data->mRect;
       auto bottomRight = bounds.BottomRight();
@@ -915,8 +917,8 @@ void Grouper::PaintContainerItem(DIGroup* aGroup, nsDisplayItem* aItem,
         aContext->GetDrawTarget()->FlushItem(aItemBounds);
       } else {
         aContext->Multiply(ThebesMatrix(trans2d));
-        aGroup->PaintItemRange(this, aChildren->GetBottom(), nullptr, aContext,
-                               aRecorder, aRootManager, aResources);
+        aGroup->PaintItemRange(this, nsDisplayList::Iterator(aChildren),
+                               aContext, aRecorder, aRootManager, aResources);
       }
 
       if (currentClip.HasClip()) {
@@ -938,7 +940,7 @@ void Grouper::PaintContainerItem(DIGroup* aGroup, nsDisplayItem* aItem,
       GP("beginGroup %s %p-%d\n", aItem->Name(), aItem->Frame(),
          aItem->GetPerFrameKey());
       aContext->GetDrawTarget()->FlushItem(aItemBounds);
-      aGroup->PaintItemRange(this, aChildren->GetBottom(), nullptr, aContext,
+      aGroup->PaintItemRange(this, nsDisplayList::Iterator(aChildren), aContext,
                              aRecorder, aRootManager, aResources);
       aContext->GetDrawTarget()->PopLayer();
       GP("endGroup %s %p-%d\n", aItem->Name(), aItem->Frame(),
@@ -955,7 +957,7 @@ void Grouper::PaintContainerItem(DIGroup* aGroup, nsDisplayItem* aItem,
       GP("beginGroup %s %p-%d\n", aItem->Name(), aItem->Frame(),
          aItem->GetPerFrameKey());
       aContext->GetDrawTarget()->FlushItem(aItemBounds);
-      aGroup->PaintItemRange(this, aChildren->GetBottom(), nullptr, aContext,
+      aGroup->PaintItemRange(this, nsDisplayList::Iterator(aChildren), aContext,
                              aRecorder, aRootManager, aResources);
       aContext->GetDrawTarget()->PopLayer();
       GP("endGroup %s %p-%d\n", aItem->Name(), aItem->Frame(),
@@ -969,7 +971,7 @@ void Grouper::PaintContainerItem(DIGroup* aGroup, nsDisplayItem* aItem,
       GP("beginGroup %s %p-%d\n", aItem->Name(), aItem->Frame(),
          aItem->GetPerFrameKey());
       aContext->GetDrawTarget()->FlushItem(aItemBounds);
-      aGroup->PaintItemRange(this, aChildren->GetBottom(), nullptr, aContext,
+      aGroup->PaintItemRange(this, nsDisplayList::Iterator(aChildren), aContext,
                              aRecorder, aRootManager, aResources);
       aContext->GetDrawTarget()->PopLayer();
       GP("endGroup %s %p-%d\n", aItem->Name(), aItem->Frame(),
@@ -987,7 +989,7 @@ void Grouper::PaintContainerItem(DIGroup* aGroup, nsDisplayItem* aItem,
               GP("beginGroup %s %p-%d\n", aItem->Name(), aItem->Frame(),
                  aItem->GetPerFrameKey());
               aContext->GetDrawTarget()->FlushItem(aItemBounds);
-              aGroup->PaintItemRange(this, aChildren->GetBottom(), nullptr,
+              aGroup->PaintItemRange(this, nsDisplayList::Iterator(aChildren),
                                      aContext, aRecorder, aRootManager,
                                      aResources);
               GP("endGroup %s %p-%d\n", aItem->Name(), aItem->Frame(),
@@ -1020,7 +1022,7 @@ void Grouper::PaintContainerItem(DIGroup* aGroup, nsDisplayItem* aItem,
     }
 
     default:
-      aGroup->PaintItemRange(this, aChildren->GetBottom(), nullptr, aContext,
+      aGroup->PaintItemRange(this, nsDisplayList::Iterator(aChildren), aContext,
                              aRecorder, aRootManager, aResources);
       break;
   }
@@ -1052,8 +1054,8 @@ static bool HasActiveChildren(const nsDisplayList& aList,
                               const mozilla::layers::StackingContextHelper& aSc,
                               mozilla::layers::RenderRootStateManager* aManager,
                               nsDisplayListBuilder* aDisplayListBuilder) {
-  for (nsDisplayItem* i = aList.GetBottom(); i; i = i->GetAbove()) {
-    if (IsItemProbablyActive(i, aBuilder, aResources, aSc, aManager,
+  for (nsDisplayItem* item : aList) {
+    if (IsItemProbablyActive(item, aBuilder, aResources, aSc, aManager,
                              aDisplayListBuilder, false)) {
       return true;
     }
@@ -1143,15 +1145,20 @@ void Grouper::ConstructGroups(nsDisplayListBuilder* aDisplayListBuilder,
                               wr::IpcResourceUpdateQueue& aResources,
                               DIGroup* aGroup, nsDisplayList* aList,
                               const StackingContextHelper& aSc) {
-  DIGroup* currentGroup = aGroup;
-
-  nsDisplayItem* item = aList->GetBottom();
-  nsDisplayItem* startOfCurrentGroup = item;
   RenderRootStateManager* manager =
       aCommandBuilder->mManager->GetRenderRootStateManager();
+
+  nsDisplayItem* startOfCurrentGroup = nullptr;
+  DIGroup* currentGroup = aGroup;
+
   // We need to track whether we have active siblings for mixed blend mode.
   bool encounteredActiveItem = false;
-  while (item) {
+
+  for (nsDisplayItem* item : *aList) {
+    if (!startOfCurrentGroup) {
+      startOfCurrentGroup = item;
+    }
+
     if (IsItemProbablyActive(item, aBuilder, aResources, aSc, manager,
                              mDisplayListBuilder, encounteredActiveItem)) {
       encounteredActiveItem = true;
@@ -1225,15 +1232,12 @@ void Grouper::ConstructGroups(nsDisplayListBuilder* aDisplayListBuilder,
         sIndent--;
       }
 
+      startOfCurrentGroup = nullptr;
       currentGroup = &groupData->mFollowingGroup;
-
-      startOfCurrentGroup = item->GetAbove();
     } else {  // inactive item
       ConstructItemInsideInactive(aCommandBuilder, aBuilder, aResources,
                                   currentGroup, item, aSc);
     }
-
-    item = item->GetAbove();
   }
 
   currentGroup->EndGroup(aCommandBuilder->mManager, aDisplayListBuilder,
@@ -1247,12 +1251,10 @@ bool Grouper::ConstructGroupInsideInactive(
     WebRenderCommandBuilder* aCommandBuilder, wr::DisplayListBuilder& aBuilder,
     wr::IpcResourceUpdateQueue& aResources, DIGroup* aGroup,
     nsDisplayList* aList, const StackingContextHelper& aSc) {
-  nsDisplayItem* item = aList->GetBottom();
   bool invalidated = false;
-  while (item) {
+  for (nsDisplayItem* item : *aList) {
     invalidated |= ConstructItemInsideInactive(aCommandBuilder, aBuilder,
                                                aResources, aGroup, item, aSc);
-    item = item->GetAbove();
   }
   return invalidated;
 }
diff --git a/layout/base/PresShell.cpp b/layout/base/PresShell.cpp
index f2fdff0c5b69..241066d2cffe 100644
--- a/layout/base/PresShell.cpp
+++ b/layout/base/PresShell.cpp
@@ -5160,7 +5160,7 @@ void PresShell::AddPrintPreviewBackgroundItem(nsDisplayListBuilder* aBuilder,
 static bool AddCanvasBackgroundColor(const nsDisplayList* aList,
                                      nsIFrame* aCanvasFrame, nscolor aColor,
                                      bool aCSSBackgroundColor) {
-  for (nsDisplayItem* i = aList->GetBottom(); i; i = i->GetAbove()) {
+  for (nsDisplayItem* i : *aList) {
     const DisplayItemType type = i->GetType();
 
     if (i->Frame() == aCanvasFrame &&
@@ -5803,7 +5803,7 @@ void PresShell::ProcessSynthMouseMoveEvent(bool aFromScroll) {
 /* static */
 void PresShell::MarkFramesInListApproximatelyVisible(
     const nsDisplayList& aList) {
-  for (nsDisplayItem* item = aList.GetBottom(); item; item = item->GetAbove()) {
+  for (nsDisplayItem* item : aList) {
     nsDisplayList* sublist = item->GetChildren();
     if (sublist) {
       MarkFramesInListApproximatelyVisible(*sublist);
diff --git a/layout/base/nsLayoutDebugger.cpp b/layout/base/nsLayoutDebugger.cpp
index e78ca5cceb08..8aea6a662b81 100644
--- a/layout/base/nsLayoutDebugger.cpp
+++ b/layout/base/nsLayoutDebugger.cpp
@@ -179,7 +179,7 @@ static void PrintDisplayListTo(nsDisplayListBuilder* aBuilder,
     aStream << "<ul>";
   }
 
-  for (nsDisplayItem* i = aList.GetBottom(); i != nullptr; i = i->GetAbove()) {
+  for (nsDisplayItem* i : aList) {
     if (aDumpHtml) {
       aStream << "<li>";
     }
diff --git a/layout/base/nsLayoutUtils.cpp b/layout/base/nsLayoutUtils.cpp
index cae4a2b9bd6e..6a95f44cd912 100644
--- a/layout/base/nsLayoutUtils.cpp
+++ b/layout/base/nsLayoutUtils.cpp
@@ -2953,7 +2953,7 @@ static RetainedDisplayListBuilder* GetOrCreateRetainedDisplayListBuilder(
 void PrintHitTestInfoStatsInternal(nsDisplayList* aList, int& aTotal,
                                    int& aHitTest, int& aVisible,
                                    int& aSpecial) {
-  for (nsDisplayItem* i = aList->GetBottom(); i; i = i->GetAbove()) {
+  for (nsDisplayItem* i : *aList) {
     aTotal++;
 
     if (i->GetChildren()) {
diff --git a/layout/generic/nsGfxScrollFrame.cpp b/layout/generic/nsGfxScrollFrame.cpp
index 8ef7e7e450a4..8b4bc1bf8df5 100644
--- a/layout/generic/nsGfxScrollFrame.cpp
+++ b/layout/generic/nsGfxScrollFrame.cpp
@@ -3204,8 +3204,7 @@ void ScrollFrameHelper::ScrollToImpl(nsPoint aPt, const nsRect& aRange,
 static Maybe<int32_t> MaxZIndexInList(nsDisplayList* aList,
                                       nsDisplayListBuilder* aBuilder) {
   Maybe<int32_t> maxZIndex = Nothing();
-  for (nsDisplayItem* item = aList->GetBottom(); item;
-       item = item->GetAbove()) {
+  for (nsDisplayItem* item : *aList) {
     int32_t zIndex = item->ZIndex();
     if (zIndex < 0) {
       continue;
@@ -3480,7 +3479,7 @@ static void ClipItemsExceptCaret(
     const DisplayItemClipChain* aExtraClip,
     nsTHashMap<nsPtrHashKey<const DisplayItemClipChain>,
                const DisplayItemClipChain*>& aCache) {
-  for (nsDisplayItem* i = aList->GetBottom(); i; i = i->GetAbove()) {
+  for (nsDisplayItem* i : *aList) {
     if (!ShouldBeClippedByFrame(aClipFrame, i->Frame())) {
       continue;
     }
@@ -3575,8 +3574,7 @@ class MOZ_RAII AutoContainsBlendModeCapturer {
 static int32_t MaxZIndexInListOfItemsContainedInFrame(nsDisplayList* aList,
                                                       nsIFrame* aFrame) {
   int32_t maxZIndex = -1;
-  for (nsDisplayItem* item = aList->GetBottom(); item;
-       item = item->GetAbove()) {
+  for (nsDisplayItem* item : *aList) {
     nsIFrame* itemFrame = item->Frame();
     // Perspective items return the scroll frame as their Frame(), so consider
     // their TransformFrame() instead.
diff --git a/layout/generic/nsIFrame.cpp b/layout/generic/nsIFrame.cpp
index 4d38dcb069a6..7f72f5521d1c 100644
--- a/layout/generic/nsIFrame.cpp
+++ b/layout/generic/nsIFrame.cpp
@@ -3802,7 +3802,7 @@ static nsDisplayItem* WrapInWrapList(nsDisplayListBuilder* aBuilder,
   // on which items we build, so we need to ensure that we don't transition
   // to/from a wrap list without invalidating correctly.
   bool needsWrapList =
-      item->GetAbove() || item->Frame() != aFrame || item->GetChildren();
+      aList->Count() > 1 || item->Frame() != aFrame || item->GetChildren();
 
   // If we have an explicit container item (that can't change without an
   // invalidation) or we're doing a full build and don't need a wrap list, then
diff --git a/layout/painting/RetainedDisplayListBuilder.cpp b/layout/painting/RetainedDisplayListBuilder.cpp
index 520f31585655..bc9c87370c72 100644
--- a/layout/painting/RetainedDisplayListBuilder.cpp
+++ b/layout/painting/RetainedDisplayListBuilder.cpp
@@ -74,7 +74,7 @@ RetainedDisplayListData* GetOrSetRetainedDisplayListData(nsIFrame* aRootFrame) {
 }
 
 static void MarkFramesWithItemsAndImagesModified(nsDisplayList* aList) {
-  for (nsDisplayItem* i = aList->GetBottom(); i != nullptr; i = i->GetAbove()) {
+  for (nsDisplayItem* i : *aList) {
     if (!i->HasDeletedFrame() && i->CanBeReused() &&
         !i->Frame()->IsFrameModified()) {
       // If we have existing cached geometry for this item, then check that for
@@ -758,8 +758,7 @@ class MergeState {
 
 #ifdef DEBUG
 void VerifyNotModified(nsDisplayList* aList) {
-  for (nsDisplayItem* item = aList->GetBottom(); item;
-       item = item->GetAbove()) {
+  for (nsDisplayItem* item : *aList) {
     MOZ_ASSERT(!AnyContentAncestorModified(item->FrameForInvalidation()));
 
     if (item->GetChildren()) {
diff --git a/layout/painting/nsDisplayList.cpp b/layout/painting/nsDisplayList.cpp
index 5abe65e4f874..330592cdfbf0 100644
--- a/layout/painting/nsDisplayList.cpp
+++ b/layout/painting/nsDisplayList.cpp
@@ -3970,13 +3970,19 @@ nsDisplayBackgroundImage::ShouldCreateOwnLayer(nsDisplayListBuilder* aBuilder,
 }
 
 static void CheckForBorderItem(nsDisplayItem* aItem, uint32_t& aFlags) {
-  nsDisplayItem* nextItem = aItem->GetAbove();
-  while (nextItem && nextItem->GetType() == DisplayItemType::TYPE_BACKGROUND) {
-    nextItem = nextItem->GetAbove();
-  }
-  if (nextItem && nextItem->Frame() == aItem->Frame() &&
-      nextItem->GetType() == DisplayItemType::TYPE_BORDER) {
-    aFlags |= nsCSSRendering::PAINTBG_WILL_PAINT_BORDER;
+  // TODO(miko): Iterating over the display list like this is suspicious.
+  for (nsDisplayList::Iterator it(aItem); it.HasNext(); ++it) {
+    nsDisplayItem* next = *it;
+
+    if (next->GetType() == DisplayItemType::TYPE_BACKGROUND) {
+      continue;
+    }
+
+    if (next->GetType() == DisplayItemType::TYPE_BORDER) {
+      aFlags |= nsCSSRendering::PAINTBG_WILL_PAINT_BORDER;
+    }
+
+    break;
   }
 }
 
@@ -5530,11 +5536,11 @@ nsDisplayWrapList::nsDisplayWrapList(
     return;
   }
 
-  nsDisplayItem* i = mListPtr->GetBottom();
-  if (i &&
-      (!i->GetAbove() || i->GetType() == DisplayItemType::TYPE_TRANSFORM) &&
-      i->Frame() == mFrame) {
-    MOZ_ASSERT(mReferenceFrame == i->ReferenceFrame());
+  nsDisplayItem* item = mListPtr->GetBottom();
+  if (item && item->Frame() == mFrame &&
+      (mListPtr->Count() == 1 ||
+       item->GetType() == DisplayItemType::TYPE_TRANSFORM)) {
+    MOZ_ASSERT(mReferenceFrame == item->ReferenceFrame());
   }
 #endif
 }
diff --git a/layout/painting/nsDisplayList.h b/layout/painting/nsDisplayList.h
index cb5f36f75364..97e58582c2ec 100644
--- a/layout/painting/nsDisplayList.h
+++ b/layout/painting/nsDisplayList.h
@@ -3319,40 +3319,75 @@ class nsDisplayList {
   typedef mozilla::layers::LayerManager LayerManager;
   typedef mozilla::layers::PaintedLayer PaintedLayer;
 
-  template <typename T>
   class Iterator {
    public:
-    Iterator() : mItem(nullptr) {}
+    constexpr Iterator() : mCurrent(nullptr), mEnd(nullptr) {}
     ~Iterator() = default;
     Iterator(const Iterator& aOther) = default;
     Iterator& operator=(const Iterator& aOther) = default;
 
-    explicit Iterator(const nsDisplayList* aList) : mItem(aList->GetBottom()) {}
-    explicit Iterator(const nsDisplayItem* aItem) : mItem(aItem) {}
+    explicit Iterator(const nsDisplayList* aList)
+        : mCurrent(aList->GetBottom()), mEnd(nullptr) {}
+    explicit Iterator(nsDisplayItem* aStart)
+        : mCurrent(aStart), mEnd(nullptr) {}
 
     Iterator& operator++() {
-      mItem = mItem ? mItem->GetAbove() : mItem;
+      mCurrent = Next();
       return *this;
     }
 
+    nsDisplayItem* operator*() {
+      MOZ_ASSERT(mCurrent);
+      return mCurrent;
+    }
+
     bool operator==(const Iterator& aOther) const {
-      return mItem == aOther.mItem;
+      return mCurrent == aOther.mCurrent;
     }
 
     bool operator!=(const Iterator& aOther) const {
       return !operator==(aOther);
     }
 
-    T* operator*() { return mItem; }
+    bool HasNext() const { return mCurrent != nullptr; }
+
+    nsDisplayItem* GetNext() {
+      MOZ_ASSERT(HasNext());
+      auto* next = mCurrent;
+      operator++();
+      return next;
+    }
+
+   protected:
+    Iterator(nsDisplayItem* aStart, nsDisplayItem* aEnd)
+        : mCurrent(aStart), mEnd(aEnd) {}
+
+    nsDisplayItem* Next() const {
+      if (!mCurrent) {
+        return nullptr;
+      }
+
+      auto* next = mCurrent->GetAbove();
+      if (next == mEnd) {
+        return nullptr;
+      }
+
+      return next;
+    }
 
    private:
-    T* mItem;
+    nsDisplayItem* mCurrent;
+    nsDisplayItem* mEnd;
   };
 
-  using DisplayItemIterator = Iterator<nsDisplayItem>;
+  class Range final : public Iterator {
+   public:
+    Range(nsDisplayItem* aStart, nsDisplayItem* aEnd)
+        : Iterator(aStart, aEnd) {}
+  };
 
-  DisplayItemIterator begin() const { return DisplayItemIterator(this); }
-  DisplayItemIterator end() const { return DisplayItemIterator(); }
+  Iterator begin() const { return Iterator(this); }
+  constexpr Iterator end() const { return Iterator(); }
 
   /**
    * Create an empty list.