ServiceWorker: Make SWProviderHost listen to SWRegistration

ServiceWorker: Make SWProviderHost listen to SWRegistration

This is a refactoring patch. Before this patch, ServiceWorkerRegisterJob is
responsible for updating ServiceWorkerRegistration object and document's
version attributes (ie. '.installing' etc) via ServiceWorkerProviderHost.

This change makes the provider host implement Registration::Listener.
Listers are notified when the registration is updated, so the register
job no longer has to manage the version attributes.


BUG=396400
TEST=content_unittests --gtest_filter=ServiceWorker*
TEST=run_webkit_tests.py --debug http/tests/serviceworker

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

[nhiroki, 2014-08-01 06:06:06]

Hi, can you review this?

I'd like to clean up Registration/ProviderHost relations before implementing
version attributes (ie. '.installing' etc) on ServiceWorkerRegistration
interface.

Thanks!

[falken, 2014-08-05 02:41:28]

lgtm

https://codereview.chromium.org/417043006/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_provider_host.cc (right):

https://codereview.chromium.org/417043006/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_provider_host.cc:221: if
(!context_ || associated_registration_)
nit: Since AssociateRegistration only takes effect one time, the name is a bit
misleading. The code might be more clear if you add a CanAssociateRegistration()
and have SetRegistration DCHECK it, like the Set*Version functions.

https://codereview.chromium.org/417043006/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_provider_host.h (right):

https://codereview.chromium.org/417043006/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_provider_host.h:111: // Associates
to |registration| to listen its version change events.
s/listen/listen for/g

https://codereview.chromium.org/417043006/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_register_job.cc (right):

https://codereview.chromium.org/417043006/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_register_job.cc:463:
host->AssociateRegistration(registration);
I wonder what happens in cases like register(), load a document, unregister(),
then register() again. The loaded document won't change registration, but it
should get updated installing/waiting versions. I guess this is already broken
and the spec'd behavior may change in
https://github.com/slightlyoff/ServiceWorker/issues/396

[nhiroki, 2014-08-05 15:51:13]

Thank you for your comments!

https://codereview.chromium.org/417043006/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_provider_host.cc (right):

https://codereview.chromium.org/417043006/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_provider_host.cc:221: if
(!context_ || associated_registration_)
On 2014/08/05 02:41:28, falken wrote:
> nit: Since AssociateRegistration only takes effect one time, the name is a bit
> misleading. The code might be more clear if you add a
CanAssociateRegistration()
> and have SetRegistration DCHECK it, like the Set*Version functions.

Done.

https://codereview.chromium.org/417043006/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_provider_host.h (right):

https://codereview.chromium.org/417043006/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_provider_host.h:111: // Associates
to |registration| to listen its version change events.
On 2014/08/05 02:41:28, falken wrote:
> s/listen/listen for/g

Done.

https://codereview.chromium.org/417043006/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_register_job.cc (right):

https://codereview.chromium.org/417043006/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_register_job.cc:463:
host->AssociateRegistration(registration);
On 2014/08/05 02:41:28, falken wrote:
> I wonder what happens in cases like register(), load a document, unregister(),
> then register() again. The loaded document won't change registration, but it
> should get updated installing/waiting versions. I guess this is already broken
> and the spec'd behavior may change in
> https://github.com/slightlyoff/ServiceWorker/issues/396

Thank you for sharing the issue. Yes, this is already broken.

Probably my subsequent CLs will revamp this part to implement '.installing' etc
on ServiceWorkerRegistration, so let me revisit then.

[michaeln, 2014-08-05 23:57:16]

https://codereview.chromium.org/417043006/diff/100001/content/browser/service...
File content/browser/service_worker/service_worker_provider_host.h (right):

https://codereview.chromium.org/417043006/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.h:51: //
ServiceWorkerRegistration::Listener overrides.
please put this in the private section since this is not meant to be called
directly by consumers of the ServiceWorkerProviderHost class

https://codereview.chromium.org/417043006/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.h:92: void
SetInstallingVersion(ServiceWorkerVersion* version);
I think these methods to alter versions on this class shouldn't need to be here
anymore. The ServiceWorkerRegistration object is the source of truth about this
and so long as the host is listening, these can be private.

