sync: change semantics (and name) of SigninManager::GetUsername

chrome/browser/sync/profile_sync_service.cc

 // Copyright (c) 2011 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/sync/profile_sync_service.h"
 
 #include <algorithm>
 #include <cstddef>
 #include <map>
 #include <set>
@@ skipping 128 matching lines @@
   sync_prefs_.RemoveSyncPrefObserver(this);
   Shutdown(false);
 }
 
 bool ProfileSyncService::AreCredentialsAvailable() {
   if (IsManaged()) {
     return false;
   }
 
   // CrOS user is always logged in. Chrome uses signin_ to check logged in.
-  if (!cros_user_.empty() || !signin_->GetUsername().empty()) {
+  if (!cros_user_.empty() || !signin_->GetAuthenticatedUsername().empty()) {
     // TODO(chron): Verify CrOS unit test behavior.
     return profile()->GetTokenService() &&
         profile()->GetTokenService()->HasTokenForService(
             browser_sync::SyncServiceName());
   }
   return false;
 }
 
 void ProfileSyncService::Initialize() {
   InitSettings();
 
   // We clear this here (vs Shutdown) because we want to remember that an error
   // happened on shutdown so we can display details (message, location) about it
   // in about:sync.
   ClearStaleErrors();
 
   sync_prefs_.AddSyncPrefObserver(this);
 
   // For now, the only thing we can do through policy is to turn sync off.
   if (IsManaged()) {
     DisableForUser();
     return;
   }
 
   RegisterAuthNotifications();
 
   if (!HasSyncSetupCompleted())
     DisableForUser();  // Clean up in case of previous crash / setup abort.
 
-  // In Chrome, we integrate a SigninManager which works with the sync
-  // setup wizard to kick off the TokenService. CrOS does its own plumbing
-  // for the TokenService in login and does not normally rely on signin_,
-  // so only initialize this if the token service has not been initialized
-  // (e.g. the browser crashed or is being debugged).
-  if (cros_user_.empty() ||
-      !profile_->GetTokenService()->Initialized()) {
-    // Will load tokens from DB and broadcast Token events after.
-    // Note: We rely on signin_ != NULL unless !cros_user_.empty().
-    signin_->Initialize(profile_);
-  }
+  signin_->Initialize(profile_);
 
   TryStart();
 }
 
 void ProfileSyncService::TryStart() {
   if (!sync_prefs_.IsStartSuppressed() && AreCredentialsAvailable()) {
     if (HasSyncSetupCompleted() || auto_start_enabled_) {
       // If sync setup has completed we always start the backend.
       // If autostart is enabled, but we haven't completed sync setup, we try to
       // start sync anyway, since it's possible we crashed/shutdown after
@@ skipping 72 matching lines @@
       } else {
         LOG(WARNING) << "The following sync URL specified at the command-line "
                      << "is invalid: " << value;
       }
     }
   }
 }
 
 SyncCredentials ProfileSyncService::GetCredentials() {
   SyncCredentials credentials;
-  credentials.email = cros_user_.empty() ? signin_->GetUsername() : cros_user_;
+  credentials.email = cros_user_.empty() ?
+      signin_->GetAuthenticatedUsername() : cros_user_;
   DCHECK(!credentials.email.empty());
   TokenService* service = profile_->GetTokenService();
   credentials.sync_token = service->GetTokenForService(
       browser_sync::SyncServiceName());
   return credentials;
 }
 
 void ProfileSyncService::InitializeBackend(bool delete_stale_data) {
   if (!backend_.get()) {
     NOTREACHED();
@@ skipping 765 matching lines @@
 
 void ProfileSyncService::OnUserSubmittedAuth(
     const std::string& username, const std::string& password,
     const std::string& captcha, const std::string& access_code) {
   last_attempted_user_email_ = username;
   is_auth_in_progress_ = true;
   NotifyObservers();
 
   auth_start_time_ = base::TimeTicks::Now();
 
-  if (!signin_->IsInitialized()) {
-    // In ChromeOS we sign in during login, so we do not initialize signin_.
-    // If this function gets called, we need to re-authenticate (e.g. for
-    // two factor signin), so initialize signin_ here.
-    signin_->Initialize(profile_);
-  }
-
   if (!access_code.empty()) {
     signin_->ProvideSecondFactorAccessCode(access_code);
     return;
   }
 
-  if (!signin_->GetUsername().empty()) {
-    signin_->ClearInMemoryData();
-  }
-
   // The user has submitted credentials, which indicates they don't
   // want to suppress start up anymore.
   sync_prefs_.SetStartSuppressed(false);
 
   signin_->StartSignIn(username,
                        password,
                        last_auth_error_.captcha().token,
                        captcha);
 }
 
@@ skipping 349 matching lines @@
       if (AreCredentialsAvailable()) {
         if (backend_initialized_) {
           backend_->UpdateCredentials(GetCredentials());
         }
         if (!sync_prefs_.IsStartSuppressed())
           StartUp();
       }
       break;
     }
     case chrome::NOTIFICATION_TOKEN_LOADING_FINISHED: {
-      // If not in Chrome OS, and we have a username without tokens,
-      // the user will need to signin again, so sign out.
+      // If not in auto-start / Chrome OS mode, and we have a username
+      // without tokens, the user will need to signin again. NotifyObservers to
+      // trigger errors in the UI that will allow the user to re-login.
       if (cros_user_.empty() &&
-          !signin_->GetUsername().empty() &&
+          !signin_->GetAuthenticatedUsername().empty() &&
           !AreCredentialsAvailable()) {
-        DisableForUser();
+        NotifyObservers();

[Andrew T Wilson (Slow), 2011/11/28 04:54:02]

I'm not entirely sure why it's now OK to NotifyObservers() here and not actually
force a sign out? This seems to make sense (don't sign out the user
unnecessarily), but I wonder why we were doing this before?

[tim (not reviewing), 2011/12/05 21:24:50]

I'm in the same boat here, honestly. In fact I'm not sure why we need to listen
to TOKEN_LOADING_FINISHED. This was added ages ago though and seems to be
directly at odds with / not considered by Lingesh's recent fix
(http://codereview.chromium.org/8515030).  Hence why I was looking for his
comment.  We can leave it, but I just feel like if we don't understand it I'd
rather show an error and kill sync and leave the user in the "what the, sync
just disabled itself" state.

[lipalani1, 2011/12/05 21:40:44]

I assumed this meant the following:
If we have an incomplete tokens cache then on restart simply disable sync.

Although it is pretty unlikely we have the user name set but missing the
tokens(only thing that i could think of is the tokens db has different flushing
to disk behavior from the preferences db. in which case one might have made it
to disk and another still in memory. but this is far fetched).

Regardless of that in the unlikely event this happens we need to handle it.
Thats why it looked fine for me to leave it as such.

Also NotifyObservers here likely wont do any thing. Because last_auth_error
would be set to none so would not display the UI. Am I missing something here?

On 2011/12/05 21:24:50, timsteele wrote:

       }
       break;
     }
     default: {
       NOTREACHED();
     }
   }
 }
 
 void ProfileSyncService::AddObserver(Observer* observer) {
@@ skipping 43 matching lines @@
 void ProfileSyncService::StopAndSuppress() {
   sync_prefs_.SetStartSuppressed(true);
   Shutdown(false);
 }
 
 void ProfileSyncService::UnsuppressAndStart() {
   DCHECK(profile_);
   sync_prefs_.SetStartSuppressed(false);
   // Set username in SigninManager, as SigninManager::OnGetUserInfoSuccess
   // is never called for some clients.
-  if (signin_->GetUsername().empty()) {
-    signin_->SetUsername(sync_prefs_.GetGoogleServicesUsername());
+  if (signin_->GetAuthenticatedUsername().empty()) {
+    signin_->SetAuthenticatedUsername(
+        sync_prefs_.GetGoogleServicesUsername());
   }
   TryStart();
 }
 
 void ProfileSyncService::AcknowledgeSyncedTypes() {
   syncable::ModelTypeSet registered_types;
   GetRegisteredDataTypes(&registered_types);
   sync_prefs_.AcknowledgeSyncedTypes(registered_types);
 }
 
@@ skipping 15 matching lines @@
             << "Unrecoverable error.";
   } else {
     VLOG(0) << "ConfigureDataTypeManager not invoked because backend is not "
             << "initialized";
   }
 }
 
 const FailedDatatypesHandler& ProfileSyncService::failed_datatypes_handler() {
   return failed_datatypes_handler_;
 }