[fyfre] Remove work in ProfileManagementFlowController constructor

Instead of initializing the initial step via the constructor and then
switching to it via `Init()`, made that function do both. This allows
removing any constraint to using virtual methods to register the initial
step.

Also includes some quality-of-life changes around the `SwitchToStep()`
API.

Bug: 1375277
Change-Id: I390f1ffb72b84b65502d9639b8ea91db54d23f65
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4043502
Reviewed-by: David Roger <droger@chromium.org>
Commit-Queue: Nicolas Dossou-Gbété <dgn@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1075777}

chrome/browser/ui/views/profiles/first_run_flow_controller_dice.cc

@@ -142,19 +142,9 @@ FirstRunFlowControllerDice::FirstRunFlowControllerDice(
     ClearHostClosure clear_host_callback,
     Profile* profile,
     ProfilePicker::FirstRunExitedCallback first_run_exited_callback)
-    : ProfileManagementFlowController(host,
-                                      std::move(clear_host_callback),
-                                      Step::kIntro),
+    : ProfileManagementFlowController(host, std::move(clear_host_callback)),
       profile_(profile),
-      first_run_exited_callback_(std::move(first_run_exited_callback)) {
-  RegisterStep(
-      initial_step(),
-      CreateIntroStep(host,
-                      base::BindRepeating(
-                          &FirstRunFlowControllerDice::HandleIntroSigninChoice,
-                          weak_ptr_factory_.GetWeakPtr()),
-                      /*enable_animations=*/true));
-}
+      first_run_exited_callback_(std::move(first_run_exited_callback)) {}
 
 FirstRunFlowControllerDice::~FirstRunFlowControllerDice() {
   if (first_run_exited_callback_) {
@@ -163,6 +153,19 @@ FirstRunFlowControllerDice::~FirstRunFlowControllerDice() {
   }
 }
 
+void FirstRunFlowControllerDice::Init(
+    StepSwitchFinishedCallback step_switch_finished_callback) {
+  RegisterStep(
+      Step::kIntro,
+      CreateIntroStep(host(),
+                      base::BindRepeating(
+                          &FirstRunFlowControllerDice::HandleIntroSigninChoice,
+                          weak_ptr_factory_.GetWeakPtr()),
+                      /*enable_animations=*/true));
+  SwitchToStep(Step::kIntro, /*reset_state=*/true,
+               std::move(step_switch_finished_callback));
+}
+
 bool FirstRunFlowControllerDice::PreFinishWithBrowser() {
   DCHECK(first_run_exited_callback_);
   std::move(first_run_exited_callback_)
@@ -187,12 +190,13 @@ void FirstRunFlowControllerDice::HandleIntroSigninChoice(bool sign_in) {
       // Binding as Unretained as `this` outlives the step
       // controllers.
       base::Unretained(this), ProfileManagementFlowController::Step::kIntro,
-      /*reset_state=*/false, /*pop_step_callback=*/base::OnceClosure(),
-      /*step_switch_finished_callback=*/base::OnceCallback<void(bool)>());
+      /*reset_state=*/false,
+      /*step_switch_finished_callback=*/StepSwitchFinishedCallback(),
+      /*pop_step_callback=*/base::OnceClosure());
   SwitchToStep(ProfileManagementFlowController::Step::kAccountSelection,
                /*reset_state=*/true,
-               /*pop_step_callback=*/std::move(pop_closure),
                /*step_switch_finished_callback=*/
                base::IgnoreArgs<bool>(base::BindOnce(
-                   &NavigateBackInOneSecond, weak_ptr_factory_.GetWeakPtr())));
+                   &NavigateBackInOneSecond, weak_ptr_factory_.GetWeakPtr())),
+               /*pop_step_callback=*/std::move(pop_closure));
 }

chrome/browser/ui/views/profiles/first_run_flow_controller_dice.h

@@ -33,8 +33,10 @@ class FirstRunFlowControllerDice : public ProfileManagementFlowController {
       ProfilePicker::FirstRunExitedCallback first_run_exited_callback);
   ~FirstRunFlowControllerDice() override;
 
