Keep running animations across an AnimationRunner switch.

Without this, removing an animated view from a widget would stall the animation;
this could leave its state incorrect, e.g. if it was later re-added.  This can
affect the omnibox with an upcoming patch I'm writing.

This was introduced by http://crrev.com/657587 ; before that, AnimationRunner
did not exist, and removing a view from a widget would have no effect on
animations.

Bug: none
Change-Id: Iae457306a3a3f68e42772009a24d34dd65d4a144
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1857191
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Robert Flack <flackr@chromium.org>
Auto-Submit: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#706612}

ui/gfx/BUILD.gn

@@ -685,6 +685,7 @@ test("gfx_unittests") {
   if (!is_ios) {
     sources += [
       "animation/animation_container_unittest.cc",
+      "animation/animation_runner_unittest.cc",
       "animation/animation_unittest.cc",
       "animation/multi_animation_unittest.cc",
       "animation/slide_animation_unittest.cc",

ui/gfx/animation/animation_container.cc

@@ -19,16 +19,16 @@ AnimationContainer::~AnimationContainer() {
   if (observer_)
     observer_->AnimationContainerShuttingDown(this);
 
-  // The animations own us and stop themselves before being deleted. If
-  // elements_ is not empty, something is wrong.
-  DCHECK(elements_.empty());
+  // The animations own us and stop themselves before being deleted. If they're
+  // still running, something is wrong.
+  DCHECK(!is_running());
 }
 
 void AnimationContainer::Start(AnimationContainerElement* element) {
   DCHECK(elements_.count(element) == 0);  // Start should only be invoked if the
                                           // element isn't running.
 
-  if (elements_.empty()) {
+  if (!is_running()) {
     last_tick_time_ = base::TimeTicks::Now();
     SetMinTimerInterval(element->GetTimerInterval());
     min_timer_interval_count_ = 1;
@@ -49,7 +49,7 @@ void AnimationContainer::Stop(AnimationContainerElement* element) {
   base::TimeDelta interval = element->GetTimerInterval();
   elements_.erase(element);
 
-  if (elements_.empty()) {
+  if (!is_running()) {
     runner_->Stop();
     min_timer_interval_count_ = 0;
     if (observer_)
@@ -76,6 +76,8 @@ void AnimationContainer::SetAnimationRunner(
   runner_ = has_custom_animation_runner_
                 ? std::move(runner)
                 : AnimationRunner::CreateDefaultAnimationRunner();
+  if (is_running())
+    RestartTimer(base::TimeTicks::Now() - last_tick_time_);
 }
 
 void AnimationContainer::Run(base::TimeTicks current_time) {
@@ -107,14 +109,18 @@ void AnimationContainer::SetMinTimerInterval(base::TimeDelta delta) {
   // that shouldn't be a problem for uses of Animation/AnimationContainer.
   runner_->Stop();
   min_timer_interval_ = delta;
+  RestartTimer(base::TimeDelta());
+}
+
+void AnimationContainer::RestartTimer(base::TimeDelta elapsed) {
   runner_->Start(
-      min_timer_interval_,
+      min_timer_interval_, elapsed,
       base::BindRepeating(&AnimationContainer::Run, base::Unretained(this)));
 }
 
 std::pair<TimeDelta, size_t> AnimationContainer::GetMinIntervalAndCount()
     const {
-  DCHECK(!elements_.empty());
+  DCHECK(is_running());
 
   // Find the minimum interval and the number of elements sharing that same
   // interval. It is tempting to create a map of intervals -> counts in order to

ui/gfx/animation/animation_container.h

@@ -56,6 +56,7 @@ class ANIMATION_EXPORT AnimationContainer
   bool is_running() const { return !elements_.empty(); }
 
   void SetAnimationRunner(std::unique_ptr<AnimationRunner> runner);
+  AnimationRunner* animation_runner_for_testing() { return runner_.get(); }
   bool has_custom_animation_runner() const {
     return has_custom_animation_runner_;
   }
@@ -80,6 +81,10 @@ class ANIMATION_EXPORT AnimationContainer
   // Sets min_timer_interval_ and restarts the timer.
   void SetMinTimerInterval(base::TimeDelta delta);
 
+  // Restarts the timer, assuming |elapsed| has already elapsed out of the timer
+  // interval.
+  void RestartTimer(base::TimeDelta elapsed);
+
   // Returns the min timer interval of all the timers, and the count of timers
   // at that interval.
   std::pair<base::TimeDelta, size_t> GetMinIntervalAndCount() const;

ui/gfx/animation/animation_container_unittest.cc

@@ -11,6 +11,7 @@
 #include "base/test/task_environment.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/gfx/animation/animation_container_observer.h"
+#include "ui/gfx/animation/animation_test_api.h"
 #include "ui/gfx/animation/linear_animation.h"
 #include "ui/gfx/animation/test_animation_delegate.h"
 
@@ -53,6 +54,8 @@ class TestAnimation : public LinearAnimation {
 
   void AnimateToState(double state) override {}
 
+  using LinearAnimation::duration;
+
  private:
   DISALLOW_COPY_AND_ASSIGN(TestAnimation);
 };
@@ -143,4 +146,27 @@ TEST_F(AnimationContainerTest, Observer) {
   container->set_observer(NULL);
 }
 
+// Tests that calling SetAnimationRunner() keeps running animations at their
+// current point.
+TEST_F(AnimationContainerTest, AnimationsRunAcrossRunnerChange) {
+  TestAnimationDelegate delegate;
+  auto container = base::MakeRefCounted<AnimationContainer>();
+  AnimationContainerTestApi test_api(container.get());
+  TestAnimation animation(&delegate);
+  animation.SetContainer(container.get());
+
+  animation.Start();
+  test_api.IncrementTime(animation.duration() / 2);
+  EXPECT_FALSE(delegate.finished());
+
+  container->SetAnimationRunner(nullptr);
+  AnimationRunner* runner = container->animation_runner_for_testing();
+  ASSERT_TRUE(runner);
+  ASSERT_FALSE(runner->step_is_null_for_testing());
+  EXPECT_FALSE(delegate.finished());
+
+  test_api.IncrementTime(animation.duration() / 2);
+  EXPECT_TRUE(delegate.finished());
+}
+
 }  // namespace gfx

ui/gfx/animation/animation_runner.cc

@@ -18,21 +18,40 @@ class DefaultAnimationRunner : public gfx::AnimationRunner {
   ~DefaultAnimationRunner() override = default;
 
   // gfx::AnimationRunner:
-  void Stop() override { timer_.Stop(); }
+  void Stop() override;
 
  protected:
   // gfx::AnimationRunner:
-  void OnStart(base::TimeDelta min_interval) override {
-    timer_.Start(FROM_HERE, min_interval, this,
-                 &DefaultAnimationRunner::OnTimerTick);
-  }
+  void OnStart(base::TimeDelta min_interval, base::TimeDelta elapsed) override;
 
  private:
-  void OnTimerTick() { Step(base::TimeTicks::Now()); }
+  void OnTimerTick();
 
-  base::RepeatingTimer timer_;
+  base::OneShotTimer timer_;
+  base::TimeDelta min_interval_;
 };
 
+void DefaultAnimationRunner::Stop() {
+  timer_.Stop();
+}
+
+void DefaultAnimationRunner::OnStart(base::TimeDelta min_interval,
+                                     base::TimeDelta elapsed) {
+  min_interval_ = min_interval;
+  timer_.Start(FROM_HERE, min_interval - elapsed, this,
+               &DefaultAnimationRunner::OnTimerTick);
+}
+
+void DefaultAnimationRunner::OnTimerTick() {
+  // This is effectively a RepeatingTimer.  It's possible to use a true
+  // RepeatingTimer for this, but since OnStart() may need to use a OneShotTimer
+  // anyway (when |elapsed| is nonzero), it's just more complicated.
+  timer_.Start(FROM_HERE, min_interval_, this,
+               &DefaultAnimationRunner::OnTimerTick);
+  // Call Step() after timer_.Start() in case Step() calls Stop().
+  Step(base::TimeTicks::Now());
+}
+
 }  // namespace
 
 namespace gfx {
@@ -47,9 +66,10 @@ AnimationRunner::~AnimationRunner() = default;
 
 void AnimationRunner::Start(
     base::TimeDelta min_interval,
+    base::TimeDelta elapsed,
     base::RepeatingCallback<void(base::TimeTicks)> step) {
   step_ = std::move(step);
-  OnStart(min_interval);
+  OnStart(min_interval, elapsed);
 }
 
 AnimationRunner::AnimationRunner() = default;

ui/gfx/animation/animation_runner.h

@@ -27,20 +27,25 @@ class ANIMATION_EXPORT AnimationRunner {
   virtual ~AnimationRunner();
 
   // Sets the provided |step| callback, then calls OnStart() with the provided
-  // |min_interval| to allow the subclass to actually begin animating.
-  // Subclasses are expected to call Step() periodically to drive the animation.
+  // |min_interval| and |elapsed| time to allow the subclass to actually begin
+  // animating. Subclasses are expected to call Step() periodically to drive the
+  // animation.
   void Start(base::TimeDelta min_interval,
+             base::TimeDelta elapsed,
              base::RepeatingCallback<void(base::TimeTicks)> step);
 
   // Called when subclasses don't need to call Step() anymore.
   virtual void Stop() = 0;
 
+  bool step_is_null_for_testing() const { return step_.is_null(); }
+
  protected:
   AnimationRunner();
 
   // Called when subclasses should start calling Step() periodically to
   // drive the animation.
-  virtual void OnStart(base::TimeDelta min_interval) = 0;
+  virtual void OnStart(base::TimeDelta min_interval,
+                       base::TimeDelta elapsed) = 0;
 
   // Advances the animation based on |tick|.
   void Step(base::TimeTicks tick);

ui/gfx/animation/animation_runner_unittest.cc

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "ui/gfx/animation/animation_runner.h"
+
+#include "base/test/bind_test_util.h"
+#include "base/test/task_environment.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace gfx {
+namespace {
+
+using AnimationRunnerTest = testing::Test;
+
+// Verifies that calling Stop() during Step() actually stops the timer.
+TEST(AnimationRunnerTest, StopDuringStep) {
+  base::test::TaskEnvironment task_environment(
+      base::test::TaskEnvironment::TimeSource::MOCK_TIME);
+
+  auto runner = AnimationRunner::CreateDefaultAnimationRunner();
+  constexpr auto kDelay = base::TimeDelta::FromMilliseconds(20);
+  int call_count = 0;
+  runner->Start(kDelay, base::TimeDelta(),
+                base::BindLambdaForTesting([&](base::TimeTicks ticks) {
+                  ++call_count;
+                  runner->Stop();
+                }));
+  EXPECT_EQ(0, call_count);
+  task_environment.FastForwardBy(kDelay);
+  EXPECT_EQ(1, call_count);
+  task_environment.FastForwardBy(kDelay);
+  EXPECT_EQ(1, call_count);
+}
+
+}  // namespace
+}  // namespace gfx

ui/gfx/animation/animation_unittest.cc

@@ -16,7 +16,7 @@
 
 namespace gfx {
 
-class AnimationTest: public testing::Test {
+class AnimationTest : public testing::Test {
  protected:
   AnimationTest()
       : task_environment_(

ui/gfx/animation/test_animation_delegate.h

@@ -15,31 +15,25 @@ namespace gfx {
 // message loop.
 class TestAnimationDelegate : public AnimationDelegate {
  public:
-  TestAnimationDelegate() : canceled_(false), finished_(false) {
-  }
+  TestAnimationDelegate() = default;
 
   virtual void AnimationEnded(const Animation* animation) {
     finished_ = true;
-    base::RunLoop::QuitCurrentWhenIdleDeprecated();
+    if (base::RunLoop::IsRunningOnCurrentThread())
+      base::RunLoop::QuitCurrentWhenIdleDeprecated();
   }
 
   virtual void AnimationCanceled(const Animation* animation) {
-    finished_ = true;
     canceled_ = true;
-    base::RunLoop::QuitCurrentWhenIdleDeprecated();
+    AnimationEnded(animation);
   }
 
-  bool finished() const {
-    return finished_;
-  }
-
-  bool canceled() const {
-    return canceled_;
-  }
+  bool finished() const { return finished_; }
+  bool canceled() const { return canceled_; }
 
  private:
-  bool canceled_;
-  bool finished_;
+  bool canceled_ = false;
+  bool finished_ = false;
 
   DISALLOW_COPY_AND_ASSIGN(TestAnimationDelegate);
 };

ui/views/animation/compositor_animation_runner.cc

@@ -59,11 +59,12 @@ void CompositorAnimationRunner::OnCompositingShuttingDown(
   compositor_ = nullptr;
 }
 
-void CompositorAnimationRunner::OnStart(base::TimeDelta min_interval) {
+void CompositorAnimationRunner::OnStart(base::TimeDelta min_interval,
+                                        base::TimeDelta elapsed) {
   if (!compositor_)
     return;
 
-  last_tick_ = base::TimeTicks::Now();
+  last_tick_ = base::TimeTicks::Now() - elapsed;
   min_interval_ = min_interval;
   DCHECK(!compositor_->HasAnimationObserver(this));
   compositor_->AddAnimationObserver(this);

ui/views/animation/compositor_animation_runner.h

@@ -30,7 +30,7 @@ class CompositorAnimationRunner : public gfx::AnimationRunner,
 
  protected:
   // gfx::AnimationRunner:
-  void OnStart(base::TimeDelta min_interval) override;
+  void OnStart(base::TimeDelta min_interval, base::TimeDelta elapsed) override;
 
  private:
   // This observes Compositor's destruction and helps CompositorAnimationRunner