From 17d737b4586042f69bbce6e829058d95d1c82603 Mon Sep 17 00:00:00 2001 From: Matt Woodrow Date: Wed, 17 Jan 2018 12:07:42 +1300 Subject: [PATCH] Bug 1420737 - Fix merge algorithm to handle more complex z-index changes. r=mstange --- .../painting/RetainedDisplayListBuilder.cpp | 122 ++++++++++++------ layout/reftests/display-list/reftest.list | 2 + .../retained-dl-zindex-1-ref.html | 27 ++++ .../display-list/retained-dl-zindex-1.html | 35 +++++ .../retained-dl-zindex-2-ref.html | 33 +++++ .../display-list/retained-dl-zindex-2.html | 41 ++++++ 6 files changed, 223 insertions(+), 37 deletions(-) create mode 100644 layout/reftests/display-list/retained-dl-zindex-1-ref.html create mode 100644 layout/reftests/display-list/retained-dl-zindex-1.html create mode 100644 layout/reftests/display-list/retained-dl-zindex-2-ref.html create mode 100644 layout/reftests/display-list/retained-dl-zindex-2.html diff --git a/layout/painting/RetainedDisplayListBuilder.cpp b/layout/painting/RetainedDisplayListBuilder.cpp index e1d04abb6041..44ee0197002c 100644 --- a/layout/painting/RetainedDisplayListBuilder.cpp +++ b/layout/painting/RetainedDisplayListBuilder.cpp @@ -306,13 +306,13 @@ void UpdateASR(nsDisplayItem* aItem, * * The basic algorithm is: * - * For-each item in the new list: + * For-each item i in the new list: * If the item has a matching item in the old list: - * Remove items from the bottom of the old list until we reach the matching item: + * 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): * Add valid items to the merged list, destroy invalid items. - * 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. + * 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. * * 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). @@ -320,7 +320,7 @@ void UpdateASR(nsDisplayItem* aItem, * Example 1: * * Old List: A,B,C,D - * New List: A,D + * Modified List: A,D * Invalidations: C,D * * We first match the A items, and add the new one to the merged list. @@ -330,10 +330,23 @@ void UpdateASR(nsDisplayItem* aItem, * * Merged List: A,B,D * - * Example 2: + * Example 2 (layout/reftests/retained-dl-zindex-1.html): * * Old List: A, B - * New List, B, A + * 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 * Invalidations: - * * This can happen because a prior merge might have changed the ordering @@ -343,6 +356,28 @@ void UpdateASR(nsDisplayItem* aItem, * 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, @@ -407,39 +442,49 @@ RetainedDisplayListBuilder::MergeDisplayLists(nsDisplayList* aNewList, // 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. - 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 containerASRForChildren; - MergeDisplayLists(&empty, old->GetChildren(), - old->GetChildren(), containerASRForChildren); - UpdateASR(old, containerASRForChildren); - old->UpdateBounds(&mBuilder); - } - ReuseItem(old); + 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 containerASRForChildren; + MergeDisplayLists(&empty, old->GetChildren(), + old->GetChildren(), containerASRForChildren); + UpdateASR(old, containerASRForChildren); + old->UpdateBounds(&mBuilder); } + aOldList->RemoveBottom(); + ReuseItem(old); } - MOZ_ASSERT(old && IsSameItem(newItem, old)); - MOZ_ASSERT(old == oldItem); + } + 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); } // Recursively merge any child lists, destroy the old item and add // the new one to the list. - if (oldItem->GetType() == DisplayItemType::TYPE_LAYER_EVENT_REGIONS && + if (destroy && + 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 @@ -459,7 +504,9 @@ RetainedDisplayListBuilder::MergeDisplayLists(nsDisplayList* aNewList, newItem->UpdateBounds(&mBuilder); } - oldItem->Destroy(&mBuilder); + if (destroy) { + oldItem->Destroy(&mBuilder); + } UseItem(newItem); } } else { @@ -471,7 +518,8 @@ RetainedDisplayListBuilder::MergeDisplayLists(nsDisplayList* aNewList, // Reuse the remaining valid items from the old display list. while (nsDisplayItem* old = aOldList->RemoveBottom()) { - if (!IsAnyAncestorModified(old->FrameForInvalidation())) { + if (!IsAnyAncestorModified(old->FrameForInvalidation()) && + !old->IsReused()) { if (old->GetChildren()) { // We are calling MergeDisplayLists() to ensure that the display items // with modified or deleted children will be correctly handled. diff --git a/layout/reftests/display-list/reftest.list b/layout/reftests/display-list/reftest.list index 63b40e7e2c4e..fed01b2caa71 100644 --- a/layout/reftests/display-list/reftest.list +++ b/layout/reftests/display-list/reftest.list @@ -8,6 +8,8 @@ skip-if(!retainedDisplayList) == retained-dl-scroll-out-of-view-1.html retained- 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 diff --git a/layout/reftests/display-list/retained-dl-zindex-1-ref.html b/layout/reftests/display-list/retained-dl-zindex-1-ref.html new file mode 100644 index 000000000000..d9cd43b5e99b --- /dev/null +++ b/layout/reftests/display-list/retained-dl-zindex-1-ref.html @@ -0,0 +1,27 @@ + + + + + + +
+
+ + diff --git a/layout/reftests/display-list/retained-dl-zindex-1.html b/layout/reftests/display-list/retained-dl-zindex-1.html new file mode 100644 index 000000000000..dd640539c9f5 --- /dev/null +++ b/layout/reftests/display-list/retained-dl-zindex-1.html @@ -0,0 +1,35 @@ + + + + + + +
+
+ + + diff --git a/layout/reftests/display-list/retained-dl-zindex-2-ref.html b/layout/reftests/display-list/retained-dl-zindex-2-ref.html new file mode 100644 index 000000000000..1eac061a7ee1 --- /dev/null +++ b/layout/reftests/display-list/retained-dl-zindex-2-ref.html @@ -0,0 +1,33 @@ + + + + + + +
+
+
+ + diff --git a/layout/reftests/display-list/retained-dl-zindex-2.html b/layout/reftests/display-list/retained-dl-zindex-2.html new file mode 100644 index 000000000000..db3b6e75b63b --- /dev/null +++ b/layout/reftests/display-list/retained-dl-zindex-2.html @@ -0,0 +1,41 @@ + + + + + + +
+
+
+ + +