Closed Bug 1025709 Opened 8 years ago Closed 8 years ago

Merge ElementTransitions with CommonElementAnimationData

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: birtles, Assigned: birtles)

References

Details

Attachments

(13 files)

11.94 KB, patch
heycam
: review+
Details | Diff | Splinter Review
14.16 KB, patch
heycam
: review+
Details | Diff | Splinter Review
10.70 KB, patch
heycam
: review+
Details | Diff | Splinter Review
25.43 KB, patch
heycam
: review+
Details | Diff | Splinter Review
7.97 KB, patch
heycam
: review+
Details | Diff | Splinter Review
5.28 KB, patch
heycam
: review+
Details | Diff | Splinter Review
11.13 KB, patch
heycam
: review+
Details | Diff | Splinter Review
16.94 KB, patch
heycam
: review+
Details | Diff | Splinter Review
6.79 KB, patch
heycam
: review+
Details | Diff | Splinter Review
5.43 KB, patch
heycam
: review+
Details | Diff | Splinter Review
3.68 KB, patch
heycam
: review+
Details | Diff | Splinter Review
17.36 KB, patch
heycam
: review+
Details | Diff | Splinter Review
6.92 KB, patch
heycam
: review+
Details | Diff | Splinter Review
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.
Attachment #8440498 - Flags: review?(cam)
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.
Attachment #8440499 - Flags: review?(cam)
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.
Attachment #8440500 - Flags: review?(cam)
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.
Attachment #8440501 - Flags: review?(cam)
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.
Attachment #8440503 - Flags: review?(cam)
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.
Attachment #8440504 - Flags: review?(cam)
This patch still leaves ElementAnimations|
ElementTransitions::GetAnimationsForCompositor as shortcuts
for the method now defined on CommonElementAnimationData.
Attachment #8440506 - Flags: review?(cam)
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.
Attachment #8440507 - Flags: review?(cam)
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.
Attachment #8440508 - Flags: review?(cam)
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.
Attachment #8440509 - Flags: review?(cam)
Now that animations and transitions are largely treated alike, we can remove
some redundant code in the processing of transform animations in
ComputeSuitableScaleForAnimation.
Attachment #8440510 - Flags: review?(cam)
Blocks: 999922
Blocks: 1026302
Attachment #8440498 - Flags: review?(cam) → review+
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?
Attachment #8440499 - Flags: review?(cam) → review+
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.
Attachment #8440500 - Flags: review?(cam) → review+
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.
Attachment #8440501 - Flags: review?(cam) → review+
Attachment #8440502 - Flags: review?(cam) → review+
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.
Attachment #8440503 - Flags: review?(cam) → review+
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.)
Attachment #8440504 - Flags: review?(cam) → review+
Attachment #8440505 - Flags: review?(cam) → review+
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.
Attachment #8440506 - Flags: review?(cam) → review+
Attachment #8440507 - Flags: review?(cam) → review+
Attachment #8440508 - Flags: review?(cam) → review+
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.
Attachment #8440509 - Flags: review?(cam) → review+
Attachment #8440510 - Flags: review?(cam) → review+
(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).
Depends on: 1028036
Depends on: 1038032
Depends on: 1045906
Depends on: 1048838
Blocks: 1061364
You need to log in before you can comment on or make changes to this bug.