Add ServiceWorkerVersion::status() (which is to be persisted unlike running status)

Add ServiceWorkerVersion::status() (which is to be persisted unlike running status)

- Changes ServiceWorkerVersion::status() to ServiceWorkerVersion::running_status()
- Adds ServiceWorkerVersion::status()
- Also changes who to assign new version ID (from registration to storage)

BUG=285976
TEST=existing tests

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

[kinuko, 2014-03-06 06:12:38]

I'm gradually making browser modules more storage aware (so that it becomes
closer to real design), and I *think* we need to make this change. Can you
review?

(Alec, this will break your internals code, sorry for that)

[alecflett, 2014-03-06 19:10:24]

lgtm

I'm not sure how the new status is persisted (would we really bother to preserve
a service worker that hadn't finished installing) but I do think we need this
status..

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

https://codereview.chromium.org/188283003/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_register_job.cc:129: // the
install/activate event is dispatched.
I don't think adding "install/" is right - we want it after the install event,
but before the activate event.

[commit-bot: I haz the power, 2014-03-06 19:16:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kinuko@chromium.org/188283003/1

[commit-bot: I haz the power, 2014-03-06 19:39:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-06 19:39:52]

Retried try job too often on win_rel for step(s) app_list_unittests,
ash_unittests, aura_unittests, cacheinvalidation_unittests, cc_unittests,
check_deps, check_deps2git, chrome_elf_unittests, chromedriver_unittests,
components_unittests, compositor_unittests, content_browsertests,
content_unittests, crypto_unittests, events_unittests, gfx_unittests,
google_apis_unittests, gpu_unittests, installer_util_unittests, ipc_tests,
jingle_unittests, media_unittests, mini_installer_test, nacl_integration,
ppapi_unittests, printing_unittests, remoting_unittests, sql_unittests,
sync_integration_tests, sync_unit_tests, telemetry_perf_unittests,
telemetry_unittests, views_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[alecflett, 2014-03-06 19:41:57]

The CQ bit was checked by alecflett@chromium.org

[commit-bot: I haz the power, 2014-03-06 19:47:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kinuko@chromium.org/188283003/1

[michaeln, 2014-03-06 19:49:54]

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

https://codereview.chromium.org/188283003/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_register_job.cc:129: // the
install/activate event is dispatched.
On 2014/03/06 19:10:24, alecflett wrote:
> I don't think adding "install/" is right - we want it after the install event,
> but before the activate event.

Regarding setting before, what if activate fails?

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

https://codereview.chromium.org/188283003/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_version.h:86: ACTIVE,   //
Activation is finished and can run as active.
Is there a state following ACTIVE to represent the period after a new version
has been activated. DEACTIVATED?

Like NEW, we probably won't want to persist that state value.

[michaeln, 2014-03-06 19:50:06]

The CQ bit was unchecked by michaeln@chromium.org

[alecflett, 2014-03-06 21:13:08]

On 2014/03/06 19:49:54, michaeln wrote:
>
https://codereview.chromium.org/188283003/diff/1/content/browser/service_work...
> File content/browser/service_worker/service_worker_register_job.cc (right):
> 
>
https://codereview.chromium.org/188283003/diff/1/content/browser/service_work...
> content/browser/service_worker/service_worker_register_job.cc:129: // the
> install/activate event is dispatched.
> On 2014/03/06 19:10:24, alecflett wrote:
> > I don't think adding "install/" is right - we want it after the install
event,
> > but before the activate event.
> 
> Regarding setting before, what if activate fails?

Activate can't fail. Once you're active, you're running. the activate event is
more of a "FYI you're now active" not "activation is in process.."

> 
>
https://codereview.chromium.org/188283003/diff/1/content/browser/service_work...
> File content/browser/service_worker/service_worker_version.h (right):
> 
>
https://codereview.chromium.org/188283003/diff/1/content/browser/service_work...
> content/browser/service_worker/service_worker_version.h:86: ACTIVE,   //
> Activation is finished and can run as active.
> Is there a state following ACTIVE to represent the period after a new version
> has been activated. DEACTIVATED?
> 
> Like NEW, we probably won't want to persist that state value.

I would imagine that DEACTIVATED is a state that couldn't be observed unless
teardown is more than one asynchronous event. (I can't think of why it would be)

[michaeln, 2014-03-06 22:37:57]

> > Regarding setting before, what if activate fails?
> 
> Activate can't fail. Once you're active, you're running. the activate event is
> more of a "FYI you're now active" not "activation is in process.."

There's a question about this in the algorithms up on github. As described
there, the sw and open pages related to the registration have an opportunity to
defer finishing activation via .waitUntil(p). What if it completes by way of
promise rejection?

> > Like NEW, we probably won't want to persist that state value.
> 
> I would imagine that DEACTIVATED is a state that couldn't be observed unless
> teardown is more than one asynchronous event. (I can't think of why it would
be)

The algorithms describe the sending of a deactivate event in its description of
Unregister which is slightly different then the REPLACED state a swap out
version gets left in here. Those states are not terribly observable by script
clients, but they are to the inner workings of our impl. If we're going to have
states, I'd vote to flesh them out (just like we did for the RunningStates). I
think like DEACTIVATED and REPLACED could be added to this set.

[michaeln, 2014-03-06 22:51:40]

On 2014/03/06 22:37:57, michaeln wrote:
> > > Regarding setting before, what if activate fails?
> > 
> > Activate can't fail. Once you're active, you're running. the activate event
is
> > more of a "FYI you're now active" not "activation is in process.."
> 
> There's a question about this in the algorithms up on github. As described
> there, the sw and open pages related to the registration have an opportunity
to
> defer finishing activation via .waitUntil(p). What if it completes by way of
> promise rejection?

Actually, if the algorithms described on github that allow deferring activation
are correct, we'll probably need an ACTIVATING state to represent when that's
happening.

> 
> > > Like NEW, we probably won't want to persist that state value.
> > 
> > I would imagine that DEACTIVATED is a state that couldn't be observed unless
> > teardown is more than one asynchronous event. (I can't think of why it would
> be)
> 
> The algorithms describe the sending of a deactivate event in its description
of
> Unregister which is slightly different then the REPLACED state a swap out
> version gets left in here. Those states are not terribly observable by script
> clients, but they are to the inner workings of our impl. If we're going to
have
> states, I'd vote to flesh them out (just like we did for the RunningStates). I
> think like DEACTIVATED and REPLACED could be added to this set.

[michaeln, 2014-03-06 23:01:03]

Hmmm...

NEW - set this way in ctor
INSTALLING - install event is in flight
INSTALLED - install event is done (including resolution of waitUntils)
ACTIVATING - activating event is in flight
ACTIVE - activation is done  (including resolution of waitUntils)
OLD - covers all terminating cases

Probably the only ones that get persisted to disk are INSTALLED and ACTIVE.

wdyt?

[kinuko, 2014-03-07 00:43:03]

On 2014/03/06 23:01:03, michaeln wrote:
> Hmmm...
> 
> NEW - set this way in ctor
> INSTALLING - install event is in flight
> INSTALLED - install event is done (including resolution of waitUntils)
> ACTIVATING - activating event is in flight
> ACTIVE - activation is done  (including resolution of waitUntils)
> OLD - covers all terminating cases
>
> Probably the only ones that get persisted to disk are INSTALLED and ACTIVE.
> 
> wdyt?

Whether if we need NEW/INSTALLING or not (and if we want to persist
non-installed version on storage or not) was/is not very clear to me and I had
to repeatedly add and remove the status while I was writing my local design
note...

but if we think about script caching (which will need to be paired with version
ID) I thought it might be easier to persist NEW version info too, otherwise we
could end up having script cache entries for a version that doesn't exist in DB
yet, and we may need some GC tasks to purge such entries.  That was my
reasoning, but I might be making wrong assumptions somewhere. Wdyt?

[michaeln, 2014-03-07 00:55:58]

> Probably the only ones that get persisted to disk are INSTALLED and ACTIVE.
> ...
> could end up having script cache entries for a version that doesn't exist in
DB

Having newly written-to-disk cached responses be associated with a NEW version
in the db could help with that. But there could be other ways to account for
that too like a seperate 'table' of unassociated response and when something
transitions to INSTALLED (and a record is written) all its cached resource get
associated to that INSTALLED version in the db.

For now, I'm more interested in nailing down the logical states than in figuring
out the persistence of those states.

[kinuko, 2014-03-07 00:59:31]

On 2014/03/07 00:55:58, michaeln wrote:
> > Probably the only ones that get persisted to disk are INSTALLED and ACTIVE.
> > ...
> > could end up having script cache entries for a version that doesn't exist in
> DB
> 
> Having newly written-to-disk cached responses be associated with a NEW version
> in the db could help with that. But there could be other ways to account for
> that too like a seperate 'table' of unassociated response and when something
> transitions to INSTALLED (and a record is written) all its cached resource get
> associated to that INSTALLED version in the db.
> 
> For now, I'm more interested in nailing down the logical states than in
figuring
> out the persistence of those states.

I agree with this (I just replied to the last line in the last comment).

Since now we have statuses listed in the new algorithms page I can align this
patch with it and reupload (I thought I should probably need to do so when I
found the list before I go bed last night...) does that make sense?

[kinuko, 2014-03-07 01:03:51]

On 2014/03/07 00:59:31, kinuko wrote:
> On 2014/03/07 00:55:58, michaeln wrote:
> > > Probably the only ones that get persisted to disk are INSTALLED and
ACTIVE.
> > > ...
> > > could end up having script cache entries for a version that doesn't exist
in
> > DB
> > 
> > Having newly written-to-disk cached responses be associated with a NEW
version
> > in the db could help with that. But there could be other ways to account for
> > that too like a seperate 'table' of unassociated response and when something
> > transitions to INSTALLED (and a record is written) all its cached resource
get
> > associated to that INSTALLED version in the db.
> > 
> > For now, I'm more interested in nailing down the logical states than in
> figuring
> > out the persistence of those states.
> 
> I agree with this (I just replied to the last line in the last comment).
> 
> Since now we have statuses listed in the new algorithms page I can align this
> patch with it and reupload (I thought I should probably need to do so when I
> found the list before I go bed last night...) does that make sense?

(Have read more comments.)  the status list in #13 is what you think we'll need
(which has slightly different from the github one), right? Let me think about
it.

[michaeln, 2014-03-07 01:49:49]

> (Have read more comments.)  the status list in #13 is what you think we'll
need
> (which has slightly different from the github one), right? Let me think about
> it.

Yes, thats what it looks like to me. 

NEW - set this way in ctor
INSTALLING - install event is in flight
INSTALLED - install event is done (including resolution of waitUntils)
ACTIVATING - activating event is in flight
ACTIVE - activation is done  (including resolution of waitUntils)
OLD - covers all terminating cases

+1 Using the similar terminology as is used on spec language for similar
concepts.

[kinuko, 2014-03-10 10:36:55]

I ended up adding 
NEW, INSTALLING, INSTALLED, ACTIVATING, ACTIVE and DEACTIVATED.

The 'deactivated' status may need to be refined (and we may need another status
for REPLACED etc as discussed), for now I basically used the same terms from
algorithms/spec on github (but with TODO).  PTAL?

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

https://codereview.chromium.org/188283003/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_register_job.cc:129: // the
install/activate event is dispatched.
On 2014/03/06 19:49:54, michaeln wrote:
> On 2014/03/06 19:10:24, alecflett wrote:
> > I don't think adding "install/" is right - we want it after the install
event,
> > but before the activate event.
> 
> Regarding setting before, what if activate fails?

I overlooked the 'just before' phrasing, just wanted to note that we need to
dispatch both install and activate events before actually making it active.

[michaeln, 2014-03-10 18:53:44]

lgtm

https://codereview.chromium.org/188283003/diff/80001/content/browser/service_...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/188283003/diff/80001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:207: if (status() !=
NEW) {
I wonder if we should [D]CHECK these instead of having a branch for it?

[kinuko, 2014-03-11 06:58:34]

https://codereview.chromium.org/188283003/diff/80001/content/browser/service_...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/188283003/diff/80001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:207: if (status() !=
NEW) {
On 2014/03/10 18:53:45, michaeln wrote:
> I wonder if we should [D]CHECK these instead of having a branch for it?

Let me land this as is (branch + NOTREACHED) for now, I'm not really determined 
how to handle error cases for these yet.

[kinuko, 2014-03-11 06:58:39]

The CQ bit was checked by kinuko@chromium.org

[commit-bot: I haz the power, 2014-03-11 07:02:45]

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

[kinuko, 2014-03-11 07:33:11]

The CQ bit was checked by kinuko@chromium.org

[commit-bot: I haz the power, 2014-03-11 07:35:26]

Failed to trigger a try job on linux_rel
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2014-03-11 07:35:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kinuko@chromium.org/188283003/100001

[kinuko, 2014-03-11 07:40:24]

The CQ bit was unchecked by kinuko@chromium.org

[kinuko, 2014-03-11 07:49:53]

The CQ bit was checked by kinuko@chromium.org

[commit-bot: I haz the power, 2014-03-11 07:50:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kinuko@chromium.org/188283003/120001

[commit-bot: I haz the power, 2014-03-11 11:26:17]

Change committed as 256200