[sync] Separate sign in from sync on Desktop Chrome

chrome/browser/sync/sync_ui_util.cc

 // Copyright (c) 2012 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/sync_ui_util.h"
 
 #include "base/i18n/number_formatting.h"
 #include "base/i18n/time_formatting.h"
 #include "base/prefs/pref_service.h"
 #include "base/string_util.h"
@@ skipping 37 matching lines @@
                                    const SigninManagerBase& signin,
                                    StatusLabelStyle style) {
   string16 user_name = UTF8ToUTF16(signin.GetAuthenticatedUsername());
 
   if (!user_name.empty()) {
     if (!service || service->IsManaged()) {
       // User is signed in, but sync is disabled.
       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);
+      }
     }
   }
 
   if (!service || !service->sync_initialized()) {
     // User is not signed in, or sync is still initializing.
     return string16();
   }
 
   DCHECK(!user_name.empty());
 
@@ skipping 149 matching lines @@
         status_label->assign(
             l10n_util::GetStringUTF16(IDS_SYNC_NTP_SETUP_IN_PROGRESS));
       }
       if (signin.AuthInProgress()) {
         if (status_label) {
           status_label->assign(
               l10n_util::GetStringUTF16(IDS_SYNC_AUTHENTICATING_LABEL));
         }
       } 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;
       }
     } else if (service->HasUnrecoverableError()) {
       result_type = SYNC_ERROR;
       ProfileSyncService::Status status;
       service->QueryDetailedSyncStatus(&status);
       if (ShouldShowActionOnUI(status.sync_protocol_error)) {
         if (status_label) {
           GetStatusForActionableError(status.sync_protocol_error,
               status_label);
         }
       } 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;
 }
 
 // Returns the status info for use on the new tab page, where we want slightly
 // different information than in the settings panel.
 MessageType GetStatusInfoForNewTabPage(ProfileSyncService* service,
                                        const SigninManagerBase& signin,
                                        string16* status_label,
@@ skipping 92 matching lines @@
 string16 ConstructTime(int64 time_in_int) {
   base::Time time = base::Time::FromInternalValue(time_in_int);
 
   // If time is null the format function returns a time in 1969.
   if (time.is_null())
     return string16();
   return base::TimeFormatFriendlyDateAndTime(time);
 }
 
 }  // namespace sync_ui_util