BaseScreen: Create new method to skip screens.

Create new method which helps to move showing condition from
wizard_controller to dedicated screen classes.

Bug: 1064561
Change-Id: I6f48ff86b959b91f3330e6e4d7512ced5061186b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2120500
Commit-Queue: Roman Aleksandrov <raleksandrov@google.com>
Reviewed-by: Denis Kuznetsov [CET] <antrim@chromium.org>
Cr-Commit-Position: refs/heads/master@{#756554}

chrome/browser/chromeos/login/screens/base_screen.cc

@@ -8,6 +8,8 @@
 
 namespace chromeos {
 
+constexpr char BaseScreen::kNotApplicable[];
+
 BaseScreen::BaseScreen(OobeScreenId screen_id,
                        OobeScreenPriority screen_priority)
     : screen_id_(screen_id), screen_priority_(screen_priority) {}
@@ -24,6 +26,14 @@ void BaseScreen::Hide() {
   is_hidden_ = true;
 }
 
+bool BaseScreen::ShouldSkipScreen() {
+  return false;
+}
+
+void BaseScreen::Skip() {
+  NOTREACHED() << "Skip methog should be overriden along with ShouldSkipScreen";
+}
+
 void BaseScreen::HandleUserAction(const std::string& action_id) {
   if (is_hidden_) {
     LOG(WARNING) << "User action came when screen is hidden: action_id="

chrome/browser/chromeos/login/screens/base_screen.h

@@ -21,6 +21,10 @@ namespace chromeos {
 // method called just once.
 class BaseScreen {
  public:
+  // String which represents not applicable exit code. This exit code refers to
+  // skipping the screen due to specific unmet condition.
+  constexpr static const char kNotApplicable[] = "NotApplicable";
+
   BaseScreen(OobeScreenId screen_id, OobeScreenPriority screen_priority);
   virtual ~BaseScreen();
 
@@ -30,6 +34,13 @@ class BaseScreen {
   // Makes wizard screen invisible.
   void Hide();
 
+  // Returns whether the screen should be skipped i. e. should be exited due to
+  // specific unmet conditions. Override along with Skip method.
+  virtual bool ShouldSkipScreen();
+
+  // Skips the screen. Override along with ShouldSkipScreen method.
+  virtual void Skip();
+
   // Forwards user action if screen is shown.
   void HandleUserAction(const std::string& action_id);
 

chrome/browser/chromeos/login/screens/recommend_apps_screen.cc

@@ -5,7 +5,10 @@
 #include "chrome/browser/chromeos/login/screens/recommend_apps_screen.h"
 
 #include "chrome/browser/chromeos/login/screens/recommend_apps/recommend_apps_fetcher.h"
+#include "chrome/browser/policy/profile_policy_connector.h"
+#include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/ui/webui/chromeos/login/recommend_apps_screen_handler.h"
+#include "components/user_manager/user_manager.h"
 
 namespace chromeos {
 
@@ -16,6 +19,8 @@ std::string RecommendAppsScreen::GetResultString(Result result) {
       return "Selected";
     case Result::SKIPPED:
       return "Skipped";
+    case Result::NOT_APPLICABLE:
+      return BaseScreen::kNotApplicable;
   }
 }
 
@@ -53,6 +58,22 @@ void RecommendAppsScreen::OnViewDestroyed(RecommendAppsScreenView* view) {
   view_ = nullptr;
 }
 
+bool RecommendAppsScreen::ShouldSkipScreen() {
+  const user_manager::UserManager* user_manager =
+      user_manager::UserManager::Get();
+  DCHECK(user_manager->IsUserLoggedIn());
+  bool is_managed_account = ProfileManager::GetActiveUserProfile()
+                                ->GetProfilePolicyConnector()
+                                ->IsManaged();
+  bool is_child_account = user_manager->IsLoggedInAsChildUser();
+  return is_managed_account || is_child_account;
+}
+
+void RecommendAppsScreen::Skip() {
+  DCHECK(ShouldSkipScreen());
+  exit_callback_.Run(Result::NOT_APPLICABLE);
+}
+
 void RecommendAppsScreen::ShowImpl() {
   view_->Show();
 

chrome/browser/chromeos/login/screens/recommend_apps_screen.h

@@ -27,7 +27,7 @@ class RecommendAppsScreenView;
 class RecommendAppsScreen : public BaseScreen,
                             public RecommendAppsFetcherDelegate {
  public:
-  enum class Result { SELECTED, SKIPPED };
+  enum class Result { SELECTED, SKIPPED, NOT_APPLICABLE };
 
   static std::string GetResultString(Result result);
 
@@ -53,6 +53,10 @@ class RecommendAppsScreen : public BaseScreen,
   void OnLoadError() override;
   void OnParseResponseError() override;
 
+  // BaseScreen:
+  bool ShouldSkipScreen() override;
+  void Skip() override;
+
  private:
   // BaseScreen:
   void ShowImpl() override;

chrome/browser/chromeos/login/screens/recommend_apps_screen_browsertest.cc

@@ -21,6 +21,7 @@
 #include "chrome/browser/chromeos/login/test/oobe_screen_waiter.h"
 #include "chrome/browser/chromeos/login/ui/login_display_host.h"
 #include "chrome/browser/chromeos/login/wizard_controller.h"
+#include "chrome/browser/policy/profile_policy_connector.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/ui/webui/chromeos/login/oobe_ui.h"
@@ -132,6 +133,12 @@ class RecommendAppsScreenTest : public InProcessBrowserTest {
 
     InProcessBrowserTest::SetUpOnMainThread();
   }
+
+  void ShowRecommendAppsScreen() {
+    WizardController::default_controller()->AdvanceToScreen(
+        RecommendAppsScreenView::kScreenId);
+  }
+
   void TearDownOnMainThread() override {
     recommend_apps_fetcher_ = nullptr;
     recommend_apps_fetcher_factory_.reset();
@@ -202,6 +209,10 @@ class RecommendAppsScreenTest : public InProcessBrowserTest {
            result;
   }
 
+  policy::ProfilePolicyConnector* GetProfilePolicyConnector() {
+    return ProfileManager::GetActiveUserProfile()->GetProfilePolicyConnector();
+  }
+
   RecommendAppsScreen* recommend_apps_screen_;
   base::Optional<RecommendAppsScreen::Result> screen_result_;
   FakeRecommendAppsFetcher* recommend_apps_fetcher_ = nullptr;
@@ -743,4 +754,13 @@ IN_PROC_BROWSER_TEST_F(RecommendAppsScreenTest, RetryOnLoadError) {
   EXPECT_EQ(base::Value(base::Value::Type::LIST), *fast_reinstall_packages);
 }
 
+IN_PROC_BROWSER_TEST_F(RecommendAppsScreenTest, SkipDueToManagedUser) {
+  GetProfilePolicyConnector()->OverrideIsManagedForTesting(true);
+
+  ShowRecommendAppsScreen();
+  WaitForScreenExit();
+  EXPECT_EQ(screen_result_.value(),
+            RecommendAppsScreen::Result::NOT_APPLICABLE);
+}
+
 }  // namespace chromeos

chrome/browser/chromeos/login/wizard_controller.cc

@@ -52,6 +52,7 @@
 #include "chrome/browser/chromeos/login/screens/app_downloading_screen.h"
 #include "chrome/browser/chromeos/login/screens/arc_terms_of_service_screen.h"
 #include "chrome/browser/chromeos/login/screens/assistant_optin_flow_screen.h"
+#include "chrome/browser/chromeos/login/screens/base_screen.h"
 #include "chrome/browser/chromeos/login/screens/demo_preferences_screen.h"
 #include "chrome/browser/chromeos/login/screens/demo_setup_screen.h"
 #include "chrome/browser/chromeos/login/screens/device_disabled_screen.h"
@@ -301,19 +302,6 @@ bool IsRemoraRequisition() {
   return policy_manager && policy_manager->IsRemoraRequisition();
 }
 
-// Return false if the logged in user is a managed or child account. Otherwise,
-// return true if the feature flag for recommend app screen is on.
-bool ShouldShowRecommendAppsScreen() {
-  const user_manager::UserManager* user_manager =
-      user_manager::UserManager::Get();
-  DCHECK(user_manager->IsUserLoggedIn());
-  bool is_managed_account = ProfileManager::GetActiveUserProfile()
-                                ->GetProfilePolicyConnector()
-                                ->IsManaged();
-  bool is_child_account = user_manager->IsLoggedInAsChildUser();
-  return !is_managed_account && !is_child_account;
-}
-
 chromeos::LoginDisplayHost* GetLoginDisplayHost() {
   return chromeos::LoginDisplayHost::default_host();
 }
@@ -808,10 +796,12 @@ void WizardController::SkipUpdateEnrollAfterEula() {
 
 void WizardController::OnScreenExit(OobeScreenId screen,
                                     const std::string& exit_reason) {
-  DCHECK(current_screen_->screen_id() == screen);
-
   VLOG(1) << "Wizard screen " << screen
           << " exited with reason: " << exit_reason;
+  // Do not perform checks and record stats for the skipped screen.
+  if (exit_reason == chromeos::BaseScreen::kNotApplicable)
+    return;
+  DCHECK(current_screen_->screen_id() == screen);
 
   RecordUMAHistogramForOOBEStepCompletionTime(
       screen, exit_reason, base::Time::Now() - screen_show_times_[screen]);
@@ -1172,16 +1162,7 @@ void WizardController::OnArcTermsOfServiceAccepted() {
     }
     return;
   }
-
-  // If the recommend app screen should be shown, show it after the user
-  // accepted the Arc TOS. Otherwise, advance to the assistant opt-in flow
-  // screen.
-  if (ShouldShowRecommendAppsScreen()) {
-    ShowRecommendAppsScreen();
-    return;
-  }
-
-  ShowAssistantOptInFlowScreen();
+  ShowRecommendAppsScreen();
 }
 
 void WizardController::OnRecommendAppsScreenExit(
@@ -1194,6 +1175,7 @@ void WizardController::OnRecommendAppsScreenExit(
       ShowAppDownloadingScreen();
       break;
     case RecommendAppsScreen::Result::SKIPPED:
+    case RecommendAppsScreen::Result::NOT_APPLICABLE:
       ShowAssistantOptInFlowScreen();
       break;
   }
@@ -1427,6 +1409,11 @@ void WizardController::SetCurrentScreen(BaseScreen* new_current) {
   if (current_screen_ == new_current || GetOobeUI() == nullptr)
     return;
 
+  if (new_current && new_current->ShouldSkipScreen()) {
+    new_current->Skip();
+    return;
+  }
+
   if (current_screen_) {
     current_screen_->Hide();
     current_screen_->SetConfiguration(nullptr);