The ShouldPrerender check for async animations is wrong

RESOLVED FIXED in mozilla17

Status

()

Core
Graphics: Layers
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dzbarsky, Assigned: dzbarsky)

Tracking

unspecified
mozilla17
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
It checks for AreLayersMarkedActive(nsChangeHint_UpdateTransformLayer).  This check fails for opacity-only animations.
(Assignee)

Comment 1

5 years ago
Created attachment 654389 [details] [diff] [review]
Patch
Attachment #654389 - Flags: review?(matt.woodrow)
(Assignee)

Updated

5 years ago
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.
(Assignee)

Comment 4

5 years ago
Created attachment 654454 [details] [diff] [review]
Patch
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+
(Assignee)

Comment 6

5 years ago
Yup, you're right.
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a645c4ea73f
https://hg.mozilla.org/mozilla-central/rev/2a645c4ea73f
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.