[Extensions Menu] Clean up BrowserActionTestUtilViews

Clean up the BrowserActionTestUtilViews implementation a bit.
- Remove the TestToolbarActionsBarHelper base class from
  BrowserActionTestUtil; there's no need for it to be (conceptually)
  cross-platform.
- Cache the BrowserActionsContainer pointer on the
  BrowserActionTestUtilViews instance to avoid always doing a look-up
  through the GetContainer() method (and remove GetContainer()).
- Avoid caching the Browser on the object; it's only necessary for
  creating the overflow bar, and can be easily passed in to those calls
  (of which there are few).
- Finesse the constructors to make it clear when a
  BrowserActionsContainer is owned (through a manufactured view) vs not
  (by accessing one on the BrowserWindow).

Bug: 984654

Change-Id: Ibab94e0562fd04026f1b2642d6dbc170c049ead4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1877447
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Peter Boström <pbos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#709319}

chrome/browser/ui/extensions/browser_action_test_util.h

@@ -21,13 +21,6 @@ class Image;
 class Size;
 }  // namespace gfx
 
-// A class that creates and owns the platform-specific views for the browser
-// actions container. Specific implementations are in the .cc/.mm files.
-class TestToolbarActionsBarHelper {
- public:
-  virtual ~TestToolbarActionsBarHelper() {}
-};
-
 class BrowserActionTestUtil {
  public:
   // Constructs a BrowserActionTestUtil which, if |is_real_window| is false,
@@ -96,7 +89,8 @@ class BrowserActionTestUtil {
 
   // Creates and returns a BrowserActionTestUtil with an "overflow" container,
   // with this object's container as the main bar.
-  virtual std::unique_ptr<BrowserActionTestUtil> CreateOverflowBar() = 0;
+  virtual std::unique_ptr<BrowserActionTestUtil> CreateOverflowBar(
+      Browser* browser) = 0;
 
   // Returns the minimum allowed size of an extension popup.
   virtual gfx::Size GetMinPopupSize() = 0;

chrome/browser/ui/toolbar/browser_actions_bar_browsertest.cc

@@ -401,7 +401,7 @@ IN_PROC_BROWSER_TEST_F(BrowserActionsBarBrowserTest,
 IN_PROC_BROWSER_TEST_F(BrowserActionsBarBrowserTest,
                        OverflowedBrowserActionPopupTest) {
   std::unique_ptr<BrowserActionTestUtil> overflow_bar =
-      browser_actions_bar()->CreateOverflowBar();
+      browser_actions_bar()->CreateOverflowBar(browser());
 
   // Load up two extensions that have browser action popups.
   base::FilePath data_dir =
@@ -488,7 +488,7 @@ IN_PROC_BROWSER_TEST_F(BrowserActionsBarBrowserTest,
 IN_PROC_BROWSER_TEST_F(BrowserActionsBarBrowserTest,
                        OverflowedBrowserActionPopupTestRemoval) {
   std::unique_ptr<BrowserActionTestUtil> overflow_bar =
-      browser_actions_bar()->CreateOverflowBar();
+      browser_actions_bar()->CreateOverflowBar(browser());
 
   // Install an extension and shrink the visible count to zero so the extension
   // is overflowed.

chrome/browser/ui/toolbar/toolbar_actions_bar_unittest.cc

@@ -172,7 +172,7 @@ void ToolbarActionsBarUnitTest::SetUp() {
   browser_action_test_util_ = BrowserActionTestUtil::Create(browser(), false);
 
   overflow_browser_action_test_util_ =
-      browser_action_test_util_->CreateOverflowBar();
+      browser_action_test_util_->CreateOverflowBar(browser());
 }
 
 void ToolbarActionsBarUnitTest::TearDown() {

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

@@ -24,16 +24,14 @@
 #include "ui/views/test/test_views.h"
 #include "ui/views/widget/widget.h"
 
-namespace {
-
-// The views-specific implementation of the TestToolbarActionsBarHelper, which
-// creates and owns a BrowserActionsContainer.
-class TestToolbarActionsBarHelperViews
-    : public TestToolbarActionsBarHelper,
-      public BrowserActionsContainer::Delegate {
+// A helper class that owns an instance of a BrowserActionsContainer; this is
+// used when testing without an associated browser window, or if this is for
+// the overflow version of the bar.
+class BrowserActionTestUtilViews::TestToolbarActionsBarHelper
+    : public BrowserActionsContainer::Delegate {
  public:
-  TestToolbarActionsBarHelperViews(Browser* browser,
-                                   BrowserActionsContainer* main_bar)
+  TestToolbarActionsBarHelper(Browser* browser,
+                              BrowserActionsContainer* main_bar)
       : browser_actions_container_(
             new BrowserActionsContainer(browser, main_bar, this, true)) {
     container_parent_.set_owned_by_client();
@@ -41,6 +39,7 @@ class TestToolbarActionsBarHelperViews
     container_parent_.Layout();
     container_parent_.AddChildView(browser_actions_container_);
   }
+  ~TestToolbarActionsBarHelper() override = default;
 
   BrowserActionsContainer* browser_actions_container() {
     return browser_actions_container_;
@@ -60,87 +59,61 @@ class TestToolbarActionsBarHelperViews
 
  private:
   // The created BrowserActionsContainer. Owned by |container_parent_|.
-  BrowserActionsContainer* browser_actions_container_;
+  BrowserActionsContainer* const browser_actions_container_;
 
   // The parent of the BrowserActionsContainer, which directly owns the
   // container as part of the views hierarchy.
   views::ResizeAwareParentView container_parent_;
 
-  DISALLOW_COPY_AND_ASSIGN(TestToolbarActionsBarHelperViews);
+  DISALLOW_COPY_AND_ASSIGN(TestToolbarActionsBarHelper);
 };
 
-BrowserActionsContainer* GetContainer(Browser* browser,
-                                      TestToolbarActionsBarHelper* helper) {
-  if (helper) {
-    return static_cast<TestToolbarActionsBarHelperViews*>(helper)
-        ->browser_actions_container();
-  }
-  return BrowserView::GetBrowserViewForBrowser(browser)->toolbar()->
-      browser_actions();
-}
-
-}  // namespace
-
-BrowserActionTestUtilViews::BrowserActionTestUtilViews(Browser* browser)
-    : BrowserActionTestUtilViews(browser, true) {}
-
-BrowserActionTestUtilViews::BrowserActionTestUtilViews(Browser* browser,
-                                                       bool is_real_window)
-    : browser_(browser) {
-  if (!is_real_window)
-    test_helper_ =
-        std::make_unique<TestToolbarActionsBarHelperViews>(browser, nullptr);
-}
-
-BrowserActionTestUtilViews::~BrowserActionTestUtilViews() {}
+BrowserActionTestUtilViews::~BrowserActionTestUtilViews() = default;
 
 int BrowserActionTestUtilViews::NumberOfBrowserActions() {
-  return GetContainer(browser_, test_helper_.get())->num_toolbar_actions();
+  return browser_actions_container_->num_toolbar_actions();
 }
 
 int BrowserActionTestUtilViews::VisibleBrowserActions() {
-  return GetContainer(browser_, test_helper_.get())->VisibleBrowserActions();
+  return browser_actions_container_->VisibleBrowserActions();
 }
 
 void BrowserActionTestUtilViews::InspectPopup(int index) {
   ToolbarActionView* view =
-      GetContainer(browser_, test_helper_.get())->GetToolbarActionViewAt(index);
+      browser_actions_container_->GetToolbarActionViewAt(index);
   static_cast<ExtensionActionViewController*>(view->view_controller())->
       InspectPopup();
 }
 
 bool BrowserActionTestUtilViews::HasIcon(int index) {
-  return !GetContainer(browser_, test_helper_.get())
-              ->GetToolbarActionViewAt(index)
+  return !browser_actions_container_->GetToolbarActionViewAt(index)
               ->GetImage(views::Button::STATE_NORMAL)
               .isNull();
 }
 
 gfx::Image BrowserActionTestUtilViews::GetIcon(int index) {
-  gfx::ImageSkia icon = GetContainer(browser_, test_helper_.get())
-                            ->GetToolbarActionViewAt(index)
-                            ->GetIconForTest();
+  gfx::ImageSkia icon =
+      browser_actions_container_->GetToolbarActionViewAt(index)
+          ->GetIconForTest();
   return gfx::Image(icon);
 }
 
 void BrowserActionTestUtilViews::Press(int index) {
-  GetContainer(browser_, test_helper_.get())
-      ->GetToolbarActionViewAt(index)
+  browser_actions_container_->GetToolbarActionViewAt(index)
       ->view_controller()
       ->ExecuteAction(true);
 }
 
 std::string BrowserActionTestUtilViews::GetExtensionId(int index) {
-  return GetContainer(browser_, test_helper_.get())
-      ->GetToolbarActionViewAt(index)
+  return browser_actions_container_->GetToolbarActionViewAt(index)
       ->view_controller()
       ->GetId();
 }
 
 std::string BrowserActionTestUtilViews::GetTooltip(int index) {
-  base::string16 tooltip = GetContainer(browser_, test_helper_.get())
-                               ->GetToolbarActionViewAt(index)
-                               ->GetTooltipText(gfx::Point());
+  base::string16 tooltip =
+      browser_actions_container_->GetToolbarActionViewAt(index)->GetTooltipText(
+          gfx::Point());
   return base::UTF16ToUTF8(tooltip);
 }
 
@@ -166,26 +139,27 @@ bool BrowserActionTestUtilViews::HidePopup() {
 }
 
 bool BrowserActionTestUtilViews::ActionButtonWantsToRun(size_t index) {
-  return GetContainer(browser_, test_helper_.get())
-      ->GetToolbarActionViewAt(index)
+  return browser_actions_container_->GetToolbarActionViewAt(index)
       ->wants_to_run_for_testing();
 }
 
 void BrowserActionTestUtilViews::SetWidth(int width) {
-  BrowserActionsContainer* container =
-      GetContainer(browser_, test_helper_.get());
-  container->SetSize(gfx::Size(width, container->height()));
+  browser_actions_container_->SetSize(
+      gfx::Size(width, browser_actions_container_->height()));
 }
 
 ToolbarActionsBar* BrowserActionTestUtilViews::GetToolbarActionsBar() {
-  return GetContainer(browser_, test_helper_.get())->toolbar_actions_bar();
+  return browser_actions_container_->toolbar_actions_bar();
 }
 
 std::unique_ptr<BrowserActionTestUtil>
-BrowserActionTestUtilViews::CreateOverflowBar() {
+BrowserActionTestUtilViews::CreateOverflowBar(Browser* browser) {
   CHECK(!GetToolbarActionsBar()->in_overflow_mode())
       << "Only a main bar can create an overflow bar!";
-  return base::WrapUnique(new BrowserActionTestUtilViews(browser_, this));
+
+  return base::WrapUnique(new BrowserActionTestUtilViews(
+      std::make_unique<TestToolbarActionsBarHelper>(
+          browser, browser_actions_container_)));
 }
 
 gfx::Size BrowserActionTestUtilViews::GetMinPopupSize() {
@@ -197,29 +171,43 @@ gfx::Size BrowserActionTestUtilViews::GetMaxPopupSize() {
 }
 
 bool BrowserActionTestUtilViews::CanBeResized() {
-  BrowserActionsContainer* container =
-      BrowserView::GetBrowserViewForBrowser(browser_)
-          ->toolbar()
-          ->browser_actions();
-
   // The container can only be resized if we can start a drag for the view.
-  DCHECK_LE(1u, container->num_toolbar_actions());
-  ToolbarActionView* action_view = container->GetToolbarActionViewAt(0);
+  DCHECK_LE(1u, browser_actions_container_->num_toolbar_actions());
+  ToolbarActionView* action_view =
+      browser_actions_container_->GetToolbarActionViewAt(0);
   gfx::Point point(action_view->x(), action_view->y());
-  return container->CanStartDragForView(action_view, point, point);
+  return browser_actions_container_->CanStartDragForView(action_view, point,
+                                                         point);
 }
 
 BrowserActionTestUtilViews::BrowserActionTestUtilViews(
-    Browser* browser,
-    BrowserActionTestUtilViews* main_bar)
-    : browser_(browser),
-      test_helper_(new TestToolbarActionsBarHelperViews(
-          browser_,
-          GetContainer(browser_, main_bar->test_helper_.get()))) {}
+    BrowserActionsContainer* browser_actions_container)
+    : browser_actions_container_(browser_actions_container) {}
+
+BrowserActionTestUtilViews::BrowserActionTestUtilViews(
+    std::unique_ptr<TestToolbarActionsBarHelper> test_helper)
+    : test_helper_(std::move(test_helper)),
+      browser_actions_container_(test_helper_->browser_actions_container()) {}
 
 // static
 std::unique_ptr<BrowserActionTestUtil> BrowserActionTestUtil::Create(
     Browser* browser,
     bool is_real_window) {
-  return std::make_unique<BrowserActionTestUtilViews>(browser, is_real_window);
+  std::unique_ptr<BrowserActionTestUtil> browser_action_test_util;
+
+  if (is_real_window) {
+    browser_action_test_util = base::WrapUnique(new BrowserActionTestUtilViews(
+        BrowserView::GetBrowserViewForBrowser(browser)
+            ->toolbar()
+            ->browser_actions()));
+  } else {
+    // This is the main bar.
+    BrowserActionsContainer* main_bar = nullptr;
+    browser_action_test_util = base::WrapUnique(new BrowserActionTestUtilViews(
+        std::make_unique<
+            BrowserActionTestUtilViews::TestToolbarActionsBarHelper>(
+            browser, main_bar)));
+  }
+
+  return browser_action_test_util;
 }

chrome/browser/ui/views/toolbar/browser_action_test_util_views.h

@@ -5,12 +5,14 @@
 #ifndef CHROME_BROWSER_UI_VIEWS_TOOLBAR_BROWSER_ACTION_TEST_UTIL_VIEWS_H_
 #define CHROME_BROWSER_UI_VIEWS_TOOLBAR_BROWSER_ACTION_TEST_UTIL_VIEWS_H_
 
+#include <memory>
+
 #include "chrome/browser/ui/extensions/browser_action_test_util.h"
 
+class BrowserActionsContainer;
+
 class BrowserActionTestUtilViews : public BrowserActionTestUtil {
  public:
-  explicit BrowserActionTestUtilViews(Browser* browser);
-  BrowserActionTestUtilViews(Browser* browser, bool is_real_window);
   ~BrowserActionTestUtilViews() override;
 
   // BrowserActionTestUtil:
@@ -29,7 +31,8 @@ class BrowserActionTestUtilViews : public BrowserActionTestUtil {
   bool ActionButtonWantsToRun(size_t index) override;
   void SetWidth(int width) override;
   ToolbarActionsBar* GetToolbarActionsBar() override;
-  std::unique_ptr<BrowserActionTestUtil> CreateOverflowBar() override;
+  std::unique_ptr<BrowserActionTestUtil> CreateOverflowBar(
+      Browser* browser) override;
   gfx::Size GetMinPopupSize() override;
   gfx::Size GetMaxPopupSize() override;
   bool CanBeResized() override;
@@ -37,14 +40,23 @@ class BrowserActionTestUtilViews : public BrowserActionTestUtil {
  private:
   friend class BrowserActionTestUtil;
 
-  Browser* const browser_;  // weak
+  class TestToolbarActionsBarHelper;
+
+  // Constructs a version of BrowserActionTestUtilViews that does not own the
+  // BrowserActionsContainer it tests.
+  explicit BrowserActionTestUtilViews(
+      BrowserActionsContainer* browser_actions_container);
+  // Constructs a version of BrowserActionTestUtilViews given a |test_helper|
+  // responsible for owning the BrowserActionsContainer.
+  explicit BrowserActionTestUtilViews(
+      std::unique_ptr<TestToolbarActionsBarHelper> test_helper);
 
-  // Our test helper, which constructs and owns the views if we don't have a
-  // real browser window, or if this is an overflow version.
   std::unique_ptr<TestToolbarActionsBarHelper> test_helper_;
 
-  BrowserActionTestUtilViews(Browser* browser,
-                             BrowserActionTestUtilViews* main_bar);
+  // The associated BrowserActionsContainer. Not owned.
+  BrowserActionsContainer* const browser_actions_container_;
+
+  DISALLOW_COPY_AND_ASSIGN(BrowserActionTestUtilViews);
 };
 
 #endif  // CHROME_BROWSER_UI_VIEWS_TOOLBAR_BROWSER_ACTION_TEST_UTIL_VIEWS_H_

chrome/browser/ui/views/toolbar/browser_actions_container.h

@@ -116,6 +116,8 @@ class BrowserActionsContainer : public views::View,
  public:
   class Delegate {
    public:
+    virtual ~Delegate() {}
+
     // Returns the view of the toolbar actions overflow menu to use as a
     // reference point for a popup when this view isn't visible.
     virtual views::LabelButton* GetOverflowReferenceView() = 0;

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

@@ -238,7 +238,7 @@ namespace {
 class ForwardingDelegate : public BrowserActionsContainer::Delegate {
  public:
   explicit ForwardingDelegate(BrowserActionsContainer::Delegate* forward_to);
-  virtual ~ForwardingDelegate() = default;
+  ~ForwardingDelegate() override = default;
 
   BrowserActionsContainer::Delegate* forward_to() { return forward_to_; }
   void set_max_browser_actions_width(