Backed out changeset ff731fad7630 (bug 1420737) for causing bug 1431064. a=RyanVM
authorRyan VanderMeulen <ryanvm@gmail.com>
Wed, 17 Jan 2018 16:08:13 -0500
changeset 453995 6fe09f2de63da649e9a8bbff701b445df7d75094
parent 453994 ef04f3ad847540c876c130b7d140a960d922a024
child 453996 3fb310e17608d482f88cc2d0084904c4bdee9079
push id1648
push usermtabara@mozilla.com
push dateThu, 01 Mar 2018 12:45:47 +0000
treeherdermozilla-release@cbb9688c2eeb [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersRyanVM
bugs1420737, 1431064
milestone59.0a1
backs outff731fad76304a7f02cade18828e089f21b39811
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
Backed out changeset ff731fad7630 (bug 1420737) for causing bug 1431064. a=RyanVM
layout/painting/RetainedDisplayListBuilder.cpp
layout/reftests/display-list/reftest.list
layout/reftests/display-list/retained-dl-zindex-1-ref.html
layout/reftests/display-list/retained-dl-zindex-1.html
layout/reftests/display-list/retained-dl-zindex-2-ref.html
layout/reftests/display-list/retained-dl-zindex-2.html
--- a/layout/painting/RetainedDisplayListBuilder.cpp
+++ b/layout/painting/RetainedDisplayListBuilder.cpp
@@ -301,88 +301,53 @@ void UpdateASR(nsDisplayItem* aItem,
                                      aContainerASR.value()));
 }
 
 /**
  * Takes two display lists and merges them into an output list.
  *
  * The basic algorithm is:
  *
- * For-each item i in the new list:
+ * For-each item in the new list:
  *     If the item has a matching item in the old list:
- *         Remove items from the start of the old list up until we reach an item that also exists in the new list (leaving the matched item in place):
+ *         Remove items from the bottom of the old list until we reach the matching item:
  *             Add valid items to the merged list, destroy invalid items.
- *     Add i into the merged list.
- *     If the start of the old list matches i, remove and destroy it, otherwise mark the old version of i as used.
- * Add all remaining valid items from the old list into the merged list, skipping over (and destroying) any that are marked as used.
+ *         Destroy the matching item from the old list.
+ *     Add the item from the new list into the merged list.
+ * Add all remaining valid items from the old list into the merged list.
  *
  * If any item has a child display list, then we recurse into the merge
  * algorithm once we match up the new/old versions (if present).
  *
  * Example 1:
  *
  * Old List: A,B,C,D
- * Modified List: A,D
+ * New List: A,D
  * Invalidations: C,D
  *
  * We first match the A items, and add the new one to the merged list.
  * We then match the D items, copy B into the merged list, but not C
  * (since it's invalid). We then add the new D to the list and we're
  * finished.
  *
  * Merged List: A,B,D
  *
- * Example 2 (layout/reftests/retained-dl-zindex-1.html):
+ * Example 2:
  *
  * Old List: A, B
- * Modified List: B, A
- * Invalidations: A
- *
- * In this example A has been explicitly moved to the back.
- *
- * We match the B items, but don't copy A since it's invalid, and then add the
- * new B into the merged list. We then add A, and we're done.
- *
- * Merged List: B, A
- *
- * Example 3:
- *
- * Old List: A, B
- * Modified List: B, A
+ * New List, B, A
  * Invalidations: -
  *
  * This can happen because a prior merge might have changed the ordering
  * for non-intersecting items.
  *
  * We match the B items, but don't copy A since it's also present in the new list
  * and then add the new B into the merged list. We then add A, and we're done.
  *
  * Merged List: B, A
- *
- * Example 4 (layout/reftests/retained-dl-zindex-2.html):
- *
- * Element A has two elements covering it (B and C), that don't intersect each
- * other. We then move C to the back.
- *
- * The correct initial ordering has B and C after A, in any order.
- *
- * Old List: A, B, C
- * Modified List: C, A
- * Invalidations: C
- *
- * We match the C items, but don't add anything from the old list because A is present
- * in both lists. We add C to the merged list, and mark the old version of C as reused.
- *
- * We then match A, add the new version the merged list and delete the old version.
- *
- * We then process the remainder of the old list, B is added (since it is valid,
- * and hasn't been mark as reused), C is destroyed since it's marked as reused and
- * is already present in the merged list.
- *
- * Merged List: C, A, B
  */
 void
 RetainedDisplayListBuilder::MergeDisplayLists(nsDisplayList* aNewList,
                                               nsDisplayList* aOldList,
                                               nsDisplayList* aOutList,
                                               Maybe<const ActiveScrolledRoot*>& aOutContainerASR)
 {
   nsDisplayList merged;
@@ -437,59 +402,49 @@ RetainedDisplayListBuilder::MergeDisplay
     newListLookup.Put({ i->Frame(), i->GetPerFrameKey() }, i);
   }
 
   while (nsDisplayItem* newItem = aNewList->RemoveBottom()) {
     if (nsDisplayItem* oldItem = oldListLookup.Get({ newItem->Frame(), newItem->GetPerFrameKey() })) {
       // The new item has a matching counterpart in the old list that we haven't yet reached,
       // so copy all valid items from the old list into the merged list until we get to the
       // matched item.
-      nsDisplayItem* old = nullptr;
-      while ((old = aOldList->GetBottom()) && old != oldItem) {
-        if (IsAnyAncestorModified(old->FrameForInvalidation())) {
-          // The old item is invalid, discard it.
-          oldListLookup.Remove({ old->Frame(), old->GetPerFrameKey() });
-          aOldList->RemoveBottom();
-          old->Destroy(&mBuilder);
-        } else if (newListLookup.Get({ old->Frame(), old->GetPerFrameKey() })) {
-          // This old item is also in the new list, but we haven't got to it yet.
-          // Stop now, and we'll deal with it when we get to the new entry.
-          break;
-        } else {
-          // Recurse into the child list (without a matching new list) to
-          // ensure that we find and remove any invalidated items.
-          if (old->GetChildren()) {
-            nsDisplayList empty;
-            Maybe<const ActiveScrolledRoot*> containerASRForChildren;
-            MergeDisplayLists(&empty, old->GetChildren(),
-                              old->GetChildren(), containerASRForChildren);
-            UpdateASR(old, containerASRForChildren);
-            old->UpdateBounds(&mBuilder);
+      if (!oldItem->IsReused()) {
+        nsDisplayItem* old = nullptr;
+        while ((old = aOldList->RemoveBottom()) && !IsSameItem(newItem, old)) {
+          if (IsAnyAncestorModified(old->FrameForInvalidation())) {
+            // The old item is invalid, discard it.
+            oldListLookup.Remove({ old->Frame(), old->GetPerFrameKey() });
+            old->Destroy(&mBuilder);
+          } else if (newListLookup.Get({ old->Frame(), old->GetPerFrameKey() })) {
+            // The old item is also in the new list, but we haven't got to it yet.
+            // Mark that we've found it, and we'll deal with it when we get to the new
+            // entry.
+            old->SetReused(true);
+          } else {
+            // Recurse into the child list (without a matching new list) to
+            // ensure that we find and remove any invalidated items.
+            if (old->GetChildren()) {
+              nsDisplayList empty;
+              Maybe<const ActiveScrolledRoot*> containerASRForChildren;
+              MergeDisplayLists(&empty, old->GetChildren(),
+                                old->GetChildren(), containerASRForChildren);
+              UpdateASR(old, containerASRForChildren);
+              old->UpdateBounds(&mBuilder);
+            }
+            ReuseItem(old);
           }
-          aOldList->RemoveBottom();
-          ReuseItem(old);
         }
-      }
-      bool destroy = false;
-      if (old == oldItem) {
-        // If we advanced the old list until the matching item then we can pop
-        // the matching item off the old list and make sure we clean it up.
-        aOldList->RemoveBottom();
-        destroy = true;
-      } else {
-        // If we didn't get to the matching item, then mark the old item
-        // as being reused (since we're adding the new version to the new
-        // list now) so that we don't add it twice at the end.
-        oldItem->SetReused(true);
+        MOZ_ASSERT(old && IsSameItem(newItem, old));
+        MOZ_ASSERT(old == oldItem);
       }
 
       // Recursively merge any child lists, destroy the old item and add
       // the new one to the list.
-      if (!destroy &&
-          oldItem->GetType() == DisplayItemType::TYPE_LAYER_EVENT_REGIONS &&
+      if (oldItem->GetType() == DisplayItemType::TYPE_LAYER_EVENT_REGIONS &&
           !IsAnyAncestorModified(oldItem->FrameForInvalidation())) {
         // Event regions items don't have anything interesting other than
         // the lists of regions and frames, so we have no need to use the
         // newer item. Always use the old item instead since we assume it's
         // likely to have the bigger lists and merging will be quicker.
         MergeLayerEventRegions(oldItem, newItem);
         ReuseItem(oldItem);
         newItem->Destroy(&mBuilder);
@@ -499,32 +454,29 @@ RetainedDisplayListBuilder::MergeDisplay
           MOZ_ASSERT(newItem->GetChildren());
           Maybe<const ActiveScrolledRoot*> containerASRForChildren;
           MergeDisplayLists(newItem->GetChildren(), oldItem->GetChildren(),
                             newItem->GetChildren(), containerASRForChildren);
           UpdateASR(newItem, containerASRForChildren);
           newItem->UpdateBounds(&mBuilder);
         }
 
-        if (destroy) {
-          oldItem->Destroy(&mBuilder);
-        }
+        oldItem->Destroy(&mBuilder);
         UseItem(newItem);
       }
     } else {
       // If there was no matching item in the old list, then we only need to
       // add the new item to the merged list.
       UseItem(newItem);
     }
   }
 
   // Reuse the remaining valid items from the old display list.
   while (nsDisplayItem* old = aOldList->RemoveBottom()) {
-    if (!IsAnyAncestorModified(old->FrameForInvalidation()) &&
-        !old->IsReused()) {
+    if (!IsAnyAncestorModified(old->FrameForInvalidation())) {
       if (old->GetChildren()) {
         // We are calling MergeDisplayLists() to ensure that the display items
         // with modified or deleted children will be correctly handled.
         // Passing an empty new display list as an argument skips the merging
         // loop above and jumps back here.
         nsDisplayList empty;
         Maybe<const ActiveScrolledRoot*> containerASRForChildren;
 
--- a/layout/reftests/display-list/reftest.list
+++ b/layout/reftests/display-list/reftest.list
@@ -3,14 +3,12 @@ skip-if(!retainedDisplayList) == retaine
 skip-if(!retainedDisplayList) == retained-dl-frame-created-1.html retained-dl-style-change-1-ref.html
 skip-if(!retainedDisplayList) == retained-dl-style-change-stacking-context-1.html retained-dl-style-change-stacking-context-1-ref.html
 skip-if(!retainedDisplayList||!asyncPan) == retained-dl-async-scrolled-1.html retained-dl-async-scrolled-1-ref.html
 skip-if(!retainedDisplayList) == retained-dl-remove-for-ancestor-change-1.html retained-dl-remove-for-ancestor-change-1-ref.html
 skip-if(!retainedDisplayList) == retained-dl-scroll-out-of-view-1.html retained-dl-scroll-out-of-view-1-ref.html
 skip-if(!retainedDisplayList) == retained-dl-displayport-1.html retained-dl-displayport-1-ref.html
 skip-if(!retainedDisplayList) == retained-dl-prerender-transform-1.html retained-dl-prerender-transform-1-ref.html
 == retained-dl-wrap-list.html retained-dl-wrap-list-ref.html
-== retained-dl-zindex-1.html retained-dl-zindex-1-ref.html
-== retained-dl-zindex-2.html retained-dl-zindex-2-ref.html
 fuzzy(1,235200) == 1413073.html 1413073-ref.html
 == 1416291.html 1416291-ref.html
 == 1417601-1.html 1417601-1-ref.html
 == 1418945-1.html 1418945-1-ref.html
deleted file mode 100644
--- a/layout/reftests/display-list/retained-dl-zindex-1-ref.html
+++ /dev/null
@@ -1,27 +0,0 @@
-<!DOCTYPE html>
-<html>
-<head>
-<style>
-div {
-  width: 200px;
-  height: 200px;
-  position:relative;
-  will-change: transform;
-}
-#one {
-  top:120px;
-  background-color:blue;
-}
-#two {
-  top: -200px;
-  left: 100px;
-  background-color:green;
-  z-index: -1;
-}
-</style>
-</head>
-<body>
-  <div id="one"></div>
-  <div id="two"></div>
-</body>
-</html>
deleted file mode 100644
--- a/layout/reftests/display-list/retained-dl-zindex-1.html
+++ /dev/null
@@ -1,35 +0,0 @@
-<!DOCTYPE html>
-<html class="reftest-wait">
-<head>
-<style>
-div {
-  width: 200px;
-  height: 200px;
-  position:relative;
-  will-change: transform;
-}
-#one {
-  top:120px;
-  background-color:blue;
-}
-#two {
-  top: -200px;
-  left: 100px;
-  background-color:green;
-  z-index: 1;
-}
-</style>
-</head>
-<body>
-  <div id="one"></div>
-  <div id="two"></div>
-</body>
-<script>
-function doTest() {
-  document.getElementById("two").style.zIndex = -1;
-  document.documentElement.removeAttribute("class");
-}
-
-window.addEventListener("MozReftestInvalidate", doTest);
-</script>
-</html>
deleted file mode 100644
--- a/layout/reftests/display-list/retained-dl-zindex-2-ref.html
+++ /dev/null
@@ -1,33 +0,0 @@
-<!DOCTYPE html>
-<html>
-<head>
-<style>
-div {
-  width: 200px;
-  height: 200px;
-  position:relative;
-  will-change:transform;
-}
-#one {
-  top:120px;
-  background-color:blue;
-}
-#two {
-  top: -200px;
-  left: 100px;
-  background-color:green;
-}
-#three {
-  top: -180px;
-  left: 100px;
-  background-color:red;
-  z-index: -1;
-}
-</style>
-</head>
-<body>
-  <div id="one"></div>
-  <div id="two"></div>
-  <div id="three"></div>
-</body>
-</html>
deleted file mode 100644
--- a/layout/reftests/display-list/retained-dl-zindex-2.html
+++ /dev/null
@@ -1,41 +0,0 @@
-<!DOCTYPE html>
-<html class="reftest-wait">
-<head>
-<style>
-div {
-  width: 200px;
-  height: 200px;
-  position:relative;
-  will-change:transform;
-}
-#one {
-  top:120px;
-  background-color:blue;
-}
-#two {
-  top: -200px;
-  left: 100px;
-  background-color:green;
-}
-#three {
-  top: -180px;
-  left: 100px;
-  background-color:red;
-  z-index: 1;
-}
-</style>
-</head>
-<body>
-  <div id="one"></div>
-  <div id="two"></div>
-  <div id="three"></div>
-</body>
-<script>
-function doTest() {
-  document.getElementById("three").style.zIndex = -1;
-  document.documentElement.removeAttribute("class");
-}
-
-window.addEventListener("MozReftestInvalidate", doTest);
-</script>
-</html>