ServiceWorker: SWRegisterJob should manage status of SWVersion

ServiceWorker: SWRegisterJob should manage status of SWVersion

In the current implementation, ServiceWorkerVersion controls its status based
on event notifications (eg. ServiceWorkerVersion::OnInstallEventFinished).

But the spec algorithm requires to manage the status more flexibly during
registration process. For example, the version should be set to INSTALLED
status after associated to the documents because the page should be able to
see '.waiting' on 'onstatechange' event.

Because of this reason the register job should manage the status of the version.

(Spec) https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html


BUG=379012
TEST=content_unittest --gtest_filter=ServiceWorker* 
TEST=run_webkit_tests.py http/tests/serviceworker/waiting.html

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

[nhiroki, 2014-07-03 16:27:17]

Hi michaeln@ and falken@,

Can you give me your early feedback on this?

I'm working on '.installing'[1] (and '.active') support and that could need this
change (see my inline comment for more details).

[1] WIP patch: https://codereview.chromium.org/369013002/

Thanks!

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

https://codereview.chromium.org/369063004/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_register_job.cc:321:
new_version()->SetStatus(ServiceWorkerVersion::INSTALLED);
In the original implementation, SWVersion controls its status based on event
notifications (ie. SWVersion::OnInstallEventFinished,
SWVersion::OnActivateEventFinished etc).

But, in this case, the version should be set to INSTALLED after associated to
the documents (line 317-319) because the page should be able to see '.waiting'
on 'onstatechange' event triggered by SetStatus(INSTALLED). (cf.
"http/tests/serviceworker/waiting.html"). This is the reason why I think the job
should manage version's status instead of the version itself.

This may seem to cause wide range of changes (some tests assume the version
changes its status)... so I'd like to hear your opinion before I take this way.

[falken, 2014-07-04 02:19:54]

I think this change makes sense especially as the [[Register]] steps directly
include "Run the [[UpdateState]] algorithm..." steps. So having RegisterJob do
it directly makes it easier to follow the spec as written.

It also sounds consistent with what Michael described in
https://codereview.chromium.org/360123002/#msg4

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

https://codereview.chromium.org/369063004/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_register_job.cc:268: // "3. Set
registration.installingWorker to worker.".
nit: no need for trailing period

https://codereview.chromium.org/369063004/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_register_job.cc:320: 
nit: add: "12. Run the [[UpdateState]] algorithm..."

[nhiroki, 2014-07-04 02:47:18]

Thank you for your comments!

On 2014/07/04 02:19:54, falken wrote:
> I think this change makes sense especially as the [[Register]] steps directly
> include "Run the [[UpdateState]] algorithm..." steps. So having RegisterJob do
> it directly makes it easier to follow the spec as written.
> 
> It also sounds consistent with what Michael described in
> https://codereview.chromium.org/360123002/#msg4

michaeln@: Oh... sorry, I was completely missing your comment in that review. It
sounds reasonable to me. Please let me know if you've already been working
around the state transition. It's no problem at all to recall this CL :)

[nhiroki, 2014-07-04 02:59:18]

On 2014/07/04 02:47:18, nhiroki wrote:
> Thank you for your comments!
> 
> On 2014/07/04 02:19:54, falken wrote:
> > I think this change makes sense especially as the [[Register]] steps
directly
> > include "Run the [[UpdateState]] algorithm..." steps. So having RegisterJob
do
> > it directly makes it easier to follow the spec as written.
> > 
> > It also sounds consistent with what Michael described in
> > https://codereview.chromium.org/360123002/#msg4
> 
> michaeln@: Oh... sorry, I was completely missing your comment in that review.
It
> sounds reasonable to me. Please let me know if you've already been working
> around the state transition. It's no problem at all to recall this CL :)

(Supporting '.installing' and '.active' depends on the job cleanup, so I'm
thinking I should suspend my CLs until the cleanup is complete.)

[nhiroki, 2014-07-04 07:54:35]

On 2014/07/04 02:59:18, nhiroki wrote:
> On 2014/07/04 02:47:18, nhiroki wrote:
> > Thank you for your comments!
> > 
> > On 2014/07/04 02:19:54, falken wrote:
> > > I think this change makes sense especially as the [[Register]] steps
> directly
> > > include "Run the [[UpdateState]] algorithm..." steps. So having
RegisterJob
> do
> > > it directly makes it easier to follow the spec as written.
> > > 
> > > It also sounds consistent with what Michael described in
> > > https://codereview.chromium.org/360123002/#msg4
> > 
> > michaeln@: Oh... sorry, I was completely missing your comment in that
review.
> It
> > sounds reasonable to me. Please let me know if you've already been working
> > around the state transition. It's no problem at all to recall this CL :)
> 
> (Supporting '.installing' and '.active' depends on the job cleanup, so I'm
> thinking I should suspend my CLs until the cleanup is complete.)

Chatted with falken@ and dominicc@ locally, and we found that...

