Don't bother showing an unused Browser* for FileManagerBrowserTestBase.

This affects file manager tests, gallery tests, and others. With a few
exceptions, these tests have no use for the browser() that
StartupBrowserCreator creates during startup. Not creating one leaves
more time for the tests to do things they really care about before
potentially timing out.

In fact, _not_ creating the Browser surfaces an existing bug more
prominently. That is, some tests have an iceberg dependency on the
existence of this Browser* that they should never have had in the
first place.

Bug: 736930
Change-Id: I1030e41800f3d2e0e5868ecd64d70c7d30f47c28
Reviewed-on: https://chromium-review.googlesource.com/1117977Reviewed-by: Noel Gordon <noel@chromium.org>
Commit-Queue: Trent Apted <tapted@chromium.org>
Cr-Commit-Position: refs/heads/master@{#576427}

chrome/browser/chromeos/file_manager/external_filesystem_apitest.cc

@@ -391,15 +391,13 @@ class LocalFileSystemExtensionApiTest : public FileSystemExtensionApiTestBase {
 
   // FileSystemExtensionApiTestBase override.
   void AddTestMountPoint() override {
-    EXPECT_TRUE(content::BrowserContext::GetMountPoints(browser()->profile())
-                    ->RegisterFileSystem(kLocalMountPointName,
-                                         storage::kFileSystemTypeNativeLocal,
-                                         storage::FileSystemMountOption(),
-                                         mount_point_dir_));
-    VolumeManager::Get(browser()->profile())
-        ->AddVolumeForTesting(mount_point_dir_, VOLUME_TYPE_TESTING,
-                              chromeos::DEVICE_TYPE_UNKNOWN,
-                              false /* read_only */);
+    EXPECT_TRUE(
+        content::BrowserContext::GetMountPoints(profile())->RegisterFileSystem(
+            kLocalMountPointName, storage::kFileSystemTypeNativeLocal,
+            storage::FileSystemMountOption(), mount_point_dir_));
+    VolumeManager::Get(profile())->AddVolumeForTesting(
+        mount_point_dir_, VOLUME_TYPE_TESTING, chromeos::DEVICE_TYPE_UNKNOWN,
+        false /* read_only */);
   }
 
  private:
@@ -424,15 +422,13 @@ class RestrictedFileSystemExtensionApiTest
   // FileSystemExtensionApiTestBase override.
   void AddTestMountPoint() override {
     EXPECT_TRUE(
-        content::BrowserContext::GetMountPoints(browser()->profile())
-            ->RegisterFileSystem(kRestrictedMountPointName,
-                                 storage::kFileSystemTypeRestrictedNativeLocal,
-                                 storage::FileSystemMountOption(),
-                                 mount_point_dir_));
-    VolumeManager::Get(browser()->profile())
-        ->AddVolumeForTesting(mount_point_dir_, VOLUME_TYPE_TESTING,
-                              chromeos::DEVICE_TYPE_UNKNOWN,
-                              true /* read_only */);
+        content::BrowserContext::GetMountPoints(profile())->RegisterFileSystem(
+            kRestrictedMountPointName,
+            storage::kFileSystemTypeRestrictedNativeLocal,
+            storage::FileSystemMountOption(), mount_point_dir_));
+    VolumeManager::Get(profile())->AddVolumeForTesting(
+        mount_point_dir_, VOLUME_TYPE_TESTING, chromeos::DEVICE_TYPE_UNKNOWN,
+        true /* read_only */);
   }
 
  private:
@@ -465,7 +461,7 @@ class DriveFileSystemExtensionApiTest : public FileSystemExtensionApiTestBase {
 
   // FileSystemExtensionApiTestBase override.
   void AddTestMountPoint() override {
-    test_util::WaitUntilDriveMountPointIsAdded(browser()->profile());
+    test_util::WaitUntilDriveMountPointIsAdded(profile());
   }
 
   // FileSystemExtensionApiTestBase override.
@@ -550,7 +546,7 @@ class MultiProfileDriveFileSystemExtensionApiTest :
   }
 
   void AddTestMountPoint() override {
-    test_util::WaitUntilDriveMountPointIsAdded(browser()->profile());
+    test_util::WaitUntilDriveMountPointIsAdded(profile());
     test_util::WaitUntilDriveMountPointIsAdded(second_profile_);
   }
 
@@ -571,7 +567,7 @@ class MultiProfileDriveFileSystemExtensionApiTest :
     const char kResourceId[] = "unique-id-for-multiprofile-copy-test";
     drive::FakeDriveService* const main_service =
         static_cast<drive::FakeDriveService*>(
-            drive::util::GetDriveServiceByProfile(browser()->profile()));
+            drive::util::GetDriveServiceByProfile(profile()));
     drive::FakeDriveService* const sub_service =
         static_cast<drive::FakeDriveService*>(
             drive::util::GetDriveServiceByProfile(second_profile_));
