Browser Action Container should not shrink when visible extension is disabled.

We now calculate the visible actions before removing them from the vector
so we can maintain the same number after deletion occurs.

Also added unit tests and fixed a bug in GetTooltip where we were hard-coding
the index to be 0.

BUG=35349
TEST=Covered by new unit test.

Review URL: http://codereview.chromium.org/606008

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@38959 0039d316-1c4b-4281-b951-d872f2087c98

8 files changed, 251 insertions, 21 deletions

diff --git a/chrome/browser/extensions/browser_action_test_util.h b/chrome/browser/extensions/browser_action_test_util.h
index 6af89b4..df3e2cf 100644
--- a/chrome/browser/extensions/browser_action_test_util.h
+++ b/chrome/browser/extensions/browser_action_test_util.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2009 The Chromium Authors. All rights reserved.
+// Copyright (c) 2010 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.
 
@@ -21,12 +21,18 @@ class BrowserActionTestUtil {
   // Returns the number of browser action buttons in the window toolbar.
   int NumberOfBrowserActions();
 
+  // Returns the number of browser action currently visible.
+  int VisibleBrowserActions();
+
   // Returns whether the browser action at |index| has a non-null icon.
   bool HasIcon(int index);
 
   // Simulates a user click on the browser action button at |index|.
   void Press(int index);
 
+  // Returns the extension id of the extension at |index|.
+  std::string GetExtensionId(int index);
+
   // Returns the current tooltip for the browser action button.
   std::string GetTooltip(int index);
 
@@ -39,6 +45,9 @@ class BrowserActionTestUtil {
   // Hides the given popup and returns whether the hide was successful.
   bool HidePopup();
 
+  // Set how many icons should be visible.
+  void SetIconVisibilityCount(size_t icons);
+
   // Returns the minimum allowed size of an extension popup.
   static gfx::Size GetMinPopupSize();
 
diff --git a/chrome/browser/extensions/browser_action_test_util_views.cc b/chrome/browser/extensions/browser_action_test_util_views.cc
index c116f22..3e6b2972 100644
--- a/chrome/browser/extensions/browser_action_test_util_views.cc
+++ b/chrome/browser/extensions/browser_action_test_util_views.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2009 The Chromium Authors. All rights reserved.
+// Copyright (c) 2010 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.
 
@@ -30,6 +30,10 @@ int BrowserActionTestUtil::NumberOfBrowserActions() {
   return GetContainer(browser_)->num_browser_actions();
 }
 
+int BrowserActionTestUtil::VisibleBrowserActions() {
+  return GetContainer(browser_)->VisibleBrowserActions();
+}
+
 bool BrowserActionTestUtil::HasIcon(int index) {
   return !GetContainer(browser_)->GetBrowserActionViewAt(index)->
       button()->icon().empty();
@@ -39,6 +43,12 @@ void BrowserActionTestUtil::Press(int index) {
   GetContainer(browser_)->TestExecuteBrowserAction(index);
 }
 
+std::string BrowserActionTestUtil::GetExtensionId(int index) {
+  BrowserActionButton* button =
+      GetContainer(browser_)->GetBrowserActionViewAt(index)->button();
+  return button->extension()->id();
+}
+
 std::string BrowserActionTestUtil::GetTooltip(int index) {
   std::wstring text;
   GetContainer(browser_)->GetBrowserActionViewAt(index)->button()->
@@ -60,6 +70,10 @@ bool BrowserActionTestUtil::HidePopup() {
   return !HasPopup();
 }
 
+void BrowserActionTestUtil::SetIconVisibilityCount(size_t icons) {
+  GetContainer(browser_)->TestSetIconVisibilityCount(icons);
+}
+
 gfx::Size BrowserActionTestUtil::GetMinPopupSize() {
   return gfx::Size(ExtensionPopup::kMinWidth, ExtensionPopup::kMinHeight);
 }
diff --git a/chrome/browser/extensions/extension_browsertest.cc b/chrome/browser/extensions/extension_browsertest.cc
index 7bb8084..9b4ddd8 100644
--- a/chrome/browser/extensions/extension_browsertest.cc
+++ b/chrome/browser/extensions/extension_browsertest.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2009 The Chromium Authors. All rights reserved.
+// Copyright (c) 2010 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.
 
@@ -76,8 +76,8 @@ class MockAbortExtensionInstallUI : public ExtensionInstallUI {
   MockAbortExtensionInstallUI() : ExtensionInstallUI(NULL) {}
 
   // Simulate a user abort on an extension installation.
-  void ConfirmInstall(Delegate* delegate, Extension* extension, SkBitmap* icon)
-  {
+  void ConfirmInstall(
+      Delegate* delegate, Extension* extension, SkBitmap* icon) {
     delegate->InstallUIAbort();
     MessageLoopForUI::current()->Quit();
   }
@@ -165,6 +165,16 @@ void ExtensionBrowserTest::UninstallExtension(const std::string& extension_id) {
   service->UninstallExtension(extension_id, false);
 }
 
+void ExtensionBrowserTest::DisableExtension(const std::string& extension_id) {
+  ExtensionsService* service = browser()->profile()->GetExtensionsService();
+  service->DisableExtension(extension_id);
+}
+
+void ExtensionBrowserTest::EnableExtension(const std::string& extension_id) {
+  ExtensionsService* service = browser()->profile()->GetExtensionsService();
+  service->EnableExtension(extension_id);
+}
+
 bool ExtensionBrowserTest::WaitForPageActionCountChangeTo(int count) {
   NotificationRegistrar registrar;
   registrar.Add(this,
diff --git a/chrome/browser/extensions/extension_browsertest.h b/chrome/browser/extensions/extension_browsertest.h
index a284607..263f582 100644
--- a/chrome/browser/extensions/extension_browsertest.h
+++ b/chrome/browser/extensions/extension_browsertest.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2009 The Chromium Authors. All rights reserved.
+// Copyright (c) 2010 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.
 
@@ -51,6 +51,10 @@ class ExtensionBrowserTest
 
   void UninstallExtension(const std::string& extension_id);
 
+  void DisableExtension(const std::string& extension_id);
+
+  void EnableExtension(const std::string& extension_id);
+
   // Wait for the total number of page actions to change to |count|.
   bool WaitForPageActionCountChangeTo(int count);
 
diff --git a/chrome/browser/views/browser_actions_container.cc b/chrome/browser/views/browser_actions_container.cc
index be8cf1b..9f26598 100644
--- a/chrome/browser/views/browser_actions_container.cc
+++ b/chrome/browser/views/browser_actions_container.cc
@@ -37,6 +37,8 @@
 
 #include "grit/theme_resources.h"
 
+namespace {
+
 // The size (both dimensions) of the buttons for page actions.
 static const int kButtonSize = 29;
 
@@ -79,6 +81,11 @@ static const SkColor kDropIndicatorColor = SK_ColorBLACK;
 static const int kDropIndicatorOffsetLtr = 3;
 static const int kDropIndicatorOffsetRtl = 9;
 
+}  // namespace.
+
+// Static.
+bool BrowserActionsContainer::disable_animations_during_testing_ = false;
+
 ////////////////////////////////////////////////////////////////////////////////
 // BrowserActionButton
 
@@ -502,6 +509,13 @@ void BrowserActionsContainer::TestExecuteBrowserAction(int index) {
   OnBrowserActionExecuted(button);
 }
 
+void BrowserActionsContainer::TestSetIconVisibilityCount(size_t icons) {
+  chevron_->SetVisible(icons < browser_action_views_.size());
+  container_size_.set_width(IconCountToWidth(icons));
+  Layout();
+  SchedulePaint();
+}
+
 void BrowserActionsContainer::OnBrowserActionExecuted(
     BrowserActionButton* button) {
   ExtensionAction* browser_action = button->browser_action();
@@ -602,7 +616,8 @@ void BrowserActionsContainer::Layout() {
       ((browser_action_views_.size() - 1) *
           kBrowserActionButtonPadding);
 
-  int max_x = width() - kDividerHorizontalMargin - kChevronRightMargin;
+  gfx::Size sz = GetPreferredSize();
+  int max_x = sz.width() - kDividerHorizontalMargin - kChevronRightMargin;
 
   // If they don't all fit, show the chevron (unless suppressed).
   gfx::Size chevron_size;
@@ -974,10 +989,7 @@ void BrowserActionsContainer::BrowserActionAdded(Extension* extension,
     // in the header for why we do this.
     suppress_chevron_ = !chevron_->IsVisible();
 
-    // Animate!
-    resize_animation_->Reset();
-    resize_animation_->SetTweenType(SlideAnimation::NONE);
-    resize_animation_->Show();
+    Animate(SlideAnimation::NONE);
   }
 }
 
@@ -987,6 +999,10 @@ void BrowserActionsContainer::BrowserActionRemoved(Extension* extension) {
   if (popup_ && popup_->host()->extension() == extension)
     HidePopup();
 
+  // Before we change anything, determine the number of visible browser
+  // actions.
+  size_t visible_actions = VisibleBrowserActions();
+
   for (BrowserActionViews::iterator iter =
        browser_action_views_.begin(); iter != browser_action_views_.end();
        ++iter) {
@@ -1003,10 +1019,6 @@ void BrowserActionsContainer::BrowserActionRemoved(Extension* extension) {
       // For details on why we do the following see the class comments in the
       // header.
 
-      // Before we change anything, determine the number of visible browser
-      // actions.
-      size_t visible_actions = VisibleBrowserActions();
-
       // Calculate the target size we'll animate to (end state). This might be
       // the same size (if the icon we are removing is in the overflow bucket
       // and there are other icons there). We don't decrement visible_actions
@@ -1015,10 +1027,7 @@ void BrowserActionsContainer::BrowserActionRemoved(Extension* extension) {
       animation_target_size_ =
           ClampToNearestIconCount(IconCountToWidth(visible_actions), true);
 
-      // Animate!
-      resize_animation_->Reset();
-      resize_animation_->SetTweenType(SlideAnimation::EASE_OUT);
-      resize_animation_->Show();
+      Animate(SlideAnimation::EASE_OUT);
       return;
     }
   }
@@ -1063,6 +1072,18 @@ int BrowserActionsContainer::ContainerMinSize() const {
   return resize_gripper_->width() + chevron_->width() + kChevronRightMargin;
 }
 
+void BrowserActionsContainer::Animate(
+    SlideAnimation::TweenType tween_type) {
+  if (!disable_animations_during_testing_) {
+    // Animate!
+    resize_animation_->Reset();
+    resize_animation_->SetTweenType(tween_type);
+    resize_animation_->Show();
+  } else {
+    AnimationEnded(resize_animation_.get());
+  }
+}
+
 size_t BrowserActionsContainer::VisibleBrowserActions() const {
   size_t visible_actions = 0;
   for (size_t i = 0; i < browser_action_views_.size(); ++i) {
diff --git a/chrome/browser/views/browser_actions_container.h b/chrome/browser/views/browser_actions_container.h
index 1ace12d..5df29f1 100644
--- a/chrome/browser/views/browser_actions_container.h
+++ b/chrome/browser/views/browser_actions_container.h
@@ -9,6 +9,7 @@
 #include <string>
 #include <vector>
 
+#include "app/slide_animation.h"
 #include "base/task.h"
 #include "chrome/browser/extensions/extension_toolbar_model.h"
 #include "chrome/browser/extensions/image_loading_tracker.h"
@@ -31,7 +32,6 @@ class ExtensionAction;
 class ExtensionPopup;
 class PrefService;
 class Profile;
-class SlideAnimation;
 
 ////////////////////////////////////////////////////////////////////////////////
 // BrowserActionButton
@@ -353,6 +353,15 @@ class BrowserActionsContainer
   // Retrieve the current popup.  This should only be used by unit tests.
   ExtensionPopup* TestGetPopup() { return popup_; }
 
+  // Set how many icons the container should show. This should only be used by
+  // unit tests.
+  void TestSetIconVisibilityCount(size_t icons);
+
+  // During testing we can disable animations by setting this flag to true,
+  // so that the bar resizes instantly, instead of having to poll it while it
+  // animates to open/closed status.
+  static bool disable_animations_during_testing_;
+
  private:
   typedef std::vector<BrowserActionView*> BrowserActionViews;
 
@@ -399,6 +408,10 @@ class BrowserActionsContainer
   // all the padding that we normally show if there are icons.
   int ContainerMinSize() const;
 
+  // Animate to the target value (unless testing, in which case we go straight
+  // to the target size).
+  void Animate(SlideAnimation::TweenType tween_type);
+
   // Returns true if this extension should be shown in this toolbar. This can
   // return false if we are in an incognito window and the extension is disabled
   // for incognito.
@@ -413,7 +426,7 @@ class BrowserActionsContainer
 
   Profile* profile_;
 
-  // The Browser object the containier is associated with.
+  // The Browser object the container is associated with.
   Browser* browser_;
 
   // The view that owns us.
diff --git a/chrome/browser/views/browser_actions_container_unittest.cc b/chrome/browser/views/browser_actions_container_unittest.cc
new file mode 100644
index 0000000..763bb4b
--- /dev/null
+++ b/chrome/browser/views/browser_actions_container_unittest.cc
@@ -0,0 +1,158 @@
+// Copyright (c) 2010 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/extensions/browser_action_test_util.h"
+#include "chrome/browser/extensions/extension_browsertest.h"
+#include "chrome/browser/views/browser_actions_container.h"
+#include "chrome/common/chrome_switches.h"
+
+class BrowserActionsContainerTest : public ExtensionBrowserTest {
+ public:
+  BrowserActionsContainerTest() {
+  }
+  virtual ~BrowserActionsContainerTest() {}
+
+  virtual Browser* CreateBrowser(Profile* profile) {
+    Browser* b = InProcessBrowserTest::CreateBrowser(profile);
+    browser_actions_bar_.reset(new BrowserActionTestUtil(b));
+    return b;
+  }
+
+  BrowserActionTestUtil* browser_actions_bar() {
+    return browser_actions_bar_.get();
+  }
+
+ private:
+  scoped_ptr<BrowserActionTestUtil> browser_actions_bar_;
+};
+
+// Test the basic functionality.
+IN_PROC_BROWSER_TEST_F(BrowserActionsContainerTest, Basic) {
+  CommandLine::ForCurrentProcess()->AppendSwitch(
+      switches::kEnableExperimentalExtensionApis);
+
+  BrowserActionsContainer::disable_animations_during_testing_ = true;
+
+  // Load an extension with no browser action.
+  ASSERT_TRUE(LoadExtension(test_data_dir_.AppendASCII("api_test")
+                                          .AppendASCII("browser_action")
+                                          .AppendASCII("none")));
+  // This extension should not be in the model (has no browser action).
+  EXPECT_EQ(0, browser_actions_bar()->NumberOfBrowserActions());
+
+  // Load an extension with a browser action.
+  ASSERT_TRUE(LoadExtension(test_data_dir_.AppendASCII("api_test")
+                                          .AppendASCII("browser_action")
+                                          .AppendASCII("basics")));
+  EXPECT_EQ(1, browser_actions_bar()->NumberOfBrowserActions());
+  EXPECT_TRUE(browser_actions_bar()->HasIcon(0));
+
+  // Unload the extension.
+  std::string id = browser_actions_bar()->GetExtensionId(0);
+  UnloadExtension(id);
+  EXPECT_EQ(0, browser_actions_bar()->NumberOfBrowserActions());
+}
+
+#if defined(OS_LINUX) && defined(TOOLKIT_VIEWS)
+// Linux with toolkits doesn't yet support an overflow for the browser actions
+// container, so it doesn't make sense to test that feature yet.
+// See http://crbug.com/35593.
+#define MAYBE_TestVisibility DISABLED_TestVisibility
+#else
+#define MAYBE_TestVisibility TestVisibility
+#endif
+
+IN_PROC_BROWSER_TEST_F(BrowserActionsContainerTest, MAYBE_TestVisibility) {
+  CommandLine::ForCurrentProcess()->AppendSwitch(
+      switches::kEnableExperimentalExtensionApis);
+
+  BrowserActionsContainer::disable_animations_during_testing_ = true;
+
+  // Load extension A (contains browser action).
+  ASSERT_TRUE(LoadExtension(test_data_dir_.AppendASCII("api_test")
+                                          .AppendASCII("browser_action")
+                                          .AppendASCII("basics")));
+  EXPECT_EQ(1, browser_actions_bar()->NumberOfBrowserActions());
+  EXPECT_TRUE(browser_actions_bar()->HasIcon(0));
+  EXPECT_EQ(1, browser_actions_bar()->VisibleBrowserActions());
+  std::string idA = browser_actions_bar()->GetExtensionId(0);
+
+  // Load extension B (contains browser action).
+  ASSERT_TRUE(LoadExtension(test_data_dir_.AppendASCII("api_test")
+                                          .AppendASCII("browser_action")
+                                          .AppendASCII("add_popup")));
+  EXPECT_EQ(2, browser_actions_bar()->NumberOfBrowserActions());
+  EXPECT_TRUE(browser_actions_bar()->HasIcon(1));
+  EXPECT_EQ(2, browser_actions_bar()->VisibleBrowserActions());
+  std::string idB = browser_actions_bar()->GetExtensionId(1);
+
+  EXPECT_STRNE(idA.c_str(), idB.c_str());
+
+  // Load extension C (contains browser action).
+  ASSERT_TRUE(LoadExtension(test_data_dir_.AppendASCII("api_test")
+                                          .AppendASCII("browser_action")
+                                          .AppendASCII("remove_popup")));
+  EXPECT_EQ(3, browser_actions_bar()->NumberOfBrowserActions());
+  EXPECT_TRUE(browser_actions_bar()->HasIcon(2));
+  EXPECT_EQ(3, browser_actions_bar()->VisibleBrowserActions());
+  std::string idC = browser_actions_bar()->GetExtensionId(2);
+
+  // Change container to show only one action, rest in overflow: A, [B, C].
+  browser_actions_bar()->SetIconVisibilityCount(1);
+  EXPECT_EQ(1, browser_actions_bar()->VisibleBrowserActions());
+
+  // Disable extension A (should disappear). State becomes: B [C].
+  DisableExtension(idA);
+  EXPECT_EQ(2, browser_actions_bar()->NumberOfBrowserActions());
+  EXPECT_EQ(1, browser_actions_bar()->VisibleBrowserActions());
+  EXPECT_STREQ(idB.c_str(), browser_actions_bar()->GetExtensionId(0).c_str());
+
+  // Enable A again. A should get its spot in the same location and the bar
+  // should not grow (chevron is showing). For details: http://crbug.com/35349.
+  // State becomes: A, [B, C].
+  EnableExtension(idA);
+  EXPECT_EQ(3, browser_actions_bar()->NumberOfBrowserActions());
+  EXPECT_EQ(1, browser_actions_bar()->VisibleBrowserActions());
+  EXPECT_STREQ(idA.c_str(), browser_actions_bar()->GetExtensionId(0).c_str());
+
+  // Disable C (in overflow). State becomes: A, [B].
+  DisableExtension(idC);
+  EXPECT_EQ(2, browser_actions_bar()->NumberOfBrowserActions());
+  EXPECT_EQ(1, browser_actions_bar()->VisibleBrowserActions());
+  EXPECT_STREQ(idA.c_str(), browser_actions_bar()->GetExtensionId(0).c_str());
+
+  // Enable C again. State becomes: A, [B, C].
+  EnableExtension(idC);
+  EXPECT_EQ(3, browser_actions_bar()->NumberOfBrowserActions());
+  EXPECT_EQ(1, browser_actions_bar()->VisibleBrowserActions());
+  EXPECT_STREQ(idA.c_str(), browser_actions_bar()->GetExtensionId(0).c_str());
+
+  // Now we have 3 extensions. Make sure they are all visible. State: A, B, C.
+  browser_actions_bar()->SetIconVisibilityCount(3);
+  EXPECT_EQ(3, browser_actions_bar()->VisibleBrowserActions());
+
+  // Disable extension A (should disappear). State becomes: B, C.
+  DisableExtension(idA);
+  EXPECT_EQ(2, browser_actions_bar()->NumberOfBrowserActions());
+  EXPECT_EQ(2, browser_actions_bar()->VisibleBrowserActions());
+  EXPECT_STREQ(idB.c_str(), browser_actions_bar()->GetExtensionId(0).c_str());
+
+  // Disable extension B (should disappear). State becomes: C.
+  DisableExtension(idB);
+  EXPECT_EQ(1, browser_actions_bar()->NumberOfBrowserActions());
+  EXPECT_EQ(1, browser_actions_bar()->VisibleBrowserActions());
+  EXPECT_STREQ(idC.c_str(), browser_actions_bar()->GetExtensionId(0).c_str());
+
+  // Enable B (makes B and C showing now). State becomes: B, C.
+  EnableExtension(idB);
+  EXPECT_EQ(2, browser_actions_bar()->NumberOfBrowserActions());
+  EXPECT_EQ(2, browser_actions_bar()->VisibleBrowserActions());
+  EXPECT_STREQ(idB.c_str(), browser_actions_bar()->GetExtensionId(0).c_str());
+
+  // Enable A (makes A, B and C showing now). State becomes: B, C, A.
+  EnableExtension(idA);
+  EXPECT_EQ(3, browser_actions_bar()->NumberOfBrowserActions());
+  EXPECT_EQ(3, browser_actions_bar()->VisibleBrowserActions());
+  EXPECT_STREQ(idA.c_str(), browser_actions_bar()->GetExtensionId(2).c_str());
+}
diff --git a/chrome/chrome_tests.gypi b/chrome/chrome_tests.gypi
index 6e07b4d..f1c9740 100755
--- a/chrome/chrome_tests.gypi
+++ b/chrome/chrome_tests.gypi
@@ -6,6 +6,7 @@
   'variables' : {
     'browser_tests_sources_views_specific': [
       'browser/extensions/browser_action_test_util_views.cc',
+      'browser/views/browser_actions_container_unittest.cc',
       'browser/views/find_bar_host_browsertest.cc',
     ],
     'browser_tests_sources_win_specific': [