NQE: Combine HTTP RTT and transport RTT related methods

This CL does not introduce any functionality change, but makes
it easier to add a new category of RTTs by combining similar
methods in Network Quality Estimator (NQE).

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

net/nqe/network_quality_estimator.cc

@@ -470,8 +470,10 @@ void NetworkQualityEstimator::RecordAccuracyAfterMainFrame(
     return;
 
   base::TimeDelta recent_http_rtt;
-  if (!GetRecentHttpRTT(last_main_frame_request_, &recent_http_rtt))
+  if (!GetRecentRTT(nqe::internal::OBSERVATION_CATEGORY_HTTP,
+                    last_main_frame_request_, &recent_http_rtt, nullptr)) {
     recent_http_rtt = nqe::internal::InvalidRTT();
+  }
 
   if (estimated_quality_at_last_main_frame_.http_rtt() !=
           nqe::internal::InvalidRTT() &&
@@ -488,8 +490,8 @@ void NetworkQualityEstimator::RecordAccuracyAfterMainFrame(
   base::TimeDelta recent_transport_rtt;
   if (estimated_quality_at_last_main_frame_.transport_rtt() !=
           nqe::internal::InvalidRTT() &&
-      GetRecentTransportRTT(last_main_frame_request_, &recent_transport_rtt,
-                            nullptr)) {
+      GetRecentRTT(nqe::internal::OBSERVATION_CATEGORY_TRANSPORT,
+                   last_main_frame_request_, &recent_transport_rtt, nullptr)) {
     const int estimated_observed_diff_milliseconds =
         estimated_quality_at_last_main_frame_.transport_rtt().InMilliseconds() -
         recent_transport_rtt.InMilliseconds();
@@ -1125,11 +1127,13 @@ NetworkQualityEstimator::GetRecentEffectiveConnectionTypeUsingMetrics(
     return EFFECTIVE_CONNECTION_TYPE_OFFLINE;
   }
 
-  if (!GetRecentHttpRTT(start_time, http_rtt))
+  if (!GetRecentRTT(nqe::internal::OBSERVATION_CATEGORY_HTTP, start_time,
+                    http_rtt, nullptr)) {
     *http_rtt = nqe::internal::InvalidRTT();
+  }
 
-  if (!GetRecentTransportRTT(start_time, transport_rtt,
-                             transport_rtt_observation_count)) {
+  if (!GetRecentRTT(nqe::internal::OBSERVATION_CATEGORY_TRANSPORT, start_time,
+                    transport_rtt, transport_rtt_observation_count)) {
     *transport_rtt = nqe::internal::InvalidRTT();
   }
 
@@ -1261,23 +1265,14 @@ void NetworkQualityEstimator::RemoveRTTAndThroughputEstimatesObserver(
   rtt_and_throughput_estimates_observer_list_.RemoveObserver(observer);
 }
 
-bool NetworkQualityEstimator::GetRecentHttpRTT(
-    const base::TimeTicks& start_time,
-    base::TimeDelta* rtt) const {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  *rtt = GetRTTEstimateInternal(
-      start_time, nqe::internal::OBSERVATION_CATEGORY_HTTP, 50, nullptr);
-  return (*rtt != nqe::internal::InvalidRTT());
-}
-
-bool NetworkQualityEstimator::GetRecentTransportRTT(
+bool NetworkQualityEstimator::GetRecentRTT(
+    nqe::internal::ObservationCategory observation_category,
     const base::TimeTicks& start_time,
     base::TimeDelta* rtt,
     size_t* observations_count) const {
   DCHECK(thread_checker_.CalledOnValidThread());
-  *rtt = GetRTTEstimateInternal(start_time,
-                                nqe::internal::OBSERVATION_CATEGORY_TRANSPORT,
-                                50, observations_count);
+  *rtt = GetRTTEstimateInternal(start_time, observation_category, 50,
+                                observations_count);
   return (*rtt != nqe::internal::InvalidRTT());
 }
 

net/nqe/network_quality_estimator.h

@@ -227,27 +227,16 @@ class NET_EXPORT NetworkQualityEstimator
   void OnConnectionTypeChanged(
       NetworkChangeNotifier::ConnectionType type) override;
 
-  // Returns true if median RTT at the HTTP layer is available and sets |rtt|
-  // to the median of RTT observations since |start_time|.
-  // Virtualized for testing. |rtt| should not be null. The RTT at the HTTP
-  // layer measures the time from when the request was sent (this happens after
-  // the connection is established) to the time when the response headers were
-  // received.
-  // TODO(tbansal): Change it to return HTTP RTT as base::TimeDelta.
-  virtual bool GetRecentHttpRTT(const base::TimeTicks& start_time,
-                                base::TimeDelta* rtt) const WARN_UNUSED_RESULT;
-
-  // Returns true if the median RTT at the transport layer is available and sets
-  // |rtt| to the median of transport layer RTT observations since
-  // |start_time|. |rtt| should not be null. Virtualized for testing.
-  // If |observations_count| is not null, then it is set to the number of
-  // transport RTT observations that are available when computing the RTT
-  // estimate.
-  // TODO(tbansal): Change it to return transport RTT as base::TimeDelta.
-  virtual bool GetRecentTransportRTT(const base::TimeTicks& start_time,
-                                     base::TimeDelta* rtt,
-                                     size_t* observations_count) const
-      WARN_UNUSED_RESULT;
+  // Returns true if median RTT across all samples that belong to
+  // |observation_category| is available and sets |rtt| to the median of RTT
+  // observations since |start_time|. Virtualized for testing. |rtt| should not
+  // be null. If |observations_count| is not null, then it is set to the number
+  // of RTT observations that were used for computing the RTT estimate.
+  virtual bool GetRecentRTT(
+      nqe::internal::ObservationCategory observation_category,
+      const base::TimeTicks& start_time,
+      base::TimeDelta* rtt,
+      size_t* observations_count) const WARN_UNUSED_RESULT;
 
   // Returns true if median downstream throughput is available and sets |kbps|
   // to the median of downstream throughput (in kilobits per second)

net/nqe/network_quality_estimator_test_util.cc

@@ -138,8 +138,10 @@ TestNetworkQualityEstimator::GetRecentEffectiveConnectionTypeAndNetworkQuality(
     int32_t* downstream_throughput_kbps,
     size_t* observations_count) const {
   if (recent_effective_connection_type_) {
-    GetRecentHttpRTT(start_time, http_rtt);
-    GetRecentTransportRTT(start_time, transport_rtt, observations_count);
+    GetRecentRTT(nqe::internal::OBSERVATION_CATEGORY_HTTP, start_time, http_rtt,
+                 nullptr);
+    GetRecentRTT(nqe::internal::OBSERVATION_CATEGORY_TRANSPORT, start_time,
+                 transport_rtt, observations_count);
     GetRecentDownlinkThroughputKbps(start_time, downstream_throughput_kbps);
     return recent_effective_connection_type_.value();
   }
@@ -149,46 +151,54 @@ TestNetworkQualityEstimator::GetRecentEffectiveConnectionTypeAndNetworkQuality(
           observations_count);
 }
 
-bool TestNetworkQualityEstimator::GetRecentHttpRTT(
-    const base::TimeTicks& start_time,
-    base::TimeDelta* rtt) const {
-  if (start_time.is_null()) {
-    if (start_time_null_http_rtt_) {
-      *rtt = start_time_null_http_rtt_.value();
-      return true;
-    }
-    return NetworkQualityEstimator::GetRecentHttpRTT(start_time, rtt);
-  }
-  if (recent_http_rtt_) {
-    *rtt = recent_http_rtt_.value();
-    return true;
-  }
-  return NetworkQualityEstimator::GetRecentHttpRTT(start_time, rtt);
-}
-
-bool TestNetworkQualityEstimator::GetRecentTransportRTT(
+bool TestNetworkQualityEstimator::GetRecentRTT(
+    nqe::internal::ObservationCategory observation_category,
     const base::TimeTicks& start_time,
     base::TimeDelta* rtt,
     size_t* observations_count) const {
-  if (start_time.is_null()) {
-    if (start_time_null_transport_rtt_) {
-      *rtt = start_time_null_transport_rtt_.value();
-      if (transport_rtt_observation_count_last_ect_computation_) {
-        *observations_count =
-            transport_rtt_observation_count_last_ect_computation_.value();
+  switch (observation_category) {
+    case nqe::internal::OBSERVATION_CATEGORY_HTTP:
+
+      if (start_time.is_null()) {
+        if (start_time_null_http_rtt_) {
+          *rtt = start_time_null_http_rtt_.value();
+          return true;
+        }
+        return NetworkQualityEstimator::GetRecentRTT(
+            observation_category, start_time, rtt, observations_count);
+      }
+      if (recent_http_rtt_) {
+        *rtt = recent_http_rtt_.value();
+        return true;
+      }
+      break;
+
+    case nqe::internal::OBSERVATION_CATEGORY_TRANSPORT:
+      if (start_time.is_null()) {
+        if (start_time_null_transport_rtt_) {
+          *rtt = start_time_null_transport_rtt_.value();
+          if (transport_rtt_observation_count_last_ect_computation_) {
+            *observations_count =
+                transport_rtt_observation_count_last_ect_computation_.value();
+          }
+          return true;
+        }
+        return NetworkQualityEstimator::GetRecentRTT(
+            observation_category, start_time, rtt, observations_count);
       }
-      return true;
-    }
-    return NetworkQualityEstimator::GetRecentTransportRTT(start_time, rtt,
-                                                          observations_count);
-  }
 
-  if (recent_transport_rtt_) {
-    *rtt = recent_transport_rtt_.value();
-    return true;
+      if (recent_transport_rtt_) {
+        *rtt = recent_transport_rtt_.value();
+        return true;
+      }
+      break;
+
+    case nqe::internal::OBSERVATION_CATEGORY_COUNT:
+      NOTREACHED();
   }
-  return NetworkQualityEstimator::GetRecentTransportRTT(start_time, rtt,
-                                                        observations_count);
+
+  return NetworkQualityEstimator::GetRecentRTT(observation_category, start_time,
+                                               rtt, observations_count);
 }
 
 base::Optional<base::TimeDelta> TestNetworkQualityEstimator::GetTransportRTT()

net/nqe/network_quality_estimator_test_util.h

@@ -122,11 +122,14 @@ class TestNetworkQualityEstimator : public NetworkQualityEstimator {
     DCHECK(!effective_connection_type_ && !recent_effective_connection_type_);
     recent_http_rtt_ = recent_http_rtt;
   }
-  // Returns the recent HTTP RTT that was set using |set_recent_http_rtt|. If
-  // the recent HTTP RTT has not been set, then the base implementation is
-  // called.
-  bool GetRecentHttpRTT(const base::TimeTicks& start_time,
-                        base::TimeDelta* rtt) const override;
+
+  // Returns the recent RTT that was set using set_recent_http_rtt() or
+  // set_recent_transport_rtt(). If the recent RTT has not been set, then the
+  // base implementation is called.
+  bool GetRecentRTT(nqe::internal::ObservationCategory observation_category,
+                    const base::TimeTicks& start_time,
+                    base::TimeDelta* rtt,
+                    size_t* observations_count) const override;
 
   // Force set the transport RTT estimate.
   void SetStartTimeNullTransportRtt(const base::TimeDelta transport_rtt);
@@ -140,13 +143,6 @@ class TestNetworkQualityEstimator : public NetworkQualityEstimator {
 
   base::Optional<base::TimeDelta> GetTransportRTT() const override;
 
-  // Returns the recent transport RTT that was set using
-  // |set_recent_transport_rtt|. If the recent transport RTT has not been set,
-  // then the base implementation is called.
-  bool GetRecentTransportRTT(const base::TimeTicks& start_time,
-                             base::TimeDelta* rtt,
-                             size_t* observations_count) const override;
-
   void set_start_time_null_downlink_throughput_kbps(
       int32_t downlink_throughput_kbps) {
     start_time_null_downlink_throughput_kbps_ = downlink_throughput_kbps;

net/nqe/network_quality_provider.h

@@ -44,11 +44,15 @@ class NET_EXPORT NetworkQualityProvider {
       EffectiveConnectionTypeObserver* observer) {}
 
   // Returns the current HTTP RTT estimate. If the estimate is unavailable,
-  // the returned optional value is null.
+  // the returned optional value is null. The RTT at the HTTP layer measures the
+  // time from when the request was sent (this happens after the connection is
+  // established) to the time when the response headers were received.
   virtual base::Optional<base::TimeDelta> GetHttpRTT() const;
 
   // Returns the current transport RTT estimate. If the estimate is
-  // unavailable, the returned optional value is null.
+  // unavailable, the returned optional value is null.  The RTT at the transport
+  // layer provides an aggregate estimate of the transport RTT as computed by
+  // various underlying TCP and QUIC connections.
   virtual base::Optional<base::TimeDelta> GetTransportRTT() const;
 
   // Returns the current downstream throughput estimate (in kilobits per