Add coalescing timer to ResourceScheduler. CL 2 from BUG=128035.

content/browser/loader/resource_scheduler.cc

Index: content/browser/loader/resource_scheduler.cc
diff --git a/content/browser/loader/resource_scheduler.cc b/content/browser/loader/resource_scheduler.cc
index fadbc2933ac89c3ba56048bfeea732171fca16a9..bc4cee88f28b3e62cfab95b48d4c08d187dc4b31 100644
--- a/content/browser/loader/resource_scheduler.cc
+++ b/content/browser/loader/resource_scheduler.cc
@@ -22,6 +22,7 @@
 
 namespace content {
 
+static const size_t kCoalescedTimerPeriod = 5000;
 static const size_t kMaxNumDelayableRequestsPerClient = 10;
 static const size_t kMaxNumDelayableRequestsPerHost = 6;
 static const size_t kMaxNumThrottledRequestsPerClient = 1;
@@ -239,7 +240,14 @@ class ResourceScheduler::Client {
         throttle_state_(ResourceScheduler::THROTTLED) {
     scheduler_ = scheduler;
   }
-  ~Client() {}
+
+  ~Client() {
+    // Update to default state and pause to ensure the scheduler has a
+    // correct count of important types of clients.

[mmenke, 2014/07/29 15:43:26]

nit:  I think "important" is a bit weird here.  Maybe "various" or "relevant"? 
(Admittedly, the latter can be a synonym for important).

[aiolos (Not reviewing), 2014/07/29 17:58:51]

Done.

+    is_visible_ = false;
+    is_audible_ = false;
+    UpdateThrottleState(true /* should_pause */);
+  }
 
   void ScheduleRequest(
       net::URLRequest* url_request,
@@ -283,7 +291,7 @@ class ResourceScheduler::Client {
       return;
     }
     is_audible_ = is_audible;
-    UpdateThrottleState();
+    UpdateThrottleState(false /* should_pause */);
   }
 
   void OnVisibilityChanged(bool is_visible) {
@@ -291,7 +299,7 @@ class ResourceScheduler::Client {
       return;
     }
     is_visible_ = is_visible;
-    UpdateThrottleState();
+    UpdateThrottleState(false /* should_pause */);
   }
 
   void OnLoadingStateChanged(bool is_loaded) {
@@ -299,12 +307,14 @@ class ResourceScheduler::Client {
       return;
     }
     is_loaded_ = is_loaded;
-    UpdateThrottleState();
+    UpdateThrottleState(false /* should_pause */);
   }
 
-  void UpdateThrottleState() {
+  void UpdateThrottleState(bool should_pause) {
     ClientThrottleState old_throttle_state = throttle_state_;
-    if (is_active() && !is_loaded_) {
+    if (should_pause) {
+      SetThrottleState(PAUSED);
+    } else if (is_active() && !is_loaded_) {
       SetThrottleState(ACTIVE_AND_LOADING);
     } else if (is_active()) {
       SetThrottleState(UNTHROTTLED);
@@ -315,6 +325,7 @@ class ResourceScheduler::Client {
     } else if (!is_active()) {
       SetThrottleState(UNTHROTTLED);
     }
+
     if (throttle_state_ == old_throttle_state) {
       return;
     }
@@ -323,6 +334,11 @@ class ResourceScheduler::Client {
     } else if (old_throttle_state == ACTIVE_AND_LOADING) {
       scheduler_->DecrementActiveClientsLoading();
     }
+    if (throttle_state_ == COALESCED) {
+      scheduler_->IncrementCoalescedClients();
+    } else if (old_throttle_state == COALESCED) {
+      scheduler_->DecrementCoalescedClients();
+    }
   }
 
   void OnNavigate() {
@@ -634,9 +650,13 @@ class ResourceScheduler::Client {
   ResourceScheduler::ClientThrottleState throttle_state_;
 };
 
-ResourceScheduler::ResourceScheduler(): should_coalesce_(false),
-                                        should_throttle_(false),
-                                        active_clients_loading_(0) {
+ResourceScheduler::ResourceScheduler()
+    : should_coalesce_(false),
+      should_throttle_(false),
+      active_clients_loading_(0),
+      coalesced_clients_(0),
+      coalescing_timer_(new base::Timer(true /* retain_user_task */,
+                                        true /* is_repeating */)) {
 }
 
 ResourceScheduler::~ResourceScheduler() {
@@ -648,7 +668,7 @@ void ResourceScheduler::SetThrottleOptionsForTesting(bool should_throttle,
                                                      bool should_coalesce) {
   should_coalesce_ = should_coalesce;
   should_throttle_ = should_throttle;
-  OnLoadingActiveClientsStateChanged();
+  OnLoadingActiveClientsStateChangedForAllClients();
 }
 
 ResourceScheduler::ClientThrottleState
@@ -710,7 +730,7 @@ void ResourceScheduler::OnClientCreated(int child_id, int route_id) {
 
   // TODO(aiolos): set Client visibility/audibility when signals are added
   // this will UNTHROTTLE Clients as needed
-  client->UpdateThrottleState();
+  client->UpdateThrottleState(false /* should_paused */);
 }
 
 void ResourceScheduler::OnClientDeleted(int child_id, int route_id) {
@@ -722,7 +742,6 @@ void ResourceScheduler::OnClientDeleted(int child_id, int route_id) {
     return;
 
   Client* client = it->second;
-  client->OnLoadingStateChanged(true);
   // FYI, ResourceDispatcherHost cancels all of the requests after this function
   // is called. It should end up canceling all of the requests except for a
   // cross-renderer navigation.
@@ -818,7 +837,7 @@ void ResourceScheduler::DecrementActiveClientsLoading() {
   --active_clients_loading_;
   DCHECK_EQ(active_clients_loading_, CountActiveClientsLoading());
   if (active_clients_loading_ == 0) {
-    OnLoadingActiveClientsStateChanged();
+    OnLoadingActiveClientsStateChangedForAllClients();
   }
 }
 
@@ -826,22 +845,22 @@ void ResourceScheduler::IncrementActiveClientsLoading() {
   ++active_clients_loading_;
   DCHECK_EQ(active_clients_loading_, CountActiveClientsLoading());
   if (active_clients_loading_ == 1) {
-    OnLoadingActiveClientsStateChanged();
+    OnLoadingActiveClientsStateChangedForAllClients();
   }
 }
 
-void ResourceScheduler::OnLoadingActiveClientsStateChanged() {
+void ResourceScheduler::OnLoadingActiveClientsStateChangedForAllClients() {
   ClientMap::iterator client_it = client_map_.begin();
   while (client_it != client_map_.end()) {
     Client* client = client_it->second;
-    client->UpdateThrottleState();
+    client->UpdateThrottleState(false /* should_pause */);

[mmenke, 2014/07/29 15:43:26]

This unpauses all clients.  While not yet a bug, it seems potentially
problematic.

My suggestion:

Add a function:

Client::SetPaused(bool is_paused) {
  is_paused_ = is_paused;
  UpdateThrottleState();
}

And add a check for is_paused_ in update throttle state.  Open to other ideas.

[aiolos (Not reviewing), 2014/07/29 17:58:51]

Done.

     ++client_it;
   }
 }
 
-size_t ResourceScheduler::CountActiveClientsLoading() {
+size_t ResourceScheduler::CountActiveClientsLoading() const {
   size_t active_and_loading = 0;
-  ClientMap::iterator client_it = client_map_.begin();
+  ClientMap::const_iterator client_it = client_map_.begin();
   while (client_it != client_map_.end()) {
     Client* client = client_it->second;
     if (client->throttle_state() == ACTIVE_AND_LOADING) {
@@ -852,6 +871,49 @@ size_t ResourceScheduler::CountActiveClientsLoading() {
   return active_and_loading;
 }
 
+void ResourceScheduler::IncrementCoalescedClients() {
+  ++coalesced_clients_;
+  DCHECK_EQ(coalesced_clients_, CountCoalescedClients());

[mmenke, 2014/07/29 15:43:26]

DCHECK(should_coalesce_), maybe?

Could also put that in some of these other functions, I believe.

[aiolos (Not reviewing), 2014/07/29 17:58:51]

Done.

+  if (coalesced_clients_ == 1) {
+    coalescing_timer_->Start(
+        FROM_HERE,
+        base::TimeDelta::FromMilliseconds(kCoalescedTimerPeriod),
+        base::Bind(&ResourceScheduler::LoadCoalescedRequests,
+                   base::Unretained(this)));
+  }
+}
+
+void ResourceScheduler::DecrementCoalescedClients() {
+  DCHECK_NE(0U, coalesced_clients_);
+  --coalesced_clients_;
+  DCHECK_EQ(coalesced_clients_, CountCoalescedClients());
+  if (coalesced_clients_ == 0) {
+    coalescing_timer_->Stop();
+  }
+}
+
+size_t ResourceScheduler::CountCoalescedClients() const {
+  size_t coalesced_clients = 0;
+  ClientMap::const_iterator client_it = client_map_.begin();
+  while (client_it != client_map_.end()) {
+    Client* client = client_it->second;
+    if (client->throttle_state() == COALESCED) {
+      ++coalesced_clients;
+    }
+    ++client_it;
+  }
+  return coalesced_clients_;
+}
+
+void ResourceScheduler::LoadCoalescedRequests() {
+  ClientMap::iterator client_it = client_map_.begin();
+  while (client_it != client_map_.end()) {
+    Client* client = client_it->second;
+    client->LoadCoalescedRequests();
+    ++client_it;
+  }
+}
+
 void ResourceScheduler::ReprioritizeRequest(ScheduledResourceRequest* request,
                                             net::RequestPriority new_priority,
                                             int new_intra_priority_value) {