Refactor WebAppFrameToolbarView ownership 6/x: Tests.

This modifies a bunch of tests to access the WebAppFrameToolbarView
via BrowserView, to make sure they pass both with and without the
WebAppFrameToolbarInBrowserView feature enabled.

Differences in behavior exposed by these tests ("now" being the
future state where the feature is enabled):
- popups on Chrome OS now have their toolbar hidden rather than
  not created at all.
- also on Chrome OS, before if a permission bubble was open when
  entering full screen the bubble would move from being attached to
  the toolbar to instead just being drawn in the top-left corner.
  Now instead the bubble will remain attached to the toolbar (and the
  toolbar will be auto-revealed). This change in behavior is because
  previously the code that checked if any bubbles are attached to
  things in the top-container when entering fullscreen ran before the
  web app toolbar was reparented to the top-container. Now the web app
  toolbar is always in the top-container so this existing logic can
  now properly deal with these bubbles/revealing the top-container.
- before visibility of the toolbar when entering/exiting "overview"
  mode on chrome OS would synchronously update. Now the visibility
  only gets updated once layout completes. The test for this has been
  modified to trigger this scheduled layout before verifying the
  result.
- WebAppNonClientFrameViewAshTest.ButtonVisibilityInOverviewMode
  apparently only passed previously because it verified button
  visibility before doing any pending layouts. With the new
  implementation layout is needed to update button visibility,
  which revealed this bug in the old implementation. Rather than
  trying to fix the old implementation, for now I made the test only
  trigger the layout with the new implementation, since the old one
  should go away soon anyway.

Bug: 1407240
Change-Id: I2e77ad409d7c04550d96ddbfde821ec470929492
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4194052
Commit-Queue: Marijn Kruisselbrink <mek@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1100830}

chrome/browser/ui/views/frame/browser_non_client_frame_view.h

@@ -139,10 +139,6 @@ class BrowserNonClientFrameView : public views::NonClientFrameView,
   void VisibilityChanged(views::View* starting_from, bool is_visible) override;
   int NonClientHitTest(const gfx::Point& point) override;
 
-  WebAppFrameToolbarView* web_app_frame_toolbar_for_testing() {
-    return web_app_frame_toolbar_;
-  }
-
   // TODO(https://crbug.com/1407240): Remove these methods (and all other
   // WebAppFrameToolbarView access/usage) from this class once work to refactor
   // ownership has been completed.

chrome/browser/ui/views/frame/browser_non_client_frame_view_browsertest.cc

@@ -19,6 +19,7 @@
 #include "chrome/browser/web_applications/test/web_app_install_test_utils.h"
 #include "chrome/browser/web_applications/web_app_install_info.h"
 #include "chrome/browser/web_applications/web_app_install_utils.h"
