Client changes for disabled dasher account

chrome/browser/signin/signin_tracker.cc

Index: chrome/browser/signin/signin_tracker.cc
diff --git a/chrome/browser/signin/signin_tracker.cc b/chrome/browser/signin/signin_tracker.cc
index cdddc4966590e381a1aeffda7c2072334b4b6b94..42cc54f0e5936712bcf2bcb983b1ca3244b9d4f0 100644
--- a/chrome/browser/signin/signin_tracker.cc
+++ b/chrome/browser/signin/signin_tracker.cc
@@ -171,6 +171,10 @@ bool SigninTracker::AreServicesSignedIn(Profile* profile) {
     return true;
   ProfileSyncService* service =
       ProfileSyncServiceFactory::GetForProfile(profile);
+  // Check if sync disabled for domain flag was received from server. Don't care
+  // about the sync state in this case.
+  if (service->IsManaged())

[Andrew T Wilson (Slow), 2013/05/24 15:13:10]

Rather than making every caller of Profile::IsSyncAccessible() also call
service->IsManaged(), it seems cleaner to change IsSyncAccessible() to do this:

bool Profile::IsSyncAccessible() {
  if (ProfileSyncServiceFactory::HasProfileSyncService(this))
    return !ProfileSyncServiceFactory::GetProfileSyncService(this)->IsManaged();

  // No ProfileSyncService created yet - we don't want to create one, so just
infer the accessible state
  // by looking at prefs/command line flags.
  browser_sync::SyncPrefs prefs(GetPrefs());
  return ProfileSyncService::IsSyncEnabled() && !prefs.IsManaged())
}

[pavely, 2013/05/24 22:17:11]

I tried this in my experiments. The issue is that ProfileSyncService transitions

from accessible to not-accessible state while setup is in progress. 
OneClickSigninSyncStarter calls SetSetupInProgress(true) in the beginning, it 
should call SetSetupInProgress(false) at the end, otherwise after user signs out
settings page will disable button and show that setup is in progress. If
GetProfileSyncService returns NULL for disabled user then 
OneClickSigninSyncStarter won't have PSS pointer to call 
SetSetupInProgress(false). This reminds me that I'm not handling case of user
selecting "Choose what to sync" correctly.
Another solution for this problem is to call SetSetupInProgress(false) when 
I set disabled flag in PSS, but I think it hides dependency in implementation 
that shouldn't be there.

[Andrew T Wilson (Slow), 2013/05/27 15:03:34]

Yeah, that's a funky edge case. You could deal with it by having the code that
calls SetSetupInProgress(false) call ProfileSyncServiceFactory::GetForProfile()
to grab the PSS instead of calling IsProfileAccessible() in that specific case.
It could still return null if sync is disabled via a command-line flag so we
have to check for null.

I think it's better to explicitly handle the case of sync changing its
accessibility than to make everyone manually check the disabled status
separately. I just know from hard experience that if you don't do this, people
will forget to do it in their code and we'll get breakages when people disable
sync on the domain (in the same way that sporadically we get crashes when you
run chrome with --sync-disabled because people forget to check for a null PSS).

+    return true;
   return (service->IsSyncEnabledAndLoggedIn() &&
           service->IsSyncTokenAvailable() &&
           service->GetAuthError().state() == GoogleServiceAuthError::NONE &&
@@ -195,6 +199,8 @@ SigninTracker::LoginState SigninTracker::GetSigninState(
   // also.
   ProfileSyncService* service = profile->IsSyncAccessible() ?
       ProfileSyncServiceFactory::GetForProfile(profile) : NULL;
+  if (service->IsManaged())

[Andrew T Wilson (Slow), 2013/05/24 15:13:10]

See my previous comment - if IsSyncAccessible() calls service->IsManaged() then
we don't have to explicitly call it here.

+    service = NULL;
   if (service && service->waiting_for_auth()) {
     // Still waiting for an auth token to come in so stay in the INITIALIZING
     // state (we do this to avoid triggering an early signin error in the case