Closed Bug 784846 Opened 8 years ago Closed 8 years ago

The ShouldPrerender check for async animations is wrong

Categories

(Core :: Graphics: Layers, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla17

People

(Reporter: dzbarsky, Assigned: dzbarsky)

References

Details

Attachments

(1 file, 1 obsolete file)

It checks for AreLayersMarkedActive(nsChangeHint_UpdateTransformLayer).  This check fails for opacity-only animations.
Attached patch Patch (obsolete) — Splinter Review
Attachment #654389 - Flags: review?(matt.woodrow)
Blocks: 780692
Comment on attachment 654389 [details] [diff] [review]
Patch

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

::: layout/base/nsDisplayList.cpp
@@ +344,5 @@
>    }
>  
>    // If the frame is not prerendered, bail out.  Layout will still perform the
>    // animation.
> +  if (!nsDisplayTransform::ShouldPrerenderContent(aBuilder, frame)) {

This only changes the 'can we assume that the content will be prerendered, and thus do async animation' check, it doesn't actually make the content prerender.

You'll need to find all the callers of ShouldPrerenderTransformedContent and all the equivalent places in nsDisplayOpacity and add the same checks there.

The ShouldPrerenderContent code probably should move from nsDisplayTransform -> nsDisplayItem too.
Comment on attachment 654389 [details] [diff] [review]
Patch

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

As discussed on irc, this should be fine because opacity animations don't need to be prerendered as such.

Can you make this a function on nsDisplayItem instead though. Maybe 'CanUseAsyncAnimations' or similar? Return false by default and make the opacity/transform display items overload it with what they need.

Returning true from nsDisplayTransform::ShouldPrerenderContent seems a bit weird when it's an nsDisplayOpacity and it isn't prerendered.

Rest looks good.
Attached patch PatchSplinter Review
Attachment #654389 - Attachment is obsolete: true
Attachment #654389 - Flags: review?(matt.woodrow)
Attachment #654454 - Flags: review?(matt.woodrow)
Comment on attachment 654454 [details] [diff] [review]
Patch

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

::: layout/base/nsDisplayList.h
@@ +1963,5 @@
>  
>    NS_DISPLAY_DECL_NAME("FixedPosition", TYPE_FIXED_POSITION)
>  
> +  bool CanUseAsyncAnimations(nsDisplayListBuilder* aBuilder) {
> +    return GetUnderlyingFrame()->AreLayersMarkedActive();

AreLayersMarkedActive(nsChangeHint_UpdateOpacityLayer) I think.
Attachment #654454 - Flags: review?(matt.woodrow) → review+
https://hg.mozilla.org/mozilla-central/rev/2a645c4ea73f
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.