Bug 927349 part 22 - Make AnimationPlayer wait for animations to be rendered before starting; r=jwatt
authorBrian Birtles <birtles@gmail.com>
Thu, 25 Dec 2014 16:28:24 +0900
changeset 221985 6a4a22732aed7933fcd6af8e55c10a2fb762877b
parent 221984 30bec23b21b32e3964738bdbf1811c7d7c5aa4d3
child 221986 37343347ab6a63330a335a6745dac1f093b6d902
push id28055
push userkwierso@gmail.com
push dateTue, 06 Jan 2015 00:19:38 +0000
treeherdermozilla-central@72d7ae169b09 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjwatt
bugs927349
milestone37.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 927349 part 22 - Make AnimationPlayer wait for animations to be rendered before starting; r=jwatt This patch (finally!) introduces the delayed start behavior. It updates AnimationPlayer::DoPlay to put animations in the PendingPlayerTracker from where they are triggered. This patch also updates nsTransitionManager to set the animation's source before calling Play as otherwise the AnimationPlayer won't be able to access the pending player tracker (which it locates by navigating AnimationPlayer -> Animation (source content) -> target element -> composed doc -> pending player tracker). In future, when we support setting the AnimationPlayer.source property we will make this more robust so that the order in which these steps are performed doesn't matter. This patch also updates a couple of tests to reflect the fact that AnimationPlayer will now return the pending state.
dom/animation/AnimationPlayer.cpp
dom/animation/AnimationPlayer.h
dom/animation/test/css-animations/test_animation-player-playstate.html
layout/style/nsTransitionManager.cpp
--- a/dom/animation/AnimationPlayer.cpp
+++ b/dom/animation/AnimationPlayer.cpp
@@ -128,35 +128,55 @@ AnimationPlayer::SetSource(Animation* aS
   if (mSource) {
     mSource->SetParentTime(GetCurrentTime());
   }
 }
 
 void
 AnimationPlayer::Tick()
 {
+  // FIXME (bug 1112969): Check if we are pending but have lost access to the
+  // pending player tracker. If that's the case we should probably trigger the
+  // animation now.
+
   UpdateSourceContent();
 }
 
 void
 AnimationPlayer::StartNow()
 {
-  // Currently we only expect this method to be called when we are in the
-  // middle of initiating/resuming playback so we should have an unresolved
-  // start time to update and a fixed current time to seek to.
-  MOZ_ASSERT(mStartTime.IsNull() && !mHoldTime.IsNull(),
-             "Resolving the start time but we don't appear to be waiting"
-             " to begin playback");
+  // This method is only expected to be called for an animation that is
+  // waiting to play. We can easily adapt it to handle other states
+  // but it's currently not necessary.
+  MOZ_ASSERT(PlayState() == AnimationPlayState::Pending,
+             "Expected to start a pending player");
+  MOZ_ASSERT(!mHoldTime.IsNull(),
+             "A player in the pending state should have a resolved hold time");
 
   Nullable<TimeDuration> readyTime = mTimeline->GetCurrentTime();
-  // Bug 1096776: Once we support disappearing or inactive timelines we
-  // will need special handling here.
+
+  // FIXME (bug 1096776): If readyTime.IsNull(), we should return early here.
+  // This will leave mIsPending = true but the caller will remove us from the
+  // PendingPlayerTracker if we were added there.
+  // Then, in Tick(), if we have:
+  // - a resolved timeline, and
+  // - mIsPending = true, and
+  // - *no* document or we are *not* in the PendingPlayerTracker
+  // then we should call StartNow.
+  //
+  // For now, however, we don't support inactive/missing timelines so
+  // |readyTime| should be resolved.
   MOZ_ASSERT(!readyTime.IsNull(), "Missing or inactive timeline");
+
   mStartTime.SetValue(readyTime.Value() - mHoldTime.Value());
   mHoldTime.SetNull();
+  mIsPending = false;
+
+  UpdateSourceContent();
+  PostUpdate();
 
   if (mReady) {
     mReady->MaybeResolve(this);
   }
 }
 
 void
 AnimationPlayer::Cancel()
@@ -241,17 +261,37 @@ AnimationPlayer::DoPlay()
   } else if (mHoldTime.IsNull()) {
     // If the hold time is null, we are already playing normally
     return;
   }
 
   // Clear ready promise. We'll create a new one lazily.
   mReady = nullptr;
 