+#include "chrome/common/chrome_features.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "components/autofill/content/browser/content_autofill_driver.h"
@@ -221,7 +222,13 @@ IN_PROC_BROWSER_TEST_F(BrowserNonClientFrameViewBrowserTest,
 IN_PROC_BROWSER_TEST_F(BrowserNonClientFrameViewBrowserTest,
                        FullscreenForTabTitlebarHeight) {
   InstallAndLaunchBookmarkApp();
-  EXPECT_GT(GetAppFrameView()->GetTopInset(false), 0);
+  if (!base::FeatureList::IsEnabled(
+          features::kWebAppFrameToolbarInBrowserView)) {
+    // When WebAppFrameToolbarView lives in the BrowserView it is perfectly
+    // valid for the top insets to be 0 at all times. So only do this check when
+    // the feature is disabled.
+    EXPECT_GT(GetAppFrameView()->GetTopInset(false), 0);
+  }
 
   static_cast<content::WebContentsDelegate*>(app_browser_)
       ->EnterFullscreenModeForTab(web_contents_->GetPrimaryMainFrame(), {});

chrome/browser/ui/views/frame/browser_non_client_frame_view_chromeos_browsertest.cc

@@ -33,6 +33,7 @@
 #include "ui/aura/client/aura_constants.h"
 #include "ui/aura/window.h"
 #include "ui/gfx/geometry/size.h"
+#include "ui/views/test/views_test_utils.h"
 #include "ui/views/widget/widget.h"
 
 #if BUILDFLAG(IS_CHROMEOS_ASH)
@@ -93,6 +94,7 @@
 #include "chrome/browser/web_applications/test/web_app_install_test_utils.h"
 #include "chrome/browser/web_applications/web_app_install_info.h"
 #include "chrome/browser/web_applications/web_app_provider.h"
+#include "chrome/common/chrome_features.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "chromeos/ui/base/chromeos_ui_constants.h"
 #include "chromeos/ui/base/window_pin_type.h"
@@ -765,7 +767,7 @@ class WebAppNonClientFrameViewAshTest
     frame_header_ = static_cast<chromeos::DefaultFrameHeader*>(
         BrowserNonClientFrameViewChromeOSTestApi(frame_view).GetFrameHeader());
 
-    web_app_frame_toolbar_ = frame_view->web_app_frame_toolbar_for_testing();
+    web_app_frame_toolbar_ = browser_view_->web_app_frame_toolbar_for_testing();
     DCHECK(web_app_frame_toolbar_);
     DCHECK(web_app_frame_toolbar_->GetVisible());
 
@@ -865,8 +867,17 @@ IN_PROC_BROWSER_TEST_P(WebAppNonClientFrameViewAshTest,
   EXPECT_TRUE(web_app_frame_toolbar_->GetVisible());
 
   StartOverview();
+  if (base::FeatureList::IsEnabled(
+          features::kWebAppFrameToolbarInBrowserView)) {
+    // This RunScheduledLayout call should be done unconditionally, but as it
+    // turns out this causes this test to fail when
+    // WebAppFrameToolbarInBrowserView is disabled, because in that case Layout
+    // incorrectly causes the toolbar to be made visible again.
+    views::test::RunScheduledLayout(browser_view_);
+  }
   EXPECT_FALSE(web_app_frame_toolbar_->GetVisible());
   EndOverview();
+  views::test::RunScheduledLayout(browser_view_);
   EXPECT_TRUE(web_app_frame_toolbar_->GetVisible());
 }
 
@@ -1117,9 +1128,8 @@ IN_PROC_BROWSER_TEST_P(WebAppNonClientFrameViewAshTest, PopupHasNoToolbar) {
   Browser* popup_browser = BrowserList::GetInstance()->GetLastActive();
   BrowserView* browser_view =
       BrowserView::GetBrowserViewForBrowser(popup_browser);
-  BrowserNonClientFrameViewChromeOS* frame_view =
-      GetFrameViewChromeOS(browser_view);
-  EXPECT_FALSE(frame_view->web_app_frame_toolbar_for_testing());
+  EXPECT_FALSE(browser_view->web_app_frame_toolbar_for_testing() &&
+               browser_view->web_app_frame_toolbar_for_testing()->GetVisible());
 }
 #endif  // BUILDFLAG(IS_CHROMEOS_ASH)
 

chrome/browser/ui/views/frame/browser_view.h

@@ -783,6 +783,10 @@ class BrowserView : public BrowserWindow,
   void SetLoadingAnimationStateChangeClosureForTesting(
       base::OnceClosure closure);
 
+  WebAppFrameToolbarView* web_app_frame_toolbar_for_testing() {
+    return web_app_frame_toolbar();
+  }
+
  private:
   // Do not friend BrowserViewLayout. Use the BrowserViewLayoutDelegate
   // interface to keep these two classes decoupled and testable.

chrome/browser/ui/views/frame/glass_browser_frame_view.h

@@ -71,7 +71,6 @@ class GlassBrowserFrameView : public BrowserNonClientFrameView,
 
   SkColor GetTitlebarColor() const;
 
-  const views::Label* window_title_for_testing() const { return window_title_; }
   const GlassBrowserCaptionButtonContainer*
   caption_button_container_for_testing() const {
     return caption_button_container_;

chrome/browser/ui/views/frame/glass_browser_frame_view_browsertest_win.cc

@@ -170,8 +170,7 @@ class WebAppGlassBrowserFrameViewTest : public InProcessBrowserTest {
       return false;
     glass_frame_view_ = static_cast<GlassBrowserFrameView*>(frame_view);
 
-    web_app_frame_toolbar_ =
-        glass_frame_view_->web_app_frame_toolbar_for_testing();
+    web_app_frame_toolbar_ = browser_view_->web_app_frame_toolbar_for_testing();
     DCHECK(web_app_frame_toolbar_);
     DCHECK(web_app_frame_toolbar_->GetVisible());
     return true;
@@ -209,8 +208,10 @@ IN_PROC_BROWSER_TEST_F(WebAppGlassBrowserFrameViewTest, MaximizedLayout) {
   glass_frame_view_->frame()->Maximize();
   RunScheduledLayouts();
 
-  DCHECK_GT(glass_frame_view_->window_title_for_testing()->x(), 0);
-  DCHECK_GE(glass_frame_view_->web_app_frame_toolbar_for_testing()->y(), 0);
+  views::View* const window_title =
+      glass_frame_view_->GetViewByID(VIEW_ID_WINDOW_TITLE);
+  DCHECK_GT(window_title->x(), 0);
+  DCHECK_GE(web_app_frame_toolbar_->y(), 0);
 }
 
 IN_PROC_BROWSER_TEST_F(WebAppGlassBrowserFrameViewTest, RTLTopRightHitTest) {
@@ -251,13 +252,13 @@ IN_PROC_BROWSER_TEST_F(WebAppGlassBrowserFrameViewTest, ContainerHeight) {
       ->LayoutRootViewIfNecessary();
 
   EXPECT_EQ(
-      glass_frame_view_->web_app_frame_toolbar_for_testing()->height(),
+      web_app_frame_toolbar_->height(),
       glass_frame_view_->caption_button_container_for_testing()->height());
 
   glass_frame_view_->frame()->Maximize();
 
   EXPECT_EQ(
-      glass_frame_view_->web_app_frame_toolbar_for_testing()->height(),
+      web_app_frame_toolbar_->height(),
       glass_frame_view_->caption_button_container_for_testing()->height());
 }
 
@@ -321,7 +322,7 @@ class WebAppGlassBrowserFrameViewWindowControlsOverlayTest
 
     glass_frame_view_ = static_cast<GlassBrowserFrameView*>(frame_view);
     auto* web_app_frame_toolbar =
-        glass_frame_view_->web_app_frame_toolbar_for_testing();
+        browser_view_->web_app_frame_toolbar_for_testing();
 
     DCHECK(web_app_frame_toolbar);
     DCHECK(web_app_frame_toolbar->GetVisible());
@@ -354,13 +355,13 @@ IN_PROC_BROWSER_TEST_F(WebAppGlassBrowserFrameViewWindowControlsOverlayTest,
   ToggleWindowControlsOverlayEnabledAndWait();
 
   EXPECT_EQ(
-      glass_frame_view_->web_app_frame_toolbar_for_testing()->height(),
+      browser_view_->web_app_frame_toolbar_for_testing()->height(),
       glass_frame_view_->caption_button_container_for_testing()->height());
 
   glass_frame_view_->frame()->Maximize();
 
   EXPECT_EQ(
-      glass_frame_view_->web_app_frame_toolbar_for_testing()->height(),
+      browser_view_->web_app_frame_toolbar_for_testing()->height(),
       glass_frame_view_->caption_button_container_for_testing()->height());
 }
 
@@ -457,7 +458,7 @@ IN_PROC_BROWSER_TEST_F(WebAppGlassBrowserFrameViewWindowControlsOverlayTest,
   glass_frame_view_->UpdateWindowTitle();
 
   WebAppFrameToolbarView* web_app_frame_toolbar =
-      glass_frame_view_->web_app_frame_toolbar_for_testing();
+      browser_view_->web_app_frame_toolbar_for_testing();
 
   // Verify that the center container doesn't consume space by expecting the
   // right container to consume the full width of the WebAppFrameToolbarView.

chrome/browser/ui/views/frame/immersive_mode_controller_chromeos_browsertest.cc

@@ -23,6 +23,7 @@
 #include "chrome/browser/ui/views/web_apps/frame_toolbar/web_app_toolbar_button_container.h"
 #include "chrome/browser/ui/web_applications/web_app_controller_browsertest.h"
 #include "chrome/browser/web_applications/web_app_install_info.h"
+#include "chrome/common/chrome_features.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "chrome/test/permissions/permission_request_manager_test_api.h"
 #include "chromeos/ui/frame/caption_buttons/frame_caption_button_container_view.h"
@@ -110,10 +111,9 @@ class ImmersiveModeControllerChromeosWebAppBrowserTest
     }
   }
 
-  void VerifyButtonsInImmersiveMode(
-      BrowserNonClientFrameViewChromeOS* frame_view) {
+  void VerifyButtonsInImmersiveMode(BrowserView* browser_view) {
     WebAppFrameToolbarView* container =
-        frame_view->web_app_frame_toolbar_for_testing();
+        browser_view->web_app_frame_toolbar_for_testing();
     views::test::InkDropHostTestApi ink_drop_api(
         views::InkDrop::Get(container->GetAppMenuButton()));
     EXPECT_TRUE(container->GetContentSettingContainerForTesting()->layer());
@@ -283,7 +283,7 @@ IN_PROC_BROWSER_TEST_F(ImmersiveModeControllerChromeosWebAppBrowserTest,
 
   EXPECT_TRUE(frame_test_api.size_button()->GetVisible());
 
-  VerifyButtonsInImmersiveMode(frame_view);
+  VerifyButtonsInImmersiveMode(browser_view);
 
   // Verify the size button is visible in clamshell mode, and that it does not
   // cover the other two buttons.
@@ -296,7 +296,7 @@ IN_PROC_BROWSER_TEST_F(ImmersiveModeControllerChromeosWebAppBrowserTest,
   EXPECT_FALSE(frame_test_api.size_button()->GetBoundsInScreen().Intersects(
       frame_test_api.minimize_button()->GetBoundsInScreen()));
 
-  VerifyButtonsInImmersiveMode(frame_view);
+  VerifyButtonsInImmersiveMode(browser_view);
 }
 
 // Verify that the frame layout for new windows is as expected when using
@@ -317,15 +317,12 @@ IN_PROC_BROWSER_TEST_F(ImmersiveModeControllerChromeosWebAppBrowserTest,
     task_runner->FastForwardBy(titlebar_animation_delay());
   }
 
-  BrowserNonClientFrameViewChromeOS* frame_view =
-      static_cast<BrowserNonClientFrameViewChromeOS*>(
-          browser_view->GetWidget()->non_client_view()->frame_view());
-  VerifyButtonsInImmersiveMode(frame_view);
+  VerifyButtonsInImmersiveMode(browser_view);
 
   // Verify the size button is visible in clamshell mode, and that it does not
   // cover the other two buttons.
   ash::ShellTestApi().SetTabletModeEnabledForTest(false);
-  VerifyButtonsInImmersiveMode(frame_view);
+  VerifyButtonsInImmersiveMode(browser_view);
 }
 
 // Tests that the permissions bubble dialog is anchored to the correct location.
@@ -350,6 +347,8 @@ IN_PROC_BROWSER_TEST_F(ImmersiveModeControllerChromeosWebAppBrowserTest,
 
   // The permission prompt is shown asynchronously. Without immersive mode
   // enabled the anchor should exist.
+  // TODO(https://crbug.com/1317865): Change from RunUntilIdle to a more
+  // explicit notification.
   base::RunLoop().RunUntilIdle();
 
   views::Widget* prompt_widget = test_api->GetPromptWindow();
@@ -358,18 +357,56 @@ IN_PROC_BROWSER_TEST_F(ImmersiveModeControllerChromeosWebAppBrowserTest,
   ASSERT_TRUE(bubble_dialog);
   EXPECT_TRUE(bubble_dialog->GetAnchorView());
 
-  // Turn on immersive, but do not reveal. The app menu button is hidden from
-  // sight so the anchor should be null. The bubble will get placed in the top
-  // left corner of the app.
+  // Turn on immersive, but do not reveal.
   auto* immersive_mode_controller =
       BrowserView::GetBrowserViewForBrowser(browser())
           ->immersive_mode_controller();
   immersive_mode_controller->SetEnabled(true);
+
+  if (base::FeatureList::IsEnabled(
+          features::kWebAppFrameToolbarInBrowserView)) {
+    // Since a bubble was visible and anchored to the header, the header should
+    // have been automatically revealed.
+    EXPECT_TRUE(immersive_mode_controller->IsRevealed());
+    EXPECT_TRUE(bubble_dialog->GetAnchorView());
+
+    // Closing the bubble should cause the header to no longer be revealed.
+    bubble_dialog->AcceptDialog();
+    EXPECT_FALSE(immersive_mode_controller->IsRevealed());
+
+    // Make sure the old permission prompt fully goes away before opening a new
+    // prompt.
+    // TODO(https://crbug.com/1317865): Change from RunUntilIdle to a more
+    // explicit notification.
+    base::RunLoop().RunUntilIdle();
+    ASSERT_FALSE(test_api->GetPromptWindow());
+
+    // Opening a new permission bubble should not cause the header to reveal.
+    test_api->AddSimpleRequest(browser()
+                                   ->tab_strip_model()
+                                   ->GetActiveWebContents()
+                                   ->GetPrimaryMainFrame(),
+                               permissions::RequestType::kMicStream);
+
+    // The permission prompt is shown asynchronously.
+    // TODO(https://crbug.com/1317865): Change from RunUntilIdle to a more
+    // explicit notification.
+    base::RunLoop().RunUntilIdle();
+    prompt_widget = test_api->GetPromptWindow();
+    ASSERT_TRUE(prompt_widget);
+    ASSERT_TRUE(prompt_widget->widget_delegate());
+    bubble_dialog = prompt_widget->widget_delegate()->AsBubbleDialogDelegate();
+    ASSERT_TRUE(bubble_dialog);
+  }
+
+  // The app menu button is hidden from
+  // sight so the anchor should be null. The bubble will get placed in the top
+  // left corner of the app.
   EXPECT_FALSE(immersive_mode_controller->IsRevealed());
   EXPECT_FALSE(bubble_dialog->GetAnchorView());
 
-  // Reveal the header. The anchor should exist since the app menu button is now
-  // visible.
+  // Reveal the header. The anchor should exist since the app menu button is
+  // now visible.
   {
     std::unique_ptr<ImmersiveRevealedLock> focus_reveal_lock =
         immersive_mode_controller->GetRevealedLock(

chrome/browser/ui/views/frame/opaque_browser_frame_view_browsertest.cc

@@ -110,8 +110,7 @@ class WebAppOpaqueBrowserFrameViewTest : public InProcessBrowserTest {
 
     opaque_browser_frame_view_ =
         static_cast<OpaqueBrowserFrameView*>(frame_view);
-    web_app_frame_toolbar_ =
-        opaque_browser_frame_view_->web_app_frame_toolbar_for_testing();
+    web_app_frame_toolbar_ = browser_view_->web_app_frame_toolbar_for_testing();
     DCHECK(web_app_frame_toolbar_);
     DCHECK(web_app_frame_toolbar_->GetVisible());
 
@@ -388,7 +387,7 @@ class WebAppOpaqueBrowserFrameViewWindowControlsOverlayTest
     opaque_browser_frame_view_ =
         static_cast<OpaqueBrowserFrameView*>(frame_view);
     auto* web_app_frame_toolbar =
-        opaque_browser_frame_view_->web_app_frame_toolbar_for_testing();
+        browser_view_->web_app_frame_toolbar_for_testing();
     DCHECK(web_app_frame_toolbar);
     DCHECK(web_app_frame_toolbar->GetVisible());
 

chrome/browser/ui/views/frame/system_web_app_non_client_frame_view_browsertest.cc

@@ -24,8 +24,6 @@ IN_PROC_BROWSER_TEST_P(SystemWebAppNonClientFrameViewBrowserTest,
   Browser* app_browser;
   LaunchApp(ash::SystemWebAppType::SETTINGS, &app_browser);
   EXPECT_EQ(nullptr, BrowserView::GetBrowserViewForBrowser(app_browser)
-                         ->frame()
-                         ->GetFrameView()
                          ->web_app_frame_toolbar_for_testing()
                          ->GetAppMenuButton());
 }
@@ -38,8 +36,6 @@ IN_PROC_BROWSER_TEST_P(SystemWebAppNonClientFrameViewBrowserTest,
   LaunchApp(ash::SystemWebAppType::SETTINGS, &app_browser);
   WebAppFrameToolbarView* toolbar =
       BrowserView::GetBrowserViewForBrowser(app_browser)
-          ->frame()
-          ->GetFrameView()
           ->web_app_frame_toolbar_for_testing();
   EXPECT_FALSE(
       toolbar->GetPageActionIconView(PageActionIconType::kFileSystemAccess));