Remove OmniboxViewViews::ElideAnimation::HasStarted

This method was a relic from an earlier version of this code, when the
animation faded the path from a given starting color to a given ending
color. In that version, HasStarted() was necessary to avoid duplicate
animations, which would cause a flicker. For example, an initial user
interaction would start to animate the path from opaque to transparent,
and a subsequent user interaction while still animating could restart
the same animation, causing the path to flicker from a midpoint color
back to opaque to start the animation again. To avoid this flicker, I
introduced HasStarted() to avoid running a given animation more than
once.

But the new ElideAnimation version of the code animates from whatever
the current state is to the desired end state. This means that
restarting a duplicate animation is a no-op and doesn't cause any
flicker, so we can get rid of HasStarted(). I've also added unit tests
for the cases where HasStarted() was used.

Bug: 1099875
Change-Id: I53af1ae2437fbe24e6646c99d743f0af2a398ffa
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2278164
Commit-Queue: Emily Stark <estark@chromium.org>
Reviewed-by: Justin Donnelly <jdonnelly@chromium.org>
Cr-Commit-Position: refs/heads/master@{#785747}

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

@@ -200,12 +200,6 @@ bool OmniboxViewViews::ElideAnimation::IsAnimating() {
   return animation_ && animation_->is_animating();
 }
 
-bool OmniboxViewViews::ElideAnimation::HasStarted() {
-  // |animation_| is created when Start() is called, so the animation has been
-  // run if and only if |animation_| exists.
-  return !!animation_;
-}
-
 const gfx::Range& OmniboxViewViews::ElideAnimation::GetElideToBounds() const {
   return elide_to_bounds_;
 }
@@ -1686,8 +1680,8 @@ void OmniboxViewViews::OnBlur() {
       }
     } else if (OmniboxFieldTrial::ShouldHidePathQueryRefOnInteraction()) {
       // When hide-on-interaction is enabled, this method ensures that, once the
-      // omnibox is blurred, the URL is visible and that the animation is
-      // created so that the URL will be animated to the simplified domain the
+      // omnibox is blurred, the URL is visible and that the animation state is
+      // set so that the URL will be animated to the simplified domain the
       // next time the user interacts with the page.
       ResetToHideOnInteraction();
     }
