Reland "SyncConsentScreen: Skip screen properly."

This is a reland of 8cd221ee

Original change's description:
> SyncConsentScreen: Skip screen properly.
> 
> Use new MaybeSkip method for skipping screen. Add tests for UMA stats.
> 
> Bug: 1064561
> Change-Id: I9ddfa0fb5c994f1cb6d6c7abf5e61ca42fd065a3
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2146900
> Commit-Queue: Roman Aleksandrov <raleksandrov@google.com>
> Reviewed-by: Roman Sorokin [CET] <rsorokin@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#762370}

Bug: 1064561
Change-Id: Ic9f6be0ea98423614d0ed2a524313679dd99baa5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2166163
Commit-Queue: Roman Aleksandrov <raleksandrov@google.com>
Reviewed-by: Roman Sorokin [CET] <rsorokin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#762667}

chrome/browser/chromeos/login/login_ui_shelf_visibility_browsertest.cc

@@ -65,7 +65,7 @@ IN_PROC_BROWSER_TEST_F(LoginUIShelfVisibilityTest, GaiaDialogOpen) {
 // Verifies that guest button and add user button are hidden on post-login
 // screens, after a user session is started.
 IN_PROC_BROWSER_TEST_F(LoginUIShelfVisibilityTest, PostLoginScreen) {
-  auto override = WizardController::ForceBrandedBuildForTesting();
+  auto autoreset = SyncConsentScreen::ForceBrandedBuildForTesting(true);
   EXPECT_TRUE(ash::LoginScreenTestApi::ClickAddUserButton());
   test::OobeGaiaPageWaiter().WaitUntilReady();
   LoginDisplayHost::default_host()

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

@@ -2,6 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "base/auto_reset.h"
 #include "base/bind.h"
 #include "base/macros.h"
 #include "base/strings/stringprintf.h"
@@ -14,20 +15,27 @@
 #include "chrome/browser/chromeos/login/test/login_manager_mixin.h"
 #include "chrome/browser/chromeos/login/test/oobe_base_test.h"
 #include "chrome/browser/chromeos/login/test/oobe_screen_exit_waiter.h"
+#include "chrome/browser/chromeos/login/test/oobe_screen_waiter.h"
+#include "chrome/browser/chromeos/login/test/oobe_screens_utils.h"
 #include "chrome/browser/chromeos/login/test/session_manager_state_waiter.h"
 #include "chrome/browser/chromeos/login/test/test_condition_waiter.h"
 #include "chrome/browser/chromeos/login/ui/login_display_host.h"
+#include "chrome/browser/chromeos/login/users/fake_chrome_user_manager.h"
 #include "chrome/browser/chromeos/login/wizard_controller.h"
 #include "chrome/browser/lifetime/application_lifetime.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.h"
+#include "chrome/browser/ui/webui/chromeos/login/marketing_opt_in_screen_handler.h"
 #include "chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/grit/generated_resources.h"
+#include "chrome/test/base/testing_profile_manager.h"
 #include "chromeos/constants/chromeos_features.h"
 #include "chromeos/constants/chromeos_switches.h"
 #include "components/prefs/pref_service.h"
 #include "components/sync/base/pref_names.h"
+#include "components/user_manager/scoped_user_manager.h"
+#include "components/user_manager/user_manager.h"
 #include "content/public/test/test_utils.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "ui/base/l10n/l10n_util.h"
@@ -90,12 +98,15 @@ std::string GetLocalizedConsentString(const int id) {
 
 class SyncConsentTest : public OobeBaseTest {
  public:
-  SyncConsentTest() = default;
+  SyncConsentTest() {
+    // To reuse existing wizard controller in the flow.
+    feature_list_.InitAndEnableFeature(
+        chromeos::features::kOobeScreensPriority);
+  }
   ~SyncConsentTest() override = default;
 
   void SetUpOnMainThread() override {
     OobeBaseTest::SetUpOnMainThread();
-    branded_build_override_ = WizardController::ForceBrandedBuildForTesting();
     if (features::IsSplitSettingsSyncEnabled()) {
       expected_consent_ids_ = {
           IDS_LOGIN_SYNC_CONSENT_SCREEN_TITLE,
@@ -119,6 +130,12 @@ class SyncConsentTest : public OobeBaseTest {
           IDS_LOGIN_SYNC_CONSENT_SCREEN_ACCEPT_AND_CONTINUE,
       };
     }
+
+    ReplaceExitCallback();
+
+    // Set up screen to be shown.
+    GetSyncConsentScreen()->SetProfileSyncDisabledByPolicyForTesting(false);
+    GetSyncConsentScreen()->SetProfileSyncEngineInitializedForTesting(true);
   }
 
   void TearDownOnMainThread() override {
@@ -142,13 +159,26 @@ class SyncConsentTest : public OobeBaseTest {
     test::OobeJS().CreateWaiter(condition)->Wait();
   }
 
+  void WaitForScreenShown() {
+    OobeScreenWaiter(SyncConsentScreenView::kScreenId).Wait();
+  }
+
+  void ReplaceExitCallback() {
+    original_callback_ =
+        GetSyncConsentScreen()->get_exit_callback_for_testing();
+    GetSyncConsentScreen()->set_exit_callback_for_testing(base::BindRepeating(
+        &SyncConsentTest::HandleScreenExit, base::Unretained(this)));
+  }
+
   void LoginToSyncConsentScreen() {
     login_manager_mixin_.LoginAsNewReguarUser();
     OobeScreenExitWaiter(GaiaView::kScreenId).Wait();
-    LoginDisplayHost::default_host()->StartWizard(
-        SyncConsentScreenView::kScreenId);
+    // No need to explicitly show the screen as it is the first one after login.
   }
 
+  base::Optional<SyncConsentScreen::Result> screen_result_;
+  base::HistogramTester histogram_tester_;
+
  protected:
   static SyncConsentScreen* GetSyncConsentScreen() {
     return static_cast<SyncConsentScreen*>(
@@ -163,9 +193,6 @@ class SyncConsentTest : public OobeBaseTest {
     ConsentRecordedWaiter consent_recorded_waiter;
     screen->SetDelegateForTesting(&consent_recorded_waiter);
 
-    screen->SetProfileSyncDisabledByPolicyForTesting(false);
-    screen->SetProfileSyncEngineInitializedForTesting(true);
-    screen->OnStateChanged(nullptr);
     test::OobeJS().CreateVisibilityWaiter(true, {"sync-consent-impl"})->Wait();
 
     if (features::IsSplitSettingsSyncEnabled()) {
@@ -195,6 +222,13 @@ class SyncConsentTest : public OobeBaseTest {
               consent_recorded_waiter.consent_description_ids_);
     EXPECT_EQ(expected_consent_confirmation_id,
               consent_recorded_waiter.consent_confirmation_id_);
+
+    WaitForScreenExit();
+    EXPECT_EQ(screen_result_.value(), SyncConsentScreen::Result::NEXT);
+    histogram_tester_.ExpectTotalCount(
+        "OOBE.StepCompletionTimeByExitReason.Sync-consent.Next", 1);
+    histogram_tester_.ExpectTotalCount("OOBE.StepCompletionTime.Sync-consent",
+                                       1);
   }
 
   std::vector<std::string> GetLocalizedExpectedConsentStrings() const {
@@ -205,17 +239,67 @@ class SyncConsentTest : public OobeBaseTest {
     return result;
   }
 
-  std::unique_ptr<base::AutoReset<bool>> branded_build_override_;
+  void WaitForScreenExit() {
+    if (screen_exited_)
+      return;
+
+    base::RunLoop run_loop;
+    screen_exit_callback_ = run_loop.QuitClosure();
+    run_loop.Run();
+  }
+
+ private:
+  void HandleScreenExit(SyncConsentScreen::Result result) {
+    ASSERT_FALSE(screen_exited_);
+    screen_exited_ = true;
+    screen_result_ = result;
+    original_callback_.Run(result);
+    if (screen_exit_callback_)
+      std::move(screen_exit_callback_).Run();
+  }
+
+  bool screen_exited_ = false;
+  base::RepeatingClosure screen_exit_callback_;
+  SyncConsentScreen::ScreenExitCallback original_callback_;
+
   std::vector<int> expected_consent_ids_;
   LoginManagerMixin login_manager_mixin_{&mixin_host_};
 
- private:
+  base::test::ScopedFeatureList feature_list_;
   DISALLOW_COPY_AND_ASSIGN(SyncConsentTest);
 };
 
+IN_PROC_BROWSER_TEST_F(SyncConsentTest, SkippedNotBrandedBuild) {
+  auto autoreset = SyncConsentScreen::ForceBrandedBuildForTesting(false);
+  LoginToSyncConsentScreen();
+
+  WaitForScreenExit();
+  EXPECT_EQ(screen_result_.value(), SyncConsentScreen::Result::NOT_APPLICABLE);
+  histogram_tester_.ExpectTotalCount(
+      "OOBE.StepCompletionTimeByExitReason.Sync-consent.Next", 0);
+  histogram_tester_.ExpectTotalCount("OOBE.StepCompletionTime.Sync-consent", 0);
+}
+
+IN_PROC_BROWSER_TEST_F(SyncConsentTest, SkippedSyncDisabledByPolicy) {
+  // Set up screen and policy.
+  auto autoreset = SyncConsentScreen::ForceBrandedBuildForTesting(true);
+  SyncConsentScreen* screen = GetSyncConsentScreen();
+  screen->SetProfileSyncDisabledByPolicyForTesting(true);
+
+  LoginToSyncConsentScreen();
+
+  WaitForScreenExit();
+  EXPECT_EQ(screen_result_.value(), SyncConsentScreen::Result::NOT_APPLICABLE);
+  histogram_tester_.ExpectTotalCount(
+      "OOBE.StepCompletionTimeByExitReason.Sync-consent.Next", 0);
+  histogram_tester_.ExpectTotalCount("OOBE.StepCompletionTime.Sync-consent", 0);
+}
+
 IN_PROC_BROWSER_TEST_F(SyncConsentTest, SyncConsentRecorder) {
+  auto autoreset = SyncConsentScreen::ForceBrandedBuildForTesting(true);
   EXPECT_EQ(g_browser_process->GetApplicationLocale(), "en-US");
   LoginToSyncConsentScreen();
+  WaitForScreenShown();
   // For En-US we hardcode strings here to catch string issues too.
   std::vector<std::string> expected_consent_strings;
   if (features::IsSplitSettingsSyncEnabled()) {
@@ -302,6 +386,7 @@ class SyncConsentPolicyDisabledTest : public SyncConsentTest,
 
 IN_PROC_BROWSER_TEST_P(SyncConsentPolicyDisabledTest,
                        SyncConsentPolicyDisabled) {
+  auto autoreset = SyncConsentScreen::ForceBrandedBuildForTesting(true);
   LoginToSyncConsentScreen();
 
   SyncConsentScreen* screen = GetSyncConsentScreen();
@@ -340,6 +425,7 @@ class SyncConsentSplitSettingsSyncTest : public SyncConsentTest {
 #define MAYBE_DefaultFlow DefaultFlow
 #endif
 IN_PROC_BROWSER_TEST_F(SyncConsentSplitSettingsSyncTest, MAYBE_DefaultFlow) {
+  auto autoreset = SyncConsentScreen::ForceBrandedBuildForTesting(true);
   LoginToSyncConsentScreen();
 
   // OS sync is disabled by default.
@@ -391,6 +477,12 @@ IN_PROC_BROWSER_TEST_F(SyncConsentSplitSettingsSyncTest, MAYBE_DefaultFlow) {
 
   // Toggle button is on-by-default, so OS sync should be on.
   EXPECT_TRUE(prefs->GetBoolean(syncer::prefs::kOsSyncFeatureEnabled));
+
+  WaitForScreenExit();
+  EXPECT_EQ(screen_result_.value(), SyncConsentScreen::Result::NEXT);
+  histogram_tester_.ExpectTotalCount(
+      "OOBE.StepCompletionTimeByExitReason.Sync-consent.Next", 1);
+  histogram_tester_.ExpectTotalCount("OOBE.StepCompletionTime.Sync-consent", 1);
 }
 
 // Flaky failures on sanitizer builds. https://crbug.com/1054377
@@ -400,6 +492,7 @@ IN_PROC_BROWSER_TEST_F(SyncConsentSplitSettingsSyncTest, MAYBE_DefaultFlow) {
 #define MAYBE_UserCanDisable UserCanDisable
 #endif
 IN_PROC_BROWSER_TEST_F(SyncConsentSplitSettingsSyncTest, MAYBE_UserCanDisable) {
+  auto autoreset = SyncConsentScreen::ForceBrandedBuildForTesting(true);
   LoginToSyncConsentScreen();
 
   // Wait for content to load.
@@ -423,6 +516,12 @@ IN_PROC_BROWSER_TEST_F(SyncConsentSplitSettingsSyncTest, MAYBE_UserCanDisable) {
   // OS sync is off.
   PrefService* prefs = ProfileManager::GetPrimaryUserProfile()->GetPrefs();
   EXPECT_FALSE(prefs->GetBoolean(syncer::prefs::kOsSyncFeatureEnabled));
+
+  WaitForScreenExit();
+  EXPECT_EQ(screen_result_.value(), SyncConsentScreen::Result::NEXT);
+  histogram_tester_.ExpectTotalCount(
+      "OOBE.StepCompletionTimeByExitReason.Sync-consent.Next", 1);
+  histogram_tester_.ExpectTotalCount("OOBE.StepCompletionTime.Sync-consent", 1);
 }
 
 // Tests that the SyncConsent screen performs a timezone request so that
@@ -438,6 +537,7 @@ class SyncConsentTimezoneOverride : public SyncConsentTest {
 };
 
 IN_PROC_BROWSER_TEST_F(SyncConsentTimezoneOverride, MakesTimezoneRequest) {
+  auto autoreset = SyncConsentScreen::ForceBrandedBuildForTesting(true);
   LoginToSyncConsentScreen();
   EXPECT_EQ("TimezeonPropagationTest",
             g_browser_process->local_state()->GetString(

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

@@ -46,6 +46,23 @@ void RecordUmaReviewFollowingSetup(bool value) {
 
 }  // namespace
 
+// static
+#if BUILDFLAG(GOOGLE_CHROME_BRANDING)
+bool g_is_branded_build = true;
+#else
+bool g_is_branded_build = false;
+#endif
+
+// static
+std::string SyncConsentScreen::GetResultString(Result result) {
+  switch (result) {
+    case Result::NEXT:
+      return "Next";
+    case Result::NOT_APPLICABLE:
+      return BaseScreen::kNotApplicable;
+  }
+}
+
 // static
 void SyncConsentScreen::MaybeLaunchSyncConsentSettings(Profile* profile) {
   if (profile->GetPrefs()->GetBoolean(prefs::kShowSyncSettingsOnSessionStart)) {
@@ -66,9 +83,8 @@ void SyncConsentScreen::MaybeLaunchSyncConsentSettings(Profile* profile) {
   }
 }
 
-SyncConsentScreen::SyncConsentScreen(
-    SyncConsentScreenView* view,
-    const base::RepeatingClosure& exit_callback)
+SyncConsentScreen::SyncConsentScreen(SyncConsentScreenView* view,
+                                     const ScreenExitCallback& exit_callback)
     : BaseScreen(SyncConsentScreenView::kScreenId, OobeScreenPriority::DEFAULT),
       view_(view),
       exit_callback_(exit_callback) {
@@ -80,17 +96,33 @@ SyncConsentScreen::~SyncConsentScreen() {
   view_->Bind(NULL);
 }
 
-void SyncConsentScreen::ShowImpl() {
+void SyncConsentScreen::Init() {
+  if (is_initialized_)
+    return;
+  is_initialized_ = true;
   user_ = user_manager::UserManager::Get()->GetPrimaryUser();
   profile_ = ProfileHelper::Get()->GetProfileByUser(user_);
-
   UpdateScreen();
+}
+
+bool SyncConsentScreen::MaybeSkip() {
+  if (!g_is_branded_build) {
+    exit_callback_.Run(Result::NOT_APPLICABLE);
+    return true;
+  }
+  Init();
 
   if (behavior_ == SyncScreenBehavior::SKIP) {
-    exit_callback_.Run();
-    return;
+    exit_callback_.Run(Result::NOT_APPLICABLE);
+    return true;
   }
 
+  return false;
+}
+
+void SyncConsentScreen::ShowImpl() {
+  Init();
+
   if (behavior_ != SyncScreenBehavior::SHOW) {
     // Wait for updates and set the loading throbber to be visible.
     view_->SetThrobberVisible(true /*visible*/);
@@ -122,7 +154,7 @@ void SyncConsentScreen::OnContinueAndReview(
   RecordConsent(CONSENT_GIVEN, consent_description, consent_confirmation);
   profile_->GetPrefs()->SetBoolean(prefs::kShowSyncSettingsOnSessionStart,
                                    true);
-  exit_callback_.Run();
+  exit_callback_.Run(Result::NEXT);
 }
 
 void SyncConsentScreen::OnContinueWithDefaults(
@@ -132,7 +164,7 @@ void SyncConsentScreen::OnContinueWithDefaults(
     return;
   RecordUmaReviewFollowingSetup(false);
   RecordConsent(CONSENT_GIVEN, consent_description, consent_confirmation);
-  exit_callback_.Run();
+  exit_callback_.Run(Result::NEXT);
 }
 
 void SyncConsentScreen::OnAcceptAndContinue(
@@ -156,7 +188,13 @@ void SyncConsentScreen::OnAcceptAndContinue(
                                      true);
   }
 
-  exit_callback_.Run();
+  exit_callback_.Run(Result::NEXT);
+}
+
+// static
+std::unique_ptr<base::AutoReset<bool>>
+SyncConsentScreen::ForceBrandedBuildForTesting(bool value) {
+  return std::make_unique<base::AutoReset<bool>>(&g_is_branded_build, value);
 }
 
 void SyncConsentScreen::SetDelegateForTesting(
@@ -215,7 +253,7 @@ void SyncConsentScreen::UpdateScreen() {
 
   // Screen is shown and behavior has changed.
   if (behavior_ == SyncScreenBehavior::SKIP)
-    exit_callback_.Run();
+    exit_callback_.Run(Result::NEXT);
 
   if (behavior_ == SyncScreenBehavior::SHOW) {
     view_->SetThrobberVisible(false /*visible*/);

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

@@ -8,6 +8,7 @@
 #include <memory>
 #include <string>
 
+#include "base/auto_reset.h"
 #include "base/macros.h"
 #include "base/optional.h"
 #include "base/scoped_observer.h"
@@ -35,6 +36,12 @@ class SyncConsentScreen : public BaseScreen,
  public:
   enum ConsentGiven { CONSENT_NOT_GIVEN, CONSENT_GIVEN };
 
+  enum class Result { NEXT, NOT_APPLICABLE };
+
+  static std::string GetResultString(Result result);
+
+  using ScreenExitCallback = base::RepeatingCallback<void(Result result)>;
+
   class SyncConsentScreenTestDelegate {
    public:
     SyncConsentScreenTestDelegate() = default;
@@ -61,9 +68,12 @@ class SyncConsentScreen : public BaseScreen,
   static void MaybeLaunchSyncConsentSettings(Profile* profile);
 
   SyncConsentScreen(SyncConsentScreenView* view,
-                    const base::RepeatingClosure& exit_callback);
+                    const ScreenExitCallback& exit_callback);
   ~SyncConsentScreen() override;
 
+  // Inits |user_|, its |profile_| and |behavior_| before using the screen.
+  void Init();
+
   // syncer::SyncServiceObserver:
   void OnStateChanged(syncer::SyncService* sync) override;
 
@@ -81,6 +91,9 @@ class SyncConsentScreen : public BaseScreen,
                            bool enable_os_sync,
                            bool review_browser_sync);
 
+  static std::unique_ptr<base::AutoReset<bool>> ForceBrandedBuildForTesting(
+      bool value);
+
   // Sets internal condition "Sync disabled by policy" for tests.
   void SetProfileSyncDisabledByPolicyForTesting(bool value);
 
@@ -92,8 +105,17 @@ class SyncConsentScreen : public BaseScreen,
       SyncConsentScreen::SyncConsentScreenTestDelegate* delegate);
   SyncConsentScreenTestDelegate* GetDelegateForTesting() const;
 
+  void set_exit_callback_for_testing(const ScreenExitCallback& exit_callback) {
+    exit_callback_ = exit_callback;
+  }
+
+  const ScreenExitCallback& get_exit_callback_for_testing() {
+    return exit_callback_;
+  }
+
  private:
   // BaseScreen:
+  bool MaybeSkip() override;
   void ShowImpl() override;
   void HideImpl() override;
 
@@ -119,7 +141,7 @@ class SyncConsentScreen : public BaseScreen,
   SyncScreenBehavior behavior_ = UNKNOWN;
 
   SyncConsentScreenView* const view_;
-  base::RepeatingClosure exit_callback_;
+  ScreenExitCallback exit_callback_;
 
   // Manages sync service observer lifetime.
   ScopedObserver<syncer::SyncService, syncer::SyncServiceObserver>
@@ -128,6 +150,7 @@ class SyncConsentScreen : public BaseScreen,
   // Primary user ind his Profile (if screen is shown).
   const user_manager::User* user_ = nullptr;
   Profile* profile_ = nullptr;
+  bool is_initialized_ = false;
 
   base::Optional<bool> test_sync_disabled_by_policy_;
   base::Optional<bool> test_sync_engine_initialized_;

chrome/browser/chromeos/login/wizard_controller.cc

@@ -680,10 +680,7 @@ void WizardController::ShowSyncConsentScreen() {
   // region. Currently used on the MarketingOptInScreen.
   StartNetworkTimezoneResolve();
 
-  if (is_branded_build_)
   SetCurrentScreen(GetScreen(SyncConsentScreenView::kScreenId));
-  else
-    OnSyncConsentFinished();
 }
 
 void WizardController::ShowFingerprintSetupScreen() {
@@ -1095,12 +1092,10 @@ void WizardController::OnTermsOfServiceScreenExit(
   }
 }
 
-void WizardController::OnSyncConsentScreenExit() {
-  OnScreenExit(SyncConsentScreenView::kScreenId, kDefaultExitReason);
-  OnSyncConsentFinished();
-}
-
-void WizardController::OnSyncConsentFinished() {
+void WizardController::OnSyncConsentScreenExit(
+    SyncConsentScreen::Result result) {
+  OnScreenExit(SyncConsentScreenView::kScreenId,
+               SyncConsentScreen::GetResultString(result));
   ShowFingerprintSetupScreen();
 }
 

chrome/browser/chromeos/login/wizard_controller.h

@@ -34,6 +34,7 @@
 #include "chrome/browser/chromeos/login/screens/network_screen.h"
 #include "chrome/browser/chromeos/login/screens/packaged_license_screen.h"
 #include "chrome/browser/chromeos/login/screens/recommend_apps_screen.h"
+#include "chrome/browser/chromeos/login/screens/sync_consent_screen.h"
 #include "chrome/browser/chromeos/login/screens/terms_of_service_screen.h"
 #include "chrome/browser/chromeos/login/screens/update_screen.h"
 #include "chrome/browser/chromeos/policy/enrollment_config.h"
@@ -228,9 +229,8 @@ class WizardController {
   void OnDemoPreferencesScreenExit(DemoPreferencesScreen::Result result);
   void OnDemoSetupScreenExit(DemoSetupScreen::Result result);
   void OnTermsOfServiceScreenExit(TermsOfServiceScreen::Result result);
-  void OnSyncConsentScreenExit();
-  void OnSyncConsentFinished();
   void OnFingerprintSetupScreenExit(FingerprintSetupScreen::Result result);
+  void OnSyncConsentScreenExit(SyncConsentScreen::Result result);
   void OnDiscoverScreenExit();
   void OnArcTermsOfServiceScreenExit(ArcTermsOfServiceScreen::Result result);
   void OnArcTermsOfServiceAccepted();