Bug 1195180 part 6 - Lazily remove animations from timelines; r=heycam

Now that DocumentTimeline observes the refresh driver we can use regular
ticks to remove unnecessary animations.

We do this because in a subsequent patch, in order to provide deterministic
enumeration order when ticking animations, we will store animations in an array.
Removing an arbitrary element from an nsTArray is O(n) since we have to search
for the array index first, or O(log n) if we keep the array sorted. If we
destroy a subtree containing n animations, the operation effectively becomes
O(n^2), or, if we keep the array sorted, O(n log n). By destroying during a
tick when we are already iterating over the array, however, we will be able
to do this much more efficiently.

Whether an animation is newly associated with a timeline, or is disassociated
from a timeline, or if it merely has its timing updated, the behavior
implemented in this patch is to simply make sure we are observing the refresh
driver and deal with the animation on the next tick.

It might seem that we could be a lot more clever about this and, for example, if
an animation reports NeedsTicks() == false, not start observing the refresh
driver. There are various edge cases however that need to be taken into account.
For example, if a CSS animation is finished (IsRelevant() == false so that
animation will have been removed from the timeline), and paused
(NeedsTicks() == false), and we seek it back to the point where it is relevant
again, we actually need to observe the refresh driver so that it can dispatch an
animationstart event on the next tick. A test case in a subsequent patch tests
this specific situation.

We could possibly add logic to detect if we need to fire events on the next tick
but the complexity does not seem warranted given that even if we unnecessarily
start observing the refresh driver, we will stop watching it on the next tick.

This patch removes some rather lengthy comments from
AnimationTiming::UpdateTiming. This is, in part, because of the behavior
described above that makes these comments no longer relevant. Other parts are
removed because the Web Animations specification has been updated such that a
timeline becoming inactive now pauses the animation[1] so that the issue
regarding detecting timelines becoming active/inactive no longer applies
since animations attached to an inactive timeline remain "relevant".

[1] https://w3c.github.io/web-animations/#responding-to-a-newly-inactive-timeline

dom/animation/Animation.cpp

@@ -73,7 +73,7 @@ Animation::SetTimeline(AnimationTimeline* aTimeline)
   }
 
   if (mTimeline) {
-    mTimeline->RemoveAnimation(*this);
+    mTimeline->NotifyAnimationUpdated(*this);
   }
 
   mTimeline = aTimeline;
@@ -842,39 +842,8 @@ Animation::UpdateTiming(SeekFlag aSeekFlag, SyncNotifyFlag aSyncNotifyFlag)
   UpdateFinishedState(aSeekFlag, aSyncNotifyFlag);
   UpdateEffect();
 
-  // Unconditionally Add/Remove from the timeline. This is ok because if the
-  // animation has already been added/removed (which will be true more often
-  // than not) the work done by AnimationTimeline/DocumentTimeline is still
-  // negligible and its easier than trying to detect whenever we are switching
-  // to/from being relevant.
-  //
-  // We need to do this after calling UpdateEffect since it updates some
-  // cached state used by IsRelevant.
-  //
-  // Note that we only store relevant animations on the timeline since they
-  // are the only ones that need ticks and are the only ones returned from
-  // AnimationTimeline::GetAnimations. Storing any more than that would mean
-  // that we fail to garbage collect irrelevant animations since the timeline
-  // keeps a strong reference to each animation.
-  //
-  // Once we tick animations from the their timeline, and once we expect
-  // timelines to go in and out of being inactive, we will also need to store
-  // non-idle animations that are waiting for their timeline to become active
-  // on their timeline (as otherwise once the timeline becomes active it will
-  // have no way of notifying its animations). For now, however, we can
-  // simply store just the relevant animations.
   if (mTimeline) {
-    // FIXME: Once we expect animations to go back and forth betweeen being
-    // inactive and active, we will need to store more than just relevant
-    // animations on the timeline. This is because an animation might be
-    // deemed irrelevant because its timeline is inactive. If it is removed
-    // from the timeline at that point the timeline will have no way of
-    // getting the animation to add itself again once it becomes active.
-    if (IsRelevant()) {
-      mTimeline->AddAnimation(*this);
-    } else {
-      mTimeline->RemoveAnimation(*this);
-    }
+    mTimeline->NotifyAnimationUpdated(*this);
   }
 }
 

