Reland "[Dice sync delegate] Split sync confirmation and sync disabled"

This is a reland of d42167eb

Compared to the original CL, this CL minimizes the changes in the test
that was failing on Win7.

Original change's description:
> [Dice sync delegate] Split sync confirmation and sync disabled
>
> This CL splits the ShowSyncConfirmation() function of the dice turn on
> sync helper delegate into one that shows the normal sync confirmation
> and one that shows a error message that sync is disabled by the admin.
>
> Since both the dialogs are implemented by the same webUI page, there
> is not much behavioral change. The only change is for the delegate
> for the signed-in profile creation flow which opens a new browser
> window and shows the dialog in this window instead of showing it in
> the creation flow window.
>
> In follow-up CLs, the variant of the UI will be passed as a param
> to the WebUI page replacing the logic to decide which variant to show.
>
> Bug: 1126913
> Change-Id: Ibac9a339c4fb071f960f15864482c818247f5719
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2487100
> Commit-Queue: Jan Krcal <jkrcal@chromium.org>
> Reviewed-by: David Roger <droger@chromium.org>
> Auto-Submit: Jan Krcal <jkrcal@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#820222}

Bug: 1126913
Change-Id: I741140df31b8ca91fe5eb1a05a643fc0d2401f58
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2495069
Auto-Submit: Jan Krcal <jkrcal@chromium.org>
Reviewed-by: David Roger <droger@chromium.org>
Commit-Queue: Jan Krcal <jkrcal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#823175}

chrome/browser/policy/cloud/user_policy_signin_service_browsertest.cc

@@ -63,6 +63,9 @@ class TestDiceTurnSyncOnHelperDelegate : public DiceTurnSyncOnHelper::Delegate {
       const std::string& email,
       DiceTurnSyncOnHelper::SigninChoiceCallback callback) override;
   void ShowSyncConfirmation(
+      base::OnceCallback<void(LoginUIService::SyncConfirmationUIClosedResult)>
+          callback) override {}
+  void ShowSyncDisabledConfirmation(
       base::OnceCallback<void(LoginUIService::SyncConfirmationUIClosedResult)>
           callback) override;
   void ShowSyncSettings() override;
@@ -163,7 +166,7 @@ class UserPolicySigninServiceTest : public InProcessBrowserTest {
     std::move(callback).Run(DiceTurnSyncOnHelper::SIGNIN_CHOICE_CONTINUE);
   }
 
