Fixes unnecessary animating of security text on tab change

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

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

@@ -85,6 +85,9 @@ class IconLabelBubbleView : public views::InkDropObserver,
   // Exposed for testing.
   SeparatorView* separator_view() const { return separator_view_; }
 
+  // Exposed for testing.
+  bool is_animating_label() const { return slide_animation_.is_animating(); }
+
   void set_next_element_interior_padding(int padding) {
     next_element_interior_padding_ = padding;
   }

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

@@ -592,7 +592,7 @@ void LocationBarView::OnNativeThemeChanged(const ui::NativeTheme* theme) {
     return;
 
   RefreshBackground();
-  location_icon_view_->Update();
+  location_icon_view_->Update(/*suppress_animations=*/false);
   RefreshClearAllButtonIcon();
   SchedulePaint();
 }
@@ -606,7 +606,7 @@ void LocationBarView::Update(const WebContents* contents) {
   RefreshContentSettingViews();
 
   RefreshPageActionIconViews();
-  location_icon_view_->Update();
+  location_icon_view_->Update(/*suppress_animations=*/contents);
 
   if (star_view_) {
     // TODO(calamity): Refactor Update to use PageActionIconView::Refresh.
@@ -1085,7 +1085,7 @@ void LocationBarView::AnimationCanceled(const gfx::Animation* animation) {
 // LocationBarView, private OmniboxEditController implementation:
 
 void LocationBarView::OnChanged() {
-  location_icon_view_->Update();
+  location_icon_view_->Update(/*suppress_animations=*/false);
   clear_all_button_->SetVisible(GetLocationBarModel()->input_in_progress() &&
                                 !omnibox_view_->text().empty() &&
                                 IsVirtualKeyboardVisible(GetWidget()));
@@ -1159,7 +1159,7 @@ void LocationBarView::OnTouchUiChanged() {
     view->SetFontList(font_list);
   if (save_credit_card_icon_view_)
     save_credit_card_icon_view_->SetFontList(font_list);
-  location_icon_view_->Update();
+  location_icon_view_->Update(/*suppress_animations=*/false);
   Layout();
   SchedulePaint();
 }

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

@@ -29,7 +29,7 @@ LocationIconView::LocationIconView(const gfx::FontList& font_list,
     : IconLabelBubbleView(font_list), delegate_(delegate) {
   label()->SetElideBehavior(gfx::ELIDE_MIDDLE);
   set_id(VIEW_ID_LOCATION_ICON);
-  Update();
+  Update(true);
   SetUpForInOutAnimation();
 }
 
@@ -174,11 +174,11 @@ bool LocationIconView::ShouldAnimateTextVisibilityChange() const {
           level == SecurityLevel::HTTP_SHOW_WARNING);
 }
 
-void LocationIconView::UpdateTextVisibility() {
+void LocationIconView::UpdateTextVisibility(bool suppress_animations) {
   SetLabel(GetText());
 
   bool should_show = ShouldShowText();
-  if (!ShouldAnimateTextVisibilityChange())
+  if (!ShouldAnimateTextVisibilityChange() || suppress_animations)
     ResetSlideAnimation(should_show);
   else if (should_show)
     AnimateIn(base::nullopt);
@@ -204,8 +204,8 @@ void LocationIconView::OnIconFetched(const gfx::Image& image) {
   SetImage(image.AsImageSkia());
 }
 
-void LocationIconView::Update() {
-  UpdateTextVisibility();
+void LocationIconView::Update(bool suppress_animations) {
+  UpdateTextVisibility(suppress_animations);
   UpdateIcon();
 
   bool is_editing_or_empty = delegate_->IsEditingOrEmpty();

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

@@ -82,8 +82,10 @@ class LocationIconView : public IconLabelBubbleView {
   // Returns what the minimum width for the label text.
   int GetMinimumLabelTextWidth() const;
 
-  // Updates the icon's ink drop mode and focusable behavior.
-  void Update();
+  // Updates the icon's ink drop mode, focusable behavior, text and security
+  // status. |suppress_animations| indicates whether this update should suppress
+  // the text change animation (e.g. when swapping tabs).
+  void Update(bool suppress_animations);
 
   // Returns text to be placed in the view.
   // - For secure/insecure pages, returns text describing the URL's security
@@ -128,7 +130,8 @@ class LocationIconView : public IconLabelBubbleView {
 
   // Updates visibility of the text and determines whether the transition
   // (if any) should be animated.
-  void UpdateTextVisibility();
+  // If |suppress_animations| is true, the text change will not be animated.
+  void UpdateTextVisibility(bool suppress_animations);
 
   // Updates Icon based on the current state and theme.
   void UpdateIcon();

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

@@ -42,13 +42,13 @@ class LocationIconViewBrowserTest : public InProcessBrowserTest {
 IN_PROC_BROWSER_TEST_F(LocationIconViewBrowserTest, InkDropMode) {
   OmniboxEditModel* model = location_bar()->GetOmniboxView()->model();
   model->SetInputInProgress(true);
-  icon_view()->Update();
+  icon_view()->Update(/*suppress_animations=*/true);
 
   EXPECT_EQ(IconLabelBubbleView::InkDropMode::OFF,
             views::test::InkDropHostViewTestApi(icon_view()).ink_drop_mode());
 
   model->SetInputInProgress(false);
-  icon_view()->Update();
+  icon_view()->Update(/*suppress_animations=*/true);
 
   EXPECT_EQ(IconLabelBubbleView::InkDropMode::ON,
             views::test::InkDropHostViewTestApi(icon_view()).ink_drop_mode());

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

+// Copyright 2018 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 "chrome/browser/ui/views/location_bar/location_icon_view.h"
+#include "base/strings/utf_string_conversions.h"
+#include "chrome/test/views/chrome_views_test_base.h"
+#include "components/omnibox/browser/location_bar_model.h"
+#include "components/omnibox/browser/test_location_bar_model.h"
+#include "ui/views/widget/widget.h"
+
+namespace {
+
+class TestLocationIconDelegate : public LocationIconView::Delegate {
+ public:
+  explicit TestLocationIconDelegate(LocationBarModel* location_bar_model)
+      : location_bar_model_(location_bar_model) {}
+
+  content::WebContents* GetWebContents() override { return nullptr; }
+
+  bool IsEditingOrEmpty() override { return false; }
+
+  SkColor GetSecurityChipColor(
+      security_state::SecurityLevel security_level) const override {
+    return SK_ColorWHITE;
+  }
+
+  bool ShowPageInfoDialog() override { return false; }
+
+  // Gets the LocationBarModel.
+  const LocationBarModel* GetLocationBarModel() const override {
+    return location_bar_model_;
+  }
+
+  // Gets an icon for the location bar icon chip.
+  gfx::ImageSkia GetLocationIcon(
+      IconFetchedCallback on_icon_fetched) const override {
+    return gfx::ImageSkia();
+  }
+
+  // Gets the color to use for icon ink highlights.
+  SkColor GetLocationIconInkDropColor() const override { return SK_ColorBLACK; }
+
+ private:
+  LocationBarModel* location_bar_model_;
+};
+
+}  // namespace
+
+class LocationIconViewTest : public ChromeViewsTestBase {
+ protected:
+  // ChromeViewsTestBase:
+  void SetUp() override {
+    ChromeViewsTestBase::SetUp();
+    gfx::FontList font_list;
+
+    CreateWidget();
+
+    location_bar_model_ = std::make_unique<TestLocationBarModel>();
+    delegate_ =
+        std::make_unique<TestLocationIconDelegate>(location_bar_model());
+
+    view_ = new LocationIconView(font_list, delegate());
+    view_->SetBoundsRect(gfx::Rect(0, 0, 24, 24));
+    widget_->SetContentsView(view_);
+
+    widget_->Show();
+  }
+
+  void TearDown() override {
+    if (widget_ && !widget_->IsClosed())
+      widget_->Close();
+
+    ChromeViewsTestBase::TearDown();
+  }
+
+  TestLocationBarModel* location_bar_model() {
+    return location_bar_model_.get();
+  }
+
+  void SetSecurityLevel(security_state::SecurityLevel level) {
+    location_bar_model()->set_security_level(level);
+
+    base::string16 secure_display_text = base::string16();
+    if (level == security_state::SecurityLevel::DANGEROUS ||
+        level == security_state::SecurityLevel::HTTP_SHOW_WARNING)
+      secure_display_text = base::ASCIIToUTF16("Insecure");
+
+    location_bar_model()->set_secure_display_text(secure_display_text);
+  }
+
+  TestLocationIconDelegate* delegate() { return delegate_.get(); }
+  LocationIconView* view() { return view_; }
+
+ private:
+  std::unique_ptr<TestLocationBarModel> location_bar_model_;
+  std::unique_ptr<TestLocationIconDelegate> delegate_;
+  LocationIconView* view_;
+  views::Widget* widget_ = nullptr;
+
+  void CreateWidget() {
+    DCHECK(!widget_);
+
+    widget_ = new views::Widget;
+    views::Widget::InitParams params =
+        CreateParams(views::Widget::InitParams::TYPE_WINDOW_FRAMELESS);
+    params.bounds = gfx::Rect(0, 0, 200, 200);
+    widget_->Init(params);
+  }
+};
+
+TEST_F(LocationIconViewTest, ShouldNotAnimateWhenSuppressingAnimations) {
+  // Make sure the initial status is secure.
+  SetSecurityLevel(security_state::SecurityLevel::SECURE);
+  view()->Update(/*suppress_animations=*/true);
+
+  SetSecurityLevel(security_state::SecurityLevel::DANGEROUS);
+  view()->Update(/*suppress_animations=*/true);
+  // When we change tab, suppress animations is true.
+  EXPECT_FALSE(view()->is_animating_label());
+}
+
+TEST_F(LocationIconViewTest, ShouldAnimateTextWhenWarning) {
+  // Make sure the initial status is secure.
+  SetSecurityLevel(security_state::SecurityLevel::SECURE);
+  view()->Update(/*suppress_animations=*/true);
+
+  SetSecurityLevel(security_state::SecurityLevel::HTTP_SHOW_WARNING);
+  view()->Update(/*suppress_animations=*/false);
+  EXPECT_TRUE(view()->is_animating_label());
+}
+
+TEST_F(LocationIconViewTest, ShouldAnimateTextWhenDangerous) {
+  // Make sure the initial status is secure.
+  SetSecurityLevel(security_state::SecurityLevel::SECURE);
+  view()->Update(/*suppress_animations=*/true);
+
+  SetSecurityLevel(security_state::SecurityLevel::DANGEROUS);
+  view()->Update(/*suppress_animations=*/false);
+  EXPECT_TRUE(view()->is_animating_label());
+}
+
+TEST_F(LocationIconViewTest, ShouldNotAnimateWarningToDangerous) {
+  // Make sure the initial status is secure.
+  SetSecurityLevel(security_state::SecurityLevel::HTTP_SHOW_WARNING);
+  view()->Update(/*suppress_animations=*/true);
+
+  SetSecurityLevel(security_state::SecurityLevel::DANGEROUS);
+  view()->Update(/*suppress_animations=*/false);
+  EXPECT_FALSE(view()->is_animating_label());
+}

chrome/test/BUILD.gn

@@ -4312,6 +4312,7 @@ test("unit_tests") {
       "../browser/ui/views/infobars/infobar_view_unittest.cc",
       "../browser/ui/views/layout_provider_unittest.cc",
       "../browser/ui/views/location_bar/icon_label_bubble_view_unittest.cc",
+      "../browser/ui/views/location_bar/location_icon_view_unittest.cc",
       "../browser/ui/views/media_router/cast_dialog_metrics_unittest.cc",
       "../browser/ui/views/media_router/cast_dialog_no_sinks_view_unittest.cc",
       "../browser/ui/views/media_router/cast_dialog_sink_button_unittest.cc",

components/omnibox/browser/test_location_bar_model.cc

@@ -52,7 +52,7 @@ const gfx::VectorIcon& TestLocationBarModel::GetVectorIcon() const {
 }
 
 base::string16 TestLocationBarModel::GetSecureDisplayText() const {
-  return base::string16();
+  return secure_display_text_;
 }
 
 base::string16 TestLocationBarModel::GetSecureAccessibilityText() const {

components/omnibox/browser/test_location_bar_model.h

@@ -55,6 +55,9 @@ class TestLocationBarModel : public LocationBarModel {
     should_display_url_ = should_display_url;
   }
   void set_offline_page(bool offline_page) { offline_page_ = offline_page; }
+  void set_secure_display_text(base::string16 secure_display_text) {
+    secure_display_text_ = secure_display_text;
+  }
 
  private:
   // If either of these is not explicitly set, the test class will return
@@ -68,6 +71,7 @@ class TestLocationBarModel : public LocationBarModel {
   base::string16 ev_cert_name_;
   bool should_display_url_ = false;
   bool offline_page_ = false;
+  base::string16 secure_display_text_ = base::string16();
 
   DISALLOW_COPY_AND_ASSIGN(TestLocationBarModel);
 };