Fix IsPrecachingAllowed() to check Sync instead of Data Saver.

components/precache/content/precache_manager.cc

 // Copyright 2013 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 "components/precache/content/precache_manager.h"
 
 #include <string>
 #include <utility>
 #include <vector>
 
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/files/file_path.h"
 #include "base/logging.h"
 #include "base/metrics/field_trial.h"
 #include "base/prefs/pref_service.h"
 #include "base/time/time.h"
-#include "components/data_reduction_proxy/core/common/data_reduction_proxy_pref_names.h"
 #include "components/history/core/browser/history_service.h"
 #include "components/precache/core/precache_database.h"
 #include "components/precache/core/precache_switches.h"
-#include "components/user_prefs/user_prefs.h"
+#include "components/sync_driver/sync_service.h"
 #include "components/variations/variations_associated_data.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
 #include "net/base/network_change_notifier.h"
 
 using content::BrowserThread;
 
 namespace {
 
 const char kPrecacheFieldTrialName[] = "Precache";
 const char kPrecacheFieldTrialEnabledGroup[] = "Enabled";
 const char kManifestURLPrefixParam[] = "manifest_url_prefix";
 const int kNumTopHosts = 100;
 
 }  // namespace
 
 namespace precache {
 
 int NumTopHosts() {
   return kNumTopHosts;
 }
 
-PrecacheManager::PrecacheManager(content::BrowserContext* browser_context)
+PrecacheManager::PrecacheManager(content::BrowserContext* browser_context,
+                                 sync_driver::SyncService* sync_service)
     : browser_context_(browser_context),
+      sync_service_(sync_service),
       precache_database_(new PrecacheDatabase()),
       is_precaching_(false) {
   base::FilePath db_path(browser_context_->GetPath().Append(
       base::FilePath(FILE_PATH_LITERAL("PrecacheDatabase"))));
 
   BrowserThread::PostTask(
       BrowserThread::DB, FROM_HERE,
       base::Bind(base::IgnoreResult(&PrecacheDatabase::Init),
                  precache_database_, db_path));
 }
 
 PrecacheManager::~PrecacheManager() {}
 
 // static
 bool PrecacheManager::IsPrecachingEnabled() {
   return base::FieldTrialList::FindFullName(kPrecacheFieldTrialName) ==
              kPrecacheFieldTrialEnabledGroup ||
          base::CommandLine::ForCurrentProcess()->HasSwitch(
              switches::kEnablePrecache);
 }
 
 bool PrecacheManager::IsPrecachingAllowed() {
-  DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  return user_prefs::UserPrefs::Get(browser_context_)->GetBoolean(
-      data_reduction_proxy::prefs::kDataReductionProxyEnabled);
+  return sync_service_ && sync_service_->CanSyncStart() &&
+         sync_service_->HasSyncSetupCompleted() &&
+         sync_service_->GetPreferredDataTypes().Has(syncer::SESSIONS) &&
+         !sync_service_->GetEncryptedDataTypes().Has(syncer::SESSIONS);

[Nicolas Zea, 2015/06/18 18:25:48]

One correction (which I think was overlooked in the email):
GetEncryptedDataTypes will return the minimal set (just passwords) if sync's
backend hasn't initialized yet, and will also not take into account any pending
configuration changes (that haven't been finalized). Given the goal of this, I
think it's safer to ensure sync's backend is loaded and check
EncryptEverythingEnabled.

As such, the condition should be (instead of GetEncryptedDataTypes):
sync_service->backend_initialized() && !sync_service->EncryptEverythingEnabled()

It's yet uglier, but this is starting to dig into the internal of sync state,
which has to be concerned with transient issues. Really, the safest approach
would be the one that checks runtime conditions rather than preferences:
sync_service && sync_service->GetActiveDataTypes().Has(syncer::SESSIONS) &&
!sync_service->EncryptEverythingEnabled().

This does have the effect of meaning precaching would be disabled during the
time sync hasn't yet loaded or is shutting down, but from a privacy standpoint
I'm guessing that's okay?

 }
 
 void PrecacheManager::StartPrecaching(
     const PrecacheCompletionCallback& precache_completion_callback,
     const history::HistoryService& history_service) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   if (is_precaching_) {
     DLOG(WARNING) << "Cannot start precaching because precaching is already "
                      "in progress.";
@@ skipping 105 matching lines @@
   // Start precaching.
   precache_fetcher_.reset(
       new PrecacheFetcher(hosts, browser_context_->GetRequestContext(),
                           variations::GetVariationParamValue(
                               kPrecacheFieldTrialName, kManifestURLPrefixParam),
                           this));
   precache_fetcher_->Start();
 }
 
 }  // namespace precache