Distinguish manual cancellation of profile creation from error conditions.

Since manual cancellation comes from the same place as the original
registration request, there's no need to use a callback to tell the
caller that registration has been cancelled: just do any necessary
cleanup right away.

This is needed because some OAuth problems can also result in the
GoogleServiceAuthError::REQUEST_CANCELED error, and it's important to
distinguish those actual errors from user-initiated cancellation.

Also some additional cleanup and bug-fixing for metrics.

TBR=sail@chromium.org

BUG=244418

Review URL: https://chromiumcodereview.appspot.com/16576011

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@205168 0039d316-1c4b-4281-b951-d872f2087c98

7 files changed, 105 insertions, 80 deletions

diff --git a/chrome/browser/managed_mode/managed_user_registration_service.cc b/chrome/browser/managed_mode/managed_user_registration_service.cc
index 7a3e2e8..32a5164 100644
--- a/chrome/browser/managed_mode/managed_user_registration_service.cc
+++ b/chrome/browser/managed_mode/managed_user_registration_service.cc
@@ -38,7 +38,7 @@ using syncer::SyncMergeResult;
 using sync_pb::ManagedUserSpecifics;
 using user_prefs::PrefRegistrySyncable;
 
-// How long to wait before canceling user registration. If this is changed, the
+// How long to wait before aborting user registration. If this is changed, the
 // histogram limits in the BrowserOptionsHandler should also be updated.
 static const int kRegistrationTimeoutMS = 30 * 1000;
 
@@ -99,8 +99,9 @@ void ManagedUserRegistrationService::Register(
         FROM_HERE,
         base::TimeDelta::FromMilliseconds(kRegistrationTimeoutMS),
         base::Bind(
-            &ManagedUserRegistrationService::CancelPendingRegistrationImpl,
+            &ManagedUserRegistrationService::AbortPendingRegistration,
             weak_ptr_factory_.GetWeakPtr(),
+            true,  // Run the callback.
             GoogleServiceAuthError(GoogleServiceAuthError::CONNECTION_FAILED)));
   }
 
