Last Comment Bug 784846 - The ShouldPrerender check for async animations is wrong
: The ShouldPrerender check for async animations is wrong
Product: Core
Classification: Components
Component: Graphics: Layers (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla17
Assigned To: David Zbarsky (:dzbarsky)
: Milan Sreckovic [:milan]
Depends on:
Blocks: 780692
  Show dependency treegraph
Reported: 2012-08-22 15:29 PDT by David Zbarsky (:dzbarsky)
Modified: 2012-08-23 19:22 PDT (History)
2 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Patch (4.53 KB, patch)
2012-08-22 15:30 PDT, David Zbarsky (:dzbarsky)
no flags Details | Diff | Splinter Review
Patch (3.70 KB, patch)
2012-08-22 17:31 PDT, David Zbarsky (:dzbarsky)
matt.woodrow: review+
Details | Diff | Splinter Review

Description David Zbarsky (:dzbarsky) 2012-08-22 15:29:37 PDT
It checks for AreLayersMarkedActive(nsChangeHint_UpdateTransformLayer).  This check fails for opacity-only animations.
Comment 1 David Zbarsky (:dzbarsky) 2012-08-22 15:30:36 PDT
Created attachment 654389 [details] [diff] [review]
Comment 2 Matt Woodrow (:mattwoodrow) 2012-08-22 16:06:39 PDT
Comment on attachment 654389 [details] [diff] [review]

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) 2012-08-22 16:29:45 PDT
Comment on attachment 654389 [details] [diff] [review]

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) 2012-08-22 17:31:05 PDT
Created attachment 654454 [details] [diff] [review]
Comment 5 Matt Woodrow (:mattwoodrow) 2012-08-22 17:39:39 PDT
Comment on attachment 654454 [details] [diff] [review]

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

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

AreLayersMarkedActive(nsChangeHint_UpdateOpacityLayer) I think.
Comment 6 David Zbarsky (:dzbarsky) 2012-08-22 23:05:59 PDT
Yup, you're right.
Comment 7 Ryan VanderMeulen [:RyanVM] 2012-08-23 19:22:38 PDT

Note You need to log in before you can comment on or make changes to this bug.