1232561 - Add a ComposeAnimationRule method that works off the effect set

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Description

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

This covers the first part of bug 1230040:

  * Add AnimationLevel to EffectCompositor (use with AppliesToTransitionsLevel)
  * Move mStyleRule to the EffectSet
  * Add static ComposeStyleRule to EffectCompositor and call from EnsureStyleRule

Comment 1

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

Attachment: part 1 - Replace AppliesToTransitionsLevel() with a cascade level enumeration

Introducing an enum will simplify further patches in this series by providing
a common vocabulary for this distinction.

Comment 2

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

Attachment: part 2 - Move the animation style rules from AnimationCollection to EffectSet

This is needed in order to support script-generated animations since they do not
belong to any AnimationCollection.

This patch adopts the naming "animation rule" over "style rule". Currently we
are inconsistent about this (e.g. GetAnimationRule vs EnsureStyleRuleFor).
We don't do a mass rename here but just a few places near where we're touching.
Many of the other references to "style rule" will be revised in this bug or
related bugs so we can fix those references when we come to them.

Comment 3

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

Attachment: part 3 - Move AnimValuesStyleRule to a separate file

As we gradually move logic from layout/style/AnimationCommon.cpp to
dom/animation/EffectSet and EffectCompositor it makes sense to let this class
live in its own file inside dom/animation where it is used.

Comment 4

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

Attachment: part 4 - Add EffectCompositor::ComposeAnimationRule

This patch just moves a piece of functionality from
AnimationCollection::EnsureStyleRuleFor to the EffectCompositor. In subsequent
bugs we will moves more and more of this functionality across until this
logic is fully contained in the EffectCompositor.

Comment 5

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

I was going to wait for bug 1228229 to be reviewed before putting this up for review (in case I need to do rebasing) but I've been waiting a long time for those reviews so I'm just going to go ahead with requesting review on this.

Comment 6

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c44b725e9e16

Comment 7

[Cameron McCormack (:heycam)]

Comment on attachment 8701684 [details] [diff] [review]
part 1 - Replace AppliesToTransitionsLevel() with a cascade level enumeration

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

::: dom/animation/Animation.h
@@ +10,5 @@
>  #include "nsWrapperCache.h"
>  #include "nsCycleCollectionParticipant.h"
>  #include "mozilla/Attributes.h"
>  #include "mozilla/LinkedList.h"
> +#include "mozilla/EffectCompositor.h" // For EffectCompositor::CascadeLevel

Up one line, if you want to keep the includes (approximately) sorted.

Comment 8

[Cameron McCormack (:heycam)]

Comment on attachment 8701707 [details] [diff] [review]
part 2 - Move the animation style rules from AnimationCollection to EffectSet

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

::: dom/animation/EffectCompositor.h
@@ +39,5 @@
>      // The transitions sheet (CSS transitions that are tied to CSS markup)
>      Transitions
>    };
> +  static const size_t kCascadeLevelCount =
> +    static_cast<size_t>(CascadeLevel::Transitions) + 1;

This can be removed if we use an EnumeratedArray per my later comment.

::: dom/animation/EffectSet.h
@@ +7,5 @@
>  #ifndef mozilla_EffectSet_h
>  #define mozilla_EffectSet_h
>  
> +#include "mozilla/EffectCompositor.h"
> +#include "AnimationCommon.h" // For AnimValuesStyleRule

Swap these around to keep these sorted (unless you are sorting by directory first, then file within directory).

@@ +126,5 @@
>    }
>    bool IsEmpty() const { return mEffects.IsEmpty(); }
>  
> +  RefPtr<AnimValuesStyleRule>& AnimationRule(EffectCompositor::CascadeLevel
> +                                           aCascadeLevel)

Indenting is off.

@@ +152,5 @@
> +  // style without animation, we need to not use them so that we can
> +  // detect any new changes; if necessary we restyle immediately
> +  // afterwards with animation.
> +  RefPtr<AnimValuesStyleRule>
> +    mAnimationRule[EffectCompositor::kCascadeLevelCount];

How about we use a mozilla::EnumeratedArray here instead of a C array?  That will let us index into it with CascadeLevel values directly without needing to cast to an integer type.

