Fix two bugs in the extension actions toolbar

1. Previously, we didn't animate the browser actions
   container when the visible count changed, because we set
   the width prior to calling animate. Fix it by animating to
   the desired width.
2. There was a bug where we would improperly hide an extension
   upon loading another, because we wouldn't account for the
   chevron width. Fix this, and add a test.

BUG=410348
BUG=410361

Review URL: https://codereview.chromium.org/539543002

Cr-Commit-Position: refs/heads/master@{#293947}

3 files changed, 68 insertions, 25 deletions

diff --git a/chrome/browser/ui/views/toolbar/browser_actions_container.cc b/chrome/browser/ui/views/toolbar/browser_actions_container.cc
index 2b7c705..5ed94f3 100644
--- a/chrome/browser/ui/views/toolbar/browser_actions_container.cc
+++ b/chrome/browser/ui/views/toolbar/browser_actions_container.cc
@@ -192,8 +192,10 @@ void BrowserActionsContainer::Init() {
 
   // We wait to set the container width until now so that the chevron images
   // will be loaded.  The width calculation needs to know the chevron size.
-  if (model_ && model_->extensions_initialized())
-    SetContainerWidth();
+  if (model_ && model_->extensions_initialized()) {
+    container_width_ = GetPreferredWidth();
+    SetChevronVisibility();
+  }
 }
 
 BrowserActionView* BrowserActionsContainer::GetViewForExtension(
@@ -640,6 +642,7 @@ void BrowserActionsContainer::AnimationEnded(const gfx::Animation* animation) {
   animation_target_size_ = 0;
   resize_amount_ = 0;
   suppress_chevron_ = false;
+  SetChevronVisibility();
   OnBrowserActionVisibilityChanged();
 
   FOR_EACH_OBSERVER(BrowserActionsContainerObserver,
@@ -803,17 +806,28 @@ void BrowserActionsContainer::ToolbarExtensionAdded(const Extension* extension,
   if (!model_->extensions_initialized())
     return;
 
-  // Enlarge the container if it was already at maximum size and we're not in
-  // the middle of upgrading.
-  if ((model_->GetVisibleIconCount() < 0) &&
-      !extensions::ExtensionSystem::Get(profile_)->runtime_data()->
+  // If this is just an upgrade, then don't worry about resizing.
+  if (!extensions::ExtensionSystem::Get(profile_)->runtime_data()->
           IsBeingUpgraded(extension)) {
-    suppress_chevron_ = true;
-    Animate(gfx::Tween::LINEAR, browser_action_views_.size());
-  } else {
-    // Just redraw the (possibly modified) visible icon set.
-    OnBrowserActionVisibilityChanged();
+    // We need to resize if either:
+    // - The container is set to display all icons (visible count = -1), or
+    // - The container will need to expand to include the chevron. This can
+    //   happen when the container is set to display <n> icons, where <n> is
+    //   the number of icons before the new icon. With the new icon, the chevron
+    //   will need to be displayed.
+    int model_icon_count = model_->GetVisibleIconCount();
+    if (model_icon_count == -1 ||
+        (static_cast<size_t>(model_icon_count) < browser_action_views_.size() &&
+         (chevron_ && !chevron_->visible()))) {
+      suppress_chevron_ = true;
+      Animate(gfx::Tween::LINEAR, GetIconCount());
+      return;
+    }
   }
+
+  // Otherwise, we don't have to resize, so just redraw the (possibly modified)
+  // visible icon set.
+  OnBrowserActionVisibilityChanged();
 }
 
 void BrowserActionsContainer::ToolbarExtensionRemoved(
@@ -902,9 +916,7 @@ bool BrowserActionsContainer::ShowExtensionActionPopup(
 }
 
 void BrowserActionsContainer::ToolbarVisibleCountChanged() {
-  int old_container_width = container_width_;
-  SetContainerWidth();
-  if (old_container_width != container_width_)
+  if (GetPreferredWidth() != container_width_)
     Animate(gfx::Tween::EASE_OUT, GetIconCount());
 }
 
@@ -916,7 +928,7 @@ void BrowserActionsContainer::ToolbarHighlightModeChanged(
   // the extra complexity to create and insert only the new extensions.
   DeleteBrowserActionViews();
   CreateBrowserActionViews();
-  Animate(gfx::Tween::LINEAR, browser_action_views_.size());
+  Animate(gfx::Tween::LINEAR, GetIconCount());
 }
 
 Browser* BrowserActionsContainer::GetBrowser() {
@@ -943,14 +955,16 @@ void BrowserActionsContainer::OnBrowserActionVisibilityChanged() {
   }
 }
 
-void BrowserActionsContainer::SetContainerWidth() {
-  int visible_actions = GetIconCount();
-  if (chevron_) {
-    chevron_->SetVisible(
-      static_cast<size_t>(visible_actions) < model_->toolbar_items().size());
-  }
-  container_width_ =
-      IconCountToWidth(visible_actions, chevron_ && chevron_->visible());
+int BrowserActionsContainer::GetPreferredWidth() {
+  size_t visible_actions = GetIconCount();
+  return IconCountToWidth(
+      visible_actions,
+      chevron_ && visible_actions < browser_action_views_.size());
+}
+
+void BrowserActionsContainer::SetChevronVisibility() {
+  if (chevron_)
+    chevron_->SetVisible(GetIconCount() < browser_action_views_.size());
 }
 
 void BrowserActionsContainer::CloseOverflowMenu() {
diff --git a/chrome/browser/ui/views/toolbar/browser_actions_container.h b/chrome/browser/ui/views/toolbar/browser_actions_container.h
index ab09ad1..f3702c5 100644
--- a/chrome/browser/ui/views/toolbar/browser_actions_container.h
+++ b/chrome/browser/ui/views/toolbar/browser_actions_container.h
@@ -295,8 +295,13 @@ class BrowserActionsContainer
   // Called when a browser action's visibility may have changed.
   void OnBrowserActionVisibilityChanged();
 
-  // Sets the initial container width.
-  void SetContainerWidth();
+  // Returns the preferred width of the container in order to show all icons
+  // that should be visible and, optionally, the chevron.
+  int GetPreferredWidth();
+
+  // Sets the chevron to be visible or not based on whether all browser actions
+  // are displayed.
+  void SetChevronVisibility();
 
   // Closes the overflow menu if open.
   void CloseOverflowMenu();
diff --git a/chrome/browser/ui/views/toolbar/browser_actions_container_browsertest.cc b/chrome/browser/ui/views/toolbar/browser_actions_container_browsertest.cc
index 0b2abb8..6704a7b 100644
--- a/chrome/browser/ui/views/toolbar/browser_actions_container_browsertest.cc
+++ b/chrome/browser/ui/views/toolbar/browser_actions_container_browsertest.cc
@@ -229,6 +229,30 @@ IN_PROC_BROWSER_TEST_F(BrowserActionsContainerTest, Visibility) {
   ASSERT_TRUE(container->chevron());
   EXPECT_TRUE(container->chevron()->visible());
   EXPECT_FALSE(container->GetPreferredSize().IsEmpty());
+
+  // Reset visibility count to 2. State should be A, B, [C], and the chevron
+  // should be visible.
+  browser_actions_bar()->SetIconVisibilityCount(2);
+  EXPECT_EQ(2, browser_actions_bar()->VisibleBrowserActions());
+  EXPECT_EQ(idA, browser_actions_bar()->GetExtensionId(0));
+  EXPECT_EQ(idB, browser_actions_bar()->GetExtensionId(1));
+  EXPECT_TRUE(container->chevron()->visible());
+
+  // Disable C (the overflowed extension). State should now be A, B, and the
+  // chevron should be hidden.
+  DisableExtension(idC);
+  EXPECT_EQ(2, browser_actions_bar()->VisibleBrowserActions());
+  EXPECT_EQ(idA, browser_actions_bar()->GetExtensionId(0));
+  EXPECT_EQ(idB, browser_actions_bar()->GetExtensionId(1));
+  EXPECT_FALSE(container->chevron()->visible());
+
+  // Re-enable C. We should still only have 2 visible icons, and the chevron
+  // should be visible.
+  EnableExtension(idC);
+  EXPECT_EQ(2, browser_actions_bar()->VisibleBrowserActions());
+  EXPECT_EQ(idA, browser_actions_bar()->GetExtensionId(0));
+  EXPECT_EQ(idB, browser_actions_bar()->GetExtensionId(1));
+  EXPECT_TRUE(container->chevron()->visible());
 }
 
 IN_PROC_BROWSER_TEST_F(BrowserActionsContainerTest, ForceHide) {