Implement registration job ordering

content/browser/service_worker/service_worker_job_coordinator.cc

Index: content/browser/service_worker/service_worker_job_coordinator.cc
diff --git a/content/browser/service_worker/service_worker_job_coordinator.cc b/content/browser/service_worker/service_worker_job_coordinator.cc
index 16f92adf8dff82451afd67fb90218d92f22e2cc0..3495ba311118c5a31a496ac164990af1c81320f3 100644
--- a/content/browser/service_worker/service_worker_job_coordinator.cc
+++ b/content/browser/service_worker/service_worker_job_coordinator.cc
@@ -4,6 +4,7 @@
 
 #include "content/browser/service_worker/service_worker_job_coordinator.h"
 
+#include "base/stl_util.h"
 #include "content/browser/service_worker/service_worker_registration.h"
 
 namespace content {
@@ -12,43 +13,77 @@ ServiceWorkerJobCoordinator::ServiceWorkerJobCoordinator(
     const base::WeakPtr<ServiceWorkerStorage>& storage)
     : storage_(storage), weak_factory_(this) {}
 
-ServiceWorkerJobCoordinator::~ServiceWorkerJobCoordinator() {}
+ServiceWorkerJobCoordinator::~ServiceWorkerJobCoordinator() {
+  for (RegistrationJobMap::iterator it = jobs_.begin(); it != jobs_.end();
+       ++it) {
+    STLDeleteElements(&it->second);
+  }
+}
 
 void ServiceWorkerJobCoordinator::Register(
     const GURL& pattern,
     const GURL& script_url,
     const ServiceWorkerRegisterJob::RegistrationCallback& callback) {
-  scoped_ptr<ServiceWorkerRegisterJob> job(new ServiceWorkerRegisterJob(
-      storage_,
-      base::Bind(&ServiceWorkerJobCoordinator::RegisterComplete,
-                 weak_factory_.GetWeakPtr(),
-                 callback)));
-  job->StartRegister(pattern, script_url);
-  registration_jobs_.push_back(job.release());
+  ServiceWorkerRegisterJob* job;

[kinuko, 2014/01/08 08:38:36]

Please initialize with NULL etc

[alecflett, 2014/01/08 23:59:16]

Done.

+
+  if (MostRecentJobEquals(
+          pattern, script_url, ServiceWorkerRegisterJob::REGISTER))
+    job = *jobs_.find(pattern)->second.rbegin();
+  else {
+    job = new ServiceWorkerRegisterJob(storage_,
+                                       this,
+                                       pattern,
+                                       script_url,
+                                       ServiceWorkerRegisterJob::REGISTER);
+    jobs_[pattern].push_back(job);
+
+    // If this is the first job for this pattern, start it. Otherwise,
+    // starting it will be kicked off by the completion of the
+    // previous job.
+    if (jobs_[pattern].size() == 1)
+      job->Start();
+  }
+
+  job->AddCallback(base::Bind(&ServiceWorkerJobCoordinator::RegisterComplete,
+                              weak_factory_.GetWeakPtr(),
+                              callback));

[kinuko, 2014/01/08 08:38:36]

This code path is mostly same as that of Unregister, and handling jobs_[pattern]
map and pending jobs queue together is making the code a bit messty. Could we
introduce a smaller inner class for each queue and separate the code out from
jobs_[pattern] map part?  Also I think it's ok to always create RegisterJob
first and delete it if it's coalesced into another one for simpler code. I'd
probably write this like:

class JobQueue {
 public:
  ~JobQueue() {
    STLDeleteElements(&jobs_);
  }

  void Push(scoped_ptr<Job> job, callback) {
    if (jobs_.empty()) { 
      jobs_.push_back(job);
      job->Start();
    } else if (!job->Equals(jobs_.back())) {
      jobs_.push_back(job);
    }
    DCHECK(!jobs_.empty());
    jobs_.back()->AddCallback(callback);
  }

  void Pop(Job* job) {
    DCHECK(job == jobs_.front());
    jobs_.pop_front();
    if (!jobs_.empty())
      jobs_.front()->Start();
  }

 private:
  // The last one must be always active / running.
  std::deque<Job*> jobs_;
};

Then the code for Register(), Unregister() and FinishJob() would become much
smaller.  Wdyt?

[alecflett, 2014/01/08 23:59:16]

I like it. I'll give it a try.

 }
 
 void ServiceWorkerJobCoordinator::Unregister(
     const GURL& pattern,
     const ServiceWorkerRegisterJob::UnregistrationCallback& callback) {
-  scoped_ptr<ServiceWorkerRegisterJob> job(new ServiceWorkerRegisterJob(
-      storage_,
-      base::Bind(&ServiceWorkerJobCoordinator::UnregisterComplete,
-                 weak_factory_.GetWeakPtr(),
-                 callback)));
-  job->StartUnregister(pattern);
-  registration_jobs_.push_back(job.release());
-}
+  ServiceWorkerRegisterJob* job;
 
-void ServiceWorkerJobCoordinator::EraseJob(ServiceWorkerRegisterJob* job) {
-  ScopedVector<ServiceWorkerRegisterJob>::iterator job_position =
-      registration_jobs_.begin();
-  for (; job_position != registration_jobs_.end(); ++job_position) {
-    if (*job_position == job) {
-      registration_jobs_.erase(job_position);
-      return;
-    }
+  if (MostRecentJobEquals(
+          pattern, GURL(), ServiceWorkerRegisterJob::UNREGISTER))
+    job = *jobs_.find(pattern)->second.rbegin();
+  else {
+    job = new ServiceWorkerRegisterJob(
+        storage_, this, pattern, GURL(), ServiceWorkerRegisterJob::UNREGISTER);
+    jobs_[pattern].push_back(job);
+
+    // If this is the first job for this pattern, start it. Otherwise,
+    // starting it will be kicked off by the completion of the
+    // previous job.
+    if (jobs_[pattern].size() == 1)
+      job->Start();
   }
-  NOTREACHED() << "Deleting non-existent job. ";
+
+  job->AddCallback(base::Bind(&ServiceWorkerJobCoordinator::UnregisterComplete,
+                              weak_factory_.GetWeakPtr(),
+                              callback));
+}
+
+void ServiceWorkerJobCoordinator::FinishJob(ServiceWorkerRegisterJob* job) {
+  RegistrationJobMap::iterator pending_jobs = jobs_.find(job->pattern());
+  DCHECK(pending_jobs != jobs_.end()) << "Deleting non-existent job.";
+  DCHECK(job == *pending_jobs->second.begin());
+  pending_jobs->second.erase(pending_jobs->second.begin());
+  if (pending_jobs->second.size() == 0)
+    jobs_.erase(pending_jobs);
+  else
+    // queue up the next job here?
+    (*pending_jobs->second.begin())->Start();
 }
 
 void ServiceWorkerJobCoordinator::RegisterComplete(
@@ -57,7 +92,6 @@ void ServiceWorkerJobCoordinator::RegisterComplete(
     ServiceWorkerRegistrationStatus status,
     ServiceWorkerRegistration* registration) {
   callback.Run(status, registration);
-  EraseJob(job);
 }
 
 void ServiceWorkerJobCoordinator::UnregisterComplete(
@@ -66,7 +100,21 @@ void ServiceWorkerJobCoordinator::UnregisterComplete(
     ServiceWorkerRegistrationStatus status,
     ServiceWorkerRegistration* previous_registration) {
   callback.Run(status);
-  EraseJob(job);
+}
+
+bool ServiceWorkerJobCoordinator::MostRecentJobEquals(
+    const GURL& pattern,
+    const GURL& script_url,
+    ServiceWorkerRegisterJob::RegistrationType type) {
+  RegistrationJobMap::const_iterator it = jobs_.find(pattern);
+  if (it == jobs_.end())
+    return false;
+
+  if (it->second.size() == 0)
+    return false;
+
+  return ((*it->second.rbegin())->type() == type &&
+          (*it->second.rbegin())->script_url() == script_url);

[kinuko, 2014/01/08 08:38:36]

This line 116-117 could be probably separated out as a small generic routine
(like Equals) of ServiceWorkerRegistrationJob

[alecflett, 2014/01/08 23:59:16]

Done.

 }
 
 }  // namespace content