Last Comment Bug 784239 - Perform async animations even when not all properties can be asyncified.
: Perform async animations even when not all properties can be asyncified.
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:
  Show dependency treegraph
 
Reported: 2012-08-20 19:11 PDT by David Zbarsky (:dzbarsky)
Modified: 2012-12-03 15:01 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (3.40 KB, patch)
2012-08-20 20:09 PDT, David Zbarsky (:dzbarsky)
no flags Details | Diff | Splinter Review
Revised patch (14.42 KB, patch)
2012-08-21 09:45 PDT, David Zbarsky (:dzbarsky)
no flags Details | Diff | Splinter Review
Revised patch (14.64 KB, patch)
2012-08-21 09:50 PDT, David Zbarsky (:dzbarsky)
roc: review+
Details | Diff | Splinter Review

Description David Zbarsky (:dzbarsky) 2012-08-20 19:11:33 PDT

    
Comment 1 David Zbarsky (:dzbarsky) 2012-08-20 20:09:38 PDT
Created attachment 653636 [details] [diff] [review]
Patch

Much easier than what I was doing before :)
Comment 2 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-08-20 20:57:11 PDT
Comment on attachment 653636 [details] [diff] [review]
Patch

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

I don't really understand this approach. Why should CanAnimatePropertyOnCompositor return true for properties that can't really be animated on the compositor? It seems to me we need a slightly different setup here, so that when CanAnimatePropertyOnCompositor is called for the transform property, it checks for left/top/etc and returns false if they're being animated.

::: layout/style/AnimationCommon.cpp
@@ +236,2 @@
>        }
>        return false;

Wouldn't it make more sense to have the default case in this switch statement by that the property is not supported?
Comment 3 David Zbarsky (:dzbarsky) 2012-08-21 09:45:09 PDT
Created attachment 653802 [details] [diff] [review]
Revised patch

This ended up simplifying some of the CanPerformOnCompositor checks that I really didn't like before.

I think it is better to allow properties by default, since there are many properties that can be animated and we will forget to update this as new ones are added.
Comment 4 David Zbarsky (:dzbarsky) 2012-08-21 09:50:56 PDT
Created attachment 653805 [details] [diff] [review]
Revised patch

The if (frame) checks are left over from a long time ago, when we used to call CanPerformOnCompositorThread when adding rules to nsAnimationManager.  There is always a frame now.
Comment 5 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-08-21 16:08:48 PDT
Comment on attachment 653805 [details] [diff] [review]
Revised patch

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

::: layout/style/AnimationCommon.cpp
@@ +234,5 @@
>  
>  bool
>  CommonElementAnimationData::CanAnimatePropertyOnCompositor(const dom::Element *aElement,
> +                                                           nsCSSProperty aProperty,
> +                                                           bool hasGeometricProperties)

aHasGeometricProperties

::: layout/style/AnimationCommon.h
@@ +153,5 @@
>  
>    static bool
>    CanAnimatePropertyOnCompositor(const dom::Element *aElement,
> +                                 nsCSSProperty aProperty,
> +                                 bool hasGeometricProperties);

aHasGeometricProperties
Comment 6 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-08-21 16:09:37 PDT
Did any support for animating 'visibility' on the compositor land already? If so, you should back it out after this patch has landed.
Comment 7 David Zbarsky (:dzbarsky) 2012-08-21 16:11:21 PDT
The only thing I landed was the check for visibility in CanAnimatePropertyOnCompositor, which is removed in this patch.
Comment 8 David Zbarsky (:dzbarsky) 2012-08-21 18:49:33 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/1941821345f3
Comment 9 Ed Morley [:emorley] 2012-08-22 02:44:43 PDT
https://hg.mozilla.org/mozilla-central/rev/1941821345f3
Comment 10 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-12-03 14:17:09 PST
What's the intent of HasGeometricProperties here?  If it's to catch any animation that might change the size of the frame, this check is far from sufficient.  (There also could be animations on *other* frames that might change the size of the frame.)

That said, I'm also not sure what the benefit of this patch is -- I guess it'll lead us to animate more smoothly when the main thread can't keep up, and even if the thing we're doing on the compositor thread isn't exactly the right animation it'll get fixed every time the main thread does something?  It seems like if that's the idea, it should at least be explained clearly in code comments.
Comment 11 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-12-03 14:27:11 PST
Also, given the changes this makes to CommonElementAnimationData::CanAnimatePropertyOnCompositor, what prevents us from trying to animate every animation not involving width,height,top,right,bottom, or left on the compositor, even if it doesn't involve any of the properties actually can be animated on the compositor?
Comment 12 David Zbarsky (:dzbarsky) 2012-12-03 15:01:29 PST
(In reply to David Baron [:dbaron] from comment #10)
> What's the intent of HasGeometricProperties here?  If it's to catch any
> animation that might change the size of the frame, this check is far from
> sufficient.  (There also could be animations on *other* frames that might
> change the size of the frame.)

Yes, that was the original intent.  I wanted to avoid animating transform if it would be incorrect.

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