[OOBE][AutoEnrollmentCheck]: Extract screen skip logic to MaybeSkip

Also remove OnViewDestroyed logic together with delegate subclass from AutoEnrollmentCheckScreenView. Bug: 1315218, 1309022 Change-Id: Ic6d870067cb6afde6734fe8bda1260a7a3534f33 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3582207 Reviewed-by: Roman Sorokin <[email protected]> Commit-Queue: Danila Kuzmin <[email protected]> Cr-Commit-Position: refs/heads/main@{#1018609}
hadissulejmani · Jun 28, 2022 · b0e90bf · b0e90bf
1 parent 79d0a31
commit b0e90bf
Show file tree

Hide file tree

Showing 10 changed files with 181 additions and 206 deletions.
diff --git a/chrome/browser/ash/login/enrollment/auto_enrollment_check_screen.cc b/chrome/browser/ash/login/enrollment/auto_enrollment_check_screen.cc
@@ -29,28 +29,33 @@ NetworkPortalDetector::CaptivePortalStatus GetCaptivePortalStatus() {
 
 }  // namespace
 
+// static
+std::string AutoEnrollmentCheckScreen::GetResultString(Result result) {
+  switch (result) {
+    case Result::NEXT:
+      return "Next";
+    case Result::NOT_APPLICABLE:
+      return BaseScreen::kNotApplicable;
+  }
+}
+
 AutoEnrollmentCheckScreen::AutoEnrollmentCheckScreen(
-    AutoEnrollmentCheckScreenView* view,
+    base::WeakPtr<AutoEnrollmentCheckScreenView> view,
     ErrorScreen* error_screen,
-    const base::RepeatingClosure& exit_callback)
+    const base::RepeatingCallback<void(Result result)>& exit_callback)
     : BaseScreen(AutoEnrollmentCheckScreenView::kScreenId,
                  OobeScreenPriority::DEFAULT),
-      view_(view),
+      view_(std::move(view)),
       error_screen_(error_screen),
       exit_callback_(exit_callback),
       auto_enrollment_controller_(nullptr),
       captive_portal_status_(
           NetworkPortalDetector::CAPTIVE_PORTAL_STATUS_UNKNOWN),
       auto_enrollment_state_(policy::AUTO_ENROLLMENT_STATE_IDLE),
