HasMinimalUiButtons tests wait for navigation

The ExecuteScriptAndExtractBool calls in HasMinimalUiButtons tests
were sometimes failing.

We now wait for navigation, and close app windows when they
are no longer required.

Bug: 1120504
Change-Id: Ifd869e6ec9e480edd9e85fbd9116b237e38e70e6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2532480
Auto-Submit: Eric Willigers <ericwilligers@chromium.org>
Reviewed-by: Daniel Murphy <dmurph@chromium.org>
Commit-Queue: Eric Willigers <ericwilligers@chromium.org>
Cr-Commit-Position: refs/heads/master@{#826505}

chrome/browser/ui/web_applications/test/web_app_browsertest_util.cc

@@ -8,6 +8,7 @@
 #include "base/run_loop.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/test/bind.h"
+#include "base/threading/thread_task_runner_handle.h"
 #include "chrome/browser/apps/app_service/app_launch_params.h"
 #include "chrome/browser/apps/app_service/app_service_proxy.h"
 #include "chrome/browser/apps/app_service/app_service_proxy_factory.h"
@@ -16,7 +17,9 @@
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/ui/browser_list.h"
+#include "chrome/browser/ui/browser_list_observer.h"
 #include "chrome/browser/ui/browser_navigator_params.h"
+#include "chrome/browser/ui/browser_window.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/browser/ui/toolbar/app_menu_model.h"
 #include "chrome/browser/ui/web_applications/app_browser_controller.h"
@@ -55,6 +58,34 @@ void WaitUntilReady(WebAppProvider* provider) {
   run_loop.Run();
 }
 
+// Waits for |browser| to be removed from BrowserList and then calls |callback|.
+class BrowserRemovedWaiter final : public BrowserListObserver {
+ public:
+  explicit BrowserRemovedWaiter(Browser* browser) : browser_(browser) {}
+  ~BrowserRemovedWaiter() override = default;
+
+  void Wait() {
+    BrowserList::AddObserver(this);
+    run_loop_.Run();
+  }
+
+  // BrowserListObserver
+  void OnBrowserRemoved(Browser* browser) override {
+    if (browser != browser_)
+      return;
+
+    BrowserList::RemoveObserver(this);
+    // Post a task to ensure the Remove event has been dispatched to all
+    // observers.
+    base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE,
+                                                  run_loop_.QuitClosure());
+  }
+
+ private:
+  Browser* browser_;
+  base::RunLoop run_loop_;
+};
+
 }  // namespace
 
 AppId InstallWebApp(Profile* profile,
@@ -254,6 +285,12 @@ AppMenuCommandState GetAppMenuCommandState(int command_id, Browser* browser) {
   return model->IsEnabledAt(index) ? kEnabled : kDisabled;
 }
 
+void CloseAndWait(Browser* browser) {
+  BrowserRemovedWaiter waiter(browser);
+  browser->window()->Close();
+  waiter.Wait();
+}
+
 bool IsBrowserOpen(const Browser* test_browser) {
   for (Browser* browser : *BrowserList::GetInstance()) {
     if (browser == test_browser)

chrome/browser/ui/web_applications/test/web_app_browsertest_util.h

@@ -63,6 +63,8 @@ enum AppMenuCommandState {
 // For a non-app browser, determines if the command is enabled/disabled/absent.
 AppMenuCommandState GetAppMenuCommandState(int command_id, Browser* browser);
 
+void CloseAndWait(Browser* browser);
+
 bool IsBrowserOpen(const Browser* test_browser);
 
 // Launches the app for |app| in |profile|.

chrome/browser/ui/web_applications/web_app_browsertest.cc

@@ -124,9 +124,10 @@ class WebAppBrowserTest : public WebAppControllerBrowserTest {
 
     base::HistogramTester tester;
     const std::string base_url = "https://example.com/path";
+    const GURL app_url(base_url + base::NumberToString(index++));
     auto web_app_info = std::make_unique<WebApplicationInfo>();
-    web_app_info->start_url = GURL(base_url + base::NumberToString(index++));
-    web_app_info->scope = web_app_info->start_url;
+    web_app_info->start_url = app_url;
+    web_app_info->scope = app_url;
     web_app_info->display_mode = display_mode;
     web_app_info->open_as_window = open_as_window;
     if (display_override_mode)
@@ -139,6 +140,8 @@ class WebAppBrowserTest : public WebAppControllerBrowserTest {
         kLaunchWebAppDisplayModeHistogram,
         display_override_mode ? *display_override_mode : display_mode, 1);
 
+    NavigateToURLAndWait(app_browser, app_url);
+
     bool matches;
     EXPECT_TRUE(ExecuteScriptAndExtractBool(
         app_browser->tab_strip_model()->GetActiveWebContents(),
@@ -146,6 +149,7 @@ class WebAppBrowserTest : public WebAppControllerBrowserTest {
         "minimal-ui)').matches)",
         &matches));
     EXPECT_EQ(app_browser->app_controller()->HasMinimalUiButtons(), matches);
+    CloseAndWait(app_browser);
 
     return matches;
   }

chrome/browser/ui/web_applications/web_app_ui_manager_impl_browsertest.cc

@@ -6,14 +6,10 @@
 
 #include "base/barrier_closure.h"
 #include "base/test/bind.h"
-#include "base/threading/thread_task_runner_handle.h"
 #include "chrome/browser/apps/app_service/app_service_proxy.h"
 #include "chrome/browser/apps/app_service/app_service_proxy_factory.h"
 #include "chrome/browser/apps/app_service/built_in_chromeos_apps.h"
 #include "chrome/browser/ui/browser.h"
-#include "chrome/browser/ui/browser_list.h"
-#include "chrome/browser/ui/browser_list_observer.h"
-#include "chrome/browser/ui/browser_window.h"
 #include "chrome/browser/ui/web_applications/test/web_app_browsertest_util.h"
 #include "chrome/browser/web_applications/components/web_app_id.h"
 #include "chrome/browser/web_applications/components/web_application_info.h"
@@ -32,44 +28,8 @@
 #include "chrome/browser/ui/app_list/test/chrome_app_list_test_support.h"
 #endif
 
-namespace web_app {
-
 namespace {
 
-// Waits for |browser| to be removed from BrowserList and then calls |callback|.
-class BrowserRemovedWaiter final : public BrowserListObserver {
- public:
-  explicit BrowserRemovedWaiter(Browser* browser) : browser_(browser) {}
-  ~BrowserRemovedWaiter() override = default;
-
-  void Wait() {
-    BrowserList::AddObserver(this);
-    run_loop_.Run();
-  }
-
-  // BrowserListObserver
-  void OnBrowserRemoved(Browser* browser) override {
-    if (browser != browser_)
-      return;
-
-    BrowserList::RemoveObserver(this);
-    // Post a task to ensure the Remove event has been dispatched to all
-    // observers.
-    base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE,
-                                                  run_loop_.QuitClosure());
-  }
-
- private:
-  Browser* browser_;
-  base::RunLoop run_loop_;
-};
-
-void CloseAndWait(Browser* browser) {
-  BrowserRemovedWaiter waiter(browser);
-  browser->window()->Close();
-  waiter.Wait();
-}
-
 // TODO(https://crbug.com/1042727): Fix test GURL scoping and remove this getter
 // function.
 GURL FooUrl() {
@@ -81,6 +41,8 @@ GURL BarUrl() {
 
 }  // namespace
 
+namespace web_app {
+
 class WebAppUiManagerImplBrowserTest : public InProcessBrowserTest {
  protected:
   Profile* profile() { return browser()->profile(); }