-  StartNow();
+  mIsPending = true;
+
+  nsIDocument* doc = GetRenderedDocument();
+  if (!doc) {
+    // If we have no rendered document (e.g. because the source content's
+    // target element is orphaned), then treat the animation as ready and
+    // start it immediately. It is probably preferable to make playing
+    // *always* asynchronous (e.g. by setting some additional state that
+    // marks this player as pending and queueing a runnable to resolve the
+    // start time). That situation, however, is currently rare enough that
+    // we don't bother for now.
+    StartNow();
+    return;
+  }
+
+  PendingPlayerTracker* tracker = doc->GetOrCreatePendingPlayerTracker();
+  tracker->AddPlayPending(*this);
+
+  // We may have updated the current time when we set the hold time above
+  // so notify source content.
+  UpdateSourceContent();
 }
 
 void
 AnimationPlayer::DoPause()
 {
   if (mIsPending) {
     CancelPendingPlay();
     // Resolve the ready promise since we currently only use it for
--- a/dom/animation/AnimationPlayer.h
+++ b/dom/animation/AnimationPlayer.h
@@ -90,18 +90,37 @@ public:
   // PauseFromJS is currently only here for symmetry with PlayFromJS but
   // in future we will likely have to flush style in
   // CSSAnimationPlayer::PauseFromJS so we leave it for now.
   void PauseFromJS() { Pause(); }
 
   void SetSource(Animation* aSource);
   void Tick();
 
-  // Sets the start time of a player that is waiting to play to the current
-  // time of its timeline.
+  /**
+   * Sets the start time of a player that is waiting to play to the current
+   * time of its timeline.
+   *
+   * This will reset the pending flag if the call succeeded. The caller is
+   * responsible for removing the player from the PendingPlayerTracker though.
+   *
+   * Typically, when a player is played, it does not start immediately but is
+   * added to a table of pending players on the document of its source content.
+   * In the meantime it sets its hold time to the time from which should
+   * begin playback.
+   *
+   * When the document finishes painting, any pending players in its table
+   * are started by calling this method.
+   *
+   * This approach means that any setup time required for performing the
+   * initial paint of an animation such as layerization is not deducted from
+   * the running time of the animation. Without this we can easily drop the
+   * first few frames of an animation, or, on slower devices, the whole
+   * animation.
+   */
   void StartNow();
   void Cancel();
 
   const nsString& Name() const {
     return mSource ? mSource->Name() : EmptyString();
   }
 
   bool IsPaused() const { return PlayState() == AnimationPlayState::Paused; }
--- a/dom/animation/test/css-animations/test_animation-player-playstate.html
+++ b/dom/animation/test/css-animations/test_animation-player-playstate.html
@@ -11,18 +11,17 @@
 'use strict';
 
 async_test(function(t) {
   var div = addDiv(t);
   var cs = window.getComputedStyle(div);
   div.style.animation = 'anim 1000s';
 
   var player = div.getAnimationPlayers()[0];
-  assert_equals(player.playState, 'running');
-  // Bug 927349: Check for pending state here
+  assert_equals(player.playState, 'pending');
 
   player.ready.then(t.step_func(function() {
     assert_equals(player.playState, 'running');
     t.done();
   }));
 }, 'Player returns correct playState when running');
 
 test(function(t) {
@@ -47,13 +46,14 @@ test(function(t) {
 test(function(t) {
   var div = addDiv(t);
   var cs = window.getComputedStyle(div);
   div.style.animation = 'anim 1000s paused';
 
   var player = div.getAnimationPlayers()[0];
   div.style.animationPlayState = 'running';
   // This test also checks that calling playState flushes style
-  // Bug 927349: Make this check for 'pending'
-  assert_equals(player.playState, 'running');
+  assert_equals(player.playState, 'pending',
+                'Player.playState reports pending after updating'
+                + ' animation-play-state (got: ' + player.playState + ')');
 }, 'Player.playState updates when resumed by setting style');
 
 </script>
--- a/layout/style/nsTransitionManager.cpp
+++ b/layout/style/nsTransitionManager.cpp
@@ -535,18 +535,22 @@ nsTransitionManager::ConsiderStartingTra
   AnimationPropertySegment& segment = *prop.mSegments.AppendElement();
   segment.mFromValue = startValue;
   segment.mToValue = endValue;
   segment.mFromKey = 0;
   segment.mToKey = 1;
   segment.mTimingFunction.Init(tf);
 
   nsRefPtr<CSSTransitionPlayer> player = new CSSTransitionPlayer(timeline);
+  // The order of the following two calls is important since PlayFromStyle
+  // will add the player to the PendingPlayerTracker of its source content's
+  // document. When we come to make source writeable (bug 1049975) we should
+  // remove this dependency.
+  player->SetSource(pt);
   player->PlayFromStyle();
-  player->SetSource(pt);
 
   if (!aElementTransitions) {
     aElementTransitions =
       GetAnimationPlayers(aElement, aNewStyleContext->GetPseudoType(), true);
     if (!aElementTransitions) {
       NS_WARNING("allocating CommonAnimationManager failed");
       return;
     }