-      histogram_helper_(new ErrorScreensHistogramHelper("Enrollment")) {
-  if (view_)
-    view_->SetDelegate(this);
-}
+      histogram_helper_(new ErrorScreensHistogramHelper("Enrollment")) {}
 
 AutoEnrollmentCheckScreen::~AutoEnrollmentCheckScreen() {
   network_portal_detector::GetInstance()->RemoveObserver(this);
-  if (view_)
-    view_->SetDelegate(NULL);
 }
 
 void AutoEnrollmentCheckScreen::ClearState() {
@@ -63,19 +68,14 @@ void AutoEnrollmentCheckScreen::ClearState() {
 }
 
 void AutoEnrollmentCheckScreen::ShowImpl() {
-  // If the decision got made already, don't show the screen at all.
-  if (!policy::AutoEnrollmentTypeChecker::IsEnabled() || IsCompleted()) {
-    SignalCompletion();
-    return;
-  }
-
   // Start from a clean slate.
   ClearState();
 
   // Bring up the screen. It's important to do this before updating the UI,
   // because the latter may switch to the error screen, which needs to stay on
   // top.
-  view_->Show();
+  if (view_)
+    view_->Show();
   histogram_helper_->OnScreenShow();
 
   // Set up state change observers.
@@ -116,12 +116,17 @@ void AutoEnrollmentCheckScreen::ShowImpl() {
   network_portal_detector::GetInstance()->StartPortalDetection();
 }
 
-void AutoEnrollmentCheckScreen::HideImpl() {}
+void AutoEnrollmentCheckScreen::HideImpl() {
+  network_portal_detector::GetInstance()->RemoveObserver(this);
+}
 
-void AutoEnrollmentCheckScreen::OnViewDestroyed(
-    AutoEnrollmentCheckScreenView* view) {
-  if (view_ == view)
-    view_ = nullptr;
+bool AutoEnrollmentCheckScreen::MaybeSkip(WizardContext* context) {
+  // If the decision got made already, don't show the screen at all.
+  if (!policy::AutoEnrollmentTypeChecker::IsEnabled() || IsCompleted()) {
+    RunExitCallback(Result::NOT_APPLICABLE);
+    return true;
+  }
+  return false;
 }
 
 void AutoEnrollmentCheckScreen::OnPortalDetectionCompleted(
@@ -268,7 +273,7 @@ void AutoEnrollmentCheckScreen::SignalCompletion() {
   // finish their work before.
   base::ThreadTaskRunnerHandle::Get()->PostTask(
       FROM_HERE, base::BindOnce(&AutoEnrollmentCheckScreen::RunExitCallback,
-                                weak_ptr_factory_.GetWeakPtr()));
+                                weak_ptr_factory_.GetWeakPtr(), Result::NEXT));
 }
 
 bool AutoEnrollmentCheckScreen::IsCompleted() const {

diff --git a/chrome/browser/ash/login/enrollment/auto_enrollment_check_screen.h b/chrome/browser/ash/login/enrollment/auto_enrollment_check_screen.h
@@ -8,6 +8,7 @@
 #include <memory>
 
 #include "base/callback.h"
+#include "base/memory/raw_ptr.h"
 #include "base/memory/weak_ptr.h"
 #include "chrome/browser/ash/login/enrollment/auto_enrollment_check_screen_view.h"
 #include "chrome/browser/ash/login/enrollment/auto_enrollment_controller.h"
@@ -25,23 +26,28 @@ namespace ash {
 // keeping track of current auto-enrollment state and displaying and updating
 // the error screen upon failures. Similar to a screen controller, but it
 // doesn't actually drive a dedicated screen.
-class AutoEnrollmentCheckScreen
-    : public AutoEnrollmentCheckScreenView::Delegate,
-      public BaseScreen,
-      public NetworkPortalDetector::Observer {
+class AutoEnrollmentCheckScreen : public BaseScreen,
+                                  public NetworkPortalDetector::Observer {
  public:
+  enum class Result {
+    NEXT,
+    NOT_APPLICABLE,
+  };
   using TView = AutoEnrollmentCheckScreenView;
 
-  AutoEnrollmentCheckScreen(AutoEnrollmentCheckScreenView* view,
-                            ErrorScreen* error_screen,
-                            const base::RepeatingClosure& exit_callback);
+  AutoEnrollmentCheckScreen(
+      base::WeakPtr<AutoEnrollmentCheckScreenView> view,
+      ErrorScreen* error_screen,
+      const base::RepeatingCallback<void(Result result)>& exit_callback);
 
   AutoEnrollmentCheckScreen(const AutoEnrollmentCheckScreen&) = delete;
   AutoEnrollmentCheckScreen& operator=(const AutoEnrollmentCheckScreen&) =
       delete;
 
   ~AutoEnrollmentCheckScreen() override;
 
+  static std::string GetResultString(Result result);
+
   // Clears the cached state causing the forced enrollment check to be retried.
   void ClearState();
 
@@ -50,13 +56,11 @@ class AutoEnrollmentCheckScreen
     auto_enrollment_controller_ = auto_enrollment_controller;
   }
 
-  void set_exit_callback_for_testing(const base::RepeatingClosure& callback) {
+  void set_exit_callback_for_testing(
+      const base::RepeatingCallback<void(Result result)>& callback) {
     exit_callback_ = callback;
   }
 
-  // AutoEnrollmentCheckScreenView::Delegate implementation:
-  void OnViewDestroyed(AutoEnrollmentCheckScreenView* view) override;
-
   // NetworkPortalDetector::Observer implementation:
   void OnPortalDetectionCompleted(
       const NetworkState* network,
@@ -66,11 +70,12 @@ class AutoEnrollmentCheckScreen
   // BaseScreen:
   void ShowImpl() override;
   void HideImpl() override;
+  bool MaybeSkip(WizardContext* context) override;
 
   // Runs `exit_callback_` - used to prevent `exit_callback_` from running after
   // `this` has been destroyed (by wrapping it with a callback bound to a weak
   // ptr).
-  void RunExitCallback() { exit_callback_.Run(); }
+  void RunExitCallback(Result result) { exit_callback_.Run(result); }
 
  private:
   // Handles update notifications regarding the auto-enrollment check.
@@ -113,10 +118,10 @@ class AutoEnrollmentCheckScreen
   // necessary".
   bool ShouldBlockOnServerError() const;
 
-  AutoEnrollmentCheckScreenView* view_;
+  base::WeakPtr<AutoEnrollmentCheckScreenView> view_;
   ErrorScreen* error_screen_;
-  base::RepeatingClosure exit_callback_;
-  AutoEnrollmentController* auto_enrollment_controller_;
+  base::RepeatingCallback<void(Result result)> exit_callback_;
+  base::raw_ptr<AutoEnrollmentController> auto_enrollment_controller_ = nullptr;
 
   base::CallbackListSubscription auto_enrollment_progress_subscription_;
 

diff --git a/chrome/browser/ash/login/enrollment/auto_enrollment_check_screen_view.h b/chrome/browser/ash/login/enrollment/auto_enrollment_check_screen_view.h
@@ -5,30 +5,20 @@
 #ifndef CHROME_BROWSER_ASH_LOGIN_ENROLLMENT_AUTO_ENROLLMENT_CHECK_SCREEN_VIEW_H_
 #define CHROME_BROWSER_ASH_LOGIN_ENROLLMENT_AUTO_ENROLLMENT_CHECK_SCREEN_VIEW_H_
 
+#include "base/memory/weak_ptr.h"
 #include "chrome/browser/ash/login/oobe_screen.h"
 
 namespace ash {
 
 // Interface between auto-enrollment check screen and its representation.
-// Note, do not forget to call OnViewDestroyed in the dtor.
-class AutoEnrollmentCheckScreenView {
+class AutoEnrollmentCheckScreenView
+    : public base::SupportsWeakPtr<AutoEnrollmentCheckScreenView> {
  public:
-  // Allows us to get info from auto-enrollment check screen that we need.
-  class Delegate {
-   public:
-    virtual ~Delegate() {}
-
-    // This method is called, when view is being destroyed. Note, if Delegate
-    // is destroyed earlier then it has to call SetDelegate(NULL).
-    virtual void OnViewDestroyed(AutoEnrollmentCheckScreenView* view) = 0;
-  };
-
   constexpr static StaticOobeScreenId kScreenId{"auto-enrollment-check"};
 
   virtual ~AutoEnrollmentCheckScreenView() {}
 
   virtual void Show() = 0;
-  virtual void SetDelegate(Delegate* delegate) = 0;
 };
 
 }  // namespace ash

diff --git a/chrome/browser/ash/login/enrollment/mock_auto_enrollment_check_screen.cc b/chrome/browser/ash/login/enrollment/mock_auto_enrollment_check_screen.cc
@@ -7,10 +7,10 @@
 namespace ash {
 
 MockAutoEnrollmentCheckScreen::MockAutoEnrollmentCheckScreen(
-    AutoEnrollmentCheckScreenView* view,
+    base::WeakPtr<AutoEnrollmentCheckScreenView> view,
     ErrorScreen* error_screen,
-    const base::RepeatingClosure& exit_callback)
-    : AutoEnrollmentCheckScreen(view, error_screen, exit_callback) {}
+    const base::RepeatingCallback<void(Result result)>& exit_callback)
+    : AutoEnrollmentCheckScreen(std::move(view), error_screen, exit_callback) {}
 
 MockAutoEnrollmentCheckScreen::~MockAutoEnrollmentCheckScreen() {}
 
@@ -19,20 +19,13 @@ void MockAutoEnrollmentCheckScreen::RealShow() {
 }
 
 void MockAutoEnrollmentCheckScreen::ExitScreen() {
-  RunExitCallback();
+  RunExitCallback(Result::NEXT);
 }
 
 MockAutoEnrollmentCheckScreenView::MockAutoEnrollmentCheckScreenView() =
     default;
 
-MockAutoEnrollmentCheckScreenView::~MockAutoEnrollmentCheckScreenView() {
-  if (screen_)
-    screen_->OnViewDestroyed(this);
-}
-
-void MockAutoEnrollmentCheckScreenView::SetDelegate(Delegate* screen) {
-  screen_ = screen;
-  MockSetDelegate(screen);
-}
+MockAutoEnrollmentCheckScreenView::~MockAutoEnrollmentCheckScreenView() =
+    default;
 
 }  // namespace ash
diff --git a/chrome/browser/ash/login/enrollment/mock_auto_enrollment_check_screen.h b/chrome/browser/ash/login/enrollment/mock_auto_enrollment_check_screen.h
@@ -13,9 +13,10 @@ namespace ash {
 
 class MockAutoEnrollmentCheckScreen : public AutoEnrollmentCheckScreen {
  public:
-  MockAutoEnrollmentCheckScreen(AutoEnrollmentCheckScreenView* view,
-                                ErrorScreen* error_screen,
-                                const base::RepeatingClosure& exit_callback);
+  MockAutoEnrollmentCheckScreen(
+      base::WeakPtr<AutoEnrollmentCheckScreenView> view,
+      ErrorScreen* error_screen,
+      const base::RepeatingCallback<void(Result result)>& exit_callback);
   ~MockAutoEnrollmentCheckScreen() override;
 
   MOCK_METHOD(void, ShowImpl, ());
@@ -30,13 +31,7 @@ class MockAutoEnrollmentCheckScreenView : public AutoEnrollmentCheckScreenView {
   MockAutoEnrollmentCheckScreenView();
   ~MockAutoEnrollmentCheckScreenView() override;
 
-  void SetDelegate(Delegate* screen) override;
-
-  MOCK_METHOD(void, MockSetDelegate, (Delegate * screen));
   MOCK_METHOD(void, Show, ());
-
- private:
-  Delegate* screen_ = nullptr;
 };
 
 }  // namespace ash

diff --git a/chrome/browser/ash/login/wizard_controller.cc b/chrome/browser/ash/login/wizard_controller.cc
@@ -659,7 +659,7 @@ WizardController::CreateScreens() {
   }
 
   append(std::make_unique<AutoEnrollmentCheckScreen>(
-      oobe_ui->GetView<AutoEnrollmentCheckScreenHandler>(),
+      oobe_ui->GetView<AutoEnrollmentCheckScreenHandler>()->AsWeakPtr(),
       oobe_ui->GetErrorScreen(),
       base::BindRepeating(&WizardController::OnAutoEnrollmentCheckScreenExit,
                           weak_factory_.GetWeakPtr())));
@@ -1514,8 +1514,10 @@ void WizardController::OnUpdateCompleted() {
   ShowAutoEnrollmentCheckScreen();
 }
 
-void WizardController::OnAutoEnrollmentCheckScreenExit() {
-  OnScreenExit(AutoEnrollmentCheckScreenView::kScreenId, kDefaultExitReason);
+void WizardController::OnAutoEnrollmentCheckScreenExit(
+    AutoEnrollmentCheckScreen::Result result) {
+  OnScreenExit(AutoEnrollmentCheckScreenView::kScreenId,
+               AutoEnrollmentCheckScreen::GetResultString(result));
   VLOG(1) << "WizardController::OnAutoEnrollmentCheckScreenExit()";
   // Check whether the device is disabled. OnDeviceDisabledChecked() will be
   // invoked when the result of this check is known. Until then, the current

diff --git a/chrome/browser/ash/login/wizard_controller.h b/chrome/browser/ash/login/wizard_controller.h
@@ -17,6 +17,7 @@
 #include "base/time/time.h"
 #include "chrome/browser/ash/accessibility/accessibility_manager.h"
 #include "chrome/browser/ash/login/demo_mode/demo_session.h"
+#include "chrome/browser/ash/login/enrollment/auto_enrollment_check_screen.h"
 #include "chrome/browser/ash/login/enrollment/auto_enrollment_controller.h"
 #include "chrome/browser/ash/login/enrollment/enrollment_screen.h"
 #include "chrome/browser/ash/login/oobe_screen.h"
@@ -326,7 +327,8 @@ class WizardController : public OobeUI::Observer {
   void OnEulaAccepted(bool usage_statistics_reporting_enabled);
   void OnUpdateScreenExit(UpdateScreen::Result result);
   void OnUpdateCompleted();
-  void OnAutoEnrollmentCheckScreenExit();
+  void OnAutoEnrollmentCheckScreenExit(
+      AutoEnrollmentCheckScreen::Result result);
   void OnEnrollmentScreenExit(EnrollmentScreen::Result result);
   void OnEnrollmentDone();
   void OnEnableAdbSideloadingScreenExit();