Last Comment Bug 784846 - The ShouldPrerender check for async animations is wrong
: The ShouldPrerender check for async animations is wrong
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics: Layers (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla17
Assigned To: David Zbarsky (:dzbarsky)
:
Mentors:
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:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (4.53 KB, patch)
2012-08-22 15:30 PDT, David Zbarsky (:dzbarsky)
no flags Details | Diff | Review
Patch (3.70 KB, patch)
2012-08-22 17:31 PDT, David Zbarsky (:dzbarsky)
matt.woodrow: review+
Details | Diff | 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]
Patch
Comment 2 Matt Woodrow (:mattwoodrow) 2012-08-22 16:06:39 PDT
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) 2012-08-22 16:29:45 PDT
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) 2012-08-22 17:31:05 PDT
Created attachment 654454 [details] [diff] [review]
Patch
Comment 5 Matt Woodrow (:mattwoodrow) 2012-08-22 17:39:39 PDT
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) 2012-08-22 23:05:59 PDT
Yup, you're right.
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a645c4ea73f
Comment 7 Ryan VanderMeulen [:RyanVM] 2012-08-23 19:22:38 PDT
https://hg.mozilla.org/mozilla-central/rev/2a645c4ea73f

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