1025709 - Merge ElementTransitions with CommonElementAnimationData

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Brian Birtles (:birtles, Feb 26~ parental leave)]

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.

Comment 1

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 1 - Move mDelay to AnimationTiming

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.

Comment 2

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 2 - Add IsFinished() to ElementAnimation

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.

Comment 3

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 3 - Move delay calculation to GetComputedTimingAt

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.

Comment 4

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 4 - Move EnsureStyleRuleFor from ElementTransitions and ElementAnimations to CommonElementAnimationData

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.

Comment 5

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 5 - Replace aIsThrottled bool value with an enum

Comment 6

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 6 - Reuse nsAnimationManager::EnsureStyleRuleFor

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.

Comment 7

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 7 - Move HasAnimationOfProperty from ElementAnimations/ElementTransitions to base class

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.

Comment 8

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 8 - Move CanPerformOnCompositorThread from ElementAnimations/ElementTransitions to the base class

Comment 9

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 9 - Move heavy lifting of GetAnimationsForCompositor from ElementAnimations/ElementTransitions to base class

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

Comment 10

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 10 - Move active layer notification out of CanPerformOnCompositorThread to call sites

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.

Comment 11

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 11 - Make ElementPropertyTransition::ValuePortionFor reuse GetComputedTimingAt

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.

Comment 12

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 12 - Remove ElementTransitions

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.

Comment 13

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 13 - Simplify ComputeSuitableScaleForAnimation

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

Comment 14

[Brian Birtles (:birtles, Feb 26~ parental leave)]

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

Comment 15

[Cameron McCormack (:heycam)]

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 16

[Cameron McCormack (:heycam)]

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 17

[Cameron McCormack (:heycam)]

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 18

[Cameron McCormack (:heycam)]

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 19

[Cameron McCormack (:heycam)]

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 20

[Cameron McCormack (:heycam)]

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 21

[Cameron McCormack (:heycam)]

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.

Comment 22

[Brian Birtles (:birtles, Feb 26~ parental leave)]

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

Comment 23

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(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).

Comment 24

[Ed Morley [:emorley]]

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