ServiceWorkerControlleeRequestHandler::PrepareForMainResource probably should
also establish the association between a host and its registration.

https://codereview.chromium.org/417043006/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.h:99: void
SetRegistration(ServiceWorkerRegistration* registration);
We'll need to clear this association too in some cases.

https://codereview.chromium.org/417043006/diff/100001/content/browser/service...
File content/browser/service_worker/service_worker_register_job.cc (right):

https://codereview.chromium.org/417043006/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_register_job.cc:186:
AssociateProviderHostToRegistration(existing_registration);
Not sure this is needed in the existing registration case. If there's an
existing registration for 'scope', it should already have been associated at
main resource load time for all existing documents.

https://codereview.chromium.org/417043006/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_register_job.cc:379: if
(!registration()->active_version()) {
In the failed registration case, the dead registration is left associated with
existing documents.

[nhiroki, 2014-08-06 06:57:27]

Thank you for reviewing. PTAL.

falken@: Can you take another look, too? I think this patchset will affect your
'unregister-and-register' issue.

https://codereview.chromium.org/417043006/diff/100001/content/browser/service...
File content/browser/service_worker/service_worker_provider_host.h (right):

https://codereview.chromium.org/417043006/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.h:51: //
ServiceWorkerRegistration::Listener overrides.
On 2014/08/05 23:57:15, michaeln wrote:
> please put this in the private section since this is not meant to be called
> directly by consumers of the ServiceWorkerProviderHost class

Moved.

https://codereview.chromium.org/417043006/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.h:92: void
SetInstallingVersion(ServiceWorkerVersion* version);
On 2014/08/05 23:57:15, michaeln wrote:
> I think these methods to alter versions on this class shouldn't need to be
here
> anymore. The ServiceWorkerRegistration object is the source of truth about
this
> and so long as the host is listening, these can be private.

Moved.

> ServiceWorkerControlleeRequestHandler::PrepareForMainResource probably should
> also establish the association between a host and its registration.
> 

Done. Made PrepareForMainResource() and so on set the registration.

https://codereview.chromium.org/417043006/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.h:99: void
SetRegistration(ServiceWorkerRegistration* registration);
On 2014/08/05 23:57:15, michaeln wrote:
> We'll need to clear this association too in some cases.

Done (please see my reply comment in service_worker_register_job.cc).

https://codereview.chromium.org/417043006/diff/100001/content/browser/service...
File content/browser/service_worker/service_worker_register_job.cc (right):

https://codereview.chromium.org/417043006/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_register_job.cc:186:
AssociateProviderHostToRegistration(existing_registration);
On 2014/08/05 23:57:16, michaeln wrote:
> Not sure this is needed in the existing registration case. If there's an
> existing registration for 'scope', it should already have been associated at
> main resource load time for all existing documents.

I think you are correct. All existing documents should already be associated
with the existing registration when we reach here. Removed this call.

https://codereview.chromium.org/417043006/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_register_job.cc:379: if
(!registration()->active_version()) {
On 2014/08/05 23:57:15, michaeln wrote:
> In the failed registration case, the dead registration is left associated with
> existing documents.

I see... probably we have to remove listeners from observer list when the
associated registration is marked as deleted (or actually deleted from the DB?).
To do that, I added Registration::Listener::OnRegistrationDeleted that is
notified when the registration is deleted, and made ProviderHost implement it to
disassociate from the registration.

falken@: How does this look to you? I assume this will affect your
'unregister-and-register' issue/patch.

https://codereview.chromium.org/417043006/diff/140001/content/browser/service...
File content/browser/service_worker/service_worker_storage.cc (right):

https://codereview.chromium.org/417043006/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_storage.cc:911:
StartPurgingResources(newly_purgeable_resources);
(Or do we have to notify the listeners of OnRegistrationDeleted here instead of
SetIsDeleted()?)

[falken, 2014-08-06 16:09:35]

https://codereview.chromium.org/417043006/diff/100001/content/browser/service...
File content/browser/service_worker/service_worker_register_job.cc (right):

https://codereview.chromium.org/417043006/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_register_job.cc:379: if
(!registration()->active_version()) {
On 2014/08/06 06:57:27, nhiroki wrote:
> On 2014/08/05 23:57:15, michaeln wrote:
> > In the failed registration case, the dead registration is left associated
with
> > existing documents.
> 
> I see... probably we have to remove listeners from observer list when the
> associated registration is marked as deleted (or actually deleted from the
DB?).
> To do that, I added Registration::Listener::OnRegistrationDeleted that is
> notified when the registration is deleted, and made ProviderHost implement it
to
> disassociate from the registration.
> 
> falken@: How does this look to you? I assume this will affect your
> 'unregister-and-register' issue/patch.

Don't worry about the unreg-and-reg patch, it's on hold while the spec is
getting revised.

But.... I don't really understand why we need to disassociate when the
registration is deleted?

storage()->DeleteRegistration just means the registration is deleted from the
DB. It's conceivable the live registration can still have version changes.

What's the problem you're trying to solve by disassociating when the reg is
deleted?

https://codereview.chromium.org/417043006/diff/140001/content/browser/service...
File content/browser/service_worker/service_worker_provider_host_unittest.cc
(right):

https://codereview.chromium.org/417043006/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_provider_host_unittest.cc:62: void
SetActiveVersion(
I don't understand the motivation for this function? Seems clearer without it.

https://codereview.chromium.org/417043006/diff/140001/content/browser/service...
File content/browser/service_worker/service_worker_register_job.cc (right):

https://codereview.chromium.org/417043006/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_register_job.cc:453: void
ServiceWorkerRegisterJob::AssociateProviderHostToRegistration(
nit: it's multiple hosts, so s/ProviderHost/ProviderHosts/

https://codereview.chromium.org/417043006/diff/140001/content/browser/service...
File content/browser/service_worker/service_worker_registration.cc (right):

https://codereview.chromium.org/417043006/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_registration.cc:216: is_deleted_ =
false;
As here, it's possible to flip back out of is_deleted_. So I'm not sure
listening for the moment is_deleted_ turns true then cutting ties with the
registration is right.

[nhiroki, 2014-08-07 01:27:59]

Reply only. Will update the patch soon...

https://codereview.chromium.org/417043006/diff/100001/content/browser/service...
File content/browser/service_worker/service_worker_register_job.cc (right):

https://codereview.chromium.org/417043006/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_register_job.cc:379: if
(!registration()->active_version()) {
On 2014/08/06 16:09:35, falken wrote:
> On 2014/08/06 06:57:27, nhiroki wrote:
> > On 2014/08/05 23:57:15, michaeln wrote:
> > > In the failed registration case, the dead registration is left associated
> with
> > > existing documents.
> > 
> > I see... probably we have to remove listeners from observer list when the
> > associated registration is marked as deleted (or actually deleted from the
> DB?).
> > To do that, I added Registration::Listener::OnRegistrationDeleted that is
> > notified when the registration is deleted, and made ProviderHost implement
it
> to
> > disassociate from the registration.
> > 
> > falken@: How does this look to you? I assume this will affect your
> > 'unregister-and-register' issue/patch.
> 
> Don't worry about the unreg-and-reg patch, it's on hold while the spec is
> getting revised.
> 
> But.... I don't really understand why we need to disassociate when the
> registration is deleted?
> 
> storage()->DeleteRegistration just means the registration is deleted from the
> DB. It's conceivable the live registration can still have version changes.

Good point and I had a misconception... I have to disassociate the provider host
from the registration *only* in this dead registration case.

> What's the problem you're trying to solve by disassociating when the reg is
> deleted?

I wanted to make the provider host release the reference (refptr) to the
registration after deleting its on-disk representation, or the provider host
will keep the reference to the deleted one until the corresponding document gets
closed. 

But as you said, the live registration will continue to notify and I don't have
to disassociate it in normal cases.

https://codereview.chromium.org/417043006/diff/140001/content/browser/service...
File content/browser/service_worker/service_worker_provider_host_unittest.cc
(right):

https://codereview.chromium.org/417043006/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_provider_host_unittest.cc:62: void
SetActiveVersion(
On 2014/08/06 16:09:35, falken wrote:
> I don't understand the motivation for this function? Seems clearer without it.

ServiceWorkerProviderHost::SetXXXVersion() is private. I made
ServiceWorkerProviderHostTest 'friend' instead of making all tests 'friend'.

[michaeln, 2014-08-07 01:34:35]

https://codereview.chromium.org/417043006/diff/140001/content/browser/service...
File content/browser/service_worker/service_worker_provider_host.cc (right):

https://codereview.chromium.org/417043006/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.cc:222: bool
ServiceWorkerProviderHost::CanAssociateVersion(
i think this check gets simplified to version->registration_id() ==
associated_reg_->id()

https://codereview.chromium.org/417043006/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.cc:249: return
false;
also if (running_hosted_version_) return false;

https://codereview.chromium.org/417043006/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.cc:250: if
(!registration)
seems like NULL should not be valid inputs to this method?

https://codereview.chromium.org/417043006/diff/140001/content/browser/service...
File content/browser/service_worker/service_worker_provider_host.h (right):

https://codereview.chromium.org/417043006/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.h:86: void
SetRegistration(ServiceWorkerRegistration* registration);
For similarity with the Can method, i'd call this AssociateRegistration().
Instead of accepting NULL as an input here, maybe it must be non-null but we
have an UnassociateRegistration() method that takes no input for that purpose.
The impl of each method is much more of a straight line, and the inputs are more
clear.

also an associated_registration() getter is probably a good addition too

https://codereview.chromium.org/417043006/diff/140001/content/browser/service...
File content/browser/service_worker/service_worker_register_job.cc (right):

https://codereview.chromium.org/417043006/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_register_job.cc:378: if
(!registration()->active_version()) {
and to call UnassociateFailedRegistration() here

https://codereview.chromium.org/417043006/diff/140001/content/browser/service...
File content/browser/service_worker/service_worker_registration.cc (right):

https://codereview.chromium.org/417043006/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_registration.cc:216: is_deleted_ =
false;
On 2014/08/06 16:09:35, falken wrote:
> As here, it's possible to flip back out of is_deleted_. So I'm not sure
> listening for the moment is_deleted_ turns true then cutting ties with the
> registration is right.

agreed that its not deletion that matters so much as registration that fails to
launch (so to speak), maybe the most direct way to handle this is to have the
RegistrationJob use another helper to UnassociateFailedRegistration.

[nhiroki, 2014-08-07 02:57:27]

Thanks! Updated.

https://codereview.chromium.org/417043006/diff/140001/content/browser/service...
File content/browser/service_worker/service_worker_provider_host.cc (right):

https://codereview.chromium.org/417043006/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.cc:222: bool
ServiceWorkerProviderHost::CanAssociateVersion(
On 2014/08/07 01:34:35, michaeln wrote:
> i think this check gets simplified to version->registration_id() ==
> associated_reg_->id()

Acknowledged. I'll make a separate CL for this clean up.

https://codereview.chromium.org/417043006/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.cc:249: return
false;
On 2014/08/07 01:34:35, michaeln wrote:
> also if (running_hosted_version_) return false;

Done.

https://codereview.chromium.org/417043006/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.cc:250: if
(!registration)
On 2014/08/07 01:34:35, michaeln wrote:
> seems like NULL should not be valid inputs to this method?

Done.

https://codereview.chromium.org/417043006/diff/140001/content/browser/service...
File content/browser/service_worker/service_worker_provider_host.h (right):

https://codereview.chromium.org/417043006/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.h:86: void
SetRegistration(ServiceWorkerRegistration* registration);
On 2014/08/07 01:34:35, michaeln wrote:
> For similarity with the Can method, i'd call this AssociateRegistration().
> Instead of accepting NULL as an input here, maybe it must be non-null but we
> have an UnassociateRegistration() method that takes no input for that purpose.
> The impl of each method is much more of a straight line, and the inputs are
more
> clear.

Done.

> also an associated_registration() getter is probably a good addition too

I don't really understand why this is a good addition since
|associated_registration_| is used only in this class.

https://codereview.chromium.org/417043006/diff/140001/content/browser/service...
File content/browser/service_worker/service_worker_register_job.cc (right):

https://codereview.chromium.org/417043006/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_register_job.cc:378: if
(!registration()->active_version()) {
On 2014/08/07 01:34:35, michaeln wrote:
> and to call UnassociateFailedRegistration() here

Done.

https://codereview.chromium.org/417043006/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_register_job.cc:453: void
ServiceWorkerRegisterJob::AssociateProviderHostToRegistration(
On 2014/08/06 16:09:35, falken wrote:
> nit: it's multiple hosts, so s/ProviderHost/ProviderHosts/

Done.

https://codereview.chromium.org/417043006/diff/140001/content/browser/service...
File content/browser/service_worker/service_worker_registration.cc (right):

https://codereview.chromium.org/417043006/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_registration.cc:216: is_deleted_ =
false;
On 2014/08/07 01:34:35, michaeln wrote:
> On 2014/08/06 16:09:35, falken wrote:
> > As here, it's possible to flip back out of is_deleted_. So I'm not sure
> > listening for the moment is_deleted_ turns true then cutting ties with the
> > registration is right.
> 
> agreed that its not deletion that matters so much as registration that fails
to
> launch (so to speak), maybe the most direct way to handle this is to have the
> RegistrationJob use another helper to UnassociateFailedRegistration.

Acknowledged.

[nhiroki, 2014-08-07 03:13:54]

https://codereview.chromium.org/417043006/diff/140001/content/browser/service...
File content/browser/service_worker/service_worker_provider_host.h (right):

https://codereview.chromium.org/417043006/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.h:86: void
SetRegistration(ServiceWorkerRegistration* registration);
On 2014/08/07 02:57:27, nhiroki wrote:
> On 2014/08/07 01:34:35, michaeln wrote:
> > For similarity with the Can method, i'd call this AssociateRegistration().
> > Instead of accepting NULL as an input here, maybe it must be non-null but we
> > have an UnassociateRegistration() method that takes no input for that
purpose.
> > The impl of each method is much more of a straight line, and the inputs are
> more
> > clear.
> 
> Done.
> 
> > also an associated_registration() getter is probably a good addition too
> 
> I don't really understand why this is a good addition since
> |associated_registration_| is used only in this class.

Ah, maybe I got it... you mean that having the getter is useful to check the
existence of the associated registration before calling
UnassociateRegistration(). Is this correct?

In the patchset6, I made the function just return if the associated registration
is null.

[falken, 2014-08-07 03:59:28]

Thanks for the explanations. LGTM

https://codereview.chromium.org/417043006/diff/220001/content/browser/service...
File content/browser/service_worker/service_worker_provider_host.cc (right):

https://codereview.chromium.org/417043006/diff/220001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.cc:186: if
(associated_registration_)
Now this can't happen, CanAssociateRegistration would return false.

[nhiroki, 2014-08-07 04:10:43]

https://codereview.chromium.org/417043006/diff/220001/content/browser/service...
File content/browser/service_worker/service_worker_provider_host.cc (right):

https://codereview.chromium.org/417043006/diff/220001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.cc:186: if
(associated_registration_)
On 2014/08/07 03:59:26, falken wrote:
> Now this can't happen, CanAssociateRegistration would return false.

Done. Thanks!

[michaeln, 2014-08-07 20:33:07]

lgtm

https://codereview.chromium.org/417043006/diff/140001/content/browser/service...
File content/browser/service_worker/service_worker_provider_host.h (right):

https://codereview.chromium.org/417043006/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.h:86: void
SetRegistration(ServiceWorkerRegistration* registration);
On 2014/08/07 03:13:54, nhiroki wrote:
> On 2014/08/07 02:57:27, nhiroki wrote:
> > On 2014/08/07 01:34:35, michaeln wrote:
> > > For similarity with the Can method, i'd call this AssociateRegistration().
> > > Instead of accepting NULL as an input here, maybe it must be non-null but
we
> > > have an UnassociateRegistration() method that takes no input for that
> purpose.
> > > The impl of each method is much more of a straight line, and the inputs
are
> > more
> > > clear.
> > 
> > Done.
> > 
> > > also an associated_registration() getter is probably a good addition too
> > 
> > I don't really understand why this is a good addition since
> > |associated_registration_| is used only in this class.

Seems more reflective of the object model. The getters for active, waiting, and
installing versions could be removed. W/o it, there is api to associated and
unassociate, but no api to query the association which seems odd, what's so
secret about it?

https://codereview.chromium.org/417043006/diff/240001/content/browser/service...
File content/browser/service_worker/service_worker_provider_host.cc (right):

https://codereview.chromium.org/417043006/diff/240001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.cc:202:
SetWaitingVersion(NULL);
i think we should clear the controllingVersion here too since i don't think
having a controlling version w/o a registration associated is a valid state to
be in.

https://codereview.chromium.org/417043006/diff/240001/content/browser/service...
File content/browser/service_worker/service_worker_register_job.cc (right):

https://codereview.chromium.org/417043006/diff/240001/content/browser/service...
content/browser/service_worker/service_worker_register_job.cc:378: if
(!registration()->active_version()) {
i think this may need to be if (!active_version() && !waiting_version()) now
that jobs can complete prior to setting up the active version?

[nhiroki, 2014-08-08 06:31:10]

On 2014/08/07 20:33:07, michaeln wrote:
> lgtm
> 
>
https://codereview.chromium.org/417043006/diff/140001/content/browser/service...
> File content/browser/service_worker/service_worker_provider_host.h (right):
> 
>
https://codereview.chromium.org/417043006/diff/140001/content/browser/service...
> content/browser/service_worker/service_worker_provider_host.h:86: void
> SetRegistration(ServiceWorkerRegistration* registration);
> On 2014/08/07 03:13:54, nhiroki wrote:
> > On 2014/08/07 02:57:27, nhiroki wrote:
> > > On 2014/08/07 01:34:35, michaeln wrote:
> > > > For similarity with the Can method, i'd call this
AssociateRegistration().
> > > > Instead of accepting NULL as an input here, maybe it must be non-null
but
> we
> > > > have an UnassociateRegistration() method that takes no input for that
> > purpose.
> > > > The impl of each method is much more of a straight line, and the inputs
> are
> > > more
> > > > clear.
> > > 
> > > Done.
> > > 
> > > > also an associated_registration() getter is probably a good addition too
> > > 
> > > I don't really understand why this is a good addition since
> > > |associated_registration_| is used only in this class.
> 
> Seems more reflective of the object model. The getters for active, waiting,
and
> installing versions could be removed. W/o it, there is api to associated and
> unassociate, but no api to query the association which seems odd, what's so
> secret about it?

Thank you for your explanation. It makes sense to me. Probably the subsequent
CLs to implement ServiceWorkerRegistration object will remove/modify
|associated_registration_| since the provider host needs to listen to multiple
registrations, so let me keep it as is for now...

[nhiroki, 2014-08-08 06:31:45]

https://codereview.chromium.org/417043006/diff/240001/content/browser/service...
File content/browser/service_worker/service_worker_provider_host.cc (right):

https://codereview.chromium.org/417043006/diff/240001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.cc:202:
SetWaitingVersion(NULL);
On 2014/08/07 20:33:07, michaeln wrote:
> i think we should clear the controllingVersion here too since i don't think
> having a controlling version w/o a registration associated is a valid state to
> be in.

Done.

https://codereview.chromium.org/417043006/diff/240001/content/browser/service...
File content/browser/service_worker/service_worker_register_job.cc (right):

https://codereview.chromium.org/417043006/diff/240001/content/browser/service...
content/browser/service_worker/service_worker_register_job.cc:378: if
(!registration()->active_version()) {
On 2014/08/07 20:33:07, michaeln wrote:
> i think this may need to be if (!active_version() && !waiting_version()) now
> that jobs can complete prior to setting up the active version?

Done.

[nhiroki, 2014-08-08 06:33:06]

The CQ bit was checked by nhiroki@chromium.org

[commit-bot: I haz the power, 2014-08-08 06:36:44]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/nhiroki@chromium.org/417043006/260001

[commit-bot: I haz the power, 2014-08-08 10:55:43]

Change committed as 288323