784846 - The ShouldPrerender check for async animations is wrong

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[David Zbarsky (:dzbarsky)]

It checks for AreLayersMarkedActive(nsChangeHint_UpdateTransformLayer).  This check fails for opacity-only animations.

Comment 1

[David Zbarsky (:dzbarsky)]

Attachment: Patch

Comment 2

[Matt Woodrow (:mattwoodrow)]

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 3

[Matt Woodrow (:mattwoodrow)]

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.

Comment 4

[David Zbarsky (:dzbarsky)]

Attachment: Patch

Comment 5

[Matt Woodrow (:mattwoodrow)]

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.

Comment 6

[David Zbarsky (:dzbarsky)]

Yup, you're right.
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a645c4ea73f

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2a645c4ea73f