Add logic to dispatch queued requests if ECT changes

Dispatch requests that are currently queued if the
effective connection type (ECT) changes since the newer
ECT may allow more requests to go in flight.

Change-Id: I095feaaf17965909c3dc36612da0960324e7f6ea
Bug: 	913424
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1692482Reviewed-by: Doug Arnett <dougarnett@chromium.org>
Commit-Queue: Tarun Bansal <tbansal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#675746}

services/network/resource_scheduler.cc

@@ -61,6 +61,7 @@ enum class RequestStartTrigger {
   SPDY_PROXY_DETECTED,
   REQUEST_REPRIORITIZED,
   LONG_QUEUED_REQUESTS_TIMER_FIRED,
+  EFFECTIVE_CONNECTION_TYPE_CHANGED,
 };
 
 const char* RequestStartTriggerString(RequestStartTrigger trigger) {
@@ -81,6 +82,8 @@ const char* RequestStartTriggerString(RequestStartTrigger trigger) {
       return "REQUEST_REPRIORITIZED";
     case RequestStartTrigger::LONG_QUEUED_REQUESTS_TIMER_FIRED:
       return "LONG_QUEUED_REQUESTS_TIMER_FIRED";
+    case RequestStartTrigger::EFFECTIVE_CONNECTION_TYPE_CHANGED:
+      return "EFFECTIVE_CONNECTION_TYPE_CHANGED";
   }
 }
 
