Wire NQE Prefs to Profile

chrome/browser/net/nqe/ui_network_quality_estimator_service.cc

Index: chrome/browser/net/nqe/ui_network_quality_estimator_service.cc
diff --git a/chrome/browser/net/nqe/ui_network_quality_estimator_service.cc b/chrome/browser/net/nqe/ui_network_quality_estimator_service.cc
index 87c4a6afb39338ae235989d703ccf75c5d0bd012..688b314e77230f15e2317bdca69f6a57af993a62 100644
--- a/chrome/browser/net/nqe/ui_network_quality_estimator_service.cc
+++ b/chrome/browser/net/nqe/ui_network_quality_estimator_service.cc
@@ -4,10 +4,100 @@
 
 #include "chrome/browser/net/nqe/ui_network_quality_estimator_service.h"
 
+#include <string>
+
 #include "base/bind.h"
+#include "base/metrics/histogram_macros.h"
+#include "base/threading/thread_checker.h"
+#include "base/values.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/io_thread.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/common/pref_names.h"
+#include "components/prefs/pref_registry.h"
+#include "components/prefs/pref_registry_simple.h"
+#include "components/prefs/pref_service.h"
+#include "components/variations/variations_associated_data.h"
 #include "content/public/browser/browser_thread.h"
+#include "net/nqe/network_qualities_prefs_manager.h"
+
+namespace {
+
+// Name of the network quality estimator field trial.
+const char kNetworkQualityEstimatorFieldTrialName[] = "NetworkQualityEstimator";
+
+// Returns the variation value for |parameter_name|. If the value is
+// unavailable, |default_value| is returned.
+std::string GetStringValueForVariationParamWithDefaultValue(
+    const std::string& parameter_name,
+    const std::string& default_value) {
+  std::map<std::string, std::string> network_quality_estimator_params;
+  variations::GetVariationParams(kNetworkQualityEstimatorFieldTrialName,
+                                 &network_quality_estimator_params);
+
+  const auto it = network_quality_estimator_params.find(parameter_name);
+  return it == network_quality_estimator_params.end() ? default_value
+                                                      : it->second;
+}
+
+// Returns true if persistent caching has been enabled in the field trial.
+bool persistent_caching_enabled() {
+  return GetStringValueForVariationParamWithDefaultValue(
+             "persistent_caching_enabled", "false") == "true";
+}
+
+// PrefDelegateImpl writes the provided dictionary value to the network quality
+// estimator prefs on the disk.
+class PrefDelegateImpl
+    : public net::NetworkQualitiesPrefsManager::PrefDelegate {
+ public:
+  // |pref_service| is used to read and write prefs from/to the disk.
+  explicit PrefDelegateImpl(PrefService* pref_service)
+      : pref_service_(pref_service), path_(prefs::kNetworkQualities) {
+    DCHECK(pref_service_);
+  }
+  ~PrefDelegateImpl() override {}
+
+  void SetDictionaryValue(const base::DictionaryValue& value) override {
+    DCHECK(thread_checker_.CalledOnValidThread());
+    if (!persistent_caching_enabled())
+      return;
+
+    pref_service_->Set(path_, value);
+    UMA_HISTOGRAM_COUNTS_1000("NQE.Prefs.WriteCount", 1);
+  }
+
+  const base::DictionaryValue& GetDictionaryValue() override {
+    DCHECK(thread_checker_.CalledOnValidThread());
+    UMA_HISTOGRAM_COUNTS_1000("NQE.Prefs.ReadCount", 1);
+    return *pref_service_->GetDictionary(path_);
+  }
+
+ private:
+  PrefService* pref_service_;
+
+  // |path_| is the location of the network quality estimator prefs.
+  const std::string path_;
+
+  base::ThreadChecker thread_checker_;
+
+  DISALLOW_COPY_AND_ASSIGN(PrefDelegateImpl);
+};
+
+// Initializes |pref_manager| on |io_thread|.
+void SetNQEOnIOThread(net::NetworkQualitiesPrefsManager* prefs_manager,
+                      IOThread* io_thread) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+
+  // Avoid null pointer referencing during browser shutdown.
+  if (!io_thread->globals()->network_quality_estimator)
+    return;
+
+  prefs_manager->InitializeOnNetworkThread(
+      io_thread->globals()->network_quality_estimator.get());
+}
+
+}  // namespace
 
 // A class that sets itself as an observer of the EffectiveconnectionType for
 // the browser IO thread. It reports any change in EffectiveConnectionType back