@@ -1718,11 +1712,26 @@ void OmniboxViewViews::DidFinishNavigation(
   }
 
   if (navigation->IsSameDocument() || !navigation->IsInMainFrame()) {
-    // If we've already elided to the simplified domain, make sure the full URL
-    // is not re-shown for same-document or subframe navigations.
+    // Handling same-document or non-main-frame navigations is a bit tricky
+    // because they shouldn't change the current elision/unelision state:
+    // - If the user hadn't interacted with the previous page, then we don't
+    // need to do anything: the URL is currently unelided and we're waiting for
+    // a user interaction to animate it to the simplified domain.
+    // - If the user interacted with the page, and we are currently animating to
+    // the simplified domain as a result, we want to let the animation run
+    // undisturbed, eventually ending up in the elided state.
+    // - If the user interacted with the page, and we have already finished
+    // animating to the simplified domain, then make sure we stay in the elided
+    // state, showing only the simplified domain. This is an abrupt elision,
+    // rather than an animation, because we don't want there to be any visible
+    // change in the URL from the user's perspective.
+    //
+    // |elide_after_interaction_animation_| is only created after the user
+    // interacts with the page (in DidGetUserInteraction()), so we use its
+    // existence to determine whether the user has interacted with the page yet
+    // or not.
     if (IsURLEligibleForSimplifiedDomainEliding() &&
         elide_after_interaction_animation_ &&
-        elide_after_interaction_animation_->HasStarted() &&
         !elide_after_interaction_animation_->IsAnimating()) {
       ElideToSimplifiedDomain();
     }
@@ -1747,17 +1756,22 @@ void OmniboxViewViews::DidGetUserInteraction(
   // designed to draw the user's attention and suggest that they can return to
   // the omnibox to uncover the full URL.
 
-  // This elision animation should only run once per navigation. It is
-  // recreated for the next navigation in DidFinishNavigation.
+  // Only create and run the animation if we haven't already done so on an
+  // earlier call to this method.
   if (IsURLEligibleForSimplifiedDomainEliding() &&
-      !elide_after_interaction_animation_->HasStarted()) {
+      !elide_after_interaction_animation_) {
     GetRenderText()->SetElideBehavior(gfx::NO_ELIDE);
+    elide_after_interaction_animation_ =
+        std::make_unique<ElideAnimation>(this, GetRenderText());
+    std::make_unique<ElideAnimation>(this, GetRenderText());
     elide_after_interaction_animation_->Start(GetSimplifiedDomainBounds(),
                                               0 /* delay_ms */);
   }
   // Now that the URL is being elided, create the animation to bring it back on
-  // hover (if enabled via field trial).
-  if (OmniboxFieldTrial::ShouldRevealPathQueryRefOnHover()) {
+  // hover (if enabled via field trial), if it hasn't already been created on an
+  // earlier call to this method.
+  if (OmniboxFieldTrial::ShouldRevealPathQueryRefOnHover() &&
+      !hover_elide_or_unelide_animation_) {
     hover_elide_or_unelide_animation_ =
         std::make_unique<ElideAnimation>(this, GetRenderText());
   }
@@ -2276,12 +2290,11 @@ void OmniboxViewViews::ResetToHideOnInteraction() {
       model()->ShouldPreventElision()) {
     return;
   }
-  // Delete the hover elide/unelide animation; it'll get recreated in
-  // DidGetUserInteraction() if reveal-on-hover is enabled. We don't want to
-  // unelide while the unelided URL is already showing.
+  // Delete the animations; they'll get recreated in DidGetUserInteraction().
+  // This prevents us from running any animations until the user interacts with
+  // the page.
   hover_elide_or_unelide_animation_.reset();
-  elide_after_interaction_animation_ =
-      std::make_unique<ElideAnimation>(this, GetRenderText());
+  elide_after_interaction_animation_.reset();
   if (IsURLEligibleForSimplifiedDomainEliding())
     UnelideFromSimplifiedDomain();
 }

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

@@ -182,6 +182,10 @@ class OmniboxViewViews : public OmniboxView,
       PathChangeDuringAnimation);
   FRIEND_TEST_ALL_PREFIXES(OmniboxViewViewsHideOnInteractionTest,
                            SameDocNavigations);
+  FRIEND_TEST_ALL_PREFIXES(OmniboxViewViewsHideOnInteractionTest,
+                           SameDocNavigationDuringAnimation);
+  FRIEND_TEST_ALL_PREFIXES(OmniboxViewViewsHideOnInteractionTest,
+                           UserInteractionDuringAnimation);
   FRIEND_TEST_ALL_PREFIXES(OmniboxViewViewsHideOnInteractionTest,
                            SubframeNavigations);
   FRIEND_TEST_ALL_PREFIXES(OmniboxViewViewsRevealOnHoverTest,
@@ -229,16 +233,6 @@ class OmniboxViewViews : public OmniboxView,
     // Returns true if the animation is currently running.
     bool IsAnimating();
 
-    // Returns true if the animation is currently running or has run since its
-    // creation. In contrast to IsAnimating(), this is useful for animations
-    // that should only run once in their lifetime (e.g., we animate to simplify
-    // the URL after user interaction, but only for the first user interaction
-    // on each navigation).
-    //
-    // TODO(https://crbug.com/1099875): this is an artifact of an old animation
-    // implementation and is no longer needed.
-    bool HasStarted();
-
     // Returns the bounds to which the animation is eliding, as passed in to
     // Start().
     const gfx::Range& GetElideToBounds() const;
@@ -473,20 +467,21 @@ class OmniboxViewViews : public OmniboxView,
   // when the mouse hovers or exits the omnibox.
   std::unique_ptr<ElideAnimation> hover_elide_or_unelide_animation_;
   // When ShouldHidePathQueryRefOnInteraction() is enabled, we don't
-  // create any animations until a navigation finishes. At that point, we
-  // unelide the URL if it was a full cross-document navigation, and create
-  // |elide_after_interaction_animation_| to elide the URL once the
-  // user interacts with the page. If ShouldRevealPathQueryRefOnHover() is also
-  // enabled, we defer the creation of |hover_elide_or_unelide_animation_| until
-  // the user interacts with the page; its creation is deferred to avoid
-  // flickering the URL in and out as the user hovers over the omnibox before
-  // they've interacted with the page. After the first user interaction,
+  // create any animations until the user interacts with the page. When a
+  // navigation finishes, we unelide the URL if it was a full cross-document
+  // navigation. Once the user interacts with the page, we create and run
+  // |elide_after_interaction_animation_| to elide the URL. If
+  // ShouldRevealPathQueryRefOnHover() is also enabled, we defer the creation of
+  // |hover_elide_or_unelide_animation_| until the user interacts with the page
+  // as well, since we don't want to do any hover animations until the URL has
+  // been elided after user interaction. After the first user interaction,
   // |elide_after_interaction_animation_| doesn't run again until it's
-  // re-created for the next navigation, and |hover_elide_or_unelide_animation_|
-  // behaves as described above for the rest of the navigation. There are 2
-  // separate animations (one for after-interaction and one hovering) so that
-  // the state of the after-interaction animation can be queried to avoid
-  // flickering the URL after multiple user interactions.
+  // re-created after the next navigation, and
+  // |hover_elide_or_unelide_animation_| behaves as described above for the rest
+  // of the navigation. There are 2 separate animations (one for
+  // after-interaction and one hovering) so that the state of the
+  // after-interaction animation can be queried to know when the user has or has
+  // not already interacted with the page.
   std::unique_ptr<ElideAnimation> elide_after_interaction_animation_;
 
   // Selection persisted across temporary text changes, like popup suggestions.