Add Network Quality Estimator (NQE) pref manager

net/nqe/network_qualities_prefs_manager.h

+// Copyright 2016 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.
+
+#ifndef NET_NQE_NETWORK_QUALITIES_PREFS_MANAGER_H_
+#define NET_NQE_NETWORK_QUALITIES_PREFS_MANAGER_H_
+
+#include <memory>
+
+#include "base/macros.h"
+#include "base/memory/weak_ptr.h"
+#include "net/base/net_export.h"
+#include "net/nqe/effective_connection_type.h"
+#include "net/nqe/network_id.h"
+#include "net/nqe/network_quality_store.h"
+
+namespace base {
+class DictionaryValue;
+class SequencedTaskRunner;
+}
+
+namespace net {
+namespace nqe {
+namespace internal {
+class CachedNetworkQuality;
+}
+}
+class NetworkQualityEstimator;
+
+typedef base::Callback<void(
+    const nqe::internal::NetworkID& network_id,
+    const nqe::internal::CachedNetworkQuality& cached_network_quality)>
+    OnChangeInCachedNetworkQualityCallback;
+
+// Using the provided PrefDelegate, NetworkQualitiesPrefsManager creates and

[Not at Google. Contact bengr, 2016/09/15 21:13:16]

How about starting with:
// Propagates network quality pref changes from network
// thread to the pref delegate on the pref thread.

[tbansal1, 2016/09/15 21:32:30]

Your comment made me think what exactly class comments should include.
https://google.github.io/styleguide/cppguide.html#Class_Comments says that it
should include "what the class is for" and "how the class should be used".

It seems that including the details on how the network quality is moved from
network thread to pref thread seems like an internal detail. wdyt?

[Not at Google. Contact bengr, 2016/09/15 22:46:57]

It seems to me my suggestion sticks to these exact guidelines. Or am I missing
something?

"what the class is for": For propagating network quality pref changes from
network thread to pref_delegate_ thread. 

Isn't it? If these events were already available on the pref_delegate_ thread
then you wouldn't need this class.

I agree that there is no need to say how you implement the thread hop, which
happens to be using task runner primitives.

[RyanSturm, 2016/09/16 16:23:41]

Sounds like you guys are arguing about what the consumer needs to know. If the
consumer *should* know that NQE is only on the network thread, then adding the
comment is useful. I'm of the belief that a consumer of this shouldn't need to
know how NQE is implemented; they should just know that they can get evetns
about network quality on the pref thread.

[Not at Google. Contact bengr, 2016/09/16 17:01:28]

My comment suggestion was mainly for any engineer that happens to look at them.

It is not obvious to me from the name of this class and the comments here, what
the class is for.

I don't have a strong opinion, since now I already know what this class does.
Please feel free to disregard if you prefer the comments as written:)

[tbansal1, 2016/09/16 18:05:26]

I added the suggested comment, although I added it in the middle (not in the
beginning as suggested). Thanks.

+// updates network quality prefs. Instances of this class must be constructed on
+// the pref thread.
+//
+// This class interacts with the pref thread, where it reads/writes to the pref
+// using the provided PrefDelegate. This class also interacts with the network
+// thread on which it interacts with the network quality estimator.
+//
+// ShutdownOnPrefThread must be called from pref thread before destruction.
+class NET_EXPORT NetworkQualitiesPrefsManager
+    : public nqe::internal::NetworkQualityStore::NetworkQualitiesCacheObserver {
+ public:
+  // Provides an interface that must be implemented by the embedder.
+  class NET_EXPORT PrefDelegate {
+   public:
+    // Sets the persistent pref to the given value.
+    virtual void SetDictionaryValue(const base::DictionaryValue& value) = 0;
+  };
+
+  // Create an instance of the NetworkQualitiesPrefsManager. Ownership of
+  // |pref_delegate| is taken by this class. Must be constructed on the Pref
+  // thread, and then moved to network thread.
+  explicit NetworkQualitiesPrefsManager(
+      std::unique_ptr<PrefDelegate> pref_delegate);
+  ~NetworkQualitiesPrefsManager() override;
+
+  // Initialize on Network thread.
+  void InitializeOnNetworkThread(
+      NetworkQualityEstimator* network_quality_estimator);
+
+  // Prepare for shutdown. Must be called on the Pref thread before destruction.
+  void ShutdownOnPrefThread();
+
+ private:
+  // -----------
+  // Pref thread
+  // -----------
+
+  // Called on pref thread when there is a change in the cached network quality.
+  void OnChangeInCachedNetworkQualityOnPrefThread(
+      const nqe::internal::NetworkID& network_id,
+      const nqe::internal::CachedNetworkQuality& cached_network_quality);
+
+  // Responsible for writing the persistent prefs to the disk.
+  std::unique_ptr<PrefDelegate> pref_delegate_;
+
+  scoped_refptr<base::SequencedTaskRunner> pref_task_runner_;
+
+  // Should be accessed only on the pref thread.
+  base::WeakPtr<NetworkQualitiesPrefsManager> pref_weak_ptr_;
+
+  // --------------
+  // Network thread
+  // --------------
+
+  // nqe::internal::NetworkQualityStore::NetworkQualitiesCacheObserver
+  // implementation:
+  void OnChangeInCachedNetworkQuality(
+      const nqe::internal::NetworkID& network_id,
+      const nqe::internal::CachedNetworkQuality& cached_network_quality)
+      override;
+
+  NetworkQualityEstimator* network_quality_estimator_;
+
+  scoped_refptr<base::SequencedTaskRunner> network_task_runner_;
+
+  // --------------
+  // Common
+  // --------------
+
+  // Used to get |weak_ptr_| to self on the pref thread.
+  base::WeakPtrFactory<NetworkQualitiesPrefsManager> pref_weak_ptr_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(NetworkQualitiesPrefsManager);
+};
+
+}  // namespace net
+
+#endif  // NET_NQE_NETWORK_QUALITIES_PREFS_MANAGER_H_