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 043362acae7290a66aff40c45d58acebf2dd961a..a347cfe5a6c508840d4e0e4048d049a66fd64683 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 {
@@ -18,55 +19,71 @@ 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());
+  scoped_ptr<ServiceWorkerRegisterJob> job = make_scoped_ptr(
+      new ServiceWorkerRegisterJob(storage_,
+                                   this,
+                                   pattern,
+                                   script_url,
+                                   ServiceWorkerRegisterJob::REGISTER));
+  jobs_[pattern].Push(job.Pass(), callback);
 }
 
 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());
+
+  scoped_ptr<ServiceWorkerRegisterJob> job = make_scoped_ptr(
+      new ServiceWorkerRegisterJob(storage_,
+                                   this,
+                                   pattern,
+                                   GURL(),
+                                   ServiceWorkerRegisterJob::UNREGISTER));
+  jobs_[pattern]
+      .Push(job.Pass(),

[kinuko, 2014/01/09 10:54:08]

Was this formatting done by clang-format?  I (as an incomplete human) slightly
prefer

jobs_[pattern].Push(
    job.Pass(),
    base::Bind(...));

but if that's the case I'm ok with it...

[alecflett, 2014/01/09 21:23:14]

Yes, that was clang-format. the problem was this was the only way to format it
and stay within 80 cols.

+            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.";
+  pending_jobs->second.Pop(job);

[kinuko, 2014/01/09 14:08:08]

You may also want to remove the pattern from the jobs_ map if the queue is
empty?  (JobQueue class could have empty() method)

[alecflett, 2014/01/09 21:23:14]

oops, oversight! Done.

+}
+
+ServiceWorkerJobCoordinator::JobQueue::JobQueue() {}

[kinuko, 2014/01/09 14:08:08]

style-nit: I think we usually put inner class implementation before (or after,
though less common) the entire outer class definition (so each class
implementation doesn't interleave with each other).

[alecflett, 2014/01/09 21:23:14]

I'll move before.

+
+ServiceWorkerJobCoordinator::JobQueue::~JobQueue() {
+  STLDeleteElements(&jobs_);
 }
 
-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;
-    }
+void ServiceWorkerJobCoordinator::JobQueue::Push(
+    scoped_ptr<ServiceWorkerRegisterJob> job,
+    const ServiceWorkerRegisterJob::RegistrationCallback& callback) {
+  if (jobs_.empty()) {
+    job->Start();
+    jobs_.push_back(job.release());
+  } else if (!job->Equals(jobs_.back())) {
+    jobs_.push_back(job.release());
   }
-  NOTREACHED() << "Deleting non-existent job. ";
+
+  DCHECK(!jobs_.empty());
+  jobs_.back()->AddCallback(callback);
 }
 
-void ServiceWorkerJobCoordinator::RegisterComplete(
-    const ServiceWorkerRegisterJob::RegistrationCallback& callback,
-    ServiceWorkerRegisterJob* job,
-    ServiceWorkerRegistrationStatus status,
-    ServiceWorkerRegistration* registration) {
-  callback.Run(status, registration);
-  EraseJob(job);
+void ServiceWorkerJobCoordinator::JobQueue::Pop(ServiceWorkerRegisterJob* job) {
+  DCHECK(job == jobs_.front());
+  jobs_.pop_front();
+  if (!jobs_.empty())
+    jobs_.front()->Start();
 }
 
+
 void ServiceWorkerJobCoordinator::UnregisterComplete(
     const ServiceWorkerRegisterJob::UnregistrationCallback& callback,
-    ServiceWorkerRegisterJob* job,
     ServiceWorkerRegistrationStatus status,
-    ServiceWorkerRegistration* previous_registration) {
+    const scoped_refptr<ServiceWorkerRegistration>& previous_registration) {
   callback.Run(status);
-  EraseJob(job);
 }
 
 }  // namespace content