[sync] Automatic bootstrapping of Sync on Win 8 from cached credentials

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 7fa28b89d5b21eead6ff2fe2cc757b6a56e3e707..c6ccff575f5ae3610302a4341ebee056efa45836 100644
--- a/chrome/browser/sync/profile_sync_service.cc
+++ b/chrome/browser/sync/profile_sync_service.cc
@@ -21,6 +21,9 @@
 #include "base/string16.h"
 #include "base/stringprintf.h"
 #include "base/threading/thread_restrictions.h"
+#if defined(OS_WIN)
+#include "base/win/windows_version.h"
+#endif
 #include "chrome/browser/about_flags.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/defaults.h"
@@ -31,6 +34,8 @@
 #include "chrome/browser/signin/token_service.h"
 #include "chrome/browser/signin/token_service_factory.h"
 #include "chrome/browser/sync/backend_migrator.h"
+#include "chrome/browser/sync/credential_cache_service_win.h"
+#include "chrome/browser/sync/credential_cache_service_factory_win.h"
 #include "chrome/browser/sync/glue/change_processor.h"
 #include "chrome/browser/sync/glue/chrome_encryptor.h"
 #include "chrome/browser/sync/glue/chrome_report_unrecoverable_error.h"
@@ -139,7 +144,8 @@ ProfileSyncService::ProfileSyncService(ProfileSyncComponentsFactory* factory,
       auto_start_enabled_(start_behavior == AUTO_START),
       failed_datatypes_handler_(ALLOW_THIS_IN_INITIALIZER_LIST(this)),
       configure_status_(DataTypeManager::UNKNOWN),
-      setup_in_progress_(false) {
+      setup_in_progress_(false),
+      credential_cache_service_(NULL) {
 #if defined(OS_ANDROID)
   chrome::VersionInfo version_info;
   if (version_info.IsOfficialBuild()) {
@@ -208,7 +214,41 @@ void ProfileSyncService::Initialize() {
   TryStart();
 }
 
+bool ProfileSyncService::ShouldInitializeCredentialCacheService() {
+#if defined(OS_WIN)
+  return
+      base::win::GetVersion() >= base::win::VERSION_WIN8 &&

[Andrew T Wilson (Slow), 2012/07/20 01:04:05]

This seems like a non-optimal pattern - why not just have this routine only be
defined on OS_WIN (why do we need it on other platforms at all?).

[Raghu Simha, 2012/07/20 23:35:22]

I did this based on Roger's comment that we shouldn't conditionally define
methods for only some datatypes. But on taking a look around the codebase, I
notice other places where this is done. I've cleaned things up a little bit.

+      syncer::CredentialCacheService::IsDefaultProfileDir(
+          profile()->GetPath()) &&
+      !IsManaged() &&
+      !sync_prefs_.IsStartSuppressed();
+#else
+  return false;
+#endif
+}
+
+bool ProfileSyncService::LoadedCachedCredentials() {
+#if defined(OS_WIN)
+  return (credential_cache_service_ &&
+          credential_cache_service_->SuccessfullyLoadedCredentials());
+#else
+  return false;
+#endif
+}
+
 void ProfileSyncService::TryStart() {
+#if defined(OS_WIN)
+  if (ShouldInitializeCredentialCacheService()) {

[Andrew T Wilson (Slow), 2012/07/20 01:04:05]

I'm not clear on why we need to do this initialization - why doesn't the
CredentialCacheService self-initialize on startup?

[Raghu Simha, 2012/07/20 23:35:22]

At first, I thought we would not initialize the credential cache service if sync
was disabled or suppressed. I suppose this logic lives in the PSS startup code
anyway, so I might as well move the initialization there.

+    // Lazily instantiate the credential cache service if required.
+    credential_cache_service_ =
+        CredentialCacheServiceFactory::GetInstance()->GetForProfile(profile_);
+    if (!setup_in_progress_ && signin_->GetAuthenticatedUsername().empty()) {
+      credential_cache_service_->LookForCachedCredentialsInAlternateProfile();
+    }
+    return;
+  }
+#endif
+
   if (!IsSyncEnabledAndLoggedIn())
     return;
   TokenService* token_service = TokenServiceFactory::GetForProfile(profile_);
@@ -696,10 +736,12 @@ void ProfileSyncService::OnBackendInitialized(
   }
   NotifyObservers();
 
-  if (auto_start_enabled_ && !FirstSetupInProgress()) {
-    // Backend is initialized but we're not in sync setup, so this must be an
-    // autostart - mark our sync setup as completed and we'll start syncing
-    // below.
+  if ((auto_start_enabled_ && !FirstSetupInProgress()) ||
+      LoadedCachedCredentials()) {

[Andrew T Wilson (Slow), 2012/07/20 01:04:05]

I think a better way to do this would be for the CCS to set
SyncSetupCompleted=true when it loads cached credentials, then we don't need
this logic here.

[Raghu Simha, 2012/07/20 23:35:22]

Agreed. Done.

+    // If one of following is true, mark setup as completed and start syncing.
+    // 1) Backend is initialized but we're not in sync setup, so this must be an
+    //    autostart.
+    // 2) We are starting up using cached credentials.
     SetSyncSetupCompleted();
     NotifyObservers();
   }