Add tests for the BrowserActionsContainer in overflow mode

chrome/browser/ui/views/toolbar/browser_actions_container_browsertest.cc

Index: chrome/browser/ui/views/toolbar/browser_actions_container_browsertest.cc
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 4b943c79b6de404011ab7878fc0d028aad24b59a..f7cd77a7a74d1863b6359a3cb1e0c00104f7080f 100644
--- a/chrome/browser/ui/views/toolbar/browser_actions_container_browsertest.cc
+++ b/chrome/browser/ui/views/toolbar/browser_actions_container_browsertest.cc
@@ -11,6 +11,7 @@
 #include "chrome/browser/extensions/extension_toolbar_model.h"
 #include "chrome/browser/ui/browser_window.h"
 #include "chrome/browser/ui/browser_window_testing_views.h"
+#include "chrome/browser/ui/views/frame/browser_view.h"
 #include "chrome/browser/ui/views/toolbar/browser_action_view.h"
 #include "chrome/browser/ui/views/toolbar/toolbar_view.h"
 #include "content/public/test/test_utils.h"
@@ -20,6 +21,9 @@
 #include "ui/views/view.h"
 
 using extensions::Extension;
+using extensions::ExtensionToolbarModel;
+using testing::AssertionFailure;
+using testing::AssertionSuccess;

[Peter Kasting, 2014/08/22 18:00:52]

Nit: Avoid using statements unless they significantly improve readability. 
ExtensionToolbarModel is already used in a qualified way in several places in
the file; the other two you added are used a small number of times.  I'd remove
all of these, and see if it's possible to remove the existing one as well.

[Devlin, 2014/08/22 20:10:59]

