Last Comment Bug 755084 - do animations on the compositor thread when possible
: do animations on the compositor thread when possible
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: 706179 769193 849399
Blocks: 772642 775629 869129 771367
  Show dependency treegraph
 
Reported: 2012-05-14 15:45 PDT by Andreas Gal :gal
Modified: 2013-05-06 12:41 PDT (History)
22 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (13.07 KB, patch)
2012-05-14 15:46 PDT, Andreas Gal :gal
no flags Details | Diff | Splinter Review
Rollup patch (56.25 KB, patch)
2012-05-21 14:55 PDT, David Zbarsky (:dzbarsky)
no flags Details | Diff | Splinter Review
Layout patch (37.00 KB, patch)
2012-06-05 17:35 PDT, David Zbarsky (:dzbarsky)
no flags Details | Diff | Splinter Review
Layout patch (73.98 KB, patch)
2012-06-13 17:38 PDT, David Zbarsky (:dzbarsky)
no flags Details | Diff | Splinter Review
Patch, cleaned up a little (74.98 KB, patch)
2012-06-14 14:47 PDT, David Zbarsky (:dzbarsky)
no flags Details | Diff | Splinter Review
Patch (75.79 KB, patch)
2012-06-19 12:38 PDT, David Zbarsky (:dzbarsky)
no flags Details | Diff | Splinter Review
Move ElementAnimations to the header. (12.46 KB, patch)
2012-06-26 05:16 PDT, David Zbarsky (:dzbarsky)
no flags Details | Diff | Splinter Review
Patch (70.30 KB, patch)
2012-06-26 05:17 PDT, David Zbarsky (:dzbarsky)
cjones.bugs: review+
Details | Diff | Splinter Review
Move ElementAnimations to the header. (10.00 KB, patch)
2012-07-09 15:18 PDT, David Zbarsky (:dzbarsky)
dbaron: review+
Details | Diff | Splinter Review
Layout patch (105.65 KB, patch)
2012-07-09 15:19 PDT, David Zbarsky (:dzbarsky)
no flags Details | Diff | Splinter Review
Patch (105.33 KB, patch)
2012-07-16 18:34 PDT, David Zbarsky (:dzbarsky)
no flags Details | Diff | Splinter Review
Patch -w (79.23 KB, patch)
2012-07-17 07:00 PDT, David Zbarsky (:dzbarsky)
dbaron: review+
roc: review+
Details | Diff | Splinter Review
Patch to add support for perspective transform function (3.29 KB, patch)
2012-07-18 20:59 PDT, David Zbarsky (:dzbarsky)
roc: review+
dbaron: review+
Details | Diff | Splinter Review
Move CanAnimateOpacity/Transforms to nsLayoutUtils (8.44 KB, patch)
2012-07-20 19:48 PDT, David Zbarsky (:dzbarsky)
cjones.bugs: review+
dbaron: review+
Details | Diff | Splinter Review

Description Andreas Gal :gal 2012-05-14 15:45:21 PDT
This will require support at the layers level, and in layout. This bug will focus on the layout changes, and we will make it depend on another bug cjones is working on for the layers changes.
Comment 1 Andreas Gal :gal 2012-05-14 15:46:12 PDT
Created attachment 623852 [details] [diff] [review]
patch
Comment 2 Andreas Gal :gal 2012-05-14 16:14:59 PDT
Here is the basic plan of attack:
- transition and animation manager keep separate lists of animations-per-element
- when adding stuff to that list, see whether all the animations on the element can be done OMT (opacity and transform, and no preserve-3d), and if so, put in separate list and don't register us for the refresh driver
- only do this when there are active layers for the frame, since we need a layer to do this (when frames are animated we automatically give them a layer and then after a short timeout we revoke the layer, we do this to save memory)
- figure out a way to pull the animation onto the layer, this will be done in BuildLayer
- when we repaint, BuildLayer gets called and sets up the layer and it recycles existing layers where it can (retained layers) so we don't have to repaint it
- the idea is to put the animation (e.g. opacity 0->1 over 0.5s) onto the layer in build layer and then have the compositor do it
- the API cjones is doing for layers will do a callback when the animation is done, we will fire completion events from that callback
- when the style context changes we might have to revoke animations from the layer, this will be tricky, we have to chat with bz whats the best way to do this. Imaging an element has only an opacity changes and then we change the animation to do something that requires reflow. We may or may not want to keep running the animation on the compositor thread (though, maybe we can, that would simplify things)
Comment 3 Boris Zbarsky [:bz] 2012-05-14 18:13:38 PDT
Note that if main thread queries computed style while the animation is running it needs to see the animation-affected value...
Comment 4 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-05-17 19:31:31 PDT
Blake, this is the layout side of what we discussed in private email.

