Remove AshTestBase::SetUp(std::unique_ptr<TestShellDelegate>)

This is dangerous because SetUp() in subclass may call this, which in
turn may bypass a SetUp in its super class. (like in exo)

Instead of adding aonther "set_delegate", CL allows a subclass update
init_params_ directly via input_parmas(). This uses reference as the
caller should never see nullptr (or it should be treated as fatal).

This also updates ctor of ChromeAhsTestBase to call its ctor so that it
has single call path to super classe's ctor.

Bug: None

Change-Id: If18131a3f3e5e893b473013f9383044b22c9e9e7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6355206
Reviewed-by: Hidehiko Abe <hidehiko@chromium.org>
Commit-Queue: Mitsuru Oshima <oshima@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1435864}

ash/drag_drop/drag_drop_capture_delegate_unittest.cc

@@ -39,7 +39,8 @@ class DragDropCaptureDelegateTest : public AshTestBase {
   // AshTestBase:
   void SetUp() override {
     drag_drop_capture_delegate_.reset(new DragDropCaptureDelegate());
-    AshTestBase::SetUp(std::make_unique<TestShellDelegate>());
+    set_shell_delegate(std::make_unique<TestShellDelegate>());
+    AshTestBase::SetUp();
   }
 
   void TearDown() override {

ash/drag_drop/drag_drop_controller_unittest.cc

@@ -464,7 +464,8 @@ class DragDropControllerTest : public AshTestBase {
   void SetUp() override {
     auto mock_shell_delegate = std::make_unique<NiceMock<MockShellDelegate>>();
     mock_shell_delegate_ = mock_shell_delegate.get();
-    AshTestBase::SetUp(std::move(mock_shell_delegate));
+    set_shell_delegate(std::move(mock_shell_delegate));
+    AshTestBase::SetUp();
 
     drag_drop_controller_ = std::make_unique<TestDragDropController>();
     drag_drop_controller_->set_should_block_during_drag_drop(false);

ash/drag_drop/tab_drag_drop_delegate_unittest.cc

@@ -91,7 +91,8 @@ class TabDragDropDelegateTest : public AshTestBase {
 
     auto mock_shell_delegate = std::make_unique<NiceMock<MockShellDelegate>>();
     mock_shell_delegate_ = mock_shell_delegate.get();
-    AshTestBase::SetUp(std::move(mock_shell_delegate));
+    set_shell_delegate(std::move(mock_shell_delegate));
+    AshTestBase::SetUp();
     ash::TabletModeControllerTestApi().EnterTabletMode();
 
     // Create a dummy window and exit overview mode since drags can't be

ash/lobster/lobster_session_impl_unittest.cc

@@ -110,7 +110,8 @@ class LobsterSessionImplTest : public AshTestBase {
     auto shell_delegate = std::make_unique<TestShellDelegate>();
     shell_delegate->SetSendSpecializedFeatureFeedbackCallback(
         mock_send_specialized_feature_feedback_.Get());
-    AshTestBase::SetUp(std::move(shell_delegate));
+    set_shell_delegate(std::move(shell_delegate));
+    AshTestBase::SetUp();
     ASSERT_TRUE(scoped_temp_dir_.CreateUniqueTempDir());
   }
 

ash/multi_device_setup/multi_device_notification_presenter_unittest.cc

@@ -117,7 +117,8 @@ class MultiDeviceNotificationPresenterTest : public NoSessionAshTestBase {
     delegate->SetMultiDeviceSetupBinder(base::BindRepeating(
         &multidevice_setup::MultiDeviceSetupBase::BindReceiver,
         base::Unretained(fake_multidevice_setup_.get())));
-    NoSessionAshTestBase::SetUp(std::move(delegate));
+    set_shell_delegate(std::move(delegate));
+    NoSessionAshTestBase::SetUp();
 
     test_system_tray_client_ = GetSystemTrayClient();
 

ash/scanner/scanner_controller_unittest.cc

@@ -174,7 +174,8 @@ class ScannerControllerTest : public AshTestBase {
     auto shell_delegate = std::make_unique<TestShellDelegate>();
     shell_delegate->SetSendSpecializedFeatureFeedbackCallback(
         mock_send_specialized_feature_feedback_.Get());
-    AshTestBase::SetUp(std::move(shell_delegate));
+    set_shell_delegate(std::move(shell_delegate));
+    AshTestBase::SetUp();
   }
 
   base::MockCallback<TestShellDelegate::SendSpecializedFeatureFeedbackCallback>&

ash/session/fullscreen_controller_unittest.cc

@@ -54,7 +54,8 @@ class FullscreenControllerTest : public AshTestBase {
     auto test_shell_delegate = std::make_unique<TestShellDelegate>();
     test_shell_delegate_ = test_shell_delegate.get();
 
-    AshTestBase::SetUp(std::move(test_shell_delegate));
+    set_shell_delegate(std::move(test_shell_delegate));
+    AshTestBase::SetUp();
 
     CreateFullscreenWindow();
   }

ash/shelf/shelf_widget_unittest.cc

@@ -952,9 +952,9 @@ TEST_F(ShelfWidgetTest,
   ASSERT_FALSE(GetShelfWidget()->GetAnimatingBackground()->visible());
 }
 
-class ShelfWidgetAfterLoginTest : public AshTestBase {
+class ShelfWidgetAfterLoginTest : public NoSessionAshTestBase {
  public:
-  ShelfWidgetAfterLoginTest() { set_start_session(false); }
+  ShelfWidgetAfterLoginTest() = default;
 
   ShelfWidgetAfterLoginTest(const ShelfWidgetAfterLoginTest&) = delete;
   ShelfWidgetAfterLoginTest& operator=(const ShelfWidgetAfterLoginTest&) =
@@ -1035,9 +1035,9 @@ TEST_F(ShelfWidgetAfterLoginTest, CreateLockedShelf) {
             SHELF_VISIBLE, SHELF_AUTO_HIDE_HIDDEN);
 }
 
-class ShelfWidgetViewsVisibilityTest : public AshTestBase {
+class ShelfWidgetViewsVisibilityTest : public NoSessionAshTestBase {
  public:
-  ShelfWidgetViewsVisibilityTest() { set_start_session(false); }
+  ShelfWidgetViewsVisibilityTest() = default;
 
   ShelfWidgetViewsVisibilityTest(const ShelfWidgetViewsVisibilityTest&) =
       delete;

ash/system/channel_indicator/channel_indicator_pixeltest.cc

@@ -32,7 +32,8 @@ class ChannelIndicatorPixelTest
     std::unique_ptr<TestShellDelegate> shell_delegate =
         std::make_unique<TestShellDelegate>();
     shell_delegate->set_channel(GetChannel());
-    AshTestBase::SetUp(std::move(shell_delegate));
+    set_shell_delegate(std::move(shell_delegate));
+    AshTestBase::SetUp();
     GetSessionControllerClient()->SetSessionState(
         IsLoggedIn() ? session_manager::SessionState::ACTIVE
                      : session_manager::SessionState::LOGIN_PRIMARY);

ash/system/channel_indicator/channel_indicator_quick_settings_view_pixeltest.cc

@@ -31,7 +31,8 @@ class ChannelIndicatorQuickSettingsViewPixelTest : public AshTestBase {
     // Install a test delegate to allow overriding channel version.
     auto delegate = std::make_unique<TestShellDelegate>();
     delegate->set_channel(version_info::Channel::BETA);
-    AshTestBase::SetUp(std::move(delegate));
+    set_shell_delegate(std::move(delegate));
+    AshTestBase::SetUp();
 
     system_tray_client_ = GetSystemTrayClient();
     system_tray_client_->set_user_feedback_enabled(true);

ash/system/channel_indicator/channel_indicator_unittest.cc

@@ -47,7 +47,8 @@ class ChannelIndicatorViewTest
     std::unique_ptr<TestShellDelegate> shell_delegate =
         std::make_unique<TestShellDelegate>();
     shell_delegate->set_channel(static_cast<version_info::Channel>(GetParam()));
-    AshTestBase::SetUp(std::move(shell_delegate));
+    set_shell_delegate(std::move(shell_delegate));
+    AshTestBase::SetUp();
   }
 
   void SetSessionState(session_manager::SessionState state) {

ash/system/channel_indicator/channel_indicator_utils_unittest.cc

@@ -39,7 +39,8 @@ class ChannelIndicatorUtilsTest : public AshTestBase {
     std::unique_ptr<TestShellDelegate> shell_delegate =
         std::make_unique<TestShellDelegate>();
     shell_delegate->set_version_string(kTestOsVersion);
-    AshTestBase::SetUp(std::move(shell_delegate));
+    set_shell_delegate(std::move(shell_delegate));
+    AshTestBase::SetUp();
   }
 };
 

ash/system/keyboard_brightness/keyboard_backlight_color_controller_unittest.cc

@@ -69,12 +69,11 @@ class TestWallpaperObserver : public ash::WallpaperControllerObserver {
 };
 }  // namespace
 
-class KeyboardBacklightColorControllerTest : public AshTestBase {
+class KeyboardBacklightColorControllerTest : public NoSessionAshTestBase {
  public:
   KeyboardBacklightColorControllerTest() {
     scoped_feature_list_.InitWithFeatures({features::kMultiZoneRgbKeyboard},
                                           {});
-    set_start_session(false);
   }
 
   KeyboardBacklightColorControllerTest(
@@ -86,7 +85,7 @@ class KeyboardBacklightColorControllerTest : public AshTestBase {
 
   // testing::Test:
   void SetUp() override {
-    AshTestBase::SetUp();
+    NoSessionAshTestBase::SetUp();
 
     controller_ =
         std::make_unique<KeyboardBacklightColorController>(local_state());
@@ -96,7 +95,7 @@ class KeyboardBacklightColorControllerTest : public AshTestBase {
 
   void TearDown() override {
     controller_.reset();
-    AshTestBase::TearDown();
+    NoSessionAshTestBase::TearDown();
   }
 
  protected:

ash/system/network/network_list_view_controller_unittest.cc

@@ -204,8 +204,8 @@ class NetworkListViewControllerTest : public AshTestBase,
     delegate->SetMultiDeviceSetupBinder(base::BindRepeating(
         &multidevice_setup::MultiDeviceSetupBase::BindReceiver,
         base::Unretained(fake_multidevice_setup_.get())));
-
-    AshTestBase::SetUp(std::move(delegate));
+    set_shell_delegate(std::move(delegate));
+    AshTestBase::SetUp();
 
     cros_network_ = std::make_unique<FakeCrosNetworkConfig>();
     Shell::Get()

ash/system/phonehub/phone_hub_ui_controller_unittest.cc

@@ -36,22 +36,21 @@ constexpr char kScreenOnOpenedMetric[] =
     "PhoneHub.BubbleOpened.Connectable.Page";
 constexpr base::TimeDelta kConnectingViewGracePeriod = base::Seconds(40);
 
-class PhoneHubUiControllerTest : public AshTestBase,
+class PhoneHubUiControllerTest : public NoSessionAshTestBase,
                                  public PhoneHubUiController::Observer {
  public:
   PhoneHubUiControllerTest()
-      : AshTestBase(base::test::TaskEnvironment::TimeSource::MOCK_TIME) {
-    set_start_session(false);
-  }
+      : NoSessionAshTestBase(
+            base::test::TaskEnvironment::TimeSource::MOCK_TIME) {}
 
   ~PhoneHubUiControllerTest() override { controller_->RemoveObserver(this); }
 
-  // AshTestBase:
+  // NoSessionAshTestBase:
   void SetUp() override {
     feature_list_.InitWithFeatures(
         {features::kEcheSWA, features::kEcheNetworkConnectionState}, {});
 
-    AshTestBase::SetUp();
+    NoSessionAshTestBase::SetUp();
 
     handler_ = std::make_unique<eche_app::EcheConnectionStatusHandler>();
     phone_hub_manager_.set_host_last_seen_timestamp(std::nullopt);

ash/system/unified/quick_settings_header_unittest.cc

@@ -51,7 +51,8 @@ class QuickSettingsHeaderTest : public NoSessionAshTestBase {
     // Install a test delegate to allow overriding channel version.
     auto delegate = std::make_unique<TestShellDelegate>();
     test_shell_delegate_ = delegate.get();
-    NoSessionAshTestBase::SetUp(std::move(delegate));
+    set_shell_delegate(std::move(delegate));
+    NoSessionAshTestBase::SetUp();
 
     model_ = base::MakeRefCounted<UnifiedSystemTrayModel>(nullptr);
     controller_ = std::make_unique<UnifiedSystemTrayController>(model_.get());

ash/system/unified/unified_system_tray_unittest.cc

@@ -1014,7 +1014,8 @@ class UnifiedSystemTrayAccessibilityTest : public AshTestBase {
     std::unique_ptr<TestShellDelegate> shell_delegate =
         std::make_unique<TestShellDelegate>();
     shell_delegate->set_channel(version_info::Channel::BETA);
-    AshTestBase::SetUp(std::move(shell_delegate));
+    set_shell_delegate(std::move(shell_delegate));
+    AshTestBase::SetUp();
 
     scoped_fake_power_status_ = std::make_unique<ScopedFakePowerStatus>();
 

ash/test/ash_test_base.cc

@@ -145,23 +145,18 @@ AshTestBase::~AshTestBase() {
 }
 
 void AshTestBase::SetUp() {
-  SetUp(nullptr);
-}
-
-void AshTestBase::SetUp(std::unique_ptr<TestShellDelegate> delegate) {
   // At this point, the task APIs should already be provided by
   // |task_environment_|.
   CHECK(base::SingleThreadTaskRunner::HasCurrentDefault());
   CHECK(base::ThreadPoolInstance::Get());
 
   setup_called_ = true;
-
-  init_params_.delegate = std::move(delegate);
-  init_params_.local_state = local_state();
+  CHECK(!init_params_->local_state) << "local state can not be overridden";
+  init_params_->local_state = local_state();
   // AshTestBase destroys the Screen instance at the destructor,
   // because some of the tests verifies the screen instance
   // after the ash::Shell destroyed in AshTestHelper::TearDown().
-  init_params_.destroy_screen = false;
+  init_params_->destroy_screen = false;
 
   // Prepare for a pixel test if having pixel init params.
   std::optional<pixel_test::InitParams> pixel_test_init_params =
@@ -176,7 +171,8 @@ void AshTestBase::SetUp(std::unique_ptr<TestShellDelegate> delegate) {
       std::make_unique<ui::TestContextFactories>(/*enable_pixel_output=*/false);
   ash_test_helper_ = std::make_unique<AshTestHelper>(
       test_context_factories_->GetContextFactory());
-  ash_test_helper_->SetUp(std::move(init_params_));
+  ash_test_helper_->SetUp(std::move(*init_params_));
+  init_params_.reset();
 
   // Call `StabilizeUI()` after the user session is activated (if any) in the
   // test setup.

ash/test/ash_test_base.h

@@ -74,7 +74,6 @@ class AshTestHelper;
 class NotificationCenterTray;
 class Shelf;
 class TestAppListClient;
-class TestShellDelegate;
 class TestSystemTrayClient;
 class UnifiedSystemTray;
 class WorkAreaInsets;
@@ -105,7 +104,6 @@ class AshTestBase : public testing::Test {
 
   // testing::Test:
   void SetUp() override;
-  void SetUp(std::unique_ptr<TestShellDelegate> delegate);
   void TearDown() override;
 
   // Returns the notification center tray on the primary display.
@@ -274,21 +272,34 @@ class AshTestBase : public testing::Test {
       const;
 
   void set_start_session(bool start_session) {
-    init_params_.start_session = start_session;
+    CHECK(init_params_) << "start_session must set before calling SetUp()";
+    init_params_->start_session = start_session;
   }
 
   void set_create_signin_pref_service(bool create_signin_pref_service) {
-    init_params_.create_signin_pref_service = create_signin_pref_service;
+    CHECK(init_params_)
+        << "create_create_signin_pref_service must set before calling SetUp()";
+    init_params_->create_signin_pref_service = create_signin_pref_service;
   }
 
   void set_create_global_cras_audio_handler(
       bool create_global_cras_audio_handler) {
-    init_params_.create_global_cras_audio_handler =
+    CHECK(init_params_)
+        << "create_global_cras_audio_handler must set before calling SetUp()";
+    init_params_->create_global_cras_audio_handler =
         create_global_cras_audio_handler;
   }
 
   void set_create_quick_pair_mediator(bool create_quick_pair_mediator) {
-    init_params_.create_quick_pair_mediator = create_quick_pair_mediator;
+    CHECK(init_params_)
+        << "create_quick_pair_mediator must set before calling SetUp()";
+    init_params_->create_quick_pair_mediator = create_quick_pair_mediator;
+  }
+
+  void set_shell_delegate(std::unique_ptr<ShellDelegate> shell_delegate) {
+    CHECK(init_params_) << "shell_delegate mustset before calling SetUp()";
+    CHECK(!init_params_->delegate);
+    init_params_->delegate = std::move(shell_delegate);
   }
 
   base::test::TaskEnvironment* task_environment() {
@@ -388,7 +399,8 @@ class AshTestBase : public testing::Test {
   bool teardown_called_ = false;
 
   // AshTestHelper's init params.
-  AshTestHelper::InitParams init_params_;
+  std::unique_ptr<AshTestHelper::InitParams> init_params_ =
+      std::make_unique<AshTestHelper::InitParams>();
 
   // |task_environment_| is initialized-once at construction time but
   // subclasses may elect to provide their own.

ash/user_education/user_education_ash_test_base.cc

@@ -96,7 +96,8 @@ void UserEducationAshTestBase::SetUp() {
 
         return user_education_delegate;
       }));
-  NoSessionAshTestBase::SetUp(std::move(shell_delegate));
+  set_shell_delegate(std::move(shell_delegate));
+  NoSessionAshTestBase::SetUp();
 }
 
 }  // namespace ash

ash/wm/gestures/back_gesture/back_gesture_contextual_nudge_controller_impl_unittest.cc

@@ -62,7 +62,8 @@ class BackGestureContextualNudgeControllerTest : public NoSessionAshTestBase {
       delegate = std::make_unique<TestShellDelegate>();
       delegate->SetCanGoBack(false);
     }
-    NoSessionAshTestBase::SetUp(std::move(delegate));
+    set_shell_delegate(std::move(delegate));
+    NoSessionAshTestBase::SetUp();
 
     auto accountId1 = SimulateUserLogin({kUser1Email});
     user1_pref_service_ =

ash/wm/gestures/back_gesture/back_gesture_event_handler_unittest.cc

@@ -72,7 +72,8 @@ class BackGestureEventHandlerTest : public AshTestBase {
       delegate = std::make_unique<TestShellDelegate>();
       delegate->SetCanGoBack(false);
     }
-    AshTestBase::SetUp(std::move(delegate));
+    set_shell_delegate(std::move(delegate));
+    AshTestBase::SetUp();
 
     RecreateTopWindow(chromeos::AppType::BROWSER);
     TabletModeControllerTestApi().EnterTabletMode();

chrome/browser/ui/ash/global_media_controls/media_notification_provider_impl_unittest.cc

@@ -134,7 +134,8 @@ class MediaNotificationProviderImplTest : public ChromeAshTestBase {
   void SetUp() override {
     auto shell_delegate = std::make_unique<MediaTestShellDelegate>();
     shell_delegate_ = shell_delegate.get();
-    ChromeAshTestBase::SetUp(std::move(shell_delegate));
+    set_shell_delegate(std::move(shell_delegate));
+    ChromeAshTestBase::SetUp();
 
     crosapi_environment_.SetUp();
     provider_ = static_cast<MediaNotificationProviderImpl*>(

chrome/browser/ui/ash/multi_user/multi_profile_support_unittest.cc

@@ -304,7 +304,8 @@ void MultiProfileSupportTest::SetUp() {
   cros_settings_holder_ = std::make_unique<ash::CrosSettingsHolder>(
       ash::DeviceSettingsService::Get(),
       TestingBrowserProcess::GetGlobal()->local_state());
-  ChromeAshTestBase::SetUp(std::make_unique<TestShellDelegateChromeOS>());
+  set_shell_delegate(std::make_unique<TestShellDelegateChromeOS>());
+  ChromeAshTestBase::SetUp();
   GetSessionControllerClient()->set_pref_service_must_exist(true);
 
   profile_manager_ = std::make_unique<TestingProfileManager>(

chrome/test/base/chrome_ash_test_base.cc

@@ -9,7 +9,7 @@
 #include "content/public/test/browser_task_environment.h"
 
 ChromeAshTestBase::ChromeAshTestBase()
-    : ash::AshTestBase(std::unique_ptr<base::test::TaskEnvironment>(
+    : ChromeAshTestBase(std::unique_ptr<base::test::TaskEnvironment>(
           std::make_unique<content::BrowserTaskEnvironment>())) {}
 
 ChromeAshTestBase::~ChromeAshTestBase() = default;

components/exo/drag_drop_operation_unittest.cc

@@ -168,10 +168,8 @@ class DragDropOperationTestWithWebUITabStripTest
   void SetUp() override {
     auto mock_shell_delegate = std::make_unique<NiceMock<MockShellDelegate>>();
     mock_shell_delegate_ = mock_shell_delegate.get();
-
-    ExoTestBase::SetUp(std::move(mock_shell_delegate));
-    aura::client::GetDragDropClient(ash::Shell::GetPrimaryRootWindow())
-        ->AddObserver(this);
+    set_shell_delegate(std::move(mock_shell_delegate));
+    DragDropOperationTest::SetUp();
   }
 
   MockShellDelegate* mock_shell_delegate() { return mock_shell_delegate_; }

components/exo/test/exo_test_base.cc

@@ -51,7 +51,10 @@ ExoTestBase::ExoTestBase() = default;
 ExoTestBase::~ExoTestBase() = default;
 
 void ExoTestBase::SetUp() {
-  SetUp(nullptr);
+  AshTestBase::SetUp();
+  wm_helper_ = std::make_unique<WMHelper>();
+  wm_helper_->RegisterAppPropertyResolver(
+      base::WrapUnique(new TestPropertyResolver()));
 
   if (task_environment()->UsesMockTime()) {
     // Reduce the refresh rate to save cost for fast forwarding when mock time
@@ -65,14 +68,6 @@ void ExoTestBase::TearDown() {
   AshTestBase::TearDown();
 }
 
-void ExoTestBase::SetUp(
-    std::unique_ptr<ash::TestShellDelegate> shell_delegate) {
-  AshTestBase::SetUp(std::move(shell_delegate));
-  wm_helper_ = std::make_unique<WMHelper>();
-  wm_helper_->RegisterAppPropertyResolver(
-      base::WrapUnique(new TestPropertyResolver()));
-}
-
 viz::SurfaceManager* ExoTestBase::GetSurfaceManager() {
   return static_cast<ui::InProcessContextFactory*>(
              aura::Env::GetInstance()->context_factory())

components/exo/test/exo_test_base.h

@@ -10,10 +10,6 @@
 #include "ash/test/ash_test_base.h"
 #include "components/exo/test/exo_test_helper.h"
 
-namespace ash {
-class TestShellDelegate;
-}
-
 namespace viz {
 class SurfaceManager;
 }
@@ -44,8 +40,6 @@ class ExoTestBase : public ash::AshTestBase {
   void SetUp() override;
   void TearDown() override;
 
-  void SetUp(std::unique_ptr<ash::TestShellDelegate> shell_delegate);
-
   viz::SurfaceManager* GetSurfaceManager();
 
   gfx::Point GetOriginOfShellSurface(const ShellSurfaceBase* shell_surface);