Bug 1475413 - Prefer using old items for uninvalidated frames during display list merging. r?miko draft
authorMatt Woodrow <mwoodrow@mozilla.com>
Fri, 13 Jul 2018 12:26:36 +1200
changeset 817585 eec1c707a4801807e8e9c9e13d279f9b9f3fff34
parent 816164 9302fd8c95c05e5a5cd295dde3bbdac2d58d6256
push id116120
push usermwoodrow@mozilla.com
push dateFri, 13 Jul 2018 00:27:10 +0000
reviewersmiko
bugs1475413
milestone63.0a1
Bug 1475413 - Prefer using old items for uninvalidated frames during display list merging. r?miko MozReview-Commit-ID: 14gc2IBJT1o
layout/painting/RetainedDisplayListBuilder.cpp
--- a/layout/painting/RetainedDisplayListBuilder.cpp
+++ b/layout/painting/RetainedDisplayListBuilder.cpp
@@ -230,17 +230,16 @@ UpdateASR(nsDisplayItem* aItem,
     ActiveScrolledRoot::PickAncestor(wrapList->GetFrameActiveScrolledRoot(),
                                      aContainerASR.value()));
 }
 
 void
 OldItemInfo::AddedMatchToMergedList(RetainedDisplayListBuilder* aBuilder,
                                     MergedListIndex aIndex)
 {
-  mItem->Destroy(aBuilder->Builder());
   AddedToMergedList(aIndex);
 }
 
 void
 OldItemInfo::Discard(RetainedDisplayListBuilder* aBuilder,
                      nsTArray<MergedListIndex>&& aDirectPredecessors)
 {
   MOZ_ASSERT(!IsUsed());
@@ -285,41 +284,86 @@ public:
     OldListIndex oldIndex;
     if (!HasModifiedFrame(aNewItem) &&
         HasMatchingItemInOldList(aNewItem, &oldIndex)) {
       nsDisplayItem* oldItem = mOldItems[oldIndex.val].mItem;
       MOZ_DIAGNOSTIC_ASSERT(oldItem->GetPerFrameKey() == aNewItem->GetPerFrameKey() &&
                             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;
       }
     }
     mResultIsModified = true;
     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()) {
         continue;
       }
 
       AutoTArray<MergedListIndex, 2> directPredecessors =
         ResolveNodeIndexesOldToMerged(mOldDAG.GetDirectPredecessors(OldListIndex(i)));