This corresponds to phase 3 outlined in bug 999922. The idea is to eventually have just one class called something like "StyleAnimationCollection" to represent a set of animations.

This patch is the first part in preparing the way to merge ElementTransitions with CommonElementAnimationData (which we'll eventually rename to something nicer). Here we move mTiming from CommonElementAnimationData to the AnimationTiming struct. While this is not strictly necessary in order to do the later refactoring it makes it simpler since it: - Divides time calculation into calculation based on dynamic play state (the responsibility of animation players in Web Animations terms) and static author-specific timing parameters (a property of animations in Web Animations terms). - In future we will probably put animations on the compositor during their delay phase so we will want the delay to be present in the AnimationTiming struct then. - Makes AnimationTiming line up with the dictionary of the same name in Web Animations.

One of the main differences in handling a list of transitions vs a list of regular animations is that when we are dealing with a list of transitions we need to check for transitions that have finished and are about to be discarded but need to be retained temporarily to provide correct triggering of subsequent transitions. Such transitions are marked as "removed sentinels" and are ignored for most operations. This patch moves the methods for setting and checking such transitions to the base class ElementAnimation so that we can treat animations and transitions alike without having to downcast or do obscure checks for mStartTime.IsNull() (which equates to checking if the animation is a "removed sentinel" but is not particularly clear). In the process, this patch renames said methods to Is/SetFinishedTransition since hopefully that is a little easier to understand at a glance.

Now that an animation's delay is part of AnimationTiming--the struct we pass to GetComputedTimingAt--it makes sense to act on it in GetComputedTimingAt. This also happens to bring the procedures here closer to the algorithm definitions in Web Animations. As part of this refactoring, this patch converts ElementAnimation::IsRunningAt to use GetComputedTiming since the previous approach no longer wheres now that GetLocalTimeAt (nee ElapsedDurationAt) no longer handles delays. This also removes duplicated logic. Also, previously ElapsedDurationAt would assert if called on a finished transition since TimeDuration's - operator wouldn't like the null mStartTime. This patch adds an assertion for this case to GetLocalTimeAt to ease debugging.

Both ElementAnimations and ElementTransitions have an EnsureStyleRuleFor method. The ElementAnimations version is a more general of the ElementTransitions one with the exception that the ElementTransitions version checks for finished transitions. This patch moves the code from ElementAnimations to CommonElementAnimationData with one minor change: adding the checks for finished transitions. The ElementTransitions version is removed. Since the ElementAnimations version contains a second parameter, aIsThrottled, callers of ElementTransitions must include this extra parameter. In a subsequent we add an enum for this parameter to make call sites easier to read. The ElementAnimations version also sets the mNeedsRefreshes member so at the same time we move mNeedsRefreshes to CommonElementAnimationData. Furthermore, since the ElementAnimations version which we have adopted returns early if mNeedsRefreshes is false, this patch ensures that when we call EnsureStyleRuleFor from ElementTransitions::WalkTransitionRule, we set mNeedsRefreshes to true first. Another difference to account for is that the ElementTransitions version of EnsureStyleRuleFor *always* sets mStyleRule (even if it doesn't add anything to it) where as the ElementAnimations version only creates the rule when necessary so we need to add a check to ElementTransitions::WalkTransitionRule that mStyleRule is actually set before using it.

In a number of places in nsAnimationManager we have the following sequence of calls: CommonElementAnimationData::EnsureStyleRuleFor ElementAnimations::GetEventsAt nsAnimationManager::CheckNeedsRefresh nsAnimationManager::EnsureStyleRuleFor already does exactly that so we should just reuse it.

This patch moves HasAnimationOfProperty to CommonElementAnimationData. It also takes the chance to start removing some redundancy from nsLayoutUtils / ActiveLayerTracker. Some of this should never have been added in the first place and some could have been removed earlier on but while we're fixing up HasAnimationOfProperty it seems like an appropriate time to fix up its call sites too.

This patch still leaves ElementAnimations| ElementTransitions::GetAnimationsForCompositor as shortcuts for the method now defined on CommonElementAnimationData.

A previous patch moved CanPerformOnCompositorThread to CommonElementAnimationData including a FIXME saying that active layer notification should happen at call sites. Now that the code for GetAnimationsForCompositor is common, we can do the active layer notification there.

In order to remove redundant code and generally make transitions less special, this patch reworks ValuePortionFor to reuse the existing code for calculation the fractional distance of within the animation interval.

This patch replaces all references to ElementTransitions (now that it is empty) with references to the base class, CommonElementAnimationData. It also takes the opportunity to tidy up some of the call sites in nsLayoutUtils since they no longer need to differentiate between animations and transitions.

Now that animations and transitions are largely treated alike, we can remove some redundant code in the processing of transform animations in ComputeSuitableScaleForAnimation.

Try run: https://tbpl.mozilla.org/?tree=Try&rev=8b11a8698421

Comment on attachment 8440499 [details] [diff] [review] part 2 - Add IsFinished() to ElementAnimation Review of attachment 8440499 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/style/AnimationCommon.h @@ +310,5 @@ > + } > + void SetFinishedTransition() { > + mStartTime = mozilla::TimeStamp(); > + } > + Assert in these methods that this ElementAnimation is indeed a transition?

Comment on attachment 8440500 [details] [diff] [review] part 3 - Move delay calculation to GetComputedTimingAt Review of attachment 8440500 [details] [diff] [review]: ----------------------------------------------------------------- s/no longer wheres/no longer works/ in the commit message.

Comment on attachment 8440501 [details] [diff] [review] part 4 - Move EnsureStyleRuleFor from ElementTransitions and ElementAnimations to CommonElementAnimationData Review of attachment 8440501 [details] [diff] [review]: ----------------------------------------------------------------- s/subsequent/subsequent patch/ in the commit message.

Comment on attachment 8440503 [details] [diff] [review] part 6 - Reuse nsAnimationManager::EnsureStyleRuleFor Review of attachment 8440503 [details] [diff] [review]: ----------------------------------------------------------------- Should nsAnimationManager::EnsureStyleRuleFor be called something that implies it will do the event checking and refresh observer listening/unlistening? Up to you.

Comment on attachment 8440504 [details] [diff] [review] part 7 - Move HasAnimationOfProperty from ElementAnimations/ElementTransitions to base class Review of attachment 8440504 [details] [diff] [review]: ----------------------------------------------------------------- HasAnimationOrTransition is not a great name, since it returns an object rather than a bool. How about renaming it in this patch -- would GetAnimationOrTransition be better? r=me with that and the below. ::: layout/base/nsLayoutUtils.cpp @@ +294,3 @@ > nsIAtom* aAnimationProperty, > nsCSSProperty aProperty) > { Can you add an assertion in here that aAnimationProperty is one of the expected two atoms? ::: layout/style/AnimationCommon.cpp @@ +585,5 @@ > +bool > +CommonElementAnimationData::HasAnimationOfProperty( > + nsCSSProperty aProperty) const > +{ > + for (uint32_t animIdx = mAnimations.Length(); animIdx-- != 0; ) { (No need to change this here, since you're basically just moving the existing code, but I think we should use size_t rather than uint32_t in the future for nsTArray index variables, if you're not aware.)

Comment on attachment 8440506 [details] [diff] [review] part 9 - Move heavy lifting of GetAnimationsForCompositor from ElementAnimations/ElementTransitions to base class Review of attachment 8440506 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/style/AnimationCommon.cpp @@ +79,5 @@ > +CommonElementAnimationData* > +CommonAnimationManager::GetAnimationsForCompositor(nsIContent* aContent, > + nsIAtom* aElementProperty, > + nsCSSProperty aProperty) > +{ Assert that aElementProperty is valid.

Comment on attachment 8440509 [details] [diff] [review] part 12 - Remove ElementTransitions Review of attachment 8440509 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/style/AnimationCommon.cpp @@ +267,5 @@ > ea, mPresContext->RefreshDriver()->MostRecentRefresh(), > EnsureStyleRule_IsNotThrottled); > curRule.mRule = ea->mStyleRule; > } else if (curRule.mLevel == nsStyleSet::eTransitionSheet) { > + CommonElementAnimationData *et = Move the * next to the type while you're touching this line. And on a few other lines later in the patch. ::: layout/style/nsTransitionManager.h @@ +157,5 @@ > private: > void ConsiderStartingTransition(nsCSSProperty aProperty, > const nsTransition& aTransition, > mozilla::dom::Element *aElement, > + CommonElementAnimationData *&aElementTransitions, "*&" next to type.

https://hg.mozilla.org/integration/mozilla-inbound/rev/6aa5cfba2d4d https://hg.mozilla.org/integration/mozilla-inbound/rev/458a988940c6 https://hg.mozilla.org/integration/mozilla-inbound/rev/2d45653b217d https://hg.mozilla.org/integration/mozilla-inbound/rev/31695984cfe2 https://hg.mozilla.org/integration/mozilla-inbound/rev/8434b5e208d6 https://hg.mozilla.org/integration/mozilla-inbound/rev/dde79ba0a596 https://hg.mozilla.org/integration/mozilla-inbound/rev/d045b05628df https://hg.mozilla.org/integration/mozilla-inbound/rev/f34e89ebac60 https://hg.mozilla.org/integration/mozilla-inbound/rev/a4c6f7cb5fe4 https://hg.mozilla.org/integration/mozilla-inbound/rev/b0b2c12fd058 https://hg.mozilla.org/integration/mozilla-inbound/rev/3af866f895f2 https://hg.mozilla.org/integration/mozilla-inbound/rev/8cc5818e3f83 https://hg.mozilla.org/integration/mozilla-inbound/rev/ab404d8e7d88

(In reply to Cameron McCormack (:heycam) from comment #19) > Comment on attachment 8440504 [details] [diff] [review] > part 7 - Move HasAnimationOfProperty from > ElementAnimations/ElementTransitions to base class ... > ::: layout/base/nsLayoutUtils.cpp > @@ +294,3 @@ > > nsIAtom* aAnimationProperty, > > nsCSSProperty aProperty) > > { > > Can you add an assertion in here that aAnimationProperty is one of the > expected two atoms? Oh, I replied to this before but forgot to press Save Changes. There are actually six atoms here that make sense. The extra four correspond to transitions/animations of pseudo-elements ::before/::after. This function is not currently used for those but there's no reason why it wouldn't work. Likewise for comment 20 on part 9, there are six atoms and in that case the function is actually used for pseudo-elements (CanPerformOnCompositorThread checks for pseudo-elements). So I haven't added those checks yet since we'd be checking for six different possibilities (unless we rely on them being clumped together).

https://hg.mozilla.org/mozilla-central/rev/6aa5cfba2d4d https://hg.mozilla.org/mozilla-central/rev/458a988940c6 https://hg.mozilla.org/mozilla-central/rev/2d45653b217d https://hg.mozilla.org/mozilla-central/rev/31695984cfe2 https://hg.mozilla.org/mozilla-central/rev/8434b5e208d6 https://hg.mozilla.org/mozilla-central/rev/dde79ba0a596 https://hg.mozilla.org/mozilla-central/rev/d045b05628df https://hg.mozilla.org/mozilla-central/rev/f34e89ebac60 https://hg.mozilla.org/mozilla-central/rev/a4c6f7cb5fe4 https://hg.mozilla.org/mozilla-central/rev/b0b2c12fd058 https://hg.mozilla.org/mozilla-central/rev/3af866f895f2 https://hg.mozilla.org/mozilla-central/rev/8cc5818e3f83 https://hg.mozilla.org/mozilla-central/rev/ab404d8e7d88