@@ -625,16 +621,14 @@ class LocalAndDriveFileSystemExtensionApiTest
 
   // FileSystemExtensionApiTestBase override.
   void AddTestMountPoint() override {
-    EXPECT_TRUE(content::BrowserContext::GetMountPoints(browser()->profile())
-                    ->RegisterFileSystem(kLocalMountPointName,
-                                         storage::kFileSystemTypeNativeLocal,
-                                         storage::FileSystemMountOption(),
-                                         local_mount_point_dir_));
-    VolumeManager::Get(browser()->profile())
-        ->AddVolumeForTesting(local_mount_point_dir_, VOLUME_TYPE_TESTING,
-                              chromeos::DEVICE_TYPE_UNKNOWN,
-                              false /* read_only */);
-    test_util::WaitUntilDriveMountPointIsAdded(browser()->profile());
+    EXPECT_TRUE(
+        content::BrowserContext::GetMountPoints(profile())->RegisterFileSystem(
+            kLocalMountPointName, storage::kFileSystemTypeNativeLocal,
+            storage::FileSystemMountOption(), local_mount_point_dir_));
+    VolumeManager::Get(profile())->AddVolumeForTesting(
+        local_mount_point_dir_, VOLUME_TYPE_TESTING,
+        chromeos::DEVICE_TYPE_UNKNOWN, false /* read_only */);
+    test_util::WaitUntilDriveMountPointIsAdded(profile());
   }
 
  protected:

chrome/browser/chromeos/file_manager/file_manager_browsertest.cc

@@ -42,11 +42,23 @@ struct TestCase {
     return *this;
   }
 
+  // Show the startup browser. Some tests invoke the file picker dialog during
+  // the test. Requesting a file picker from a background page is forbidden by
+  // the apps platform, and it's a bug that these tests do so.
+  // FindRuntimeContext() in select_file_dialog_extension.cc will use the last
+  // active browser in this case, which requires a Browser to be present. See
+  // https://crbug.com/736930.
+  TestCase& WithBrowser() {
+    with_browser = true;
+    return *this;
+  }
+
   const char* test_case_name = nullptr;
   GuestMode guest_mode = NOT_IN_GUEST_MODE;
   bool trusted_events = false;
   bool tablet_mode = false;
   bool enable_drivefs = false;
+  bool with_browser = false;
 };
 
 // EventCase: FilesAppBrowserTest with trusted JS Events.
@@ -81,6 +93,9 @@ class FilesAppBrowserTest : public FileManagerBrowserTestBase,
 
   GuestMode GetGuestMode() const override { return GetParam().guest_mode; }
   bool GetEnableDriveFs() const override { return GetParam().enable_drivefs; }
