NetworkQualityEstimator: Enable prefs writing by default

Enable writing to the prefs by default in the network quality estimator
(NQE). The reading from the prefs is still disable by default, and
controlled via field trial.

Also enable correlation logging by default which records UMA on
correlation between a higher layer metric, and a lower layer metric.

Also, use UMA_HISTPGRAM_EXACT_LINEAR for 2 histograms since those
histograms only need 2 buckets for storing data.

BUG=490870
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

Review-Url: https://codereview.chromium.org/2622663002
Cr-Commit-Position: refs/heads/master@{#443025}

chrome/browser/net/nqe/ui_network_quality_estimator_service.cc

@@ -43,14 +43,20 @@ std::string GetStringValueForVariationParamWithDefaultValue(
 // trial.
 bool persistent_cache_writing_enabled() {
   return GetStringValueForVariationParamWithDefaultValue(
-             "persistent_cache_writing_enabled", "false") == "true";
+             "persistent_cache_writing_enabled", "true") == "true";
 }
 
 // Returns true if reading from the persistent cache has been enabled via field
 // trial.
 bool persistent_cache_reading_enabled() {
-  return GetStringValueForVariationParamWithDefaultValue(
-             "persistent_cache_reading_enabled", "false") == "true";
+  if (GetStringValueForVariationParamWithDefaultValue(
+          "persistent_cache_reading_enabled", "false") != "true") {
+    return false;
+  }
+  // If reading from prefs is enabled, then writing to prefs must be enabled
+  // too.
+  DCHECK(persistent_cache_writing_enabled());
+  return true;
 }
 
 // PrefDelegateImpl writes the provided dictionary value to the network quality
@@ -71,14 +77,14 @@ class PrefDelegateImpl
       return;
 
     pref_service_->Set(path_, value);
-    UMA_HISTOGRAM_COUNTS_1000("NQE.Prefs.WriteCount", 1);
+    UMA_HISTOGRAM_EXACT_LINEAR("NQE.Prefs.WriteCount", 1, 2);
   }
 
   std::unique_ptr<base::DictionaryValue> GetDictionaryValue() override {
     DCHECK(thread_checker_.CalledOnValidThread());
     if (!persistent_cache_reading_enabled())
       return base::WrapUnique(new base::DictionaryValue());
-    UMA_HISTOGRAM_COUNTS_1000("NQE.Prefs.ReadCount", 1);
+    UMA_HISTOGRAM_EXACT_LINEAR("NQE.Prefs.ReadCount", 1, 2);
     return pref_service_->GetDictionary(path_)->CreateDeepCopy();
   }
 

chrome/browser/net/nqe/ui_network_quality_estimator_service_browsertest.cc

@@ -6,6 +6,7 @@
 #include <string>
 
 #include "base/bind.h"
+#include "base/metrics/field_trial.h"
 #include "base/run_loop.h"
 #include "base/test/histogram_tester.h"
 #include "chrome/browser/browser_process.h"
@@ -60,13 +61,13 @@ class UINetworkQualityEstimatorServiceBrowserTest
   // Verifies that the network quality prefs are written amd read correctly.
   void VerifyWritingReadingPrefs(bool persistent_cache_writing_enabled,
                                  bool persistent_cache_reading_enabled) {
+    variations::testing::ClearAllVariationParams();
     std::map<std::string, std::string> variation_params;
 
-    if (persistent_cache_writing_enabled)
-      variation_params["persistent_cache_writing_enabled"] = "true";
-
-    if (persistent_cache_reading_enabled)
-      variation_params["persistent_cache_reading_enabled"] = "true";
+    variation_params["persistent_cache_writing_enabled"] =
+        persistent_cache_writing_enabled ? "true" : "false";
+    variation_params["persistent_cache_reading_enabled"] =
+        persistent_cache_reading_enabled ? "true" : "false";
 
     variations::AssociateVariationParams("NetworkQualityEstimator", "Enabled",
                                          variation_params);
@@ -216,13 +217,6 @@ IN_PROC_BROWSER_TEST_F(UINetworkQualityEstimatorServiceBrowserTest,
             nqe_observer_3.effective_connection_type());
 }
 
-// Verify that prefs are not writen when writing of the prefs is not enabled
-// via field trial.
-IN_PROC_BROWSER_TEST_F(UINetworkQualityEstimatorServiceBrowserTest,
-                       WritingToPrefsDisabled) {
-  VerifyWritingReadingPrefs(false, true);
-}
-
 // Verify that prefs are not read when reading of the prefs is not enabled
 // via field trial.
 IN_PROC_BROWSER_TEST_F(UINetworkQualityEstimatorServiceBrowserTest,

net/nqe/network_quality_estimator.cc

@@ -140,6 +140,7 @@ static_assert(32 >= kBitsPerMetric * 4,
 int32_t FitInKBitsPerMetricBits(int32_t metric) {
   // Remove the last kTrimBits. This will allow the metric to fit within
   // kBitsPerMetric while losing only the least significant bits.
+  DCHECK_LE(0, metric);
   metric = metric >> kTrimBits;
 
   // kLargestValuePossible is the largest value that can be recorded using
@@ -149,7 +150,7 @@ int32_t FitInKBitsPerMetricBits(int32_t metric) {
     // Fit |metric| in kBitsPerMetric by clamping it down.
     metric = kLargestValuePossible;
   }
-  DCHECK_EQ(0, metric >> kBitsPerMetric);
+  DCHECK_EQ(0, metric >> kBitsPerMetric) << metric;
   return metric;
 }
 
@@ -606,7 +607,10 @@ void NetworkQualityEstimator::RecordCorrelationMetric(const URLRequest& request,
   if (load_timing_info.receive_headers_end < last_main_frame_request_)
     return;
 
-  const base::TimeTicks now = tick_clock_->NowTicks();
+  // Use the system clock instead of |tick_clock_| to compare the current
+  // timestamp with the |load_timing_info| timestamp since the latter is set by
+  // the system clock, and may be different from |tick_clock_| in tests.
+  const base::TimeTicks now = base::TimeTicks::Now();
   // Record UMA only for requests that started recently.
   if (now - last_main_frame_request_ > base::TimeDelta::FromSeconds(15))
     return;

net/nqe/network_quality_estimator_params.cc

@@ -330,7 +330,7 @@ double correlation_uma_logging_probability(
     const std::map<std::string, std::string>& variation_params) {
   double correlation_uma_logging_probability =
       GetDoubleValueForVariationParamWithDefaultValue(
-          variation_params, "correlation_logging_probability", 0.0);
+          variation_params, "correlation_logging_probability", 0.01);
   DCHECK_LE(0.0, correlation_uma_logging_probability);
   DCHECK_GE(1.0, correlation_uma_logging_probability);
   return correlation_uma_logging_probability;