Reduce the number of calls to external estimate provider

Currently, the External Estimate Provider (EEP) may get
called multiple times per connection change. This CL ensures
that EEP is called exactly once per connection change.

This CL also updates the callback to UpdatedEstimateDelegate.
The callback now contains the updated estimates. This means
that the delegate does not have to query for those estimates
on receiving the callback. It makes the logic easier to
understand.

BUG=604417

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

chrome/browser/android/net/external_estimate_provider_android.cc

@@ -27,12 +27,10 @@ ExternalEstimateProviderAndroid::ExternalEstimateProviderAndroid()
           reinterpret_cast<intptr_t>(this)));
   DCHECK(!j_external_estimate_provider_.is_null());
   no_value_ = Java_ExternalEstimateProviderAndroid_getNoValue(env);
-  net::NetworkChangeNotifier::AddConnectionTypeObserver(this);
 }
 
 ExternalEstimateProviderAndroid::~ExternalEstimateProviderAndroid() {
   DCHECK(thread_checker_.CalledOnValidThread());
-  net::NetworkChangeNotifier::RemoveConnectionTypeObserver(this);
   Java_ExternalEstimateProviderAndroid_destroy(
       base::android::AttachCurrentThread(),
       j_external_estimate_provider_.obj());
@@ -106,11 +104,6 @@ void ExternalEstimateProviderAndroid::Update() const {
       env, j_external_estimate_provider_.obj());
 }
 
