Closed Bug 1472830 Opened 2 years ago Closed 2 years ago

Work from ApplyOpacityToChildren is never reused

Categories

(Core :: Web Painting, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox62 --- fixed
firefox63 --- fixed

People

(Reporter: bas.schouten, Assigned: bas.schouten)

Details

Attachments

(1 file)

With retained display list, if ApplyOpacityToChildren has concluded the opacity can not be handled by the children, it doesn't need to check again next frame if the child list has not mutated at all.
Comment on attachment 8989283 [details]
Bug 1472830: Reuse the work from ApplyOpacityToChildren when possible.

https://reviewboard.mozilla.org/r/254338/#review261198

::: layout/painting/RetainedDisplayListBuilder.cpp:300
(Diff revision 1)
>            if (mBuilder->MergeDisplayLists(aNewItem->GetChildren(),
>                                            oldItem->GetChildren(),
>                                            aNewItem->GetChildren(),
>                                            containerASRForChildren,
>                                            aNewItem->GetPerFrameKey())) {
> +            aNewItem->InvalidateCachedChildInfo();

This call is unnecessary, since aNewItem is new, and there can't be any cached state on it.
Attachment #8989283 - Flags: review?(matt.woodrow) → review+
Pushed by bschouten@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e6eabfa350b
Reuse the work from ApplyOpacityToChildren when possible. r=mattwoodrow
https://hg.mozilla.org/mozilla-central/rev/7e6eabfa350b
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Comment on attachment 8989283 [details]
Bug 1472830: Reuse the work from ApplyOpacityToChildren when possible.

Approval Request Comment
[Feature/Bug causing the regression]: Used by patch for bug 1472465 to fix opacity invalidation issue.
[User impact if declined]: Blocks uplifting bug 1472465. A similar state check would have to be implemented anyway for uplift.
[Is this code covered by automated tests?]: The code is run during most tests that use opacity, especially mochitests.
[Has the fix been verified in Nightly?]: N/A, this is just an optimization.
[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?]: Low risk.
[Why is the change risky/not risky?]: The code is easy to reason about - caching of enum value instead of recalculating it.
[String changes made/needed]: No.
Attachment #8989283 - Flags: approval-mozilla-beta?
Comment on attachment 8989283 [details]
Bug 1472830: Reuse the work from ApplyOpacityToChildren when possible.

Looks like we should land this along with the patch in bug 1472465.
Fix for recent regression, has good existing test coverage.
Attachment #8989283 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.