ServiceWorker: some more groundwork in support of the update process

ServiceWorker: some more groundwork in support of the update process.
* add an 'installing' version to the registration object
* have the register job move the new version first to installing slot and then to waiting slot

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

[michaeln, 2014-07-01 00:45:13]

hi matt, this isn't ready yet but i think my first step is to add a way to
represent the 'installing' version in the registration and host classes.

[nhiroki, 2014-07-01 01:14:00]

FYI: I have a wip CL to support '.active' version (http://crrev.com/354643003)
and probably it will conflict this CL. Cleanup around version association in
this CL would be useful for my CL, too, so please feel free to commit this
first. Thanks.

[falken, 2014-07-01 05:56:57]

Yep, this all looks reasonable to me.

[michaeln, 2014-07-02 02:53:37]

ok, ptal

The way i'm getting a handle on the reg/update process is to have the 'job' do
the work of creating a new version/registration and of taking those objects thru
the state transitions described by the algos. I'm moving away from having the
job directly manipulate the state of host objects. In future cls, i intend to...

* associate a registration with each provider host (primarily, this happens at
main resource load time)
* when that association first occurs, send a message like
ServiceWorkerMsg_SetContainerAttributes (crbug/371671) to inform the renderer
* have each associated provider host listen for OnVersionAttributesChanged()
notifications
* when they hear something, send a ServiceWorkerMsg_SetContainerAttributes
message
* when starting a new registration, the job will have to first associate a
registration with each provider host, if the job is doing an update this won't
be needed (they'll already be associated)
* from there on, the job moves things forward by calling reg->SetXxxVersion()
and version->DispatchXxxEvent().

[falken, 2014-07-02 05:42:12]

lgtm with some nits, please also write CL description

https://codereview.chromium.org/360123002/diff/60001/content/browser/service_...
File content/browser/service_worker/service_worker_info.cc (right):

https://codereview.chromium.org/360123002/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_info.cc:56: 
extra newlines

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

https://codereview.chromium.org/360123002/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_provider_host.cc:45:
ServiceWorkerVersion* version) {
Do we want DCHECK(CanAssociateVersion) here too? Otherwise it looks like you can
sneak in a bad active version, then CanAssociateVersion's if/else cascade over
active then waiting then installed becomes moot.

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

https://codereview.chromium.org/360123002/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_provider_host.h:86: // provide,
the method will reset that field to NULL.
"|version|"
"waiting, or active"
"provider"

https://codereview.chromium.org/360123002/diff/60001/content/browser/service_...
File content/browser/service_worker/service_worker_provider_host_unittest.cc
(right):

https://codereview.chromium.org/360123002/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_provider_host_unittest.cc:154:
class ServiceWorkerProviderHostWaitingVersionTest : public testing::Test {
Maybe this should now be called ServiceWorkerProviderHostAssociatedVersionTest?
Or just ServiceWorkerProviderHostVersionTest?

https://codereview.chromium.org/360123002/diff/60001/content/browser/service_...
File content/browser/service_worker/service_worker_register_job.cc (left):

https://codereview.chromium.org/360123002/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_register_job.cc:444: continue;
I wonder why this was here. Is it possible the provider host can have a dead
context if there was a db corruption recovery? I vaguely remember talking to
nhiroki about this function. +nhiroki

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

https://codereview.chromium.org/360123002/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_register_job.cc:420: // since
there's nothing new.
Now we have this comment twice.

https://codereview.chromium.org/360123002/diff/60001/content/browser/service_...
File content/browser/service_worker/service_worker_register_job.h (right):

https://codereview.chromium.org/360123002/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_register_job.h:121: // Associates
a waiting version to documents matched with a scope of the
"an installing version"

https://codereview.chromium.org/360123002/diff/60001/content/browser/service_...
File content/browser/service_worker/service_worker_registration.cc (right):

https://codereview.chromium.org/360123002/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_registration.cc:115: } else if
(waiting_version_  == version) {
extra space before the ==

https://codereview.chromium.org/360123002/diff/60001/content/browser/service_...
File content/browser/service_worker/service_worker_registration.h (right):

https://codereview.chromium.org/360123002/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_registration.h:74: // Sets the
corresposding version attribute and resets the position (if any)
"corresponding"

https://codereview.chromium.org/360123002/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_registration.h:82: // registation,
the method will reset that field to NULL, and notify
"waiting, or active"
"registration"

https://codereview.chromium.org/360123002/diff/60001/content/common/service_w...
File content/common/service_worker/service_worker_types.h (right):

https://codereview.chromium.org/360123002/diff/60001/content/common/service_w...
content/common/service_worker/service_worker_types.h:99: bool
intalling_changed() const { return !!(changed_ & INSTALLING_VERSION); }
"installing"

[nhiroki, 2014-07-02 07:25:12]

lgtm

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

https://codereview.chromium.org/360123002/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_provider_host.cc:91:
previous_version->RemoveWaitingControllee(this);
We are adding/removing 'this' as a waiting controllee. Is this correct?

https://codereview.chromium.org/360123002/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_provider_host.cc:96:
dispatcher_host_->Send(new ServiceWorkerMsg_SetWaitingServiceWorker(
nit: indent looks broken.

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

https://codereview.chromium.org/360123002/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_provider_host.h:56: //  Getters
for the navigator.serviceWorker attribute values.
nit: there is an extra space just after "//".

https://codereview.chromium.org/360123002/diff/60001/content/browser/service_...
File content/browser/service_worker/service_worker_register_job.cc (left):

https://codereview.chromium.org/360123002/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_register_job.cc:444: continue;
On 2014/07/02 05:42:11, falken wrote:
> I wonder why this was here. Is it possible the provider host can have a dead
> context if there was a db corruption recovery? I vaguely remember talking to
> nhiroki about this function. +nhiroki

Right. After the corruption recovery, there can be hosts referring to the dead
context, so we needed to check it before associating the version.

In michael's patch, CanAssociateVersion() checks it so this deletion should be
OK. (I like this way. Thank you for cleaning up :)

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

https://codereview.chromium.org/360123002/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_register_job.cc:233:
DCHECK(!registration()->waiting_version());
Can you check installing_version() instead of waiting_version()? (or just remove
this because we'll check it at line 267)

https://codereview.chromium.org/360123002/diff/60001/content/browser/service_...
File content/browser/service_worker/service_worker_registration.cc (right):

https://codereview.chromium.org/360123002/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_registration.cc:65:
ChangedVersionAttributesMask::ACTIVE_VERSION);
nit: indent

https://codereview.chromium.org/360123002/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_registration.cc:71:
ChangedVersionAttributesMask::WAITING_VERSION);
ditto.

https://codereview.chromium.org/360123002/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_registration.cc:77:
ChangedVersionAttributesMask::INSTALLING_VERSION);
ditto.

https://codereview.chromium.org/360123002/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_registration.cc:108: void
ServiceWorkerRegistration::UnsetVersionInternal(
nit: indent

https://codereview.chromium.org/360123002/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_registration.cc:124: 
nit: an extra empty line

[michaeln, 2014-07-02 23:17:14]

https://codereview.chromium.org/360123002/diff/60001/content/browser/service_...
File content/browser/service_worker/service_worker_info.cc (right):

https://codereview.chromium.org/360123002/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_info.cc:56: 
On 2014/07/02 05:42:11, falken wrote:
> extra newlines

Done.

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

https://codereview.chromium.org/360123002/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_provider_host.cc:45:
ServiceWorkerVersion* version) {
On 2014/07/02 05:42:11, falken wrote:
> Do we want DCHECK(CanAssociateVersion) here too? Otherwise it looks like you
can
> sneak in a bad active version, then CanAssociateVersion's if/else cascade over
> active then waiting then installed becomes moot.

Done.

https://codereview.chromium.org/360123002/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_provider_host.cc:91:
previous_version->RemoveWaitingControllee(this);
On 2014/07/02 07:25:12, nhiroki wrote:
> We are adding/removing 'this' as a waiting controllee. Is this correct?

ooops, i've generalized this to add/rmv PotentialControllee

https://codereview.chromium.org/360123002/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_provider_host.cc:96:
dispatcher_host_->Send(new ServiceWorkerMsg_SetWaitingServiceWorker(
On 2014/07/02 07:25:11, nhiroki wrote:
> nit: indent looks broken.

Done.

https://codereview.chromium.org/360123002/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_provider_host.cc:96:
dispatcher_host_->Send(new ServiceWorkerMsg_SetWaitingServiceWorker(
also added a comment about updating the msg once the renderer is ready to
receive a msg about SetInstalling

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

https://codereview.chromium.org/360123002/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_provider_host.h:56: //  Getters
for the navigator.serviceWorker attribute values.
On 2014/07/02 07:25:12, nhiroki wrote:
> nit: there is an extra space just after "//".

Done.

https://codereview.chromium.org/360123002/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_provider_host.h:86: // provide,
the method will reset that field to NULL.
On 2014/07/02 05:42:11, falken wrote:
> "|version|"
> "waiting, or active"
> "provider"

Done.

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

https://codereview.chromium.org/360123002/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_register_job.cc:233:
DCHECK(!registration()->waiting_version());
On 2014/07/02 07:25:12, nhiroki wrote:
> Can you check installing_version() instead of waiting_version()? (or just
remove
> this because we'll check it at line 267)

Done.

https://codereview.chromium.org/360123002/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_register_job.cc:420: // since
there's nothing new.
On 2014/07/02 05:42:11, falken wrote:
> Now we have this comment twice.

Done.

https://codereview.chromium.org/360123002/diff/60001/content/common/service_w...
File content/common/service_worker/service_worker_types.h (right):

https://codereview.chromium.org/360123002/diff/60001/content/common/service_w...
content/common/service_worker/service_worker_types.h:99: bool
intalling_changed() const { return !!(changed_ & INSTALLING_VERSION); }
On 2014/07/02 05:42:12, falken wrote:
> "installing"

Done.

[michaeln, 2014-07-02 23:33:27]

The CQ bit was checked by michaeln@chromium.org

[commit-bot: I haz the power, 2014-07-02 23:35:03]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/michaeln@chromium.org/360123002/80001

[commit-bot: I haz the power, 2014-07-03 02:48:25]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-07-03 06:29:13]

Change committed as 281189