-  void OnShowSyncConfirmation(
+  void OnShowSyncDisabledConfirmation(
       base::OnceCallback<void(LoginUIService::SyncConfirmationUIClosedResult)>
           callback) {
     sync_confirmation_callback_ = std::move(callback);
@@ -329,10 +332,10 @@ void TestDiceTurnSyncOnHelperDelegate::ShowEnterpriseAccountConfirmation(
                                                      std::move(callback));
 }
 
-void TestDiceTurnSyncOnHelperDelegate::ShowSyncConfirmation(
+void TestDiceTurnSyncOnHelperDelegate::ShowSyncDisabledConfirmation(
     base::OnceCallback<void(LoginUIService::SyncConfirmationUIClosedResult)>
         callback) {
-  test_fixture_->OnShowSyncConfirmation(std::move(callback));
+  test_fixture_->OnShowSyncDisabledConfirmation(std::move(callback));
 }
 
 void TestDiceTurnSyncOnHelperDelegate::ShowSyncSettings() {

chrome/browser/signin/signin_util_win_browsertest.cc

@@ -59,6 +59,9 @@ class TestDiceTurnSyncOnHelperDelegate : public DiceTurnSyncOnHelper::Delegate {
           callback) override {
     std::move(callback).Run(LoginUIService::SYNC_WITH_DEFAULT_SETTINGS);
   }
+  void ShowSyncDisabledConfirmation(
+      base::OnceCallback<void(LoginUIService::SyncConfirmationUIClosedResult)>
+          callback) override {}
   void ShowSyncSettings() override {}
   void SwitchToProfile(Profile* new_profile) override {}
 };

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

@@ -16,6 +16,7 @@
 #include "chrome/browser/profiles/profile_attributes_storage.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/signin/identity_manager_factory.h"
+#include "chrome/browser/sync/profile_sync_service_factory.h"
 #include "chrome/browser/themes/theme_service.h"
 #include "chrome/browser/themes/theme_service_factory.h"
 #include "chrome/browser/ui/browser.h"
@@ -31,7 +32,10 @@
 #include "components/signin/public/identity_manager/account_info.h"
 #include "components/signin/public/identity_manager/identity_manager.h"
 #include "components/signin/public/identity_manager/identity_test_utils.h"
+#include "components/sync/base/sync_prefs.h"
 #include "components/sync/driver/sync_driver_switches.h"
+#include "components/sync/driver/sync_service.h"
+#include "components/sync/driver/sync_user_settings.h"
 #include "content/public/browser/web_contents_observer.h"
 #include "content/public/test/browser_test.h"
 #include "content/public/test/browser_test_utils.h"
@@ -80,7 +84,8 @@ class FirstVisuallyNonEmptyPaintObserver : public content::WebContentsObserver {
       return;
     }
     run_loop_.Run();
-    EXPECT_TRUE(IsExitConditionSatisfied());
+    EXPECT_TRUE(IsExitConditionSatisfied())
+        << web_contents()->GetVisibleURL() << " != " << url_;
   }
 
  private:
@@ -264,10 +269,6 @@ class ProfilePickerCreationFlowBrowserTest : public InProcessBrowserTest {
                 base::Bind(&ProfilePickerCreationFlowBrowserTest::
                                OnWillCreateBrowserContextServices,
                            base::Unretained(this)));
-
-    // A hack for DiceTurnSyncOnHelper to actually skip talking to SyncService.
-    base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
-    command_line->AppendSwitch(switches::kDisableSync);
   }
 
   virtual void OnWillCreateBrowserContextServices(
@@ -334,6 +335,8 @@ IN_PROC_BROWSER_TEST_F(ProfilePickerCreationFlowBrowserTest,
   // Add an account - simulate a successful Gaia sign-in.
   Profile* profile_being_created =
       static_cast<Profile*>(web_view()->GetBrowserContext());
+  syncer::SyncService* sync_service =
+      ProfileSyncServiceFactory::GetForProfile(profile_being_created);
   signin::IdentityManager* identity_manager =
       IdentityManagerFactory::GetForProfile(profile_being_created);
   CoreAccountInfo core_account_info =
@@ -373,6 +376,75 @@ IN_PROC_BROWSER_TEST_F(ProfilePickerCreationFlowBrowserTest,
   EXPECT_EQ(ThemeServiceFactory::GetForProfile(profile_being_created)
                 ->GetAutogeneratedThemeColor(),
             kProfileColor);
+  EXPECT_TRUE(sync_service->GetUserSettings()->IsSyncRequested());
+  EXPECT_TRUE(sync_service->GetUserSettings()->IsFirstSetupComplete());
+}
+
+IN_PROC_BROWSER_TEST_F(ProfilePickerCreationFlowBrowserTest,
+                       CreateSignedInProfileWithSyncDisabled) {
+  ASSERT_EQ(1u, BrowserList::GetInstance()->size());
+
+  ProfilePicker::Show(ProfilePicker::EntryPoint::kProfileMenuAddNewProfile);
+  WaitForNewWebView();
+
+  // Simulate a click on the signin button.
+  base::MockCallback<base::OnceClosure> switch_failure_callback;
+  EXPECT_CALL(switch_failure_callback, Run()).Times(0);
+  ProfilePicker::SwitchToSignIn(kProfileColor, switch_failure_callback.Get());
+
+  // The DICE navigation happens in a new web view (for the profile being
+  // created), wait for it.
+  WaitForNewWebView();
+  FirstVisuallyNonEmptyPaintObserver(
+      web_contents(), GaiaUrls::GetInstance()->signin_chrome_sync_dice())
+      .Wait();
+
+  // Disable sync by setting the device as managed in prefs.
+  Profile* profile_being_created =
+      static_cast<Profile*>(web_view()->GetBrowserContext());
+  syncer::SyncPrefs prefs(profile_being_created->GetPrefs());
+  prefs.SetManagedForTest(true);
+  syncer::SyncService* sync_service =
+      ProfileSyncServiceFactory::GetForProfile(profile_being_created);
+
+  // Add an account - simulate a successful Gaia sign-in.
+  signin::IdentityManager* identity_manager =
+      IdentityManagerFactory::GetForProfile(profile_being_created);
+  CoreAccountInfo core_account_info =
+      signin::MakeAccountAvailable(identity_manager, "joe.consumer@gmail.com");
+  ASSERT_TRUE(identity_manager->HasAccountWithRefreshToken(
+      core_account_info.account_id));
+
+  AccountInfo account_info = FillAccountInfo(core_account_info, "Joe");
+  signin::UpdateAccountInfoForAccount(identity_manager, account_info);
+
+  // Wait for the sign-in to propagate to the flow, resulting in new browser
+  // getting opened.
+  Browser* new_browser = BrowserAddedWaiter(2u).Wait();
+  FirstVisuallyNonEmptyPaintObserver(
+      new_browser->tab_strip_model()->GetActiveWebContents(),
+      GURL("chrome://newtab/"))
+      .Wait();
+
+  EXPECT_FALSE(ProfilePicker::IsOpen());
+
+  // Now the sync consent screen is shown, simulate closing the UI with "Stay
+  // signed in"
+  LoginUIServiceFactory::GetForProfile(profile_being_created)
+      ->SyncConfirmationUIClosed(LoginUIService::SYNC_WITH_DEFAULT_SETTINGS);
+
+  // Check expectations when the profile creation flow is done.
+  ProfileAttributesEntry* entry = nullptr;
+  ASSERT_TRUE(g_browser_process->profile_manager()
+                  ->GetProfileAttributesStorage()
+                  .GetProfileAttributesWithPath(
+                      profile_being_created->GetPath(), &entry));
+  EXPECT_FALSE(entry->IsEphemeral());
+  EXPECT_EQ(entry->GetLocalProfileName(), base::UTF8ToUTF16("Joe"));
+  EXPECT_EQ(ThemeServiceFactory::GetForProfile(profile_being_created)
+                ->GetAutogeneratedThemeColor(),
+            kProfileColor);
+  EXPECT_FALSE(sync_service->GetUserSettings()->IsSyncRequested());
 }
 
 IN_PROC_BROWSER_TEST_F(ProfilePickerCreationFlowBrowserTest,
@@ -660,11 +732,14 @@ IN_PROC_BROWSER_TEST_F(ProfilePickerEnterpriseCreationFlowBrowserTest,
       static_cast<Profile*>(web_view()->GetBrowserContext());
   signin::IdentityManager* identity_manager =
       IdentityManagerFactory::GetForProfile(profile_being_created);
-  CoreAccountInfo core_account_info =
-      signin::MakeAccountAvailable(identity_manager, "joe@enterprise.com");
+  // Consumer-looking gmail address avoids code that forces the sync service to
+  // actually start which would add overhead in mocking further stuff.
+  CoreAccountInfo core_account_info = signin::MakeAccountAvailable(
+      identity_manager, "joe.enterprise@gmail.com");
   ASSERT_TRUE(identity_manager->HasAccountWithRefreshToken(
       core_account_info.account_id));
 
+  // Enterprise domain needed for this profile being detected as Work.
   AccountInfo account_info =
       FillAccountInfo(core_account_info, "Joe", "enterprise.com");
   signin::UpdateAccountInfoForAccount(identity_manager, account_info);
@@ -679,19 +754,22 @@ IN_PROC_BROWSER_TEST_F(ProfilePickerEnterpriseCreationFlowBrowserTest,
       &loop_until_sync_confirmed);
   // Wait until the final sync confirmation screen in shown and confirmed.
   loop_until_sync_confirmed.Run();
-  // Now the sync consent screen is shown, simulate closing the UI with "Yes,
-  // I'm in".
-  LoginUIServiceFactory::GetForProfile(profile_being_created)
-      ->SyncConfirmationUIClosed(LoginUIService::SYNC_WITH_DEFAULT_SETTINGS);
 
+  // The picker should be closed even before the enterprise confirmation but it
+  // is closed asynchronously after opening the browser so after the NTP
+  // renders, it is safe to check.
   FirstVisuallyNonEmptyPaintObserver(
       new_browser->tab_strip_model()->GetActiveWebContents(),
       GURL("chrome://newtab/"))
       .Wait();
-
-  // Check expectations when the profile creation flow is done.
   EXPECT_FALSE(ProfilePicker::IsOpen());
 
+  // Now the sync consent screen is shown, simulate closing the UI with "Yes,
+  // I'm in".
+  LoginUIServiceFactory::GetForProfile(profile_being_created)
+      ->SyncConfirmationUIClosed(LoginUIService::SYNC_WITH_DEFAULT_SETTINGS);
+
+  // Check expectations when the profile creation flow is done.
   ProfileAttributesEntry* entry = nullptr;
   ASSERT_TRUE(g_browser_process->profile_manager()
                   ->GetProfileAttributesStorage()
@@ -728,11 +806,14 @@ IN_PROC_BROWSER_TEST_F(ProfilePickerEnterpriseCreationFlowBrowserTest,
       static_cast<Profile*>(web_view()->GetBrowserContext());
   signin::IdentityManager* identity_manager =
       IdentityManagerFactory::GetForProfile(profile_being_created);
-  CoreAccountInfo core_account_info =
-      signin::MakeAccountAvailable(identity_manager, "joe@enterprise.com");
+  // Consumer-looking gmail address avoids code that forces the sync service to
+  // actually start which would add overhead in mocking further stuff.
+  CoreAccountInfo core_account_info = signin::MakeAccountAvailable(
+      identity_manager, "joe.enterprise@gmail.com");
   ASSERT_TRUE(identity_manager->HasAccountWithRefreshToken(
       core_account_info.account_id));
 
+  // Enterprise domain needed for this profile being detected as Work.
   AccountInfo account_info =
       FillAccountInfo(core_account_info, "Joe", "enterprise.com");
   signin::UpdateAccountInfoForAccount(identity_manager, account_info);

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

@@ -18,6 +18,16 @@ void OpenSettingsInBrowser(Browser* browser) {
   chrome::ShowSettingsSubPage(browser, chrome::kSyncSetupSubPage);
 }
 
+void OpenSyncConfirmationDialogInBrowser(Browser* browser) {
+  // This is a very rare corner case (e.g. the user manages to close the only
+  // browser window in a very short span of time between enterprise
+  // confirmation and this callback), not worth handling fully. Instead, the
+  // flow is aborted.
+  if (!browser)
+    return;
+  browser->signin_view_controller()->ShowModalSyncConfirmationDialog();
+}
+
 }  // namespace
 
 ProfilePickerViewSyncDelegate::ProfilePickerViewSyncDelegate(
@@ -71,20 +81,27 @@ void ProfilePickerViewSyncDelegate::ShowSyncConfirmation(
       LoginUIServiceFactory::GetForProfile(profile_));
 
   if (enterprise_confirmation_shown_) {
-    Browser* browser = chrome::FindLastActiveWithProfile(profile_);
-    // This is a very rare corner case (e.g. the user manages to close the only
-    // browser window in a very short span of time between enterprise
-    // confirmation and this callback), not worth handling fully. Instead, the
-    // flow is aborted.
-    if (!browser)
-      return;
-    browser->signin_view_controller()->ShowModalSyncConfirmationDialog();
+    OpenSyncConfirmationDialogInBrowser(
+        chrome::FindLastActiveWithProfile(profile_));
     return;
   }
 
   ProfilePicker::SwitchToSyncConfirmation();
 }
 
+void ProfilePickerViewSyncDelegate::ShowSyncDisabledConfirmation(
+    base::OnceCallback<void(LoginUIService::SyncConfirmationUIClosedResult)>
+        callback) {
+  DCHECK(callback);
+  sync_confirmation_callback_ = std::move(callback);
+  scoped_login_ui_service_observer_.Add(
+      LoginUIServiceFactory::GetForProfile(profile_));
+
+  // Open the browser and when it's done, show the confirmation dialog.
+  std::move(open_browser_callback_)
+      .Run(base::BindOnce(&OpenSyncConfirmationDialogInBrowser));
+}
+
 void ProfilePickerViewSyncDelegate::ShowSyncSettings() {
   if (enterprise_confirmation_shown_) {
     Browser* browser = chrome::FindLastActiveWithProfile(profile_);

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

@@ -38,6 +38,9 @@ class ProfilePickerViewSyncDelegate : public DiceTurnSyncOnHelper::Delegate,
   void ShowSyncConfirmation(
       base::OnceCallback<void(LoginUIService::SyncConfirmationUIClosedResult)>
           callback) override;
+  void ShowSyncDisabledConfirmation(
+      base::OnceCallback<void(LoginUIService::SyncConfirmationUIClosedResult)>
+          callback) override;
   void ShowSyncSettings() override;
   void SwitchToProfile(Profile* new_profile) override;
 

chrome/browser/ui/webui/signin/dice_turn_sync_on_helper.cc

@@ -528,9 +528,15 @@ void DiceTurnSyncOnHelper::SyncStartupFailed() {
 }
 
 void DiceTurnSyncOnHelper::ShowSyncConfirmationUI() {
-  delegate_->ShowSyncConfirmation(
-      base::BindOnce(&DiceTurnSyncOnHelper::FinishSyncSetupAndDelete,
-                     weak_pointer_factory_.GetWeakPtr()));
+  if (GetSyncService()) {
+    delegate_->ShowSyncConfirmation(
+        base::BindOnce(&DiceTurnSyncOnHelper::FinishSyncSetupAndDelete,
+                       weak_pointer_factory_.GetWeakPtr()));
+  } else {
+    delegate_->ShowSyncDisabledConfirmation(
+        base::BindOnce(&DiceTurnSyncOnHelper::FinishSyncSetupAndDelete,
+                       weak_pointer_factory_.GetWeakPtr()));
+  }
 }
 
 void DiceTurnSyncOnHelper::FinishSyncSetupAndDelete(

chrome/browser/ui/webui/signin/dice_turn_sync_on_helper.h

@@ -90,6 +90,14 @@ class DiceTurnSyncOnHelper
         base::OnceCallback<void(LoginUIService::SyncConfirmationUIClosedResult)>
             callback) = 0;
 
+    // Shows a confirmation screen offering to stay signed-in or to signout.
+    // |callback| must be called.
+    // TODO(crbug.com/1126913): Use a new enum for this callback with only
+    // values that make sense here (stay signed-in / signout).
+    virtual void ShowSyncDisabledConfirmation(
+        base::OnceCallback<void(LoginUIService::SyncConfirmationUIClosedResult)>
+            callback) = 0;
+
     // Opens the Sync settings page.
     virtual void ShowSyncSettings() = 0;
 

chrome/browser/ui/webui/signin/dice_turn_sync_on_helper_delegate_impl.cc

@@ -100,6 +100,13 @@ void DiceTurnSyncOnHelperDelegateImpl::ShowSyncConfirmation(
   browser_->signin_view_controller()->ShowModalSyncConfirmationDialog();
 }
 
+void DiceTurnSyncOnHelperDelegateImpl::ShowSyncDisabledConfirmation(
+    base::OnceCallback<void(LoginUIService::SyncConfirmationUIClosedResult)>
+        callback) {
+  // This is handled by the same UI element as the normal sync confirmation.
+  ShowSyncConfirmation(std::move(callback));
+}
+
 void DiceTurnSyncOnHelperDelegateImpl::ShowMergeSyncDataConfirmation(
     const std::string& previous_email,
     const std::string& new_email,

chrome/browser/ui/webui/signin/dice_turn_sync_on_helper_delegate_impl.h

@@ -38,6 +38,9 @@ class DiceTurnSyncOnHelperDelegateImpl : public DiceTurnSyncOnHelper::Delegate,
   void ShowSyncConfirmation(
       base::OnceCallback<void(LoginUIService::SyncConfirmationUIClosedResult)>
           callback) override;
+  void ShowSyncDisabledConfirmation(
+      base::OnceCallback<void(LoginUIService::SyncConfirmationUIClosedResult)>
+          callback) override;
   void ShowSyncSettings() override;
   void SwitchToProfile(Profile* new_profile) override;
 

chrome/browser/ui/webui/signin/dice_turn_sync_on_helper_unittest.cc

@@ -87,6 +87,9 @@ class TestDiceTurnSyncOnHelperDelegate : public DiceTurnSyncOnHelper::Delegate {
   void ShowSyncConfirmation(
       base::OnceCallback<void(LoginUIService::SyncConfirmationUIClosedResult)>
           callback) override;
+  void ShowSyncDisabledConfirmation(
+      base::OnceCallback<void(LoginUIService::SyncConfirmationUIClosedResult)>
+          callback) override {}
   void ShowSyncSettings() override;
   void SwitchToProfile(Profile* new_profile) override;
 

chrome/browser/ui/webui/signin/login_ui_service.h

@@ -35,6 +35,8 @@ class LoginUIService : public KeyedService {
   // Used when the sync confirmation UI is closed to signify which option was
   // selected by the user.
   enum SyncConfirmationUIClosedResult {
+    // TODO(crbug.com/1141341): Rename the first option to make it work better
+    // for the sync-disabled variant of the UI.
     // Start sync immediately.
     SYNC_WITH_DEFAULT_SETTINGS,
     // Show the user the sync settings before starting sync.