Bug 1298742 - Part 2: Make sure UpdateRelevance() is called before NotifyAnimationTimingUpdated. r=hiro
authorBoris Chiou <boris.chiou@gmail.com>
Mon, 29 Aug 2016 16:22:46 +0800
changeset 312139 0c391afe7f0a76ddf58296677e70a0175eb72263
parent 312138 a97ed883028af807f6ea36a4e84fdc3e20dc010b
child 312140 ddf122a7200b0f87d50b3ba959d34db1ecfefa59
push id81294
push userryanvm@gmail.com
push dateThu, 01 Sep 2016 02:34:56 +0000
treeherdermozilla-inbound@a7ef41c19c53 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewershiro
bugs1298742
milestone51.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 1298742 - Part 2: Make sure UpdateRelevance() is called before NotifyAnimationTimingUpdated. r=hiro MozReview-Commit-ID: Ki0Aqhgl1dO
dom/animation/Animation.cpp
dom/animation/KeyframeEffect.cpp
layout/reftests/web-animations/1298742-1.html
layout/reftests/web-animations/1298742-ref.html
layout/reftests/web-animations/reftest.list
--- a/dom/animation/Animation.cpp
+++ b/dom/animation/Animation.cpp
@@ -169,24 +169,26 @@ Animation::SetEffectNoUpdate(AnimationEf
   if (aEffect) {
     // Break links from the new effect to its previous animation, if any.
     RefPtr<AnimationEffectReadOnly> newEffect = aEffect;
     Animation* prevAnim = aEffect->GetAnimation();
     if (prevAnim) {
       prevAnim->SetEffect(nullptr);
     }
 
-    // Create links with the new effect.
+    // Create links with the new effect. SetAnimation(this) will also update
+    // mIsRelevant of this animation, and then notify mutation observer if
+    // needed by calling Animation::UpdateRelevance(), so we don't need to
+    // call it again.
     mEffect = newEffect;
     mEffect->SetAnimation(this);
 
-    // Update relevance and then notify possible add or change.
+    // Notify possible add or change.
     // If the target is different, the change notification will be ignored by
     // AutoMutationBatchForAnimation.
-    UpdateRelevance();
     if (wasRelevant && mIsRelevant) {
       nsNodeUtils::AnimationChanged(this);
     }
 
     // Reschedule pending pause or pending play tasks.
     // If we have a pending animation, it will either be registered
     // in the pending animation tracker and have a null pending ready time,
     // or, after it has been painted, it will be removed from the tracker
--- a/dom/animation/KeyframeEffect.cpp
+++ b/dom/animation/KeyframeEffect.cpp
@@ -1212,19 +1212,27 @@ KeyframeEffectReadOnly::SetAnimation(Ani
   // Restyle for the old animation.
   RequestRestyle(EffectCompositor::RestyleType::Layer);
 
   mAnimation = aAnimation;
 
   // Restyle for the new animation.
   RequestRestyle(EffectCompositor::RestyleType::Layer);
 
-  MarkCascadeNeedsUpdate();
-
+  // The order of these function calls is important:
+  // NotifyAnimationTimingUpdated() need the updated mIsRelevant flag to check
+  // if it should create the effectSet or not, and MarkCascadeNeedsUpdate()
+  // needs a valid effectSet, so we should call them in this order.
+  if (mAnimation) {
+    mAnimation->UpdateRelevance();
+  }
   NotifyAnimationTimingUpdated();
+  if (mAnimation) {
+    MarkCascadeNeedsUpdate();
+  }
 }
 
 bool
 KeyframeEffectReadOnly::CanIgnoreIfNotVisible() const
 {
   if (!AnimationUtils::IsOffscreenThrottlingEnabled()) {
     return false;
   }
new file mode 100644
--- /dev/null
+++ b/layout/reftests/web-animations/1298742-1.html
@@ -0,0 +1,32 @@
+<!DOCTYPE html>
+<html class="reftest-wait reftest-no-flush">
+<title>Bug 1298742</title>
+<style>
+div {
+  width: 100px;
+  height: 100px;
+  background: blue;
+  transform: translate(10px);
+}
+</style>
+<div id="target"></div>
+<script>
+  const MS_PER_SEC = 1000;
+  var elem = document.getElementById("target");
+  var animA =
+    elem.animate({ transform: [ 'translate(0px)', 'translate(60px)' ] },
+                 100 * MS_PER_SEC);
+  var animB =
+    elem.animate({ transform: [ 'translate(60px)', 'translate(0px)' ] },
+                 100 * MS_PER_SEC);
+  animA.pause();
+  animB.pause();
+
+  Promise.all([animA.ready, animB.ready]).then(function() {
+    animB.effect = null;
+    requestAnimationFrame(function() {
+      document.documentElement.classList.remove("reftest-wait");
+    });
+  });
+</script>
+</html>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/web-animations/1298742-ref.html
@@ -0,0 +1,13 @@
+<!DOCTYPE html>
+<html>
+<title>Reference of bug 1298742</title>
+<style>
+div {
+  width: 100px;
+  height: 100px;
+  background: blue;
+  transform: translate(0px);
+}
+</style>
+<div></div>
+</html>
--- a/layout/reftests/web-animations/reftest.list
+++ b/layout/reftests/web-animations/reftest.list
@@ -1,10 +1,11 @@
 test-pref(dom.animations-api.core.enabled,true) == 1246046-1.html green-box.html
 test-pref(dom.animations-api.core.enabled,true) == 1267937-1.html 1267937-ref.html
+test-pref(dom.animations-api.core.enabled,true) == 1298742-1.html 1298742-ref.html
 test-pref(dom.animations-api.core.enabled,true) == stacking-context-transform-none-animation-before-appending-element.html stacking-context-animation-ref.html
 test-pref(dom.animations-api.core.enabled,true) == stacking-context-opacity-changing-keyframe.html stacking-context-animation-ref.html
 test-pref(dom.animations-api.core.enabled,true) == stacking-context-opacity-changing-target.html stacking-context-animation-changing-target-ref.html
 test-pref(dom.animations-api.core.enabled,true) == stacking-context-opacity-changing-effect.html stacking-context-animation-ref.html
 test-pref(dom.animations-api.core.enabled,true) == stacking-context-transform-changing-keyframe.html stacking-context-animation-ref.html
 test-pref(dom.animations-api.core.enabled,true) == stacking-context-transform-changing-target.html stacking-context-animation-changing-target-ref.html
 test-pref(dom.animations-api.core.enabled,true) == stacking-context-transform-changing-effect.html stacking-context-animation-ref.html
 test-pref(dom.animations-api.core.enabled,true) == stacking-context-transform-changing-display-property.html stacking-context-animation-ref.html