Bug 1188251 part 8 - Remove call to Animation::Tick from CheckAnimationRule; r=dholbert
authorBrian Birtles <birtles@gmail.com>
Mon, 17 Aug 2015 13:59:45 +0900
changeset 258002 2eddf957653d6d1a790ac681a8d86282295e4526
parent 258001 2be8060a496921b76fad7c48c47bf4ff195d6401
child 258003 2526fbcbe37a5eae117ce258e8a872a7f8ae4830
push id29238
push userryanvm@gmail.com
push dateMon, 17 Aug 2015 13:06:57 +0000
treeherdermozilla-central@a6eeb28458fd [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdholbert
bugs1188251, 1134163
milestone43.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1188251 part 8 - Remove call to Animation::Tick from CheckAnimationRule; r=dholbert We want to move the newly-introduced RequestRestyle call from FlushAnimations to Animation::Tick. However, nsAnimationManager::CheckAnimationRule calls Animation::Tick so this would cause us to start posting animation restyles within a restyle. Typically, Animations have an effect (currently there is only one type of effect: KeyframeEffectReadOnly) and when there is any change in timing they pass it down to their effect. However, the Animation is dependent on the duration of the effect for determining if it is "finished" or not. As a result, when an effect's timing changes, the owning Animation needs to know. (The way this *should* work is that effects should tell their animation or trigger some chain of events that causes animation's to update themselves. However, the current implementation of effects is fairly primitive and does not do this or even have a reference to the owning Animation. When we implement the script API for updating the timing properties of effects we will have to fix this but for now it is up to code in layout/style to update the Animation when it touches the corresponding effect's timing.) nsAnimationManager::CheckAnimationRule currently does this by calling Animation::Tick() which ensures the Animation's finished state is updated accordingly. Ultimately we want to ensure that Animation::Tick is called exactly once per frame (and at the appropriate point in that frame) so we'd like to remove this call from CheckAnimationRule. This patch achieves that by: * Making Animation::SetEffect update the animation's timing - this is necessary for animations that are created by CheckAnimationRule and will be necessary when once we make Animation.effect writeable from script anyway. * Calling Animation::SetEffect even for the case when we are updating the existing effect. Another side-effect of calling Animation::Tick within nsAnimationManager::CheckAnimationRule is that CSSAnimation::Tick queues events. There are some tests (e.g. layout/style/test/test_animations.html) that assume that animationstart events are dispatched immediately when new animations are created. That will change with bug 1134163 but for now we should maintain this existing behavior since changing this might introduce compatibility issues that are best dealt with as a separate bug rather than blocking this refactoring. To that end, this patch also explicitly queues animationstart events for newly-created animations.
dom/animation/Animation.cpp
layout/style/nsAnimationManager.cpp
layout/style/nsAnimationManager.h
layout/style/test/test_animations.html
--- a/dom/animation/Animation.cpp
+++ b/dom/animation/Animation.cpp
@@ -46,24 +46,28 @@ Animation::WrapObject(JSContext* aCx, JS
 //
 // Animation interface:
 //
 // ---------------------------------------------------------------------------
 
 void
 Animation::SetEffect(KeyframeEffectReadOnly* aEffect)
 {
+  // FIXME: We should perform an early return if aEffect == mEffect but
+  // current nsAnimationManager::CheckAnimationRule is relying on this
+  // method updating timing even in that case.
   if (mEffect) {
     mEffect->SetParentTime(Nullable<TimeDuration>());
   }
   mEffect = aEffect;
   if (mEffect) {
     mEffect->SetParentTime(GetCurrentTime());
   }
-  UpdateRelevance();
+
+  UpdateTiming(SeekFlag::NoSeek, SyncNotifyFlag::Async);
 }
 
 void
 Animation::SetTimeline(AnimationTimeline* aTimeline)
 {
   if (mTimeline == aTimeline) {
     return;
   }
--- a/layout/style/nsAnimationManager.cpp
+++ b/layout/style/nsAnimationManager.cpp
@@ -430,31 +430,44 @@ nsAnimationManager::CheckAnimationRule(n
                         " be CSSAnimation objects");
           if (a->AnimationName() ==
               newAnim->AsCSSAnimation()->AnimationName()) {
             oldAnim = a;
             break;
           }
         }
         if (!oldAnim) {
+          // FIXME: Bug 1134163 - We shouldn't queue animationstart events
+          // until the animation is actually ready to run. However, we
+          // currently have some tests that assume that these events are
+          // dispatched within the same tick as the animation is added
+          // so we need to queue up any animationstart events from newly-created
+          // animations.
+          newAnim->AsCSSAnimation()->QueueEvents();
           continue;
         }
 
         bool animationChanged = false;
 
         // Update the old from the new so we can keep the original object
         // identity (and any expando properties attached to it).
         if (oldAnim->GetEffect() && newAnim->GetEffect()) {
           KeyframeEffectReadOnly* oldEffect = oldAnim->GetEffect();
           KeyframeEffectReadOnly* newEffect = newAnim->GetEffect();
           animationChanged =
             oldEffect->Timing() != newEffect->Timing() ||
             oldEffect->Properties() != newEffect->Properties();
           oldEffect->Timing() = newEffect->Timing();
           oldEffect->Properties() = newEffect->Properties();
+          // FIXME: Currently assigning to KeyframeEffect::Timing() does not
+          // update the corresponding Animation (which may, for example, no
+          // longer be finished). Until we introduce proper setters for
+          // properties on effects, we need to manually cause the owning
+          // Animation to update its timing by setting the effect again.
+          oldAnim->SetEffect(oldEffect);
         }
 
         // Reset compositor state so animation will be re-synchronized.
         oldAnim->ClearIsRunningOnCompositor();
 
         // Handle changes in play state. If the animation is idle, however,
         // changes to animation-play-state should *not* restart it.
         if (oldAnim->PlayState() != AnimationPlayState::Idle) {
@@ -472,17 +485,20 @@ nsAnimationManager::CheckAnimationRule(n
                     !newAnim->IsPausedOrPausing()) {
             oldAnim->PlayFromStyle();
             animationChanged = true;
           }
         }
 
         oldAnim->CopyAnimationIndex(*newAnim->AsCSSAnimation());
 
-        if (animationChanged) {
+        // Updating the effect timing above might already have caused the
+        // animation to become irrelevant so only add a changed record if
+        // the animation is still relevant.
+        if (animationChanged && oldAnim->IsRelevant()) {
           nsNodeUtils::AnimationChanged(oldAnim);
         }
 
         // Replace new animation with the (updated) old one and remove the
         // old one from the array so we don't try to match it any more.
         //
         // Although we're doing this while iterating this is safe because
         // we're not changing the length of newAnimations and we've finished
@@ -495,20 +511,25 @@ nsAnimationManager::CheckAnimationRule(n
         // We've touched the old animation's timing properties, so this
         // could update the old animation's relevance.
         oldAnim->UpdateRelevance();
       }
     }
   } else {
     collection =
       GetAnimations(aElement, aStyleContext->GetPseudoType(), true);
+    for (Animation* animation : newAnimations) {
+      // FIXME: Bug 1134163 - As above, we have shouldn't actually need to
+      // queue events here. (But we do for now since some tests expect
+      // animationstart events to be dispatched immediately.)
+      animation->AsCSSAnimation()->QueueEvents();
+    }
   }
   collection->mAnimations.SwapElements(newAnimations);
   collection->mNeedsRefreshes = true;
-  collection->Tick();
 
   // Cancel removed animations
   for (size_t newAnimIdx = newAnimations.Length(); newAnimIdx-- != 0; ) {
     newAnimations[newAnimIdx]->CancelFromStyle();
   }
 
   UpdateCascadeResults(aStyleContext, collection);
 
--- a/layout/style/nsAnimationManager.h
+++ b/layout/style/nsAnimationManager.h
@@ -95,16 +95,17 @@ public:
   void CancelFromStyle() override
   {
     mOwningElement = OwningElementRef();
     Animation::CancelFromStyle();
     MOZ_ASSERT(mSequenceNum == kUnsequenced);
   }
 
   void Tick() override;
+  void QueueEvents();
 
   bool IsStylePaused() const { return mIsStylePaused; }
 
   bool HasLowerCompositeOrderThan(const Animation& aOther) const override;
   bool IsUsingCustomCompositeOrder() const override
   {
     return mOwningElement.IsSet();
   }
@@ -158,18 +159,16 @@ public:
 protected:
   virtual ~CSSAnimation()
   {
     MOZ_ASSERT(!mOwningElement.IsSet(), "Owning element should be cleared "
                                         "before a CSS animation is destroyed");
   }
   virtual CommonAnimationManager* GetAnimationManager() const override;
 
-  void QueueEvents();
-
   nsString mAnimationName;
 
   // The (pseudo-)element whose computed animation-name refers to this
   // animation (if any).
   OwningElementRef mOwningElement;
 
   // When combining animation-play-state with play() / pause() the following
   // behavior applies:
--- a/layout/style/test/test_animations.html
+++ b/layout/style/test/test_animations.html
@@ -1199,27 +1199,27 @@ advance_clock(380);
 is_approx(px_to_num(cs.marginRight), 100 * gTF.ease_in(0.02), 0.01,
           "large negative delay test at 380ms");
 check_events([]);
 advance_clock(20);
 is(cs.marginRight, "0px", "large negative delay test at 400ms");
 check_events([{ type: 'animationiteration', target: div,
                 animationName: 'anim2', elapsedTime: 4.0,
                 pseudoElement: "" }],
-             "right after start in large negative delay test");
+             "large negative delay test at 400ms");
 advance_clock(800);
 is_approx(px_to_num(cs.marginRight), 100 * gTF.ease_in(0.8), 0.01,
           "large negative delay test at 1200ms");
 check_events([]);
 advance_clock(200);
 is(cs.marginRight, "100px", "large negative delay test at 1400ms");
 check_events([{ type: 'animationend', target: div,
                 animationName: 'anim2', elapsedTime: 5.0,
                 pseudoElement: "" }],
-             "right after start in large negative delay test");
+             "large negative delay test at 1400ms");
 done_div();
 
 /*
  * css3-animations:  3.9. The 'animation-fill-mode' Property
  * http://dev.w3.org/csswg/css3-animations/#the-animation-fill-mode-property-
  */
 
 // animation-fill-mode is tested in the tests for section (2).