Call AnimationProgressed when MultiAnimation finishes

When gfx::MultiAnimation exceeds its cycle time and isn't running
continuously, it calls Stop(), but doesn't first call
AnimationProgressed(). This means that the final animation state
might not be reached, i.e. AnimationProgressed() is not guaranteed
to run with a current value of 1.0. I've fixed that quirk in this CL
by calling AnimationProgressed() before stopping the animation.

This allows me to delete some code that was hacking around this quirk
in the omnibox simplified domain field trial implementation.
OmniboxViewViews::ElideAnimation was forcing the final frame to run
by calling AnimationProgressed() from AnimationEnded(). This hack was
causing a DCHECK failure when the simplified domain elision animation
was cancelled after the underlying RenderText text contents changed.
In other words, because ElideAnimation::Stop() really meant "run the
final frame and then stop", there was no safe way to cancel the
animation when its state had become stale. Now, with this
MultiAnimation fix, MultiAnimation ensures that the final frame runs
before the animation finishes, and ElideAnimation::Stop() just
stops the animation without running any frames.

Bug: 1103738

Change-Id: Idbf55bc2f326d3eaa9bf97a288e3ab467097953e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2330613
Commit-Queue: Emily Stark <estark@chromium.org>
Reviewed-by: Robert Flack <flackr@chromium.org>
Reviewed-by: Tommy Li <tommycli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#795209}

chrome/browser/ui/views/omnibox/omnibox_view_views.cc

@@ -332,11 +332,6 @@ void OmniboxViewViews::ElideAnimation::AnimationProgressed(
   view_->SchedulePaint();
 }
 
-void OmniboxViewViews::ElideAnimation::AnimationEnded(
-    const gfx::Animation* animation) {
-  AnimationProgressed(animation);
-}
-
 // OmniboxViewViews -----------------------------------------------------------
 
 // static

chrome/browser/ui/views/omnibox/omnibox_view_views.h

@@ -172,6 +172,10 @@ class OmniboxViewViews : public OmniboxView,
   void OnThemeChanged() override;
   bool IsDropCursorForInsertion() const override;
 
+  // Applies the given |color| to |range|. This is a wrapper method around
+  // Textfield::ApplyColor that tests can override.
+  virtual void ApplyColor(SkColor color, const gfx::Range& range);
+
  private:
   FRIEND_TEST_ALL_PREFIXES(OmniboxViewViewsRevealOnHoverTest, HoverAndExit);
   FRIEND_TEST_ALL_PREFIXES(
@@ -184,6 +188,8 @@ class OmniboxViewViews : public OmniboxView,
       OmniboxViewViewsHideOnInteractionAndRevealOnHoverTest,
       BoundsChanged);
   FRIEND_TEST_ALL_PREFIXES(OmniboxViewViewsRevealOnHoverTest, BoundsChanged);
+  FRIEND_TEST_ALL_PREFIXES(OmniboxViewViewsRevealOnHoverTest,
+                           CancellingAnimationDoesNotCrash);
   FRIEND_TEST_ALL_PREFIXES(
       OmniboxViewViewsHideOnInteractionAndRevealOnHoverTest,
       SchemeAndTrivialSubdomainElision);
@@ -286,7 +292,6 @@ class OmniboxViewViews : public OmniboxView,
 
     // views::AnimationDelegateViews:
     void AnimationProgressed(const gfx::Animation* animation) override;
-    void AnimationEnded(const gfx::Animation* animation) override;
 
    private:
     // Non-owning pointers. |view_| and |render_text_| must always outlive this
@@ -508,10 +513,6 @@ class OmniboxViewViews : public OmniboxView,
   // applicable), and returns the result.
   url::Component GetHostComponentAfterTrivialSubdomain();
 
-  // Applies the given |color| to |range|. This is a wrapper method around
-  // Textfield::ApplyColor that tests can override.
-  virtual void ApplyColor(SkColor color, const gfx::Range& range);
-
   ElideAnimation* GetHoverElideOrUnelideAnimationForTesting();
   ElideAnimation* GetElideAfterInteractionAnimationForTesting();
 

chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc

@@ -196,6 +196,7 @@ void TestingOmniboxView::UpdateSchemeStyle(const Range& range) {
 
 void TestingOmniboxView::ApplyColor(SkColor color, const gfx::Range& range) {
   range_colors_.emplace_back(std::pair<SkColor, gfx::Range>(color, range));
+  OmniboxViewViews::ApplyColor(color, range);
 }
 
 // TestingOmniboxEditController -----------------------------------------------
@@ -1910,6 +1911,48 @@ TEST_P(OmniboxViewViewsRevealOnHoverTest, BoundsChanged) {
       kSimplifiedDomainDisplayUrlPath, ShouldElideToRegistrableDomain()));
 }
 