@@ -16,7 +106,7 @@
 class UINetworkQualityEstimatorService::IONetworkQualityObserver
     : public net::NetworkQualityEstimator::EffectiveConnectionTypeObserver {
  public:
-  IONetworkQualityObserver(
+  explicit IONetworkQualityObserver(
       base::WeakPtr<UINetworkQualityEstimatorService> service)
       : service_(service) {
     DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
@@ -64,21 +154,32 @@ class UINetworkQualityEstimatorService::IONetworkQualityObserver
   DISALLOW_COPY_AND_ASSIGN(IONetworkQualityObserver);
 };
 
-UINetworkQualityEstimatorService::UINetworkQualityEstimatorService()
+UINetworkQualityEstimatorService::UINetworkQualityEstimatorService(
+    Profile* profile)
     : type_(net::EFFECTIVE_CONNECTION_TYPE_UNKNOWN),
       io_observer_(nullptr),
+      prefs_manager_(nullptr),
       weak_factory_(this) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  DCHECK(profile);
   // If this is running in a context without an IOThread, don't try to create
   // the IO object.
   if (!g_browser_process->io_thread())
     return;
   io_observer_ = new IONetworkQualityObserver(weak_factory_.GetWeakPtr());
+  std::unique_ptr<PrefDelegateImpl> pref_delegate(
+      new PrefDelegateImpl(profile->GetPrefs()));
+  prefs_manager_ =
+      new net::NetworkQualitiesPrefsManager(std::move(pref_delegate));
+
   content::BrowserThread::PostTask(
       content::BrowserThread::IO, FROM_HERE,
       base::Bind(&IONetworkQualityObserver::InitializeOnIOThread,
                  base::Unretained(io_observer_),
                  g_browser_process->io_thread()));
+  content::BrowserThread::PostTask(content::BrowserThread::IO, FROM_HERE,
+                                   base::Bind(&SetNQEOnIOThread, prefs_manager_,
+                                              g_browser_process->io_thread()));
 }
 
 UINetworkQualityEstimatorService::~UINetworkQualityEstimatorService() {
@@ -88,8 +189,17 @@ UINetworkQualityEstimatorService::~UINetworkQualityEstimatorService() {
 void UINetworkQualityEstimatorService::Shutdown() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   weak_factory_.InvalidateWeakPtrs();
-  DCHECK(content::BrowserThread::DeleteSoon(content::BrowserThread::IO,
-                                            FROM_HERE, io_observer_));
+  if (io_observer_) {
+    DCHECK(content::BrowserThread::DeleteSoon(content::BrowserThread::IO,

[bengr, 2016/10/14 22:37:12]

Why is this in a DCHECK? Do you not want this to happen in release builds?

[RyanSturm, 2016/10/14 22:52:03]

Should we put io_observer_ and prefs_manager_ in std::unique_ptr and call
release inside of DeleteSoon and stop doing x = nullptr; We keep accidentally
leaking these. I think I would rather crash than leak if we mess this up.

[tbansal1, 2016/10/14 23:25:53]

Done.

[tbansal1, 2016/10/14 23:25:53]

Done.

+                                              FROM_HERE, io_observer_));
+    io_observer_ = nullptr;
+  }
+  if (prefs_manager_) {
+    prefs_manager_->ShutdownOnPrefThread();
+    DCHECK(content::BrowserThread::DeleteSoon(content::BrowserThread::IO,

[bengr, 2016/10/14 22:37:13]

Why is this in a DCHECK? Do you not want this to happen in release builds?

[tbansal1, 2016/10/14 23:25:53]

Done.

+                                              FROM_HERE, prefs_manager_));
+    prefs_manager_ = nullptr;
+  }
 }
 
 void UINetworkQualityEstimatorService::EffectiveConnectionTypeChanged(
@@ -109,3 +219,20 @@ UINetworkQualityEstimatorService::GetEffectiveConnectionType() const {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   return type_;
 }
+
+// static
+void UINetworkQualityEstimatorService::RegisterProfilePrefs(

[bengr, 2016/10/14 22:37:12]

I don't like the name of this class. UI... ick.

[tbansal1, 2016/10/14 23:25:53]

May be we can change it in  a separate CL.

+    PrefRegistrySimple* registry) {
+  registry->RegisterDictionaryPref(prefs::kNetworkQualities,
+                                   PrefRegistry::LOSSY_PREF);
+}
+
+std::map<net::nqe::internal::NetworkID,
+         net::nqe::internal::CachedNetworkQuality>
+UINetworkQualityEstimatorService::ForceReadPrefsForTesting() const {
+  if (!prefs_manager_) {
+    return std::map<net::nqe::internal::NetworkID,
+                    net::nqe::internal::CachedNetworkQuality>();
+  }
+  return prefs_manager_->ForceReadPrefsForTesting();
+}