[Dice] Split DiceTurnSyncOnHelper and add unittests

The UI code is moved out of DiceTurnSyncOnHelper into
DiceTurnSyncOnHelper::Delegate, to make the code easier to
test.

Change-Id: I5d4fd9a94a64a05f5c1a63ccd593b3260eea8421
Reviewed-on: https://chromium-review.googlesource.com/870595Reviewed-by: Julian Pastarmov <pastarmovj@chromium.org>
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Commit-Queue: David Roger <droger@chromium.org>
Cr-Commit-Position: refs/heads/master@{#531922}

chrome/browser/policy/cloud/user_policy_signin_service.h

@@ -55,7 +55,8 @@ class UserPolicySigninService : public UserPolicySigninServiceBase,
   // Registers a CloudPolicyClient for fetching policy for a user. |username| is
   // explicitly passed because the user is not yet authenticated, but the token
   // service has a refresh token available for |account_id|.
-  void RegisterForPolicyWithAccountId(
+  // Virtual for testing.
+  virtual void RegisterForPolicyWithAccountId(
       const std::string& username,
       const std::string& account_id,
       const PolicyRegistrationCallback& callback);

chrome/browser/policy/cloud/user_policy_signin_service_base.h

@@ -75,7 +75,8 @@ class UserPolicySigninServiceBase : public KeyedService,
   // |client_id| fetched via RegisterForPolicyXXX(). |callback| is invoked
   // once the policy fetch is complete, passing true if the policy fetch
   // succeeded.
-  void FetchPolicyForSignedInUser(
+  // Virtual for testing.
+  virtual void FetchPolicyForSignedInUser(
       const std::string& username,
       const std::string& dm_token,
       const std::string& client_id,

chrome/browser/ui/BUILD.gn

@@ -1681,6 +1681,8 @@ split_static_library("ui") {
       sources += [
         "webui/signin/dice_turn_sync_on_helper.cc",
         "webui/signin/dice_turn_sync_on_helper.h",
+        "webui/signin/dice_turn_sync_on_helper_delegate_impl.cc",
+        "webui/signin/dice_turn_sync_on_helper_delegate_impl.h",
         "webui/signin/signin_dice_internals_handler.cc",
         "webui/signin/signin_dice_internals_handler.h",
       ]

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

@@ -5,20 +5,18 @@
 #ifndef CHROME_BROWSER_UI_WEBUI_SIGNIN_DICE_TURN_SYNC_ON_HELPER_H_
 #define CHROME_BROWSER_UI_WEBUI_SIGNIN_DICE_TURN_SYNC_ON_HELPER_H_
 
+#include <memory>
 #include <string>
 
+#include "base/callback_forward.h"
+#include "base/macros.h"
 #include "base/memory/weak_ptr.h"
-#include "base/scoped_observer.h"
 #include "chrome/browser/profiles/profile.h"
-#include "chrome/browser/ui/browser_list_observer.h"
-#include "chrome/browser/ui/sync/profile_signin_confirmation_helper.h"
 #include "chrome/browser/ui/webui/signin/login_ui_service.h"
-#include "chrome/browser/ui/webui/signin/signin_email_confirmation_dialog.h"
 #include "components/signin/core/browser/account_info.h"
 #include "components/signin/core/browser/signin_metrics.h"
 
 class Browser;
-class BrowserList;
 class ProfileOAuth2TokenService;
 class SigninManager;
 
@@ -28,8 +26,7 @@ class ProfileSyncService;
 
 // Handles details of signing the user in with SigninManager and turning on
 // sync for an account that is already present in the token service.
-class DiceTurnSyncOnHelper : public BrowserListObserver,
-                             public LoginUIService::Observer {
+class DiceTurnSyncOnHelper {
  public:
   // Behavior when the signin is aborted (by an error or cancelled by the user).
   enum class SigninAbortedMode {
@@ -39,8 +36,66 @@ class DiceTurnSyncOnHelper : public BrowserListObserver,
     KEEP_ACCOUNT
   };
 
+  // User choice when signing in.
+  // Used for UMA histograms, Hence, constants should never be deleted or
+  // reordered, and  new constants should only be appended at the end.
+  // Keep this in sync with SigninChoice in histograms.xml.
+  enum SigninChoice {
+    SIGNIN_CHOICE_CANCEL = 0,       // Signin is cancelled.
+    SIGNIN_CHOICE_CONTINUE = 1,     // Signin continues in the current profile.
+    SIGNIN_CHOICE_NEW_PROFILE = 2,  // Signin continues in a new profile.
+    // SIGNIN_CHOICE_SIZE should always be last.
+    SIGNIN_CHOICE_SIZE,
+  };
+
+  using SigninChoiceCallback = base::OnceCallback<void(SigninChoice)>;
+
+  // Delegate implementing the UI prompts.
+  class Delegate {
+   public:
+    virtual ~Delegate() {}
+
+    // Shows a login error to the user.
+    virtual void ShowLoginError(const std::string& email,
+                                const std::string& error_message) = 0;
+
+    // Shows a confirmation dialog when the user was previously signed in with a
+    // different account in the same profile. |callback| must be called.
+    virtual void ShowMergeSyncDataConfirmation(
+        const std::string& previous_email,
+        const std::string& new_email,
+        SigninChoiceCallback callback) = 0;
+
+    // Shows a confirmation dialog when the user is signing in a managed
+    // account. |callback| must be called.
+    virtual void ShowEnterpriseAccountConfirmation(
+        const std::string& email,
+        SigninChoiceCallback callback) = 0;
+
+    // Shows a sync confirmation screen offering to open the Sync settings.
+    // |callback| must be called.
+    virtual void ShowSyncConfirmation(
+        base::OnceCallback<void(LoginUIService::SyncConfirmationUIClosedResult)>
+            callback) = 0;
+
+    // Opens the Sync settings page.
+    virtual void ShowSyncSettings() = 0;
+
+    // Opens the signin page in a new profile.
+    virtual void ShowSigninPageInNewProfile(Profile* new_profile,
+                                            const std::string& username) = 0;
+  };
+
   // Create a helper that turns sync on for an account that is already present
   // in the token service.
+  DiceTurnSyncOnHelper(Profile* profile,
+                       signin_metrics::AccessPoint signin_access_point,
+                       signin_metrics::Reason signin_reason,
+                       const std::string& account_id,
+                       SigninAbortedMode signin_aborted_mode,
+                       std::unique_ptr<Delegate> delegate);
+
+  // Convenience constructor using the default delegate.
   DiceTurnSyncOnHelper(Profile* profile,
                        Browser* browser,
                        signin_metrics::AccessPoint signin_access_point,
@@ -60,30 +115,18 @@ class DiceTurnSyncOnHelper : public BrowserListObserver,
     NEW_PROFILE
   };
 
-  // User input handler for the signin confirmation dialog.
-  class SigninDialogDelegate : public ui::ProfileSigninConfirmationDelegate {
-   public:
-    explicit SigninDialogDelegate(
-        base::WeakPtr<DiceTurnSyncOnHelper> sync_starter);
-    ~SigninDialogDelegate() override;
-    void OnCancelSignin() override;
-    void OnContinueSignin() override;
-    void OnSigninWithNewProfile() override;
-
-   private:
-    base::WeakPtr<DiceTurnSyncOnHelper> sync_starter_;
-  };
-  friend class SigninDialogDelegate;
-
   // DiceTurnSyncOnHelper deletes itself.
-  ~DiceTurnSyncOnHelper() override;
+  ~DiceTurnSyncOnHelper();
 
   // Handles can offer sign-in errors.  It returns true if there is an error,
   // and false otherwise.
   bool HasCanOfferSigninError();
 
-  // Callback used with ConfirmEmailDialogDelegate.
-  void ConfirmEmailAction(SigninEmailConfirmationDialog::Action action);
+  // Used as callback for ShowMergeSyncDataConfirmation().
+  void OnMergeAccountConfirmation(SigninChoice choice);
+
+  // Used as callback for ShowEnterpriseAccountConfirmation().
+  void OnEnterpriseAccountConfirmation(SigninChoice choice);
 
   // Turns sync on with the current profile or a new profile.
   void TurnSyncOnWithProfileMode(ProfileMode profile_mode);
@@ -117,18 +160,16 @@ class DiceTurnSyncOnHelper : public BrowserListObserver,
   // UI.
   void SigninAndShowSyncConfirmationUI();
 
-  // LoginUIService::Observer override. Deletes this object.
-  void OnSyncConfirmationUIClosed(
-      LoginUIService::SyncConfirmationUIClosedResult result) override;
-
-  // BrowserListObserver override.
-  void OnBrowserRemoved(Browser* browser) override;
+  // Handles the user input from the sync confirmation UI and deletes this
+  // object.
+  void FinishSyncSetupAndDelete(
+      LoginUIService::SyncConfirmationUIClosedResult result);
 
   // Aborts the flow and deletes this object.
   void AbortAndDelete();
 
+  std::unique_ptr<Delegate> delegate_;
   Profile* profile_;
-  Browser* browser_;
   SigninManager* signin_manager_;
   ProfileOAuth2TokenService* token_service_;
   const signin_metrics::AccessPoint signin_access_point_;
@@ -145,11 +186,6 @@ class DiceTurnSyncOnHelper : public BrowserListObserver,
   std::string dm_token_;
   std::string client_id_;
 
-  ScopedObserver<BrowserList, BrowserListObserver>
-      scoped_browser_list_observer_;
-  ScopedObserver<LoginUIService, LoginUIService::Observer>
-      scoped_login_ui_service_observer_;
-
   base::WeakPtrFactory<DiceTurnSyncOnHelper> weak_pointer_factory_;
   DISALLOW_COPY_AND_ASSIGN(DiceTurnSyncOnHelper);
 };

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

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/ui/webui/signin/dice_turn_sync_on_helper_delegate_impl.h"
+
+#include "base/logging.h"
+#include "base/metrics/user_metrics.h"
+#include "base/metrics/user_metrics_action.h"
+#include "base/strings/utf_string_conversions.h"
+#include "chrome/browser/profiles/profile_window.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_finder.h"
+#include "chrome/browser/ui/browser_list.h"
+#include "chrome/browser/ui/browser_tabstrip.h"
+#include "chrome/browser/ui/browser_window.h"
+#include "chrome/browser/ui/chrome_pages.h"
+#include "chrome/browser/ui/startup/startup_types.h"
+#include "chrome/browser/ui/tab_dialogs.h"
+#include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/browser/ui/webui/signin/login_ui_service_factory.h"
+#include "chrome/browser/ui/webui/signin/signin_email_confirmation_dialog.h"
+#include "chrome/common/url_constants.h"
+
+namespace {
+
+// If the |browser| argument is non-null, returns the pointer directly.
+// Otherwise creates a new browser for the profile, adds an empty tab and makes
+// sure the browser is visible.
+Browser* EnsureBrowser(Browser* browser, Profile* profile) {
+  if (!browser) {
+    // The user just created a new profile or has closed the browser that
+    // we used previously. Grab the most recently active browser or else
+    // create a new one.
+    browser = chrome::FindLastActiveWithProfile(profile);
+    if (!browser) {
+      browser = new Browser(Browser::CreateParams(profile, true));
+      chrome::AddTabAt(browser, GURL(), -1, true);
+    }
+    browser->window()->Show();
+  }
+  return browser;
+}
+
+// Converts SigninEmailConfirmationDialog::Action to
+// DiceTurnSyncOnHelper::SigninChoice and invokes |callback| on it.
+void OnEmailConfirmation(DiceTurnSyncOnHelper::SigninChoiceCallback callback,
+                         SigninEmailConfirmationDialog::Action action) {
+  DCHECK(callback) << "This function should be called only once.";
+  switch (action) {
+    case SigninEmailConfirmationDialog::START_SYNC:
+      std::move(callback).Run(DiceTurnSyncOnHelper::SIGNIN_CHOICE_CONTINUE);
+      return;
+    case SigninEmailConfirmationDialog::CREATE_NEW_USER:
+      std::move(callback).Run(DiceTurnSyncOnHelper::SIGNIN_CHOICE_NEW_PROFILE);
+      return;
+    case SigninEmailConfirmationDialog::CLOSE:
+      std::move(callback).Run(DiceTurnSyncOnHelper::SIGNIN_CHOICE_CANCEL);
+      return;
+  }
+  NOTREACHED();
+}
+
+}  // namespace
+
+DiceTurnSyncOnHelperDelegateImpl::SigninDialogDelegate::SigninDialogDelegate(
+    DiceTurnSyncOnHelper::SigninChoiceCallback callback)
+    : callback_(std::move(callback)) {
+  DCHECK(callback_);
+}
+
+DiceTurnSyncOnHelperDelegateImpl::SigninDialogDelegate::
+    ~SigninDialogDelegate() = default;
+
+void DiceTurnSyncOnHelperDelegateImpl::SigninDialogDelegate::OnCancelSignin() {
+  DCHECK(callback_);
+  std::move(callback_).Run(DiceTurnSyncOnHelper::SIGNIN_CHOICE_CANCEL);
+}
+
+void DiceTurnSyncOnHelperDelegateImpl::SigninDialogDelegate::
+    OnContinueSignin() {
+  DCHECK(callback_);
+  std::move(callback_).Run(DiceTurnSyncOnHelper::SIGNIN_CHOICE_CONTINUE);
+}
+
+void DiceTurnSyncOnHelperDelegateImpl::SigninDialogDelegate::
+    OnSigninWithNewProfile() {
+  DCHECK(callback_);
+  std::move(callback_).Run(DiceTurnSyncOnHelper::SIGNIN_CHOICE_NEW_PROFILE);
+}
+
+DiceTurnSyncOnHelperDelegateImpl::DiceTurnSyncOnHelperDelegateImpl(
+    Browser* browser)
+    : browser_(browser),
+      profile_(browser_->profile()),
+      scoped_browser_list_observer_(this),
+      scoped_login_ui_service_observer_(this) {
+  DCHECK(browser);
+  DCHECK(profile_);
+  scoped_browser_list_observer_.Add(BrowserList::GetInstance());
+}
+
+DiceTurnSyncOnHelperDelegateImpl::~DiceTurnSyncOnHelperDelegateImpl() {}
+
+void DiceTurnSyncOnHelperDelegateImpl::ShowLoginError(
+    const std::string& email,
+    const std::string& error_message) {
+  LoginUIServiceFactory::GetForProfile(profile_)->DisplayLoginResult(
+      browser_, base::UTF8ToUTF16(error_message), base::UTF8ToUTF16(email));
+}
+
+void DiceTurnSyncOnHelperDelegateImpl::ShowEnterpriseAccountConfirmation(
+    const std::string& email,
+    DiceTurnSyncOnHelper::SigninChoiceCallback callback) {
+  DCHECK(callback);
+  browser_ = EnsureBrowser(browser_, profile_);
+  content::WebContents* web_contents =
+      browser_->tab_strip_model()->GetActiveWebContents();
+  if (!web_contents) {
+    std::move(callback).Run(DiceTurnSyncOnHelper::SIGNIN_CHOICE_CANCEL);
+    return;
+  }
+
+  base::RecordAction(
+      base::UserMetricsAction("Signin_Show_EnterpriseAccountPrompt"));
+  TabDialogs::FromWebContents(web_contents)
+      ->ShowProfileSigninConfirmation(
+          browser_, profile_, email,
+          std::make_unique<SigninDialogDelegate>(std::move(callback)));
+}
+
+void DiceTurnSyncOnHelperDelegateImpl::ShowSyncConfirmation(
+    base::OnceCallback<void(LoginUIService::SyncConfirmationUIClosedResult)>
+        callback) {
+  DCHECK(callback);
+  sync_confirmation_callback_ = std::move(callback);
+  scoped_login_ui_service_observer_.Add(
+      LoginUIServiceFactory::GetForProfile(profile_));
+  browser_ = EnsureBrowser(browser_, profile_);
+  browser_->signin_view_controller()->ShowModalSyncConfirmationDialog(browser_);
+}
+
+void DiceTurnSyncOnHelperDelegateImpl::ShowMergeSyncDataConfirmation(
+    const std::string& previous_email,
+    const std::string& new_email,
+    DiceTurnSyncOnHelper::SigninChoiceCallback callback) {
+  DCHECK(callback);
+  content::WebContents* web_contents =
+      browser_->tab_strip_model()->GetActiveWebContents();
+  // TODO(droger): Replace Bind with BindOnce once the
+  // SigninEmailConfirmationDialog supports it.
+  SigninEmailConfirmationDialog::AskForConfirmation(
+      web_contents, profile_, previous_email, new_email,
+      base::Bind(&OnEmailConfirmation, base::Passed(std::move(callback))));
+}
+
+void DiceTurnSyncOnHelperDelegateImpl::ShowSyncSettings() {
+  browser_ = EnsureBrowser(browser_, profile_);
+  chrome::ShowSettingsSubPage(browser_, chrome::kSyncSetupSubPage);
+}
+
+void DiceTurnSyncOnHelperDelegateImpl::ShowSigninPageInNewProfile(
+    Profile* new_profile,
+    const std::string& username) {
+  profiles::FindOrCreateNewWindowForProfile(
+      new_profile, chrome::startup::IS_PROCESS_STARTUP,
+      chrome::startup::IS_FIRST_RUN, false);
+  Browser* browser = chrome::FindTabbedBrowser(new_profile, false);
+  browser->signin_view_controller()->ShowDiceSigninTab(
+      profiles::BUBBLE_VIEW_MODE_GAIA_SIGNIN, browser,
+      signin_metrics::AccessPoint::ACCESS_POINT_START_PAGE, username);
+}
+
+void DiceTurnSyncOnHelperDelegateImpl::OnSyncConfirmationUIClosed(
+    LoginUIService::SyncConfirmationUIClosedResult result) {
+  DCHECK(sync_confirmation_callback_);
+  std::move(sync_confirmation_callback_).Run(result);
+}
+
+void DiceTurnSyncOnHelperDelegateImpl::OnBrowserRemoved(Browser* browser) {
+  if (browser == browser_)
+    browser_ = nullptr;
+}

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

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROME_BROWSER_UI_WEBUI_SIGNIN_DICE_TURN_SYNC_ON_HELPER_DELEGATE_IMPL_H_
+#define CHROME_BROWSER_UI_WEBUI_SIGNIN_DICE_TURN_SYNC_ON_HELPER_DELEGATE_IMPL_H_
+
+#include "base/callback_forward.h"
+#include "base/macros.h"
+#include "base/scoped_observer.h"
+#include "chrome/browser/ui/browser_list_observer.h"
+#include "chrome/browser/ui/sync/profile_signin_confirmation_helper.h"
+#include "chrome/browser/ui/webui/signin/dice_turn_sync_on_helper.h"
+#include "chrome/browser/ui/webui/signin/login_ui_service.h"
+
+class Browser;
+class BrowserList;
+class Profile;
+
+// Default implementation for DiceTurnSyncOnHelper::Delegate.
+class DiceTurnSyncOnHelperDelegateImpl : public DiceTurnSyncOnHelper::Delegate,
+                                         public BrowserListObserver,
+                                         public LoginUIService::Observer {
+ public:
+  explicit DiceTurnSyncOnHelperDelegateImpl(Browser* browser);
+  ~DiceTurnSyncOnHelperDelegateImpl() override;
+
+ private:
+  // User input handler for the signin confirmation dialog.
+  class SigninDialogDelegate : public ui::ProfileSigninConfirmationDelegate {
+   public:
+    explicit SigninDialogDelegate(
+        DiceTurnSyncOnHelper::SigninChoiceCallback callback);
+    ~SigninDialogDelegate() override;
+    void OnCancelSignin() override;
+    void OnContinueSignin() override;
+    void OnSigninWithNewProfile() override;
+
+   private:
+    DiceTurnSyncOnHelper::SigninChoiceCallback callback_;
+
+    DISALLOW_COPY_AND_ASSIGN(SigninDialogDelegate);
+  };
+
+  // DiceTurnSyncOnHelper::Delegate:
+  void ShowLoginError(const std::string& email,
+                      const std::string& error_message) override;
+  void ShowMergeSyncDataConfirmation(
+      const std::string& previous_email,
+      const std::string& new_email,
+      DiceTurnSyncOnHelper::SigninChoiceCallback callback) override;
+  void ShowEnterpriseAccountConfirmation(
+      const std::string& email,
+      DiceTurnSyncOnHelper::SigninChoiceCallback callback) override;
+  void ShowSyncConfirmation(
+      base::OnceCallback<void(LoginUIService::SyncConfirmationUIClosedResult)>
+          callback) override;
+  void ShowSyncSettings() override;
+  void ShowSigninPageInNewProfile(Profile* new_profile,
+                                  const std::string& username) override;
+
+  // LoginUIService::Observer:
+  void OnSyncConfirmationUIClosed(
+      LoginUIService::SyncConfirmationUIClosedResult result) override;
+
+  // BrowserListObserver:
+  void OnBrowserRemoved(Browser* browser) override;
+
+  Browser* browser_;
+  Profile* profile_;
+  base::OnceCallback<void(LoginUIService::SyncConfirmationUIClosedResult)>
+      sync_confirmation_callback_;
+  ScopedObserver<BrowserList, BrowserListObserver>
+      scoped_browser_list_observer_;
+  ScopedObserver<LoginUIService, LoginUIService::Observer>
+      scoped_login_ui_service_observer_;
+
+  DISALLOW_COPY_AND_ASSIGN(DiceTurnSyncOnHelperDelegateImpl);
+};
+
+#endif  // CHROME_BROWSER_UI_WEBUI_SIGNIN_DICE_TURN_SYNC_ON_HELPER_DELEGATE_IMPL_H_

chrome/test/BUILD.gn

@@ -2720,6 +2720,7 @@ test("unit_tests") {
       "../browser/signin/dice_response_handler_unittest.cc",
       "../browser/signin/dice_tab_helper_unittest.cc",
       "../browser/signin/process_dice_header_delegate_impl_unittest.cc",
+      "../browser/ui/webui/signin/dice_turn_sync_on_helper_unittest.cc",
     ]
   }