[UI] Use composition instead of inheritance for WebAppFrameToolbarTestMixin

Composition is preferred over inheritance in general, and this also
avoids multiple implementation inheritance (which is strongly
discouraged by style) in two test suites.

Also rename the Mixin to be "Helper".

https://google.github.io/styleguide/cppguide.html#Inheritance

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

chrome/browser/ui/views/web_apps/web_app_frame_toolbar_browsertest.cc

@@ -15,7 +15,7 @@
 #include "chrome/browser/ui/views/frame/browser_view.h"
 #include "chrome/browser/ui/views/frame/toolbar_button_provider.h"
 #include "chrome/browser/ui/views/page_action/page_action_icon_controller.h"
-#include "chrome/browser/ui/views/web_apps/web_app_frame_toolbar_test_mixin.h"
+#include "chrome/browser/ui/views/web_apps/web_app_frame_toolbar_test_helper.h"
 #include "chrome/browser/ui/views/web_apps/web_app_frame_toolbar_view.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "content/public/browser/web_contents.h"
@@ -51,8 +51,7 @@ T* GetLastVisible(const std::vector<T*>& views) {
 
 }  // namespace
 
-class WebAppFrameToolbarBrowserTest : public InProcessBrowserTest,
-                                      public WebAppFrameToolbarTestMixin {
+class WebAppFrameToolbarBrowserTest : public InProcessBrowserTest {
  public:
   WebAppFrameToolbarBrowserTest()
       : https_server_(net::EmbeddedTestServer::TYPE_HTTPS) {}
@@ -66,43 +65,51 @@ class WebAppFrameToolbarBrowserTest : public InProcessBrowserTest,
     InProcessBrowserTest::SetUp();
   }
 
+  WebAppFrameToolbarTestHelper* helper() {
+    return &web_app_frame_toolbar_helper_;
+  }
+
  private:
   net::EmbeddedTestServer https_server_;
+  WebAppFrameToolbarTestHelper web_app_frame_toolbar_helper_;
 };
 
 IN_PROC_BROWSER_TEST_F(WebAppFrameToolbarBrowserTest, SpaceConstrained) {
   const GURL app_url("https://test.org");
-  InstallAndLaunchWebApp(browser(), app_url);
+  helper()->InstallAndLaunchWebApp(browser(), app_url);
 
   views::View* const toolbar_left_container =
-      web_app_frame_toolbar()->GetLeftContainerForTesting();
-  EXPECT_EQ(toolbar_left_container->parent(), web_app_frame_toolbar());
+      helper()->web_app_frame_toolbar()->GetLeftContainerForTesting();
+  EXPECT_EQ(toolbar_left_container->parent(),
+            helper()->web_app_frame_toolbar());
 
   views::View* const window_title =
-      frame_view()->GetViewByID(VIEW_ID_WINDOW_TITLE);
+      helper()->frame_view()->GetViewByID(VIEW_ID_WINDOW_TITLE);
 #if defined(OS_CHROMEOS)
   EXPECT_FALSE(window_title);
 #else
-  EXPECT_EQ(window_title->parent(), frame_view());
+  EXPECT_EQ(window_title->parent(), helper()->frame_view());
 #endif
 
   views::View* const toolbar_right_container =
-      web_app_frame_toolbar()->GetRightContainerForTesting();
-  EXPECT_EQ(toolbar_right_container->parent(), web_app_frame_toolbar());
+      helper()->web_app_frame_toolbar()->GetRightContainerForTesting();
+  EXPECT_EQ(toolbar_right_container->parent(),
+            helper()->web_app_frame_toolbar());
 
   std::vector<const PageActionIconView*> page_actions =
-      web_app_frame_toolbar()
+      helper()
+          ->web_app_frame_toolbar()
           ->GetPageActionIconControllerForTesting()
           ->GetPageActionIconViewsForTesting();
   for (const PageActionIconView* action : page_actions)
     EXPECT_EQ(action->parent(), toolbar_right_container);
 
   views::View* const menu_button =
-      browser_view()->toolbar_button_provider()->GetAppMenuButton();
+      helper()->browser_view()->toolbar_button_provider()->GetAppMenuButton();
   EXPECT_EQ(menu_button->parent(), toolbar_right_container);
 
   // Ensure we initially have abundant space.
-  frame_view()->SetSize(gfx::Size(1000, 1000));
+  helper()->frame_view()->SetSize(gfx::Size(1000, 1000));
 
   EXPECT_TRUE(toolbar_left_container->GetVisible());
   const int original_left_container_width = toolbar_left_container->width();
@@ -119,11 +126,11 @@ IN_PROC_BROWSER_TEST_F(WebAppFrameToolbarBrowserTest, SpaceConstrained) {
   EXPECT_GT(original_menu_button_width, 0);
 
   // Cause the zoom page action icon to be visible.
-  chrome::Zoom(app_browser(), content::PAGE_ZOOM_IN);
+  chrome::Zoom(helper()->app_browser(), content::PAGE_ZOOM_IN);
 
   // The layout should be invalidated, but since we don't have the benefit of
   // the compositor to immediately kick a layout off, we have to do it manually.
-  frame_view()->Layout();
+  helper()->frame_view()->Layout();
 
   // The page action icons should now take up width, leaving less space on
   // Windows and Linux for the window title. (On Mac, the window title remains
@@ -142,15 +149,16 @@ IN_PROC_BROWSER_TEST_F(WebAppFrameToolbarBrowserTest, SpaceConstrained) {
 
   // Resize the WebAppFrameToolbarView just enough to clip out the page action
   // icons (and toolbar contents left of them).
-  const int original_toolbar_width = web_app_frame_toolbar()->width();
+  const int original_toolbar_width = helper()->web_app_frame_toolbar()->width();
   const int new_toolbar_width = toolbar_right_container->width() -
                                 GetLastVisible(page_actions)->bounds().right();
-  const int new_frame_width =
-      frame_view()->width() - original_toolbar_width + new_toolbar_width;
+  const int new_frame_width = helper()->frame_view()->width() -
+                              original_toolbar_width + new_toolbar_width;
 
-  web_app_frame_toolbar()->SetSize(
-      {new_toolbar_width, web_app_frame_toolbar()->height()});
-  frame_view()->SetSize({new_frame_width, frame_view()->height()});
+  helper()->web_app_frame_toolbar()->SetSize(
+      {new_toolbar_width, helper()->web_app_frame_toolbar()->height()});
+  helper()->frame_view()->SetSize(
+      {new_frame_width, helper()->frame_view()->height()});
 
   // The left container (containing Back and Reload) should be hidden.
   EXPECT_FALSE(toolbar_left_container->GetVisible());
@@ -169,17 +177,17 @@ IN_PROC_BROWSER_TEST_F(WebAppFrameToolbarBrowserTest, SpaceConstrained) {
 IN_PROC_BROWSER_TEST_F(WebAppFrameToolbarBrowserTest, ThemeChange) {
   ASSERT_TRUE(https_server()->Start());
   const GURL app_url = https_server()->GetURL("/banners/theme-color.html");
-  InstallAndLaunchWebApp(browser(), app_url);
+  helper()->InstallAndLaunchWebApp(browser(), app_url);
 
   content::WebContents* web_contents =
-      app_browser()->tab_strip_model()->GetActiveWebContents();
+      helper()->app_browser()->tab_strip_model()->GetActiveWebContents();
   content::AwaitDocumentOnLoadCompleted(web_contents);
 
 #if !defined(OS_LINUX) || defined(OS_CHROMEOS)
   // Avoid dependence on Linux GTK+ Themes appearance setting.
 
   ToolbarButtonProvider* const toolbar_button_provider =
-      browser_view()->toolbar_button_provider();
+      helper()->browser_view()->toolbar_button_provider();
   AppMenuButton* const app_menu_button =
       toolbar_button_provider->GetAppMenuButton();
 
@@ -210,27 +218,27 @@ IN_PROC_BROWSER_TEST_F(WebAppFrameToolbarBrowserTest, ThemeChange) {
 // Test that a tooltip is shown when hovering over a truncated title.
 IN_PROC_BROWSER_TEST_F(WebAppFrameToolbarBrowserTest, TitleHover) {
   const GURL app_url("https://test.org");
-  InstallAndLaunchWebApp(browser(), app_url);
+  helper()->InstallAndLaunchWebApp(browser(), app_url);
 
   views::View* const toolbar_left_container =
-      web_app_frame_toolbar()->GetLeftContainerForTesting();
+      helper()->web_app_frame_toolbar()->GetLeftContainerForTesting();
   views::View* const toolbar_right_container =
-      web_app_frame_toolbar()->GetRightContainerForTesting();
+      helper()->web_app_frame_toolbar()->GetRightContainerForTesting();
 
   auto* const window_title = static_cast<views::Label*>(
-      frame_view()->GetViewByID(VIEW_ID_WINDOW_TITLE));
+      helper()->frame_view()->GetViewByID(VIEW_ID_WINDOW_TITLE));
 #if defined(OS_CHROMEOS)
   // Chrome OS PWA windows do not display app titles.
   EXPECT_EQ(nullptr, window_title);
   return;
 #endif
-  EXPECT_EQ(window_title->parent(), frame_view());
+  EXPECT_EQ(window_title->parent(), helper()->frame_view());
 
   window_title->SetText(base::string16(30, 't'));
 
   // Ensure we initially have abundant space.
-  frame_view()->SetSize(gfx::Size(1000, 1000));
-  frame_view()->Layout();
+  helper()->frame_view()->SetSize(gfx::Size(1000, 1000));
+  helper()->frame_view()->Layout();
   EXPECT_GT(window_title->width(), 0);
   const int original_title_gap = toolbar_right_container->x() -
                                  toolbar_left_container->x() -
@@ -240,19 +248,20 @@ IN_PROC_BROWSER_TEST_F(WebAppFrameToolbarBrowserTest, TitleHover) {
   const int narrow_title_gap =
       window_title->CalculatePreferredSize().width() * 3 / 4;
   int narrow_frame_width =
-      frame_view()->width() - original_title_gap + narrow_title_gap;
+      helper()->frame_view()->width() - original_title_gap + narrow_title_gap;
 #if defined(OS_MAC)
   // Increase frame width to allow for title padding.
   narrow_frame_width = base::checked_cast<int>(
       std::ceil(narrow_frame_width / (1 - 2 * kTitlePaddingWidthFraction)));
 #endif
-  frame_view()->SetSize(gfx::Size(narrow_frame_width, 1000));
-  frame_view()->Layout();
+  helper()->frame_view()->SetSize(gfx::Size(narrow_frame_width, 1000));
+  helper()->frame_view()->Layout();
 
   EXPECT_GT(window_title->width(), 0);
   EXPECT_EQ(window_title->GetTooltipHandlerForPoint(gfx::Point(0, 0)),
             window_title);
 
-  EXPECT_EQ(frame_view()->GetTooltipHandlerForPoint(window_title->origin()),
-            window_title);
+  EXPECT_EQ(
+      helper()->frame_view()->GetTooltipHandlerForPoint(window_title->origin()),
+      window_title);
 }

chrome/browser/ui/views/web_apps/web_app_frame_toolbar_interactive_uitest.cc

@@ -10,7 +10,7 @@
 #include "chrome/browser/ui/view_ids.h"
 #include "chrome/browser/ui/views/extensions/extensions_toolbar_container.h"
 #include "chrome/browser/ui/views/frame/browser_view.h"
-#include "chrome/browser/ui/views/web_apps/web_app_frame_toolbar_test_mixin.h"
+#include "chrome/browser/ui/views/web_apps/web_app_frame_toolbar_test_helper.h"
 #include "chrome/browser/ui/views/web_apps/web_app_frame_toolbar_view.h"
 #include "chrome/browser/ui/web_applications/test/web_app_browsertest_util.h"
 #include "chrome/test/base/in_process_browser_test.h"
@@ -21,12 +21,18 @@
 #include "ui/views/view.h"
 #include "url/gurl.h"
 
-// TODO(devlin): This class violates style - it shouldn't inherit from multiple
-// non-pure-virtual classes. It seems like the WebAppFrameToolbarTestMixin could
-// be a member.
 class WebAppFrameToolbarInteractiveUITest
-    : public extensions::ExtensionBrowserTest,
-      public WebAppFrameToolbarTestMixin {};
+    : public extensions::ExtensionBrowserTest {
+ public:
+  WebAppFrameToolbarInteractiveUITest() = default;
+
+  WebAppFrameToolbarTestHelper* helper() {
+    return &web_app_frame_toolbar_helper_;
+  }
+
+ private:
+  WebAppFrameToolbarTestHelper web_app_frame_toolbar_helper_;
+};
 
 // Verifies that for minimal-ui web apps, the toolbar keyboard focus cycles
 // among the toolbar buttons: the reload button, the extensions menu button, and
@@ -35,25 +41,27 @@ IN_PROC_BROWSER_TEST_F(WebAppFrameToolbarInteractiveUITest, CycleFocus) {
   ASSERT_TRUE(LoadExtension(test_data_dir_.AppendASCII("simple_with_icon/")));
 
   const GURL app_url("https://test.org");
-  InstallAndLaunchWebApp(browser(), app_url);
+  helper()->InstallAndLaunchWebApp(browser(), app_url);
 
   // Test relies on browser window activation, while platform such as Linux's
   // window activation is asynchronous.
-  ui_test_utils::BrowserActivationWaiter waiter(app_browser());
+  ui_test_utils::BrowserActivationWaiter waiter(helper()->app_browser());
   waiter.WaitForActivation();
 
   // Wait for the extensions menu button to appear.
   ExtensionsToolbarContainer* extensions_container =
-      web_app_frame_toolbar()->GetExtensionsToolbarContainer();
+      helper()->web_app_frame_toolbar()->GetExtensionsToolbarContainer();
   views::test::ReduceAnimationDuration(extensions_container);
   views::test::WaitForAnimatingLayoutManager(extensions_container);
 
   // Send focus to the toolbar as if the user pressed Alt+Shift+T.
-  app_browser()->command_controller()->ExecuteCommand(IDC_FOCUS_TOOLBAR);
+  helper()->app_browser()->command_controller()->ExecuteCommand(
+      IDC_FOCUS_TOOLBAR);
 
   // After focusing the toolbar, the reload button should immediately have focus
   // because the back button is disabled (no navigation yet).
-  views::FocusManager* const focus_manager = browser_view()->GetFocusManager();
+  views::FocusManager* const focus_manager =
+      helper()->browser_view()->GetFocusManager();
   EXPECT_EQ(focus_manager->GetFocusedView()->GetID(), VIEW_ID_RELOAD_BUTTON);
 
   // Press Tab to cycle through controls until we end up back where we started.
@@ -77,7 +85,8 @@ IN_PROC_BROWSER_TEST_F(WebAppFrameToolbarInteractiveUITest, CycleFocus) {
 
   // Test that back button is enabled after navigating to another page
   const GURL another_url("https://anothertest.org");
-  web_app::NavigateToURLAndWait(app_browser(), another_url);
-  app_browser()->command_controller()->ExecuteCommand(IDC_FOCUS_TOOLBAR);
+  web_app::NavigateToURLAndWait(helper()->app_browser(), another_url);
+  helper()->app_browser()->command_controller()->ExecuteCommand(
+      IDC_FOCUS_TOOLBAR);
   EXPECT_EQ(focus_manager->GetFocusedView()->GetID(), VIEW_ID_BACK_BUTTON);
 }

chrome/browser/ui/views/web_apps/web_app_frame_toolbar_test_mixin.cc → chrome/browser/ui/views/web_apps/web_app_frame_toolbar_test_helper.cc

@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "chrome/browser/ui/views/web_apps/web_app_frame_toolbar_test_mixin.h"
+#include "chrome/browser/ui/views/web_apps/web_app_frame_toolbar_test_helper.h"
 
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/ui/browser.h"
@@ -17,12 +17,14 @@
 #include "ui/views/window/non_client_view.h"
 #include "url/gurl.h"
 
-WebAppFrameToolbarTestMixin::WebAppFrameToolbarTestMixin() {
+WebAppFrameToolbarTestHelper::WebAppFrameToolbarTestHelper() {
   WebAppFrameToolbarView::DisableAnimationForTesting();
 }
 
-void WebAppFrameToolbarTestMixin::InstallAndLaunchWebApp(Browser* browser,
-                                                         const GURL& app_url) {
+WebAppFrameToolbarTestHelper::~WebAppFrameToolbarTestHelper() = default;
+
+void WebAppFrameToolbarTestHelper::InstallAndLaunchWebApp(Browser* browser,
+                                                          const GURL& app_url) {
   auto web_app_info = std::make_unique<WebApplicationInfo>();
   web_app_info->app_url = app_url;
   web_app_info->scope = app_url.GetWithoutFilename();

chrome/browser/ui/views/web_apps/web_app_frame_toolbar_test_mixin.h → chrome/browser/ui/views/web_apps/web_app_frame_toolbar_test_helper.h

@@ -2,8 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef CHROME_BROWSER_UI_VIEWS_WEB_APPS_WEB_APP_FRAME_TOOLBAR_TEST_MIXIN_H_
-#define CHROME_BROWSER_UI_VIEWS_WEB_APPS_WEB_APP_FRAME_TOOLBAR_TEST_MIXIN_H_
+#ifndef CHROME_BROWSER_UI_VIEWS_WEB_APPS_WEB_APP_FRAME_TOOLBAR_TEST_HELPER_H_
+#define CHROME_BROWSER_UI_VIEWS_WEB_APPS_WEB_APP_FRAME_TOOLBAR_TEST_HELPER_H_
 
 class Browser;
 class BrowserNonClientFrameView;
@@ -12,9 +12,13 @@ class GURL;
 class WebAppFrameToolbarView;
 
 // Mixin for setting up and launching a web app in a browser test.
-class WebAppFrameToolbarTestMixin {
+class WebAppFrameToolbarTestHelper {
  public:
-  WebAppFrameToolbarTestMixin();
+  WebAppFrameToolbarTestHelper();
+  WebAppFrameToolbarTestHelper(const WebAppFrameToolbarTestHelper&) = delete;
+  WebAppFrameToolbarTestHelper& operator=(const WebAppFrameToolbarTestHelper&) =
+      delete;
+  ~WebAppFrameToolbarTestHelper();
 
   void InstallAndLaunchWebApp(Browser* browser, const GURL& app_url);
 
@@ -32,4 +36,4 @@ class WebAppFrameToolbarTestMixin {
   WebAppFrameToolbarView* web_app_frame_toolbar_ = nullptr;
 };
 
-#endif  // CHROME_BROWSER_UI_VIEWS_WEB_APPS_WEB_APP_FRAME_TOOLBAR_TEST_MIXIN_H_
+#endif  // CHROME_BROWSER_UI_VIEWS_WEB_APPS_WEB_APP_FRAME_TOOLBAR_TEST_HELPER_H_

chrome/test/BUILD.gn

@@ -370,8 +370,8 @@ static_library("test_support") {
       "../browser/ui/views/in_product_help/mock_feature_promo_controller.h",
       "../browser/ui/views/media_router/app_menu_test_api.h",
       "../browser/ui/views/media_router/app_menu_test_api_views.cc",
-      "../browser/ui/views/web_apps/web_app_frame_toolbar_test_mixin.cc",
-      "../browser/ui/views/web_apps/web_app_frame_toolbar_test_mixin.h",
+      "../browser/ui/views/web_apps/web_app_frame_toolbar_test_helper.cc",
+      "../browser/ui/views/web_apps/web_app_frame_toolbar_test_helper.h",
       "base/test_theme_provider.cc",
       "base/test_theme_provider.h",
       "views/accessibility_checker.cc",