Add a ResourceScheduler to ResourceDispatcherHost.

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
new file mode 100644
index 0000000000000000000000000000000000000000..70529af390446a5ddb16db126f7a955cc39f2fc6
--- /dev/null
+++ b/content/browser/loader/resource_scheduler.cc
@@ -0,0 +1,209 @@
+// 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/stl_util.h"
+#include "content/public/browser/resource_controller.h"
+#include "content/public/browser/resource_throttle.h"
+#include "net/base/load_flags.h"
+#include "net/base/request_priority.h"
+#include "net/url_request/url_request.h"
+
+// TODO(simonjam): This is arbitrary. Experiment.
+static const int MAX_NUM_NAVIGATIONS_TO_TRACK = 5;

[darin (slow to review), 2013/01/14 22:03:30]

nit: constants use kConstantStyle.  not required for file scoped things, but i
usually just put everything in the content namespace unless there is a reason to
do so.

[James Simonsen, 2013/01/15 01:10:06]

Done.

+
+namespace content {
+
+class ResourceScheduler::ScheduledResourceRequest : public ResourceThrottle {
+ public:
+  ScheduledResourceRequest(const ClientId& client_id,
+                           const net::URLRequest& request,
+                           ResourceScheduler* scheduler)
+      : client_id_(client_id),
+        request_(request),
+        ready_(false),
+        deferred_(false),
+        scheduler_(scheduler) {
+  }
+
+  virtual ~ScheduledResourceRequest() {
+    scheduler_->RemoveRequest(this);

[darin (slow to review), 2013/01/14 22:56:22]

how do you know that the scheduler_ is still a valid pointer?

[James Simonsen, 2013/01/15 01:10:06]

|this| is owned by the same ResourceDispatcher that owns |scheduler_|.

+  }
+
+  void Start() {
+    ready_ = true;
+    if (deferred_ &&
+        request_.status().status() != net::URLRequestStatus::CANCELED) {
+      controller()->Resume();
+    }
+  }
+
+  const ClientId& client_id() const { return client_id_; }
+  const net::URLRequest& url_request() const { return request_; }
+
+ private:
+  virtual void WillStartRequest(bool* defer) OVERRIDE {
+    deferred_ = *defer = !ready_;
+  }
+
+  ClientId client_id_;
+  const net::URLRequest& request_;
+  bool ready_;
+  bool deferred_;
+  ResourceScheduler* scheduler_;
+
+  DISALLOW_COPY_AND_ASSIGN(ScheduledResourceRequest);
+};
+
+ResourceScheduler::ResourceScheduler()
+    : client_map_(MAX_NUM_NAVIGATIONS_TO_TRACK) {
+  DetachFromThread();  // Construction happens on the main thread.
+}
+
+ResourceScheduler::~ResourceScheduler() {
+  DetachFromThread();  // Destruction happens on the main thread.
+}

[darin (slow to review), 2013/01/14 22:56:22]

Hmm, should you assert that the pending_requests lists of all clients are empty?
 Or, maybe that is not worth worrying about since this class is only destroyed
when the IO thread is going away.  I was just wondering about the possibility of
leaks.

[James Simonsen, 2013/01/15 01:10:06]

Yeah, sounds good. Done.

+
+scoped_ptr<ResourceThrottle> ResourceScheduler::ScheduleRequest(
+    int child_id,
+    int route_id,
+    const net::URLRequest& url_request) {
+  DCHECK(CalledOnValidThread());
+  ClientId client_id = MakeClientId(child_id, route_id);
+  scoped_ptr<ScheduledResourceRequest> request(
+      new ScheduledResourceRequest(client_id, url_request, this));
+
+  ClientMap::iterator it = client_map_.Get(client_id);
+  if (it == client_map_.end()) {
+    // There are several ways this could happen:
+    // 1. <a ping> requests don't have a route_id.

[darin (slow to review), 2013/01/14 22:56:22]

It seems like a bug that <a ping> requests don't have a route id !!

[James Simonsen, 2013/01/15 01:10:06]

That wouldn't be the only bug with <a ping>. Will pointed out earlier that if we
destroy a renderer process, all of its requests are canceled, including the
pings. I assume those are supposed to finish. Seems like a common case with
cross-renderer navigation.

+    // 2. Most unittests don't send the IPCs needed to register Clients.
+    // 3. The tab is closed while a RequestResource IPC is in flight.
+    // 4. The tab hasn't navigated recently.
+    unowned_requests_.insert(request.get());
+    request->Start();
+    return request.PassAs<ResourceThrottle>();
+  }
+
+  Client* client = it->second;
+
+  bool is_synchronous = (url_request.load_flags() & net::LOAD_IGNORE_LIMITS) ==
+      net::LOAD_IGNORE_LIMITS;
+  bool is_low_priority =
+      url_request.priority() < net::MEDIUM && !is_synchronous;
+
+  if (is_low_priority && !client->in_flight_requests.empty() &&
+      !client->has_painted) {
+    client->pending_requests.push_back(request.get());
+  } else {
+    StartRequest(request.get(), client);
+  }
+  return request.PassAs<ResourceThrottle>();
+}
+
+void ResourceScheduler::RemoveRequest(ScheduledResourceRequest* request) {
+  DCHECK(CalledOnValidThread());
+  if (ContainsKey(unowned_requests_, request)) {
+    unowned_requests_.erase(request);
+    return;
+  }
+
+  ClientMap::iterator client_it = client_map_.Get(request->client_id());
+  if (client_it == client_map_.end()) {
+    return;
+  }
+
+  Client* client = client_it->second;
+  RequestSet::iterator request_it = client->in_flight_requests.find(request);
+  if (request_it == client->in_flight_requests.end()) {
+    bool removed = false;
+    RequestQueue::iterator queue_it;
+    for (queue_it = client->pending_requests.begin();
+         queue_it != client->pending_requests.end(); ++queue_it) {
+      if (*queue_it == request) {
+        client->pending_requests.erase(queue_it);
+        removed = true;
+        break;
+      }
+    }
+    DCHECK(removed);
+    DCHECK(!ContainsKey(client->in_flight_requests, request));
+  } else {
+    size_t erased = client->in_flight_requests.erase(request);
+    DCHECK(erased);
+  }
+
+  LoadPendingRequests(client);

[darin (slow to review), 2013/01/14 22:56:22]

I'm not sure I understand why you are calling LoadPendingRequests here. 
RemoveRequest could be called because a resource request was simply cancelled.

[James Simonsen, 2013/01/15 01:10:06]

Good catch! It's only supposed to do that when that client is idle. Somehow that
got lost in one of the iterations on this CL. Added a test case too.

+}
+
+void ResourceScheduler::OnNavigate(int child_id, int route_id) {
+  DCHECK(CalledOnValidThread());
+  ClientId client_id = MakeClientId(child_id, route_id);
+
+  ClientMap::iterator it = client_map_.Get(client_id);
+  if (it == client_map_.end()) {
+    it = client_map_.Put(client_id, new Client(this));
+  }
+
+  Client* client = it->second;
+  client->has_painted = false;
+}
+
+void ResourceScheduler::OnFirstPaint(int child_id, int route_id) {
+  DCHECK(CalledOnValidThread());
+  ClientId client_id = MakeClientId(child_id, route_id);
+  ClientMap::iterator it = client_map_.Get(client_id);
+  if (it == client_map_.end()) {
+    return;
+  }
+
+  Client* client = it->second;
+  if (!client->has_painted) {
+    client->has_painted = true;
+    LoadPendingRequests(client);
+  }
+}
+
+void ResourceScheduler::StartRequest(ScheduledResourceRequest* request,
+                                     Client* client) {
+  client->in_flight_requests.insert(request);
+  request->Start();
+}
+
+void ResourceScheduler::LoadPendingRequests(Client* client) {
+  RequestQueue::iterator it;
+  for (it = client->pending_requests.begin();

[darin (slow to review), 2013/01/14 22:56:22]

it seems like you are asserting that client->has_painted is true here.  maybe
you should write a DCHECK to indicate that.

that said, it seems like we could reach here from RemoveRequest, which could be
called before client->has_painted becomes true??

NOTE:  Since StartRequest calls ResourceController::Resume(), I worry a bit
about the potential for re-entrancy.  It would be bad if somehow during
StartRequest another call was made to the ResourceScheduler that resulted in
mutation to the pending_requests list.  Hmm...

[James Simonsen, 2013/01/15 01:10:06]

Yeah, there are a couple of ways we might decide to start a request:

1. It's high priority.
2. We've already painted.
3. We don't have any other requests in flight.
4. We've blown the MRU cache and can't track this client any more.
Good point. I made the code more robust and wrote a test case to exercise it.

+       it != client->pending_requests.end(); ++it) {
+    StartRequest(*it, client);
+  }
+  client->pending_requests.clear();
+}
+
+void ResourceScheduler::RemoveClient(Client* client) {
+  LoadPendingRequests(client);
+  for (RequestSet::iterator it = client->in_flight_requests.begin();
+       it != client->in_flight_requests.end(); ++it) {
+    unowned_requests_.insert(*it);
+  }
+  client->in_flight_requests.clear();
+}
+
+ResourceScheduler::ClientId ResourceScheduler::MakeClientId(
+    int child_id, int route_id) {
+  return (static_cast<ResourceScheduler::ClientId>(child_id) << 32) | route_id;
+}
+
+ResourceScheduler::Client::Client(ResourceScheduler* scheduler)
+    : has_painted(false),
+      scheduler_(scheduler) {
+}
+
+ResourceScheduler::Client::~Client() {
+  scheduler_->RemoveClient(this);
+  DCHECK(in_flight_requests.empty());
+  DCHECK(pending_requests.empty());
+}
+
+}  // namespace content