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 e14767a7f8dbe51d66065e879a3f1adc4ae6934e..ef6aeb4e8a0ea74d5f2ef7065cfb5539d4dc619c 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() ?

[Andrew T Wilson (Slow), 2011/12/06 21:09:15]

BTW, is the plan to eventually get rid of cros_user_ and instead move the
username for cros into SigninManager? I ask, because as we move the signin code
out of sync it would be nice for there to be one place that can authoritatively
say whether there's a signed-in user or not.

[tim (not reviewing), 2011/12/06 21:23:47]

As I mention in the CL description and in my first comment response (and there's
a blurb in the notes from yday's meeting too), I'm working towards removing
cros_user :) That (and having one single place store username info) is the sole
purpose for this CL.  I have a patch that I haven't sent up for review (because
it's based on this and my earlier removal of GetAuthenticatedUsername from PSS
so the diffs are messy) that removes cros_user, and distinguishes more
explicitly between auto-starting and manual-starting the service.

+      signin_->GetAuthenticatedUsername() : cros_user_;
   DCHECK(!credentials.email.empty());
   TokenService* service = profile_->GetTokenService();
   credentials.sync_token = service->GetTokenForService(
@@ -1073,22 +1064,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);
@@ -1458,12 +1438,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/12/06 21:09:15]

Agreed with Lingesh - can you explain why the UI is going to display an error in
this case? Does the UI someplace check for AreCredentialsAvailable() rather than
just last_auth_error?

[tim (not reviewing), 2011/12/06 21:23:47]

Woops, I musn't have uploaded the right patchset. I addressed this, will
reupload shortly.

       }
       break;
     }
@@ -1527,8 +1508,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();
 }