Refactor job coordination into a separate class.

Refactor job coordination into a separate class.

This makes ServiceWorkerStorage return to being a mostly stateless class,
at least with respect to jobs, and in fact it is now ignorant of registraiton jobs. This CL is purely mechanical, moving tests and functions around without altering any behavior.
BUG=
TBR=jam@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=243759

[alecflett, 2013-12-17 00:02:08]

michaeln@, kinuko@ - PTAL?

[kinuko, 2013-12-18 05:43:34]

Since we're stepping out from original class structure design, it might be
better to have another lightweight class-structure writeup or something?

https://codereview.chromium.org/113133013/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_context_core.cc (right):

https://codereview.chromium.org/113133013/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_context_core.cc:25:
job_coordinator_(new ServiceWorkerJobCoordinator(storage_->AsWeakPtr())),
I think probably this should take ContextCore's weakptr rather than storage's. 
Storage and other related classes can be accessed via context, and the loss of
context's weakptr indicates the loss of storage.

https://codereview.chromium.org/113133013/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_job_coordinator.cc (right):

https://codereview.chromium.org/113133013/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_job_coordinator.cc:48:
registration_jobs_.erase(job_position);
Keeping this as vector (scoped_vector) while we'll erase random item feels a bit
inefficient.

https://codereview.chromium.org/113133013/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_storage.h (right):

https://codereview.chromium.org/113133013/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_storage.h:55: }
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)

https://codereview.chromium.org/113133013/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_storage.h:67:
PatternToRegistrationMap registration_by_pattern_;
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]

https://codereview.chromium.org/113133013/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_context_core.cc (right):

https://codereview.chromium.org/113133013/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_context_core.cc:25:
job_coordinator_(new ServiceWorkerJobCoordinator(storage_->AsWeakPtr())),
On 2013/12/18 05:43:35, kinuko wrote:
> I think probably this should take ContextCore's weakptr rather than storage's.

> Storage and other related classes can be accessed via context, and the loss of
> context's weakptr indicates the loss of storage.

My interest here is more around encapsulation and testing - I imagine that
ContextCore is going to continue to grow, and by limiting exposure to Storage,
it's easier to test the registration process.

https://codereview.chromium.org/113133013/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_job_coordinator.cc (right):

https://codereview.chromium.org/113133013/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_job_coordinator.cc:48:
registration_jobs_.erase(job_position);
On 2013/12/18 05:43:35, kinuko wrote:
> Keeping this as vector (scoped_vector) while we'll erase random item feels a
bit
> inefficient.

Agreed. This array is going to get cleaned up in my next patch (hopefully
available by the time you read this)

https://codereview.chromium.org/113133013/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_storage.h (right):

https://codereview.chromium.org/113133013/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_storage.h:55: }
On 2013/12/18 05:43:35, kinuko wrote:
> 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)

The real problem with SupportsWeakPtr is the odd order that the object gets
destroyed. To quote wez:

> SupportsWeakPtr invalidates WeakPtrs to itself in the destructor, but the
destructor only runs after the derived class' destructor (and those of the
derived class' members).