::: layout/style/AnimationCommon.cpp
@@ +317,5 @@
>  
>    collection->EnsureStyleRuleFor(
>      mPresContext->RefreshDriver()->MostRecentRefresh());
>  
> +  EffectSet *effectSet = EffectSet::GetEffectSet(aElement, aPseudoType);

"*" next to type.

@@ +324,5 @@
> +  }
> +
> +  return IsAnimationManager() ?
> +         effectSet->AnimationRule(EffectCompositor::CascadeLevel::Animations) :
> +         effectSet->AnimationRule(EffectCompositor::CascadeLevel::Transitions);

Can you explain why we don't do the kind of check like in CSSTransition::CascadeLevel()?  Are all the Animation objects in the manager tied to markup?

::: layout/style/nsAnimationManager.cpp
@@ -419,5 @@
>      return nullptr;
>    }
>  
>    if (collection) {
> -    collection->mStyleRule = nullptr;

Why don't we need to do the equivalent thing on the EffectSet?

Comment 9

[Cameron McCormack (:heycam)]

Comment on attachment 8701708 [details] [diff] [review]
part 3 - Move AnimValuesStyleRule to a separate file

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

r=me though I didn't check the moved code carefully.

Comment 10

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

(In reply to Cameron McCormack (:heycam) (away Dec 25-28) from comment #8)
> Comment on attachment 8701707 [details] [diff] [review]
> part 2 - Move the animation style rules from AnimationCollection to EffectSet
> 
> Review of attachment 8701707 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/animation/EffectCompositor.h
> @@ +39,5 @@
> >      // The transitions sheet (CSS transitions that are tied to CSS markup)
> >      Transitions
> >    };
> > +  static const size_t kCascadeLevelCount =
> > +    static_cast<size_t>(CascadeLevel::Transitions) + 1;
> 
> This can be removed if we use an EnumeratedArray per my later comment.

Is it ok to keep this member but use an EnumeratedArray? The trouble with adding a Count/Max member to the enumeration is you end up with weaker static type checking.

For example, if you pass CascadeLevel::Count as an argument to a method taking a CascadeLevel parameter, the compiler won't complain and the call site will have to explicitly check for it. If you pass kCascadeLevelCount, however, the compiler will complain.

Likewise, if you use the enum type in a case statement, some compilers will complain if you don't have 'default' or 'case CascadeLevel::Count' branch. Suppose you add a default branch, then if we introduce another cascade level, the compiler won't warn you if you fail to update the case statement to handle the extra level. So I think if we keep the enum as only having valid values you get end up with better static type checking.

> ::: dom/animation/EffectSet.h
> @@ +7,5 @@
> >  #ifndef mozilla_EffectSet_h
> >  #define mozilla_EffectSet_h
> >  
> > +#include "mozilla/EffectCompositor.h"
> > +#include "AnimationCommon.h" // For AnimValuesStyleRule
> 
> Swap these around to keep these sorted (unless you are sorting by directory
> first, then file within directory).

Yeah, I've been grouping the mozilla/ ones first. I think I was told to do that once but I can't remember who told me that.

> @@ +152,5 @@
> > +  // style without animation, we need to not use them so that we can
> > +  // detect any new changes; if necessary we restyle immediately
> > +  // afterwards with animation.
> > +  RefPtr<AnimValuesStyleRule>
> > +    mAnimationRule[EffectCompositor::kCascadeLevelCount];
> 
> How about we use a mozilla::EnumeratedArray here instead of a C array?  That
> will let us index into it with CascadeLevel values directly without needing
> to cast to an integer type.

Done.

> @@ +324,5 @@
> > +  }
> > +
> > +  return IsAnimationManager() ?
> > +         effectSet->AnimationRule(EffectCompositor::CascadeLevel::Animations) :
> > +         effectSet->AnimationRule(EffectCompositor::CascadeLevel::Transitions);
> 
> Can you explain why we don't do the kind of check like in
> CSSTransition::CascadeLevel()?  Are all the Animation objects in the manager
> tied to markup?

This isn't quite right, but we can't do any better until bug 1232577 where we completely rewrite this function. It will be correct for all currently supported cases (we only support animations tied to markup at the moment).
  
