Bug 1475413 - Prefer using old items for uninvalidated frames during display list merging. r=miko

MozReview-Commit-ID: 14gc2IBJT1o

--HG--
extra : rebase_source : 6de86aec8ef1039b3adf8006cea456efadbed73b

layout/painting/RetainedDisplayListBuilder.cpp

@@ -235,7 +235,6 @@ void
 OldItemInfo::AddedMatchToMergedList(RetainedDisplayListBuilder* aBuilder,
                                     MergedListIndex aIndex)
 {
-  mItem->Destroy(aBuilder->Builder());
   AddedToMergedList(aIndex);
 }
 
@@ -290,24 +289,30 @@ public:
                             oldItem->Frame() == aNewItem->Frame());
       if (!mOldItems[oldIndex.val].IsChanged()) {
         MOZ_DIAGNOSTIC_ASSERT(!mOldItems[oldIndex.val].IsUsed());
+        nsDisplayItem* destItem = ShouldUseNewItem(aNewItem) ? aNewItem : oldItem;
         if (aNewItem->GetChildren()) {
           Maybe<const ActiveScrolledRoot*> containerASRForChildren;
           if (mBuilder->MergeDisplayLists(aNewItem->GetChildren(),
                                           oldItem->GetChildren(),
-                                          aNewItem->GetChildren(),
+                                          destItem->GetChildren(),
                                           containerASRForChildren,
                                           aNewItem->GetPerFrameKey())) {
-            aNewItem->InvalidateCachedChildInfo();
+            destItem->InvalidateCachedChildInfo();
             mResultIsModified = true;
 
           }
-          UpdateASR(aNewItem, containerASRForChildren);
-          aNewItem->UpdateBounds(mBuilder->Builder());
+          UpdateASR(destItem, containerASRForChildren);
+          destItem->UpdateBounds(mBuilder->Builder());
         }
 
         AutoTArray<MergedListIndex, 2> directPredecessors = ProcessPredecessorsOfOldNode(oldIndex);
-        MergedListIndex newIndex = AddNewNode(aNewItem, Some(oldIndex), directPredecessors, aPreviousItem);
+        MergedListIndex newIndex = AddNewNode(destItem, Some(oldIndex), directPredecessors, aPreviousItem);
         mOldItems[oldIndex.val].AddedMatchToMergedList(mBuilder, newIndex);
+        if (destItem == aNewItem) {
+          oldItem->Destroy(mBuilder->Builder());
+        } else {
+          aNewItem->Destroy(mBuilder->Builder());
+        }
         return newIndex;
       }
     }
@@ -315,6 +320,45 @@ public:
     return AddNewNode(aNewItem, Nothing(), Span<MergedListIndex>(), aPreviousItem);
   }
 
+  bool ShouldUseNewItem(nsDisplayItem* aNewItem)
+  {
+    // Generally we want to use the old item when the frame isn't marked as modified
+    // so that any cached information on the item (or referencing the item) gets
+    // retained. Quite a few FrameLayerBuilder performance improvements benefit
+    // by this.
+    // Sometimes, however, we can end up where the new item paints something different
+    // from the old item, even though we haven't modified the frame, and it's hard to
+    // fix. In these cases we just always use the new item to be safe.
+    DisplayItemType type = aNewItem->GetType();
+    if (type == DisplayItemType::TYPE_CANVAS_BACKGROUND_COLOR ||
+        type == DisplayItemType::TYPE_SOLID_COLOR) {
+      // The canvas background color item can paint the color from another
+      // frame, and even though we schedule a paint, we don't mark the canvas
+      // frame as invalid.
+      return true;
+    } else if (type == DisplayItemType::TYPE_TABLE_BORDER_COLLAPSE) {
+      // We intentionally don't mark the root table frame as modified when a subframe
+      // changes, even though the border collapse item for the root frame is what paints
+      // the changed border. Marking the root frame as modified would rebuild display
+      // items for the whole table area, and we don't want that.
+      return true;
+    } else if (type == DisplayItemType::TYPE_TEXT_OVERFLOW) {
+      // Text overflow marker items are created with the wrapping block as their frame,
+      // and have an index value to note which line they are created for. Their rendering
+      // can change if the items on that line change, which may not mark the block as modified.
+      // We rebuild them if we build any item on the line, so we should always get new items
+      // if they might have changed rendering, and it's easier to just use the new items
+      // rather than computing if we actually need them.
+      return true;
+    } else if (type == DisplayItemType::TYPE_SUBDOCUMENT) {
+      // nsDisplaySubDocument::mShouldFlatten can change without an invalidation
+      // (and is the reason we unconditionally build the subdocument item), so always
+      // use the new one to make sure we get the right value.
+      return true;
+    }
+    return false;
+  }
+
   RetainedDisplayList Finalize() {
     for (size_t i = 0; i < mOldDAG.Length(); i++) {
       if (mOldItems[i].IsUsed()) {