As the first step towards implementing Web Animations we want to unify some of the code used for animations and transitions. Nick will provide more details in time.

(In reply to Brian Birtles (:birtles) from comment #0) > As the first step towards implementing Web Animations we want to unify some > of the code used for animations and transitions. > > Nick will provide more details in time. More time than expected, sorry. This bug deals with way we represent animations and transitions in gecko, rather than the logic of starting/stopping/flushing etc. We should replace the two sets of structs in nsAnimationManager.h and nsTransitionManager.h (which inherit from structs in AnimationCommon.h) with a single set. We already convert some transitions to animations for OMTA in AddAnimationsAndTransitionsToLayer in nsDisplayList.cpp. It would be better to not do this and do something logically equivalent when we create our internal representations. We do some computation on animations in Layer::SetAnimations it would be nice if we could avoid this and have one canonical representation of animations which we use everywhere. Do we need to have the AnimData class or could this be merged into our web animations super-structs?

I started doing something like this - take a look at content/base/src/Animation.h. Obviously we'll need to add some more things to it.

I think we might want to do something similar to the MediaStream module, there is a DOMMediaStream which take cares of the webidl level, and MediaStream which does the real work. For example, DOMTiming would be a wrapper of Timing(a pure class that doesn't expose to javascript), and we can swap out those timing related attributes in CSS Animations/Transitions to this Timing class.

SVG does the same thing. The issue is that we have to hold on to the DOM wrappers to make sure we return the same object every time, so you have to store them in a hashtable. But then you also have to make sure you don't leak them - I've fixed several leaks in the SVG code, and there are probably more. Unless we're overly concerned about memory usage here, I would advise against doing this splitting.

Ahh, I see, thanks for your explain! And after taking a second look at Brian's comment in bug 875219, I meant to take the second one "Make CSS Animations use this new animation object", unless you already have patches for that or are doing that?

Nope, I haven't worked on that at all. Go for it.

But in the process of doing that, I'm sure you'll need to add more things to the Animation thing I made, so I guess you'll be doing both

Talked to Brian on irc, sorry about holding it for so long and doing nothing, my hands are full by other tasks, release to Brian, and I'll pick it up afterward :)

As a first step towards making CSS animations and CSS transitions use the same data structures, this patch aligns their behavior with regards to start time and delay handling. Previously, ElementAnimation objects maintained separate mStartTime and mDelay members whilst ElementPropertyTransition objects maintained a single mStartTime property that incorporated the delay. This patch adds an mDelay member to ElementPropertyTransition and stores the delay and start time separately. Calculations involving ElementPropertyTransition::mStartTime for are adjusted to incorporate mDelay.

As part of moving towards more shared data structures for animation, this patch makes ElementPropertyTransition inherit from ElementAnimation. At the same time we switch from storing the target property, start/end values, start time, delay, and timing function on the transition to the corresponding location in ElementAnimation. Since nsDisplayList::AddAnimationsAndTransitionsToLayer was already doing this conversion in order to create animations to pass to the compositor thread, we can remove the conversion code from there and just use the ElementAnimation data structures as is.

Both ElementPropertyTransition and ElementAnimation specify an IsRunningAt method which have the same purpose but with two subtle differences: a) ElementPropertyTransition::IsRunningAt checks if the transition is a removed sentinel and if so returns false. This patch adds a check for a null start time to IsRunningAt since I think in future we will want to allow null times in various places to represent, for example, animations that are not connected to a timeline. (However, ultimately we will probably not allow start times on *animations* to be null, only on their associated player.) Should we later use a different mechanism for marking sentinel transitions (e.g. a boolean flag) this method should still be correct as it checks if aTime is inside the transition interval before returning true. b) ElementPropertyTransition::IsRunningAt returns false if the transition is in the delay phase, that is, waiting to start. This patch changes this behavior so that transitions are considered running even if they are in the delay phase. This brings their behavior into line with animations and removes the need for the ElementPropertyTransition::mIsRunningOnCompositor since it is only used to determine when a transition in the delay phase has begun. ElementAnimation::IsRunningAt also handles pause state and iterations but this logic should still be correct for transitions which, in this area, only use a subset of the functionality of animations since their pause state is always playing and their iteration count is 1.

