NQE: Do not convert int32_t to base::optional

Network Quality Estimator (NQE) encapsulates int32_t as a
base::Optional<int32_t> at some of the places. This happens
on-the-fly. This is confusing, and may also be the
cause of some of the crashes.

Change-Id: I66142aa5047d9da577d60e17b3186c37b0c9a149
Bug: 887162
Reviewed-on: https://chromium-review.googlesource.com/1237730Reviewed-by: Ryan Sturm <ryansturm@chromium.org>
Commit-Queue: Tarun Bansal <tbansal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#593251}

net/nqe/network_quality_estimator.cc

@@ -818,6 +818,8 @@ void NetworkQualityEstimator::UpdateSignalStrength() {
   if (new_signal_strength == INT32_MIN)
     return;
 
+  DCHECK(new_signal_strength >= 0 && new_signal_strength <= 4);
+
   // 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_,

net/nqe/network_quality_estimator_unittest.cc

@@ -2855,10 +2855,10 @@ TEST_F(NetworkQualityEstimatorTest,
   // When a cached estimate is available, RTT observations from the external
   // estimate provider and platform must be discarded.
   estimator.AddAndNotifyObserversOfRTT(nqe::internal::Observation(
-      1, base::TimeTicks::Now(), base::Optional<int32_t>(),
+      1, base::TimeTicks::Now(), INT32_MIN,
       DEPRECATED_NETWORK_QUALITY_OBSERVATION_SOURCE_HTTP_EXTERNAL_ESTIMATE));
   estimator.AddAndNotifyObserversOfRTT(nqe::internal::Observation(
-      1, base::TimeTicks::Now(), base::Optional<int32_t>(),
+      1, base::TimeTicks::Now(), INT32_MIN,
       NETWORK_QUALITY_OBSERVATION_SOURCE_DEFAULT_HTTP_FROM_PLATFORM));
   EXPECT_EQ(3u, rtt_observer.observations().size());
   EXPECT_EQ(
@@ -2870,9 +2870,9 @@ TEST_F(NetworkQualityEstimatorTest,
       estimator
           .rtt_ms_observations_[nqe::internal::OBSERVATION_CATEGORY_TRANSPORT]
           .Size());
-  estimator.AddAndNotifyObserversOfRTT(nqe::internal::Observation(
-      1, base::TimeTicks::Now(), base::Optional<int32_t>(),
-      NETWORK_QUALITY_OBSERVATION_SOURCE_HTTP));
+  estimator.AddAndNotifyObserversOfRTT(
+      nqe::internal::Observation(1, base::TimeTicks::Now(), INT32_MIN,
+                                 NETWORK_QUALITY_OBSERVATION_SOURCE_HTTP));
   EXPECT_EQ(4u, rtt_observer.observations().size());
   EXPECT_EQ(
       3u,
@@ -2893,16 +2893,16 @@ TEST_F(NetworkQualityEstimatorTest,
   // cached estimate is received.
   EXPECT_EQ(1u, estimator.http_downstream_throughput_kbps_observations_.Size());
   estimator.AddAndNotifyObserversOfThroughput(nqe::internal::Observation(
-      1, base::TimeTicks::Now(), base::Optional<int32_t>(),
+      1, base::TimeTicks::Now(), INT32_MIN,
       DEPRECATED_NETWORK_QUALITY_OBSERVATION_SOURCE_HTTP_EXTERNAL_ESTIMATE));
   estimator.AddAndNotifyObserversOfThroughput(nqe::internal::Observation(
-      1, base::TimeTicks::Now(), base::Optional<int32_t>(),
+      1, base::TimeTicks::Now(), INT32_MIN,
       NETWORK_QUALITY_OBSERVATION_SOURCE_DEFAULT_HTTP_FROM_PLATFORM));
   EXPECT_EQ(2u, throughput_observer.observations().size());
   EXPECT_EQ(2u, estimator.http_downstream_throughput_kbps_observations_.Size());
-  estimator.AddAndNotifyObserversOfThroughput(nqe::internal::Observation(
-      1, base::TimeTicks::Now(), base::Optional<int32_t>(),
-      NETWORK_QUALITY_OBSERVATION_SOURCE_HTTP));
+  estimator.AddAndNotifyObserversOfThroughput(
+      nqe::internal::Observation(1, base::TimeTicks::Now(), INT32_MIN,
+                                 NETWORK_QUALITY_OBSERVATION_SOURCE_HTTP));
   EXPECT_EQ(3u, throughput_observer.observations().size());
   EXPECT_EQ(3u, estimator.http_downstream_throughput_kbps_observations_.Size());
 

net/nqe/network_quality_observation.cc

@@ -13,13 +13,13 @@ namespace internal {
 
 Observation::Observation(int32_t value,
                          base::TimeTicks timestamp,
-                         const base::Optional<int32_t>& signal_strength,
+                         int32_t signal_strength,
                          NetworkQualityObservationSource source)
     : Observation(value, timestamp, signal_strength, source, base::nullopt) {}
 
 Observation::Observation(int32_t value,
                          base::TimeTicks timestamp,
-                         const base::Optional<int32_t>& signal_strength,
+                         int32_t signal_strength,
                          NetworkQualityObservationSource source,
                          const base::Optional<IPHash>& host)
     : value_(value),
@@ -28,6 +28,8 @@ Observation::Observation(int32_t value,
       source_(source),
       host_(host) {
   DCHECK(!timestamp_.is_null());
+  DCHECK(signal_strength_ == INT32_MIN ||
+         (signal_strength_ >= 0 && signal_strength_ <= 4));
 }
 
 Observation::Observation(const Observation& other) = default;

net/nqe/network_quality_observation.h

@@ -29,12 +29,12 @@ class NET_EXPORT_PRIVATE Observation {
  public:
   Observation(int32_t value,
               base::TimeTicks timestamp,
-              const base::Optional<int32_t>& signal_strength,
+              int32_t signal_strength,
               NetworkQualityObservationSource source);
 
   Observation(int32_t value,
               base::TimeTicks timestamp,
-              const base::Optional<int32_t>& signal_strength,
+              int32_t signal_strength,
               NetworkQualityObservationSource source,
               const base::Optional<IPHash>& host);
 
@@ -49,8 +49,9 @@ class NET_EXPORT_PRIVATE Observation {
   // Time when the observation was taken.
   base::TimeTicks timestamp() const { return timestamp_; }
 
-  // Signal strength when the observation was taken.
-  base::Optional<int32_t> signal_strength() const { return signal_strength_; }
+  // Signal strength when the observation was taken. Set to INT32_MIN when the
+  // value is unavailable. Otherwise, must be between 0 and 4 (both inclusive).
+  int32_t signal_strength() const { return signal_strength_; }
 
   // The source of the observation.
   NetworkQualityObservationSource source() const { return source_; }
@@ -66,7 +67,10 @@ class NET_EXPORT_PRIVATE Observation {
 
   base::TimeTicks timestamp_;
 
-  base::Optional<int32_t> signal_strength_;
+  // Signal strength of the network when the observation was taken. Set to
+  // INT32_MIN when the value is unavailable. Otherwise, must be between 0 and 4
+  // (both inclusive).
+  int32_t signal_strength_;
 
   NetworkQualityObservationSource source_;
 

net/nqe/observation_buffer.cc

@@ -58,6 +58,10 @@ void ObservationBuffer::AddObservation(const Observation& observation) {
   DCHECK(observations_.empty() ||
          observation.timestamp() >= observations_.back().timestamp());
 
+  DCHECK(observation.signal_strength() == INT32_MIN ||
+         (observation.signal_strength() >= 0 &&
+          observation.signal_strength() <= 4));
+
   // Evict the oldest element if the buffer is already full.
   if (observations_.size() == params_->observation_buffer_size())
     observations_.pop_front();
@@ -68,9 +72,12 @@ void ObservationBuffer::AddObservation(const Observation& observation) {
 
 base::Optional<int32_t> ObservationBuffer::GetPercentile(
     base::TimeTicks begin_timestamp,
-    const base::Optional<int32_t>& current_signal_strength,
+    int32_t current_signal_strength,
     int percentile,
     size_t* observations_count) const {
+  DCHECK(current_signal_strength == INT32_MIN ||
+         (current_signal_strength >= 0 && current_signal_strength <= 4));
+
   // Stores weighted observations in increasing order by value.
   std::vector<WeightedObservation> weighted_observations;
 
@@ -172,7 +179,7 @@ void ObservationBuffer::RemoveObservationsWithSource(
 
 void ObservationBuffer::ComputeWeightedObservations(
     const base::TimeTicks& begin_timestamp,
-    const base::Optional<int32_t>& current_signal_strength,
+    int32_t current_signal_strength,
     std::vector<WeightedObservation>* weighted_observations,
     double* total_weight) const {
   DCHECK_GE(Capacity(), Size());
@@ -190,10 +197,9 @@ void ObservationBuffer::ComputeWeightedObservations(
         pow(weight_multiplier_per_second_, time_since_sample_taken.InSeconds());
 
     double signal_strength_weight = 1.0;
-    if (current_signal_strength && observation.signal_strength()) {
+    if (current_signal_strength >= 0 && observation.signal_strength() >= 0) {
       int32_t signal_strength_weight_diff =
-          std::abs(current_signal_strength.value() -
-                   observation.signal_strength().value());
+          std::abs(current_signal_strength - observation.signal_strength());
       signal_strength_weight =
           pow(weight_multiplier_per_signal_level_, signal_strength_weight_diff);
     }

net/nqe/observation_buffer.h

@@ -74,11 +74,10 @@ class NET_EXPORT_PRIVATE ObservationBuffer {
   // signal strength. |result| must not be null. If |observations_count| is not
   // null, then it is set to the number of observations that were available
   // in the observation buffer for computing the percentile.
-  base::Optional<int32_t> GetPercentile(
-      base::TimeTicks begin_timestamp,
-      const base::Optional<int32_t>& current_signal_strength,
-      int percentile,
-      size_t* observations_count) const;
+  base::Optional<int32_t> GetPercentile(base::TimeTicks begin_timestamp,
+                                        int32_t current_signal_strength,
+                                        int percentile,
+                                        size_t* observations_count) const;
 
   void SetTickClockForTesting(const base::TickClock* tick_clock) {
     tick_clock_ = tick_clock;
@@ -114,7 +113,7 @@ class NET_EXPORT_PRIVATE ObservationBuffer {
   // at least one observation in the buffer.
   void ComputeWeightedObservations(
       const base::TimeTicks& begin_timestamp,
-      const base::Optional<int32_t>& current_signal_strength,
+      int32_t current_signal_strength,
       std::vector<WeightedObservation>* weighted_observations,
       double* total_weight) const;
 

net/nqe/observation_buffer_unittest.cc

@@ -221,10 +221,10 @@ TEST(NetworkQualityObservationBufferTest, PercentileDifferentRSSI) {
   NetworkQualityEstimatorParams params(variation_params);
   base::SimpleTestTickClock tick_clock;
   tick_clock.Advance(base::TimeDelta::FromMinutes(1));
-  ObservationBuffer buffer(&params, &tick_clock, 1.0, 0.5);
+  ObservationBuffer buffer(&params, &tick_clock, 1.0, 0.25);
   const base::TimeTicks now = tick_clock.NowTicks();
-  int32_t high_rssi = 0;
-  int32_t low_rssi = -100;
+  int32_t high_rssi = 4;
+  int32_t low_rssi = 0;
 
   // Network quality should be unavailable when no observations are available.
   EXPECT_FALSE(buffer.GetPercentile(base::TimeTicks(), INT32_MIN, 50, nullptr)
@@ -248,7 +248,7 @@ TEST(NetworkQualityObservationBufferTest, PercentileDifferentRSSI) {
     base::Optional<int32_t> result =
         buffer.GetPercentile(now, high_rssi, i, nullptr);
     EXPECT_TRUE(result.has_value());
-    EXPECT_NEAR(result.value(), 51 + 0.49 * i, 1);
+    EXPECT_NEAR(result.value(), 51 + 0.49 * i, 2);
   }
 
   // When the current RSSI is |low_rssi|, higher weight should be assigned
@@ -257,7 +257,7 @@ TEST(NetworkQualityObservationBufferTest, PercentileDifferentRSSI) {
     base::Optional<int32_t> result =
         buffer.GetPercentile(now, low_rssi, i, nullptr);
     EXPECT_TRUE(result.has_value());
-    EXPECT_NEAR(result.value(), i / 2, 1);
+    EXPECT_NEAR(result.value(), i / 2, 2);
   }
 }