dom/animation/AnimationTimeline.cpp

@@ -34,10 +34,12 @@ AnimationTimeline::GetAnimations(AnimationSequence& aAnimations)
   for (auto iter = mAnimations.Iter(); !iter.Done(); iter.Next()) {
     Animation* animation = iter.Get()->GetKey();
 
-    MOZ_ASSERT(animation->IsRelevant(),
-               "Animations registered with a timeline should be relevant");
-    MOZ_ASSERT(animation->GetTimeline() == this,
-               "Animation should refer to this timeline");
+    // Skip animations which are no longer relevant or which have been
+    // associated with another timeline. These animations will be removed
+    // on the next tick.
+    if (!animation->IsRelevant() || animation->GetTimeline() != this) {
+      continue;
+    }
 
     // Bug 1174575: Until we implement a suitable PseudoElement interface we
     // don't have anything to return for the |target| attribute of
@@ -59,16 +61,10 @@ AnimationTimeline::GetAnimations(AnimationSequence& aAnimations)
 }
 
 void
-AnimationTimeline::AddAnimation(Animation& aAnimation)
+AnimationTimeline::NotifyAnimationUpdated(Animation& aAnimation)
 {
   mAnimations.PutEntry(&aAnimation);
 }
 
-void
-AnimationTimeline::RemoveAnimation(Animation& aAnimation)
-{
-  mAnimations.RemoveEntry(&aAnimation);
-}
-
 } // namespace dom
 } // namespace mozilla

dom/animation/AnimationTimeline.h

@@ -83,8 +83,13 @@ public:
 
   virtual TimeStamp ToTimeStamp(const TimeDuration& aTimelineTime) const = 0;
 
-  void AddAnimation(Animation& aAnimation);
-  void RemoveAnimation(Animation& aAnimation);
+  /**
+   * Inform this timeline that |aAnimation| which is or was observing the
+   * timeline, has been updated. This serves as both the means to associate
+   * AND disassociate animations with a timeline. The timeline itself will
+   * determine if it needs to begin, continue or stop tracking this animation.
+   */
+  virtual void NotifyAnimationUpdated(Animation& aAnimation);
 
 protected:
   nsCOMPtr<nsIGlobalObject> mWindow;

dom/animation/DocumentTimeline.cpp

@@ -91,6 +91,20 @@ DocumentTimeline::ToTimelineTime(const TimeStamp& aTimeStamp) const
   return result;
 }
 
+void
+DocumentTimeline::NotifyAnimationUpdated(Animation& aAnimation)
+{
+  AnimationTimeline::NotifyAnimationUpdated(aAnimation);
+
+  if (!mIsObservingRefreshDriver) {
+    nsRefreshDriver* refreshDriver = GetRefreshDriver();
+    if (refreshDriver) {
+      refreshDriver->AddRefreshObserver(this, Flush_Style);
+      mIsObservingRefreshDriver = true;
+    }
+  }
+}
+
 void
 DocumentTimeline::WillRefresh(mozilla::TimeStamp aTime)
 {
@@ -100,6 +114,14 @@ DocumentTimeline::WillRefresh(mozilla::TimeStamp aTime)
 
   for (auto iter = mAnimations.Iter(); !iter.Done(); iter.Next()) {
     Animation* animation = iter.Get()->GetKey();
+
+    // Drop any animations which no longer need to be tracked by this timeline.
+    if (animation->GetTimeline() != this ||
+        (!animation->IsRelevant() && !animation->NeedsTicks())) {
+      iter.Remove();
+      continue;
+    }
+
     needsTicks |= animation->NeedsTicks();
   }
 

dom/animation/DocumentTimeline.h

@@ -57,6 +57,8 @@ public:
                                                                      override;
   TimeStamp ToTimeStamp(const TimeDuration& aTimelineTime) const override;
 
+  void NotifyAnimationUpdated(Animation& aAnimation) override;
+
   // nsARefreshObserver methods
   void WillRefresh(TimeStamp aTime) override;