Blocking bug 706179 I guess we can refocus to the layers side of things.  It has to land before this, but that work doesn't depend on this at all.
Comment 5 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-05-17 19:33:30 PDT
(In reply to Boris Zbarsky (:bz) from comment #3)
> Note that if main thread queries computed style while the animation is
> running it needs to see the animation-affected value...

Is it OK if those values report what they currently would, but not necessary 100% precisely the "current" value as interpolated by the compositor thread?  I can't really thing of any other sane way to make this work.
Comment 6 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-17 19:35:22 PDT
(In reply to Chris Jones [:cjones] [:warhammer] from comment #5)
> (In reply to Boris Zbarsky (:bz) from comment #3)
> > Note that if main thread queries computed style while the animation is
> > running it needs to see the animation-affected value...
> 
> Is it OK if those values report what they currently would, but not necessary
> 100% precisely the "current" value as interpolated by the compositor thread?
> I can't really thing of any other sane way to make this work.

Currently, content querying computed style in a background tab does not see animated values.

I think this is OK and we can also allow the values to lag async compositing.
Comment 7 Boris Zbarsky [:bz] 2012-05-17 19:52:57 PDT
> Is it OK if those values report what they currently would

Yes, absolutely.
Comment 8 David Zbarsky (:dzbarsky) 2012-05-18 10:37:17 PDT
Note to whoever works on this: ElementAnimations::CanPerformOnCompositorThread() should return true at the end instead of false.
Comment 9 David Zbarsky (:dzbarsky) 2012-05-21 14:55:09 PDT
Created attachment 625774 [details] [diff] [review]
Rollup patch
Comment 10 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-21 15:43:33 PDT
Comment on attachment 625774 [details] [diff] [review]
Rollup patch

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

The Layers API seems reasonable, but I'd like Brian and Bas to chime in with any thoughts.

Probably the hardest part here is hooking up FrameLayerBuilder to automatically extract the animation data and place it on the layers, making sure that we cancel animations at the right time without accidentally restarting animations in the compositor. And that isn't done yet, so it's a little hard to see how much work it's going to be. But so far so good.

::: gfx/layers/Layers.h
@@ +731,5 @@
> +  public:
> +    virtual ~AnimationObserver() { }
> +    virtual void Cancelled(/* Animation& ?*/) = 0;
> +    virtual void Done(/* Animation& ?*/) = 0;
> +  };

What's this for?

::: gfx/layers/ipc/PLayers.ipdl
@@ +108,5 @@
> +
> +struct Animation {
> +  CubicBezier sampleFn;
> +  TimeStamp startTime;
> +  TimeDuration duration;

You're relying on TimeStamps being portable across processes. I don't know that that's true. If it is, better document it in TimeStamp.h.

@@ +114,5 @@
> +  // elapsed time between [start, end] and the sample function.  Then
> +  // the property's value is interpolated at that point.  There must
> +  // be keyframes for points 0.0 and 1.0.  The keyframes must be
> +  // sorted by |point|.
> +  KeyFrame[] keyframes;

Wouldn't it make more sense to have one CubicBezier between each KeyFrame? Seems to me that would match CSS animations better. Seems like a better model would be to give each KeyFrame a duration and sampleFn and remove duration from Animation.

@@ +117,5 @@
> +  // sorted by |point|.
> +  KeyFrame[] keyframes;
> +  // How many times to repeat the animation.  < 0 means "forever".
> +  int32_t numIterations;
> +};

Are you going to require that there be at most one Animation per animatable attribute of the layer? If so, better specify it.
Comment 11 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-05-21 15:47:50 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #10)
> Comment on attachment 625774 [details] [diff] [review]
> Rollup patch
> 
> ::: gfx/layers/Layers.h
> @@ +731,5 @@
> > +  public:
> > +    virtual ~AnimationObserver() { }
> > +    virtual void Cancelled(/* Animation& ?*/) = 0;
> > +    virtual void Done(/* Animation& ?*/) = 0;
> > +  };
> 
> What's this for?
> 

Consumers in layout need to know when the animation finishes on the compositor so that they can send out animationend/transitionend events.  This is just a sketch of how that might look, not hooked up yet.  We might want to do something different.

> ::: gfx/layers/ipc/PLayers.ipdl
> @@ +108,5 @@
> > +
> > +struct Animation {
> > +  CubicBezier sampleFn;
> > +  TimeStamp startTime;
> > +  TimeDuration duration;
> 
> You're relying on TimeStamps being portable across processes. I don't know
> that that's true. If it is, better document it in TimeStamp.h.
> 

It is.  Yes, that should be documented.

> @@ +114,5 @@
> > +  // elapsed time between [start, end] and the sample function.  Then
> > +  // the property's value is interpolated at that point.  There must
> > +  // be keyframes for points 0.0 and 1.0.  The keyframes must be
> > +  // sorted by |point|.
> > +  KeyFrame[] keyframes;
> 
> Wouldn't it make more sense to have one CubicBezier between each KeyFrame?
> Seems to me that would match CSS animations better. Seems like a better
> model would be to give each KeyFrame a duration and sampleFn and remove
> duration from Animation.
> 

Sure, that would be a trivial change on the layers side.  We should just do whichever is cleaner.

> @@ +117,5 @@
> > +  // sorted by |point|.
> > +  KeyFrame[] keyframes;
> > +  // How many times to repeat the animation.  < 0 means "forever".
> > +  int32_t numIterations;
> > +};
> 
> Are you going to require that there be at most one Animation per animatable
> attribute of the layer? If so, better specify it.

Yes, that's documented in Layers.h I believe.
Comment 12 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-21 15:52:13 PDT
(In reply to Chris Jones [:cjones] [:warhammer] from comment #11)
> Consumers in layout need to know when the animation finishes on the
> compositor so that they can send out animationend/transitionend events. 
> This is just a sketch of how that might look, not hooked up yet.  We might
> want to do something different.

Is that necessary? Seems to me that layout could just time them itself. I think a one-way interface would be simpler, if it can be made to work properly.
Comment 13 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-21 15:52:47 PDT
(In reply to Chris Jones [:cjones] [:warhammer] from comment #11)
> Yes, that's documented in Layers.h I believe.

Not as far as I can tell.
Comment 14 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-05-21 16:03:51 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #12)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #11)
> > Consumers in layout need to know when the animation finishes on the
> > compositor so that they can send out animationend/transitionend events. 
> > This is just a sketch of how that might look, not hooked up yet.  We might
> > want to do something different.
> 
> Is that necessary? Seems to me that layout could just time them itself. I
> think a one-way interface would be simpler, if it can be made to work
> properly.

Sure.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #13)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #11)
> > Yes, that's documented in Layers.h I believe.
> 
> Not as far as I can tell.

Ah, that's because there are no comments in Layers.h.  See the assertions in Layer::AddAnimation(const Animation& aAnimation).
Comment 15 Brian Birtles (:birtles) 2012-05-22 21:57:52 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #10)
> The Layers API seems reasonable, but I'd like Brian and Bas to chime in with
> any thoughts.

It looks good to me although I'm not really sure where this fits in the pipeline since I'm not familiar with the CSS animations code.

Just a few thoughts:
* How do you cancel a single animation? ClearAnimations followed by SetAnimations? (This is reasonably common in SVG and the new Web Animations stuff we're proposing.)
* I assume that the CSS animations code does sampling in parallel so it can dispatch iteration events etc.? Is that right, the purpose of the Done/Cancel notification is just for thread synchronisation not for event dispatching? (Bear in mind those events need to be dispatched in a timely fashion for various reasons I can expand on here.)
* Just to clarify, I assume this is intended for common/simple cases. It's ok if there are many arrangements of animations that can't be handled by the API right?

Otherwise, looks good.
Comment 16 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-05-23 01:08:21 PDT
(In reply to Brian Birtles (:birtles) from comment #15)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #10)
> > The Layers API seems reasonable, but I'd like Brian and Bas to chime in with
> > any thoughts.
> 
> Just a few thoughts:
> * How do you cancel a single animation? ClearAnimations followed by
> SetAnimations? (This is reasonably common in SVG and the new Web Animations
> stuff we're proposing.)

The cancel would be initiated by the content thread (running content JS code), which would trigger a layers transaction that removed the animated property.  After we forwarded the layers transaction to the compositor, the animation would simply just stop running because it would no longer exist.  The mechanics of that would probably require ClearAnimations() or something like it, yeah.

> * I assume that the CSS animations code does sampling in parallel so it can
> dispatch iteration events etc.?

Hm, I know nothing about those.

> Is that right, the purpose of the
> Done/Cancel notification is just for thread synchronisation not for event
> dispatching? (Bear in mind those events need to be dispatched in a timely
> fashion for various reasons I can expand on here.)

I prefer roc's suggestion that we decouple the compositor's animations from the content threads'.  In that case, event dispatch would continue to happen as it does currently, and we would make a best effort to have it match what's happening on the compositor thread.

> * Just to clarify, I assume this is intended for common/simple cases. It's
> ok if there are many arrangements of animations that can't be handled by the
> API right?
> 

Absolutely!
Comment 17 Brian Birtles (:birtles) 2012-05-23 16:27:40 PDT
(In reply to Chris Jones [:cjones] [:warhammer] from comment #16)
> > Just a few thoughts:
> > * How do you cancel a single animation? ClearAnimations followed by
> > SetAnimations? (This is reasonably common in SVG and the new Web Animations
> > stuff we're proposing.)
> 
> The cancel would be initiated by the content thread (running content JS
> code), which would trigger a layers transaction that removed the animated
> property.  After we forwarded the layers transaction to the compositor, the
> animation would simply just stop running because it would no longer exist. 
> The mechanics of that would probably require ClearAnimations() or something
> like it, yeah.

Yeah, I'm just interested in those mechanics. I wonder if its good to be able to remove a single animation rather than clearing all then setting them all minus 1. Maybe that's something to add later though.

> > * I assume that the CSS animations code does sampling in parallel so it can
> > dispatch iteration events etc.?
> 
> Hm, I know nothing about those.

I think the next comment about decoupling the compositor's animations from the content threads' answers my questions here. So long as the content thread continues dispatching events without waiting for the compositor thread I think it's fine.

Thanks Chris!
Comment 18 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-05-23 16:30:31 PDT
(In reply to Brian Birtles (:birtles) from comment #17)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #16)
> > > Just a few thoughts:
> > > * How do you cancel a single animation? ClearAnimations followed by
> > > SetAnimations? (This is reasonably common in SVG and the new Web Animations
> > > stuff we're proposing.)
> > 
> > The cancel would be initiated by the content thread (running content JS
> > code), which would trigger a layers transaction that removed the animated
> > property.  After we forwarded the layers transaction to the compositor, the
> > animation would simply just stop running because it would no longer exist. 
> > The mechanics of that would probably require ClearAnimations() or something
> > like it, yeah.
> 
> Yeah, I'm just interested in those mechanics. I wonder if its good to be
> able to remove a single animation rather than clearing all then setting them
> all minus 1. Maybe that's something to add later though.
> 

Gotcha.  I suspect it might be simpler to clear all animations and rebuild them when needed.  But experience and/or bz/dbaron/roc will tell.
Comment 19 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-05-29 17:24:01 PDT
We're close to being able to land a pref'd-off v1.  Time to start thinking about testing.

Existing reftests/etc. should cover rendering correctness already.  (Except for the total omtc bustage, which is being tracked in another bug.)

I think the extra thing we need to test here is that animations/transitions we expect to be async'ified actually are.  To do that, I think we can add a new interface kind of like

http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/base/nsIDOMWindowUtils.idl#1037

that returns whether the *Layer for a DOM element has an async animation configured for it.  (A yes/no answer is probably good enough for now.)  The tricky thing will be ensuring that the check happens while the animation is still in progress, but we should be able to listen for animationend/transitionend and discard the results of the check if our code doesn't run before then.

Modeling these tests after the ones that use checkAndClearPaintedState() seems reasonable to me.
Comment 20 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-29 17:26:20 PDT
Sounds reasonable.
Comment 21 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-29 17:28:01 PDT
Also, since the current plan is to not update getComputedStyle for off-main-thread animations, we should be able to write a test that a) takes drawWindow snapshots routed through OTMC's compositor to confirm that animation is happening and b) checks getComputedStyle to confirm that they're not happening on the main thread.
Comment 22 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-05-29 17:30:44 PDT
Oh ... we were planning to update getComputedStyle on-demand.  Do you think we can get away with totally ignoring it?
Comment 23 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-29 17:33:54 PDT
We get away with totally ignoring it in background tabs. But true, that may not carry over to foreground tabs.

I'm not sure what you mean by "on demand". If you mean "on any FlushPendingNotifications(Flush_Style)", then that's going to happen a lot.
Comment 24 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-05-29 17:40:00 PDT
When some code requests the value of a property that's being asynchronously animated.  Not sure how hard that would be to implement, maybe we can punt to v2.
Comment 25 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-29 17:42:15 PDT
We could make that work for getComputedStyle for the particular property. But it would be harder to make work for everything that depends on the property, e.g. callers of getBoundingClientRect.
Comment 26 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-29 17:46:49 PDT
BTW one issue with off-main-thread animated transforms is when a transformed element is animated to overflow a scrollable container (e.g. overflow:auto). We need to reflow to show the scrollbar properly.

I had thought it would be a good idea to apply all animations at a low rate on the main thread to address issues like this.
Comment 27 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-05-29 17:56:32 PDT
That's not an issue for android or b2g because of async panning/zooming.

I would like to vote for transforms *not* showing a scrollbar.
Comment 28 Boris Zbarsky [:bz] 2012-05-29 18:03:24 PDT
> We need to reflow to show the scrollbar properly.

Fwiw, see https://bugs.webkit.org/show_bug.cgi?id=78494
Comment 29 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-29 19:04:57 PDT
(In reply to Chris Jones [:cjones] [:warhammer] from comment #27)
> I would like to vote for transforms *not* showing a scrollbar.

You mean transformed elements being treated as if there's no transform for the purposes of computing the scrollable overflow area of their scrollable container?

That might be possible but would require spec work, and in many cases it's not what authors would want.
Comment 30 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-05-29 19:09:21 PDT
Right.  It's appealing to me to have a mental model of transforms as only affecting rendering, not layout, to have a more reliable baseline guarantee of perf.  My understanding was that was the original problem they were trying to solve.

But this is of course not the right place to have that discussion :).
Comment 31 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-30 14:15:01 PDT
Where do you want to have that discussion? www-style?
Comment 32 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-05-30 14:18:22 PDT
That, or we could start on dev-platform.
Comment 33 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-05-30 14:27:55 PDT
Even if you have a model where transforms don't need to affect scrollable overflow, they're still going to need to affect visual overflow.  (Even when things are layerized, we use visual overflow for things other than painting, so I'd think not updating it would break things.)
Comment 34 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-30 16:05:07 PDT
We use visual overflow to determine where to draw CSS 'outline'. That's really a bug though. What else do we use visual overflow for that's not related to invalidation or paint optimization?
Comment 35 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-30 16:32:05 PDT
Thread launched on www-style: http://lists.w3.org/Archives/Public/www-style/2012May/1155.html
Comment 36 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-05-30 16:41:59 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #34)
> We use visual overflow to determine where to draw CSS 'outline'. That's
> really a bug though. What else do we use visual overflow for that's not
> related to invalidation or paint optimization?

Event handling, element-from-point APIs.
Comment 37 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-30 17:32:58 PDT
Ah yes. In fact, to handle events Webkit actually does flush out the current animation state during an animation sometimes :-(. I'll follow up on the www-style thread.
Comment 38 David Zbarsky (:dzbarsky) 2012-06-05 17:35:30 PDT
Created attachment 630386 [details] [diff] [review]
Layout patch
Comment 39 David Zbarsky (:dzbarsky) 2012-06-13 17:38:37 PDT
Created attachment 632976 [details] [diff] [review]
Layout patch
Comment 40 David Zbarsky (:dzbarsky) 2012-06-14 14:47:15 PDT
Created attachment 633288 [details] [diff] [review]
Patch, cleaned up a little
Comment 41 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-06-18 15:11:26 PDT
Comment on attachment 633288 [details] [diff] [review]
Patch, cleaned up a little

Could you describe:
 * what this patch does, and
 * how it fits in to the larger picture?
Comment 42 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-06-18 16:03:07 PDT
(answer is in bug 706179 comment 29)
Comment 43 David Zbarsky (:dzbarsky) 2012-06-19 12:38:30 PDT
Created attachment 634550 [details] [diff] [review]
Patch

Passes tryserver preffed off.
Comment 44 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-06-19 16:02:52 PDT
Comment on attachment 634550 [details] [diff] [review]
Patch

>+      case eCSSKeyword_matrix:
>+      {
>+        gfx3DMatrix matrix;
>+        matrix._11 = array->Item(1).GetFloatValue();
>+        matrix._12 = array->Item(2).GetFloatValue();
>+        matrix._13 = array->Item(3).GetFloatValue();
>+        matrix._14 = array->Item(4).GetFloatValue();
>+        matrix._21 = array->Item(5).GetFloatValue();
>+        matrix._22 = array->Item(6).GetFloatValue();
>+        matrix._23 = array->Item(7).GetFloatValue();
>+        matrix._24 = array->Item(8).GetFloatValue();
>+        matrix._31 = array->Item(9).GetFloatValue();
>+        matrix._32 = array->Item(10).GetFloatValue();
>+        matrix._33 = array->Item(11).GetFloatValue();
>+        matrix._34 = array->Item(12).GetFloatValue();
>+        matrix._41 = array->Item(13).GetFloatValue();
>+        matrix._42 = array->Item(14).GetFloatValue();
>+        matrix._43 = array->Item(15).GetFloatValue();
>+        matrix._44 = array->Item(16).GetFloatValue();
>+        aFunctions.AppendElement(TransformMatrix(matrix));
>+        break;
>+      }
>+      case eCSSKeyword_matrix3d:
>+      {
>+        gfx3DMatrix matrix;
>+        matrix._11 = array->Item(1).GetFloatValue();
>+        matrix._12 = array->Item(2).GetFloatValue();
>+        matrix._13 = 0;
>+        matrix._14 = array->Item(3).GetFloatValue();
>+        matrix._21 = array->Item(4).GetFloatValue();
>+        matrix._22 = array->Item(5).GetFloatValue();
>+        matrix._23 = 0;
>+        matrix._24 = array->Item(6).GetFloatValue();
>+        matrix._31 = 0;
>+        matrix._32 = 0;
>+        matrix._33 = 1;
>+        matrix._34 = 0;
>+        matrix._41 = 0;
>+        matrix._42 = 0;
>+        matrix._43 = 0;
>+        matrix._44 = 1;
>+        aFunctions.AppendElement(TransformMatrix(matrix));
>+        break;
>+      }

These are pretty clearly backwards.

I think you need to find a way to repurpose existing tests (like test_transform_transition() in layout/style/test/test_transitions_per_property.html) to test this code and the existing code at the same time.  Otherwise we're going to keep finding differences.


(I still need to do more reading to understand what this patch is doing, though.)
Comment 45 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-06-19 16:31:28 PDT
Comment on attachment 633288 [details] [diff] [review]
Patch, cleaned up a little

>+static void
>+AddTransformAnimations(ElementAnimations* ea, Layer* aLayer, const nsPoint& aOrigin) {
>+
>+  nsIFrame* frame = ea->mElement->GetPrimaryFrame();
>+  nsRect bounds = nsDisplayTransform::GetFrameBoundsForTransform(frame);
>+  float scale = nsDeviceContext::AppUnitsPerCSSPixel();
>+  gfxPoint3D toMozOrigin =  nsDisplayTransform::GetDeltaToMozTransformOrigin(frame, scale, &bounds);
>+  gfxPoint3D toPerspectiveOrigin = nsDisplayTransform::GetDeltaToMozPerspectiveOrigin(frame, scale);

The first of these should be called toTransformOrigin, not toMozOrigin.

>+  nscoord perspective = 0.0;
>+  nsStyleContext* parentStyleContext = frame->GetStyleContext()->GetParent();
>+  if (parentStyleContext) {
>+    const nsStyleDisplay* disp = parentStyleContext->GetStyleDisplay();
>+    if (disp && disp->mChildPerspective.GetUnit() == eStyleUnit_Coord) {
>+      perspective = disp->mChildPerspective.GetCoordValue();
>+    }
>+  }

This code seems unused.

>+  for (PRUint32 i = 0; i < ea->mAnimations.Length(); i++) {
>+    int iterations = ea->mAnimations[i].mIterationCount != NS_IEEEPositiveInfinity()
>+                       ? ea->mAnimations[i].mIterationCount : -1;
>+    for (PRUint32 j = 0; j < ea->mAnimations[i].mProperties.Length(); j++) {
>+      AnimationProperty* property = &ea->mAnimations[i].mProperties[j];
>+      InfallibleTArray<AnimationSegment> segments;
>+
>+      if (property->mProperty == eCSSProperty_transform) {
>+        for (PRUint32 k = 0; k < property->mSegments.Length(); k++) {

Is this code going to end up being duplicated for every property that we support animating on the compositor thread?
Comment 46 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-06-19 16:47:55 PDT
Er, ok, |perspective| is used.  But I'm still a bit puzzled.  Transform, transform-origin, perspective, and perspective-origin can all be animated separately.  It looks like the idea of the patch is that you only do compositor-based animation when only transform is animating on that element.  But that doesn't account for:
 * one of the others, or the element's bounds, being changed in some other way (e.g., a transition, a SMIL animation, or )
 * perspective or perspective origin being animated on the parent (since it's the perspective for a different element that matters)
What happens in these cases?  Do we constantly send animations over to the compositor thread and then change them?  Or do we get things wrong?

And, actually, I think the |toPerspectiveOrigin| and |toTransformOrigin| names are still very confusing, since within the same function you're using the "to" prefix for the destination of an animation.  I think they need to be called "offsetTo..." to distinguish them from "fromFunctions" and "toFunctions".
Comment 47 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-06-20 12:54:00 PDT
Comment on attachment 634550 [details] [diff] [review]
Patch

>@@ -444,16 +372,25 @@ nsAnimationManager::CheckAnimationRule(n

>+      if (ea->CanPerformOnCompositorThread()) {
>+        // We must invalidate to ensure that we actually flush layout,
>+        // since we are not interpolating on the main thread.
>+        nsIFrame* frame = mPresContext->GetRootPresContext()->PresShell()->GetRootFrame();
>+        frame->InvalidateWithFlags(frame->GetRect(), nsIFrame::INVALIDATE_NO_THEBES_LAYERS);
>+        mPresContext->Document()->SetNeedLayoutFlush();
>+      }

So this InvalidateWithFlags call seems pretty bogus to me since at this point frame->GetRect() isn't up-to-date, and in fact may well be 0,0,0,0 if we haven't yet performed any Reflow.  Is there some better way to do what this needs to do that doesn't involve bogus parameters?  roc?

I'm also puzzled by the need to call SetNeedLayoutFlush directly, since I thought a more limited set of things would need to call that.  Maybe bz knows?
Comment 48 Boris Zbarsky [:bz] 2012-06-20 13:52:14 PDT
SetNeedLayoutFlush() needs to be called any time some state that's only flushed by Flush_Layout is marked as dirty.

What state is that in this case?
Comment 49 Boris Zbarsky [:bz] 2012-06-20 13:52:53 PDT
Also, why are we invalidating the _viewport_ there, exactly?
Comment 50 David Zbarsky (:dzbarsky) 2012-06-20 14:01:31 PDT
I just want a way to make sure that we rebuild the layer tree.
Comment 51 Boris Zbarsky [:bz] 2012-06-20 14:58:49 PDT
OK, fine (though that could use a comment).  Why do you need to flag the document as needing a layout flush?  What's that layout flush triggering, and why does it need to be triggered via document flushes?
Comment 52 David Zbarsky (:dzbarsky) 2012-06-20 15:12:38 PDT
I think without the document part I couldn't get a layout flush to actually happen, but I can check again.  So the only purpose of the layout flush is to rebuild the layer tree.  Is there a better way to do what I'm trying to do here?
Comment 53 Boris Zbarsky [:bz] 2012-06-20 15:33:47 PDT
The only thing the document part does is that if script flushes layout it'll get propagated to the presshell, basically.

What's the callstack from a layout flush to the layer tree rebuild?  If you tell me that, I can probably come up with some ideas for how to do this better.
Comment 54 David Zbarsky (:dzbarsky) 2012-06-20 19:13:41 PDT
(In reply to David Baron [:dbaron] from comment #47)
> Comment on attachment 634550 [details] [diff] [review]
> Patch
> 
> 
> So this InvalidateWithFlags call seems pretty bogus to me since at this
> point frame->GetRect() isn't up-to-date, and in fact may well be 0,0,0,0 if
> we haven't yet performed any Reflow.  Is there some better way to do what
> this needs to do that doesn't involve bogus parameters?  roc?

Still need an answer to this.

> I'm also puzzled by the need to call SetNeedLayoutFlush directly, since I
> thought a more limited set of things would need to call that.  Maybe bz
> knows?

Turns out the layout flush isn't needed, I took it out.
Comment 55 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-06-22 16:22:43 PDT
Comment on attachment 634550 [details] [diff] [review]
Patch

So the conversion done by AddTransformFunctions means that we're
inconsistent about animating depending on which path we take, because
the old code hasn't been updated to the new rules in the spec on
compatible functions (rather than identical ones).  This probably
requires that we do that updating before landing.



One broader comment about our previous discussions:  when you're trying
something out to see if it might make something work (rather than
because you believe it's the correct thing to do), you should mark it as
such in the patch, with a question or comment (preferably beginning with
"REVIEW:" or "FIXME:").  Don't just guess and leave it unmarked in the
patch if you're not sure, because when you do I have to review assuming
that you don't know whether any of the code is correct.  If you're
marking things you're unsure about then I can focus on those areas and
assume that there's at least one person (you) who already thinks the
other code is correct.


This needs tests.  Lots of them.  Since the current transitions and
animations tests don't test this since they're looking at computed
style.  And we really do need tests like the transforms tests in
layout/style/test/test_transitions_per_property.html that test all sorts
of combinations since there are things in here that could break one
transform function while leaving the rest working fine.


If you can't share it, you should update the copied logic to include
fixes to bug 765218 and bug 655920.


>+    NS_ASSERTION(currElem.GetUnit() == eCSSUnit_Function,
>+         "Stream should consist solely of functions!");
>+    NS_ASSERTION(currElem.GetArrayValue()->Count() >= 1,
>+         "Incoming function is too short!");

The second line should line up with "curr" in the first line.

Also, I don't think the second assertion is useful; it's weaker than the
assertions from all the calls to Item().

>+    switch(nsStyleTransformMatrix::TransformFunctionOf(array)) {

s/switch(/switch (/

AddTransformFunctions needs to handle translate() having only 1
parameter.  (Or, alternatively we could make
nsStyleAnimation::ExtractComputedValue normalize for this case and for
scale() as well.)

AddTransformFunctions got matrix and matrix3d backwards

>+AddTransformAnimations(ElementAnimations* ea, Layer* aLayer, const nsPoint& aOrigin) {

Opening brace on its own line.  And actually the third parameter should
be on a new line too, since otherwise it's an 80th column violation.

AddTransformAnimations incorrectly assumes that the iteration count is
an integer.  It's a float.

I also recommend against using variables call i, j, and k here.  You'll
end up using the wrong one at some point.  I'd suggest animIdx, propIdx,
segIdx, or similar.

You should either handle step functions or fail when you encounter one
rather than assuming they won't occur.

The structure of:
>+      InfallibleTArray<AnimationSegment> segments;
>+
>+      if (property->mProperty == eCSSProperty_transform) {
>+        for (PRUint32 k = 0; k < property->mSegments.Length(); k++) {
>           ...
>+        }
>+      }
>+
>+      if (segments.Length() == 0) {
>+        continue;
>+      }
seems pretty odd.  Instead, how about just:
>+      if (property->mProperty != eCSSProperty_transform) {
>+        continue;
>+      }
>+
>+      InfallibleTArray<AnimationSegment> segments;
>+      for (PRUint32 k = 0; k < property->mSegments.Length(); k++) {
>         ...
>+      }

AddOpacityAnimations:  many of the comments on AddTransformAnimations
apply here too.

>+        animations = static_cast<ElementAnimations*>(content->GetProperty(nsGkAtoms::animationsProperty));

You do this a lot.  Could you add a static method to nsAnimationManager
to do it rather than copying the code?


>-static
>-gfxPoint3D GetDeltaToMozTransformOrigin(const nsIFrame* aFrame,
>+gfxPoint3D
>+nsDisplayTransform::GetDeltaToMozTransformOrigin(const nsIFrame* aFrame,

Could you mark this as:

/* static */ gfxPoint3D
nsDisplayTransform::...

(two functions like this)


Could nsDisplayTransform::GetResultingTransformMatrix be refactored
into one function with fewer params (but including an nsIFrame*) that
calls another with more params (but not an nsIFrame*)?  That might
be simpler.  (And no new default parameters, please.)



Why do you need to override nsIFrame::IsTransformed?  Your current
change is clearly incorrect since it will return true when opacity is
being animated.  Is there a better way we can handle
transforms-from-none?  (And is that the only issue?)

The inaccuracy of nsIFrame::HasOpacity also seems like a problem,
although only for performance of animating transforms rather than
correctness.


I don't understand the RenderFrameParent change at all; you'll need
someone else to review that.

The preference checks in
CommonElementAnimationData::CanPerformOnCompositorThread are probably
too slow and will need to be cached.  Please don't comment blocks of
code with /* */ (use #if 0).  And why do you disable off-main-thread
animation of opacity when there's an SVG transform?  Why not just
disable animations of transform?

ComputedTimingFunction::GetFunction() should have assertcions about
mType, since mTimingFunction is only valid for some types.


CommonElementAnimationData intentionally did not have virtual functions.
I'd prefer if you didn't add any, and I don't see why you need any here.
(I don't see a problem with adding a CanPerformOnCompositorThread method
to both ElementAnimations and ElementTransitions eventually, though I
on't think you need the one on transitions in this patch.)

It would really have been better if the "move things to header files"
bits were in a separate patch in a patch queue.  I'm basically ignoring
things that I think seem likely to be unchanged in this pass of review,
but that means I'm probably missing things.
(And I particularly don't see why you're moving transitions stuff as part
of this patch.)



>-static already_AddRefed<nsCSSValue::Array>
>-AppendTransformFunction(nsCSSKeyword aTransformFunction,
>+already_AddRefed<nsCSSValue::Array>
>+nsStyleAnimation::AppendTransformFunction(nsCSSKeyword aTransformFunction,
>                         nsCSSValueList**& aListTail)

Same note about "/* static */".  Also fix the indent.





What makes us recompute style when the off-main-thread animation
finishes?  We do need to.

And what ensures that we have up-to-date style when there's a style
flush that's needed for script (which we might need to distinguish from
those for the refresh driver), given the shouldInterpolate checks that
you've added?
Comment 56 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-06-22 16:32:56 PDT
(In reply to David Baron [:dbaron] from comment #55)
> And what ensures that we have up-to-date style when there's a style
> flush that's needed for script (which we might need to distinguish from
> those for the refresh driver), given the shouldInterpolate checks that
> you've added?

One possibility here is that we update the style as the refresh driver ticks (instead of suppressing the style update on each tick), but instead suppress the handling of the dynamic changes.
Comment 57 David Zbarsky (:dzbarsky) 2012-06-25 06:01:39 PDT
(In reply to David Baron [:dbaron] from comment #55)
> Comment on attachment 634550 [details] [diff] [review]
> Patch
> 
> So the conversion done by AddTransformFunctions means that we're
> inconsistent about animating depending on which path we take, because
> the old code hasn't been updated to the new rules in the spec on
> compatible functions (rather than identical ones).  This probably
> requires that we do that updating before landing.

Ok, will file a bug and ask you for review.

> 
> This needs tests.  Lots of them.  Since the current transitions and
> animations tests don't test this since they're looking at computed
> style.  And we really do need tests like the transforms tests in
> layout/style/test/test_transitions_per_property.html that test all sorts
> of combinations since there are things in here that could break one
> transform function while leaving the rest working fine.

Chris and I had an idea: if we use step functions, the same frame will be displayed for a long time.  We can then use the conversion code I have written to ship the interpolated CSSValueList back from the compositor to make sure it is correct.

> 
> If you can't share it, you should update the copied logic to include
> fixes to bug 765218 and bug 655920.

I have a patch that shares it.


> AddTransformAnimations incorrectly assumes that the iteration count is
> an integer.  It's a float.

Already have a patch fixing it.

> You should either handle step functions or fail when you encounter one
> rather than assuming they won't occur.

Already have a patch fixing this.

> Could nsDisplayTransform::GetResultingTransformMatrix be refactored
> into one function with fewer params (but including an nsIFrame*) that
> calls another with more params (but not an nsIFrame*)?  That might
> be simpler.  (And no new default parameters, please.)
> 

Yeah, that sounds cleaner than what I did

> 
> Why do you need to override nsIFrame::IsTransformed?  Your current
> change is clearly incorrect since it will return true when opacity is
> being animated.  Is there a better way we can handle
> transforms-from-none?  (And is that the only issue?)

This change ensures that we will create an nsDisplayTransform for the frame.  Without this change, when we have a transform from none with an animationdelay, there is no transform in the nsStyleContext at the beginning, so we don't end up with a containerlayer, and no animations occurs.

> 
> I don't understand the RenderFrameParent change at all; you'll need
> someone else to review that.

Ok, i'll ask cjones.

> 
> ComputedTimingFunction::GetFunction() should have assertcions about
> mType, since mTimingFunction is only valid for some types.

I want to define nsTimingFunction in ipdl, to reduce some of the conversions going on.  That should get rid of this.

> 
> It would really have been better if the "move things to header files"
> bits were in a separate patch in a patch queue.  I'm basically ignoring
> things that I think seem likely to be unchanged in this pass of review,
> but that means I'm probably missing things.
> (And I particularly don't see why you're moving transitions stuff as part
> of this patch.)

I will do that for the next version, and undo the transitions changes.
Comment 58 David Zbarsky (:dzbarsky) 2012-06-25 08:38:22 PDT
(In reply to David Baron [:dbaron] from comment #55)
 
> Could nsDisplayTransform::GetResultingTransformMatrix be refactored
> into one function with fewer params (but including an nsIFrame*) that
> calls another with more params (but not an nsIFrame*)?  That might
> be simpler.  (And no new default parameters, please.)
> 

Actually, I don't think it will be cleaner, now that I'm looking at it some more.  The issue is that we have to check for SVG transforms when we have a frame, but we know that those can't happen on the compositor.

So if we have 2 versions of the function, one that takes a frame and one that takes a bunch of params, we would have to pass in the 2 svg matrices, as well as the style context and parent context, which would bring the total params to over 10.
Comment 59 Boris Zbarsky [:bz] 2012-06-25 22:23:32 PDT
Comment on attachment 634550 [details] [diff] [review]
Patch

Since David is reviewing this...
Comment 60 David Zbarsky (:dzbarsky) 2012-06-26 04:55:36 PDT
(In reply to David Baron [:dbaron] from comment #56)
> (In reply to David Baron [:dbaron] from comment #55)
> > And what ensures that we have up-to-date style when there's a style
> > flush that's needed for script (which we might need to distinguish from
> > those for the refresh driver), given the shouldInterpolate checks that
> > you've added?
> 
> One possibility here is that we update the style as the refresh driver ticks
> (instead of suppressing the style update on each tick), but instead suppress
> the handling of the dynamic changes.

I'm not quite sure what you mean by this.
Comment 61 David Zbarsky (:dzbarsky) 2012-06-26 05:16:04 PDT
Created attachment 636666 [details] [diff] [review]
Move ElementAnimations to the header.
Comment 62 David Zbarsky (:dzbarsky) 2012-06-26 05:17:16 PDT
Created attachment 636667 [details] [diff] [review]
Patch
Comment 63 David Zbarsky (:dzbarsky) 2012-06-26 05:17:58 PDT
Comment on attachment 636667 [details] [diff] [review]
Patch

Cjones, can you review the LocalOpacity stuff?
Comment 64 David Zbarsky (:dzbarsky) 2012-06-26 05:18:48 PDT
Comment on attachment 636667 [details] [diff] [review]
Patch

Roc, can you look at the invalidation in nsAnimationManager and anything else you want to review?
Comment 65 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-06-26 20:52:17 PDT
I think I review everything except the style system changes.
Comment 66 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-06-26 21:08:25 PDT
Comment on attachment 636667 [details] [diff] [review]
Patch

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

::: layout/base/FrameLayerBuilder.cpp
@@ +1943,5 @@
> +      ElementAnimations* animations = nsnull;
> +      if (content) {
> +        animations = nsAnimationManager::GetAnimations(content);
> +      }
> +      if (animations && animations->CanPerformOnCompositorThread()) {

So if some of the animations can't be performed on the compositor thread, we perform none of them on the compositor thread? That seems suboptimal, why are we doing that?

@@ +1949,5 @@
> +          ownLayer->ClearAnimations();
> +          AddOpacityAnimations(animations, ownLayer);
> +        } else if (item->GetType() == nsDisplayItem::TYPE_TRANSFORM) {
> +          ownLayer->ClearAnimations();
> +          AddTransformAnimations(animations, ownLayer, item->ToReferenceFrame());

What if opacity or transform aren't actually animated?

I think it would make more sense if nsDisplayOpacity::BuildLayer and nsDisplayTransform::BuildLayer added the animations.

@@ +1959,4 @@
>          // The layer's current transform is applied first, then the result is scaled.
>          gfx3DMatrix transform = ownLayer->GetTransform()*
>              gfx3DMatrix::ScalingMatrix(mParameters.mXScale, mParameters.mYScale, 1.0f);
>          ownLayer->SetTransform(transform);

This needs to happen even if you have an animated transform above; the extra scale transform still needs to be added. Currently we'd only set an animated transform on a container layer, so you could assert here that there are simply no animations on ownLayer.

::: layout/base/nsDisplayList.cpp
@@ +2617,1 @@
>                                            float aAppUnitsPerPixel)

Fix indent

@@ +2682,5 @@
>                                                  const nsRect* aBoundsOverride,
> +                                                const nsCSSValueList* aTransformOverride,
> +                                                gfxPoint3D* aToMozOrigin,
> +                                                gfxPoint3D* aToPerspectiveOrigin,
> +                                                nscoord* aChildPerspective,

Maybe you should just duplicate this function rather than giving it two different modes?

@@ +2735,2 @@
>                    "If we don't have a transform, then we must have another reason to have an nsDisplayTransform created");
> +  */}

Why are you taking this out?

::: layout/generic/nsFrame.cpp
@@ +939,2 @@
>            (GetStyleDisplay()->HasTransform() ||
> +           IsSVGTransformed())) || hasOMTATransform;

IsTransformed is quite hot (and HasOpacity will be too). I think we should probably reserve a frame state bit to mark frames whose elements have animations, i.e., for which nsAnimationManager::GetAnimations may return non-null.

::: layout/style/nsAnimationManager.cpp
@@ +160,5 @@
> +        aAnimation->mLastNotification !=
> +          ElementAnimation::LAST_NOTIFICATION_END) {
> +      aAnimation->mLastNotification = ElementAnimation::LAST_NOTIFICATION_END;
> +    if (aEa->CanPerformOnCompositorThread()) {
> +      // We must invalidate because this animations was done on the compositor,

"this animation"
Comment 67 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-06-27 02:58:24 PDT
Comment on attachment 636667 [details] [diff] [review]
Patch

r+ on SetShadowOpacity() usage.
Comment 68 David Zbarsky (:dzbarsky) 2012-06-27 08:17:13 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #66)

> So if some of the animations can't be performed on the compositor thread, we
> perform none of them on the compositor thread? That seems suboptimal, why
> are we doing that?

It would be weird to have an animation be out of sync, which could happen if the gecko thread can't keep up with the compositor.
 

> What if opacity or transform aren't actually animated?
> 
> I think it would make more sense if nsDisplayOpacity::BuildLayer and
> nsDisplayTransform::BuildLayer added the animations.

It won't actually matter because no animations will be added, but I think moving this into BuildLayer is needed for transitions, so I will do it.
 
> This needs to happen even if you have an animated transform above; the extra
> scale transform still needs to be added. Currently we'd only set an animated
> transform on a container layer, so you could assert here that there are
> simply no animations on ownLayer.

I'm not sure I follow, but if I set the scale transform, I end up double scaling things.

> Maybe you should just duplicate this function rather than giving it two
> different modes?

Dbaron, what do you think of this?
Comment 69 David Zbarsky (:dzbarsky) 2012-06-27 08:20:37 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #66)
> 
> ::: layout/base/nsDisplayList.cpp
> @@ +2617,1 @@
> >                                            float aAppUnitsPerPixel)
> 
> Fix indent
> 

This patch is -w because I have a lot of lines that trim trailing whitespace.
Comment 70 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-06-27 15:04:16 PDT
(In reply to David Zbarsky from comment #68)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #66)
> > So if some of the animations can't be performed on the compositor thread, we
> > perform none of them on the compositor thread? That seems suboptimal, why
> > are we doing that?
> 
> It would be weird to have an animation be out of sync, which could happen if
> the gecko thread can't keep up with the compositor.

It depends on what we're animating. I think in most cases it wouldn't be weird. For example if we're animating the color of a gradient and a transform, I don't think anyone could tell that they were "out of sync". I suspect the only cases where you could tell they're out of sync is when transforms are animated along with other geometry properties (e.g. 'left' or 'width'), but, well, don't do that.

OTOH I think it would be unfortunate for animating the gradient color to make the transform animation jerky.

> I'm not sure I follow, but if I set the scale transform, I end up double
> scaling things.

You should only apply the scale to non-container layers, like the current code already does. Since currently animations are only applied to container layers, I don't think we should end up double-scaling things...
Comment 71 David Zbarsky (:dzbarsky) 2012-06-28 04:53:36 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #70)
> It depends on what we're animating. I think in most cases it wouldn't be
> weird. For example if we're animating the color of a gradient and a
> transform, I don't think anyone could tell that they were "out of sync". I
> suspect the only cases where you could tell they're out of sync is when
> transforms are animated along with other geometry properties (e.g. 'left' or
> 'width'), but, well, don't do that.

For something like color and transform, this would make sense.  However, imagine somebody decides to animate transform and transform-origin.  With the current patch, we will just do both on the main thread.  With your suggestion, we will animate transform with the original transform-origin on the compositor and animate transform-origin on the main thread.  I don't think this will do the right thing.

> OTOH I think it would be unfortunate for animating the gradient color to
> make the transform animation jerky.
> 

> You should only apply the scale to non-container layers, like the current
> code already does. Since currently animations are only applied to container
> layers, I don't think we should end up double-scaling things...

If we scale the container and also scale the actual layer, that won't double scale the layer?
Comment 72 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-06-28 06:29:22 PDT
(In reply to David Zbarsky from comment #71)
> For something like color and transform, this would make sense.  However,
> imagine somebody decides to animate transform and transform-origin.  With
> the current patch, we will just do both on the main thread.  With your
> suggestion, we will animate transform with the original transform-origin on
> the compositor and animate transform-origin on the main thread.  I don't
> think this will do the right thing.

OK. Then you can do something intelligent in nsAnimationManager to determine whether it should offload an animation or not, based on which properties are being animated. Either way, it would be good to have nsAnimationManager export an API like GetAnimationForCompositor(element, property) which encapsulates all these decisions, looking at the current set of animations for the element and deciding which ones are eligible to run on the compositor thread.

> > You should only apply the scale to non-container layers, like the current
> > code already does. Since currently animations are only applied to container
> > layers, I don't think we should end up double-scaling things...
> 
> If we scale the container and also scale the actual layer, that won't double
> scale the layer?

No, FrameLayerBuilder won't apply ScalingMatrix(mParameters.mXScale, mParameters.mYScale) to both a container and its children --- not with the same scale factors, at least. If it did this would already be a bug.

By the way, you may need to do something in ChooseScaleAndSetTransform when you are animating the transform. That function tries to turn something like transform:scale(2) on an element into doubling the size of all its child layers (and rendering ThebesLayers with higher resolution). Instead of the SetTransform call in that function, when you're offloading transform animation to the compositor you'll need to add the gfx3DMatrix::ScalingMatrix(1.0/scale.width, 1.0/scale.height, 1.0) to the animated transform.
Comment 73 David Zbarsky (:dzbarsky) 2012-07-05 23:39:58 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #72)

> OK. Then you can do something intelligent in nsAnimationManager to determine
> whether it should offload an animation or not, based on which properties are
> being animated. Either way, it would be good to have nsAnimationManager
> export an API like GetAnimationForCompositor(element, property) which
> encapsulates all these decisions, looking at the current set of animations
> for the element and deciding which ones are eligible to run on the
> compositor thread.

I plan to work on animating transform-origin, perspective, and perspective-origin soon, so it may be easier to just do what I'm doing right now for the initial landing.
Once those are implemented, we can decide which property combinations can or can't be done async.
Comment 74 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-07-06 01:32:06 PDT
It seems like very little work to encapsulate the code you're currently writing inline at each call site into a single GetAnimationForCompositor method. So why not do that now? For now, it's OK if that method has the all-or-nothing behaviour, but I don't want that logic sprayed through all the callsites each with its own if (animations->CanPerformOnCompositorThread()) test.
Comment 75 David Zbarsky (:dzbarsky) 2012-07-09 15:18:30 PDT
Created attachment 640382 [details] [diff] [review]
Move ElementAnimations to the header.
Comment 76 David Zbarsky (:dzbarsky) 2012-07-09 15:19:05 PDT
Created attachment 640383 [details] [diff] [review]
Layout patch
Comment 77 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-07-09 21:19:03 PDT
Comment on attachment 640383 [details] [diff] [review]
Layout patch

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

::: content/base/src/nsINode.cpp
@@ +160,5 @@
>                                                                    aOldValue);
>    if (NS_SUCCEEDED(rv)) {
>      SetFlags(NODE_HAS_PROPERTIES);
> +    if (aPropertyName == nsGkAtoms::animationsProperty)
> +      SetMayHaveAnimations();

{}

::: layout/base/FrameLayerBuilder.cpp
@@ +1833,5 @@
> +                       const nsPoint& aOrigin)
> +{
> +  if (!ea)
> +    return;
> +  printf("\n adding animations\n");

remove

@@ +1901,5 @@
> +  }
> +}
> +
> +static void
> +AddOpacityAnimations(ElementAnimations* ea, Layer* aLayer) {

{ on new line

@@ +1908,5 @@
> +  NS_ASSERTION(aLayer->AsContainerLayer(), "Should only animate ContainerLayer");
> +  for (PRUint32 animIdx = 0; animIdx < ea->mAnimations.Length(); animIdx++) {
> +    ElementAnimation* anim = &ea->mAnimations[animIdx];
> +    float iterations = anim->mIterationCount != NS_IEEEPositiveInfinity()
> +                         ? anim->mIterationCount : -1;

Add a helper function that does this. Currently you do it from two places.

@@ +1914,5 @@
> +      continue;
> +    }
> +    for (PRUint32 propIdx = 0; propIdx < anim->mProperties.Length(); propIdx++) {
> +      AnimationProperty* property = &anim->mProperties[propIdx];
> +      InfallibleTArray<AnimationSegment> segments;

Why is this inner loop needed? There can only be one property, 'opacity', right?

@@ +2023,5 @@
> +      if (item->GetType() == nsDisplayItem::TYPE_OPACITY) {
> +        ElementAnimations* ea =
> +          nsAnimationManager::GetAnimationsForCompositor(content, eCSSProperty_opacity);
> +        AddOpacityAnimations(ea, ownLayer);
> +      } else if (item->GetType() == nsDisplayItem::TYPE_TRANSFORM) {

Use 'type' instead of item->GetType().

But why can't adding these animations be done in item->BuildLayer()?

::: layout/base/nsChangeHint.h
@@ +105,5 @@
> +   * An element is being animated by the compositor, so when we recompute style
> +   * on the main thread, we do not want to invalidate the frame.  The compositor
> +   * will take care of making sure the element is display correctly.
> +   */
> +  nsChangeHint_NoRepaint = 0x4000,

I'm not comfortable with this hint. Currently we don't have negative hints and we often combine hints with binary-OR. Negative hints don't make sense that way. Can we avoid negative hints?

::: layout/base/nsDisplayList.cpp
@@ +1946,5 @@
>      return LAYER_ACTIVE;
> +  if (mFrame->GetContent()) {
> +    ElementAnimations* ea = nsAnimationManager::GetAnimations(mFrame->GetContent());
> +    if (ea && ea->CanPerformOnCompositorThread())
> +      return LAYER_ACTIVE;

Shouldn't we be calling GetAnimationsForCompositor here?

@@ +2800,2 @@
>                    "If we don't have a transform, then we must have another reason to have an nsDisplayTransform created");
> +  */}

Just remove the assertion

@@ +2926,5 @@
>      return LAYER_ACTIVE;
> +  if (mFrame->GetContent()) {
> +    ElementAnimations* ea = nsAnimationManager::GetAnimations(mFrame->GetContent());
> +    if (ea && ea->CanPerformOnCompositorThread())
> +      return LAYER_ACTIVE;

Shouldn't we be calling GetAnimationsForCompositor here?

::: layout/base/nsIPresShell.h
@@ +488,5 @@
>  
>    void PostRecreateFramesFor(mozilla::dom::Element* aElement);
>    void RestyleForAnimation(mozilla::dom::Element* aElement,
> +                           nsRestyleHint aRestyleHint,
> +                           nsChangeHint aChangeHint);

What does aChangeHint mean here? Need to document.

::: layout/style/nsAnimationManager.h
@@ +126,5 @@
> +  // This function returns -1 for the position if the animation should not be
> +  // run (because it is not currently active and has no fill behavior.)
> +  static double GetPositionInIteration(mozilla::TimeStamp aStartTime,
> +                                       mozilla::TimeStamp aCurrentTime,
> +                                       mozilla::TimeDuration aDuration,

Add
  typedef mozilla::TimeStamp TimeStamp;
etc to the nsAnimationManager class to make these prefixes go away

@@ +196,4 @@
>  
> +  static ElementAnimations*
> +    GetAnimationsForCompositor(nsIContent* aContent,
> +                               nsCSSProperty aProperty)

This looks good, but put the function name on the same line as the result type.
Comment 78 David Zbarsky (:dzbarsky) 2012-07-11 01:25:49 PDT
In retrospect, I probably should have been a little more careful as to which changes ended up in this patch and which in bug 768440.  The way I ended up doing things, I moved the AddOpacityAnimations/AddTransformAnimations stuff to nsDisplayList.cpp in this patch, and then combining everything into AddAnimationsForProperty in the patch for bug 768440.
Comment 79 David Zbarsky (:dzbarsky) 2012-07-16 18:34:36 PDT
Created attachment 642823 [details] [diff] [review]
Patch
Comment 80 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-07-16 21:08:01 PDT
Comment on attachment 642823 [details] [diff] [review]
Patch

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

There are a lot of patch hunks with no significant changes, just whitespace changes or something else I can't identify. Please remove them --- that'll make the patch much easier to review.

Folding this together with the patch in bug 768440 might make sense. Up to you.

::: layout/base/FrameLayerBuilder.cpp
@@ +2036,3 @@
>    aLayer->SetTransform(transform);
> +  // Store the inverse of our resolution-scale on the layer
> +  aLayer->SetScale(1.0f/float(scale.width), 1.0/float(scale.height));

1.0f

::: layout/base/nsDisplayList.cpp
@@ +162,5 @@
> +        // translate(x) is shorthand for translate(x, 0)
> +        double y = 0;
> +        if (array->Count() == 3) {
> +           y = nsStyleTransformMatrix::ProcessTranslatePart(
> +            array->Item(1), aContext, aPresContext, dummy,

Shouldn't you be checking Count() == 2 and accessing array->Item(2)?

@@ +178,5 @@
> +          array->Item(1), aContext, aPresContext, dummy,
> +          aBounds.Height(), aAppUnitsPerPixel);
> +        double z = nsStyleTransformMatrix::ProcessTranslatePart(
> +          array->Item(1), aContext, aPresContext, dummy,
> +          0, aAppUnitsPerPixel);

Shouldn't you be accessing items 1, 2 and 3?

@@ +240,5 @@
> +        aFunctions.AppendElement(TransformMatrix(matrix));
> +        break;
> +      }
> +      default:
> +        printf("\nFunction not handled yet!\n");

NS_ERROR

@@ +282,5 @@
> +  }
> +
> +  for (PRUint32 animIdx = 0; animIdx < ea->mAnimations.Length(); animIdx++) {
> +    ElementAnimation* anim = &ea->mAnimations[animIdx];
> +    if (!anim->CanPerformOnCompositor(ea->mElement, TimeStamp::Now())) {

Hoist TimeStamp::Now() call outside the loop. Actually, this should probably be a value retrieved from the refresh driver. Having this value change under us while processing can't be a good idea.

@@ +307,5 @@
> +        list = segment->mToValue.GetCSSValueListValue();
> +        InfallibleTArray<TransformFunction> toFunctions;
> +        AddTransformFunctions(list, frame->GetStyleContext(),
> +                              frame->PresContext(), bounds,
> +                              scale, toFunctions);

Hoist GetStyleContext() and PresContext() calls outside the loop.

@@ +339,5 @@
> +  for (PRUint32 animIdx = 0; animIdx < ea->mAnimations.Length(); animIdx++) {
> +    ElementAnimation* anim = &ea->mAnimations[animIdx];
> +    float iterations = anim->mIterationCount != NS_IEEEPositiveInfinity()
> +                         ? anim->mIterationCount : -1;
> +    if (!anim->CanPerformOnCompositor(ea->mElement, TimeStamp::Now())) {

As above, don't call TimeStamp::Now().

@@ +2256,5 @@
> +                                                   eCSSProperty_opacity);
> +  AddOpacityAnimations(ea, container);
> +
> +
> +  return container.forget();

Remove extra blank line.

::: layout/style/AnimationCommon.cpp
@@ +224,5 @@
> +  // testcases.
> +  /*if (frame && !frame->AreLayersMarkedActive()) {
> +    printf("\n running into inactive layers....\n");
> +    return false;
> +  }*/

remove commented-out code
Comment 81 David Zbarsky (:dzbarsky) 2012-07-17 07:00:49 PDT
Created attachment 642935 [details] [diff] [review]
Patch -w
Comment 82 David Zbarsky (:dzbarsky) 2012-07-17 07:05:12 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #80)

> Folding this together with the patch in bug 768440 might make sense. Up to
> you.

Dbaron says it will be harder for him to review a folded patch.


> Shouldn't you be checking Count() == 2 and accessing array->Item(2)?

Count() == 3 is correct because Item(0) is the function, and Item(1) and Item(2) are the arguments.
 

> Hoist TimeStamp::Now() call outside the loop. Actually, this should probably
> be a value retrieved from the refresh driver. Having this value change under
> us while processing can't be a good idea.

I actually don't think it matters too much because if we have a refresh tick and the time changes during the tick so we fail to start an animation, we will start it on the next tick.  In any case, I will change this in the transitions patch.


> Hoist GetStyleContext() and PresContext() calls outside the loop.

Done in transitions.
 
> @@ +339,5 @@
> > +  for (PRUint32 animIdx = 0; animIdx < ea->mAnimations.Length(); animIdx++) {
> > +    ElementAnimation* anim = &ea->mAnimations[animIdx];
> > +    float iterations = anim->mIterationCount != NS_IEEEPositiveInfinity()
> > +                         ? anim->mIterationCount : -1;
> > +    if (!anim->CanPerformOnCompositor(ea->mElement, TimeStamp::Now())) {
> 
> As above, don't call TimeStamp::Now().

Will do in transitions.
Comment 83 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-07-17 08:27:20 PDT
Did you actually change anything in that patch or did you upload the old patch by mistake?
Comment 84 David Zbarsky (:dzbarsky) 2012-07-17 08:41:55 PDT
I uploaded the previous patch without whitespace changes.  I have changed the things I didn't mention in my previous comment in my repository.  I can post a diff of those changes as well if you want.
Comment 85 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-07-17 09:48:00 PDT
No, it's OK.
Comment 86 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-07-17 09:48:06 PDT
Comment on attachment 642935 [details] [diff] [review]
Patch -w

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

::: layout/base/FrameLayerBuilder.cpp
@@ +2036,3 @@
>    aLayer->SetTransform(transform);
> +  // Store the inverse of our resolution-scale on the layer
> +  aLayer->SetScale(1.0f/float(scale.width), 1.0/float(scale.height));

1.0f
Comment 87 David Zbarsky (:dzbarsky) 2012-07-18 20:59:08 PDT
Created attachment 643717 [details] [diff] [review]
Patch to add support for perspective transform function
Comment 88 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-07-18 22:05:27 PDT
Comment on attachment 643717 [details] [diff] [review]
Patch to add support for perspective transform function

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

Looks OK, but isn't there additional code needed to fully hook this up?
Comment 89 David Zbarsky (:dzbarsky) 2012-07-18 22:15:07 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #88)
> Comment on attachment 643717 [details] [diff] [review]
> Patch to add support for perspective transform function
> 
> Review of attachment 643717 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks OK, but isn't there additional code needed to fully hook this up?

Nope, once we have the perspective function in the nsCSSValueList, nsStyleAnimation handles the rest for us :)
Comment 90 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-07-19 19:39:01 PDT
Comment on attachment 640382 [details] [diff] [review]
Move ElementAnimations to the header.

>From: David Zbarsky <dzbarsky@gmail.com>
>

This needs a commit message.  How about:

Move ElementAnimations, ElementAnimation, AnimationProperty, and AnimationPropertySegment classes to the header file.  (Bug 755084)  r=dbaron

>-  ElementAnimations(dom::Element *aElement, nsIAtom *aElementProperty,
>+ElementAnimations::ElementAnimations(mozilla::dom::Element *aElement, nsIAtom *aElementProperty,
>                      nsAnimationManager *aAnimationManager)
>     : CommonElementAnimationData(aElement, aElementProperty,
>                                  aAnimationManager),
>       mNeedsRefreshes(true)
>   {
>   }

Please reindent (i.e., two fewer spaces) the rest of the lines of
this function as well.

>-namespace mozilla {
>-namespace css {
>-class Declaration;
>-}
>-}

Please leave this at the top of the header file (since it's a forward
declaration) rather than moving it down.

Why are you moving AnimationEventInfo to not be a nested class anymore?
I'd rather you leave it as a nested class inside nsAnimationManager.
(Also, if it's not a nested class, it should be un-indented.)  This should
be easy enough by leaving the forward declarations:
>-struct AnimationPropertySegment;
>-struct ElementAnimation;
>-struct ElementAnimations;
before nsAnimationManager is defined, and putting the definitions that
you've moved to the header file *after* nsAnimationManager.  You'll
need to move the typedef back inside nsAnimationManager as well.

r=dbaron with that
Comment 91 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-07-19 20:58:20 PDT
Comment on attachment 642935 [details] [diff] [review]
Patch -w

Functions should have their opening { on its own line throughout, (maybe
with the exception of layers or display list code, if that's local style
there -- but definitely in layout/style/).

nsINode.cpp:

The code you're adding to nsINode::SetProperty really just belongs in
nsAnimationManager::GetElementAnimations next to the SetProperty call;
there's no need to put it inside SetProperty since there's only a single
caller setting that property.  Further, I think you should be doing it
for any of the three animation properties so that it will also work for
animations on the :before or :after pseudo-elements unless there's a
reason not to do so that I'm missing based on a patch I haven't gotten
to yet.  Or maybe not... maybe your code just doesn't work on them,
but in that case you can just have the condition there.  But I think
you should at least file a followup bug if it doesn't work.  (I think
you'd need to tweak nsAnimationManager::GetAnimationsForCompositor
too.)

nsDisplayList.cpp:

>+    if (currElem.GetUnit() == eCSSUnit_None) {
>+      continue;
>+    }

I think you can move this out of the loop (and change currElem to aList
and continue to return), if not remove it entirely.  The assertion
should tell you which.

>+    bool dummy = false;

Could you call this canStoreInRuleTree and initialize to true?

>+      case eCSSKeyword_rotatez:
>+      {
>+        double theta = array->Item(1).GetAngleValueInRadians();
>+        aFunctions.AppendElement(RotationZ(theta));
>+        break;
>+      }
>+      case eCSSKeyword_rotate:
>+      {
>+        double theta = array->Item(1).GetAngleValueInRadians();
>+        aFunctions.AppendElement(Rotation(theta));
>+        break;
>+      }

Can you collapse these cases?  (Why are Rotation and RotationZ
different?)


>+  PRUint32 type = aCTF.GetType() == nsTimingFunction::StepStart ? 1 : 2;
>+  return TimingFunction(StepFunction(aCTF.GetSteps(), type));

Since the definition of TimingFunction::type (in attachment 642822 [details] [diff] [review]) says:
>  int type; //this is really nsTimingFunction::Type
I think you should just pass aCTF.GetType() directly, unless the comment
is wrong.


It would be nice if AddTransformAnimations, etc., did less array
copying.  In particular, it would help if the AnimationSegment
constructor didn't take the arrays as arguments, but instead let you
fill in the arrays after constructing it.  Doesn't need to be done now,
though.  Also constructing the Animation at the top and adding the
segments directly to it.

+      if (segments.Length() == 0) {
+        continue;
+      }

I don't think you need this (twice), since there should always be
segments.  Feel free to assert this if you depend on it.


>-    metrics.mCSSContentRect = gfx::Rect(nsPresContext::AppUnitsToFloatCSSPixels(contentBounds.x),
>+    metrics.mCSSContentRect =
>+      mozilla::gfx::Rect(nsPresContext::AppUnitsToFloatCSSPixels(contentBounds.x),

leave this alone (well, the reindenting is ok if it moves it from
over 80 to under 80, but don't add mozilla::)

>-    metrics.mCSSContentRect = gfx::Rect(nsPresContext::AppUnitsToFloatCSSPixels(contentBounds.x),
>+    metrics.mCSSContentRect =
>+      mozilla::gfx::Rect(nsPresContext::AppUnitsToFloatCSSPixels(contentBounds.x),

this too


I'm up to nsDisplayTransform::GetResultingTransformMatrix; will
resume tomorrow.
Comment 92 David Zbarsky (:dzbarsky) 2012-07-20 07:52:39 PDT
(In reply to David Baron [:dbaron] from comment #91)
> Comment on attachment 642935 [details] [diff] [review]
> Patch -w
> Further, I think you should be doing it
> for any of the three animation properties so that it will also work for
> animations on the :before or :after pseudo-elements unless there's a
> reason not to do so that I'm missing based on a patch I haven't gotten
> to yet.  Or maybe not... maybe your code just doesn't work on them,
> but in that case you can just have the condition there.  

I've tried it before, and things went very badly.  I will try it again at some point after everything else lands.  I've already filed bug 771367 on this issue.

> nsDisplayList.cpp:
> 
> >+    if (currElem.GetUnit() == eCSSUnit_None) {
> >+      continue;
> >+    }
> 
> I think you can move this out of the loop (and change currElem to aList
> and continue to return), if not remove it entirely. 

This is definitely needed to handle animations without a from or to rule, but I think it can be moved out of the loop.

> 
> >+      case eCSSKeyword_rotatez:
> >+      {
> >+        double theta = array->Item(1).GetAngleValueInRadians();
> >+        aFunctions.AppendElement(RotationZ(theta));
> >+        break;
> >+      }
> >+      case eCSSKeyword_rotate:
> >+      {
> >+        double theta = array->Item(1).GetAngleValueInRadians();
> >+        aFunctions.AppendElement(Rotation(theta));
> >+        break;
> >+      }
> Can you collapse these cases?  (Why are Rotation and RotationZ
> different?)

This is needed because rotate and rotateZ are different functions, according to the transforms spec.  Perhaps one should be a primitive of the other, or something.

> 
> >+  PRUint32 type = aCTF.GetType() == nsTimingFunction::StepStart ? 1 : 2;
> >+  return TimingFunction(StepFunction(aCTF.GetSteps(), type));
> 
> Since the definition of TimingFunction::type (in attachment 642822 [details] [diff] [review]
> [diff] [review]) says:
> >  int type; //this is really nsTimingFunction::Type
> I think you should just pass aCTF.GetType() directly, unless the comment
> is wrong.

Yeah, the comment is basically a lie.  You can't actually pass an enum value to an ipdl constructor that takes an int.  Any ideas for what the comment could say instead?  (Or I guess I can just explain that I'm converting back and forth).  In general, I want to convert nsTimingFunction to ipdl, which would fix this.

> 
> >-    metrics.mCSSContentRect = gfx::Rect(nsPresContext::AppUnitsToFloatCSSPixels(contentBounds.x),
> >+    metrics.mCSSContentRect =
> >+      mozilla::gfx::Rect(nsPresContext::AppUnitsToFloatCSSPixels(contentBounds.x),
> 
> leave this alone (well, the reindenting is ok if it moves it from
> over 80 to under 80, but don't add mozilla::)
> 
> >-    metrics.mCSSContentRect = gfx::Rect(nsPresContext::AppUnitsToFloatCSSPixels(contentBounds.x),
> >+    metrics.mCSSContentRect =
> >+      mozilla::gfx::Rect(nsPresContext::AppUnitsToFloatCSSPixels(contentBounds.x),
> 
> this too
> 

These were needed because clang(and perhaps other compilers) complains that the gfx namespace is ambiguous.
Comment 93 David Zbarsky (:dzbarsky) 2012-07-20 13:36:22 PDT
(In reply to David Baron [:dbaron] from comment #90)
> Comment on attachment 640382 [details] [diff] [review]

> 
> Please leave this at the top of the header file (since it's a forward
> declaration) rather than moving it down.
> 

That won't work because it is used in ElementAnimations.

> r=dbaron with that
Comment 94 David Zbarsky (:dzbarsky) 2012-07-20 13:42:58 PDT
Also, we need to create AnimationEventInfo in ElementAnimations::GetPositionInIteration.  Do you want me to qualify all of these with nsAnimationManager::EventArray, etc.
Comment 95 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-07-20 14:53:53 PDT
Comment on attachment 642935 [details] [diff] [review]
Patch -w

continued from comment 91

nsDisplayTransform::GetResultingTransformMatrix:

>+  NS_PRECONDITION(aFrame || (aToMozOrigin && aBoundsOverride && aTransformOverride), "Cannot get transform matrix for a null frame!");

Should you:
 * also have an assertion about aChildPerspective?
 * also have an assertion that you have only one or the other and not
   both?

>+  nscoord perspectiveCoord = 0.0;

Initialize to 0, since it's not a float.

nsIPresShell.h:

>   void RestyleForAnimation(mozilla::dom::Element* aElement,
>-                           nsRestyleHint aHint);
>+                           nsRestyleHint aRestyleHint);

leave this alone.  (You're not making the other changes that would
be consistent with it; better to just leave it.)

nsIFrame.cpp:

Probably better to structure IsTransformed (particularly) and also
HasOpacity as:

return ((mState & NS_FRAME_MAY_BE_TRANSFORMED) &&
        (GetStyleDisplay()->HasTransform() ||
         IsSVGTransformed() ||
         (mContent &&
          nsAnimationManager::GetAnimationsForCompositor(...))));

so you don't call GetAnimationsForCompositor until after you've
checked the bit.


Some of the display list construction changes may have some interaction
with jwatt's work on SVG display lists.


>-      nsDisplayTransform::GetResultingTransformMatrix(this, nsPoint(0, 0),
>-                                                      scaleFactor, nsnull, aOutAncestor);
>+      nsDisplayTransform::GetResultingTransformMatrix(this, nsPoint(0, 0), scaleFactor, nsnull, 
>+                                                      nsnull, nsnull, nsnull, nsnull, aOutAncestor);

You should wrap at less than 80 columns.


AnimationCommon.cpp:

fix the indentation of the two }s that should be indented 2 or 4 spaces
but aren't


AnimationCommon.h:

>+  const nsSMILKeySpline* const GetFunction() const {

The middle of the 3 "const"s doesn't do anything and should be removed.


>-  ~CommonElementAnimationData()
>+  virtual ~CommonElementAnimationData()

I really don't want this change, and I also don't think it's neeeded.
(You're not deleting anything through a CommonElementAnimationData*
anywhere, are you?)


>+  static bool
>+  CanAnimatePropertyOnCompositor(const dom::Element *aElement,
>+                                 nsCSSProperty aProperty);
>+
>   dom::Element *mElement;

It seems bad that dom::Element rather than mozilla::dom::Element works
in a .h file.  Is there a |using namespace mozilla| somewhere there
shouldn't be?

It seems bad that dom::Element rather than mozilla::dom::Element works
in a .h file.  Is there a |using namespace mozilla| somewhere there
shouldn't be?

nsAnimationManager.cpp:

>+    if (aEa->CanPerformOnCompositorThread()) {
>+      // We must invalidate because this animation was done on the compositor,
>+      // and we must now update for the final style.
>+      nsIFrame* frame = aEa->mElement->OwnerDoc()->GetShell()->GetPresContext()->
>+        GetRootPresContext()->PresShell()->GetRootFrame();
>+      frame->InvalidateWithFlags(frame->GetRect(), nsIFrame::INVALIDATE_NO_THEBES_LAYERS);
>+      // Explicitly request a re-resolve and reflow to update style since the
>+      // animation is over.
>+      // XXXdbaron: is this the right way to do this?
>+      frame->PresContext()->PresShell()->FrameConstructor()->
>+        PostRestyleEvent(aEa->mElement, eRestyle_Subtree,
>+                         nsChangeHint_ReflowFrame);
>+    }

This chunk is indented 2 spaces too little.

Please wrap the comments at less than 80 columns.

Reflowing the root seems wrong.  Why reflow?  And why the root?  And why
do we need this at all given the current setup, where we're still
running style changes?

>+    if (aEa->CanPerformOnCompositorThread()) {
>+      // We must invalidate because this animation was done on the compositor,
>+      // and we must now update for the final style.
>+      nsIFrame* frame = aEa->mElement->OwnerDoc()->GetShell()->GetPresContext()->
>+        GetRootPresContext()->PresShell()->GetRootFrame();
>+      frame->InvalidateWithFlags(frame->GetRect(), nsIFrame::INVALIDATE_NO_THEBES_LAYERS);
>+      // Explicitly request a re-resolve and reflow to update style since the
>+      // animation is over.
>+      // XXXdbaron: is this the right way to do this?
>+      frame->PresContext()->PresShell()->FrameConstructor()->
>+        PostRestyleEvent(aEa->mElement, eRestyle_Subtree,
>+                         nsChangeHint_ReflowFrame);
>+    }

Again, why do we need this?

GetPositionInIteration and aEventsToDispatch?  The second of the
chunks of code that adds to aEventsToDispatch is only doing so if
aAnimation is true.  Should the first as well?  But it's got the
invalidate stuff in it, so you have to be a little careful.

CanPerformAnimationOnCompositor, etc.: put the { of functions on its
own line.

>+    if (!mozilla::css::CommonElementAnimationData::
>+        CanAnimatePropertyOnCompositor(aElement,
>+                                       prop.mProperty)) {

Local style would indent line starting "CanAnimate" by 3 more spaces.

>+    (aTime - mStartTime) / mIterationDuration < mIterationCount;

better to put mIterationDuration on the other side and use multiplication
rather than division

ElementAnimations::CanPerformOnCompositorThread is another piece that
would need to change to animate :before and :after animations on
the compositor thread.  (for later)

Moving the call to AddElementData from GetElementAnimations to
CheckAnimationRule and the fiddling of the SwapElements calls in
CheckAnimationRule both aren't needed anymore (based on our discussion,
a remnant of an earlier patch).


nsAnimationManager.h:

The new GetAnimations function doesn't look like it's used and I'd
rather not have it.  (We already have GetElementAnimations.)


r=dbaron with those things fixed, plus adequate discussion of the reflowing-the-root-frame thing
Comment 96 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-07-20 15:14:53 PDT
Comment on attachment 643717 [details] [diff] [review]
Patch to add support for perspective transform function

r=dbaron
Comment 97 David Zbarsky (:dzbarsky) 2012-07-20 19:48:30 PDT
Created attachment 644574 [details] [diff] [review]
Move CanAnimateOpacity/Transforms to nsLayoutUtils

Seems like a more logical place for it to live in.  Also, caches the prefs and actually checks that the compositor is running.
Comment 98 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-20 23:50:35 PDT
Comment on attachment 644574 [details] [diff] [review]
Move CanAnimateOpacity/Transforms to nsLayoutUtils

Drop the "mozilla::" qualification.

r=me with that.
Comment 99 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-07-21 16:14:14 PDT
Comment on attachment 644574 [details] [diff] [review]
Move CanAnimateOpacity/Transforms to nsLayoutUtils

>From: David Zbarsky <dzbarsky@gmail.com>
>

You need a commit message.  How about:  move checks for whether to animate opacity and transforms on the compositor thread to nsLayoutUtils, and make them also check whether ...

(where I'm not sure what ... is, but it's whatever CompositorParent::CompositorLoop() is).

r=dbaron
Comment 100 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-07-21 16:15:15 PDT
er, looks like it's whether the compositor is running, per comment 97
Comment 101 David Zbarsky (:dzbarsky) 2012-07-22 15:28:35 PDT
(In reply to David Baron [:dbaron] from comment #95)
> Comment on attachment 642935 [details] [diff] [review]
> 
> It seems bad that dom::Element rather than mozilla::dom::Element works
> in a .h file.  Is there a |using namespace mozilla| somewhere there
> shouldn't be?
> 
> It seems bad that dom::Element rather than mozilla::dom::Element works
> in a .h file.  Is there a |using namespace mozilla| somewhere there
> shouldn't be?
> 

The class is wrapped inside a namespace mozilla, so it works.
Comment 102 David Zbarsky (:dzbarsky) 2012-07-22 15:33:32 PDT
(In reply to David Baron [:dbaron] from comment #95)
> Comment on attachment 642935 [details] [diff] [review]
> 
> >+    if (aEa->CanPerformOnCompositorThread()) {
> >+      // We must invalidate because this animation was done on the compositor,
> >+      // and we must now update for the final style.
> >+      nsIFrame* frame = aEa->mElement->OwnerDoc()->GetShell()->GetPresContext()->
> >+        GetRootPresContext()->PresShell()->GetRootFrame();
> >+      frame->InvalidateWithFlags(frame->GetRect(), nsIFrame::INVALIDATE_NO_THEBES_LAYERS);
> >+      // Explicitly request a re-resolve and reflow to update style since the
> >+      // animation is over.
> >+      // XXXdbaron: is this the right way to do this?
> >+      frame->PresContext()->PresShell()->FrameConstructor()->
> >+        PostRestyleEvent(aEa->mElement, eRestyle_Subtree,
> >+                         nsChangeHint_ReflowFrame);
> >+    }
> 
> Again, why do we need this?
> 
> GetPositionInIteration and aEventsToDispatch?  The second of the
> chunks of code that adds to aEventsToDispatch is only doing so if
> aAnimation is true.  Should the first as well?  But it's got the
> invalidate stuff in it, so you have to be a little careful.
> 

Actually if aAnimation is null, we return -1 a few lines above.  I have removed the invalidate calls and replaced them with a xxx comment that we should invalidate to remind me to fix this once we throttle animation ticks.
Comment 103 David Zbarsky (:dzbarsky) 2012-07-22 15:48:02 PDT
(In reply to David Baron [:dbaron] from comment #95)
> Comment on attachment 642935 [details] [diff] [review]
> 
> better to put mIterationDuration on the other side and use multiplication
> rather than division
> 

Can't multiply a float by a TimeDuration.
Comment 104 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-07-22 16:52:35 PDT
It would make sense to add operators to TimeDuration to multiply and divide by unitless numbers (doubles I guess).
Comment 105 David Zbarsky (:dzbarsky) 2012-07-23 13:10:28 PDT
Filed bug 776666 on comment 104
Comment 107 Matt Brubeck (:mbrubeck) 2012-07-23 20:42:44 PDT
Sorry, I backed this out (along with two other bugs from the same push) because of reftest failures:
https://hg.mozilla.org/integration/mozilla-inbound/rev/442e36401b38

https://tbpl.mozilla.org/php/getParsedLog.php?id=13791557&tree=Mozilla-Inbound
REFTEST TEST-UNEXPECTED-FAIL | file:///Users/cltbld/talos-slave/test/build/reftest/tests/layout/reftests/transform-3d/perspective-origin-4a.html | image comparison (==)
Comment 108 Andreas Gal :gal 2012-07-23 20:46:06 PDT
dz, I am seeing some unintended side effects of this patch. When you do a transition that involves a frame that is partially unrendered (e.g. off screen) and for some reason we didn't pre-render it, the result is really weird (you seen the frame move into view but we keep filling in black with actual content). We might have to disable this if the content isn't prerendered and parts of it are coming into view.
Comment 110 David Zbarsky (:dzbarsky) 2012-07-25 01:55:39 PDT
Move animation checks to nsLayoutUtils:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1244b8a8e57a
Comment 114 Aaron Train [:aaronmt] 2012-07-30 08:52:58 PDT
This majorly broke Firefox for Android on Mozilla-Central; see bug 778580
Comment 115 Mark Finkle (:mfinkle) (use needinfo?) 2012-07-30 11:39:22 PDT
Backed out:
http://hg.mozilla.org/mozilla-central/rev/9d2a7a8ca1c7

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