Read and write network quality when there is a change in the signal strength

When there is a change in the signal strength, the current network quality
(using the older value of signal strength) is written to the cache.
At the same time, the cached network quality (with the updated
signal strength) is read.

Currently, the reading/writing of cached network quality is enabled
only for WiFi and Ethernet. This CL also enables reading/writing of
cached network quality for all connection types.

Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Bug: 786997
Change-Id: Ifb9d838691a3b22bc05bd559d21b722c9d4a8529
Reviewed-on: https://chromium-review.googlesource.com/806798
Commit-Queue: Tarun Bansal <tbansal@chromium.org>
Reviewed-by: Doug Arnett <dougarnett@chromium.org>
Cr-Commit-Position: refs/heads/master@{#521727}

chrome/browser/net/nqe/ui_network_quality_estimator_service_browsertest.cc

@@ -110,16 +110,6 @@ class UINetworkQualityEstimatorServiceBrowserTest
     // EffectiveConnectionType.
     Profile* profile = ProfileManager::GetActiveUserProfile();
 
-    bool network_id_available = true;
-    if (net::NetworkChangeNotifier::GetConnectionType() ==
-            net::NetworkChangeNotifier::CONNECTION_UNKNOWN ||
-        net::NetworkChangeNotifier::GetConnectionType() ==
-            net::NetworkChangeNotifier::CONNECTION_NONE ||
-        net::NetworkChangeNotifier::GetConnectionType() ==
-            net::NetworkChangeNotifier::CONNECTION_BLUETOOTH) {
-      network_id_available = false;
-    }
-
     UINetworkQualityEstimatorService* nqe_service =
         UINetworkQualityEstimatorServiceFactory::GetForProfile(profile);
     ASSERT_NE(nullptr, nqe_service);
@@ -133,10 +123,9 @@ class UINetworkQualityEstimatorServiceBrowserTest
       EXPECT_EQ(net::EFFECTIVE_CONNECTION_TYPE_OFFLINE,
                 nqe_service->GetEffectiveConnectionType());
 
-      // Prefs are written only if the network id was available, and persistent
-      // caching was enabled.
-      EXPECT_NE(network_id_available,
-                histogram_tester.GetAllSamples("NQE.Prefs.WriteCount").empty());
+      // Prefs are written only if persistent caching was enabled.
+      EXPECT_FALSE(
+          histogram_tester.GetAllSamples("NQE.Prefs.WriteCount").empty());
       histogram_tester.ExpectTotalCount("NQE.Prefs.ReadCount", 0);
 
       // NetworkQualityEstimator should not be notified of change in prefs.
@@ -151,9 +140,9 @@ class UINetworkQualityEstimatorServiceBrowserTest
       EXPECT_EQ(net::EFFECTIVE_CONNECTION_TYPE_SLOW_2G,
                 nqe_service->GetEffectiveConnectionType());
 
-      // Prefs are written only if the network id was available.
-      EXPECT_NE(network_id_available,
-                histogram_tester.GetAllSamples("NQE.Prefs.WriteCount").empty());
+      // Prefs are written even if the network id was unavailable.
+      EXPECT_FALSE(
+          histogram_tester.GetAllSamples("NQE.Prefs.WriteCount").empty());
       histogram_tester.ExpectTotalCount("NQE.Prefs.ReadCount", 0);
 
       // NetworkQualityEstimator should not be notified of change in prefs.
@@ -164,20 +153,24 @@ class UINetworkQualityEstimatorServiceBrowserTest
     std::map<net::nqe::internal::NetworkID,
              net::nqe::internal::CachedNetworkQuality>
         read_prefs = nqe_service->ForceReadPrefsForTesting();
-    EXPECT_EQ(network_id_available ? 1u : 0u, read_prefs.size());
-    if (network_id_available) {
-      // Verify that the cached network quality was written correctly.
-      EXPECT_EQ(net::EFFECTIVE_CONNECTION_TYPE_SLOW_2G,
-                read_prefs.begin()->second.effective_connection_type());
-      if (net::NetworkChangeNotifier::GetConnectionType() ==
-          net::NetworkChangeNotifier::CONNECTION_ETHERNET) {
-        // Verify that the network ID was written correctly.
-        net::nqe::internal::NetworkID ethernet_network_id(
-            net::NetworkChangeNotifier::CONNECTION_ETHERNET, std::string(),
-            INT32_MIN);
-        EXPECT_EQ(ethernet_network_id, read_prefs.begin()->first);
+    // Number of entries must be between 1 and 2. It's possible that 2 entries
+    // are added if the connection type is unknown to network quality estimator
+    // at the time of startup, and shortly after it receives a notification
+    // about the change in the connection type.
+    EXPECT_LE(1u, read_prefs.size());
+    EXPECT_GE(2u, read_prefs.size());
+
+    // Verify that the cached network quality was written correctly.
+    EXPECT_EQ(net::EFFECTIVE_CONNECTION_TYPE_SLOW_2G,
+              read_prefs.begin()->second.effective_connection_type());
+    if (net::NetworkChangeNotifier::GetConnectionType() ==
+        net::NetworkChangeNotifier::CONNECTION_ETHERNET) {
+      // Verify that the network ID was written correctly.
+      net::nqe::internal::NetworkID ethernet_network_id(
+          net::NetworkChangeNotifier::CONNECTION_ETHERNET, std::string(),
+          INT32_MIN);
+      EXPECT_EQ(ethernet_network_id, read_prefs.begin()->first);
       }
-    }
   }
 
  private:

net/nqe/network_qualities_prefs_manager_unittest.cc

@@ -88,24 +88,24 @@ TEST(NetworkQualitiesPrefManager, Write) {
 
   estimator.SimulateNetworkChange(
       NetworkChangeNotifier::ConnectionType::CONNECTION_UNKNOWN, "test");
-  EXPECT_EQ(0u, prefs_delegate_ptr->write_count());
+  EXPECT_EQ(1u, prefs_delegate_ptr->write_count());
   // Network quality generated from the default observation must be written.
   base::RunLoop().RunUntilIdle();
-  EXPECT_EQ(1u, prefs_delegate_ptr->write_count());
+  EXPECT_EQ(3u, prefs_delegate_ptr->write_count());
 
   estimator.set_recent_effective_connection_type(EFFECTIVE_CONNECTION_TYPE_2G);
   // Run a request so that effective connection type is recomputed, and
   // observers are notified of change in the network quality.
   estimator.RunOneRequest();
   base::RunLoop().RunUntilIdle();
-  EXPECT_EQ(2u, prefs_delegate_ptr->write_count());
+  EXPECT_EQ(4u, prefs_delegate_ptr->write_count());
 
   estimator.set_recent_effective_connection_type(EFFECTIVE_CONNECTION_TYPE_3G);
   // Run a request so that effective connection type is recomputed, and
   // observers are notified of change in the network quality..
   estimator.RunOneRequest();
   base::RunLoop().RunUntilIdle();
-  EXPECT_EQ(3u, prefs_delegate_ptr->write_count());
+  EXPECT_EQ(5u, prefs_delegate_ptr->write_count());
 
   // Prefs should not be read again.
   EXPECT_EQ(1u, prefs_delegate_ptr->read_count());
@@ -126,7 +126,7 @@ TEST(NetworkQualitiesPrefManager, WriteAndReadWithMultipleNetworkIDs) {
   estimator.SimulateNetworkChange(
       NetworkChangeNotifier::ConnectionType::CONNECTION_2G, "test");
 
-  EXPECT_EQ(0u, manager.ForceReadPrefsForTesting().size());
+  EXPECT_EQ(1u, manager.ForceReadPrefsForTesting().size());
 
   estimator.set_recent_effective_connection_type(
       EFFECTIVE_CONNECTION_TYPE_SLOW_2G);
@@ -136,7 +136,7 @@ TEST(NetworkQualitiesPrefManager, WriteAndReadWithMultipleNetworkIDs) {
   base::RunLoop().RunUntilIdle();
   // Verify that the observer was notified, and the updated network quality was
   // written to the prefs.
-  EXPECT_EQ(1u, manager.ForceReadPrefsForTesting().size());
+  EXPECT_EQ(2u, manager.ForceReadPrefsForTesting().size());
 
   // Change the network ID.
   for (size_t i = 0; i < kMaxCacheSize; ++i) {
@@ -147,7 +147,7 @@ TEST(NetworkQualitiesPrefManager, WriteAndReadWithMultipleNetworkIDs) {
     estimator.RunOneRequest();
     base::RunLoop().RunUntilIdle();
 
-    EXPECT_EQ(std::min(i + 2, kMaxCacheSize),
+    EXPECT_EQ(std::min(i + 3, kMaxCacheSize),
               manager.ForceReadPrefsForTesting().size());
   }
 
@@ -155,17 +155,27 @@ TEST(NetworkQualitiesPrefManager, WriteAndReadWithMultipleNetworkIDs) {
       read_prefs = manager.ForceReadPrefsForTesting();
 
   // Verify the contents of the prefs.
+  size_t count_2g_entries = 0;
   for (std::map<nqe::internal::NetworkID,
                 nqe::internal::CachedNetworkQuality>::const_iterator it =
            read_prefs.begin();
        it != read_prefs.end(); ++it) {
+    if (it->first.type ==
+        NetworkChangeNotifier::ConnectionType::CONNECTION_UNKNOWN) {
+      continue;
+    }
     EXPECT_EQ(0u, it->first.id.find("test", 0u));
     EXPECT_EQ(NetworkChangeNotifier::ConnectionType::CONNECTION_2G,
               it->first.type);
     EXPECT_EQ(EFFECTIVE_CONNECTION_TYPE_SLOW_2G,
               it->second.effective_connection_type());
+    ++count_2g_entries;
   }
 
+  // At most one entry should be for the network with connection type
+  // NetworkChangeNotifier::ConnectionType::CONNECTION_UNKNOWN.
+  EXPECT_LE(kMaxCacheSize - 1, count_2g_entries);
+
   base::HistogramTester histogram_tester;
   estimator.OnPrefsRead(read_prefs);
   histogram_tester.ExpectUniqueSample("NQE.Prefs.ReadSize", kMaxCacheSize, 1);
@@ -186,7 +196,7 @@ TEST(NetworkQualitiesPrefManager, ClearPrefs) {
   estimator.SimulateNetworkChange(
       NetworkChangeNotifier::ConnectionType::CONNECTION_UNKNOWN, "test");
 
-  EXPECT_EQ(0u, manager.ForceReadPrefsForTesting().size());
+  EXPECT_EQ(1u, manager.ForceReadPrefsForTesting().size());
 
   estimator.set_recent_effective_connection_type(
       EFFECTIVE_CONNECTION_TYPE_SLOW_2G);
@@ -196,7 +206,7 @@ TEST(NetworkQualitiesPrefManager, ClearPrefs) {
   base::RunLoop().RunUntilIdle();
   // Verify that the observer was notified, and the updated network quality was
   // written to the prefs.
-  EXPECT_EQ(1u, manager.ForceReadPrefsForTesting().size());
+  EXPECT_EQ(2u, manager.ForceReadPrefsForTesting().size());
 
   // Prefs must be completely cleared.
   manager.ClearPrefs();

net/nqe/network_quality_estimator.cc

@@ -695,7 +695,6 @@ void NetworkQualityEstimator::DisableOfflineCheckForTesting(
     bool disable_offline_check) {
   DCHECK(thread_checker_.CalledOnValidThread());
   disable_offline_check_ = disable_offline_check;
-  network_quality_store_->DisableOfflineCheckForTesting(disable_offline_check_);
 }
 
 void NetworkQualityEstimator::ReportEffectiveConnectionTypeForTesting(
@@ -868,11 +867,21 @@ void NetworkQualityEstimator::UpdateSignalStrength() {
   if (past_signal_strength == new_signal_strength)
     return;
 
-  current_network_id_.signal_strength = new_signal_strength;
   // Check if the signal strength is unavailable.
-  if (current_network_id_.signal_strength == INT32_MIN)
+  if (new_signal_strength == INT32_MIN)
     return;
 
+  // Record the network quality we experienced for the previous signal strength
+  // (for when we return to that signal strength).
+  network_quality_store_->Add(current_network_id_,
+                              nqe::internal::CachedNetworkQuality(
+                                  tick_clock_->NowTicks(), network_quality_,
+                                  effective_connection_type_));
+
+  current_network_id_.signal_strength = new_signal_strength;
+  // Update network quality from cached value for new signal strength.
+  ReadCachedNetworkQualityEstimate();
+
   min_signal_strength_since_connection_change_ =
       std::min(min_signal_strength_since_connection_change_.value_or(INT32_MAX),
                current_network_id_.signal_strength);
@@ -1577,22 +1586,12 @@ bool NetworkQualityEstimator::ReadCachedNetworkQualityEstimate() {
   if (!params_->persistent_cache_reading_enabled())
     return false;
 
-  if (current_network_id_.type !=
-          NetworkChangeNotifier::ConnectionType::CONNECTION_WIFI &&
-      current_network_id_.type !=
-          NetworkChangeNotifier::ConnectionType::CONNECTION_ETHERNET &&
-      !disable_offline_check_) {
-    return false;
-  }
-
   nqe::internal::CachedNetworkQuality cached_network_quality;
 
   const bool cached_estimate_available = network_quality_store_->GetById(
       current_network_id_, &cached_network_quality);
-  if (network_quality_store_->EligibleForCaching(current_network_id_)) {
-    UMA_HISTOGRAM_BOOLEAN("NQE.CachedNetworkQualityAvailable",
-                          cached_estimate_available);
-  }
+  UMA_HISTOGRAM_BOOLEAN("NQE.CachedNetworkQualityAvailable",
+                        cached_estimate_available);
 
   if (!cached_estimate_available)
     return false;

net/nqe/network_quality_estimator_unittest.cc

@@ -199,7 +199,8 @@ TEST(NetworkQualityEstimatorTest, TestKbpsRTTUpdates) {
 
   estimator.SimulateNetworkChange(
       NetworkChangeNotifier::ConnectionType::CONNECTION_UNKNOWN, "test");
-  histogram_tester.ExpectTotalCount("NQE.CachedNetworkQualityAvailable", 0);
+  histogram_tester.ExpectUniqueSample("NQE.CachedNetworkQualityAvailable",
+                                      false, 2);
 
   base::TimeDelta rtt;
   int32_t kbps;
@@ -282,7 +283,7 @@ TEST(NetworkQualityEstimatorTest, TestKbpsRTTUpdates) {
   estimator.SimulateNetworkChange(
       NetworkChangeNotifier::ConnectionType::CONNECTION_WIFI, "test-1");
   histogram_tester.ExpectUniqueSample("NQE.CachedNetworkQualityAvailable",
-                                      false, 1);
+                                      false, 3);
   histogram_tester.ExpectTotalCount("NQE.RatioMedianRTT.WiFi", 0);
 
   histogram_tester.ExpectTotalCount("NQE.RTT.Percentile0.Unknown", 1);
@@ -306,7 +307,7 @@ TEST(NetworkQualityEstimatorTest, TestKbpsRTTUpdates) {
   estimator.SimulateNetworkChange(
       NetworkChangeNotifier::ConnectionType::CONNECTION_WIFI, std::string());
   histogram_tester.ExpectUniqueSample("NQE.CachedNetworkQualityAvailable",
-                                      false, 1);
+                                      false, 4);
 
   EXPECT_FALSE(estimator.GetRecentHttpRTT(base::TimeTicks(), &rtt));
   EXPECT_FALSE(
@@ -336,7 +337,7 @@ TEST(NetworkQualityEstimatorTest, TestKbpsRTTUpdates) {
   estimator.SimulateNetworkChange(
       NetworkChangeNotifier::ConnectionType::CONNECTION_UNKNOWN, "test");
   histogram_tester.ExpectBucketCount("NQE.CachedNetworkQualityAvailable", false,
-                                     1);
+                                     4);
 }
 
 // Tests that the network quality estimator writes and reads network quality
@@ -359,7 +360,7 @@ TEST(NetworkQualityEstimatorTest, Caching) {
 
     estimator.SimulateNetworkChange(connection_type, connection_id);
     histogram_tester.ExpectUniqueSample("NQE.CachedNetworkQualityAvailable",
-                                        false, 1);
+                                        false, 2);
 
     base::TimeDelta rtt;
     int32_t kbps;
@@ -399,7 +400,7 @@ TEST(NetworkQualityEstimatorTest, Caching) {
     EXPECT_FALSE(estimator.GetTransportRTT());
 
     histogram_tester.ExpectBucketCount("NQE.CachedNetworkQualityAvailable",
-                                       false, 1);
+                                       false, 2);
 
     // Add the observers before changing the network type.
     TestEffectiveConnectionTypeObserver observer;
@@ -447,7 +448,7 @@ TEST(NetworkQualityEstimatorTest, Caching) {
 
     histogram_tester.ExpectBucketCount("NQE.CachedNetworkQualityAvailable",
                                        true, 1);
-    histogram_tester.ExpectTotalCount("NQE.CachedNetworkQualityAvailable", 2);
+    histogram_tester.ExpectTotalCount("NQE.CachedNetworkQualityAvailable", 3);
     base::RunLoop().RunUntilIdle();
 
     // Verify that the cached network quality was read, and observers were
@@ -2439,13 +2440,13 @@ TEST(NetworkQualityEstimatorTest, MAYBE_TestTCPSocketRTT) {
       NetworkChangeNotifier::ConnectionType::CONNECTION_2G, "test");
   histogram_tester.ExpectBucketCount(
       "NQE.RTT.ObservationSource",
-      NETWORK_QUALITY_OBSERVATION_SOURCE_TRANSPORT_CACHED_ESTIMATE, 0);
+      NETWORK_QUALITY_OBSERVATION_SOURCE_TRANSPORT_CACHED_ESTIMATE, 1);
 
   estimator.SimulateNetworkChange(
       NetworkChangeNotifier::ConnectionType::CONNECTION_WIFI, "test-1");
   histogram_tester.ExpectBucketCount(
       "NQE.RTT.ObservationSource",
-      NETWORK_QUALITY_OBSERVATION_SOURCE_TRANSPORT_CACHED_ESTIMATE, 1);
+      NETWORK_QUALITY_OBSERVATION_SOURCE_TRANSPORT_CACHED_ESTIMATE, 2);
 }
 
 #if defined(OS_IOS)
@@ -2908,7 +2909,7 @@ TEST(NetworkQualityEstimatorTest, CacheObserver) {
   estimator.SimulateNetworkChange(
       NetworkChangeNotifier::ConnectionType::CONNECTION_UNKNOWN, "test3g");
   estimator.RunOneRequest();
-  EXPECT_EQ(2u, observer.get_notification_received_and_reset());
+  EXPECT_EQ(4u, observer.get_notification_received_and_reset());
   EXPECT_EQ("test3g", observer.network_id().id);
 
   estimator.set_recent_effective_connection_type(EFFECTIVE_CONNECTION_TYPE_2G);

net/nqe/network_quality_store.cc

@@ -14,8 +14,7 @@ namespace nqe {
 
 namespace internal {
 
-NetworkQualityStore::NetworkQualityStore()
-    : disable_offline_check_(false), weak_ptr_factory_(this) {
+NetworkQualityStore::NetworkQualityStore() : weak_ptr_factory_(this) {
   static_assert(kMaximumNetworkQualityCacheSize > 0,
                 "Size of the network quality cache must be > 0");
   // This limit should not be increased unless the logic for removing the
@@ -40,9 +39,6 @@ void NetworkQualityStore::Add(
     return;
   }
 
-  if (!EligibleForCaching(network_id))
-    return;
-
   // Remove the entry from the map, if it is already present.
   cached_network_qualities_.erase(network_id);
 
@@ -136,24 +132,6 @@ void NetworkQualityStore::RemoveNetworkQualitiesCacheObserver(
   network_qualities_cache_observer_list_.RemoveObserver(observer);
 }
 
-bool NetworkQualityStore::EligibleForCaching(
-    const NetworkID& network_id) const {
-  DCHECK(thread_checker_.CalledOnValidThread());
-
-  // |disable_offline_check_| forces caching of the network quality even if
-  // the network is set to offline.
-  return network_id.type == NetworkChangeNotifier::CONNECTION_ETHERNET ||
-         !network_id.id.empty() ||
-         (network_id.type == NetworkChangeNotifier::CONNECTION_NONE &&
-          disable_offline_check_);
-}
-
-void NetworkQualityStore::DisableOfflineCheckForTesting(
-    bool disable_offline_check) {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  disable_offline_check_ = disable_offline_check;
-}
-
 void NetworkQualityStore::NotifyCacheObserverIfPresent(
     NetworkQualitiesCacheObserver* observer) const {
   DCHECK(thread_checker_.CalledOnValidThread());

net/nqe/network_quality_store.h

@@ -68,9 +68,6 @@ class NET_EXPORT_PRIVATE NetworkQualityStore {
   void RemoveNetworkQualitiesCacheObserver(
       NetworkQualitiesCacheObserver* observer);
 
-  // Returns true if network quality for |network_id| can be cached.
-  bool EligibleForCaching(const NetworkID& network_id) const;
-
   // If |disable_offline_check| is set to true, the offline check is disabled
   // when storing the network quality.
   void DisableOfflineCheckForTesting(bool disable_offline_check);
@@ -100,10 +97,6 @@ class NET_EXPORT_PRIVATE NetworkQualityStore {
   base::ObserverList<NetworkQualitiesCacheObserver>
       network_qualities_cache_observer_list_;
 
-  // When set to true, disables the offline check when storing the network
-  // quality.
-  bool disable_offline_check_;
-
   base::ThreadChecker thread_checker_;
 
   base::WeakPtrFactory<NetworkQualityStore> weak_ptr_factory_;

net/nqe/network_quality_store_unittest.cc

@@ -92,7 +92,7 @@ TEST(NetworkQualityStoreTest, TestCaching) {
   }
 
   {
-    // Entry will not be added for (Unknown, "").
+    // Entry will be added for (Unknown, "").
     nqe::internal::NetworkID network_id(
         NetworkChangeNotifier::CONNECTION_UNKNOWN, "", 0);
     nqe::internal::CachedNetworkQuality read_network_quality;
@@ -102,7 +102,7 @@ TEST(NetworkQualityStoreTest, TestCaching) {
                                       base::TimeDelta::FromSeconds(4), 4),
         EFFECTIVE_CONNECTION_TYPE_4G);
     network_quality_store.Add(network_id, set_network_quality);
-    EXPECT_FALSE(
+    EXPECT_TRUE(
         network_quality_store.GetById(network_id, &read_network_quality));
   }