Closed Bug 1416448 Opened 7 years ago Closed 7 years ago

MergeDisplayLists container asr tracking doesn't handle empty lists

Categories

(Core :: Web Painting, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox58 --- fixed
firefox59 --- fixed

People

(Reporter: mattwoodrow, Assigned: mattwoodrow)

References

Details

Attachments

(1 file)

We compute the outer ASR for all the items in the list, but we don't differentiate between the initial nullptr value and the actual root asr (represented as nullptr).

This is causing a reftest failure since we compute the wrong ASR for an empty list, and propagate it up the tree.

Switching to use a Maybe<> value lets us distinguish between the two cases, and not modify the ASR when the list is empty.

I also tried just getting rid of empty items, but it's possible for an empty nsDisplaySVGEffects to still draw something.
Attachment #8927550 - Flags: review?(mikokm)
Attachment #8927550 - Flags: review?(mstange)
Comment on attachment 8927550 [details] [diff] [review]
skip-update-asr-empty-list

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

::: layout/painting/RetainedDisplayListBuilder.cpp
@@ +414,3 @@
>            MergeDisplayLists(newItem->GetChildren(), oldItem->GetChildren(),
> +                            oldItem->GetChildren(), containerASRForChildren);
> +          if (containerASRForChildren) {

nit: UpdateASR could take Maybe<> as a parameter and have this check to avoid repeating this code.
Attachment #8927550 - Flags: review?(mikokm) → review+
(In reply to Miko Mynttinen [:miko] from comment #1)
> Comment on attachment 8927550 [details] [diff] [review]
> skip-update-asr-empty-list
> 
> Review of attachment 8927550 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/painting/RetainedDisplayListBuilder.cpp
> @@ +414,3 @@
> >            MergeDisplayLists(newItem->GetChildren(), oldItem->GetChildren(),
> > +                            oldItem->GetChildren(), containerASRForChildren);
> > +          if (containerASRForChildren) {
> 
> nit: UpdateASR could take Maybe<> as a parameter and have this check to
> avoid repeating this code.

We don't actually allow Maybe<> as a parameter, but I'll make it a reference and it should work.
Attachment #8927550 - Flags: review?(mstange)
Pushed by mwoodrow@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ec68ec7ff01
Don't update the ASR during merging for an empty container item, since we can't compute the ASR of the contents. r=miko
https://hg.mozilla.org/mozilla-central/rev/2ec68ec7ff01
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Comment on attachment 8927550 [details] [diff] [review]
skip-update-asr-empty-list

Approval Request Comment
[Feature/Bug causing the regression]: bug 1352499. This is code that is preffed off, but we want to run a shield study enabling the pref.
[User impact if declined]: None, preffed off code.
[Is this code covered by automated tests?]: Yes, when the pref is enabled.
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Code is preffed off.
[String changes made/needed]: None
Attachment #8927550 - Flags: approval-mozilla-beta?
Comment on attachment 8927550 [details] [diff] [review]
skip-update-asr-empty-list

Take this to support the shield study in 58. Beta58+.
Attachment #8927550 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: needinfo?(matt.woodrow)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: