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

content/browser/loader/resource_scheduler_unittest.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/loader/resource_scheduler.h"
 
 #include "base/memory/scoped_vector.h"
 #include "base/message_loop/message_loop.h"
 #include "base/strings/string_number_conversions.h"
+#include "base/timer/mock_timer.h"
+#include "base/timer/timer.h"
 #include "content/browser/browser_thread_impl.h"
 #include "content/browser/loader/resource_dispatcher_host_impl.h"
 #include "content/browser/loader/resource_message_filter.h"
 #include "content/browser/loader/resource_request_info_impl.h"
 #include "content/common/resource_messages.h"
 #include "content/public/browser/resource_context.h"
 #include "content/public/browser/resource_controller.h"
 #include "content/public/browser/resource_throttle.h"
 #include "content/public/common/process_type.h"
 #include "content/public/common/resource_type.h"
@@ skipping 110 matching lines @@
 
   FakeResourceContext context_;
 };
 
 class ResourceSchedulerTest : public testing::Test {
  protected:
   ResourceSchedulerTest()
       : next_request_id_(0),
         ui_thread_(BrowserThread::UI, &message_loop_),
         io_thread_(BrowserThread::IO, &message_loop_) {
+    mock_timer_ = new base::MockTimer(true, true);
+    scheduler_.set_timer_for_testing(scoped_ptr<base::Timer>(mock_timer_));
+
+    // TODO(aiolos): remove when throttling and coalescing have both landed
+    scheduler_.SetThrottleOptionsForTesting(true /* should_throttle */,
+                                            false /* should_coalesce */);
 
     scheduler_.OnClientCreated(kChildId, kRouteId);
     scheduler_.OnVisibilityChanged(kChildId, kRouteId, true);
     scheduler_.OnClientCreated(kBackgroundChildId, kBackgroundRouteId);
     context_.set_http_server_properties(http_server_properties_.GetWeakPtr());
   }
 
   virtual ~ResourceSchedulerTest() {
     scheduler_.OnClientDeleted(kChildId, kRouteId);
     scheduler_.OnClientDeleted(kBackgroundChildId, kBackgroundRouteId);
@@ skipping 100 matching lines @@
     scoped_refptr<FakeResourceMessageFilter> filter(
         new FakeResourceMessageFilter(kChildId));
     const ResourceRequestInfoImpl* info = ResourceRequestInfoImpl::ForRequest(
         request->url_request());
     const GlobalRequestID& id = info->GetGlobalRequestID();
     ResourceHostMsg_DidChangePriority msg(id.request_id, new_priority,
                                           intra_priority);
     rdh_.OnMessageReceived(msg, filter.get());
   }
 
+  void FireCoalescingTimer() {
+    EXPECT_TRUE(mock_timer_->IsRunning());
+    mock_timer_->Fire();
+  }
+
   int next_request_id_;
   base::MessageLoopForIO message_loop_;
   BrowserThreadImpl ui_thread_;
   BrowserThreadImpl io_thread_;
   ResourceDispatcherHostImpl rdh_;
   ResourceScheduler scheduler_;
+  base::MockTimer* mock_timer_;
   net::HttpServerPropertiesImpl http_server_properties_;
   net::TestURLRequestContext context_;
 };
 
 TEST_F(ResourceSchedulerTest, OneIsolatedLowRequest) {
   scoped_ptr<TestRequest> request(NewRequest("http://host/1", net::LOWEST));
   EXPECT_TRUE(request->started());
 }
 
 TEST_F(ResourceSchedulerTest, OneLowLoadsUntilIdle) {
@@ skipping 1468 matching lines @@
                                                 kBackgroundRouteId));
   EXPECT_EQ(ResourceScheduler::THROTTLED,
             scheduler_.GetClientStateForTesting(kBackgroundChildId2,
                                                 kBackgroundRouteId2));
   EXPECT_EQ(ResourceScheduler::ACTIVE_AND_LOADING,
             scheduler_.GetClientStateForTesting(kChildId, kRouteId));
 
   scheduler_.OnClientDeleted(kBackgroundChildId2, kBackgroundRouteId2);
 }
 
+TEST_F(ResourceSchedulerTest, CoalescedClientCreationStartsTimer) {
+  scheduler_.SetThrottleOptionsForTesting(true /* should_throttle */,
+                                          true /* should_coalesce */);
+  EXPECT_FALSE(mock_timer_->IsRunning());
+  scheduler_.OnLoadingStateChanged(kChildId, kRouteId, true);
+  EXPECT_FALSE(mock_timer_->IsRunning());
+  scheduler_.OnLoadingStateChanged(
+      kBackgroundChildId, kBackgroundRouteId, true);
+  EXPECT_EQ(ResourceScheduler::COALESCED,
+            scheduler_.GetClientStateForTesting(kBackgroundChildId,
+                                                kBackgroundRouteId));
+  EXPECT_TRUE(mock_timer_->IsRunning());
+}
+
+TEST_F(ResourceSchedulerTest, LastCoalescedClientDeletionStopsTimer) {
+  scheduler_.SetThrottleOptionsForTesting(true /* should_throttle */,
+                                          true /* should_coalesce */);
+  scheduler_.OnClientCreated(kBackgroundChildId2, kBackgroundRouteId2);
+  EXPECT_FALSE(mock_timer_->IsRunning());
+  scheduler_.OnLoadingStateChanged(kChildId, kRouteId, true);
+  EXPECT_FALSE(mock_timer_->IsRunning());
+  scheduler_.OnLoadingStateChanged(
+      kBackgroundChildId, kBackgroundRouteId, true);
+  EXPECT_EQ(ResourceScheduler::COALESCED,
+            scheduler_.GetClientStateForTesting(kBackgroundChildId,
+                                                kBackgroundRouteId));
+  scheduler_.OnLoadingStateChanged(
+      kBackgroundChildId2, kBackgroundRouteId2, true);
+  EXPECT_EQ(ResourceScheduler::COALESCED,
+            scheduler_.GetClientStateForTesting(kBackgroundChildId2,
+                                                kBackgroundRouteId2));
+  EXPECT_TRUE(mock_timer_->IsRunning());
+
+  scheduler_.OnClientDeleted(kBackgroundChildId, kBackgroundRouteId);
+  EXPECT_TRUE(mock_timer_->IsRunning());
+
+  scheduler_.OnClientDeleted(kBackgroundChildId2, kBackgroundRouteId2);
+  EXPECT_FALSE(mock_timer_->IsRunning());
+
+  // To avoid errors on test tear down.
+  scheduler_.OnClientCreated(kBackgroundChildId, kBackgroundRouteId);
+}
+
+TEST_F(ResourceSchedulerTest, LastCoalescedClientStartsLoadingStopsTimer) {
+  scheduler_.SetThrottleOptionsForTesting(true /* should_throttle */,
+                                          true /* should_coalesce */);
+  scheduler_.OnClientCreated(kBackgroundChildId2, kBackgroundRouteId2);
+  EXPECT_FALSE(mock_timer_->IsRunning());
+  scheduler_.OnLoadingStateChanged(kChildId, kRouteId, true);
+  EXPECT_FALSE(mock_timer_->IsRunning());
+  scheduler_.OnLoadingStateChanged(
+      kBackgroundChildId, kBackgroundRouteId, true);
+  EXPECT_EQ(ResourceScheduler::COALESCED,
+            scheduler_.GetClientStateForTesting(kBackgroundChildId,
+                                                kBackgroundRouteId));
+  scheduler_.OnLoadingStateChanged(
+      kBackgroundChildId2, kBackgroundRouteId2, true);
+  EXPECT_EQ(ResourceScheduler::COALESCED,
+            scheduler_.GetClientStateForTesting(kBackgroundChildId2,
+                                                kBackgroundRouteId2));
+  EXPECT_TRUE(mock_timer_->IsRunning());
+
+  scheduler_.OnLoadingStateChanged(
+      kBackgroundChildId, kBackgroundRouteId, false);
+  EXPECT_TRUE(mock_timer_->IsRunning());
+
+  scheduler_.OnLoadingStateChanged(
+      kBackgroundChildId2, kBackgroundRouteId2, false);
+  EXPECT_FALSE(mock_timer_->IsRunning());
+
+  // To avoid errors on test tear down.

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

nit:  Style guide encourages complete sentences.  Just add a "Needed" or "This
is needed" in front is fine.

+  scheduler_.OnClientDeleted(kBackgroundChildId2, kBackgroundRouteId2);
+}
+
+TEST_F(ResourceSchedulerTest, LastCoalescedClientBecomesVisibleStopsTimer) {
+  scheduler_.SetThrottleOptionsForTesting(true /* should_throttle */,
+                                          true /* should_coalesce */);
+  scheduler_.OnClientCreated(kBackgroundChildId2, kBackgroundRouteId2);
+  EXPECT_FALSE(mock_timer_->IsRunning());
+  scheduler_.OnLoadingStateChanged(kChildId, kRouteId, true);
+  EXPECT_FALSE(mock_timer_->IsRunning());
+  scheduler_.OnLoadingStateChanged(
+      kBackgroundChildId, kBackgroundRouteId, true);
+  EXPECT_EQ(ResourceScheduler::COALESCED,
+            scheduler_.GetClientStateForTesting(kBackgroundChildId,
+                                                kBackgroundRouteId));
+  scheduler_.OnLoadingStateChanged(
+      kBackgroundChildId2, kBackgroundRouteId2, true);
+  EXPECT_EQ(ResourceScheduler::COALESCED,
+            scheduler_.GetClientStateForTesting(kBackgroundChildId2,
+                                                kBackgroundRouteId2));
+  EXPECT_TRUE(mock_timer_->IsRunning());
+
+  scheduler_.OnVisibilityChanged(kBackgroundChildId, kBackgroundRouteId, true);
+  EXPECT_TRUE(mock_timer_->IsRunning());
+
+  scheduler_.OnVisibilityChanged(
+      kBackgroundChildId2, kBackgroundRouteId2, true);
+  EXPECT_FALSE(mock_timer_->IsRunning());
+
+  // To avoid errors on test tear down.
+  scheduler_.OnClientDeleted(kBackgroundChildId2, kBackgroundRouteId2);
+}
+
+TEST_F(ResourceSchedulerTest, CoalescedRequestsIssueOnTimer) {
+  scheduler_.SetThrottleOptionsForTesting(true /* should_throttle */,
+                                          true /* should_coalesce */);
+  scheduler_.OnLoadingStateChanged(kChildId, kRouteId, true);
+  scheduler_.OnLoadingStateChanged(
+      kBackgroundChildId, kBackgroundRouteId, true);
+  EXPECT_EQ(ResourceScheduler::COALESCED,
+            scheduler_.GetClientStateForTesting(kBackgroundChildId,
+                                                kBackgroundRouteId));
+  EXPECT_TRUE(scheduler_.active_clients_loaded());
+
+  scoped_ptr<TestRequest> high(
+      NewBackgroundRequest("http://host/high", net::HIGHEST));
+  scoped_ptr<TestRequest> low(
+      NewBackgroundRequest("http://host/low", net::LOWEST));
+  EXPECT_FALSE(high->started());
+  EXPECT_FALSE(low->started());
+
+  FireCoalescingTimer();
+
+  EXPECT_TRUE(high->started());
+  EXPECT_TRUE(low->started());
+}
+
+TEST_F(ResourceSchedulerTest, CoalescedRequestsUnthrottleCorrectlyOnTimer) {
+  scheduler_.SetThrottleOptionsForTesting(true /* should_throttle */,
+                                          true /* should_coalesce */);
+  scheduler_.OnLoadingStateChanged(kChildId, kRouteId, true);
+  scheduler_.OnLoadingStateChanged(
+      kBackgroundChildId, kBackgroundRouteId, true);
+  EXPECT_EQ(ResourceScheduler::COALESCED,
+            scheduler_.GetClientStateForTesting(kBackgroundChildId,
+                                                kBackgroundRouteId));
+  EXPECT_TRUE(scheduler_.active_clients_loaded());
+
+  scoped_ptr<TestRequest> high(
+      NewBackgroundRequest("http://host/high", net::HIGHEST));
+  scoped_ptr<TestRequest> high2(
+      NewBackgroundRequest("http://host/high", net::HIGHEST));
+  scoped_ptr<TestRequest> high3(
+      NewBackgroundRequest("http://host/high", net::HIGHEST));
+  scoped_ptr<TestRequest> high4(
+      NewBackgroundRequest("http://host/high", net::HIGHEST));
+  scoped_ptr<TestRequest> low(
+      NewBackgroundRequest("http://host/low", net::LOWEST));
+  scoped_ptr<TestRequest> low2(
+      NewBackgroundRequest("http://host/low", net::LOWEST));
+  scoped_ptr<TestRequest> low3(
+      NewBackgroundRequest("http://host/low", net::LOWEST));
+  scoped_ptr<TestRequest> low4(
+      NewBackgroundRequest("http://host/low", net::LOWEST));
+
+  http_server_properties_.SetSupportsSpdy(net::HostPortPair("spdyhost", 443),
+                                          true);
+  scoped_ptr<TestRequest> low_spdy(
+      NewBackgroundRequest("https://spdyhost/low", net::LOW));
+  scoped_ptr<TestRequest> sync_request(
+      NewBackgroundSyncRequest("http://host/req", net::LOW));
+  scoped_ptr<TestRequest> non_http_request(
+      NewBackgroundRequest("chrome-extension://req", net::LOW));
+
+  // Sync requests should issue immediately.
+  EXPECT_TRUE(sync_request->started());
+  // Non-http(s) requests should issue immediately.
+  EXPECT_TRUE(non_http_request->started());
+  // Nothing else should issue without a timer fire.
+  EXPECT_FALSE(high->started());
+  EXPECT_FALSE(high2->started());
+  EXPECT_FALSE(high3->started());
+  EXPECT_FALSE(high4->started());
+  EXPECT_FALSE(low->started());
+  EXPECT_FALSE(low2->started());
+  EXPECT_FALSE(low3->started());
+  EXPECT_FALSE(low4->started());
+  EXPECT_FALSE(low_spdy->started());
+
+  FireCoalescingTimer();
+
+  // All high priority requests should issue.
+  EXPECT_TRUE(high->started());
+  EXPECT_TRUE(high2->started());
+  EXPECT_TRUE(high3->started());
+  EXPECT_TRUE(high4->started());
+  // There should only be one net::LOWEST priority request issued with
+  // non-delayable requests in flight.
+  EXPECT_TRUE(low->started());
+  EXPECT_FALSE(low2->started());
+  EXPECT_FALSE(low3->started());
+  EXPECT_FALSE(low4->started());
+  // Spdy-Enable requests should issue regardless of priority.
+  EXPECT_TRUE(low_spdy->started());
+}
+
+TEST_F(ResourceSchedulerTest, CoalescedRequestsWaitForNextTimer) {
+  scheduler_.SetThrottleOptionsForTesting(true /* should_throttle */,
+                                          true /* should_coalesce */);
+  scheduler_.OnLoadingStateChanged(kChildId, kRouteId, true);
+  scheduler_.OnLoadingStateChanged(
+      kBackgroundChildId, kBackgroundRouteId, true);
+
+  EXPECT_EQ(ResourceScheduler::COALESCED,
+            scheduler_.GetClientStateForTesting(kBackgroundChildId,
+                                                kBackgroundRouteId));
+  EXPECT_TRUE(scheduler_.active_clients_loaded());
+
+  scoped_ptr<TestRequest> high(
+      NewBackgroundRequest("http://host/high", net::HIGHEST));
+  EXPECT_FALSE(high->started());
+
+  FireCoalescingTimer();
+
+  scoped_ptr<TestRequest> high2(
+      NewBackgroundRequest("http://host/high2", net::HIGHEST));
+  scoped_ptr<TestRequest> low(
+      NewBackgroundRequest("http://host/low", net::LOWEST));
+
+  EXPECT_TRUE(high->started());
+  EXPECT_FALSE(high2->started());
+  EXPECT_FALSE(low->started());
+
+  FireCoalescingTimer();
+
+  EXPECT_TRUE(high->started());
+  EXPECT_TRUE(high2->started());
+  EXPECT_TRUE(low->started());
+}

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

Good job on the tests, but two cases we don't seem to have:

On client goes from COALESCED -> ACTIVE_AND_LOADING.  Need to make sure that
both the decrementing number of coalesced clients takes place (Starting the
timer), and ACTIVE_AND_LOADING updates that counter, making unthrottling loading
background clients.

And the opposite as well.  I think we handle these double updates correctly, but
seems like a potential point of regression.

+
 }  // unnamed namespace
 
 }  // namespace content