Split AshTestHelper setup and put some pieces in the constructor.

This is yet more precursor work for making AshTestHelper an
AuraTestHelper subclass.  It moves into the constructor things that
are more "general-purpose" and don't rely on any state that might be
customized after construction (e.g. reading the ViewsDelegate), and
leaves SetUp() focused more on the ash-specific shell/windowing stuff.

This changes the parameters passed to the constructor vs. setup, since
ultimately some will be relevant to the Aura side and some won't.  For
now, this requires storing the |context_factory_| on the object to
pass it from the constructor to setup, but that will go away later.

This also adds a no-args SetUp() function, which just calls the main
SetUp() function.  This is a precursor to making SetUp() and TearDown()
be virtual method overrides.

A few other trivial changes:
* Explicitly resets |session_controller_client_| during TearDown(),
  since I forgot that previously.  (Should have no effect)
* Puts GetLocalStatePrefService() into the same order in the .cc file
  as the .h file.

Should be no functional effects.

Bug: none
Change-Id: Ida64c3fc0779f3bb38a032cf8d3f490104fe88ab
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2124934
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Auto-Submit: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#754707}

ash/shell/content/client/shell_browser_main_parts.cc

@@ -83,18 +83,16 @@ void ShellBrowserMainParts::ToolkitInitialized() {
 void ShellBrowserMainParts::PreMainMessageLoopRun() {
   browser_context_.reset(new content::ShellBrowserContext(false));
 
-  AshTestHelper::InitParams init_params;
-  init_params.delegate = std::make_unique<ShellDelegateImpl>();
-  init_params.context_factory = content::GetContextFactory();
+  if (!parameters_.ui_task)
+    new_window_delegate_ = std::make_unique<ShellNewWindowDelegate>();
+
   // TODO(oshima): Separate the class for ash_shell to reduce the test binary
   // size.
-  if (parameters_.ui_task) {
-    init_params.config_type = AshTestHelper::kPerfTest;
-  } else {
-    new_window_delegate_ = std::make_unique<ShellNewWindowDelegate>();
-    init_params.config_type = AshTestHelper::kShell;
-  }
-  ash_test_helper_ = std::make_unique<AshTestHelper>();
+  ash_test_helper_ = std::make_unique<AshTestHelper>(
+      parameters_.ui_task ? AshTestHelper::kPerfTest : AshTestHelper::kShell,
+      content::GetContextFactory());
+  AshTestHelper::InitParams init_params;
+  init_params.delegate = std::make_unique<ShellDelegateImpl>();
   ash_test_helper_->SetUp(std::move(init_params));
 
   window_watcher_ = std::make_unique<WindowWatcher>();

ash/test/ash_test_helper.cc

@@ -78,30 +78,14 @@ AshTestHelper::InitParams::InitParams() = default;
 AshTestHelper::InitParams::InitParams(InitParams&&) = default;
 AshTestHelper::InitParams::~InitParams() = default;
 
-AshTestHelper::AshTestHelper() {
-  views::ViewsTestHelperAura::SetFallbackTestViewsDelegateFactory(
-      base::BindOnce(&MakeDelegate));
-}
-
-AshTestHelper::~AshTestHelper() {
-  // Ensure the next test starts with a null display::Screen.  This must be done
-  // here instead of in TearDown() since some tests test access to the Screen
-  // after the shell shuts down (which they use TearDown() to trigger).
-  ScreenAsh::DeleteScreenForShutdown();
-
-  // This should never have a meaningful effect, since either there is no
-  // ViewsTestHelperAura instance or the instance is currently in its
-  // destructor.
-  views::ViewsTestHelperAura::SetFallbackTestViewsDelegateFactory(
-      base::NullCallback());
-}
-
-void AshTestHelper::SetUp(InitParams init_params) {
-  // Aura-general setup -------------------------------------------------------
+AshTestHelper::AshTestHelper(ConfigType config_type,
+                             ui::ContextFactory* context_factory)
+    : config_type_(config_type), context_factory_(context_factory) {
+  // Aura-general construction ------------------------------------------------
 
   wm_state_ = std::make_unique<::wm::WMState>();
 
-  if (init_params.config_type != kShell) {
+  if (config_type_ != kShell) {
     ui::test::EnableTestConfigForPlatformWindows();
     ui::InitializeInputMethodForTesting();
   }
@@ -109,14 +93,14 @@ void AshTestHelper::SetUp(InitParams init_params) {
   ui::test::EventGeneratorDelegate::SetFactoryFunction(
       base::BindRepeating(&aura::test::EventGeneratorDelegateAura::Create));
 
-  if (init_params.config_type == kUnitTest) {
+  if (config_type_ == kUnitTest) {
     zero_duration_mode_.reset(new ui::ScopedAnimationDurationScaleMode(
         ui::ScopedAnimationDurationScaleMode::ZERO_DURATION));
   }
 
-  if (!init_params.context_factory) {
+  if (!context_factory_) {
     context_factories_ = std::make_unique<ui::TestContextFactories>(false);
-    init_params.context_factory = context_factories_->GetContextFactory();
+    context_factory_ = context_factories_->GetContextFactory();
   }
 
   // Reset aura::Env to eliminate test dependency (https://crbug.com/586514).
@@ -124,7 +108,7 @@ void AshTestHelper::SetUp(InitParams init_params) {
   env_helper.ResetEnvForTesting();
   env_helper.SetInputStateLookup(std::unique_ptr<aura::InputStateLookup>());
 
-  // Ash-specific setup -------------------------------------------------------
+  // Ash-specific construction ------------------------------------------------
 
   command_line_ = std::make_unique<base::test::ScopedCommandLine>();
   statistics_provider_ =
@@ -136,6 +120,9 @@ void AshTestHelper::SetUp(InitParams init_params) {
   system_tray_client_ = std::make_unique<TestSystemTrayClient>();
   photo_controller_ = std::make_unique<TestPhotoController>();
 
+  views::ViewsTestHelperAura::SetFallbackTestViewsDelegateFactory(
+      base::BindOnce(&MakeDelegate));
+
   // TODO(jamescook): Can we do this without changing command line?
   // Use the origin (1,1) so that it doesn't over
   // lap with the native mouse cursor.
@@ -147,10 +134,10 @@ void AshTestHelper::SetUp(InitParams init_params) {
         ::switches::kHostWindowBounds, "10+10-800x600");
   }
 
-  if (init_params.config_type == kUnitTest)
+  if (config_type_ == kUnitTest)
     TabletModeController::SetUseScreenshotForTest(false);
 
-  if (init_params.config_type != kShell)
+  if (config_type_ != kShell)
     display::ResetDisplayIdForTest();
 
   chromeos::CrasAudioClient::InitializeFake();
@@ -161,7 +148,100 @@ void AshTestHelper::SetUp(InitParams init_params) {
   // Reset the global state for the cursor manager. This includes the
   // last cursor visibility state, etc.
   ::wm::CursorManager::ResetCursorVisibilityStateForTest();
+}
+
+AshTestHelper::~AshTestHelper() {
+  // Ensure the next test starts with a null display::Screen.  This must be done
+  // here instead of in TearDown() since some tests test access to the Screen
+  // after the shell shuts down (which they use TearDown() to trigger).
+  ScreenAsh::DeleteScreenForShutdown();
+
+  // This should never have a meaningful effect, since either there is no
+  // ViewsTestHelperAura instance or the instance is currently in its
+  // destructor.
+  views::ViewsTestHelperAura::SetFallbackTestViewsDelegateFactory(
+      base::NullCallback());
+}
+
+void AshTestHelper::SetUp() {
+  SetUp(InitParams());
+}
+
+void AshTestHelper::TearDown() {
+  // Ash-specific teardown ----------------------------------------------------
+
+  // The AppListTestHelper holds a pointer to the AppListController the Shell
+  // owns, so shut the test helper down first.
+  app_list_test_helper_.reset();
+
+  Shell::DeleteInstance();
+
+  // Suspend the tear down until all resources are returned via
+  // CompositorFrameSinkClient::ReclaimResources()
+  base::RunLoop().RunUntilIdle();
+
+  chromeos::CrasAudioHandler::Shutdown();
+  chromeos::CrasAudioClient::Shutdown();
+
+  // The PowerPolicyController holds a pointer to the PowerManagementClient, so
+  // shut the controller down first.
+  if (power_policy_controller_initialized_) {
+    chromeos::PowerPolicyController::Shutdown();
+    power_policy_controller_initialized_ = false;
+  }
+
+  chromeos::PowerManagerClient::Shutdown();
+
+  TabletModeController::SetUseScreenshotForTest(true);
+
+  // Destroy all owned objects to prevent tests from depending on their state
+  // after this returns.
+  test_keyboard_controller_observer_.reset();
+  session_controller_client_.reset();
+  test_views_delegate_.reset();
+  new_window_delegate_.reset();
+  if (bluez_dbus_manager_initialized_) {
+    device::BluetoothAdapterFactory::Shutdown();
+    bluez::BluezDBusManager::Shutdown();
+    bluez_dbus_manager_initialized_ = false;
+  }
+  photo_controller_.reset();
+  system_tray_client_.reset();
+  assistant_service_.reset();
+  notifier_settings_controller_.reset();
+  prefs_provider_.reset();
+  statistics_provider_.reset();
+  command_line_.reset();
+
+  // Aura-general teardown ----------------------------------------------------
+
+  ui::ShutdownInputMethodForTesting();
+
+  // Context factory referenced by Env is now destroyed. Reset Env's members in
+  // case some other test tries to use it. This matters if someone else created
+  // Env (such as the test suite) and is long lived.
+  if (aura::Env::HasInstance())
+    aura::Env::GetInstance()->set_context_factory(nullptr);
+
+  ui::test::EventGeneratorDelegate::SetFactoryFunction(
+      ui::test::EventGeneratorDelegate::FactoryFunction());
+
+  context_factories_.reset();
+  zero_duration_mode_.reset();
+  wm_state_.reset();
+}
 
+aura::Window* AshTestHelper::GetContext() {
+  aura::Window* root_window = Shell::GetRootWindowForNewWindows();
+  if (!root_window)
+    root_window = Shell::GetPrimaryRootWindow();
+  DCHECK(root_window);
+  return root_window;
+}
+
+void AshTestHelper::SetUp(InitParams init_params) {
+  // This block of objects are conditionally initialized here rather than in the
+  // constructor to make it easier for test classes to override them.
   if (!bluez::BluezDBusManager::IsInitialized()) {
     bluez::BluezDBusManager::InitializeFake();
     bluez_dbus_manager_initialized_ = true;
@@ -182,7 +262,7 @@ void AshTestHelper::SetUp(InitParams init_params) {
   shell_init_params.delegate = std::move(init_params.delegate);
   if (!shell_init_params.delegate)
     shell_init_params.delegate = std::make_unique<TestShellDelegate>();
-  shell_init_params.context_factory = init_params.context_factory;
+  shell_init_params.context_factory = context_factory_;
   shell_init_params.local_state = init_params.local_state;
   shell_init_params.keyboard_ui_factory =
       std::make_unique<TestKeyboardUIFactory>();
@@ -206,7 +286,7 @@ void AshTestHelper::SetUp(InitParams init_params) {
   // Requires the AppListController the Shell creates.
   app_list_test_helper_ = std::make_unique<AppListTestHelper>();
 
-  if (init_params.config_type == kShell) {
+  if (config_type_ == kShell) {
     shell->wallpaper_controller()->ShowDefaultWallpaperForTesting();
     return;
   }
@@ -221,7 +301,7 @@ void AshTestHelper::SetUp(InitParams init_params) {
       std::make_unique<TestKeyboardControllerObserver>(
           shell->keyboard_controller());
 
-  if (init_params.config_type != kUnitTest)
+  if (config_type_ != kUnitTest)
     return;
 
   // Tests that change the display configuration generally don't care about the
@@ -241,81 +321,10 @@ void AshTestHelper::SetUp(InitParams init_params) {
   shell->wallpaper_controller()->CreateEmptyWallpaperForTesting();
 }
 
-void AshTestHelper::TearDown() {
-  // Ash-specific teardown ----------------------------------------------------
-
-  // The AppListTestHelper holds a pointer to the AppListController the Shell
-  // owns, so shut the test helper down first.
-  app_list_test_helper_.reset();
-
-  Shell::DeleteInstance();
-
-  // Suspend the tear down until all resources are returned via
-  // CompositorFrameSinkClient::ReclaimResources()
-  base::RunLoop().RunUntilIdle();
-
-  chromeos::CrasAudioHandler::Shutdown();
-  chromeos::CrasAudioClient::Shutdown();
-
-  // The PowerPolicyController holds a pointer to the PowerManagementClient, so
-  // shut the controller down first.
-  if (power_policy_controller_initialized_) {
-    chromeos::PowerPolicyController::Shutdown();
-    power_policy_controller_initialized_ = false;
-  }
-
-  chromeos::PowerManagerClient::Shutdown();
-
-  TabletModeController::SetUseScreenshotForTest(true);
-
-  // Destroy all owned objects to prevent tests from depending on their state
-  // after this returns.
-  test_keyboard_controller_observer_.reset();
-  test_views_delegate_.reset();
-  new_window_delegate_.reset();
-  if (bluez_dbus_manager_initialized_) {
-    device::BluetoothAdapterFactory::Shutdown();
-    bluez::BluezDBusManager::Shutdown();
-    bluez_dbus_manager_initialized_ = false;
-  }
-  photo_controller_.reset();
-  system_tray_client_.reset();
-  assistant_service_.reset();
-  notifier_settings_controller_.reset();
-  prefs_provider_.reset();
-  statistics_provider_.reset();
-  command_line_.reset();
-
-  // Aura-general teardown ----------------------------------------------------
-
-  ui::ShutdownInputMethodForTesting();
-
-  // Context factory referenced by Env is now destroyed. Reset Env's members in
-  // case some other test tries to use it. This matters if someone else created
-  // Env (such as the test suite) and is long lived.
-  if (aura::Env::HasInstance())
-    aura::Env::GetInstance()->set_context_factory(nullptr);
-
-  ui::test::EventGeneratorDelegate::SetFactoryFunction(
-      ui::test::EventGeneratorDelegate::FactoryFunction());
-
-  context_factories_.reset();
-  zero_duration_mode_.reset();
-  wm_state_.reset();
-}
-
 PrefService* AshTestHelper::GetLocalStatePrefService() {
   return Shell::Get()->local_state_;
 }
 
-aura::Window* AshTestHelper::GetContext() {
-  aura::Window* root_window = Shell::GetRootWindowForNewWindows();
-  if (!root_window)
-    root_window = Shell::GetPrimaryRootWindow();
-  DCHECK(root_window);
-  return root_window;
-}
-
 display::Display AshTestHelper::GetSecondaryDisplay() const {
   return display::test::DisplayManagerTestApi(Shell::Get()->display_manager())
       .GetSecondaryDisplay();

ash/test/ash_test_helper.h

@@ -61,12 +61,6 @@ class TestPhotoController;
 // root window and an ash::Shell instance with a test delegate.
 class AshTestHelper {
  public:
-  // Instantiates/destroys an AshTestHelper. This can happen in a
-  // single-threaded phase without a backing task environment. As such, the vast
-  // majority of initialization/tear down will be done in SetUp()/TearDown().
-  AshTestHelper();
-  ~AshTestHelper();
-
   enum ConfigType {
     // The configuration for shell executable.
     kShell,
@@ -87,23 +81,31 @@ class AshTestHelper {
     bool start_session = true;
     // If this is not set, a TestShellDelegate will be used automatically.
     std::unique_ptr<ShellDelegate> delegate;
-    ui::ContextFactory* context_factory = nullptr;
     PrefService* local_state = nullptr;
-    ConfigType config_type = kUnitTest;
   };
 
-  // Creates the ash::Shell and performs associated initialization according
-  // to |init_params|.
-  void SetUp(InitParams init_params = InitParams());
+  // Instantiates/destroys an AshTestHelper. This can happen in a
+  // single-threaded phase without a backing task environment or ViewsDelegate,
+  // and must not create those lest the caller wish to do so.
+  explicit AshTestHelper(ConfigType config_type = kUnitTest,
+                         ui::ContextFactory* context_factory = nullptr);
+  ~AshTestHelper();
+
+  // Calls through to SetUp() below, see comments there.
+  void SetUp();
 
-  // Destroys the ash::Shell and performs associated cleanup.
+  // Tears down everything but the Screen instance, which some tests access
+  // after this point.
   void TearDown();
 
-  // Returns a root Window. Usually this is the active root Window, but that
-  // method can return NULL sometimes, and in those cases, we fall back on the
-  // primary root Window.
   aura::Window* GetContext();
 
+  // Creates the ash::Shell and performs associated initialization according
+  // to |init_params|.  When this function returns it guarantees a task
+  // environment and ViewsDelegate will exist, the shell will be started, and a
+  // window will be showing.
+  void SetUp(InitParams init_params);
+
   PrefService* GetLocalStatePrefService();
 
   display::Display GetSecondaryDisplay() const;
@@ -138,6 +140,8 @@ class AshTestHelper {
   void reset_commandline() { command_line_.reset(); }
 
  private:
+  ConfigType config_type_;
+  ui::ContextFactory* context_factory_;
   std::unique_ptr<::wm::WMState> wm_state_;
   std::unique_ptr<ui::ScopedAnimationDurationScaleMode> zero_duration_mode_;
   std::unique_ptr<ui::TestContextFactories> context_factories_;