Fixes a bug in the LocationIconView InkDrop

This stops the LocationIconView InkDrop from getting reset if
LocationIconView::Update is called while the InkDrop is animating.

Note: This issue doesn't seem to affect the LocationBarView, only the
CustomTabBar being introduced in CL: 1328084

Bug: 853593
Change-Id: I3ab4045c7a8d0db88a069f1d35d9763c9c8594f1
Reviewed-on: https://chromium-review.googlesource.com/c/1338579Reviewed-by: Justin Donnelly <jdonnelly@chromium.org>
Commit-Queue: Jay Harris <harrisjay@chromium.org>
Cr-Commit-Position: refs/heads/master@{#611893}

chrome/browser/ui/views/location_bar/location_icon_view.cc

@@ -123,6 +123,10 @@ bool LocationIconView::ShouldShowText() const {
   return !location_bar_model->GetSecureDisplayText().empty();
 }
 
+const views::InkDrop* LocationIconView::get_ink_drop_for_testing() {
+  return GetInkDrop();
+}
+
 base::string16 LocationIconView::GetText() const {
   if (delegate_->GetLocationBarModel()->GetURL().SchemeIs(
           content::kChromeUIScheme))
@@ -201,26 +205,31 @@ void LocationIconView::Update(bool suppress_animations) {
                      ? base::string16()
                      : l10n_util::GetStringUTF16(IDS_TOOLTIP_LOCATION_ICON));
 
-  // If the omnibox is empty or editing, the user should not be able to left
-  // click on the icon. As such, the icon should not show a highlight or be
-  // focusable. Note: using the middle mouse to copy-and-paste should still
-  // work on the icon.
-  if (is_editing_or_empty) {
-    SetInkDropMode(InkDropMode::OFF);
-    SetFocusBehavior(FocusBehavior::NEVER);
-    return;
-  }
-
-  SetInkDropMode(InkDropMode::ON);
+  // We should only enable/disable the InkDrop if the editing state has changed,
+  // as the drop gets recreated when SetInkDropMode is called. This can result
+  // in strange behaviour, like the the InkDrop disappearing mid animation.
+  if (is_editing_or_empty != was_editing_or_empty_) {
+    // If the omnibox is empty or editing, the user should not be able to left
+    // click on the icon. As such, the icon should not show a highlight or be
+    // focusable. Note: using the middle mouse to copy-and-paste should still
+    // work on the icon.
+    if (is_editing_or_empty) {
+      SetInkDropMode(InkDropMode::OFF);
+      SetFocusBehavior(FocusBehavior::NEVER);
+    } else {
+      SetInkDropMode(InkDropMode::ON);
 
 #if defined(OS_MACOSX)
-  SetFocusBehavior(FocusBehavior::ACCESSIBLE_ONLY);
+      SetFocusBehavior(FocusBehavior::ACCESSIBLE_ONLY);
 #else
-  SetFocusBehavior(FocusBehavior::ALWAYS);
+      SetFocusBehavior(FocusBehavior::ALWAYS);
 #endif
+    }
+  }
 
   last_update_security_level_ =
       delegate_->GetLocationBarModel()->GetSecurityLevel(false);
+  was_editing_or_empty_ = is_editing_or_empty;
 }
 
 bool LocationIconView::IsTriggerableEvent(const ui::Event& event) {

chrome/browser/ui/views/location_bar/location_icon_view.h

@@ -100,6 +100,8 @@ class LocationIconView : public IconLabelBubbleView {
   // - the current page URL is a chrome-extension:// URL.
   bool ShouldShowText() const;
 
+  const views::InkDrop* get_ink_drop_for_testing();
+
  protected:
   // IconLabelBubbleView:
   bool IsTriggerableEvent(const ui::Event& event) override;
@@ -111,6 +113,10 @@ class LocationIconView : public IconLabelBubbleView {
   security_state::SecurityLevel last_update_security_level_ =
       security_state::NONE;
 
+  // Whether the delegate's editing or empty flag was set the last time the
+  // location icon was updated.
+  bool was_editing_or_empty_ = false;
+
   // Returns what the minimum size would be if the preferred size were |size|.
   gfx::Size GetMinimumSizeForPreferredSize(gfx::Size size) const;
 

chrome/browser/ui/views/location_bar/location_icon_view_unittest.cc

@@ -18,7 +18,10 @@ class TestLocationIconDelegate : public LocationIconView::Delegate {
 
   content::WebContents* GetWebContents() override { return nullptr; }
 
-  bool IsEditingOrEmpty() override { return false; }
+  bool IsEditingOrEmpty() override { return is_editing_or_empty_; }
+  void set_is_editing_or_empty(bool is_editing_or_empty) {
+    is_editing_or_empty_ = is_editing_or_empty;
+  }
 
   SkColor GetSecurityChipColor(
       security_state::SecurityLevel security_level) const override {
@@ -43,6 +46,7 @@ class TestLocationIconDelegate : public LocationIconView::Delegate {
 
  private:
   LocationBarModel* location_bar_model_;
+  bool is_editing_or_empty_ = false;
 };
 
 }  // namespace
@@ -149,3 +153,39 @@ TEST_F(LocationIconViewTest, ShouldNotAnimateWarningToDangerous) {
   view()->Update(/*suppress_animations=*/false);
   EXPECT_FALSE(view()->is_animating_label());
 }
+
+// Whenever InkDropMode is set a new InkDrop is created, which will reset any
+// animations on the drop, so we should only set the InkDropMode when it has
+// actually changed.
+TEST_F(LocationIconViewTest, ShouldNotRecreateInkDropNeedlessly) {
+  delegate()->set_is_editing_or_empty(false);
+  view()->Update(false);
+
+  const views::InkDrop* drop = view()->get_ink_drop_for_testing();
+  view()->Update(/*suppress_animations=*/false);
+
+  // The InkDropMode has not changed (is ON), so our InkDrop should remain the
+  // same.
+  EXPECT_EQ(drop, view()->get_ink_drop_for_testing());
+
+  delegate()->set_is_editing_or_empty(true);
+  view()->Update(/*suppress_animations=*/false);
+
+  // The InkDropMode has changed (ON --> OFF), so a new InkDrop will have been
+  // created.
+  EXPECT_NE(drop, view()->get_ink_drop_for_testing());
+
+  drop = view()->get_ink_drop_for_testing();
+  view()->Update(/*suppress_animations=*/false);
+
+  // The InkDropMode has not changed (is OFF), so the InkDrop should remain the
+  // same.
+  EXPECT_EQ(drop, view()->get_ink_drop_for_testing());
+
+  delegate()->set_is_editing_or_empty(false);
+  view()->Update(/*suppress_animations=*/false);
+
+  // The InkDrop mode has changed (OFF --> ON), so a new InkDrop will have been
+  // created.
+  EXPECT_NE(drop, view()->get_ink_drop_for_testing());
+}