- protected:
   // ProfileManagementFlowController:
+  void Init(StepSwitchFinishedCallback step_switch_finished_callback) override;
+
+ protected:
   bool PreFinishWithBrowser() override;
 
  private:

chrome/browser/ui/views/profiles/first_run_flow_controller_lacros.cc

@@ -4,6 +4,7 @@
 
 #include "chrome/browser/ui/views/profiles/first_run_flow_controller_lacros.h"
 
+#include "base/functional/callback.h"
 #include "base/logging.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/profiles/profile_manager.h"
@@ -131,12 +132,28 @@ FirstRunFlowControllerLacros::FirstRunFlowControllerLacros(
     ClearHostClosure clear_host_callback,
     Profile* profile,
     ProfilePicker::DebugFirstRunExitedCallback first_run_exited_callback)
-    : ProfileManagementFlowController(host,
-                                      std::move(clear_host_callback),
-                                      Step::kPostSignInFlow),
+    : ProfileManagementFlowController(host, std::move(clear_host_callback)),
+      profile_(profile),
       first_run_exited_callback_(std::move(first_run_exited_callback)) {
   DCHECK(first_run_exited_callback_);
+}
+
+FirstRunFlowControllerLacros::~FirstRunFlowControllerLacros() {
+  // Call the callback if not called yet. This happens when the user exits the
+  // flow by closing the window, or for intent overrides.
+  if (first_run_exited_callback_) {
+    std::move(first_run_exited_callback_)
+        .Run(sync_confirmation_seen_
+                 ? ProfilePicker::FirstRunExitStatus::kQuitAtEnd
+                 : ProfilePicker::FirstRunExitStatus::kQuitEarly,
+             ProfilePicker::FirstRunExitSource::kControllerDestructor);
+    // Since the flow is exited already, we don't have anything to close or
+    // finish setting up.
+  }
+}
 
+void FirstRunFlowControllerLacros::Init(
+    StepSwitchFinishedCallback step_switch_finished_callback) {
   auto mark_sync_confirmation_seen_callback =
       base::BindOnce(&FirstRunFlowControllerLacros::MarkSyncConfirmationSeen,
                      // Unretained ok: the callback is passed to a step that
@@ -149,30 +166,19 @@ FirstRunFlowControllerLacros::FirstRunFlowControllerLacros(
                      base::Unretained(this),
                      // Unretained ok: `signed_in_flow` will register a profile
                      // keep alive.
-                     base::Unretained(profile)));
+                     base::Unretained(profile_)));
   auto signed_in_flow = std::make_unique<LacrosFirstRunSignedInFlowController>(
-      host, profile,
-      content::WebContents::Create(content::WebContents::CreateParams(profile)),
+      host(), profile_,
+      content::WebContents::Create(
+          content::WebContents::CreateParams(profile_)),
       std::move(mark_sync_confirmation_seen_callback),
       std::move(finish_flow_callback));
 
