Refactor job coordination into a separate class.

content/browser/service_worker/service_worker_storage.h

Index: content/browser/service_worker/service_worker_storage.h
diff --git a/content/browser/service_worker/service_worker_storage.h b/content/browser/service_worker/service_worker_storage.h
index b3f1dbfaf69bace0dfe72297de346abbf7693335..0ec3d6357399945fb1965b648a37ed8c04367b20 100644
--- a/content/browser/service_worker/service_worker_storage.h
+++ b/content/browser/service_worker/service_worker_storage.h
@@ -22,23 +22,15 @@ class QuotaManagerProxy;
 namespace content {
 
 class ServiceWorkerRegistration;
-class ServiceWorkerRegisterJob;
 
 // This class provides an interface to load registration data and
-// instantiate ServiceWorkerRegistration objects. Any asynchronous
-// operations are run through instances of ServiceWorkerRegisterJob.
+// instantiate ServiceWorkerRegistration objects.
 class CONTENT_EXPORT ServiceWorkerStorage {
  public:
   ServiceWorkerStorage(const base::FilePath& path,
                        quota::QuotaManagerProxy* quota_manager_proxy);
   ~ServiceWorkerStorage();
 
-  typedef base::Callback<void(ServiceWorkerRegistrationStatus status,
-                              const scoped_refptr<ServiceWorkerRegistration>&
-                                  registration)> RegistrationCallback;
-  typedef base::Callback<
-      void(ServiceWorkerRegistrationStatus status)> UnregistrationCallback;
-
   // `found` is only valid if status == REGISTRATION_OK.
   typedef base::Callback<void(bool found,
                               ServiceWorkerRegistrationStatus status,
@@ -50,20 +42,6 @@ class CONTENT_EXPORT ServiceWorkerStorage {
   void FindRegistrationForPattern(const GURL& pattern,
                                   const FindRegistrationCallback& callback);
 
-  void Register(const GURL& pattern,
-                const GURL& script_url,
-                const RegistrationCallback& callback);
-
-  void Unregister(const GURL& pattern, const UnregistrationCallback& callback);
-
- private:
-  friend class ServiceWorkerRegisterJob;
-  FRIEND_TEST_ALL_PREFIXES(ServiceWorkerStorageTest, PatternMatches);
-
-  typedef std::map<GURL, scoped_refptr<ServiceWorkerRegistration> >
-      PatternToRegistrationMap;
-  typedef ScopedVector<ServiceWorkerRegisterJob> RegistrationJobList;
-
   // TODO(alecflett): These are temporary internal methods providing
   // synchronous in-memory registration. Eventually these will be
   // replaced by asynchronous methods that persist registration to disk.
@@ -71,31 +49,23 @@ class CONTENT_EXPORT ServiceWorkerStorage {
       const GURL& pattern,
       const GURL& script_url);
   void UnregisterInternal(const GURL& pattern);
-  static bool PatternMatches(const GURL& pattern, const GURL& script_url);
 
-  // Jobs are removed whenever they are finished or canceled.
-  void EraseJob(ServiceWorkerRegisterJob* job);
+  base::WeakPtr<ServiceWorkerStorage> AsWeakPtr() {
+    return weak_factory_.GetWeakPtr();
+  }

[kinuko, 2013/12/18 05:43:35]

I don't think we should expose this as public AsWeakPtr() method, if it really
needs to do so this should just inherit SupportsWeakPtr (and I think in this
case JobCoordinator could take ContextCore's weakptr instead of storage's one)

[alecflett, 2014/01/06 21:47:12]

The real problem with SupportsWeakPtr is the odd order that the object gets
destroyed. To quote wez:
(this is the tail end of
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/N6w_CkKEwvE)
so I was reluctant to use it here. It doesn't actually impact the current code,
I was just trying to be future-proof since there is still some interest in
killing SupportsWeakPtr

[kinuko, 2014/01/08 06:19:10]

I think I'm saying that I don't want to introduce more public WeakPtr usage
(rather than SupportsWeakPtr vs WeakFactory argument) which is in general
discouraged unless we have clear reasons.  I agree with the point that
SupportsWeakPtr has unfortunate dtor order issue.

And as I noted elsewhere it doesn't look we need WeakPtr here.  I do prefer
keeping each object's lifetime explicit whenever possible, rather than relying
on RefPtr + public WeakPtr which could easily make things wild.

[alecflett, 2014/01/08 23:17:11]

Ah I understand your preference now. the real reason for the weakptr was so that
ServiceWorkerRegisterJob instances could be canceled during ServiceWorkerStorage
teardown while they still had outstanding asynchronous steps, without a lot of
explicit cleanup.  You'll notice that in the updated version of this patch I
also had to tweak ServiceWorkerRegisterJob to hold a raw pointer as well.

But I'm happy to just go with explicit cleanup as you suggest.


I'll get rid of the WeakPtr usage and annotate as appropriate.

 
-  // Called at ServiceWorkerRegisterJob completion.
-  void RegisterComplete(const RegistrationCallback& callback,
-                        ServiceWorkerRegisterJob* job,
-                        ServiceWorkerRegistrationStatus status,
-                        ServiceWorkerRegistration* registration);
+ private:
+  FRIEND_TEST_ALL_PREFIXES(ServiceWorkerStorageTest, PatternMatches);
 
-  // Called at ServiceWorkerRegisterJob completion.
-  void UnregisterComplete(const UnregistrationCallback& callback,
-                          ServiceWorkerRegisterJob* job,
-                          ServiceWorkerRegistrationStatus status,
-                          ServiceWorkerRegistration* registration);
+  typedef std::map<GURL, scoped_refptr<ServiceWorkerRegistration> >
+      PatternToRegistrationMap;
+
+  static bool PatternMatches(const GURL& pattern, const GURL& script_url);
 
   // This is the in-memory registration. Eventually the registration will be
   // persisted to disk.
-  // A list of currently running jobs. This is a temporary structure until we
-  // start managing overlapping registrations explicitly.
-  RegistrationJobList registration_jobs_;
-
-  // in-memory map, to eventually be replaced with persistence
   PatternToRegistrationMap registration_by_pattern_;

[kinuko, 2013/12/18 05:43:35]

I assume on-disk storage will have triplets like { pattern, script_url,
registration_id } while we'll still have some in-memory mapping from
registration_id to ServiceWorkerRegistration for the ones that are currently
active / in the working set. Am I correct?

And to manage ongoing registration/unregistration we'll first need to determine:
- which registration the given pattern should be mapped to, and
- if there's any ongoing registration/unregistration job for the
ServiceWorkerRegistration

But in the current code structure look-up part is in a (possibly long-running)
job itself, and I'm still not sure how JobCoordinator will manage multiple jobs.
How are you planning to do there?

[alecflett, 2014/01/06 21:47:12]

more or less - more likely something like a persistent map of pattern -> {
script_url, registration_id }
right.

The followon patch basically has a queue of registration/unregistrations for
each pattern. Basically each registration/unregistration job now has a list of
callbacks instead of a single callback. When a request for a  registration comes
in, we look at the most recent registration/unregistration for that pattern - if
it is identical, then we hook into that job's existing callbacks. If it is
different, then we put it into the queue. When the job finishes (i.e. after all
callbacks have fired) it makes a separate out-of-band call to the
TransactionCoordinator to let it know that it has completed and can be removed
from the queue.

+
   scoped_refptr<quota::QuotaManagerProxy> quota_manager_proxy_;
   base::FilePath path_;
   base::WeakPtrFactory<ServiceWorkerStorage> weak_factory_;