Install default apps if sync is disabled.

chrome/browser/extensions/external_pref_loader.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/extensions/external_pref_loader.h"
 
 #include "base/bind.h"
 #include "base/files/file_enumerator.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/json/json_file_value_serializer.h"
 #include "base/json/json_string_value_serializer.h"
 #include "base/logging.h"
 #include "base/metrics/histogram.h"
 #include "base/path_service.h"
 #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"
 
 using content::BrowserThread;
 
 namespace {
 
 base::FilePath::CharType kExternalExtensionJson[] =
     FILE_PATH_LITERAL("external_extensions.json");
 
@@ skipping 83 matching lines @@
 }
 
 void ExternalPrefLoader::StartLoading() {
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   if (options_ & DELAY_LOAD_UNTIL_PRIORITY_SYNC) {
     if (!PostLoadIfPrioritySyncReady()) {
       DCHECK(profile_);
       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(
         BrowserThread::FILE, FROM_HERE,
         base::Bind(&ExternalPrefLoader::LoadOnFileThread, this));
   }
 }
 
 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_);
 
   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));
 
   scoped_ptr<base::DictionaryValue> prefs(new base::DictionaryValue);
 
   // TODO(skerner): Some values of base_path_id_ will cause
   // PathService::Get() to return false, because the path does
   // not exist.  Find and fix the build/install scripts so that
   // this can become a CHECK().  Known examples include chrome
   // OS developer builds and linux install packages.
@@ skipping 123 matching lines @@
   LoadFinished();
 }
 
 ExternalTestingLoader::~ExternalTestingLoader() {}
 
 const base::FilePath ExternalTestingLoader::GetBaseCrxFilePath() {
   return fake_base_path_;
 }
 
 }  // namespace extensions