[sync] Separate sign in from sync on Desktop Chrome

chrome/browser/sync/sync_ui_util.cc

Index: chrome/browser/sync/sync_ui_util.cc
diff --git a/chrome/browser/sync/sync_ui_util.cc b/chrome/browser/sync/sync_ui_util.cc
index a8cd618e63474294d89b65926ded4a262712aacf..1f84317a58ef1acaf06b65a166400ad6922f6d5b 100644
--- a/chrome/browser/sync/sync_ui_util.cc
+++ b/chrome/browser/sync/sync_ui_util.cc
@@ -55,8 +55,15 @@ string16 GetSyncedStateStatusLabel(ProfileSyncService* service,
       return l10n_util::GetStringFUTF16(IDS_SIGNED_IN_WITH_SYNC_DISABLED,
                                         user_name);
     } else if (service->IsStartSuppressed()) {
-      return l10n_util::GetStringFUTF16(IDS_SIGNED_IN_WITH_SYNC_SUPPRESSED,
-                                        user_name);
+      ProfileSyncService::Status status;
+      service->QueryDetailedSyncStatus(&status);
+      if (status.sync_protocol_error.error_type == syncer::NOT_MY_BIRTHDAY) {

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

If you clear the sync dashboard, then restart chrome, will the error_type still
be set to NOT_MY_BIRTHDAY? I'm just surprised that we would persist this across
restarts.

If it's not persisted, then there's not really any point in differentiating
these two cases - we might as well just display the same string in both cases,
since that's what we'll display on the next restart anyway.

[Raghu Simha, 2013/05/16 02:13:20]

I just checked -- birthday errors are not persisted across restarts on desktop
Chrome or Chrome OS.
I originally kept the two messages around so that if a user cleared their data
via the dashboard while the settings page was visible, they would see a message
that indicated exactly what happened.

I agree with your point though. I've gone ahead and replaced the old "cleared
via dashboard" message with a single "stopped on this device" message.

+        return l10n_util::GetStringFUTF16(IDS_SIGNED_IN_WITH_SYNC_SUPPRESSED,
+                                          user_name);
+      } else {
+        return l10n_util::GetStringFUTF16(IDS_SIGNED_IN_WITH_SYNC_STOPPED,
+                                          user_name);
+      }
     }
   }
 
@@ -226,10 +233,10 @@ MessageType GetStatusInfo(ProfileSyncService* service,
         }
       } else if (auth_error.state() != AuthError::NONE &&
                  auth_error.state() != AuthError::TWO_FACTOR) {
-        if (status_label) {
+        if (status_label && link_label) {

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

Are we sure this is OK? Aren't there callers that pass in only the status_label
(no link_label)? Doesn't this break those callers by not setting the status
label?

[Raghu Simha, 2013/05/16 02:13:20]

Yes, this is okay. There's a check on line 129 that makes sure that if one label
is available, the other one is too.

           status_label->clear();
           signin_ui_util::GetStatusLabelsForAuthError(
-              signin, status_label, NULL);
+              signin, status_label, link_label);
         }
         result_type = SYNC_ERROR;
       }
@@ -245,6 +252,16 @@ MessageType GetStatusInfo(ProfileSyncService* service,
       } else if (status_label) {
         status_label->assign(l10n_util::GetStringUTF16(IDS_SYNC_SETUP_ERROR));
       }
+    } else if (!signin.GetAuthenticatedUsername().empty() &&
+               !service->HasSyncSetupCompleted()) {
+      // The user is signed in, but sync is disabled.

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

Hmmm. Do you need the !service->HasSyncSetupCompleted() here? Isn't the
service->HasSyncSetupCompleted() == true already handled by the if clause
starting on line 136?

Also, peer bonus for the first person to break up this routine - a 150-line if
statement is basically unmaintainable.

[Raghu Simha, 2013/05/16 02:13:20]

You're right. I've removed the redundant check.

+      if (status_label) {
+        string16 label = l10n_util::GetStringFUTF16(
+                             IDS_SIGNED_IN_WITH_SYNC_STOPPED,
+                             UTF8ToUTF16(signin.GetAuthenticatedUsername()));
+        status_label->assign(label);
+        result_type = PRE_SYNCED;
+      }
     }
   }
   return result_type;