Done.

 
 class BrowserActionsContainerTest : public ExtensionBrowserTest {
  public:
@@ -290,3 +294,173 @@ IN_PROC_BROWSER_TEST_F(BrowserActionsContainerTest, HighlightMode) {
   action_view = container->GetBrowserActionViewAt(0);
   EXPECT_TRUE(container->CanStartDragForView(action_view, point, point));
 }
+
+// Test the behavior of the overflow container for Extension Actions.
+class BrowserActionsContainerOverflowTest : public BrowserActionsContainerTest {
+ public:
+  BrowserActionsContainerOverflowTest() : main_bar_(NULL), model_(NULL) {
+  }
+  virtual ~BrowserActionsContainerOverflowTest() {
+  }
+
+ protected:
+  // Returns true if the order of the BrowserActionViews in |main_bar_|
+  // and |overflow_bar_| match.
+  bool ViewOrdersMatch();
+
+  // Returns Success if the visible count matches |expected_visible|. This means
+  // that the number of visible browser actions in |main_bar_| is
+  // |expected_visible| and shows the first icons, and that the overflow bar
+  // shows all (and only) the remainder.
+  testing::AssertionResult VerifyVisibleCount(size_t expected_visible);
+
+  // Accessors.
+  BrowserActionsContainer* main_bar() { return main_bar_; }
+  BrowserActionsContainer* overflow_bar() { return overflow_bar_.get(); }
+  ExtensionToolbarModel* model() { return model_; }
+
+ private:
+  virtual void SetUpCommandLine(base::CommandLine* command_line) OVERRIDE;
+  virtual void SetUpOnMainThread() OVERRIDE;
+  virtual void TearDownOnMainThread() OVERRIDE;
+
+  // The main BrowserActionsContainer (owned by the browser view).
+  BrowserActionsContainer* main_bar_;
+
+  // The overflow BrowserActionsContainer. We manufacture this so that we don't
+  // have to open the wrench menu.
+  scoped_ptr<BrowserActionsContainer> overflow_bar_;
+
+  // The associated toolbar model.
+  ExtensionToolbarModel* model_;
+
+  // Enable the feature redesign switch.
+  scoped_ptr<extensions::FeatureSwitch::ScopedOverride> enable_redesign_;
+};

[Peter Kasting, 2014/08/22 18:00:52]

Nit: DISALLOW_COPY_AND_ASSIGN

[Devlin, 2014/08/22 20:10:58]

Done.

+
+void BrowserActionsContainerOverflowTest::SetUpCommandLine(
+    base::CommandLine* command_line) {
+  BrowserActionsContainerTest::SetUpCommandLine(command_line);
+  enable_redesign_.reset(new extensions::FeatureSwitch::ScopedOverride(
+      extensions::FeatureSwitch::extension_action_redesign(),
+      true));
+}
+
+void BrowserActionsContainerOverflowTest::SetUpOnMainThread() {
+  BrowserActionsContainerTest::SetUpOnMainThread();
+  main_bar_ = BrowserView::GetBrowserViewForBrowser(browser())
+                  ->toolbar()->browser_actions();
+  overflow_bar_.reset(new BrowserActionsContainer(browser(), NULL, main_bar_));
+  overflow_bar_->set_owned_by_client();
+  model_ = ExtensionToolbarModel::Get(profile());
+}
+
+void BrowserActionsContainerOverflowTest::TearDownOnMainThread() {
+  overflow_bar_.reset();
+  enable_redesign_.reset();
+  BrowserActionsContainerTest::TearDownOnMainThread();
+}
+
+bool BrowserActionsContainerOverflowTest::ViewOrdersMatch() {
+  if (main_bar_->num_browser_actions() !=
+      overflow_bar_->num_browser_actions())
+    return false;
+  for (size_t i = 0; i < main_bar_->num_browser_actions(); ++i) {
+    if (main_bar_->GetBrowserActionViewAt(i)->extension() !=
+        overflow_bar_->GetBrowserActionViewAt(i)->extension())
+      return false;
+  }
+  return true;
+}
+
+testing::AssertionResult
+BrowserActionsContainerOverflowTest::VerifyVisibleCount(
+    size_t expected_visible) {
+  // Views order should always match (as it is based directly off the model).
+  if (!ViewOrdersMatch())
+    return AssertionFailure() << "View orders don't match";
+
+  // Loop through and check each browser action for proper visibility (which
+  // implicitly also guarantees that the proper number are visible).
+  for (size_t i = 0; i < overflow_bar_->num_browser_actions(); ++i) {
+    bool visible = i < expected_visible;
+    if (main_bar_->GetBrowserActionViewAt(i)->visible() != visible) {
+      return AssertionFailure() << "Index " << i <<
+          " has improper visibility in main: " << !visible;
+    }
+    if (overflow_bar_->GetBrowserActionViewAt(i)->visible() == visible) {
+      return AssertionFailure() << "Index " << i <<
+          " has improper visibility in overflow: " << visible;
+    }
+  }
+  return AssertionSuccess();
+}
+
+// Test the basic functionality of the BrowserActionsContainer in overflow mode.
+IN_PROC_BROWSER_TEST_F(BrowserActionsContainerOverflowTest,
+                       TestBasicActionOverflow) {
+  // Load three extensions with browser actions.
+  // TODO(devlin): Make a method to load these, and generate them rather than
+  // using files.
+  const Extension* extension_a =
+      LoadExtension(test_data_dir_.AppendASCII("api_test")
+                                  .AppendASCII("browser_action")

[Peter Kasting, 2014/08/22 18:00:52]

Nit: Why not pull
"test_data_dir_.AppendASCII("api_test").AppendASCII("browser_action")" out to a
temp?

[Devlin, 2014/08/22 20:10:59]

Done.

+                                  .AppendASCII("basics"));
+  const Extension* extension_b =
+      LoadExtension(test_data_dir_.AppendASCII("api_test")
+                                  .AppendASCII("browser_action")
+                                  .AppendASCII("add_popup"));
+  const Extension* extension_c =
+      LoadExtension(test_data_dir_.AppendASCII("api_test")
+                                  .AppendASCII("browser_action")
+                                  .AppendASCII("remove_popup"));
+
+  // Since the overflow bar isn't attached to a view, we kinda have to kick it
+  // from time to time.

[Peter Kasting, 2014/08/22 18:00:52]

Nit: Be less colloquial; say precisely when you need to call layout ("we need to
manually retrigger layout each time we change the number of icons in the bar",
that sort of thing).

[Devlin, 2014/08/22 20:10:58]

Done.

+  overflow_bar()->Layout();
+
+  // Sanity checks:
+  // All extensions loaded.
+  ASSERT_TRUE(extension_a);
+  ASSERT_TRUE(extension_b);
+  ASSERT_TRUE(extension_c);
+
+  // All actions are showing, and are in the installation order.
+  EXPECT_EQ(-1, model()->GetVisibleIconCount());
+  ASSERT_EQ(3u, main_bar()->num_browser_actions());
+  EXPECT_EQ(extension_a, main_bar()->GetBrowserActionViewAt(0)->extension());
+  EXPECT_EQ(extension_b, main_bar()->GetBrowserActionViewAt(1)->extension());
+  EXPECT_EQ(extension_c, main_bar()->GetBrowserActionViewAt(2)->extension());
+  EXPECT_TRUE(VerifyVisibleCount(3u));
+
+  // Reduce the visible count to 2. Order should be unchanged (A B C), but
+  // only A and B should be visible on the main bar.
+  model()->SetVisibleIconCountForTest(2u);
+  overflow_bar()->Layout();  // Kick.
+  EXPECT_EQ(extension_a, main_bar()->GetBrowserActionViewAt(0)->extension());
+  EXPECT_EQ(extension_b, main_bar()->GetBrowserActionViewAt(1)->extension());
+  EXPECT_EQ(extension_c, main_bar()->GetBrowserActionViewAt(2)->extension());
+  EXPECT_TRUE(VerifyVisibleCount(2u));
+
+  // Move extension C to the first position. Order should now be C A B, with
+  // C and A visible in the main bar.
+  model()->MoveExtensionIcon(extension_c, 0);
+  overflow_bar()->Layout();  // Kick.
+  EXPECT_EQ(extension_c, main_bar()->GetBrowserActionViewAt(0)->extension());
+  EXPECT_EQ(extension_a, main_bar()->GetBrowserActionViewAt(1)->extension());
+  EXPECT_EQ(extension_b, main_bar()->GetBrowserActionViewAt(2)->extension());
+  EXPECT_TRUE(VerifyVisibleCount(2u));
+
+  // Hide action A. This results in it being sent to overflow, and reducing the
+  // visible size to 1, so the order should be C A B, with only C visible in the
+  // main bar.
+  extensions::ExtensionActionAPI::SetBrowserActionVisibility(
+      extensions::ExtensionPrefs::Get(profile()),
+      extension_a->id(),
+      false);
+  overflow_bar()->Layout();  // Kick.
+  EXPECT_EQ(extension_c, main_bar()->GetBrowserActionViewAt(0)->extension());
+  EXPECT_EQ(extension_a, main_bar()->GetBrowserActionViewAt(1)->extension());
+  EXPECT_EQ(extension_b, main_bar()->GetBrowserActionViewAt(2)->extension());
+  EXPECT_TRUE(VerifyVisibleCount(1u));
+}