+// Tests that the simplified domain animation doesn't crash when it's cancelled.
+// Regression test for https://crbug.com/1103738.
+TEST_P(OmniboxViewViewsRevealOnHoverTest, CancellingAnimationDoesNotCrash) {
+  SetUpSimplifiedDomainTest();
+
+  ASSERT_NO_FATAL_FAILURE(ExpectElidedToSimplifiedDomain(
+      omnibox_view(), kSimplifiedDomainDisplayUrlScheme,
+      kSimplifiedDomainDisplayUrlSubdomain,
+      kSimplifiedDomainDisplayUrlHostnameAndScheme,
+      kSimplifiedDomainDisplayUrlPath, ShouldElideToRegistrableDomain()));
+
+  // Hover over the omnibox to begin the unelision animation, then change the
+  // URL such that the current animation would go out of bounds if it continued
+  // running.
+  omnibox_view()->OnMouseMoved(CreateMouseEvent(ui::ET_MOUSE_MOVED, {0, 0}));
+  OmniboxViewViews::ElideAnimation* unelide_animation =
+      omnibox_view()->GetHoverElideOrUnelideAnimationForTesting();
+  ASSERT_TRUE(unelide_animation);
+  EXPECT_TRUE(unelide_animation->IsAnimating());
+  // Step through the animation partially so that it has a nonzero current
+  // value. (A zero current value causes an early return that circumvents the
+  // crash we are regression-testing.)
+  gfx::AnimationContainerElement* unelide_as_element =
+      static_cast<gfx::AnimationContainerElement*>(
+          unelide_animation->GetAnimationForTesting());
+  unelide_as_element->SetStartTime(base::TimeTicks());
+  unelide_as_element->Step(
+      base::TimeTicks() +
+      base::TimeDelta::FromMilliseconds(
+          OmniboxFieldTrial::UnelideURLOnHoverThresholdMs() + 1));
+
+  location_bar_model()->set_url(GURL(base::ASCIIToUTF16("https://foo.test")));
+  location_bar_model()->set_url_for_display(
+      base::ASCIIToUTF16("https://foo.test"));
+  omnibox_view()->model()->ResetDisplayTexts();
+  omnibox_view()->RevertAll();
+
+  // Stopping the animation after changing the underlying display text should
+  // not crash.
+  unelide_animation->Stop();
+}
+
 // Tests scheme and trivial subdomain elision when simplified domain field
 // trials are enabled.
 TEST_P(OmniboxViewViewsHideOnInteractionAndRevealOnHoverTest,

ui/gfx/animation/multi_animation.cc

@@ -46,23 +46,27 @@ void MultiAnimation::Step(base::TimeTicks time_now) {
   size_t last_index = current_part_index_;
 
   base::TimeDelta delta = time_now - start_time();
-  if (delta >= cycle_time_ && !continuous_) {
+  bool should_stop = delta >= cycle_time_ && !continuous_;
+  if (should_stop) {
     current_part_index_ = parts_.size() - 1;
     current_value_ = Tween::CalculateValue(parts_[current_part_index_].type, 1);
-    Stop();
-    return;
+  } else {
+    delta %= cycle_time_;
+    const Part& part = GetPart(&delta, &current_part_index_);
+    double percent = (delta + part.part_start).InMillisecondsF() /
+                     part.total_length.InMillisecondsF();
+    DCHECK_LE(percent, 1);
+    current_value_ = Tween::CalculateValue(part.type, percent);
   }
-  delta %= cycle_time_;
-  const Part& part = GetPart(&delta, &current_part_index_);
-  double percent = (delta + part.part_start).InMillisecondsF() /
-                   part.total_length.InMillisecondsF();
-  DCHECK_LE(percent, 1);
-  current_value_ = Tween::CalculateValue(part.type, percent);
 
   if ((current_value_ != last_value || current_part_index_ != last_index) &&
       delegate()) {
+    // Run AnimationProgressed() even if the animation will be stopped, so that
+    // the animation runs its final frame.
     delegate()->AnimationProgressed(this);
   }
+  if (should_stop)
+    Stop();
 }
 
 void MultiAnimation::SetStartTime(base::TimeTicks start_time) {

ui/gfx/animation/multi_animation_unittest.cc

@@ -6,6 +6,7 @@
 
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/gfx/animation/animation_container_element.h"
+#include "ui/gfx/animation/animation_delegate.h"
 
 namespace gfx {
 
@@ -79,6 +80,40 @@ TEST(MultiAnimationTest, DontCycle) {
   EXPECT_FALSE(animation.is_animating());
 }
 
+class CurrentValueDelegate : public AnimationDelegate {
+ public:
+  CurrentValueDelegate() = default;
+
+  double latest_current_value() { return latest_current_value_; }
+
+  // AnimationDelegate overrides:
+  void AnimationProgressed(const Animation* animation) override {
+    latest_current_value_ = animation->GetCurrentValue();
+  }
+
+ private:
+  double latest_current_value_ = 0.0;
+};
+
+// Makes sure multi-animation runs the final frame when exceeding the cycle time
+// and not running continuously.
+TEST(MultiAnimationTest, ExceedCycleNonContinuous) {
+  MultiAnimation::Parts parts;
+  parts.push_back(MultiAnimation::Part(base::TimeDelta::FromMilliseconds(200),
+                                       Tween::LINEAR));
+  MultiAnimation animation(parts, MultiAnimation::kDefaultTimerInterval);
+  CurrentValueDelegate delegate;
+  animation.set_delegate(&delegate);
+  animation.set_continuous(false);
+  AnimationContainerElement* as_element =
+      static_cast<AnimationContainerElement*>(&animation);
+  as_element->SetStartTime(base::TimeTicks());
+
+  // Step to 300, which is greater than the cycle time.
+  as_element->Step(base::TimeTicks() + base::TimeDelta::FromMilliseconds(300));
+  EXPECT_EQ(1.0, delegate.latest_current_value());
+}
+
 // Makes sure multi-animation cycles correctly.
 TEST(MultiAnimationTest, Cycle) {
   MultiAnimation::Parts parts;