@@ -375,9 +378,12 @@ class ResourceScheduler::Client : public net::EffectiveConnectionTypeObserver {
         tick_clock_(tick_clock) {
     DCHECK(tick_clock_);
 
-    UpdateParamsForNetworkQuality();
-    if (network_quality_estimator_)
+    if (network_quality_estimator_) {
+      effective_connection_type_ =
+          network_quality_estimator_->GetEffectiveConnectionType();
+      UpdateParamsForNetworkQuality();
       network_quality_estimator_->AddEffectiveConnectionTypeObserver(this);
+    }
   }
 
   ~Client() override {
@@ -477,7 +483,7 @@ class ResourceScheduler::Client : public net::EffectiveConnectionTypeObserver {
         resource_scheduler_->resource_scheduler_params_manager_
             .GetParamsForEffectiveConnectionType(
                 network_quality_estimator_
-                    ? network_quality_estimator_->GetEffectiveConnectionType()
+                    ? effective_connection_type_
                     : net::EFFECTIVE_CONNECTION_TYPE_UNKNOWN);
   }
 
@@ -504,11 +510,19 @@ class ResourceScheduler::Client : public net::EffectiveConnectionTypeObserver {
     START_REQUEST
   };
 
-  // net::EffectiveConnectionTypeObserver implementation:
+  // net::EffectiveConnectionTypeObserver:
   void OnEffectiveConnectionTypeChanged(
       net::EffectiveConnectionType effective_connection_type) override {
     DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+    if (effective_connection_type_ == effective_connection_type)
+      return;
+
+    effective_connection_type_ = effective_connection_type;
     UpdateParamsForNetworkQuality();
+
+    // Change in network quality may allow |this| to dispatch more requests.
+    LoadAnyStartablePendingRequests(
+        RequestStartTrigger::EFFECTIVE_CONNECTION_TYPE_CHANGED);
   }
 
   // Records the metrics related to number of requests in flight.
@@ -822,12 +836,13 @@ class ResourceScheduler::Client : public net::EffectiveConnectionTypeObserver {
 
   ShouldStartReqResult ShouldStartRequest(
       ScheduledResourceRequestImpl* request) const {
-    // Currently, browser initiated requests are not throttled.
-    if (is_browser_client_)
+    if (!resource_scheduler_->enabled())
       return START_REQUEST;
 
-    if (!resource_scheduler_->enabled())
+    // Currently, browser initiated requests are not throttled.
+    if (is_browser_client_) {
       return START_REQUEST;
+    }
 
     const net::URLRequest& url_request = *request->url_request();
     // Syncronous requests could block the entire render, which could impact
@@ -1049,6 +1064,10 @@ class ResourceScheduler::Client : public net::EffectiveConnectionTypeObserver {
   // Time when the last non-delayble request ended in this client.
   base::Optional<base::TimeTicks> last_non_delayable_request_end_;
 
+  // Current estimated value of the effective connection type.
+  net::EffectiveConnectionType effective_connection_type_ =
+      net::EFFECTIVE_CONNECTION_TYPE_UNKNOWN;
+
   SEQUENCE_CHECKER(sequence_checker_);
 
   base::WeakPtrFactory<ResourceScheduler::Client> weak_ptr_factory_{this};

services/network/resource_scheduler_unittest.cc

@@ -478,6 +478,9 @@ TEST_F(ResourceSchedulerTest, OneIsolatedLowRequest) {
 
 TEST_F(ResourceSchedulerTest, OneLowLoadsUntilCriticalComplete) {
   base::HistogramTester histogram_tester;
+  network_quality_estimator_.SetAndNotifyObserversOfEffectiveConnectionType(
+      net::EFFECTIVE_CONNECTION_TYPE_4G);
+  InitializeScheduler();
 
   SetMaxDelayableRequests(1);
   std::unique_ptr<TestRequest> high(
@@ -1010,9 +1013,9 @@ TEST_F(ResourceSchedulerTest, RequestLimitOverrideOutsideECTRange) {
   }
 }
 
-// Test that a change in network conditions midway during loading does not
-// change the behavior of the resource scheduler.
-TEST_F(ResourceSchedulerTest, RequestLimitOverrideFixedForPageLoad) {
+// Test that a change in network conditions midway during loading
+// changes the behavior of the resource scheduler.
+TEST_F(ResourceSchedulerTest, RequestLimitOverrideNotFixedForPageLoad) {
   InitializeThrottleDelayableExperiment(true, 0.0);
   // ECT value is in range for which the limit is overridden to 2.
   network_quality_estimator_.SetAndNotifyObserversOfEffectiveConnectionType(
@@ -1047,18 +1050,26 @@ TEST_F(ResourceSchedulerTest, RequestLimitOverrideFixedForPageLoad) {
   base::RunLoop().RunUntilIdle();
   EXPECT_TRUE(second_last_singlehost->started());
 
+  //  |last_singlehost_before_ect_change| should not start because of the
+  //  limits.
+  std::unique_ptr<TestRequest> last_singlehost_before_ect_change(
+      NewRequest("http://host/last_singlehost_before_ect_change", net::LOWEST));
+  EXPECT_FALSE(last_singlehost_before_ect_change->started());
+
   // Change the ECT to go outside the experiment buckets and change the network
-  // type to 4G. This should affect the limit calculated at the beginning of
-  // the page load.
+  // type to 4G. This should affect the limit which should affect requests
+  // that are already queued (e.g., |last_singlehost_before_ect_change|), and
+  // requests that arrive later (e.g., |last_singlehost_after_ect_change|).
   network_quality_estimator_.SetAndNotifyObserversOfEffectiveConnectionType(
       net::EFFECTIVE_CONNECTION_TYPE_4G);
   base::RunLoop().RunUntilIdle();
 
-  std::unique_ptr<TestRequest> last_singlehost(
-      NewRequest("http://host/last", net::LOWEST));
+  std::unique_ptr<TestRequest> last_singlehost_after_ect_change(
+      NewRequest("http://host/last_singlehost_after_ect_change", net::LOWEST));
 
-  // Last should start because the limit should have changed.
-  EXPECT_TRUE(last_singlehost->started());
+  // Both requests should start because the limits should have changed.
+  EXPECT_TRUE(last_singlehost_before_ect_change->started());
+  EXPECT_TRUE(last_singlehost_after_ect_change->started());
 }
 
 // Test that when the network quality changes such that the new limit is lower,