1407815 - Allow merging of items that are in different wrap lists

Status: RESOLVED FIXED

(Core :: Web Painting, enhancement)

Description

[Matt Woodrow (:mattwoodrow)]

Looks like the best fix for bug 1405146 is going to be adding more wrap lists, so we really need merging to work across flattened lists.

Comment 1

[Matt Woodrow (:mattwoodrow)]

Attachment: merge-through-flattening

Comment 2

[Miko Mynttinen]

Comment on attachment 8917587 [details] [diff] [review]
merge-through-flattening

Review of attachment 8917587 [details] [diff] [review]:
-----------------------------------------------------------------

I like the iterator idea!

::: layout/painting/FrameLayerBuilder.cpp
@@ +4012,5 @@
>        item = mBuilder->MergeItems(mergedItems);
>        MOZ_ASSERT(item && itemType == item->GetType());
>      }
>  
>      nsDisplayList* childItems = item->GetSameCoordinateSystemChildren();

Do we still need this variable?

::: layout/painting/nsDisplayList.h
@@ +5596,5 @@
> +  {
> +    // Handle the case where we reach the end of a nested list, or the current
> +    // item should start a new nested list. Repeat this until we find an actual
> +    // item, or the very end of the outer list.
> +    while ((!mNext && mStack.Length()) ||

I think using individual methods and branches would make this look a bit more obvious.
while (AtListEnd() || ShouldFlatten()) {
  if (AtListEnd()) {
  // ..
  }

  if (ShouldFlatten()) {
  // ..
  }
}

::: layout/reftests/bugs/reftest.list
@@ +1686,5 @@
>  == 655549-1.html 655549-1-ref.html
>  == 655836-1.html 655836-1-ref.html
>  != 656875.html about:blank
>  == 658952.html 658952-ref.html
> +fuzzy-if(skiaContent,7,3500) == 660682-1.html 660682-1-ref.html

I am guessing this regression comes from nsDisplayOpacity not getting flattened due to frame continuation condition you added, is this intended?

Comment 3

[Matt Woodrow (:mattwoodrow)]

(In reply to Miko Mynttinen [:miko] from comment #2)
> Do we still need this variable?

It's still used (for container items that don't get flattened, like most opacities). I could move it down closer to the usage location though.

> 
> I think using individual methods and branches would make this look a bit
> more obvious.
> while (AtListEnd() || ShouldFlatten()) {
>   if (AtListEnd()) {
>   // ..
>   }
> 
>   if (ShouldFlatten()) {
>   // ..
>   }
> }

Yeah, that's a good idea. It's less obvious that the two conditions are mutually exclusive, but I can add a comment for that.

> 
> ::: layout/reftests/bugs/reftest.list
> @@ +1686,5 @@
> >  == 655549-1.html 655549-1-ref.html
> >  == 655836-1.html 655836-1-ref.html
> >  != 656875.html about:blank
> >  == 658952.html 658952-ref.html
> > +fuzzy-if(skiaContent,7,3500) == 660682-1.html 660682-1-ref.html
> 
> I am guessing this regression comes from nsDisplayOpacity not getting
> flattened due to frame continuation condition you added, is this intended?

Somewhat, yes.

Previously we resolved merging first, and then flattening second, and in that test case we decided we could flatten the opacity away even after merging the split frames together.

With the iterator, we resolve flattening first, and it's not safe to flatten something if it wants to get merged since we won't have the item to merge any more.

That breaks rare where we previously managed to flatten and now don't (like this reftest), but it should be really rare, and it's only a perf optimization, so I decided that it didn't matter.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/78f548e4f687
Allow merging of items that are in different wrap lists. r=miko

Comment 5

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/1fc4aa69ffc9 for https://treeherder.mozilla.org/logviewer.html#?job_id=137388953&repo=mozilla-inbound

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd4a928d0293
Allow merging of items that are in different wrap lists. r=miko

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/fd4a928d0293