(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

https://codereview.chromium.org/113133013/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_storage.h:67:
PatternToRegistrationMap registration_by_pattern_;
On 2013/12/18 05:43:35, kinuko wrote:
> 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?

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

> 
> 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
> 
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.

[alecflett, 2014-01-07 23:44:22]

Ok, followon patch which deals with duplicate/overlapping registration jobs is
here:
http://crrev.com/126603002

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

Some more comments about WeakPtr usage. I'm going to take a look at another
patch shortly..

https://codereview.chromium.org/113133013/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_context_core.cc (right):

https://codereview.chromium.org/113133013/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_context_core.cc:25:
job_coordinator_(new ServiceWorkerJobCoordinator(storage_->AsWeakPtr())),
I rather think that we can always create a small ContextCore for tests and which
could make things simpler, rather than making many objects depend on each
other's weak ptr separately.

...actually looking into the code more Storage and JobCoordinator (and
ContextCore) seem to have the same life time at least in the current
implementation, since both are owned by ContextCore.

Can we just remove this WeakPtr notion and add an explicit comment that (at
least in the current implementation) they all must have the same lifetime?

https://codereview.chromium.org/113133013/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_storage.h (right):

https://codereview.chromium.org/113133013/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_storage.h:55: }
On 2014/01/06 21:47:12, alecflett wrote:
> On 2013/12/18 05:43:35, kinuko wrote:
> > 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)
> 
> The real problem with SupportsWeakPtr is the odd order that the object gets
> destroyed. To quote wez:
> 
> > SupportsWeakPtr invalidates WeakPtrs to itself in the destructor, but the
> destructor only runs after the derived class' destructor (and those of the
> derived class' members).
> 
> (this is the tail end of
>
https://groups.google.com/a/chromium.org/forum/#%21topic/chromium-dev/N6w_CkK...)
> 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

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.

[kinuko, 2014-01-08 08:12:39]

On 2014/01/08 06:19:10, kinuko wrote:
> Some more comments about WeakPtr usage. I'm going to take a look at another
> patch shortly..
> 
>
https://codereview.chromium.org/113133013/diff/1/content/browser/service_work...
> File content/browser/service_worker/service_worker_context_core.cc (right):
> 
>
https://codereview.chromium.org/113133013/diff/1/content/browser/service_work...
> content/browser/service_worker/service_worker_context_core.cc:25:
> job_coordinator_(new ServiceWorkerJobCoordinator(storage_->AsWeakPtr())),
> I rather think that we can always create a small ContextCore for tests and
which
> could make things simpler, rather than making many objects depend on each
> other's weak ptr separately.
> 
> ...actually looking into the code more Storage and JobCoordinator (and
> ContextCore) seem to have the same life time at least in the current
> implementation, since both are owned by ContextCore.
> 
> Can we just remove this WeakPtr notion and add an explicit comment that (at
> least in the current implementation) they all must have the same lifetime?
> 
>
https://codereview.chromium.org/113133013/diff/1/content/browser/service_work...
> File content/browser/service_worker/service_worker_storage.h (right):
> 
>
https://codereview.chromium.org/113133013/diff/1/content/browser/service_work...
> content/browser/service_worker/service_worker_storage.h:55: }
> On 2014/01/06 21:47:12, alecflett wrote:
> > On 2013/12/18 05:43:35, kinuko wrote:
> > > 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)
> > 
> > The real problem with SupportsWeakPtr is the odd order that the object gets
> > destroyed. To quote wez:
> > 
> > > SupportsWeakPtr invalidates WeakPtrs to itself in the destructor, but the
> > destructor only runs after the derived class' destructor (and those of the
> > derived class' members).
> > 
> > (this is the tail end of
> >
>
https://groups.google.com/a/chromium.org/forum/#%2521topic/chromium-dev/N6w_C...)
> > 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
> 
> 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.

Btw since this is mostly just refactoring (and you're addressing concurrent job
issue in another patch) I'm ok to land this, but would like to make this
lifetime issue clear.

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

Here's the updated patch, PTAL

https://codereview.chromium.org/113133013/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_storage.h (right):

https://codereview.chromium.org/113133013/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_storage.h:55: }
On 2014/01/08 06:19:10, kinuko wrote:
> On 2014/01/06 21:47:12, alecflett wrote:
> > On 2013/12/18 05:43:35, kinuko wrote:
> > > 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)
> > 
> > The real problem with SupportsWeakPtr is the odd order that the object gets
> > destroyed. To quote wez:
> > 
> > > SupportsWeakPtr invalidates WeakPtrs to itself in the destructor, but the
> > destructor only runs after the derived class' destructor (and those of the
> > derived class' members).
> > 
> > (this is the tail end of
> >
>
https://groups.google.com/a/chromium.org/forum/#%2521topic/chromium-dev/N6w_C...)
> > 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
> 
> 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.

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.

[alecflett, 2014-01-08 23:18:34]

(TBR'ed jam for the .gypi changes - kinuko feel free to check the CQ bit if you
get to this while I'm offline)

[kinuko, 2014-01-09 01:41:59]

lgtm

https://codereview.chromium.org/113133013/diff/160001/content/browser/service...
File content/browser/service_worker/service_worker_job_coordinator.h (right):

https://codereview.chromium.org/113133013/diff/160001/content/browser/service...
content/browser/service_worker/service_worker_job_coordinator.h:57: // The
ServiceWorkerStorage object should always outlive this
nit: end comments with period

[commit-bot: I haz the power, 2014-01-09 01:50:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alecflett@chromium.org/113133013/160001

[commit-bot: I haz the power, 2014-01-09 04:16:42]

Change committed as 243759