Web-animation: Remove some uses of PlayStateUpdateScope.

PlayStateUpdateScope is problematic for handling timing of finished
promises in addition to not conforming with the algorithm outlined in
the web-animations spec. This patch removes several uses of
PlayStateUpdateScope and refines handling of pending flags.

Bug: 960944
Change-Id: I093a3aa86084adfbe128a5aedd36206d67dd12c8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1851927
Commit-Queue: Kevin Ellis <kevers@chromium.org>
Reviewed-by: Stephen McGruer <smcgruer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#705966}

third_party/blink/renderer/core/animation/animation.cc

@@ -408,8 +408,6 @@ double Animation::currentTime(bool& is_null) {
 
 // https://drafts.csswg.org/web-animations/#the-current-time-of-an-animation
 double Animation::currentTime() {
-  PlayStateUpdateScope update_scope(*this, kTimingUpdateOnDemand);
-
   // 1. If the animation’s hold time is resolved,
   //    The current time is the animation’s hold time.
   if (hold_time_.has_value())
@@ -439,17 +437,7 @@ double Animation::currentTime() {
 }
 
 double Animation::CurrentTimeInternal() const {
-  double result = hold_time_.value_or(CalculateCurrentTime());
-#if DCHECK_IS_ON()
-  // We can't enforce this check during Unset due to other assertions.
-  if (internal_play_state_ != kUnset) {
-    const_cast<Animation*>(this)->UpdateCurrentTimingState(
-        kTimingUpdateOnDemand);
-    double hold_or_current_time = hold_time_.value_or(CalculateCurrentTime());
-    DCHECK(AreEqualOrNull(result, hold_or_current_time));
-  }
-#endif
-  return result;
+  return hold_time_.value_or(CalculateCurrentTime());
 }
 
 double Animation::UnlimitedCurrentTimeInternal() const {
@@ -465,9 +453,6 @@ bool Animation::PreCommit(
     int compositor_group,
     const PaintArtifactCompositor* paint_artifact_compositor,
     bool start_on_compositor) {
-  PlayStateUpdateScope update_scope(*this, kTimingUpdateOnDemand,
-                                    kDoNotSetCompositorPending);
-
   bool soft_change =
       compositor_state_ &&
       (Paused() || compositor_state_->playback_rate != playback_rate_);
@@ -496,9 +481,8 @@ bool Animation::PreCommit(
 
   DCHECK(!compositor_state_ || compositor_state_->start_time);
 
-  if (!should_start) {
+  if (!should_start)
     current_time_pending_ = false;
-  }
 
   if (should_start) {
     compositor_group_ = compositor_group;
@@ -517,13 +501,24 @@ bool Animation::PreCommit(
     }
   }
 
+  // Apply updates if not waiting on a start time. Play pending animations that
+  // are waiting on a start time will get updated from NotifyStartTime.
+  // Finished animations may not trigger an Update call if finished in the same
+  // frame that they are started. Calling ApplyUpdates for this specific case
+  // ensures that the finished promise and event are properly triggered.
+  AnimationPlayState play_state = CalculateAnimationPlayState();
+  bool finished = play_state == kFinished;
+  if (pending_pause_ || (pending_play_ && (start_time_ || finished))) {
+    ApplyUpdates(timeline_
+                     ? timeline_->CurrentTimeSeconds().value_or(NullValue())
+                     : NullValue());
+  }
+
+  NotifyProbe();
   return true;
 }
 
 void Animation::PostCommit(double timeline_time) {
-  PlayStateUpdateScope update_scope(*this, kTimingUpdateOnDemand,
-                                    kDoNotSetCompositorPending);
-
   compositor_pending_ = false;
 
   if (!compositor_state_ || compositor_state_->pending_action == kNone)
@@ -537,9 +532,6 @@ void Animation::PostCommit(double timeline_time) {
 }
 
 void Animation::NotifyCompositorStartTime(double timeline_time) {
-  PlayStateUpdateScope update_scope(*this, kTimingUpdateOnDemand,
-                                    kDoNotSetCompositorPending);
-
   if (compositor_state_) {
     DCHECK_EQ(compositor_state_->pending_action, kStart);
     DCHECK(!compositor_state_->start_time);
@@ -557,7 +549,7 @@ void Animation::NotifyCompositorStartTime(double timeline_time) {
       // Unlikely, but possible.
       // FIXME: Depending on what changed above this might still be pending.
       // Maybe...
-      current_time_pending_ = false;
+      NotifyStartTime(timeline_time);
       return;
     }
 
@@ -573,23 +565,8 @@ void Animation::NotifyCompositorStartTime(double timeline_time) {
 }
 
 void Animation::NotifyStartTime(double timeline_time) {
-  if (Playing()) {
-    DCHECK(!start_time_);
-    DCHECK(hold_time_.has_value());
-
-    if (playback_rate_ == 0) {
-      SetStartTimeInternal(timeline_time);
-    } else {
-      SetStartTimeInternal(timeline_time +
-                           CurrentTimeInternal() / -playback_rate_);
-    }
-
-    // FIXME: This avoids marking this animation as outdated needlessly when a
-    // start time is notified, but we should refactor how outdating works to
-    // avoid this.
-    ClearOutdated();
-    current_time_pending_ = false;
-  }
+  current_time_pending_ = false;
+  ApplyUpdates(timeline_time);
 }
 
 bool Animation::Affects(const Element& element,
@@ -867,6 +844,24 @@ void Animation::pause(ExceptionState& exception_state) {
   SetCurrentTimeInternal(new_current_time, kTimingUpdateOnDemand);
 }
 
+void Animation::CommitPendingPause(double ready_time) {
+  internal_play_state_ = kUnset;
+
+  DCHECK(pending_pause_);
+  pending_pause_ = false;
+  if (start_time_ && !hold_time_)
+    hold_time_ = (ready_time - start_time_.value()) * playback_rate_;
+
+  ApplyPendingPlaybackRate();
+  start_time_ = base::nullopt;
+
+  // Resolve pending ready promise.
+  if (ready_promise_)
+    ResolvePromiseMaybeAsync(ready_promise_.Get());
+
+  UpdateFinishedState(UpdateType::kContinuous, NotificationType::kAsync);
+}
+
 void Animation::Unpause() {
   if (!paused_)
     return;
@@ -874,6 +869,7 @@ void Animation::Unpause() {
   PlayStateUpdateScope update_scope(*this, kTimingUpdateOnDemand);
 
   current_time_pending_ = true;
+  pending_play_ = true;
   UnpauseInternal();
 }
 
@@ -881,6 +877,7 @@ void Animation::UnpauseInternal() {
   if (!paused_)
     return;
   paused_ = false;
+  pending_pause_ = false;
   SetCurrentTimeInternal(CurrentTimeInternal(), kTimingUpdateOnDemand);
 }
 
@@ -897,16 +894,13 @@ void Animation::play(ExceptionState& exception_state) {
     return;
   }
 
-  if (!Playing()) {
+  if (!Playing())
     start_time_ = base::nullopt;
-  }
 
-  if (PlayStateInternal() == kIdle) {
+  if (PlayStateInternal() == kIdle)
     hold_time_ = 0;
-  }
 
   internal_play_state_ = kUnset;
-  pending_play_ = true;
   pending_pause_ = false;
   finished_ = false;
   UnpauseInternal();
@@ -920,6 +914,47 @@ void Animation::play(ExceptionState& exception_state) {
     start_time_ = base::nullopt;
     SetCurrentTimeInternal(EffectEnd(), kTimingUpdateOnDemand);
   }
+  pending_play_ = !start_time_ || active_playback_rate_;
+}
+
+void Animation::CommitPendingPlay(double ready_time) {
+  DCHECK(start_time_ || hold_time_);
+  DCHECK(pending_play_);
+  pending_play_ = false;
+
+  // Update hold and start time.
+  if (timeline_ && timeline_->IsScrollTimeline()) {
+    // Special handling for scroll timelines.  The start time is always zero
+    // when the animation is playing. This forces the current time to match the
+    // timeline time. TODO(crbug.com/916117): Resolve in spec.
+    start_time_ = 0;
+    ApplyPendingPlaybackRate();
+    if (playback_rate_ != 0)
+      hold_time_ = base::nullopt;
+  } else if (hold_time_) {
+    ApplyPendingPlaybackRate();
+    if (playback_rate_ != 0) {
+      start_time_ = ready_time - hold_time_.value() / playback_rate_;
+      hold_time_ = base::nullopt;
+    } else {
+      start_time_ = ready_time;
+    }
+  } else if (start_time_ && active_playback_rate_) {
+    double current_time_to_match =
+        (ready_time - start_time_.value()) * active_playback_rate_.value();
+    ApplyPendingPlaybackRate();
+    if (playback_rate_ == 0)
+      hold_time_ = start_time_ = current_time_to_match;
+    else
+      start_time_ = ready_time - current_time_to_match / playback_rate_;
+  }
+
+  // Resolve pending ready promise.
+  if (ready_promise_)
+    ResolvePromiseMaybeAsync(ready_promise_.Get());
+
+  // Update finished state.
+  UpdateFinishedState(UpdateType::kContinuous, NotificationType::kAsync);
 }
 
 // https://drafts.csswg.org/web-animations/#reversing-an-animation-section
@@ -1105,9 +1140,42 @@ void Animation::CommitFinishNotification() {
   QueueFinishedEvent();
 }
 
-void Animation::CommitAllUpdatesForTesting() {
+void Animation::ApplyUpdates(double ready_time) {
+  // Applies all updates to an animation. The state of the animation may change
+  // between the time that pending updates are triggered and when the updates
+  // are applied. Thus, flags are used instead of directly queuing callbacks
+  // to enable validation that the pending state change is still relevant.
+  // Multiple updates may be applied with the caveat that the pending play and
+  // pending pause are mutually exclusive.
+  DCHECK(!(pending_play_ && pending_pause_));
+  if (pending_play_)
+    CommitPendingPlay(ready_time);
+  else if (pending_pause_)
+    CommitPendingPause(ready_time);
+
+  ApplyPendingPlaybackRate();
+
   if (pending_finish_notification_)
     CommitFinishNotification();
+
+  DCHECK(!pending_play_);
+  DCHECK(!pending_pause_);
+  DCHECK(!pending_finish_notification_);
+  DCHECK(!active_playback_rate_);
+
+  // TODO(crbug.com/960944): Deprecate.
+  current_time_pending_ = false;
+  internal_play_state_ = CalculatePlayState();
+  animation_play_state_ = CalculateAnimationPlayState();
+
+  NotifyProbe();
+}
+
+void Animation::CommitAllUpdatesForTesting() {
+  base::Optional<double> timeline_time;
+  if (timeline_)
+    timeline_time = timeline_->CurrentTimeSeconds();
+  ApplyUpdates(timeline_time.value_or(NullValue()));
 }
 
 // https://drafts.csswg.org/web-animations/#setting-the-playback-rate-of-an-animation
@@ -1136,6 +1204,15 @@ void Animation::updatePlaybackRate(double playback_rate,
 
   setPlaybackRate(playback_rate);
 
+  if (playback_rate == 0) {
+    pending_play_ = pending_pause_ = false;
+    if (play_state == kRunning || play_state == kFinished) {
+      double timeline_time = TimelineTime();
+      if (!IsNull(timeline_time))
+        start_time_ = timeline_time;
+    }
+  }
+
   if (pending())
     active_playback_rate_ = stored_playback_rate;
 }
@@ -1155,7 +1232,7 @@ ScriptPromise Animation::ready(ScriptState* script_state) {
   if (!ready_promise_) {
     ready_promise_ = MakeGarbageCollected<AnimationPromise>(
         ExecutionContext::From(script_state), this, AnimationPromise::kReady);
-    if (PlayStateInternal() != kPending)
+    if (!pending())
       ready_promise_->Resolve(this);
   }
   return ready_promise_->Promise(script_state->World());
@@ -1242,13 +1319,20 @@ void Animation::SetPlaybackRateInternal(double playback_rate) {
   DCHECK(std::isfinite(playback_rate));
   DCHECK_NE(playback_rate, playback_rate_);
 
-  if (!Limited() && !Paused() && start_time_)
+  if (!Limited() && !Paused() && start_time_) {
     current_time_pending_ = true;
+    pending_play_ = true;
+  }
 
   double stored_current_time = CurrentTimeInternal();
-  if ((playback_rate_ < 0 && playback_rate >= 0) ||
-      (playback_rate_ > 0 && playback_rate <= 0))
+  if ((playback_rate_ < 0 && playback_rate > 0) ||
+      (playback_rate_ > 0 && playback_rate < 0)) {
     finished_ = false;
+    // Reversing the direction of a finished animation will trigger playing
+    // the animation.
+    if (Limited() && !Paused() && start_time_)
+      pending_play_ = true;
+  }
 
   playback_rate_ = playback_rate;
   start_time_ = base::nullopt;
@@ -1433,17 +1517,6 @@ bool Animation::Update(TimingUpdateReason reason) {
   if (!timeline_)
     return false;
 
-  if (reason == kTimingUpdateForAnimationFrame) {
-    // Pending tasks are committed when the animation is 'ready'. This can be
-    // at the time of promise resolution or after a frame tick.  Whereas the
-    // spec calls for creating scheduled tasks to commit pending changes, in the
-    // Blink implementation, this is an acknowledgment that the changes have
-    // taken affect.
-    // TODO(crbug.com/960944): Consider restructuring implementation to more
-    // closely align with the recommended algorithm in the spec.
-    ResetPendingTasks();
-  }
-
   PlayStateUpdateScope update_scope(*this, reason, kDoNotSetCompositorPending);
 
   ClearOutdated();
@@ -1581,7 +1654,6 @@ void Animation::cancel() {
   paused_ = false;
   internal_play_state_ = kIdle;
   current_time_pending_ = false;
-
   animation_play_state_ = kIdle;
 
   // Apply changes synchronously.

third_party/blink/renderer/core/animation/animation.h

@@ -144,11 +144,12 @@ class CORE_EXPORT Animation : public EventTargetWithInlineData,
   ScriptPromise ready(ScriptState*);
 
   bool Paused() const {
-    return GetPlayState() == kPaused && !is_paused_for_testing_;
+    return CalculateAnimationPlayState() == kPaused && !is_paused_for_testing_;
   }
 
   bool Playing() const override {
-    return GetPlayState() == kRunning && !Limited() && !is_paused_for_testing_;
+    return CalculateAnimationPlayState() == kRunning && !Limited() &&
+           !is_paused_for_testing_;
   }
 
   // Indicates if the animation is out of sync with the compositor. A change to
@@ -336,6 +337,9 @@ class CORE_EXPORT Animation : public EventTargetWithInlineData,
   void ScheduleAsyncFinish();
   void AsyncFinishMicrotask();
   void CommitFinishNotification();
+  void CommitPendingPause(double ready_time);
+  void CommitPendingPlay(double ready_time);
+  void ApplyUpdates(double ready_time);
 
   // Tracking the state of animations in dev tools.
   void NotifyProbe();

third_party/blink/renderer/core/animation/animation_test.cc

@@ -167,6 +167,8 @@ class AnimationAnimationTestNoCompositing : public RenderingTest {
   }
 
   bool SimulateFrame(double time_ms) {
+    animation->CommitAllUpdatesForTesting();
+
     last_frame_time = time_ms;
     const auto* paint_artifact_compositor =
         GetDocument().GetFrame()->View()->GetPaintArtifactCompositor();

third_party/blink/web_tests/animations/web-animations/animation-state-changes-negative-playback-rate.html

@@ -82,7 +82,7 @@ test(function() {
 
 test(function() {
   var animation = finishedAnimation();
-  assert_equals(animation.startTime, document.timeline.currentTime - (animation.playbackRate * animation.currentTime));
+  assert_times_equal(animation.startTime, document.timeline.currentTime - (animation.playbackRate * animation.currentTime));
   assert_equals(animation.currentTime, 0);
   assert_false(animation.pending);
   assert_equals(animation.playState, 'finished');
@@ -118,7 +118,7 @@ test(function() {
 test(function() {
   var animation = idleAnimation();
   animation.finish();
-  assert_equals(animation.startTime, document.timeline.currentTime - (animation.playbackRate * animation.currentTime));
+  assert_times_equal(animation.startTime, document.timeline.currentTime - (animation.playbackRate * animation.currentTime));
   assert_equals(animation.currentTime, 0);
   assert_false(animation.pending);
   assert_equals(animation.playState, 'finished');
@@ -221,7 +221,7 @@ test(function() {
   animation.play();
   assert_equals(animation.startTime, startTime);
   assert_equals(animation.currentTime, currentTime);
-  assert_true(animation.pending);
+  assert_false(animation.pending);
   assert_equals(animation.playState, 'running');
 }, "Calling play() on a running animation");
 

third_party/blink/web_tests/animations/web-animations/animation-state-changes-positive-playback-rate.html

@@ -212,7 +212,7 @@ test(function(t) {
   animation.play();
   assert_equals(animation.startTime, startTime);
   assert_equals(animation.currentTime, currentTime);
-  assert_true(animation.pending);
+  assert_false(animation.pending);
   assert_equals(animation.playState, 'running');
 }, "Setting play() on a running animation");