Install default apps if sync is disabled.

chrome/browser/extensions/external_pref_loader.cc

Index: chrome/browser/extensions/external_pref_loader.cc
diff --git a/chrome/browser/extensions/external_pref_loader.cc b/chrome/browser/extensions/external_pref_loader.cc
index 61be1597a954c58f24198a971bfc220a8f3c3cf8..4c1ce8dac429a02ce448170aa1b1f0f2c470374b 100644
--- a/chrome/browser/extensions/external_pref_loader.cc
+++ b/chrome/browser/extensions/external_pref_loader.cc
@@ -16,6 +16,7 @@
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/prefs/pref_service_syncable.h"
+#include "chrome/browser/sync/profile_sync_service_factory.h"
 #include "chrome/common/chrome_paths.h"
 #include "content/public/browser/browser_thread.h"
 
@@ -119,6 +120,12 @@ void ExternalPrefLoader::StartLoading() {
       PrefServiceSyncable* prefs = PrefServiceSyncable::FromProfile(profile_);
       DCHECK(prefs);
       syncable_pref_observer_.Add(prefs);
+      ProfileSyncService* service =
+          ProfileSyncServiceFactory::GetForProfile(profile_);
+      if (service)

[Nicolas Zea, 2015/02/26 21:46:47]

I don't think this will catch the case where sync is disabled at startup time,
i.e. if the user has never enabled sync. This is where I think you should be
checking IsSyncEnabledAndLoggedIn && HasSyncSetupCompleted, before registering
any observers. If those two are not true, sync will not be running, at least at
startup, and will only trigger observer if it's enabled (which is not guaranteed
to happen at all).

+        service->AddObserver(this);
+      else
+        PostLoadAndRemoveObservers();
     }
   } else {
     BrowserThread::PostTask(
@@ -131,6 +138,19 @@ void ExternalPrefLoader::OnIsSyncingChanged() {
   PostLoadIfPrioritySyncReady();
 }
 
+void ExternalPrefLoader::OnStateChanged() {
+  ProfileSyncService* service =
+      ProfileSyncServiceFactory::GetForProfile(profile_);
+  DCHECK(service);
+
+  ProfileSyncService::SyncStatusSummary status =
+      service->QuerySyncStatusSummary();
+  if (status == ProfileSyncService::SyncStatusSummary::NOT_ENABLED ||

[Nicolas Zea, 2015/02/25 18:26:14]

Do you have logic elsewhere to do the load when sync finishes? Or is the idea to
not do the load at all if sync is running?

Either way, this enum is primarily for debugging UI, not so much knowing sync
internal state. IsSyncEnabledAndLoggedIn() && HasSyncSetupCompleted() would be
better, and it also not transient based on the state of the backend (which can
have its startup delayed by up to 11 seconds).

[Ivan Podogov, 2015/02/26 12:50:15]

The main idea is: if sync is enabled (and we don't know that until the sync
service connects to the server, thus IsSyncEnabled won't do the trick, but
IsSyncEnabledAndLoggedIn does), then we must wait until priority sync is done
(that case is handled in OnIsSyncingChanged function above, and
HasSyncSetupCompleted is simply not sufficient in this case, we must wait a bit
more than that), otherwise we will catch the "sync disabled" server response
here. When the sync is disabled on the device itself, we will never reach this
function, so that is covered as well.

[Nicolas Zea, 2015/02/26 21:46:47]

I'm a bit confused by this; I think we might be thinking of "enabled" in two
different ways. You do know if sync is currently enabled without hitting the
server, in the sense that you know whether sync is going to attempt to start up
and sync. (see my comment above)

That said, sync can, effectively at any time, hit a "permanent error" which will
subsequently disable sync. Registering to listen to those events is fine.
IsSyncEnabledAndLoggedIn will return false once we've hit a permanent error.

Does that make things clearer?

[Ivan Podogov, 2015/02/26 21:59:06]

Um... These comments are for the old patchset, take a look at the last one (from
9 hours ago), please.

+      status == ProfileSyncService::SyncStatusSummary::UNRECOVERABLE_ERROR) {
+    PostLoadAndRemoveObservers();
+  }
+}
+
 bool ExternalPrefLoader::PostLoadIfPrioritySyncReady() {
   DCHECK(options_ & DELAY_LOAD_UNTIL_PRIORITY_SYNC);
   DCHECK(profile_);
@@ -138,16 +158,28 @@ bool ExternalPrefLoader::PostLoadIfPrioritySyncReady() {
   PrefServiceSyncable* prefs = PrefServiceSyncable::FromProfile(profile_);
   DCHECK(prefs);
   if (prefs->IsPrioritySyncing()) {
-    BrowserThread::PostTask(
-        BrowserThread::FILE, FROM_HERE,
-        base::Bind(&ExternalPrefLoader::LoadOnFileThread, this));
-    syncable_pref_observer_.Remove(prefs);
+    PostLoadAndRemoveObservers();
     return true;
   }
 
   return false;
 }
 
+void ExternalPrefLoader::PostLoadAndRemoveObservers() {
+  BrowserThread::PostTask(
+      BrowserThread::FILE, FROM_HERE,
+      base::Bind(&ExternalPrefLoader::LoadOnFileThread, this));
+
+  PrefServiceSyncable* prefs = PrefServiceSyncable::FromProfile(profile_);
+  DCHECK(prefs);
+  syncable_pref_observer_.Remove(prefs);
+
+  ProfileSyncService* service =
+      ProfileSyncServiceFactory::GetForProfile(profile_);
+  if (service)
+    service->RemoveObserver(this);
+}
+
 void ExternalPrefLoader::LoadOnFileThread() {
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));