-  RegisterStep(initial_step(),
+  RegisterStep(Step::kPostSignInFlow,
                ProfileManagementStepController::CreateForPostSignInFlow(
-                   host, std::move(signed_in_flow)));
-}
-
-FirstRunFlowControllerLacros::~FirstRunFlowControllerLacros() {
-  // Call the callback if not called yet. This happens when the user exits the
-  // flow by closing the window, or for intent overrides.
-  if (first_run_exited_callback_) {
-    std::move(first_run_exited_callback_)
-        .Run(sync_confirmation_seen_
-                 ? ProfilePicker::FirstRunExitStatus::kQuitAtEnd
-                 : ProfilePicker::FirstRunExitStatus::kQuitEarly,
-             ProfilePicker::FirstRunExitSource::kControllerDestructor);
-    // Since the flow is exited already, we don't have anything to close or
-    // finish setting up.
-  }
+                   host(), std::move(signed_in_flow)));
+  SwitchToStep(Step::kPostSignInFlow, /*reset_state=*/true,
+               std::move(step_switch_finished_callback));
 }
 
 bool FirstRunFlowControllerLacros::PreFinishWithBrowser() {

chrome/browser/ui/views/profiles/first_run_flow_controller_lacros.h

@@ -5,6 +5,7 @@
 #ifndef CHROME_BROWSER_UI_VIEWS_PROFILES_FIRST_RUN_FLOW_CONTROLLER_LACROS_H_
 #define CHROME_BROWSER_UI_VIEWS_PROFILES_FIRST_RUN_FLOW_CONTROLLER_LACROS_H_
 
+#include "base/memory/raw_ptr.h"
 #include "chrome/browser/ui/profile_picker.h"
 #include "chrome/browser/ui/views/profiles/profile_management_flow_controller.h"
 
@@ -26,13 +27,19 @@ class FirstRunFlowControllerLacros : public ProfileManagementFlowController {
 
   ~FirstRunFlowControllerLacros() override;
 
- protected:
   // ProfileManagementFlowController:
+  void Init(StepSwitchFinishedCallback step_switch_finished_callback) override;
+
+ protected:
   bool PreFinishWithBrowser() override;
 
  private:
   void MarkSyncConfirmationSeen();
 
+  // Pointer to the primary profile. Safe to keep, in particular we are going to
+  // register a profile keep alive through the step we create in `Init()`.
+  const raw_ptr<Profile> profile_;
+
   // Captures the operation that the user expected to run at the time we chose
   // to show them the FRE. When we exit the FRE, we MUST run this. We expect
   // that it will cause a UI for the primary profile to be opened.

chrome/browser/ui/views/profiles/profile_management_flow_controller.cc

@@ -17,27 +17,18 @@
 
 ProfileManagementFlowController::ProfileManagementFlowController(
     ProfilePickerWebContentsHost* host,
-    ClearHostClosure clear_host_callback,
-    Step initial_step)
-    : initial_step_(initial_step),
-      host_(host),
-      clear_host_callback_(std::move(clear_host_callback)) {}
-
-ProfileManagementFlowController::~ProfileManagementFlowController() = default;
-
-void ProfileManagementFlowController::Init(
-    base::OnceCallback<void(bool)> initial_step_switch_finished_callback) {
+    ClearHostClosure clear_host_callback)
+    : host_(host), clear_host_callback_(std::move(clear_host_callback)) {
   DCHECK(clear_host_callback_.value());
-  SwitchToStep(initial_step(), /*reset_state=*/true,
-               /*pop_step_callback=*/base::OnceClosure(),
-               std::move(initial_step_switch_finished_callback));
 }
 
+ProfileManagementFlowController::~ProfileManagementFlowController() = default;
+
 void ProfileManagementFlowController::SwitchToStep(
     Step step,
     bool reset_state,
-    base::OnceClosure pop_step_callback,
-    base::OnceCallback<void(bool)> step_switch_finished_callback) {
+    StepSwitchFinishedCallback step_switch_finished_callback,
+    base::OnceClosure pop_step_callback) {
   DCHECK_NE(Step::kUnknown, step);
   DCHECK_NE(current_step_, step);
 

chrome/browser/ui/views/profiles/profile_management_flow_controller.h

@@ -21,9 +21,9 @@ class ProfilePickerWebContentsHost;
 // `ProfileManagementStepController`s and owned by this object.
 //
 // Typical usage starts with calling `Init()` on the instantiated flow, which
-// will switch to the `initial_step()`. Then as the user interacts with the
-// flow, this controller will handle instantiating and navigating between the
-// steps.
+// will register and switch to the first step. Then as the user interacts with
+// the flow, this controller will handle instantiating and navigating between
+// the next steps.
 class ProfileManagementFlowController {
  public:
   // TODO(https://crbug.com/1358843): Split the steps more granularly across
@@ -52,33 +52,30 @@ class ProfileManagementFlowController {
     kIntro,
   };
 
-  // Creates a flow controller that will advance to `initial_step` when it is
-  // `Init()`-ed.
+  // Creates a flow controller that will start showing UI when `Init()`-ed.
   // `clear_host_callback` will be called if `host` needs to be closed.
-  explicit ProfileManagementFlowController(ProfilePickerWebContentsHost* host,
-                                           ClearHostClosure clear_host_callback,
-                                           Step initial_step);
+  explicit ProfileManagementFlowController(
+      ProfilePickerWebContentsHost* host,
+      ClearHostClosure clear_host_callback);
   virtual ~ProfileManagementFlowController();
 
-  // Switches to the `initial_step()`.
-  // If `initial_step_switch_finished_callback` is provided, it will be called
-  // with `true` when the navigation to the initial step succeeded, or with
-  // `false` otherwise.
-  virtual void Init(
-      base::OnceCallback<void(bool)> initial_step_switch_finished_callback =
-          base::OnceCallback<void(bool success)>());
+  // Starts the flow by registering and switching to the first step.
+  // If `step_switch_finished_callback` is provided, it will be called with
+  // `true` when the navigation to the initial step succeeded, or with `false`
+  // otherwise.
+  virtual void Init(StepSwitchFinishedCallback step_switch_finished_callback =
+                        StepSwitchFinishedCallback()) = 0;
 
   // Instructs a step registered as `step` to be shown.
   // If `step_switch_finished_callback` is provided, it will be called
   // with `true` when the navigation to `step` succeeded, or with
   // `false` otherwise.
   // Also see `ProfileManagementStepController::Show()`.
-  void SwitchToStep(
-      Step step,
-      bool reset_state,
-      base::OnceClosure pop_step_callback = base::OnceClosure(),
-      base::OnceCallback<void(bool)> step_switch_finished_callback =
-          base::OnceCallback<void(bool success)>());
+  void SwitchToStep(Step step,
+                    bool reset_state,
+                    StepSwitchFinishedCallback step_switch_finished_callback =
+                        StepSwitchFinishedCallback(),
+                    base::OnceClosure pop_step_callback = base::OnceClosure());
 
   void OnNavigateBackRequested();
 
@@ -115,15 +112,11 @@ class ProfileManagementFlowController {
 
   Step current_step() const { return current_step_; }
 
-  Step initial_step() const { return initial_step_; }
-
   ProfilePickerWebContentsHost* host() { return host_; }
 
  private:
   Step current_step_ = Step::kUnknown;
 
-  Step initial_step_;
-
   raw_ptr<ProfilePickerWebContentsHost> host_;
   ClearHostClosure clear_host_callback_;
 

chrome/browser/ui/views/profiles/profile_management_step_controller.cc

@@ -80,7 +80,7 @@ class DiceSignInStepController : public ProfileManagementStepController {
 
   ~DiceSignInStepController() override = default;
 
-  void Show(base::OnceCallback<void(bool)> step_shown_callback,
+  void Show(StepSwitchFinishedCallback step_shown_callback,
             bool reset_state) override {
     DCHECK(step_shown_callback);
     DCHECK(signed_in_callback_) << "Attempting to show Dice step again while "

chrome/browser/ui/views/profiles/profile_management_step_controller.h

@@ -68,7 +68,7 @@ class ProfileManagementStepController {
   // `reset_state` indicates that the step should reset its internal state and
   // appear as freshly created. Callers should pass `true` for newly created
   // steps.
-  virtual void Show(base::OnceCallback<void(bool success)> step_shown_callback,
+  virtual void Show(StepSwitchFinishedCallback step_shown_callback,
                     bool reset_state = false) = 0;
 
   // Frees up unneeded resources. `Show()` will be called if it's needed again.

chrome/browser/ui/views/profiles/profile_management_utils.h

@@ -14,6 +14,13 @@
 class Browser;
 class Profile;
 
+// Type of the callbacks that are called to be notified that the switch to a
+// given step by `ProfileManagementFlowController` is completed. `success` is
+// is set to false if some sort of error is detected, and `true` otherwise.
+// This type is intended for documentation purposes, there is no plan to treat
+// it like an opaque type.
+using StepSwitchFinishedCallback = base::OnceCallback<void(bool success)>;
+
 // Callback executed when the flow finishes, after the host was cleared and
 // we opened a browser for the newly set up profile.
 // This callback should not rely on profile management flow instances, as we

chrome/browser/ui/views/profiles/profile_picker_flow_controller.cc

@@ -256,21 +256,24 @@ ProfilePickerFlowController::ProfilePickerFlowController(
     ProfilePickerWebContentsHost* host,
     ClearHostClosure clear_host_callback,
     ProfilePicker::EntryPoint entry_point)
-    : ProfileManagementFlowController(host,
-                                      std::move(clear_host_callback),
-                                      Step::kProfilePicker),
-      entry_point_(entry_point) {
-  RegisterStep(initial_step(),
-               ProfileManagementStepController::CreateForProfilePickerApp(
-                   host, GetInitialURL(entry_point_)));
-}
+    : ProfileManagementFlowController(host, std::move(clear_host_callback)),
+      entry_point_(entry_point) {}
 
 ProfilePickerFlowController::~ProfilePickerFlowController() = default;
 
+void ProfilePickerFlowController::Init(
+    StepSwitchFinishedCallback step_switch_finished_callback) {
+  RegisterStep(Step::kProfilePicker,
+               ProfileManagementStepController::CreateForProfilePickerApp(
+                   host(), GetInitialURL(entry_point_)));
+  SwitchToStep(Step::kProfilePicker, /*reset_state=*/true,
+               std::move(step_switch_finished_callback));
+}
+
 #if BUILDFLAG(ENABLE_DICE_SUPPORT)
 void ProfilePickerFlowController::SwitchToDiceSignIn(
     absl::optional<SkColor> profile_color,
-    base::OnceCallback<void(bool)> switch_finished_callback) {
+    StepSwitchFinishedCallback switch_finished_callback) {
   DCHECK_EQ(Step::kProfilePicker, current_step());
 
   profile_color_ = profile_color;
@@ -289,11 +292,12 @@ void ProfilePickerFlowController::SwitchToDiceSignIn(
       &ProfilePickerFlowController::SwitchToStep,
       // Unretained ok:`this` outlives the step controllers.
       base::Unretained(this), Step::kProfilePicker,
-      /*reset_state=*/false, /*pop_step_callback=*/base::OnceClosure(),
-      /*step_switch_finished_callback=*/base::OnceCallback<void(bool)>());
+      /*reset_state=*/false,
+      /*step_switch_finished_callback=*/StepSwitchFinishedCallback(),
+      /*pop_step_callback=*/base::OnceClosure());
   SwitchToStep(Step::kAccountSelection,
-               /*reset_state=*/step_needs_registration, std::move(pop_closure),
-               std::move(switch_finished_callback));
+               /*reset_state=*/step_needs_registration,
+               std::move(switch_finished_callback), std::move(pop_closure));
 }
 #endif
 

chrome/browser/ui/views/profiles/profile_picker_flow_controller.h

@@ -21,9 +21,10 @@ class ProfilePickerFlowController : public ProfileManagementFlowController {
                               ProfilePicker::EntryPoint entry_point);
   ~ProfilePickerFlowController() override;
 
-  void SwitchToDiceSignIn(
-      absl::optional<SkColor> profile_color,
-      base::OnceCallback<void(bool)> switch_finished_callback);
+  void Init(StepSwitchFinishedCallback step_switch_finished_callback) override;
+
+  void SwitchToDiceSignIn(absl::optional<SkColor> profile_color,
+                          StepSwitchFinishedCallback switch_finished_callback);
 
   void SwitchToPostSignIn(Profile* signed_in_profile,
 #if BUILDFLAG(ENABLE_DICE_SUPPORT)