ResourceScheduler: remove dependency on ResourceRequestInfo and request_id

content/browser/loader/resource_dispatcher_host_unittest.cc

Index: content/browser/loader/resource_dispatcher_host_unittest.cc
diff --git a/content/browser/loader/resource_dispatcher_host_unittest.cc b/content/browser/loader/resource_dispatcher_host_unittest.cc
index ef90e30b286979409944d0724153bf784152fdfc..0fa54577d1614af9542e83ad67aa60ea7654bf3a 100644
--- a/content/browser/loader/resource_dispatcher_host_unittest.cc
+++ b/content/browser/loader/resource_dispatcher_host_unittest.cc
@@ -946,6 +946,11 @@ class ResourceDispatcherHostTest : public testing::Test,
 
   void MakeWebContentsAssociatedTestRequest(int request_id, const GURL& url);
 
+  // Generates a request with the given priority.
+  void MakeTestRequestWithPriority(int render_view_id,
+                                   int request_id,
+                                   net::RequestPriority priority);
+
   void CancelRequest(int request_id);
   void RendererCancelRequest(int request_id) {
     ResourceMessageFilter* old_filter = SetFilter(filter_.get());
@@ -1072,6 +1077,17 @@ void ResourceDispatcherHostTest::MakeWebContentsAssociatedTestRequest(
   KickOffRequest();
 }
 
+void ResourceDispatcherHostTest::MakeTestRequestWithPriority(
+    int render_view_id,
+    int request_id,
+    net::RequestPriority priority) {
+  ResourceHostMsg_Request request = CreateResourceRequest(
+      "GET", RESOURCE_TYPE_SUB_RESOURCE, GURL("http://example.com/priority"));
+  request.priority = priority;
+  ResourceHostMsg_RequestResource msg(render_view_id, request_id, request);
+  host_.OnMessageReceived(msg, filter_.get());
+}
+
 void ResourceDispatcherHostTest::CancelRequest(int request_id) {
   host_.CancelRequest(filter_->child_id(), request_id);
 }
@@ -3329,6 +3345,42 @@ TEST_F(ResourceDispatcherHostTest, TransferRequestRedirected) {
             web_contents_observer_->resource_request_redirect_count());
 }
 
+// Confirm that DidChangePriority messages are respected.
+TEST_F(ResourceDispatcherHostTest, DidChangePriority) {
+  // ResourceScheduler only throttles http and https requests.
+  HandleScheme("http");
+
+  // Needed to enable scheduling for this child.
+  host_.OnRenderViewHostCreated(filter_->child_id(),  // child_id
+                                0,                    // route_id
+                                true,                 // is_visible
+                                false);               // is_audible
+
+  // Prevent any of these requests from completing.
+  job_factory_->SetDelayedCompleteJobGeneration(true);
+  SetResponse("HTTP/1.1 200 OK\n\n", "<title>Dummy body</title>");
+
+  // Only one idle priority request will run while a high-priority request
+  // exists.
+  MakeTestRequestWithPriority(0, 1, net::HIGHEST);
+  MakeTestRequestWithPriority(0, 2, net::IDLE);
+  MakeTestRequestWithPriority(0, 3, net::IDLE);
+
+  KickOffRequest();
+
+  EXPECT_EQ(2, job_factory_->url_request_jobs_created_count());
+

[davidben, 2015/08/13 23:08:52]

This probably deserves a comment:

// Increase the priority of the second idle priority request. It was
// scheduled later, so it is not currently running.

(So it's clear you're assuming the ResourceScheduler does them in order.)

[Adam Rice, 2015/08/13 23:42:28]

Done.

It's unfortunate that this test depends on the exact behaviour of the
ResourceScheduler, but I don't know how to make it more general without adding a
lot more complexity.

At some point it's cheaper to have a simple test that someone can fix if the
behaviour changes than a complex test that will be very hard to debug if it
breaks.

[davidben, 2015/08/14 02:14:28]

Agreed. I think a comment's worthwhile so that, if it breaks, it'll be clear
what the hidden assumption was, but I don't think it's worth sweating much about
the assumption itself.

+  ResourceHostMsg_DidChangePriority priority_msg(3, net::MAXIMUM_PRIORITY, 0);
+  host_.OnMessageReceived(priority_msg, filter_.get());
+  base::MessageLoop::current()->RunUntilIdle();
+
+  EXPECT_EQ(3, job_factory_->url_request_jobs_created_count());
+
+  // Cleanup.
+  host_.OnRenderViewHostDeleted(filter_->child_id(),  // child_id
+                                0);                   // route_id
+}
+
 net::URLRequestJob* TestURLRequestJobFactory::MaybeCreateJobWithProtocolHandler(
       const std::string& scheme,
       net::URLRequest* request,