Headless: make URLRequestDispatcher aware of navigations

headless/public/util/deterministic_dispatcher.cc

Index: headless/public/util/deterministic_dispatcher.cc
diff --git a/headless/public/util/deterministic_dispatcher.cc b/headless/public/util/deterministic_dispatcher.cc
index 20982db898c8088440bc98921fdcbbb2755dab79..945c577fe1cf87d8f8f2029d689d38cfe06ca12f 100644
--- a/headless/public/util/deterministic_dispatcher.cc
+++ b/headless/public/util/deterministic_dispatcher.cc
@@ -9,6 +9,7 @@
 #include "base/bind.h"
 #include "base/logging.h"
 #include "headless/public/util/managed_dispatch_url_request_job.h"
+#include "headless/public/util/navigation_request.h"
 
 namespace headless {
 
@@ -16,20 +17,21 @@ DeterministicDispatcher::DeterministicDispatcher(
     scoped_refptr<base::SingleThreadTaskRunner> io_thread_task_runner)
     : io_thread_task_runner_(std::move(io_thread_task_runner)),
       dispatch_pending_(false),
+      navigation_in_progress_(false),
       weak_ptr_factory_(this) {}
 
 DeterministicDispatcher::~DeterministicDispatcher() {}
 
 void DeterministicDispatcher::JobCreated(ManagedDispatchURLRequestJob* job) {
   base::AutoLock lock(lock_);
-  pending_requests_.push_back(job);
+  pending_requests_.emplace_back(job);
 }
 
 void DeterministicDispatcher::JobKilled(ManagedDispatchURLRequestJob* job) {
   base::AutoLock lock(lock_);
   for (auto it = pending_requests_.begin(); it != pending_requests_.end();
        it++) {
-    if (*it == job) {
+    if (it->url_request == job) {
       pending_requests_.erase(it);
       break;
     }
@@ -56,8 +58,27 @@ void DeterministicDispatcher::JobDeleted(ManagedDispatchURLRequestJob* job) {
   MaybeDispatchJobLocked();
 }
 
+void DeterministicDispatcher::NavigationRequested(
+    std::unique_ptr<NavigationRequest> navigation) {
+  base::AutoLock lock(lock_);
+  pending_requests_.emplace_back(std::move(navigation));
+
+  MaybeDispatchNavigationJobLocked();
+}
+
+void DeterministicDispatcher::MaybeDispatchNavigationJobLocked() {

[altimin, 2017/02/09 12:01:52]

I don't like two similar methods with subtle difference. Should we refactor it
into one method, which checks first Request in the queue and add comment?

Like:

if (dispatch_pending_ || navigation_in_progress_)
  return;

if (pending_requests_.empty())
  return;

// If first url request is not ready, bail out.
if (ready_status_map_.find(pending_requests_.front().url_request &&
ready_status_map_.find(pending_requests_.front().url_request) ==
ready_status_map_.end())
  return;

[alex clarke (OOO till 29th), 2017/02/09 12:44:43]

OK I found a way of merging them.

+  if (dispatch_pending_ || navigation_in_progress_)
+    return;
+
+  dispatch_pending_ = true;
+  io_thread_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&DeterministicDispatcher::MaybeDispatchJobOnIOThreadTask,
+                 weak_ptr_factory_.GetWeakPtr()));
+}
+
 void DeterministicDispatcher::MaybeDispatchJobLocked() {
-  if (dispatch_pending_ || ready_status_map_.empty())
+  if (dispatch_pending_ || navigation_in_progress_ || ready_status_map_.empty())
     return;
 
   dispatch_pending_ = true;
@@ -68,7 +89,7 @@ void DeterministicDispatcher::MaybeDispatchJobLocked() {
 }
 
 void DeterministicDispatcher::MaybeDispatchJobOnIOThreadTask() {
-  ManagedDispatchURLRequestJob* job;
+  Request request;
   net::Error job_status;
 
   {
@@ -77,22 +98,65 @@ void DeterministicDispatcher::MaybeDispatchJobOnIOThreadTask() {
     // If the job got deleted, |pending_requests_| may be empty.
     if (pending_requests_.empty())
       return;
-    job = pending_requests_.front();
-    StatusMap::const_iterator it = ready_status_map_.find(job);
-    // Bail out if the oldest job is not be ready for dispatch yet.
-    if (it == ready_status_map_.end())
+
+    // Bail out if we're waiting for a navigation to complete.
+    if (navigation_in_progress_)
       return;
 
-    job_status = it->second;
-    ready_status_map_.erase(it);
+    request = std::move(pending_requests_.front());
+    if (request.url_request) {
+      StatusMap::const_iterator it =
+          ready_status_map_.find(request.url_request);
+      // Bail out if the oldest job is not be ready for dispatch yet.
+      if (it == ready_status_map_.end())
+        return;
+
+      job_status = it->second;
+      ready_status_map_.erase(it);
+    } else {
+      navigation_in_progress_ = true;

[Sami, 2017/02/09 12:03:26]

DCHECK(!navigation_in_progress_)?

[alex clarke (OOO till 29th), 2017/02/09 12:44:43]

In theory this is guaranteed by line 103, but I suppose it makes the intent
clearer.

+    }
     pending_requests_.pop_front();
   }
 
-  if (job_status == net::OK) {
-    job->OnHeadersComplete();
+  if (request.url_request) {
+    if (job_status == net::OK) {
+      request.url_request->OnHeadersComplete();
+    } else {
+      request.url_request->OnStartError(job_status);
+    }
   } else {
-    job->OnStartError(job_status);
+    request.navigation_request->StartProcessing(

[altimin, 2017/02/09 12:01:52]

If we bailed in navigation_in_progress_, request.navigation_request could be
empty. (also: add test for this case?)

[alex clarke (OOO till 29th), 2017/02/09 12:44:43]

Not sure I follow.  If we bail out, we exit the function (without poping
anything from |pending_requests|.

[altimin, 2017/02/09 12:51:58]

Sorry, got confused.

+        base::Bind(&DeterministicDispatcher::NavigationDoneTask,
+                   weak_ptr_factory_.GetWeakPtr()));
+  }
+}
+
+void DeterministicDispatcher::NavigationDoneTask() {
+  {
+    base::AutoLock lock(lock_);
+    navigation_in_progress_ = false;

[Sami, 2017/02/09 12:03:26]

DCHECK(navigation_in_progress_)?

[alex clarke (OOO till 29th), 2017/02/09 12:44:43]

Done.

   }
+
+  MaybeDispatchNavigationJobLocked();
+}
+
+DeterministicDispatcher::Request::Request() : url_request(nullptr) {}
+DeterministicDispatcher::Request::~Request() {}
+
+DeterministicDispatcher::Request::Request(
+    ManagedDispatchURLRequestJob* url_request)
+    : url_request(url_request) {}
+
+DeterministicDispatcher::Request::Request(
+    std::unique_ptr<NavigationRequest> navigation_request)
+    : url_request(nullptr), navigation_request(std::move(navigation_request)) {}
+
+DeterministicDispatcher::Request& DeterministicDispatcher::Request::operator=(
+    DeterministicDispatcher::Request&& other) {
+  url_request = other.url_request;
+  navigation_request = std::move(other.navigation_request);
+  return *this;
 }
 
 }  // namespace headless