@@ -133,12 +134,15 @@ void ManagedUserRegistrationService::Register(
 }
 
 void ManagedUserRegistrationService::CancelPendingRegistration() {
-  CancelPendingRegistrationImpl(
-      GoogleServiceAuthError(GoogleServiceAuthError::REQUEST_CANCELED));
+  AbortPendingRegistration(
+      false,  // Don't run the callback. The error will be ignored.
+      GoogleServiceAuthError(GoogleServiceAuthError::NONE));
 }
 
 void ManagedUserRegistrationService::Shutdown() {
-  CancelPendingRegistration();
+  AbortPendingRegistration(
+      true,  // Run the callback.
+      GoogleServiceAuthError(GoogleServiceAuthError::REQUEST_CANCELED));
 }
 
 SyncMergeResult ManagedUserRegistrationService::MergeDataAndStartSyncing(
@@ -206,7 +210,9 @@ void ManagedUserRegistrationService::StopSyncing(ModelType type) {
 
   // Canceling a pending registration might result in changes in the Sync data,
   // so we do it before resetting the |sync_processor|.
-  CancelPendingRegistration();
+  AbortPendingRegistration(
+      true,  // Run the callback.
+      GoogleServiceAuthError(GoogleServiceAuthError::REQUEST_CANCELED));
 
   sync_processor_.reset();
   error_handler_.reset();
@@ -299,7 +305,7 @@ void ManagedUserRegistrationService::OnManagedUserAcknowledged(
   DCHECK_EQ(pending_managed_user_id_, managed_user_id);
   DCHECK(!pending_managed_user_acknowledged_);
   pending_managed_user_acknowledged_ = true;
-  DispatchCallbackIfReady();
+  CompleteRegistrationIfReady();
 }
 
 void ManagedUserRegistrationService::FetchToken(
@@ -315,36 +321,39 @@ void ManagedUserRegistrationService::OnReceivedToken(
     const GoogleServiceAuthError& error,
     const std::string& token) {
   if (error.state() != GoogleServiceAuthError::NONE) {
-    DispatchCallback(error);
+    CompleteRegistration(true, error);
     return;
   }
 
   DCHECK(!token.empty());
   pending_managed_user_token_ = token;
-  DispatchCallbackIfReady();
+  CompleteRegistrationIfReady();
 }
 
-void ManagedUserRegistrationService::DispatchCallbackIfReady() {
+void ManagedUserRegistrationService::CompleteRegistrationIfReady() {
   if (!pending_managed_user_acknowledged_ ||
       pending_managed_user_token_.empty()) {
     return;
   }
 
   GoogleServiceAuthError error(GoogleServiceAuthError::NONE);
-  DispatchCallback(error);
+  CompleteRegistration(true, error);
 }
 
-void ManagedUserRegistrationService::CancelPendingRegistrationImpl(
+void ManagedUserRegistrationService::AbortPendingRegistration(
+    bool run_callback,
     const GoogleServiceAuthError& error) {
   pending_managed_user_token_.clear();
-  DispatchCallback(error);
+  CompleteRegistration(run_callback, error);
 }
 
-void ManagedUserRegistrationService::DispatchCallback(
+void ManagedUserRegistrationService::CompleteRegistration(
+    bool run_callback,
     const GoogleServiceAuthError& error) {
   registration_timer_.Stop();
   if (!callback_.is_null()) {
-    callback_.Run(error, pending_managed_user_token_);
+    if (run_callback)
+      callback_.Run(error, pending_managed_user_token_);
     callback_.Reset();
 
     DCHECK(!pending_managed_user_id_.empty());
diff --git a/chrome/browser/managed_mode/managed_user_registration_service.h b/chrome/browser/managed_mode/managed_user_registration_service.h
index 047a2e4..9b8ce7f 100644
--- a/chrome/browser/managed_mode/managed_user_registration_service.h
+++ b/chrome/browser/managed_mode/managed_user_registration_service.h
@@ -56,8 +56,9 @@ class ManagedUserRegistrationService : public BrowserContextKeyedService,
   // managed user does not yet exist.
   void Register(const string16& name, const RegistrationCallback& callback);
 
-  // Cancels any registration currently in progress and calls the callback with
-  // an appropriate error.
+  // Cancels any registration currently in progress, without calling the
+  // callback or reporting an error. This should be called when the user
+  // actively cancels the registration by canceling profile creation.
   void CancelPendingRegistration();
 
   // ProfileKeyedService implementation:
@@ -90,16 +91,19 @@ class ManagedUserRegistrationService : public BrowserContextKeyedService,
   void OnReceivedToken(const GoogleServiceAuthError& error,
                        const std::string& token);
 
-  // Dispatches the callback if all the conditions have been met.
-  void DispatchCallbackIfReady();
+  // Dispatches the callback and cleans up if all the conditions have been met.
+  void CompleteRegistrationIfReady();
 
-  // Cancels any registration currently in progress and calls the callback
-  // specified when Register was called with the given error.
-  void CancelPendingRegistrationImpl(const GoogleServiceAuthError& error);
+  // Aborts any registration currently in progress. If |run_callback| is true,
+  // calls the callback specified in Register() with the given |error|.
+  void AbortPendingRegistration(bool run_callback,
+                                const GoogleServiceAuthError& error);
 
-  // Dispatches the callback with the saved token (which may be empty) and the
-  // given |error|.
-  void DispatchCallback(const GoogleServiceAuthError& error);
+  // If |run_callback| is true, dispatches the callback with the saved token
+  // (which may be empty) and the given |error|. In any case, resets internal
+  // variables to be ready for the next registration.
+  void CompleteRegistration(bool run_callback,
+                            const GoogleServiceAuthError& error);
 
   base::WeakPtrFactory<ManagedUserRegistrationService> weak_ptr_factory_;
   PrefService* prefs_;
diff --git a/chrome/browser/managed_mode/managed_user_service.cc b/chrome/browser/managed_mode/managed_user_service.cc
index e6e07ba..8a06b7e 100644
--- a/chrome/browser/managed_mode/managed_user_service.cc
+++ b/chrome/browser/managed_mode/managed_user_service.cc
@@ -542,10 +542,7 @@ void ManagedUserService::OnManagedUserRegistered(
     Profile* custodian_profile,
     const GoogleServiceAuthError& auth_error,
     const std::string& token) {
-  if (auth_error.state() == GoogleServiceAuthError::REQUEST_CANCELED) {
-    callback.Run(profile_, Profile::CREATE_STATUS_CANCELED);
-    return;
-  } else if (auth_error.state() != GoogleServiceAuthError::NONE) {
+  if (auth_error.state() != GoogleServiceAuthError::NONE) {
     LOG(ERROR) << "Managed user OAuth error: " << auth_error.ToString();
     DCHECK_EQ(std::string(), token);
     callback.Run(profile_, Profile::CREATE_STATUS_REMOTE_FAIL);
diff --git a/chrome/browser/managed_mode/managed_user_service.h b/chrome/browser/managed_mode/managed_user_service.h
index b0e5d97..26210a5 100644
--- a/chrome/browser/managed_mode/managed_user_service.h
+++ b/chrome/browser/managed_mode/managed_user_service.h
@@ -120,7 +120,10 @@ class ManagedUserService : public BrowserContextKeyedService,
   // Convenience method that registers this managed user with
   // |registration_service| and initializes sync with the returned token.
   // Note that |registration_service| should belong to the custodian's profile,
-  // not this one.
+  // not this one. The |callback| will be called when registration is complete,
+  // whether it suceeded or not -- unless registration was cancelled in the
+  // ManagedUserRegistrationService manually, in which case the callback will
+  // be ignored.
   void RegisterAndInitSync(Profile* custodian_profile,
                            const ProfileManager::CreateCallback& callback);
 
diff --git a/chrome/browser/profiles/profile.h b/chrome/browser/profiles/profile.h
index 83b3200..a94076c 100644
--- a/chrome/browser/profiles/profile.h
+++ b/chrome/browser/profiles/profile.h
@@ -104,7 +104,8 @@ class Profile : public content::BrowserContext {
     CREATE_STATUS_CREATED,
     // Profile is created, extensions and promo resources are initialized.
     CREATE_STATUS_INITIALIZED,
-    // Profile creation (managed-user registration, generally) was canceled.
+    // Profile creation (managed-user registration, generally) was canceled
+    // by the user.
     CREATE_STATUS_CANCELED,
     MAX_CREATE_STATUS  // For histogram display.
   };
diff --git a/chrome/browser/ui/webui/options/browser_options_handler.cc b/chrome/browser/ui/webui/options/browser_options_handler.cc
index fa4a088..ec5e69d 100644
--- a/chrome/browser/ui/webui/options/browser_options_handler.cc
+++ b/chrome/browser/ui/webui/options/browser_options_handler.cc
@@ -198,7 +198,7 @@ BrowserOptionsHandler::BrowserOptionsHandler()
 }
 
 BrowserOptionsHandler::~BrowserOptionsHandler() {
-  CancelProfileCreation();
+  CancelProfileRegistration(false);
   ProfileSyncService* sync_service(ProfileSyncServiceFactory::
       GetInstance()->GetForProfile(Profile::FromWebUI(web_ui())));
   if (sync_service)
@@ -1098,6 +1098,15 @@ void BrowserOptionsHandler::SendProfilesInfo() {
                                    *GetProfilesInfoList());
 }
 
+chrome::HostDesktopType BrowserOptionsHandler::GetDesktopType() {
+  Browser* browser =
+      chrome::FindBrowserWithWebContents(web_ui()->GetWebContents());
+  chrome::HostDesktopType desktop_type = chrome::HOST_DESKTOP_TYPE_NATIVE;
+  if (browser)
+    desktop_type = browser->host_desktop_type();
+  return desktop_type;
+}
+
 void BrowserOptionsHandler::CreateProfile(const ListValue* args) {
 #if defined(ENABLE_MANAGED_USERS)
   // This handler could have been called in managed mode, for example because
@@ -1111,15 +1120,8 @@ void BrowserOptionsHandler::CreateProfile(const ListValue* args) {
     return;
 
   DCHECK(profile_path_being_created_.empty());
-
   profile_creation_start_time_ = base::TimeTicks::Now();
 
-  Browser* browser =
-      chrome::FindBrowserWithWebContents(web_ui()->GetWebContents());
-  chrome::HostDesktopType desktop_type = chrome::HOST_DESKTOP_TYPE_NATIVE;
-  if (browser)
-    desktop_type = browser->host_desktop_type();
-
   string16 name;
   string16 icon;
   bool create_shortcut = false;
@@ -1137,7 +1139,8 @@ void BrowserOptionsHandler::CreateProfile(const ListValue* args) {
 
   ProfileManager::CreateCallback show_user_feedback =
       base::Bind(&BrowserOptionsHandler::ShowProfileCreationFeedback,
-                 weak_ptr_factory_.GetWeakPtr(), desktop_type, managed_user);
+                 weak_ptr_factory_.GetWeakPtr(), GetDesktopType(),
+                 managed_user);
 
   if (managed_user && ManagedUserService::AreManagedUsersEnabled()) {
 #if defined(ENABLE_MANAGED_USERS)
@@ -1175,22 +1178,26 @@ void BrowserOptionsHandler::RegisterNewManagedUser(
                                             callback);
 }
 
+void BrowserOptionsHandler::RecordProfileCreationMetrics(
+    Profile::CreateStatus status) {
+  UMA_HISTOGRAM_ENUMERATION("Profile.CreateResult",
+                            status,
+                            Profile::MAX_CREATE_STATUS);
+  UMA_HISTOGRAM_CUSTOM_TIMES("Profile.CreateTime",
+      base::TimeTicks::Now() - profile_creation_start_time_,
+      base::TimeDelta::FromMilliseconds(1),
+      base::TimeDelta::FromSeconds(30),  // From kRegistrationTimeoutMS.
+      100);
+}
+
 void BrowserOptionsHandler::ShowProfileCreationFeedback(
     chrome::HostDesktopType desktop_type,
     bool is_managed,
     Profile* profile,
     Profile::CreateStatus status) {
   DCHECK(profile_path_being_created_ == profile->GetPath());
-  if (status != Profile::CREATE_STATUS_CREATED) {
-    UMA_HISTOGRAM_ENUMERATION("Profile.CreateResult",
-                              status,
-                              Profile::MAX_CREATE_STATUS);
-    UMA_HISTOGRAM_CUSTOM_TIMES("Profile.CreateTime",
-        base::TimeTicks::Now() - profile_creation_start_time_,
-        base::TimeDelta::FromMilliseconds(1),
-        base::TimeDelta::FromSeconds(30),  // From kRegistrationTimeoutMS.
-        100);
-  }
+  if (status != Profile::CREATE_STATUS_CREATED)
+    RecordProfileCreationMetrics(status);
 
   switch (status) {
     case Profile::CREATE_STATUS_LOCAL_FAIL: {
@@ -1233,11 +1240,9 @@ void BrowserOptionsHandler::ShowProfileCreationFeedback(
       }
       break;
     }
-    case Profile::CREATE_STATUS_CANCELED: {
-      profile_path_being_created_.clear();
-      DeleteProfileAtPath(profile->GetPath());
-      break;
-    }
+    // User-initiated cancellation is handled in CancelProfileRegistration and
+    // does not call this callback.
+    case Profile::CREATE_STATUS_CANCELED:
     case Profile::MAX_CREATE_STATUS: {
       NOTREACHED();
       break;
@@ -1270,30 +1275,16 @@ void BrowserOptionsHandler::DeleteProfileAtPath(base::FilePath file_path) {
 
   ProfileMetrics::LogProfileDeleteUser(ProfileMetrics::PROFILE_DELETED);
 
-  Browser* browser =
-      chrome::FindBrowserWithWebContents(web_ui()->GetWebContents());
-  chrome::HostDesktopType desktop_type = chrome::HOST_DESKTOP_TYPE_NATIVE;
-  if (browser)
-    desktop_type = browser->host_desktop_type();
-
   g_browser_process->profile_manager()->ScheduleProfileForDeletion(
       file_path,
-      base::Bind(&OpenNewWindowForProfile, desktop_type));
+      base::Bind(&OpenNewWindowForProfile, GetDesktopType()));
 }
 
 void BrowserOptionsHandler::HandleCancelProfileCreation(const ListValue* args) {
-  CancelProfileCreation();
-
-  UMA_HISTOGRAM_CUSTOM_TIMES("Profile.CreateTimeCanceled",
-      base::TimeTicks::Now() - profile_creation_start_time_,
-      base::TimeDelta::FromMilliseconds(1),
-      base::TimeDelta::FromSeconds(30),  // From kRegistrationTimeoutMS.
-      100);
+  CancelProfileRegistration(true);
 }
 
-void BrowserOptionsHandler::CancelProfileCreation() {
-  // UI code may call this any time "Cancel" is clicked in the dialog,
-  // whether profile creation is in progress or not.
+void BrowserOptionsHandler::CancelProfileRegistration(bool user_initiated) {
   if (profile_path_being_created_.empty())
     return;
 
@@ -1302,18 +1293,30 @@ void BrowserOptionsHandler::CancelProfileCreation() {
   if (!new_profile)
     return;
 
-  // Non-managed user creation cannot be cancelled. (Creating a non-managed
+  // Non-managed user creation cannot be canceled. (Creating a non-managed
   // profile shouldn't take significant time, and it can easily be deleted
   // afterward.)
   if (!ManagedUserService::ProfileIsManaged(new_profile))
     return;
 
+  if (user_initiated) {
+    UMA_HISTOGRAM_CUSTOM_TIMES("Profile.CreateTimeCanceled",
+      base::TimeTicks::Now() - profile_creation_start_time_,
+      base::TimeDelta::FromMilliseconds(1),
+      base::TimeDelta::FromSeconds(30),  // From kRegistrationTimeoutMS.
+      100);
+    RecordProfileCreationMetrics(Profile::CREATE_STATUS_CANCELED);
+  }
+
   ManagedUserRegistrationService* registration_service =
       ManagedUserRegistrationServiceFactory::GetForProfile(
           Profile::FromWebUI(web_ui()));
-  // The ManagedUserRegistrationService will send a cancellation error to the
-  // callback provided in CreateProfile, i.e. to ShowProfileCreationFeedback.
   registration_service->CancelPendingRegistration();
+
+  // Cancelling registration means the callback passed into
+  // RegisterAndInitSync() won't be called, so the cleanup must be done here.
+  profile_path_being_created_.clear();
+  DeleteProfileAtPath(new_profile->GetPath());
 }
 
 void BrowserOptionsHandler::ObserveThemeChanged() {
diff --git a/chrome/browser/ui/webui/options/browser_options_handler.h b/chrome/browser/ui/webui/options/browser_options_handler.h
index c7041fd..acc81cf 100644
--- a/chrome/browser/ui/webui/options/browser_options_handler.h
+++ b/chrome/browser/ui/webui/options/browser_options_handler.h
@@ -145,6 +145,9 @@ class BrowserOptionsHandler
   // Sends an array of Profile objects to javascript.
   void SendProfilesInfo();
 
+  // Returns the current desktop type.
+  chrome::HostDesktopType GetDesktopType();
+
   // Asynchronously creates and initializes a new profile.
   // The arguments are as follows:
   //   0: name (string)
@@ -163,6 +166,9 @@ class BrowserOptionsHandler
                               Profile* new_profile,
                               Profile::CreateStatus status);
 
+  // Records UMA histograms relevant to profile creation.
+  void RecordProfileCreationMetrics(Profile::CreateStatus status);
+
   // Updates the UI as the final task after a new profile has been created.
   void ShowProfileCreationFeedback(
       chrome::HostDesktopType desktop_type,
@@ -179,15 +185,17 @@ class BrowserOptionsHandler
 
   // Cancels creation of a managed-user profile currently in progress, as
   // indicated by profile_path_being_created_, removing the object and files
-  // and canceling managed-user registration. This should be called from JS, to
-  // ensure that the profile-creation dialog is properly updated. |args| is not
-  // used.
+  // and canceling managed-user registration. This is the handler for the
+  // "cancelCreateProfile" message. |args| is not used.
   // TODO(pamg): Move all the profile-handling methods into a more appropriate
   // class.
   void HandleCancelProfileCreation(const base::ListValue* args);
 
-  // Internal implementation.
-  void CancelProfileCreation();
+  // Internal implementation. This may safely be called whether profile creation
+  // or registration is in progress or not. |user_initiated| should be true if
+  // the cancellation was deliberately requested by the user, and false if it
+  // was caused implicitly, e.g. by shutting down the browser.
+  void CancelProfileRegistration(bool user_initiated);
 
   void ObserveThemeChanged();
   void ThemesReset(const base::ListValue* args);