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..ea513460dd06eabf8f7c7b3c95615606110b1cf1 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;
@@ -278,6 +279,14 @@ class ResourceScheduler::Client {
 
   bool is_loaded() const { return is_loaded_; }
 
+  // Reset state to default values for client deletion.

[mmenke, 2014/07/25 16:33:27]

Should mention this must be called prior to deletion.  Maybe also rename this
ResetStateForDeletion?

+  void ResetState() {

[mmenke, 2014/07/25 16:33:27]

Why can't you just do this on destruction?  I seem to recall some re-entrancy
related reason, but I'm having trouble figuring out what it was.

[aiolos (Not reviewing), 2014/07/25 16:54:50]

Good point.

+    is_visible_ = false;
+    is_audible_ = false;
+    is_loaded_ = false;
+    UpdateThrottleState();

[mmenke, 2014/07/25 16:33:27]

This strikes me as regression-prone logic.  We're relying on a non-visible,
non-audible, unloaded client never to be in a state that the ResourceScheduler
tracks.  If that changes, things break, and it's hard to have DCHECKs that
verify that never changes.

You had talked about adding a paused state...Maybe add something like that and
just use it for state on destruction, for now?  Open to other ideas.

[aiolos (Not reviewing), 2014/07/25 16:54:50]

I'd considered doing that. I was planning on adding that in the patch limiting
the number of throttled tabs, to keep this CL smaller. But putting it in now
works too. I assume you'd like tests for paused state added at the same time?

[mmenke, 2014/07/25 17:02:41]

If we just use it for shutdown for now, and then more broadly later, I'm fine
with the current shutdown tests for the moment.

Also, don't think we'll need quite as many tests for pausing as we have for the
current behaviors - just one or two tests pausing and unpausing should probably
be enough.

[aiolos (Not reviewing), 2014/07/25 17:12:58]

Agreed. Although shouldn't sync requests get issued even for paused Clients so
we don't hang the render? I assume we should have a test for that as well.

[mmenke, 2014/07/25 17:22:19]

Good point - a test for that definitely makes sense.

+  }
+
   void OnAudibilityChanged(bool is_audible) {
     if (is_audible == is_audible_) {
       return;
@@ -315,6 +324,7 @@ class ResourceScheduler::Client {
     } else if (!is_active()) {
       SetThrottleState(UNTHROTTLED);
     }
+
     if (throttle_state_ == old_throttle_state) {
       return;
     }
@@ -323,6 +333,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() {
@@ -393,7 +408,7 @@ class ResourceScheduler::Client {
     return throttle_state_;
   }
 
-  void LoadCoalescedRequests() {
+  void LoadRequestsForAllCoalescedClients() {

[mmenke, 2014/07/25 16:33:27]

Undo this rename.

     if (throttle_state_ != COALESCED) {
       return;
     }
@@ -634,9 +649,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 +667,7 @@ void ResourceScheduler::SetThrottleOptionsForTesting(bool should_throttle,
                                                      bool should_coalesce) {
   should_coalesce_ = should_coalesce;
   should_throttle_ = should_throttle;
-  OnLoadingActiveClientsStateChanged();
+  OnLoadingActiveClientsStateChangedForAllClients();
 }
 
 ResourceScheduler::ClientThrottleState
@@ -722,7 +741,7 @@ void ResourceScheduler::OnClientDeleted(int child_id, int route_id) {
     return;
 
   Client* client = it->second;
-  client->OnLoadingStateChanged(true);
+  client->ResetState();
   // 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,11 +845,11 @@ 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;
@@ -839,9 +858,9 @@ void ResourceScheduler::OnLoadingActiveClientsStateChanged() {
   }
 }
 
-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());
+  if (coalesced_clients_ == 1) {
+    coalescing_timer_->Start(
+        FROM_HERE,
+        base::TimeDelta::FromMilliseconds(kCoalescedTimerPeriod),
+        base::Bind(&ResourceScheduler::LoadRequestsForAllCoalescedClients,
+                   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::LoadRequestsForAllCoalescedClients() {
+  ClientMap::iterator client_it = client_map_.begin();
+  while (client_it != client_map_.end()) {
+    Client* client = client_it->second;
+    client->LoadRequestsForAllCoalescedClients();
+    ++client_it;
+  }
+}
+
 void ResourceScheduler::ReprioritizeRequest(ScheduledResourceRequest* request,
                                             net::RequestPriority new_priority,
                                             int new_intra_priority_value) {