Now that ElementTransitionProperty inherits from ElementAnimation, ElementTransitions::HasAnimationOfProperty can re-use ElementAnimation::HasAnimationOfProperty in its definition of ElementTransitions::HasAnimationOfProperty. Similarly, in nsDisplayList::AddAnimationsAndTransitionsToLayer we can use this method rather than drilling down to the appropriate segment by hand.

This doesn't seem to be overridden anywhere. I suspect it was a copy-paste mistake because the methods of the same name on ElementAnimations, ElementTransitions, and CommonElementAnimationData are virtual.

The loops for adding animations and transitions to a layer in nsDisplayList::AddAnimationsAndTransitionsToLayer are now identical and so can be factored out into a common method. Since it is not possible to implicitly convert from nsTArray<ElementPropertyTransition> to nsTArray<ElementAnimation> despite ElementPropertyTransition being a subclass of ElementAnimation a templated method is used. In the future, as animations and transitions share more and more code we may be able to remove the need for templates.

This is as far as I've gotten so far. I think the next step is to store animations and transitions in a common list. Currently there are a places where we do pretty much the same things twice: once for the list of transitions and then once for the list of animations. For example, nsLayoutUtils::GetMaximumAnimatedScale What's more, once we allow creating animations by script and we re-use this for SVG animations I'd rather we didn't end up with four different lists of animations to handle separately each time. I think there are a few different ways to tackle this: A. Add some common GetAllAnimationsForElement-style methods which basically go through these individual lists and create a new array of pointers to ElementAnimation objects (which will probably just end up being called 'Animation' objects). That seems a shame to have to make up that list over and over again. B. Continue using inheritance and make the common list store pointers to the base class. Also add IsTransition()-type methods as a kind of quick-and-dirty RTTI so that, for example, transition-related code can safely downcast if needed (i.e. if we can't put all transition-specific code in overridden methods). This is probably the cleanest and simplest approach but adds a couple of layers of indirection by storing pointers instead of objects and making methods virtual. C. Have an Animation class which is just the common data for an animation and which is never subclassed. Introduce mAnimationType (enum) and mAnimationData (void*) for storing type-specific data like values related to transition reversing. Clean-up of the type-specific data would be performed by calling mManager->ElementDataRemoved() like we already do. This would allow storing the animations directly using InfalliableTArray like we currently do. D. Just stick everything every type of animation is ever going to need in the base class. I lean towards C but once we come to implement Element.getCurrentAnimations (which returns sequence<Animation>) we'll probably need to return an array of pointers to ref-counted objects so we won't be allocating using an InfallibleTArray anyway. I'll try B for now.

This patch relocates ElementAnimation from nsAnimationManager.{h,cpp} to AnimationCommon.{h,cpp} and in the process moves it into the mozilla::css namespace.

We need a basic representation of animations from which we can derive subclasses to represent specific cases such as transitions. For now we will retrofit ElementAnimation for that purpose hence renaming it to Animation. We can later create a "CSS Animation" subclass if needed (for example, if the mName member is only used by CSS Animations we might do that). This patch removes the "using namespace mozilla::layers" line from AnimationCommon.cpp since the 'unified' build system concatenates several files together before compiling meaning using declarations like this leak into other files creating ambiguities. In this case, it creates ambiguities in nsAnimationManager.cpp between mozilla::layers::Animation and mozilla::css::Animation.

Try server run of these patches: https://tbpl.mozilla.org/?tree=Try&rev=276a48617b14 (TBPL only shows the last patch since the previous push(es) contained the other patches in the queue)

Comment on attachment 8369797 [details] [diff] [review] part 2 - Make ElementPropertyTransition inherit from ElementAnimation nsLayoutUtils.cpp: Please add a local variable: const AnimationPropertySegment& segment = pt.mProperties[0].mSegments[0]; and then use segment.mFromValue and segment.mToValue, rather than repeating pt.mProperties[0].mSegments[0] twice. nsTransitionManager.h: >+ // transition is being reversed. Normally the same as >+ // mProperties[0].mSegments[0].mFromValue, except when this transition started >+ // as the reversal of another in-progress transition. Needed so we can handle >+ // two reverses in a row. Please wrap the comment at something < 80 chars, preferably matching what surrounds it. Could you add a comment to ElementAnimations.h above mStartTime pointing out that ElementPropertyTransition also uses mStartTime in its IsRemovedSentinel/SetRemovedSentinel methods? nsTransitionManager.cpp: In ElementTransitions::EnsureStyleRuleFor please move this assertion: >+ MOZ_ASSERT(pt.mProperties[0].mSegments.Length() == 1, >+ "Animation property should have one segment for a transition"); down closer to where you depend on it. >+ MOZ_ASSERT(pt.mProperties.Length() == 1, >+ "Should have one animation property for a transition"); Please line up the " with the p in pt.mProperties (and the same for later asserts) >+ if (!css::CommonElementAnimationData::CanAnimatePropertyOnCompositor( >+ mElement, >+ prop.mProperty, >+ aFlags) || Please leave mElement through to aFlags on the same line. >+ AnimationProperty &prop = *pt.mProperties.AppendElement(); ... >+ AnimationPropertySegment &segment = *prop.mSegments.AppendElement(); new code should have & before the space (even though there's code using the other style right next to it), since I lost that style battle. >+ pt.mFillMode = NS_STYLE_ANIMATION_FILL_MODE_NONE; I think it's actually more appropriate to use NS_STYLE_ANIMATION_FILL_MODE_BACKWARDS here, since transitions with delay do fill in the old value during the delay. (Probably the only non-stylistic comment on the patch!) r=dbaron with that

Comment on attachment 8369798 [details] [diff] [review] part 3 - Remove ElementPropertyTransition::IsRunningAt and mIsRunningOnCompositor >- } else if (pt.mStartTime + pt.mDelay <= now && canThrottleTick && >- !pt.mIsRunningOnCompositor) { >- // Start a transition with a delay where we should start the >- // transition proper. >- et->UpdateAnimationGeneration(mPresContext); >- transitionStartedOrEnded = true; > } Could you explain why this removal is ok? (I'd expect it to break only off-main-thread animations, which would be B2G-only right now.)

Comment on attachment 8369805 [details] [diff] [review] part 6 - Factor out common method for adding animations and transitions to a layer Seems ok, I guess, although there doesn't seem to be much benefit. (Maybe just wait until after the refactoring that will remove the need for the template?)

Comment on attachment 8369813 [details] [diff] [review] part 7 - Move ElementAnimation to AnimationCommon >+/** >+ * A pair of animation values for a given property with their offsets and timing >+ * function. >+ */ I don't think this comment adds anything that's not already apparent from the names. (And if it did, I'd ask you to wrap it at < 80 chars.) So please remove. >+/** >+ * A target property and the animation segments with which it is to be animated. >+ */ Same here. >+/** >+ * Data about one animation running on an element. >+ */ Maybe add "such as a CSS transition for a single property, or an animation described by a single 'animation-name'"? >- TimeDuration aIterationDuration, >- double aIterationCount, >- uint32_t aDirection, >- ElementAnimation* aAnimation = nullptr, >- ElementAnimations* aEa = nullptr, >- EventArray* aEventsToDispatch = nullptr); >+ TimeDuration aIterationDuration, >+ double aIterationCount, >+ uint32_t aDirection, >+ mozilla::css::ElementAnimation* aAnimation = nullptr, >+ ElementAnimations* aEa = nullptr, >+ EventArray* aEventsToDispatch = nullptr); Could you leave this indented where it was, and just break the long line. > void BuildAnimations(nsStyleContext* aStyleContext, >- InfallibleTArray<ElementAnimation>& aAnimations); >- bool BuildSegment(InfallibleTArray<AnimationPropertySegment>& aSegments, >- nsCSSProperty aProperty, const nsAnimation& aAnimation, >- float aFromKey, nsStyleContext* aFromContext, >- mozilla::css::Declaration* aFromDeclaration, >- float aToKey, nsStyleContext* aToContext); >+ InfallibleTArray<mozilla::css::ElementAnimation>& aAnimations); >+ bool BuildSegment( >+ InfallibleTArray<mozilla::css::AnimationPropertySegment>& aSegments, >+ nsCSSProperty aProperty, const nsAnimation& aAnimation, >+ float aFromKey, nsStyleContext* aFromContext, >+ mozilla::css::Declaration* aFromDeclaration, >+ float aToKey, nsStyleContext* aToContext); Same here. Also, I'd really like the mozilla::css:: sub-namespace to go away. But I won't make you deal with that here. r=dbaron

Comment on attachment 8369819 [details] [diff] [review] part 8 - Rename ElementAnimation to Animation I'd like the mozilla::css:: namespace to go away... so I'd like to have a more specific name here. StyleAnimation? SpecifiedAnimation? something else? Also, can you do using mozilla::layers::Layer instead of using namespace mozilla:layers , and avoid the other qualification ... though it's probably not relevant given my first comment.

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #30) > Comment on attachment 8369798 [details] [diff] [review] > part 3 - Remove ElementPropertyTransition::IsRunningAt and > mIsRunningOnCompositor > > >- } else if (pt.mStartTime + pt.mDelay <= now && canThrottleTick && > >- !pt.mIsRunningOnCompositor) { > >- // Start a transition with a delay where we should start the > >- // transition proper. > >- et->UpdateAnimationGeneration(mPresContext); > >- transitionStartedOrEnded = true; > > } > > Could you explain why this removal is ok? > > (I'd expect it to break only off-main-thread animations, which would be > B2G-only right now.) It's not ok. I assumed that since transitions now specify their delay like animations do, no special handling was required at the end of the delay phase. That turns out to be incorrect but our OMTA tests failed to pick this up. I guess I'll need to add more OMTA tests for transitions too, not just animations. I've confirmed that animations on the compositor thread that have a delay do start correctly. This is regardless of whether or not they have a backwards fill. I've yet to determine why they don't need a mIsRunningOnCompositor member. I'll dig into this further and post an updated patch. (In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #33) > Comment on attachment 8369819 [details] [diff] [review] > part 8 - Rename ElementAnimation to Animation > > I'd like the mozilla::css:: namespace to go away... so I'd like to have a > more specific name here. StyleAnimation? SpecifiedAnimation? something > else? I think I'll go with mozilla::StyleAnimation for now. If we implement the Web Animations' "Animation" interface directly using this class then I guess it will be renamed mozilla::dom::Animation but that's a separate bug. I've incorporated all the other review feedback locally. Thanks very much for that.

Comment on attachment 8369798 [details] [diff] [review] part 3 - Remove ElementPropertyTransition::IsRunningAt and mIsRunningOnCompositor Clearing review request while I investigate further.

(In reply to Brian Birtles (:birtles) from comment #34) > I've confirmed that animations on the compositor thread that have a delay do > start correctly. This is regardless of whether or not they have a backwards > fill. I've yet to determine why they don't need a mIsRunningOnCompositor > member. I'll dig into this further and post an updated patch. Actually, it turns out these animations only start correctly if there are UI events that trigger the paint. Filed as bug 975261. Basically, animations with a delay are broken and this patch would break transitions in the same way.

Fix bitrot

Now that ElementAnimations have mIsRunningOnCompositor I can leave the section that I misguidely removed as pointed out in comment 30

Fix bitrot

Fix bitrot

Fix bitrot

Fix bitrot

Address review feedback (comment 33)

Two comments in AnimationCommon.h refer to 'mFlushCount' which was presumably the old name for mAnimationGeneration. Also, one comment says nsCSSFrameConstructor tracks this. This patch adjusts the comments to refer to mAnimationGeneration and RestyleManager. (The reference to nsTransitionManager::UpdateAllThrottleStyles() is still valid since there is useful documentation accompanying that method despite the fact that the relevant code is mostly contained in AnimationCommon.h since bug 914847. Eventually we will unify the structures of transitions and animations to the the point that we can replace the IMPL_UPDATE_ALL_THROTTLED_STYLES_INTERNAL macro in AnimationCommon.h with an actual method. At that point we can move the documentation accompanying nsTransitionManager::UpdateAllThrottleStyles and its references to AnimationCommon.)

Comment on attachment 8398360 [details] [diff] [review] part 3 - Remove ElementPropertyTransition::IsRunningAt and mIsRunningOnCompositor Does this mean we ship transitions to the compositor at the start of their delay rather than the end? (That seems like a reasonable change to make.) Could you add a comment to ElementPropertyTransition::IsRunningAt that ElementAnimation::IsRunningAt depends on the use of mTimeStamp being null? r=dbaron with that

Comment on attachment 8398367 [details] [diff] [review] part 9 - Fix comment about mFlushCount Yeah, a missed update from the review comments in bug 780692 comment 187. (I think I have some further related changes in my patch queue, but nothing to stop this from landing. And at some point I think we need to fix the brokenness of having different generations for animations and transitions and then having only one generation stored on the layer; I have a patch for that in my tree but I never got around to demonstrating that anything was actually broken, though perhaps I should just try to get it in anyway.)

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #45) > Comment on attachment 8398360 [details] [diff] [review] > part 3 - Remove ElementPropertyTransition::IsRunningAt and > mIsRunningOnCompositor > > Does this mean we ship transitions to the compositor at the start of their > delay rather than the end? (That seems like a reasonable change to make.) No I don't think so. ElementAnimation::IsRunningAt calls ElapsedDurationAt which will return a negative value during the delay period. IsRunningAt will then return false since the negative iterationElapsed value is not >= 0.0. (That said, in the future I'd like to ship animations/transitions to the compositor before the end of their delay so that if the main thread gets busy the start of the animation doesn't get held up which is especially important when the delay is used to synchronize animations.) > Could you add a comment to ElementPropertyTransition::IsRunningAt that > ElementAnimation::IsRunningAt depends on the use of mTimeStamp being null? This patch removes ElementPropertyTransition::IsRunningAt altogether by putting the functionality in the ElementAnimation superclass. I can put a comment on ElementAnimation::IsRunningAt but is there somewhere else you were concerned about?

(In reply to Brian Birtles (:birtles) from comment #47) > (In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #45) > > Could you add a comment to ElementPropertyTransition::IsRunningAt that > > ElementAnimation::IsRunningAt depends on the use of mTimeStamp being null? > > This patch removes ElementPropertyTransition::IsRunningAt altogether by > putting the functionality in the ElementAnimation superclass. I can put a > comment on ElementAnimation::IsRunningAt but is there somewhere else you > were concerned about? Sorry, what I meant to write was: Could you add a comment to ElementPropertyTransition::IsRemovedSentinel that ElementAnimation::IsRunningAt depends on the use of mTimeStamp being null?

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #48) > (In reply to Brian Birtles (:birtles) from comment #47) > > (In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #45) > > > Could you add a comment to ElementPropertyTransition::IsRunningAt that > > > ElementAnimation::IsRunningAt depends on the use of mTimeStamp being null? > > > > This patch removes ElementPropertyTransition::IsRunningAt altogether by > > putting the functionality in the ElementAnimation superclass. I can put a > > comment on ElementAnimation::IsRunningAt but is there somewhere else you > > were concerned about? > > Sorry, what I meant to write was: > > Could you add a comment to ElementPropertyTransition::IsRemovedSentinel that > ElementAnimation::IsRunningAt depends on the use of mTimeStamp being null? Thanks, that makes sense.

https://hg.mozilla.org/integration/mozilla-inbound/rev/2b7444f517c8 https://hg.mozilla.org/integration/mozilla-inbound/rev/e8e989773e24 https://hg.mozilla.org/integration/mozilla-inbound/rev/abd19a468375 https://hg.mozilla.org/integration/mozilla-inbound/rev/8836e0b328b7 https://hg.mozilla.org/integration/mozilla-inbound/rev/762aa62425b9 https://hg.mozilla.org/integration/mozilla-inbound/rev/56fb71158cbb https://hg.mozilla.org/integration/mozilla-inbound/rev/a8a6af1944c6 https://hg.mozilla.org/integration/mozilla-inbound/rev/85114363d5ae https://hg.mozilla.org/integration/mozilla-inbound/rev/d2f3c58a8e46

https://hg.mozilla.org/mozilla-central/rev/2b7444f517c8 https://hg.mozilla.org/mozilla-central/rev/e8e989773e24 https://hg.mozilla.org/mozilla-central/rev/abd19a468375 https://hg.mozilla.org/mozilla-central/rev/8836e0b328b7 https://hg.mozilla.org/mozilla-central/rev/762aa62425b9 https://hg.mozilla.org/mozilla-central/rev/56fb71158cbb https://hg.mozilla.org/mozilla-central/rev/a8a6af1944c6 https://hg.mozilla.org/mozilla-central/rev/85114363d5ae https://hg.mozilla.org/mozilla-central/rev/d2f3c58a8e46