Reland "Web Animations: use WTF::Optional for Animation::start_time_"

This is a reland of c838bf40

The original CL contained two subtle bugs from a comparison of
compositor_state_->start_time to start_time_. Previously if both were
'null' (represented by NaN), the comparison would still say they were
inequal because NaN != NaN. The original CL 'fixed' those comparisons,
but it turns out the behavior was intentional. This version of the CL
restores the intentional behavior, adds a test for it, and documents it.

Bug: 791086, 819591
Change-Id: I6587d028e66c6f6a9213b60f8d5f51b1dac3f899
Reviewed-on: https://chromium-review.googlesource.com/956280Reviewed-by: Robert Flack <flackr@chromium.org>
Commit-Queue: Stephen McGruer <smcgruer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#544679}

third_party/WebKit/Source/core/animation/Animation.h

@@ -52,6 +52,7 @@
 #include "platform/animation/CompositorAnimationDelegate.h"
 #include "platform/graphics/CompositorElementId.h"
 #include "platform/heap/Handle.h"
+#include "platform/wtf/Optional.h"
 
 namespace blink {
 
@@ -157,13 +158,10 @@ class CORE_EXPORT Animation final : public EventTargetWithInlineData,
   const DocumentTimeline* TimelineInternal() const { return timeline_; }
   DocumentTimeline* TimelineInternal() { return timeline_; }
 
-  double CalculateStartTime(double current_time) const;
-  bool HasStartTime() const { return !IsNull(start_time_); }
   double startTime(bool& is_null) const;
-  double startTime() const;
-  double StartTimeInternal() const { return start_time_; }
+  WTF::Optional<double> startTime() const;
+  WTF::Optional<double> StartTimeInternal() const { return start_time_; }
   void setStartTime(double, bool is_null);
-  void SetStartTimeInternal(double);
 
   const AnimationEffectReadOnly* effect() const { return content_.Get(); }
   AnimationEffectReadOnly* effect() { return content_.Get(); }
@@ -228,6 +226,8 @@ class CORE_EXPORT Animation final : public EventTargetWithInlineData,
 
   void Trace(blink::Visitor*) override;
 
+  bool CompositorPendingForTesting() const { return compositor_pending_; }
+
  protected:
   DispatchEventResult DispatchEventInternal(Event*) override;
   void AddedEventListener(const AtomicString& event_type,
@@ -242,11 +242,13 @@ class CORE_EXPORT Animation final : public EventTargetWithInlineData,
   double EffectEnd() const;
   bool Limited(double current_time) const;
 
-  AnimationPlayState CalculatePlayState();
+  AnimationPlayState CalculatePlayState() const;
+  WTF::Optional<double> CalculateStartTime(double current_time) const;
   double CalculateCurrentTime() const;
 
   void UnpauseInternal();
   void SetPlaybackRateInternal(double);
+  void SetStartTimeInternal(WTF::Optional<double>);
   void UpdateCurrentTimingState(TimingUpdateReason);
 
   void BeginUpdatingState();
@@ -276,7 +278,7 @@ class CORE_EXPORT Animation final : public EventTargetWithInlineData,
 
   AnimationPlayState play_state_;
   double playback_rate_;
-  double start_time_;
+  WTF::Optional<double> start_time_;
   double hold_time_;
 
   unsigned sequence_number_;
@@ -317,7 +319,7 @@ class CORE_EXPORT Animation final : public EventTargetWithInlineData,
           playback_rate(animation.playback_rate_),
           effect_changed(false),
           pending_action(kStart) {}
-    double start_time;
+    WTF::Optional<double> start_time;
     double hold_time;
     double playback_rate;
     bool effect_changed;

third_party/WebKit/Source/core/animation/AnimationTest.cpp

@@ -108,8 +108,7 @@ TEST_F(AnimationAnimationTest, InitialState) {
   EXPECT_EQ(0, animation->CurrentTimeInternal());
   EXPECT_FALSE(animation->Paused());
   EXPECT_EQ(1, animation->playbackRate());
-  EXPECT_FALSE(animation->HasStartTime());
-  EXPECT_TRUE(IsNull(animation->StartTimeInternal()));
+  EXPECT_FALSE(animation->StartTimeInternal());
 
   StartTimeline();
   EXPECT_EQ(Animation::kFinished, animation->PlayStateInternal());
@@ -118,7 +117,6 @@ TEST_F(AnimationAnimationTest, InitialState) {
   EXPECT_FALSE(animation->Paused());
   EXPECT_EQ(1, animation->playbackRate());
   EXPECT_EQ(0, animation->StartTimeInternal());
-  EXPECT_TRUE(animation->HasStartTime());
 }
 
 TEST_F(AnimationAnimationTest, CurrentTimeDoesNotSetOutdated) {
@@ -253,7 +251,7 @@ TEST_F(AnimationAnimationTest, StartTimePauseFinish) {
   NonThrowableExceptionState exception_state;
   animation->pause();
   EXPECT_EQ(Animation::kPending, animation->PlayStateInternal());
-  EXPECT_TRUE(std::isnan(animation->startTime()));
+  EXPECT_FALSE(animation->startTime());
   animation->finish(exception_state);
   EXPECT_EQ(Animation::kFinished, animation->PlayStateInternal());
   EXPECT_EQ(-30000, animation->startTime());
@@ -274,13 +272,13 @@ TEST_F(AnimationAnimationTest, StartTimeFinishPause) {
   animation->finish(exception_state);
   EXPECT_EQ(-30 * 1000, animation->startTime());
   animation->pause();
-  EXPECT_TRUE(std::isnan(animation->startTime()));
+  EXPECT_FALSE(animation->startTime());
 }
 
 TEST_F(AnimationAnimationTest, StartTimeWithZeroPlaybackRate) {
   animation->setPlaybackRate(0);
   EXPECT_EQ(Animation::kPending, animation->PlayStateInternal());
-  EXPECT_TRUE(std::isnan(animation->startTime()));
+  EXPECT_FALSE(animation->startTime());
   SimulateFrame(10);
   EXPECT_EQ(Animation::kRunning, animation->PlayStateInternal());
 }
@@ -733,11 +731,11 @@ TEST_F(AnimationAnimationTest, PlayAfterCancel) {
   animation->cancel();
   EXPECT_EQ(Animation::kIdle, animation->PlayStateInternal());
   EXPECT_TRUE(std::isnan(animation->currentTime()));
-  EXPECT_TRUE(std::isnan(animation->startTime()));
+  EXPECT_FALSE(animation->startTime());
   animation->play();
   EXPECT_EQ(Animation::kPending, animation->PlayStateInternal());
   EXPECT_EQ(0, animation->currentTime());
-  EXPECT_TRUE(std::isnan(animation->startTime()));
+  EXPECT_FALSE(animation->startTime());
   SimulateFrame(10);
   EXPECT_EQ(Animation::kRunning, animation->PlayStateInternal());
   EXPECT_EQ(0, animation->currentTime());
@@ -751,11 +749,11 @@ TEST_F(AnimationAnimationTest, PlayBackwardsAfterCancel) {
   animation->cancel();
   EXPECT_EQ(Animation::kIdle, animation->PlayStateInternal());
   EXPECT_TRUE(std::isnan(animation->currentTime()));
-  EXPECT_TRUE(std::isnan(animation->startTime()));
+  EXPECT_FALSE(animation->startTime());
   animation->play();
   EXPECT_EQ(Animation::kPending, animation->PlayStateInternal());
   EXPECT_EQ(30 * 1000, animation->currentTime());
-  EXPECT_TRUE(std::isnan(animation->startTime()));
+  EXPECT_FALSE(animation->startTime());
   SimulateFrame(10);
   EXPECT_EQ(Animation::kRunning, animation->PlayStateInternal());
   EXPECT_EQ(30 * 1000, animation->currentTime());
@@ -766,11 +764,11 @@ TEST_F(AnimationAnimationTest, ReverseAfterCancel) {
   animation->cancel();
   EXPECT_EQ(Animation::kIdle, animation->PlayStateInternal());
   EXPECT_TRUE(std::isnan(animation->currentTime()));
-  EXPECT_TRUE(std::isnan(animation->startTime()));
+  EXPECT_FALSE(animation->startTime());
   animation->reverse();
   EXPECT_EQ(Animation::kPending, animation->PlayStateInternal());
   EXPECT_EQ(30 * 1000, animation->currentTime());
-  EXPECT_TRUE(std::isnan(animation->startTime()));
+  EXPECT_FALSE(animation->startTime());
   SimulateFrame(10);
   EXPECT_EQ(Animation::kRunning, animation->PlayStateInternal());
   EXPECT_EQ(30 * 1000, animation->currentTime());
@@ -782,7 +780,7 @@ TEST_F(AnimationAnimationTest, FinishAfterCancel) {
   animation->cancel();
   EXPECT_EQ(Animation::kIdle, animation->PlayStateInternal());
   EXPECT_TRUE(std::isnan(animation->currentTime()));
-  EXPECT_TRUE(std::isnan(animation->startTime()));
+  EXPECT_FALSE(animation->startTime());
   animation->finish(exception_state);
   EXPECT_EQ(30000, animation->currentTime());
   EXPECT_EQ(-30000, animation->startTime());
@@ -793,11 +791,11 @@ TEST_F(AnimationAnimationTest, PauseAfterCancel) {
   animation->cancel();
   EXPECT_EQ(Animation::kIdle, animation->PlayStateInternal());
   EXPECT_TRUE(std::isnan(animation->currentTime()));
-  EXPECT_TRUE(std::isnan(animation->startTime()));
+  EXPECT_FALSE(animation->startTime());
   animation->pause();
   EXPECT_EQ(Animation::kPending, animation->PlayStateInternal());
   EXPECT_EQ(0, animation->currentTime());
-  EXPECT_TRUE(std::isnan(animation->startTime()));
+  EXPECT_FALSE(animation->startTime());
 }
 
 TEST_F(AnimationAnimationTest, NoCompositeWithoutCompositedElementId) {
@@ -841,4 +839,106 @@ TEST_F(AnimationAnimationTest, NoCompositeWithoutCompositedElementId) {
           .Ok());
 }
 
+// Regression test for http://crbug.com/819591 . If a compositable animation is
+// played and then paused before any start time is set (either blink or
+// compositor side), the pausing must still set compositor pending or the pause
+// won't be synced.
+TEST_F(AnimationAnimationTest, SetCompositorPendingWithUnresolvedStartTimes) {
+  // Get rid of the default animation.
+  animation->cancel();
+
+  EnableCompositing();
+
+  SetBodyInnerHTML("<div id='target'></div>");
+
+  // Create a compositable animation; in this case opacity from 1 to 0.
+  Timing timing;
+  timing.iteration_duration = 30;
+
+  scoped_refptr<StringKeyframe> start_keyframe = StringKeyframe::Create();
+  start_keyframe->SetCSSPropertyValue(
+      CSSPropertyOpacity, "1.0", SecureContextMode::kInsecureContext, nullptr);
+  scoped_refptr<StringKeyframe> end_keyframe = StringKeyframe::Create();
+  end_keyframe->SetCSSPropertyValue(
+      CSSPropertyOpacity, "0.0", SecureContextMode::kInsecureContext, nullptr);
+
+  StringKeyframeVector keyframes;
+  keyframes.push_back(start_keyframe);
+  keyframes.push_back(end_keyframe);
+
+  auto* element = GetElementById("target");
+  StringKeyframeEffectModel* model =
+      StringKeyframeEffectModel::Create(keyframes);
+  KeyframeEffect* keyframe_effect_composited =
+      KeyframeEffect::Create(element, model, timing);
+  Animation* animation = timeline->Play(keyframe_effect_composited);
+
+  // After creating the animation we need to clean the lifecycle so that the
+  // animation can be pushed to the compositor.
+  UpdateAllLifecyclePhases();
+
+  document->GetAnimationClock().UpdateTime(0);
+  document->GetPendingAnimations().Update(WTF::nullopt, true);
+
+  // At this point, the animation exists on both the compositor and blink side,
+  // but no start time has arrived on either side. The compositor is currently
+  // synced, no update is pending.
+  EXPECT_FALSE(animation->CompositorPendingForTesting());
+
+  // However, if we pause the animation then the compositor should still be
+  // marked pending. This is required because otherwise the compositor will go
+  // ahead and start playing the animation once it receives a start time (e.g.
+  // on the next compositor frame).
+  animation->pause();
+
+  EXPECT_TRUE(animation->CompositorPendingForTesting());
+}
+
+TEST_F(AnimationAnimationTest, PreCommitWithUnresolvedStartTimes) {
+  // Get rid of the default animation.
+  animation->cancel();
+
+  EnableCompositing();
+
+  SetBodyInnerHTML("<div id='target'></div>");
+
+  // Create a compositable animation; in this case opacity from 1 to 0.
+  Timing timing;
+  timing.iteration_duration = 30;
+
+  scoped_refptr<StringKeyframe> start_keyframe = StringKeyframe::Create();
+  start_keyframe->SetCSSPropertyValue(
+      CSSPropertyOpacity, "1.0", SecureContextMode::kInsecureContext, nullptr);
+  scoped_refptr<StringKeyframe> end_keyframe = StringKeyframe::Create();
+  end_keyframe->SetCSSPropertyValue(
+      CSSPropertyOpacity, "0.0", SecureContextMode::kInsecureContext, nullptr);
+
+  StringKeyframeVector keyframes;
+  keyframes.push_back(start_keyframe);
+  keyframes.push_back(end_keyframe);
+
+  auto* element = GetElementById("target");
+  StringKeyframeEffectModel* model =
+      StringKeyframeEffectModel::Create(keyframes);
+  KeyframeEffect* keyframe_effect_composited =
+      KeyframeEffect::Create(element, model, timing);
+  Animation* animation = timeline->Play(keyframe_effect_composited);
+
+  // After creating the animation we need to clean the lifecycle so that the
+  // animation can be pushed to the compositor.
+  UpdateAllLifecyclePhases();
+
+  document->GetAnimationClock().UpdateTime(0);
+  document->GetPendingAnimations().Update(WTF::nullopt, true);
+
+  // At this point, the animation exists on both the compositor and blink side,
+  // but no start time has arrived on either side. The compositor is currently
+  // synced, no update is pending.
+  EXPECT_FALSE(animation->CompositorPendingForTesting());
+
+  // At this point, a call to PreCommit should bail out and tell us to wait for
+  // next commit because there are no resolved start times.
+  EXPECT_FALSE(animation->PreCommit(0, WTF::nullopt, true));
+}
+
 }  // namespace blink

third_party/WebKit/Source/core/animation/PendingAnimations.cpp

@@ -76,14 +76,14 @@ bool PendingAnimations::Update(
         animation->HasActiveAnimationsOnCompositor();
     // Animations with a start time do not participate in compositor start-time
     // grouping.
-    if (animation->PreCommit(animation->HasStartTime() ? 1 : compositor_group,
+    if (animation->PreCommit(animation->startTime() ? 1 : compositor_group,
                              composited_element_ids, start_on_compositor)) {
       if (animation->HasActiveAnimationsOnCompositor() &&
           !had_compositor_animation) {
         started_synchronized_on_compositor = true;
       }
 
-      if (animation->Playing() && !animation->HasStartTime() &&
+      if (animation->Playing() && !animation->startTime() &&
           animation->TimelineInternal() &&
           animation->TimelineInternal()->IsActive()) {
         waiting_for_start_time.push_back(animation.Get());
@@ -98,13 +98,13 @@ bool PendingAnimations::Update(
   // start time. Otherwise they may start immediately.
   if (started_synchronized_on_compositor) {
     for (auto& animation : waiting_for_start_time) {
-      if (!animation->HasStartTime()) {
+      if (!animation->startTime()) {
         waiting_for_compositor_animation_start_.push_back(animation);
       }
     }
   } else {
     for (auto& animation : waiting_for_start_time) {
-      if (!animation->HasStartTime()) {
+      if (!animation->startTime()) {
         animation->NotifyCompositorStartTime(
             animation->TimelineInternal()->CurrentTimeInternal());
       }
@@ -148,7 +148,7 @@ void PendingAnimations::NotifyCompositorAnimationStarted(
   animations.swap(waiting_for_compositor_animation_start_);
 
   for (auto animation : animations) {
-    if (animation->HasStartTime() ||
+    if (animation->startTime() ||
         animation->PlayStateInternal() != Animation::kPending ||
         !animation->TimelineInternal() ||
         !animation->TimelineInternal()->IsActive()) {

third_party/WebKit/Source/core/animation/css/CSSAnimations.cpp

@@ -532,8 +532,9 @@ void CSSAnimations::MaybeApplyPendingUpdate(Element* element) {
             pending_update_.NewTransitions().end() &&
         !animation->Limited()) {
       retargeted_compositor_transitions.insert(
-          property, std::pair<KeyframeEffectReadOnly*, double>(
-                        effect, animation->StartTimeInternal()));
+          property,
+          std::pair<KeyframeEffectReadOnly*, double>(
+              effect, animation->StartTimeInternal().value_or(NullValue())));
     }
     animation->cancel();
     // after cancelation, transitions must be downgraded or they'll fail

third_party/WebKit/Source/core/inspector/InspectorAnimationAgent.cpp

@@ -254,8 +254,8 @@ Response InspectorAnimationAgent::getCurrentTime(const String& id,
     *current_time = animation->currentTime();
   } else {
     // Use startTime where possible since currentTime is limited.
-    *current_time =
-        animation->TimelineInternal()->currentTime() - animation->startTime();
+    *current_time = animation->TimelineInternal()->currentTime() -
+                    animation->startTime().value_or(NullValue());
   }
   return Response::OK();
 }
@@ -274,8 +274,8 @@ Response InspectorAnimationAgent::setPaused(
       return Response::Error("Failed to clone detached animation");
     if (paused && !clone->Paused()) {
       // Ensure we restore a current time if the animation is limited.
-      double current_time =
-          clone->TimelineInternal()->currentTime() - clone->startTime();
+      double current_time = clone->TimelineInternal()->currentTime() -
+                            clone->startTime().value_or(NullValue());
       clone->pause();
       clone->setCurrentTime(current_time, false);
     } else if (!paused && clone->Paused()) {
@@ -323,7 +323,7 @@ blink::Animation* InspectorAnimationAgent::AnimationClone(
     id_to_animation_clone_.Set(id, clone);
     id_to_animation_.Set(String::Number(clone->SequenceNumber()), clone);
     clone->play();
-    clone->setStartTime(animation->startTime(), false);
+    clone->setStartTime(animation->startTime().value_or(NullValue()), false);
 
     animation->SetEffectSuppressed(true);
   }
@@ -551,12 +551,14 @@ DocumentTimeline& InspectorAnimationAgent::ReferenceTimeline() {
 double InspectorAnimationAgent::NormalizedStartTime(
     blink::Animation& animation) {
   if (ReferenceTimeline().PlaybackRate() == 0) {
-    return animation.startTime() + ReferenceTimeline().currentTime() -
+    return animation.startTime().value_or(NullValue()) +
+           ReferenceTimeline().currentTime() -
            animation.TimelineInternal()->currentTime();
   }
-  return animation.startTime() + (animation.TimelineInternal()->ZeroTime() -
-                                  ReferenceTimeline().ZeroTime()) *
-                                     1000 * ReferenceTimeline().PlaybackRate();
+  return animation.startTime().value_or(NullValue()) +
+         (animation.TimelineInternal()->ZeroTime() -
+          ReferenceTimeline().ZeroTime()) *
+             1000 * ReferenceTimeline().PlaybackRate();
 }
 
 void InspectorAnimationAgent::Trace(blink::Visitor* visitor) {