> ::: layout/style/nsAnimationManager.cpp
> @@ -419,5 @@
> >      return nullptr;
> >    }
> >  
> >    if (collection) {
> > -    collection->mStyleRule = nullptr;
> 
> Why don't we need to do the equivalent thing on the EffectSet?

It's been a while since I wrote this patch, but I think I worked out it's not needed since:

* We call collection->mStyleRuleRefreshTime = TimeStamp(); in the following line
* After the if {} block we call collection->mStyleChanging = true;
* Then we call:
  TimeStamp refreshTime = mPresContext->RefreshDriver()->MostRecentRefresh();
  collection->EnsureStyleRuleFor(refreshTime);
* In EnsureStyleRuleFor we have the following early returns:

  1. if (!mStyleChanging) {
       mStyleRuleRefreshTime = aRefreshTime;
       return;
     }
     OK: We set mStyleChanging to true above

  2. if (!mStyleRuleRefreshTime.IsNull() &&
         mStyleRuleRefreshTime == aRefreshTime) {
       // The style rule on the EffectSet may be null and valid, if we have no
       // style to apply.
       return;
     }
     OK: We set mStyleRuleRestyleTime to the null timestamp above

  3. EffectSet* effectSet = EffectSet::GetEffectSet(mElement,
                                                    PseudoElementType());
     if (!effectSet) {
       return;
     }
     OK: If there is no effect set, there is no style rule to reset

* Then, after the early returns we run:

  RefPtr<AnimValuesStyleRule>& styleRule =
      IsForAnimations() ?
      effectSet->AnimationRule(EffectCompositor::CascadeLevel::Animations) :
      effectSet->AnimationRule(EffectCompositor::CascadeLevel::Transitions);
  styleRule = nullptr;

So it will be reset anyway.

Comment 11

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

Attachment: part 2 - Move the animation style rules from AnimationCollection to EffectSet

This is needed in order to support script-generated animations since they do not
belong to any AnimationCollection.

This patch adopts the naming "animation rule" over "style rule". Currently we
are inconsistent about this (e.g. GetAnimationRule vs EnsureStyleRuleFor).
We don't do a mass rename here but just a few places near where we're touching.
Many of the other references to "style rule" will be revised in this bug or
related bugs so we can fix those references when we come to them.

Comment 12

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

Attachment: part 2 - Move the animation style rules from AnimationCollection to EffectSet

Put EnumeratedArray include in the right file

Comment 13

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

Attachment: part 3 - Move AnimValuesStyleRule to a separate file

Rebase

Comment 14

[Cameron McCormack (:heycam)]

(In reply to Brian Birtles (:birtles, away 26 Dec~5 Jan) from comment #10)
> > > +  static const size_t kCascadeLevelCount =
> > > +    static_cast<size_t>(CascadeLevel::Transitions) + 1;
> > 
> > This can be removed if we use an EnumeratedArray per my later comment.
> 
> Is it ok to keep this member but use an EnumeratedArray? The trouble with
> adding a Count/Max member to the enumeration is you end up with weaker
> static type checking.
> [...]

OK.  FWIW the example in the comment in mfbt/EnumeratedArray.h uses an explicit Count enum value, but your reasoning for not including one makes sense to me.

Comment 15

[Cameron McCormack (:heycam)]

Comment on attachment 8701711 [details] [diff] [review]
part 4 - Add EffectCompositor::ComposeAnimationRule

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

s/moves/move/ in the commit message.

::: dom/animation/EffectCompositor.cpp
@@ +242,5 @@
> +    if (effect->GetAnimation()->CascadeLevel() == aCascadeLevel) {
> +      sortedEffectList.AppendElement(effect);
> +    }
> +  }
> +  sortedEffectList.Sort(EffectCompositeOrderComparator());

I can't find where EffectCompositeOrderComparator is defined but I'll assume it's doing what we want here. :)

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b329d19647b5
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ab3174e31a5
https://hg.mozilla.org/integration/mozilla-inbound/rev/32728ff57fcf
https://hg.mozilla.org/integration/mozilla-inbound/rev/df95b504dc3b

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b329d19647b5
https://hg.mozilla.org/mozilla-central/rev/8ab3174e31a5
https://hg.mozilla.org/mozilla-central/rev/32728ff57fcf
https://hg.mozilla.org/mozilla-central/rev/df95b504dc3b