[Extension Toolbar] Refactor and finish pop out logic for actions

chrome/browser/ui/toolbar/toolbar_actions_bar_unittest.cc

 // Copyright 2014 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/toolbar/toolbar_actions_bar.h"
 
 #include "base/command_line.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/run_loop.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/extensions/api/extension_action/extension_action_api.h"
 #include "chrome/browser/extensions/browser_action_test_util.h"
 #include "chrome/browser/extensions/extension_action.h"
 #include "chrome/browser/extensions/extension_action_manager.h"
 #include "chrome/browser/extensions/extension_action_test_util.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/test_extension_system.h"
 #include "chrome/browser/sessions/session_tab_helper.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/browser/ui/toolbar/test_toolbar_actions_bar_helper.h"
 #include "chrome/browser/ui/toolbar/toolbar_action_view_controller.h"
 #include "chrome/browser/ui/toolbar/toolbar_actions_bar_delegate.h"
 #include "chrome/test/base/browser_with_test_window_test.h"
 #include "extensions/browser/extension_system.h"
 #include "extensions/common/extension.h"
 #include "extensions/common/feature_switch.h"
 
+namespace {
+
+// Verifies that the toolbar order matches for the given |actions_bar|. If the
+// order matches, the return value is empty; otherwise, it contains the error.
+std::string VerifyToolbarOrderForBar(
+    const ToolbarActionsBar* actions_bar,
+    BrowserActionTestUtil* browser_action_test_util,
+    const char* expected_names[],
+    size_t total_size,
+    size_t visible_count) {
+  const std::vector<ToolbarActionViewController*>& toolbar_actions =
+      actions_bar->toolbar_actions();
+  // If the total size is wrong, we risk segfaulting by continuing. Abort now.
+  if (total_size != toolbar_actions.size()) {
+    return base::StringPrintf("Incorrect action count: expected %d, found %d",
+                              static_cast<int>(total_size),
+                              static_cast<int>(toolbar_actions.size()));

[Avi (use Gerrit), 2014/12/12 23:53:59]

That's what ASSERT_EQ is for, to stop things cold if it fails.

[Devlin, 2014/12/13 00:18:18]

See comment below about line numbers.  Also, we don't necessarily want to stop
the whole test cold - only this particular check.

+  }
+
+  // Check that the ToolbarActionsBar matches the expected state.
+  std::string error;
+  for (size_t i = 0; i < total_size; ++i) {
+    if (std::string(expected_names[i]) !=
+            base::UTF16ToUTF8(toolbar_actions[i]->GetActionName())) {
+      error += base::StringPrintf(
+          "Incorrect action in bar at index %d: expected '%s', found '%s'.\n",
+          static_cast<int>(i),
+          expected_names[i],
+          base::UTF16ToUTF8(toolbar_actions[i]->GetActionName()).c_str());
+    }
+  }
+  size_t icon_count = actions_bar->GetIconCount();
+  if (visible_count != icon_count)
+    error += base::StringPrintf(
+        "Incorrect visible count: expected %d, found %d.\n",
+        static_cast<int>(visible_count), static_cast<int>(icon_count));
+
+  // Test that the (platform-specific) toolbar view matches the expected state.
+  for (size_t i = 0; i < total_size; ++i) {
+    std::string id = browser_action_test_util->GetExtensionId(i);
+    if (id != toolbar_actions[i]->GetId()) {
+      error += base::StringPrintf(
+          "Incorrect action in view at index %d: expected '%s', found '%s'.\n",
+          static_cast<int>(i),
+          toolbar_actions[i]->GetId().c_str(),
+          id.c_str());
+    }
+  }
+  size_t view_icon_count = browser_action_test_util->VisibleBrowserActions();
+  if (visible_count != view_icon_count)
+    error += base::StringPrintf(
+        "Incorrect visible count in view: expected %d, found %d.\n",
+        static_cast<int>(visible_count), static_cast<int>(view_icon_count));
+
+  return error;
+}
+
+}  // namespace
+
 // A cross-platform unit test for the ToolbarActionsBar that uses the
 // TestToolbarActionsBarHelper to create the platform-specific containers.
 // TODO(devlin): Since this *does* use the real platform containers, in theory,
 // we can move all the BrowserActionsBarBrowserTests to be unittests. See about
 // doing this.
 class ToolbarActionsBarUnitTest : public BrowserWithTestWindowTest {
  public:
-  ToolbarActionsBarUnitTest() : toolbar_model_(nullptr) {}
+  ToolbarActionsBarUnitTest() : toolbar_model_(nullptr), use_redesign_(false) {}
+
+  // A constructor to allow subclasses to override the redesign value.
+  explicit ToolbarActionsBarUnitTest(bool use_redesign)
+      : toolbar_model_(nullptr),
+        use_redesign_(use_redesign) {}
+
   ~ToolbarActionsBarUnitTest() override {}
 
  protected:
   void SetUp() override;
   void TearDown() override;
 
   // Activates the tab at the given |index| in the tab strip model.
   void ActivateTab(int index);
 
   // Set whether or not the given |action| wants to run on the |web_contents|.
@@ skipping 16 matching lines @@
   // in a debug message. These aren't enforced to be unique, so don't make
   // duplicates.
   // If any of these is wrong, returns testing::AssertionFailure() with a
   // message.
   testing::AssertionResult VerifyToolbarOrder(
       const char* expected_names[],
       size_t total_size,
       size_t visible_count) WARN_UNUSED_RESULT;
 
   ToolbarActionsBar* toolbar_actions_bar() {
-    return toolbar_actions_bar_helper_->GetToolbarActionsBar();
+    return main_bar_helper_->GetToolbarActionsBar();
+  }
+  ToolbarActionsBar* overflow_bar() {
+    return overflow_bar_helper_->GetToolbarActionsBar();
   }
   extensions::ExtensionToolbarModel* toolbar_model() {
     return toolbar_model_;
   }
   BrowserActionTestUtil* browser_action_test_util() {
     return browser_action_test_util_.get();
   }
+  BrowserActionTestUtil* overflow_browser_action_test_util() {
+    return overflow_browser_action_test_util_.get();
+  }
 
  private:
   // The test helper that owns the ToolbarActionsBar and the platform-specific
   // view for it.
-  scoped_ptr<TestToolbarActionsBarHelper> toolbar_actions_bar_helper_;
+  scoped_ptr<TestToolbarActionsBarHelper> main_bar_helper_;
+
+  // The test helper for the overflow bar; only non-null if |use_redesign| is
+  // true.
+  scoped_ptr<TestToolbarActionsBarHelper> overflow_bar_helper_;
 
   // The associated ExtensionToolbarModel (owned by the keyed service setup).
   extensions::ExtensionToolbarModel* toolbar_model_;
 
   // A BrowserActionTestUtil object constructed with the associated
   // ToolbarActionsBar.
   scoped_ptr<BrowserActionTestUtil> browser_action_test_util_;
 
+  // The overflow container's BrowserActionTestUtil (only non-null if
+  // |use_redesign| is true).
+  scoped_ptr<BrowserActionTestUtil> overflow_browser_action_test_util_;
+
+  // True if the extension action redesign switch should be enabled.
+  bool use_redesign_;
+
+  scoped_ptr<extensions::FeatureSwitch::ScopedOverride> redesign_switch_;
+
   DISALLOW_COPY_AND_ASSIGN(ToolbarActionsBarUnitTest);
 };
 
 void ToolbarActionsBarUnitTest::SetUp() {
+  if (use_redesign_) {
+    redesign_switch_.reset(new extensions::FeatureSwitch::ScopedOverride(
+        extensions::FeatureSwitch::extension_action_redesign(), true));
+  }
+
   BrowserWithTestWindowTest::SetUp();
   // The toolbar typically displays extension icons, so create some extension
   // test infrastructure.
   extensions::TestExtensionSystem* extension_system =
       static_cast<extensions::TestExtensionSystem*>(
           extensions::ExtensionSystem::Get(profile()));
   extension_system->CreateExtensionService(
       base::CommandLine::ForCurrentProcess(),
       base::FilePath(),
       false);
   toolbar_model_ =
       extensions::extension_action_test_util::CreateToolbarModelForProfile(
           profile());
 
-  toolbar_actions_bar_helper_ = TestToolbarActionsBarHelper::Create(browser());
+  main_bar_helper_ = TestToolbarActionsBarHelper::Create(browser(), nullptr);
 
   BrowserActionTestUtil::DisableAnimations();
   browser_action_test_util_.reset(
       new BrowserActionTestUtil(browser(),
                                 toolbar_actions_bar()->delegate_for_test()));
+
+  if (use_redesign_) {
+    overflow_bar_helper_ =
+        TestToolbarActionsBarHelper::Create(browser(), main_bar_helper_.get());
+    overflow_browser_action_test_util_.reset(
+        new BrowserActionTestUtil(browser(),
+                                  overflow_bar()->delegate_for_test()));
+  }
 }
 
 void ToolbarActionsBarUnitTest::TearDown() {
   // Since the profile gets destroyed in BrowserWithTestWindowTest::TearDown(),
   // we need to delete this now.
-  toolbar_actions_bar_helper_.reset();
+  overflow_bar_helper_.reset();
+  main_bar_helper_.reset();
+  redesign_switch_.reset();
   BrowserWithTestWindowTest::TearDown();
 }
 
 void ToolbarActionsBarUnitTest::ActivateTab(int index) {
   ASSERT_NE(nullptr, browser()->tab_strip_model()->GetWebContentsAt(index));
   browser()->tab_strip_model()->ActivateTabAt(index, true);
   // Normally, the toolbar view would (indirectly) tell the toolbar actions
   // bar to update itself on tab change. Since this is a unittest, we have tell
   // it to.
   toolbar_actions_bar()->Update();
@@ skipping 17 matching lines @@
     bool wants_to_run) {
   action->SetIsVisible(SessionTabHelper::IdForTab(web_contents), wants_to_run);
   extensions::ExtensionActionAPI::Get(profile())->NotifyChange(
       action, web_contents, profile());
 }
 
 testing::AssertionResult ToolbarActionsBarUnitTest::VerifyToolbarOrder(
     const char* expected_names[],
     size_t total_size,
     size_t visible_count) {
-  std::vector<ToolbarActionViewController*> toolbar_actions =
-      toolbar_actions_bar()->toolbar_actions();
-  // If the total size is wrong, we risk segfaulting by continuing. Abort now.
-  if (total_size != toolbar_actions.size()) {
-    return testing::AssertionFailure() << "Incorrect action count: expected " <<
-        total_size << ", found " << toolbar_actions.size();
+  std::string main_bar_error =
+      VerifyToolbarOrderForBar(toolbar_actions_bar(),
+                               browser_action_test_util(),
+                               expected_names,
+                               total_size,
+                               visible_count);
+  std::string overflow_bar_error;
+  if (use_redesign_) {
+    overflow_bar_error =
+        VerifyToolbarOrderForBar(overflow_bar(),
+                                 overflow_browser_action_test_util(),
+                                 expected_names,
+                                 total_size,
+                                 total_size - visible_count);
+
   }
 
-  // Check that the ToolbarActionsBar matches the expected state.
-  std::string error;
-  for (size_t i = 0; i < total_size; ++i) {
-    if (std::string(expected_names[i]) !=
-            base::UTF16ToUTF8(toolbar_actions[i]->GetActionName())) {
-      error += base::StringPrintf(
-          "Incorrect action in bar at index %d: expected '%s', found '%s'.\n",
-          static_cast<int>(i),
-          expected_names[i],
-          base::UTF16ToUTF8(toolbar_actions[i]->GetActionName()).c_str());
-    }
-  }
-  size_t icon_count = toolbar_actions_bar()->GetIconCount();
-  if (visible_count != icon_count)
-    error += base::StringPrintf(
-        "Incorrect visible count: expected %d, found %d.\n",
-        static_cast<int>(visible_count), static_cast<int>(icon_count));
-
-  // Test that the (platform-specific) toolbar view matches the expected state.
-  for (size_t i = 0; i < total_size; ++i) {
-    std::string id = browser_action_test_util()->GetExtensionId(i);
-    if (id != toolbar_actions[i]->GetId()) {
-      error += base::StringPrintf(
-          "Incorrect action in view at index %d: expected '%s', found '%s'.\n",
-          static_cast<int>(i),
-          toolbar_actions[i]->GetId().c_str(),
-          id.c_str());
-    }
-  }
-  size_t view_icon_count = browser_action_test_util()->VisibleBrowserActions();
-  if (visible_count != view_icon_count)
-    error += base::StringPrintf(
-        "Incorrect visible count in view: expected %d, found %d.\n",
-        static_cast<int>(visible_count), static_cast<int>(view_icon_count));
-
-  return error.empty() ? testing::AssertionSuccess() :
-      testing::AssertionFailure() << error;
+  return main_bar_error.empty() && overflow_bar_error.empty() ?
+      testing::AssertionSuccess() :
+      testing::AssertionFailure() << "main bar error:\n" << main_bar_error <<

[Avi (use Gerrit), 2014/12/12 23:53:59]

What is this? What happened to ASSERT_EQ/EXPECT_EQ, and their friends?

[Devlin, 2014/12/13 00:18:18]

TL;DR: EXPECT/ASSERT are great in the body of a test, but not in multiply-called
helper functions.

Using EXPECT/ASSERT in helper functions is massively unhelpful in unit tests. 
In a test like this, we have the basic format of:

Test {
  DoSomething;
  CheckOrder();
  DoSomethingElse;
  CheckOrder();
  ....
}

where CheckOrder() is a helper function that checks the order.  But if this
CheckOrder() function uses EXPECT/ASSERT directly, then all the line numbers for
the failures are identical - which is very unhelpful when it could have been the
first, second, or nth CheckEqual() that failed.  By returning a
testing::AssertionResult, we can instead do:
Test {
  DoSomething;
  EXPECT_TRUE(CheckOrder());
  DoSomethingElse;
  EXPECT_TRUE(CheckOrder());
  ....
}
and then, when one of them fails, we get a real line number - allowing for much
more useful logs for what step of the test actually failed.

+          "overflow bar error:\n" << overflow_bar_error;
 }
 
-// A version of the ToolbarActionsBarUnitTest that enables the toolbar redesign.
-class ToolbarActionsBarUnitTestWithSwitch : public ToolbarActionsBarUnitTest {
- public:
-  ToolbarActionsBarUnitTestWithSwitch() {}
-  ~ToolbarActionsBarUnitTestWithSwitch() override {}
-
- protected:
-  void SetUp() override {
-    redesign_switch_.reset(new extensions::FeatureSwitch::ScopedOverride(
-        extensions::FeatureSwitch::extension_action_redesign(), true));
-    ToolbarActionsBarUnitTest::SetUp();
-  }
-  void TearDown() override {
-    redesign_switch_.reset();
-    ToolbarActionsBarUnitTest::TearDown();
-  }
-
- private:
-  scoped_ptr<extensions::FeatureSwitch::ScopedOverride> redesign_switch_;
-
-  DISALLOW_COPY_AND_ASSIGN(ToolbarActionsBarUnitTestWithSwitch);
-};
-
 TEST_F(ToolbarActionsBarUnitTest, BasicToolbarActionsBarTest) {
   // Add three extensions to the profile; this is the easiest way to have
   // toolbar actions.
   for (int i = 0; i < 3; ++i) {
     CreateAndAddExtension(
         base::StringPrintf("extension %d", i),
         extensions::extension_action_test_util::BROWSER_ACTION);
   }
 
   const ToolbarActionsBar::PlatformSettings& platform_settings =
@@ skipping 91 matching lines @@
   EXPECT_EQ(2u, toolbar_actions_bar()->GetIconCount());
 
   // If we shrink the bar so that a full icon can't fit, it should resize to
   // hide that icon.
   toolbar_actions_bar()->OnResizeComplete(width - 1);
   width -= ToolbarActionsBar::IconWidth(true);
   EXPECT_EQ(width, toolbar_actions_bar()->GetPreferredSize().width());
   EXPECT_EQ(1u, toolbar_actions_bar()->GetIconCount());
 }
 
+class ToolbarActionsBarPopOutUnitTest
+     : public ToolbarActionsBarUnitTest {
+ public:
+  ToolbarActionsBarPopOutUnitTest() : ToolbarActionsBarUnitTest(true) {}
+
+ protected:
+  void SetUp() override {
+    ToolbarActionsBar::pop_out_actions_to_run = true;
+    ToolbarActionsBarUnitTest::SetUp();
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(ToolbarActionsBarPopOutUnitTest);
+};
+
 // Test that toolbar actions can pop themselves out of overflow if they want to
 // act on a given tab.
-TEST_F(ToolbarActionsBarUnitTestWithSwitch, ActionsPopOutToAct) {
+TEST_F(ToolbarActionsBarPopOutUnitTest, ActionsPopOutToAct) {
   // Add three extensions to the profile; this is the easiest way to have
   // toolbar actions.
   const char kBrowserAction[] = "browser action";
   const char kPageAction[] = "page action";
   const char kSynthetic[] = "synthetic";  // This has a generated action icon.
 
   CreateAndAddExtension(kBrowserAction,
                         extensions::extension_action_test_util::BROWSER_ACTION);
   scoped_refptr<const extensions::Extension> page_action =
       CreateAndAddExtension(
@@ skipping 135 matching lines @@
     SetActionWantsToRunOnTab(action, web_contents, true);
 
     // Even though the second page action triggered first, the order of actions
     // wanting to run should respect the normal order of actions.
     const char* expected_names[] =
         { kPageAction, kPageAction2, kBrowserAction, kSynthetic };
     EXPECT_TRUE(VerifyToolbarOrder(expected_names, 4u, 2u));
   }
 }
 
-TEST_F(ToolbarActionsBarUnitTestWithSwitch, AdjustingActionsThatWantToAct) {
+TEST_F(ToolbarActionsBarPopOutUnitTest, AdjustingActionsThatWantToAct) {
   // Add three extensions to the profile; this is the easiest way to have
   // toolbar actions.
   const char kBrowserAction[] = "browser action";
   const char kPageAction[] = "page action";
   const char kSynthetic[] = "synthetic";  // This has a generated action icon.
 
   CreateAndAddExtension(kBrowserAction,
                         extensions::extension_action_test_util::BROWSER_ACTION);
   scoped_refptr<const extensions::Extension> page_action =
       CreateAndAddExtension(
@@ skipping 101 matching lines @@
     EXPECT_TRUE(VerifyToolbarOrder(expected_names, 3u, 3u));
     // Move "page action" to the overflow menu.
     toolbar_actions_bar()->OnDragDrop(
         0, 2, ToolbarActionsBar::DRAG_TO_OVERFLOW);
     const char* expected_names2[] = { kBrowserAction, kSynthetic, kPageAction };
     EXPECT_TRUE(VerifyToolbarOrder(expected_names2, 3u, 2u));
     SetActionWantsToRunOnTab(action, web_contents, false);
     EXPECT_TRUE(VerifyToolbarOrder(expected_names2, 3u, 2u));
   }
 }