-void ExternalEstimateProviderAndroid::OnConnectionTypeChanged(
-    net::NetworkChangeNotifier::ConnectionType type) {
-  Update();
-}
-
 void ExternalEstimateProviderAndroid::
     NotifyExternalEstimateProviderAndroidUpdate(
         JNIEnv* env,
@@ -134,8 +127,20 @@ void ExternalEstimateProviderAndroid::
 
 void ExternalEstimateProviderAndroid::NotifyUpdatedEstimateAvailable() const {
   DCHECK(thread_checker_.CalledOnValidThread());
-  if (delegate_)
-    delegate_->OnUpdatedEstimateAvailable();
+
+  base::TimeDelta rtt;
+  GetRTT(&rtt);
+
+  int32_t downstream_throughput_kbps = -1;
+  GetDownstreamThroughputKbps(&downstream_throughput_kbps);
+
+  int32_t upstream_throughput_kbps = -1;
+  GetUpstreamThroughputKbps(&upstream_throughput_kbps);
+
+  if (delegate_) {
+    delegate_->OnUpdatedEstimateAvailable(rtt, downstream_throughput_kbps,
+                                          upstream_throughput_kbps);
+  }
 }
 
 bool RegisterExternalEstimateProviderAndroid(JNIEnv* env) {

chrome/browser/android/net/external_estimate_provider_android.h

@@ -15,7 +15,6 @@
 #include "base/threading/thread_checker.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/time/time.h"
-#include "net/base/network_change_notifier.h"
 #include "net/nqe/external_estimate_provider.h"
 
 namespace chrome {
@@ -25,9 +24,7 @@ namespace android {
 // ExternalEstimateProviderAndroidHelper.java. Provides network quality
 // estimates as provided by Android. Estimates are automatically updated on a
 // network change event.
-class ExternalEstimateProviderAndroid
-    : public net::NetworkChangeNotifier::ConnectionTypeObserver,
-      public net::ExternalEstimateProvider {
+class ExternalEstimateProviderAndroid : public net::ExternalEstimateProvider {
  public:
   // Constructs and initializes the underlying provider.
   ExternalEstimateProviderAndroid();
@@ -47,10 +44,6 @@ class ExternalEstimateProviderAndroid
       override;
   void Update() const override;
 
-  // NetworkChangeNotifier::ConnectionTypeObserver implementation.
-  void OnConnectionTypeChanged(
-      net::NetworkChangeNotifier::ConnectionType type) override;
-
   // Called by Java when the external estimate provider has an updated value.
   // This may be called on a thread different from |task_runner_|.
   void NotifyExternalEstimateProviderAndroidUpdate(

chrome/browser/android/net/external_estimate_provider_android_unittest.cc

@@ -46,12 +46,18 @@ class TestNetworkQualityEstimator : public net::NetworkQualityEstimator {
 
   ~TestNetworkQualityEstimator() override {}
 
-  void OnUpdatedEstimateAvailable() override {
+  void OnUpdatedEstimateAvailable(const base::TimeDelta& rtt,
+                                  int32_t downstream_throughput_kbps,
+                                  int32_t upstream_throughput_kbps) override {
+    EXPECT_EQ(base::TimeDelta(), rtt);
+    EXPECT_EQ(-1, downstream_throughput_kbps);
+    EXPECT_EQ(-1, upstream_throughput_kbps);
     notified_ = true;
-    net::NetworkQualityEstimator::OnUpdatedEstimateAvailable();
+    net::NetworkQualityEstimator::OnUpdatedEstimateAvailable(
+        rtt, downstream_throughput_kbps, upstream_throughput_kbps);
   }
 
-  bool IsNotified() const { return notified_; }
+  bool notified() const { return notified_; }
 
  private:
   bool notified_;
@@ -67,7 +73,7 @@ class TestExternalEstimateProviderAndroid
 
   bool GetTimeSinceLastUpdate(
       base::TimeDelta* time_since_last_update) const override {
-    *time_since_last_update = base::TimeDelta::FromMilliseconds(1);
+    *time_since_last_update = base::TimeDelta::FromMilliseconds(0);
     return true;
   }
 };
@@ -89,33 +95,17 @@ TEST(ExternalEstimateProviderAndroidTest, DelegateTest) {
   TestNetworkQualityEstimator network_quality_estimator(
       std::move(external_estimate_provider), variation_params);
   ptr->NotifyUpdatedEstimateAvailable();
-  DCHECK(network_quality_estimator.IsNotified());
+  EXPECT_TRUE(network_quality_estimator.notified());
 
-  // EXTERNAL_ESTIMATE_PROVIDER_STATUS_NOT_AVAILABLE
-  histogram_tester.ExpectBucketCount("NQE.ExternalEstimateProviderStatus", 0,
-                                     0);
+  histogram_tester.ExpectTotalCount("NQE.ExternalEstimateProviderStatus", 2);
 
   // EXTERNAL_ESTIMATE_PROVIDER_STATUS_AVAILABLE
   histogram_tester.ExpectBucketCount("NQE.ExternalEstimateProviderStatus", 1,
                                      1);
 
-  // EXTERNAL_ESTIMATE_PROVIDER_STATUS_QUERIED
-  // Updated once during NetworkQualityEstimator constructor and again,
-  // when |OnUpdatedEstimateAvailable| is called.
-  histogram_tester.ExpectBucketCount("NQE.ExternalEstimateProviderStatus", 2,
-                                     2);
-
-  // EXTERNAL_ESTIMATE_PROVIDER_STATUS_QUERY_SUCCESSFUL
-  // Updated once during NetworkQualityEstimator constructor and again,
-  // when |OnUpdatedEstimateAvailable| is called.
-  histogram_tester.ExpectBucketCount("NQE.ExternalEstimateProviderStatus", 3,
-                                     2);
-
   // EXTERNAL_ESTIMATE_PROVIDER_STATUS_CALLBACK
   histogram_tester.ExpectBucketCount("NQE.ExternalEstimateProviderStatus", 4,
                                      1);
-
-  histogram_tester.ExpectTotalCount("NQE.ExternalEstimateProviderStatus", 6);
 }
 
 }  // namespace

net/nqe/external_estimate_provider.h

@@ -19,8 +19,15 @@ class NET_EXPORT ExternalEstimateProvider {
  public:
   class NET_EXPORT UpdatedEstimateDelegate {
    public:
-    // Will be called when an updated estimate is available.
-    virtual void OnUpdatedEstimateAvailable() = 0;
+    // Will be called with updated RTT, downstream and upstream throughput (both
+    // in kilobits per second) when an updated estimate is available. If |rtt|
+    // is unavailable, it is set to base::TimeDelta(). If
+    // |downstream_throughput_kbps| or |upstream_throughput_kbps| are
+    // unavailble, they are set to -1, respectively.
+    virtual void OnUpdatedEstimateAvailable(
+        const base::TimeDelta& rtt,
+        int32_t downstream_throughput_kbps,
+        int32_t upstream_throughput_kbps) = 0;
 
    protected:
     UpdatedEstimateDelegate() {}

net/nqe/network_quality_estimator.cc

@@ -276,7 +276,6 @@ NetworkQualityEstimator::NetworkQualityEstimator(
     RecordExternalEstimateProviderMetrics(
         EXTERNAL_ESTIMATE_PROVIDER_STATUS_AVAILABLE);
     external_estimate_provider_->SetUpdatedEstimateDelegate(this);
-    QueryExternalEstimateProvider();
   } else {
     RecordExternalEstimateProviderMetrics(
         EXTERNAL_ESTIMATE_PROVIDER_STATUS_NOT_AVAILABLE);
@@ -618,7 +617,18 @@ void NetworkQualityEstimator::OnConnectionTypeChanged(
   rtt_observations_.Clear();
   current_network_id_ = GetCurrentNetworkID();
 
-  QueryExternalEstimateProvider();
+  // Query the external estimate provider on certain connection types. Once the
+  // updated estimates are available, OnUpdatedEstimateAvailable will be called
+  // by |external_estimate_provider_| with updated estimates.
+  if (external_estimate_provider_ &&
+      current_network_id_.type != NetworkChangeNotifier::CONNECTION_NONE &&
+      current_network_id_.type != NetworkChangeNotifier::CONNECTION_UNKNOWN &&
+      current_network_id_.type != NetworkChangeNotifier::CONNECTION_ETHERNET &&
+      current_network_id_.type != NetworkChangeNotifier::CONNECTION_BLUETOOTH) {
+    RecordExternalEstimateProviderMetrics(
+        EXTERNAL_ESTIMATE_PROVIDER_STATUS_QUERIED);
+    external_estimate_provider_->Update();
+  }
 
   // Read any cached estimates for the new network. If cached estimates are
   // unavailable, add the default estimates.
@@ -966,13 +976,35 @@ bool NetworkQualityEstimator::ReadCachedNetworkQualityEstimate() {
   return read_cached_estimate;
 }
 
-void NetworkQualityEstimator::OnUpdatedEstimateAvailable() {
+void NetworkQualityEstimator::OnUpdatedEstimateAvailable(
+    const base::TimeDelta& rtt,
+    int32_t downstream_throughput_kbps,
+    int32_t upstream_throughput_kbps) {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(external_estimate_provider_);
 
   RecordExternalEstimateProviderMetrics(
       EXTERNAL_ESTIMATE_PROVIDER_STATUS_CALLBACK);
-  QueryExternalEstimateProvider();
+
+  if (rtt > base::TimeDelta()) {
+    RecordExternalEstimateProviderMetrics(
+        EXTERNAL_ESTIMATE_PROVIDER_STATUS_RTT_AVAILABLE);
+    UMA_HISTOGRAM_TIMES("NQE.ExternalEstimateProvider.RTT", rtt);
+    rtt_observations_.AddObservation(
+        RttObservation(rtt, base::TimeTicks::Now(),
+                       NETWORK_QUALITY_OBSERVATION_SOURCE_EXTERNAL_ESTIMATE));
+  }
+
+  if (downstream_throughput_kbps > 0) {
+    RecordExternalEstimateProviderMetrics(
+        EXTERNAL_ESTIMATE_PROVIDER_STATUS_DOWNLINK_BANDWIDTH_AVAILABLE);
+    UMA_HISTOGRAM_COUNTS("NQE.ExternalEstimateProvider.DownlinkBandwidth",
+                         downstream_throughput_kbps);
+    downstream_throughput_kbps_observations_.AddObservation(
+        ThroughputObservation(
+            downstream_throughput_kbps, base::TimeTicks::Now(),
+            NETWORK_QUALITY_OBSERVATION_SOURCE_EXTERNAL_ESTIMATE));
+  }
 }
 
 const char* NetworkQualityEstimator::GetNameForEffectiveConnectionType(
@@ -999,56 +1031,6 @@ const char* NetworkQualityEstimator::GetNameForEffectiveConnectionType(
   return "";
 }
 
-void NetworkQualityEstimator::QueryExternalEstimateProvider() {
-  DCHECK(thread_checker_.CalledOnValidThread());
-
-  if (!external_estimate_provider_)
-    return;
-  RecordExternalEstimateProviderMetrics(
-      EXTERNAL_ESTIMATE_PROVIDER_STATUS_QUERIED);
-
-  base::TimeDelta time_since_last_update;
-
-  // Request a new estimate if estimate is not available, or if the available
-  // estimate is not fresh.
-  if (!external_estimate_provider_->GetTimeSinceLastUpdate(
-          &time_since_last_update) ||
-      time_since_last_update >
-          base::TimeDelta::FromMilliseconds(
-              kExternalEstimateProviderFreshnessDurationMsec)) {
-    // Request the external estimate provider for updated estimates. When the
-    // updates estimates are available, OnUpdatedEstimateAvailable() will be
-    // called.
-    external_estimate_provider_->Update();
-    return;
-  }
-
-  RecordExternalEstimateProviderMetrics(
-      EXTERNAL_ESTIMATE_PROVIDER_STATUS_QUERY_SUCCESSFUL);
-  base::TimeDelta rtt;
-  if (external_estimate_provider_->GetRTT(&rtt)) {
-    RecordExternalEstimateProviderMetrics(
-        EXTERNAL_ESTIMATE_PROVIDER_STATUS_RTT_AVAILABLE);
-    UMA_HISTOGRAM_TIMES("NQE.ExternalEstimateProvider.RTT", rtt);
-    rtt_observations_.AddObservation(
-        RttObservation(rtt, base::TimeTicks::Now(),
-                       NETWORK_QUALITY_OBSERVATION_SOURCE_EXTERNAL_ESTIMATE));
-  }
-
-  int32_t downstream_throughput_kbps;
-  if (external_estimate_provider_->GetDownstreamThroughputKbps(
-          &downstream_throughput_kbps)) {
-    RecordExternalEstimateProviderMetrics(
-        EXTERNAL_ESTIMATE_PROVIDER_STATUS_DOWNLINK_BANDWIDTH_AVAILABLE);
-    UMA_HISTOGRAM_COUNTS("NQE.ExternalEstimateProvider.DownlinkBandwidth",
-                         downstream_throughput_kbps);
-    downstream_throughput_kbps_observations_.AddObservation(
-        ThroughputObservation(
-            downstream_throughput_kbps, base::TimeTicks::Now(),
-            NETWORK_QUALITY_OBSERVATION_SOURCE_EXTERNAL_ESTIMATE));
-  }
-}
-
 void NetworkQualityEstimator::CacheNetworkQualityEstimate() {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK_LE(cached_network_qualities_.size(),

net/nqe/network_quality_estimator.h

@@ -265,7 +265,9 @@ class NET_EXPORT_PRIVATE NetworkQualityEstimator
       NetworkChangeNotifier::ConnectionType type) override;
 
   // ExternalEstimateProvider::UpdatedEstimateObserver implementation.
-  void OnUpdatedEstimateAvailable() override;
+  void OnUpdatedEstimateAvailable(const base::TimeDelta& rtt,
+                                  int32_t downstream_throughput_kbps,
+                                  int32_t upstream_throughput_kbps) override;
 
   // Return a string equivalent to |type|.
   const char* GetNameForEffectiveConnectionType(
@@ -315,11 +317,6 @@ class NET_EXPORT_PRIVATE NetworkQualityEstimator
   // Larger size may affect performance.
   static const size_t kMaximumNetworkQualityCacheSize = 10;
 
-  // Time duration (in milliseconds) after which the estimate provided by
-  // external estimate provider is considered stale.
-  static const int kExternalEstimateProviderFreshnessDurationMsec =
-      5 * 60 * 1000;
-
   // Returns the RTT value to be used when the valid RTT is unavailable. Readers
   // should discard RTT if it is set to the value returned by |InvalidRTT()|.
   static const base::TimeDelta InvalidRTT();
@@ -339,10 +336,6 @@ class NET_EXPORT_PRIVATE NetworkQualityEstimator
   void OnUpdatedRTTAvailable(SocketPerformanceWatcherFactory::Protocol protocol,
                              const base::TimeDelta& rtt);
 
-  // Queries the external estimate provider for the latest network quality
-  // estimates, and adds those estimates to the current observation buffer.
-  void QueryExternalEstimateProvider();
-
   // Obtains operating parameters from the field trial parameters.
   void ObtainOperatingParams(
       const std::map<std::string, std::string>& variation_params);