Bug 1194639 part 4 - Report changes to currentTime to animation mutation observers; r=heycam
authorBrian Birtles <birtles@gmail.com>
Thu, 22 Oct 2015 15:16:18 +0900
changeset 304106 96a15f93f1f44d030f39fa997ce0485fe24af412
parent 304105 c00dac3fdf4aaec9de1aae9275822722680f80b5
child 304107 af2e781dc33d698244c77634ee695f4ce2aa8b46
push id1001
push userraliiev@mozilla.com
push dateMon, 18 Jan 2016 19:06:03 +0000
treeherdermozilla-release@8b89261f3ac4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersheycam
bugs1194639
milestone44.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 1194639 part 4 - Report changes to currentTime to animation mutation observers; r=heycam
dom/animation/Animation.cpp
dom/animation/test/chrome/test_animation_observers.html
--- a/dom/animation/Animation.cpp
+++ b/dom/animation/Animation.cpp
@@ -188,30 +188,44 @@ Animation::GetCurrentTime() const
   }
   return result;
 }
 
 // https://w3c.github.io/web-animations/#set-the-current-time
 void
 Animation::SetCurrentTime(const TimeDuration& aSeekTime)
 {
+  // Return early if the current time has not changed. However, if we
+  // are pause-pending, then setting the current time to any value
+  // including the current value has the effect of aborting the
+  // pause so we should not return early in that case.
+  if (mPendingState != PendingState::PausePending &&
+      Nullable<TimeDuration>(aSeekTime) == GetCurrentTime()) {
+    return;
+  }
+
+  AutoMutationBatchForAnimation mb(*this);
+
   SilentlySetCurrentTime(aSeekTime);
 
   if (mPendingState == PendingState::PausePending) {
     // Finish the pause operation
     mHoldTime.SetValue(aSeekTime);
     mStartTime.SetNull();
 
     if (mReady) {
       mReady->MaybeResolve(this);
     }
     CancelPendingTasks();
   }
 
   UpdateTiming(SeekFlag::DidSeek, SyncNotifyFlag::Async);
+  if (IsRelevant()) {
+    nsNodeUtils::AnimationChanged(this);
+  }
   PostUpdate();
 }
 
 // https://w3c.github.io/web-animations/#set-the-animation-playback-rate
 void
 Animation::SetPlaybackRate(double aPlaybackRate)
 {
   if (aPlaybackRate == mPlaybackRate) {
@@ -221,19 +235,29 @@ Animation::SetPlaybackRate(double aPlayb
   AutoMutationBatchForAnimation mb(*this);
 
   Nullable<TimeDuration> previousTime = GetCurrentTime();
   mPlaybackRate = aPlaybackRate;
   if (!previousTime.IsNull()) {
     SetCurrentTime(previousTime.Value());
   }
 
+  // In the case where GetCurrentTime() returns the same result before and
+  // after updating mPlaybackRate, SetCurrentTime will return early since,
+  // as far as it can tell, nothing has changed.
+  // As a result, we need to perform the following updates here:
+  // - update timing (since, if the sign of the playback rate has changed, our
+  //   finished state may have changed),
+  // - dispatch a change notification for the changed playback rate, and
+  // - update the playback rate on animations on layers.
+  UpdateTiming(SeekFlag::DidSeek, SyncNotifyFlag::Async);
   if (IsRelevant()) {
     nsNodeUtils::AnimationChanged(this);
   }
+  PostUpdate();
 }
 
 // https://w3c.github.io/web-animations/#play-state
 AnimationPlayState
 Animation::PlayState() const
 {
   if (mPendingState != PendingState::NotPending) {
     return AnimationPlayState::Pending;
--- a/dom/animation/test/chrome/test_animation_observers.html
+++ b/dom/animation/test/chrome/test_animation_observers.html
@@ -191,22 +191,24 @@ function assert_records(expected, desc) 
     assert_records([{ added: animations, changed: [], removed: [] }],
                    "records after transition start");
 
     // Advance until near the end of the transition, then wait for it to finish.
     animations[0].currentTime = 99900;
     yield transitionEnd;
 
     // After the transition has finished, the Animation should disappear.
-    is(e.getAnimations().length, 0, "getAnimations().length after transition end");
+    is(e.getAnimations().length, 0,
+       "getAnimations().length after transition end");
 
-    // Wait for the single MutationRecord for the Animation removal to
-    // be delivered.
+    // Wait for the change MutationRecord for seeking the Animation to be
+    // delivered, followed by the the removal MutationRecord.
     yield await_frame();
-    assert_records([{ added: [], changed: [], removed: animations }],
+    assert_records([{ added: [], changed: animations, removed: [] },
+                    { added: [], changed: [], removed: animations }],
                    "records after transition end");
 
     e.style = "";
   });
 
   // Test that starting a single transition that is cancelled by resetting
   // the transition-property property dispatches an added notification and
   // then a removed notification.
@@ -328,20 +330,22 @@ function assert_records(expected, desc) 
     animations = e.getAnimations();
     is(animations.length, 1, "getAnimations().length after transition reversal");
 
     var secondAnimation = animations[0];
 
     ok(firstAnimation != secondAnimation,
        "second Animation should be different from the first");
 
-    // Wait for the single MutationRecord for the removal of the original
-    // Animation and the addition of the new Animation to be delivered.
+    // Wait for the change Mutation record from seeking the first animation
+    // to be delivered, followed by a subsequent MutationRecord for the removal
+    // of the original Animation and the addition of the new Animation.
     yield await_frame();
-    assert_records([{ added: [secondAnimation], changed: [],
+    assert_records([{ added: [], changed: [firstAnimation], removed: [] },
+                    { added: [secondAnimation], changed: [],
                       removed: [firstAnimation] }],
                    "records after transition reversal");
 
     // Cancel the transition.
     e.style.transitionProperty = "none";
 
     // Wait for the single MutationRecord for the Animation removal to
     // be delivered.
@@ -365,37 +369,46 @@ function assert_records(expected, desc) 
     is(animations.length, 3, "getAnimations().length after transition starts");
 
     // Wait for the single MutationRecord for the Animation additions to
     // be delivered.
     yield await_frame();
     assert_records([{ added: animations, changed: [], removed: [] }],
                    "records after transition starts");
 
-    // Wait for the Animations to get going, then seek well into
-    // the transitions.
+    // Wait for the Animations to get going.
     yield animations[0].ready;
+    is(animations.filter(p => p.playState == "running").length, 3,
+       "number of running Animations");
 
+    // Seek well into each animation.
     animations.forEach(p => p.currentTime = 50000);
 
-    is(animations.filter(p => p.playState == "running").length, 3, "number of running Animations");
+    // Prepare the set of expected change MutationRecords, one for each
+    // animation that was seeked.
+    var seekRecords = animations.map(
+      p => ({ added: [], changed: [p], removed: [] })
+    );
 
     // Cancel one of the transitions by setting transition-property.
     e.style.transitionProperty = "background-color, line-height";
 
     var colorAnimation  = animations.filter(p => p.playState != "running");
     var otherAnimations = animations.filter(p => p.playState == "running");
 
-    is(colorAnimation.length, 1, "number of non-running Animations after cancelling one");
-    is(otherAnimations.length, 2, "number of running Animations after cancelling one");
+    is(colorAnimation.length, 1,
+       "number of non-running Animations after cancelling one");
+    is(otherAnimations.length, 2,
+       "number of running Animations after cancelling one");
 
-    // Wait for a MutationRecord for one of the Animation
-    // removals to be delivered.
+    // Wait for the MutationRecords to be delivered: one for each animation
+    // that was seeked, followed by one for the removal of the color animation.
     yield await_frame();
-    assert_records([{ added: [], changed: [], removed: colorAnimation }],
+    assert_records(seekRecords.concat(
+                    { added: [], changed: [], removed: colorAnimation }),
                    "records after color transition end");
 
     // Cancel the remaining transitions.
     e.style.transitionProperty = "none";
 
     // Wait for the MutationRecord for the other two Animation
     // removals to be delivered.
     yield await_frame();
@@ -424,22 +437,25 @@ function assert_records(expected, desc) 
     assert_records([{ added: animations, changed: [], removed: [] }],
                    "records after animation start");
 
     // Advance until near the end of the animation, then wait for it to finish.
     animations[0].currentTime = 99900;
     yield animationEnd;
 
     // After the animation has finished, the Animation should disappear.
-    is(e.getAnimations().length, 0, "getAnimations().length after animation end");
+    is(e.getAnimations().length, 0,
+       "getAnimations().length after animation end");
 
-    // Wait for the single MutationRecord for the Animation removal to
-    // be delivered.
+    // Wait for the change MutationRecord from seeking the Animation to
+    // be delivered, followed by a further MutationRecord for the Animation
+    // removal.
     yield await_frame();
-    assert_records([{ added: [], changed: [], removed: animations }],
+    assert_records([{ added: [], changed: animations, removed: [] },
+                    { added: [], changed: [], removed: animations }],
                    "records after animation end");
 
     e.style = "";
   });
 
   // Test that starting a single animation that is cancelled by resetting
   // the animation-name property dispatches an added notification and
   // then a removed notification.
@@ -488,20 +504,22 @@ function assert_records(expected, desc) 
 
     // Advance the animation by a second.
     animations[0].currentTime += 1000;
 
     // Cancel the animation by setting animation-duration to a value less
     // than a second.
     e.style.animationDuration = "0.1s";
 
-    // Wait for the single MutationRecord for the Animation removal to
-    // be delivered.
+    // Wait for the change MutationRecord from seeking the Animation to
+    // be delivered, followed by a further MutationRecord for the Animation
+    // removal.
     yield await_frame();
-    assert_records([{ added: [], changed: [], removed: animations }],
+    assert_records([{ added: [], changed: animations, removed: [] },
+                    { added: [], changed: [], removed: animations }],
                    "records after animation end");
 
     e.style = "";
   });
 
   // Test that starting a single animation that is cancelled by updating
   // the animation-delay property dispatches an added notification and
   // then a removed notification.
@@ -550,18 +568,20 @@ function assert_records(expected, desc) 
     yield await_frame();
     assert_records([{ added: animations, changed: [], removed: [] }],
                    "records after animation start");
 
     // Advance until near the end of the animation, then wait for it to finish.
     animations[0].currentTime = 99900;
     yield animationEnd;
 
-    // No changes to the list of animations at this point.
-    assert_records([], "records after animation starts filling");
+    // The only MutationRecord at this point should be the change from
+    // seeking the Animation.
+    assert_records([{ added: [], changed: animations, removed: [] }],
+                   "records after animation starts filling");
 
     // Cancel the animation by setting animation-fill-mode.
     e.style.animationFillMode = "none";
 
     // Wait for the single MutationRecord for the Animation removal to
     // be delivered.
     yield await_frame();
     assert_records([{ added: [], changed: [], removed: animations }],
@@ -586,18 +606,21 @@ function assert_records(expected, desc) 
     // be delivered.
     yield await_frame();
     assert_records([{ added: animations, changed: [], removed: [] }],
                    "records after animation start");
 
     // Advance the animation until we are past the first iteration.
     animations[0].currentTime += 1000;
 
-    // No changes to the list of animations at this point.
-    assert_records([], "records after animation starts repeating");
+    // The only MutationRecord at this point should be the change from
+    // seeking the Animation.
+    yield await_frame();
+    assert_records([{ added: [], changed: animations, removed: [] }],
+                   "records after seeking animations");
 
     // Cancel the animation by setting animation-iteration-count.
     e.style.animationIterationCount = "1";
 
     // Wait for the single MutationRecord for the Animation removal to
     // be delivered.
     yield await_frame();
     assert_records([{ added: [], changed: [], removed: animations }],
@@ -689,16 +712,17 @@ function assert_records(expected, desc) 
     });
   });
 
   // Test that updating a property on the Animation object dispatches a changed
   // notification.
   [
     { prop: "playbackRate", val: 0.5 },
     { prop: "startTime",    val: 50000 },
+    { prop: "currentTime",  val: 50000 },
   ].forEach(function(aChangeTest) {
     addAsyncAnimTest(`single_animation_api_change_${aChangeTest.prop}`,
                      aOptions, function*() {
       // Start a long animation
       //
       // We use a forwards fill mode so that even if the change we make causes
       // the animation to become finished, it will still be "relevant" so we
       // won't mark it as removed.
@@ -734,16 +758,60 @@ function assert_records(expected, desc) 
 
       // Wait for the single removal notification.
       yield await_frame();
       assert_records([{ added: [], changed: [], removed: animations }],
                      "records after animation end");
     });
   });
 
+  // Test that making a redundant change to currentTime while an Animation
+  // is pause-pending still generates a change MutationRecord since setting
+  // the currentTime to any value in this state aborts the pending pause.
+  addAsyncAnimTest("change_currentTime_while_pause_pending", aOptions,
+    function*() {
+    // Start a long animation
+    e.style = "animation: anim 100s";
+
+    // The animation should cause the creation of a single Animation.
+    var animations = e.getAnimations();
+    is(animations.length, 1, "getAnimations().length after animation start");
+
+    // Wait for the single MutationRecord for the Animation addition to
+    // be delivered.
+    yield await_frame();
+    assert_records([{ added: animations, changed: [], removed: [] }],
+                    "records after animation start");
+
+    // Wait until the animation is playing
+    yield animations[0].ready;
+
+    // Pause the animation
+    animations[0].pause();
+
+    // We are now pause-pending. Even if we make a redundant change to the
+    // currentTime, we should still get a change record because setting the
+    // currentTime while pause-pending has the effect of cancelling a pause.
+    animations[0].currentTime = animations[0].currentTime;
+
+    // Wait for the MutationRecord from updating the currentTime to be
+    // delivered.
+    yield await_frame();
+    assert_records([{ added: [], changed: animations, removed: [] }],
+                    "records after pausing then seeking");
+
+    // Cancel the animation.
+    e.style = "";
+
+    // Wait for the single removal notification.
+    yield await_frame();
+    assert_records([{ added: [], changed: [], removed: animations }],
+                    "records after animation end");
+  });
+
   // Test that a non-cancelling change to an animation followed immediately by a
   // cancelling change will only send an animation removal notification.
   addAsyncAnimTest("coalesce_change_cancel", aOptions, function*() {
     // Start a long animation.
     e.style = "animation: anim 100s;";
 
     // The animation should cause the creation of a single Animation.
     var animations = e.getAnimations();
@@ -828,28 +896,42 @@ addAsyncAnimTest("tree_ordering", { obse
                  "records after simultaneous animation start");
 
   // The one case where we *do* currently perform document-level (or actually
   // timeline-level) batching is when animations are updated from a refresh
   // driver tick. In particular, this means that when animations finish
   // naturally the removed records should be dispatched according to the
   // position of the elements in the tree.
   //
+
+  // First, flatten the set of animations.
+  var animations = [ divAnimations,
+                     childAAnimations,
+                     childBAnimations ].reduce(
+    (a, b) => a.concat(b), []
+  );
+
   // Fast-forward to *just* before the end of the animation.
-  var fastForward = animation => animation.currentTime = 99999;
-  divAnimations.forEach(fastForward);
-  childAAnimations.forEach(fastForward);
-  childBAnimations.forEach(fastForward);
+  animations.forEach(animation => animation.currentTime = 99999);
+
+  // Prepare the set of expected change MutationRecords, one for each
+  // animation that was seeked.
+  var seekRecords = animations.map(
+    p => ({ added: [], changed: [p], removed: [] })
+  );
 
   yield await_event(div, "animationend");
 
-  // We should get records in order div, childA, childB
-  assert_records([{ added: [], changed: [], removed: divAnimations },
-                  { added: [], changed: [], removed: childAAnimations },
-                  { added: [], changed: [], removed: childBAnimations }],
+  // After the changed notifications, which will be dispatched in the order that
+  // the animations were seeked, we should get removal MutationRecords in order
+  // div, childA, childB
+  assert_records(seekRecords.concat(
+                   { added: [], changed: [], removed: divAnimations },
+                   { added: [], changed: [], removed: childAAnimations },
+                   { added: [], changed: [], removed: childBAnimations }),
                  "records after finishing");
 
   // Clean up
   div.style = "";
   childA.remove();
   childB.remove();
 });