+  bool GetRequiresStartupBrowser() const override {
+    return GetParam().with_browser;
+  }
 
   const char* GetTestCaseName() const override {
     return GetParam().test_case_name;
@@ -407,33 +422,38 @@ WRAPPED_INSTANTIATE_TEST_CASE_P(
         EventCase("tabindexFocusDownloads"),
         EventCase("tabindexFocusDownloads").InGuestMode(),
         EventCase("tabindexFocusDirectorySelected"),
-        EventCase("tabindexOpenDialogDrive"),
-        EventCase("tabindexOpenDialogDrive").EnableDriveFs(),
-        EventCase("tabindexOpenDialogDownloads"),
-        EventCase("tabindexOpenDialogDownloads").InGuestMode(),
-        EventCase("tabindexSaveFileDialogDrive"),
-        EventCase("tabindexSaveFileDialogDrive").EnableDriveFs(),
-        EventCase("tabindexSaveFileDialogDownloads"),
-        EventCase("tabindexSaveFileDialogDownloads").InGuestMode()));
+        EventCase("tabindexOpenDialogDrive").WithBrowser(),
+        EventCase("tabindexOpenDialogDrive").WithBrowser().EnableDriveFs(),
+        EventCase("tabindexOpenDialogDownloads").WithBrowser(),
+        EventCase("tabindexOpenDialogDownloads").WithBrowser().InGuestMode(),
+        EventCase("tabindexSaveFileDialogDrive").WithBrowser(),
+        EventCase("tabindexSaveFileDialogDrive").WithBrowser().EnableDriveFs(),
+        EventCase("tabindexSaveFileDialogDownloads").WithBrowser(),
+        EventCase("tabindexSaveFileDialogDownloads")
+            .WithBrowser()
+            .InGuestMode()));
 
 WRAPPED_INSTANTIATE_TEST_CASE_P(
     FileDialog, /* file_dialog.js */
     FilesAppBrowserTest,
     ::testing::Values(
-        TestCase("openFileDialogUnload"),
-        TestCase("openFileDialogDownloads"),
-        TestCase("openFileDialogDownloads").InGuestMode(),
-        TestCase("openFileDialogDownloads").InIncognito(),
-        TestCase("openFileDialogCancelDownloads"),
-        TestCase("openFileDialogEscapeDownloads"),
-        TestCase("openFileDialogDrive"),
-        TestCase("openFileDialogDrive").InIncognito(),
-        TestCase("openFileDialogDrive").EnableDriveFs(),
-        TestCase("openFileDialogDrive").InIncognito().EnableDriveFs(),
-        TestCase("openFileDialogCancelDrive"),
-        TestCase("openFileDialogCancelDrive").EnableDriveFs(),
-        TestCase("openFileDialogEscapeDrive"),
-        TestCase("openFileDialogEscapeDrive").EnableDriveFs()));
+        TestCase("openFileDialogUnload").WithBrowser(),
+        TestCase("openFileDialogDownloads").WithBrowser(),
+        TestCase("openFileDialogDownloads").WithBrowser().InGuestMode(),
+        TestCase("openFileDialogDownloads").WithBrowser().InIncognito(),
+        TestCase("openFileDialogCancelDownloads").WithBrowser(),
+        TestCase("openFileDialogEscapeDownloads").WithBrowser(),
+        TestCase("openFileDialogDrive").WithBrowser(),
+        TestCase("openFileDialogDrive").WithBrowser().InIncognito(),
+        TestCase("openFileDialogDrive").WithBrowser().EnableDriveFs(),
+        TestCase("openFileDialogDrive")
+            .WithBrowser()
+            .InIncognito()
+            .EnableDriveFs(),
+        TestCase("openFileDialogCancelDrive").WithBrowser(),
+        TestCase("openFileDialogCancelDrive").WithBrowser().EnableDriveFs(),
+        TestCase("openFileDialogEscapeDrive").WithBrowser(),
+        TestCase("openFileDialogEscapeDrive").WithBrowser().EnableDriveFs()));
 
 WRAPPED_INSTANTIATE_TEST_CASE_P(
     CopyBetweenWindows, /* copy_between_windows.js */

chrome/browser/chromeos/file_manager/file_manager_browsertest_base.cc

@@ -890,6 +890,23 @@ void FileManagerBrowserTestBase::SetUpCommandLine(
   // Use a fake audio stream crbug.com/835626
   command_line->AppendSwitch(switches::kDisableAudioOutput);
 
+  if (!GetRequiresStartupBrowser()) {
+    // Don't sink time into showing an unused browser window.
+    // InProcessBrowserTest::browser() will be null.
+    command_line->AppendSwitch(switches::kNoStartupWindow);
+
+    // Without a browser window, opening an app window, then closing it will
+    // trigger browser shutdown. Usually this is fine, except it also prevents
+    // any _new_ app window being created, should a test want to do that.
+    // (At the time of writing, exactly one does).
+    // Although in this path no browser is created (and so one can never
+    // close..), setting this to false prevents InProcessBrowserTest from adding
+    // the kDisableZeroBrowsersOpenForTests flag, which would prevent
+    // chrome_browser_main_chromeos from adding the keepalive that normally
+    // stops chromeos from shutting down unexpectedly.
+    set_exit_when_last_browser_closes(false);
+  }
+
   if (IsGuestModeTest()) {
     command_line->AppendSwitch(chromeos::switches::kGuestSession);
     command_line->AppendSwitchNative(chromeos::switches::kLoginUser, "$guest");
@@ -963,6 +980,7 @@ void FileManagerBrowserTestBase::SetUpInProcessBrowserTestFixture() {
 void FileManagerBrowserTestBase::SetUpOnMainThread() {
   extensions::ExtensionApiTest::SetUpOnMainThread();
   CHECK(profile());
+  CHECK_EQ(!!browser(), GetRequiresStartupBrowser());
 
   CHECK(local_volume_->Mount(profile()));
 
@@ -979,8 +997,7 @@ void FileManagerBrowserTestBase::SetUpOnMainThread() {
     // CustomMountPointCallback. TODO(joelhockey): It would be better if the
     // crostini interface allowed for testing without such tight coupling.
     crostini_volume_ = std::make_unique<CrostiniTestVolume>();
-    browser()->profile()->GetPrefs()->SetBoolean(
-        crostini::prefs::kCrostiniEnabled, true);
+    profile()->GetPrefs()->SetBoolean(crostini::prefs::kCrostiniEnabled, true);
     crostini::CrostiniManager::GetInstance()->set_skip_restart_for_testing();
     chromeos::DBusThreadManager* dbus_thread_manager =
         chromeos::DBusThreadManager::Get();
@@ -1018,6 +1035,10 @@ bool FileManagerBrowserTestBase::GetEnableDriveFs() const {
   return false;
 }
 
+bool FileManagerBrowserTestBase::GetRequiresStartupBrowser() const {
+  return false;
+}
+
 void FileManagerBrowserTestBase::StartTest() {
   LOG(INFO) << "FileManagerBrowserTest::StartTest " << GetTestCaseName();
   static const base::FilePath test_extension_dir =

chrome/browser/chromeos/file_manager/file_manager_browsertest_base.h

@@ -41,6 +41,7 @@ class FileManagerBrowserTestBase : public extensions::ExtensionApiTest {
   // Overrides for each FileManagerBrowserTest test extension type.
   virtual GuestMode GetGuestMode() const = 0;
   virtual bool GetEnableDriveFs() const;
+  virtual bool GetRequiresStartupBrowser() const;
   virtual const char* GetTestCaseName() const = 0;
   virtual const char* GetTestExtensionManifestName() const = 0;