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

chrome/browser/sync/profile_sync_service.cc

Index: chrome/browser/sync/profile_sync_service.cc
diff --git a/chrome/browser/sync/profile_sync_service.cc b/chrome/browser/sync/profile_sync_service.cc
index 5f428119fbf24d09b51683a0b796e1789c4f7093..c2b5949679bef1418b9edd00327b215956d8085c 100644
--- a/chrome/browser/sync/profile_sync_service.cc
+++ b/chrome/browser/sync/profile_sync_service.cc
@@ -146,7 +146,7 @@ bool ProfileSyncService::AreCredentialsAvailable() {
   }
 
   // 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(
@@ -176,17 +176,7 @@ void ProfileSyncService::Initialize() {
   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();
 }
@@ -279,7 +269,8 @@ void ProfileSyncService::InitSettings() {
 
 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(
@@ -1065,22 +1056,11 @@ void ProfileSyncService::OnUserSubmittedAuth(
 
   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);
@@ -1450,12 +1430,13 @@ void ProfileSyncService::Observe(int type,
       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;
     }
@@ -1519,8 +1500,9 @@ void ProfileSyncService::UnsuppressAndStart() {
   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();
 }