Make NQE prefs manager a single threaded class

With servicification, NQE (Network Quality Estimator) prefs manager
can be used on a single thread. This CL makes NQE prefs manager
a single threaded class by combining methods that were previously
executed over separate threads.

Change-Id: Ic332bfeecc77f7d3401cd63ce1126f954fbbefc8
Bug: 878844
TBR: ryansturm@chromium.org
Reviewed-on: https://chromium-review.googlesource.com/c/1331178
Commit-Queue: Tarun Bansal <tbansal@chromium.org>
Reviewed-by: Tarun Bansal <tbansal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#607344}

net/nqe/network_qualities_prefs_manager.cc

@@ -60,35 +60,34 @@ ParsedPrefs ConvertDictionaryValueToMap(const base::DictionaryValue* value) {
 NetworkQualitiesPrefsManager::NetworkQualitiesPrefsManager(
     std::unique_ptr<PrefDelegate> pref_delegate)
     : pref_delegate_(std::move(pref_delegate)),
-      pref_task_runner_(base::ThreadTaskRunnerHandle::Get()),
       prefs_(pref_delegate_->GetDictionaryValue()),
-      network_quality_estimator_(nullptr),
-      read_prefs_startup_(ConvertDictionaryValueToMap(prefs_.get())),
-      pref_weak_ptr_factory_(this) {
+      network_quality_estimator_(nullptr) {
   DCHECK(pref_delegate_);
-  DCHECK(pref_task_runner_);
   DCHECK_GE(kMaxCacheSize, prefs_->size());
-
-  pref_weak_ptr_ = pref_weak_ptr_factory_.GetWeakPtr();
 }
 
 NetworkQualitiesPrefsManager::~NetworkQualitiesPrefsManager() {
-  if (!network_task_runner_)
-    return;
-  if (pref_task_runner_->RunsTasksInCurrentSequence()) {
-    ShutdownOnPrefSequence();
-  }
-  DCHECK(network_task_runner_->RunsTasksInCurrentSequence());
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
+  ShutdownOnPrefSequence();
+
   if (network_quality_estimator_)
     network_quality_estimator_->RemoveNetworkQualitiesCacheObserver(this);
 }
 
 void NetworkQualitiesPrefsManager::InitializeOnNetworkThread(
     NetworkQualityEstimator* network_quality_estimator) {
-  DCHECK(!network_task_runner_);
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   DCHECK(network_quality_estimator);
 
-  network_task_runner_ = base::ThreadTaskRunnerHandle::Get();
+  // Read |prefs_| again since they have now been fully initialized. This
+  // overwrites any values that may have been added to |prefs_| since
+  // construction of |this| via OnChangeInCachedNetworkQuality(). However, it's
+  // expected that InitializeOnNetworkThread will be called soon after
+  // construction of |this|. So, any loss of values would be minimal.
+  prefs_ = pref_delegate_->GetDictionaryValue();
+  read_prefs_startup_ = ConvertDictionaryValueToMap(prefs_.get());
+
   network_quality_estimator_ = network_quality_estimator;
   network_quality_estimator_->AddNetworkQualitiesCacheObserver(this);
 
@@ -96,27 +95,13 @@ void NetworkQualitiesPrefsManager::InitializeOnNetworkThread(
   network_quality_estimator_->OnPrefsRead(read_prefs_startup_);
 }
 
-void NetworkQualitiesPrefsManager::OnChangeInCachedNetworkQuality(
-    const nqe::internal::NetworkID& network_id,
-    const nqe::internal::CachedNetworkQuality& cached_network_quality) {
-  DCHECK(network_task_runner_->RunsTasksInCurrentSequence());
-
-  // Notify |this| on the pref thread.
-  pref_task_runner_->PostTask(
-      FROM_HERE,
-      base::Bind(&NetworkQualitiesPrefsManager::
-                     OnChangeInCachedNetworkQualityOnPrefSequence,
-                 pref_weak_ptr_, network_id, cached_network_quality));
-}
-
 void NetworkQualitiesPrefsManager::ShutdownOnPrefSequence() {
-  DCHECK(pref_task_runner_->RunsTasksInCurrentSequence());
-  pref_weak_ptr_factory_.InvalidateWeakPtrs();
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   pref_delegate_.reset();
 }
 
 void NetworkQualitiesPrefsManager::ClearPrefs() {
-  DCHECK(pref_task_runner_->RunsTasksInCurrentSequence());
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
   LOCAL_HISTOGRAM_COUNTS_100("NQE.PrefsSizeOnClearing", prefs_->size());
   prefs_->Clear();
@@ -124,11 +109,10 @@ void NetworkQualitiesPrefsManager::ClearPrefs() {
   pref_delegate_->SetDictionaryValue(*prefs_);
 }
 
-void NetworkQualitiesPrefsManager::OnChangeInCachedNetworkQualityOnPrefSequence(
+void NetworkQualitiesPrefsManager::OnChangeInCachedNetworkQuality(
     const nqe::internal::NetworkID& network_id,
     const nqe::internal::CachedNetworkQuality& cached_network_quality) {
-  // The prefs can only be written on the pref thread.
-  DCHECK(pref_task_runner_->RunsTasksInCurrentSequence());
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   DCHECK_GE(kMaxCacheSize, prefs_->size());
 
   std::string network_id_string = network_id.ToString();
@@ -171,7 +155,7 @@ void NetworkQualitiesPrefsManager::OnChangeInCachedNetworkQualityOnPrefSequence(
 }
 
 ParsedPrefs NetworkQualitiesPrefsManager::ForceReadPrefsForTesting() const {
-  DCHECK(pref_task_runner_->RunsTasksInCurrentSequence());
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   std::unique_ptr<base::DictionaryValue> value(
       pref_delegate_->GetDictionaryValue());
   return ConvertDictionaryValueToMap(value.get());

net/nqe/network_qualities_prefs_manager.h

@@ -10,6 +10,7 @@
 
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
+#include "base/sequence_checker.h"
 #include "base/time/time.h"
 #include "base/values.h"
 #include "net/base/net_export.h"
@@ -18,10 +19,6 @@
 #include "net/nqe/network_id.h"
 #include "net/nqe/network_quality_store.h"
 
-namespace base {
-class SequencedTaskRunner;
-}
-
 namespace net {
 class NetworkQualityEstimator;
 
@@ -34,16 +31,7 @@ typedef std::map<nqe::internal::NetworkID, nqe::internal::CachedNetworkQuality>
     ParsedPrefs;
 
 // Using the provided PrefDelegate, NetworkQualitiesPrefsManager creates and
-// updates network quality information that is stored in prefs. Instances of
-// this class must be constructed on the pref thread, and should later be moved
-// to the network thread by calling InitializeOnNetworkThread.
-//
-// This class interacts with both the pref thread and the network thread, and
-// propagates network quality pref changes from the network thread to the
-// provided pref delegate on the pref thread.
-//
-// If |this| is used on multiple threads, ShutdownOnPrefSequence must be called
-// from the pref thread before destruction.
+// updates network quality information that is stored in prefs.
 class NET_EXPORT NetworkQualitiesPrefsManager
     : public nqe::internal::NetworkQualityStore::NetworkQualitiesCacheObserver {
  public:
@@ -60,17 +48,17 @@ class NET_EXPORT NetworkQualitiesPrefsManager
   };
 
   // Creates 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.
+  // |pref_delegate| is taken by this class.
   explicit NetworkQualitiesPrefsManager(
       std::unique_ptr<PrefDelegate> pref_delegate);
   ~NetworkQualitiesPrefsManager() override;
 
-  // Initialize on the Network thread.
+  // Initialize on the Network thread. Must be called after pref service has
+  // been initialized, and prefs are ready for reading.
   void InitializeOnNetworkThread(
       NetworkQualityEstimator* network_quality_estimator);
 
-  // Prepare for shutdown. Must be called on the pref thread before destruction.
+  // Prepare for shutdown.
   void ShutdownOnPrefSequence();
 
   // Clear the network quality estimator prefs.
@@ -81,24 +69,12 @@ class NET_EXPORT NetworkQualitiesPrefsManager
   ParsedPrefs ForceReadPrefsForTesting() const;
 
  private:
-  // Pref thread members:
-  // Called on pref thread when there is a change in the cached network quality.
-  void OnChangeInCachedNetworkQualityOnPrefSequence(
-      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_;
-
-  // Current prefs on the disk. Should be accessed only on the pref thread.
+  // Current prefs on the disk.
   std::unique_ptr<base::DictionaryValue> prefs_;
 
-  // Should be accessed only on the pref thread.
-  base::WeakPtr<NetworkQualitiesPrefsManager> pref_weak_ptr_;
-
-  // Network thread members:
   // nqe::internal::NetworkQualityStore::NetworkQualitiesCacheObserver
   // implementation:
   void OnChangeInCachedNetworkQuality(
@@ -108,14 +84,10 @@ class NET_EXPORT NetworkQualitiesPrefsManager
 
   NetworkQualityEstimator* network_quality_estimator_;
 
-  scoped_refptr<base::SequencedTaskRunner> network_task_runner_;
-
-  // Network quality prefs read from the disk at the time of startup. Can be
-  // accessed on any thread.
-  const ParsedPrefs read_prefs_startup_;
+  // Network quality prefs read from the disk at the time of startup.
+  ParsedPrefs read_prefs_startup_;
 
-  // Used to get |weak_ptr_| to self on the pref thread.
-  base::WeakPtrFactory<NetworkQualitiesPrefsManager> pref_weak_ptr_factory_;
+  SEQUENCE_CHECKER(sequence_checker_);
 
   DISALLOW_COPY_AND_ASSIGN(NetworkQualitiesPrefsManager);
 };

net/nqe/network_qualities_prefs_manager_unittest.cc

@@ -93,11 +93,11 @@ TEST_F(NetworkQualitiesPrefManager, Write) {
   base::RunLoop().RunUntilIdle();
 
   // Prefs must be read at when NetworkQualitiesPrefsManager is constructed.
-  EXPECT_EQ(1u, prefs_delegate_ptr->read_count());
+  EXPECT_EQ(2u, prefs_delegate_ptr->read_count());
 
   estimator.SimulateNetworkChange(
       NetworkChangeNotifier::ConnectionType::CONNECTION_UNKNOWN, "test");
-  EXPECT_EQ(1u, prefs_delegate_ptr->write_count());
+  EXPECT_EQ(3u, prefs_delegate_ptr->write_count());
   // Network quality generated from the default observation must be written.
   base::RunLoop().RunUntilIdle();
   EXPECT_EQ(3u, prefs_delegate_ptr->write_count());
@@ -117,7 +117,7 @@ TEST_F(NetworkQualitiesPrefManager, Write) {
   EXPECT_EQ(5u, prefs_delegate_ptr->write_count());
 
   // Prefs should not be read again.
-  EXPECT_EQ(1u, prefs_delegate_ptr->read_count());
+  EXPECT_EQ(2u, prefs_delegate_ptr->read_count());
 
   manager.ShutdownOnPrefSequence();
 }
@@ -138,13 +138,13 @@ TEST_F(NetworkQualitiesPrefManager, WriteWhenMatchingExpectedECT) {
   base::RunLoop().RunUntilIdle();
 
   // Prefs must be read at when NetworkQualitiesPrefsManager is constructed.
-  EXPECT_EQ(1u, prefs_delegate_ptr->read_count());
+  EXPECT_EQ(2u, prefs_delegate_ptr->read_count());
 
   const nqe::internal::NetworkID network_id(
       NetworkChangeNotifier::ConnectionType::CONNECTION_4G, "test", INT32_MIN);
 
   estimator.SimulateNetworkChange(network_id.type, network_id.id);
-  EXPECT_EQ(1u, prefs_delegate_ptr->write_count());
+  EXPECT_EQ(3u, prefs_delegate_ptr->write_count());
   // Network quality generated from the default observation must be written.
   base::RunLoop().RunUntilIdle();
   EXPECT_EQ(3u, prefs_delegate_ptr->write_count());
@@ -164,7 +164,7 @@ TEST_F(NetworkQualitiesPrefManager, WriteWhenMatchingExpectedECT) {
   EXPECT_EQ(5u, prefs_delegate_ptr->write_count());
 
   // Prefs should not be read again.
-  EXPECT_EQ(1u, prefs_delegate_ptr->read_count());
+  EXPECT_EQ(2u, prefs_delegate_ptr->read_count());
 
   EXPECT_EQ(2u, manager.ForceReadPrefsForTesting().size());
   EXPECT_EQ(EFFECTIVE_CONNECTION_TYPE_3G,
@@ -204,7 +204,7 @@ TEST_F(NetworkQualitiesPrefManager, WriteAndReadWithMultipleNetworkIDs) {
   estimator.SimulateNetworkChange(
       NetworkChangeNotifier::ConnectionType::CONNECTION_2G, "test");
 
-  EXPECT_EQ(1u, manager.ForceReadPrefsForTesting().size());
+  EXPECT_EQ(2u, manager.ForceReadPrefsForTesting().size());
 
   estimator.set_recent_effective_connection_type(
       EFFECTIVE_CONNECTION_TYPE_SLOW_2G);
@@ -279,7 +279,7 @@ TEST_F(NetworkQualitiesPrefManager, ClearPrefs) {
   estimator.SimulateNetworkChange(
       NetworkChangeNotifier::ConnectionType::CONNECTION_UNKNOWN, "test");
 
-  EXPECT_EQ(1u, manager.ForceReadPrefsForTesting().size());
+  EXPECT_EQ(2u, manager.ForceReadPrefsForTesting().size());
 
   estimator.set_recent_effective_connection_type(
       EFFECTIVE_CONNECTION_TYPE_SLOW_2G);