* this CL is blocking ".active/.installing" that I'm working on.
* ".active" is blocking ".ready" that dominicc@ is working on.

To drive the development, please let me proceed this CL for now...

[nhiroki, 2014-07-04 07:56:51]

Removed "WIP" mark. Can you take another look? Thanks!

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

https://codereview.chromium.org/369063004/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_register_job.cc:268: // "3. Set
registration.installingWorker to worker.".
On 2014/07/04 02:19:53, falken wrote:
> nit: no need for trailing period

Done.

https://codereview.chromium.org/369063004/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_register_job.cc:320: 
On 2014/07/04 02:19:53, falken wrote:
> nit: add: "12. Run the [[UpdateState]] algorithm..."

Done.

[falken, 2014-07-04 09:30:50]

lgtm with nits

It would be nice to revise the CL description to include the reasoning for the
change, as you've already done in the codereview comments.

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

https://codereview.chromium.org/369063004/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_register_job.cc:314: // "9. If
registration.waitingWorker is not null, then:..."
it looks like we don't actually do this step. Make it a TODO?

https://codereview.chromium.org/369063004/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_register_job.cc:316: // "11. Set
registration.installing_version to null."
"installingWorker"

https://codereview.chromium.org/369063004/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_register_job.cc:344: // in
ServiceWorkerVersion::RemoveControllee).
You can remove the (see comments...) part, those comments are now gone.

https://codereview.chromium.org/369063004/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_register_job.cc:479:
!it->IsAtEnd();  it->Advance()) {
extra spaces

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

https://codereview.chromium.org/369063004/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_version_unittest.cc:277: //
Version's status must not be changed while installing process.
nit: "not have changed during installation"

https://codereview.chromium.org/369063004/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_version_unittest.cc:297: //
Version's status must not be changed while activating process.
nit: "not have changed during activation"

[nhiroki, 2014-07-04 10:37:26]

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

https://codereview.chromium.org/369063004/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_register_job.cc:314: // "9. If
registration.waitingWorker is not null, then:..."
On 2014/07/04 09:30:49, falken wrote:
> it looks like we don't actually do this step. Make it a TODO?

Done.

https://codereview.chromium.org/369063004/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_register_job.cc:316: // "11. Set
registration.installing_version to null."
On 2014/07/04 09:30:49, falken wrote:
> "installingWorker"

Done.

https://codereview.chromium.org/369063004/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_register_job.cc:344: // in
ServiceWorkerVersion::RemoveControllee).
On 2014/07/04 09:30:49, falken wrote:
> You can remove the (see comments...) part, those comments are now gone.

Done.

