Bug 1194028 - Part 2: Use KeyFrameEffect SetTiming. r=bbirtles
authorHiroyuki Ikezoe <hiikezoe@mozilla-japan.org>
Tue, 18 Aug 2015 03:57:00 -0400
changeset 258389 2ffca89296dea559cdfc32820c1271c2e80239a8
parent 258388 a8bf7a483a20b79f1c958eae477d1930e34ad994
child 258390 f4091e0478f86c1adb8fbcce0bddd8346b285e4c
push id63897
push userryanvm@gmail.com
push dateWed, 19 Aug 2015 12:14:35 +0000
treeherdermozilla-inbound@f4091e0478f8 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbbirtles
bugs1194028
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 1194028 - Part 2: Use KeyFrameEffect SetTiming. r=bbirtles Now KeyframeEffect.SetTiming() updates the owning animation timing and relavance, so we don't need to call each methods respectively for the animation any more.
dom/animation/Animation.cpp
layout/style/nsAnimationManager.cpp
--- a/dom/animation/Animation.cpp
+++ b/dom/animation/Animation.cpp
@@ -46,19 +46,19 @@ 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 == aEffect) {
+    return;
+  }
   if (mEffect) {
     mEffect->SetParentTime(Nullable<TimeDuration>());
   }
   mEffect = aEffect;
   if (mEffect) {
     mEffect->SetParentTime(GetCurrentTime());
   }
 
--- a/layout/style/nsAnimationManager.cpp
+++ b/layout/style/nsAnimationManager.cpp
@@ -450,24 +450,18 @@ nsAnimationManager::CheckAnimationRule(n
         // 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->SetTiming(newEffect->Timing(), *oldAnim);
           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) {
@@ -502,20 +496,16 @@ nsAnimationManager::CheckAnimationRule(n
         //
         // Although we're doing this while iterating this is safe because
         // we're not changing the length of newAnimations and we've finished
         // iterating over the list of old iterations.
         newAnim->CancelFromStyle();
         newAnim = nullptr;
         newAnimations.ReplaceElementAt(newIdx, oldAnim);
         collection->mAnimations.RemoveElementAt(oldIdx);
-
-        // 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