https://codereview.chromium.org/369063004/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_register_job.cc:479:
!it->IsAtEnd();  it->Advance()) {
On 2014/07/04 09:30:49, falken wrote:
> extra spaces

Done.

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

https://codereview.chromium.org/369063004/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_version_unittest.cc:277: //
Version's status must not be changed while installing process.
On 2014/07/04 09:30:49, falken wrote:
> nit: "not have changed during installation"

Done.

https://codereview.chromium.org/369063004/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_version_unittest.cc:297: //
Version's status must not be changed while activating process.
On 2014/07/04 09:30:49, falken wrote:
> nit: "not have changed during activation"

Done.

[nhiroki, 2014-07-07 04:26:16]

Hmm... ServiceWorkerVersionBrowserTests are failing... I'm now investigating
them.

[nhiroki, 2014-07-07 06:29:25]

On 2014/07/07 04:26:16, nhiroki wrote:
> Hmm... ServiceWorkerVersionBrowserTests are failing... I'm now investigating
> them.

Fixed. falken@, can you take another look? Thanks!

[falken, 2014-07-07 07:05:16]

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

https://codereview.chromium.org/369063004/diff/80001/content/browser/service_...
content/browser/service_worker/service_worker_context_request_handler.cc:79:
version_->status() != ServiceWorkerVersion::INSTALLING) {
I'm curious whether this can be more restrictive? Do we expect to get here when
the version is status NEW?

[nhiroki, 2014-07-07 08:26:41]

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

https://codereview.chromium.org/369063004/diff/80001/content/browser/service_...
content/browser/service_worker/service_worker_context_request_handler.cc:79:
version_->status() != ServiceWorkerVersion::INSTALLING) {
On 2014/07/07 07:05:16, falken wrote:
> I'm curious whether this can be more restrictive? Do we expect to get here
when
> the version is status NEW?

I think the both are necessary. Normally the version's status is NEW here, but
it can be INSTALLING if the worker hasn't been running on
ServiceWorkerVersion::DispatchInstallEvent() (see my inline comment there).

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

https://codereview.chromium.org/369063004/diff/80001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:244: // Schedule
calling this method after starting the worker.
As I mentioned above, this code path will reach
ServiceWorkerContextRequestHandler::ShouldAddToScriptCache() with INSTALLING
status.

[falken, 2014-07-07 08:36:44]

LGTM

[michaeln, 2014-07-07 21:56:37]

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

https://codereview.chromium.org/369063004/diff/80001/content/browser/service_...
content/browser/service_worker/service_worker_register_job.cc:292:
registration()->SetInstallingVersion(NULL);
the call to SetInstallingVersion is not needed here since the Complete method
calls reg->UnsetVersion in all error cases

can the call to SetStatus(Redundant) can be handled in Complete too?

https://codereview.chromium.org/369063004/diff/80001/content/browser/service_...
content/browser/service_worker/service_worker_register_job.cc:309:
registration()->SetInstallingVersion(NULL);
ditto

https://codereview.chromium.org/369063004/diff/80001/content/browser/service_...
content/browser/service_worker/service_worker_register_job.cc:372:
registration()->SetActiveVersion(NULL);
ditto

https://codereview.chromium.org/369063004/diff/80001/content/browser/service_...
content/browser/service_worker/service_worker_register_job.cc:396: if
(new_version()) {
right here
new_version()->SetStatus(ServiceWorkerVersion::REDUNDANT);

https://codereview.chromium.org/369063004/diff/80001/content/browser/service_...
File content/browser/service_worker/service_worker_version_unittest.cc (left):

https://codereview.chromium.org/369063004/diff/80001/content/browser/service_...
content/browser/service_worker/service_worker_version_unittest.cc:306:
TEST_F(ServiceWorkerVersionTest, RepeatedlyObserveStatusChanges) {
why remove this test?

this test was added to verify a real bug was fixed (and stays fixed), does some
other test cover this case now? if not, can you update the test to continue to
function?

[nhiroki, 2014-07-07 23:07:41]

Thank you for reviewing. Updated.

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

https://codereview.chromium.org/369063004/diff/80001/content/browser/service_...
content/browser/service_worker/service_worker_register_job.cc:292:
registration()->SetInstallingVersion(NULL);
On 2014/07/07 21:56:37, michaeln wrote:
> the call to SetInstallingVersion is not needed here since the Complete method
> calls reg->UnsetVersion in all error cases
> 
> can the call to SetStatus(Redundant) can be handled in Complete too?

Done.

https://codereview.chromium.org/369063004/diff/80001/content/browser/service_...
content/browser/service_worker/service_worker_register_job.cc:309:
registration()->SetInstallingVersion(NULL);
On 2014/07/07 21:56:37, michaeln wrote:
> ditto

Done.

https://codereview.chromium.org/369063004/diff/80001/content/browser/service_...
content/browser/service_worker/service_worker_register_job.cc:372:
registration()->SetActiveVersion(NULL);
On 2014/07/07 21:56:37, michaeln wrote:
> ditto

Done.

https://codereview.chromium.org/369063004/diff/80001/content/browser/service_...
content/browser/service_worker/service_worker_register_job.cc:396: if
(new_version()) {
On 2014/07/07 21:56:37, michaeln wrote:
> right here
> new_version()->SetStatus(ServiceWorkerVersion::REDUNDANT);

Done.

https://codereview.chromium.org/369063004/diff/80001/content/browser/service_...
File content/browser/service_worker/service_worker_version_unittest.cc (left):

https://codereview.chromium.org/369063004/diff/80001/content/browser/service_...
content/browser/service_worker/service_worker_version_unittest.cc:306:
TEST_F(ServiceWorkerVersionTest, RepeatedlyObserveStatusChanges) {
On 2014/07/07 21:56:37, michaeln wrote:
> why remove this test?
> 
> this test was added to verify a real bug was fixed (and stays fixed), does
some
> other test cover this case now? if not, can you update the test to continue to
> function?

Revived and updated this test.

Let me confirm my understanding... this test intends to make sure that status
change callbacks are correctly invoked on each status transition, is this right?

Apparently I misunderstood the purpose of the test. At first (as of PatchSet 4),
I thought this checks the status transitions by a sequence of event dispatch
functions. After this CL, those dispatch functions never change the status and
the caller of them changes it instead, so I assumed this test doesn't make sense
any more.

[michaeln, 2014-07-07 23:45:16]

thnx, lgtm

https://codereview.chromium.org/369063004/diff/120001/content/browser/service...
File content/browser/service_worker/service_worker_version_unittest.cc (right):

https://codereview.chromium.org/369063004/diff/120001/content/browser/service...
content/browser/service_worker/service_worker_version_unittest.cc:77:
version->RegisterStatusChangeCallback(
The test ensures that a callback that re-registers itself works as expected.

[nhiroki, 2014-07-08 00:42:15]

The CQ bit was checked by nhiroki@chromium.org

[commit-bot: I haz the power, 2014-07-08 00:44:25]

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

[commit-bot: I haz the power, 2014-07-08 04:48:54]

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

[commit-bot: I haz the power, 2014-07-08 06:10:16]

Change committed as 281675