service worker: Update job gets the script url when the job starts

service worker: Update job gets the script url when the job starts

Previously, the update job would use the script url provided when the
job was scheduled.

This change fixes the crash when the update job starts after the registration
has no versions, and also matches the spec.

BUG=604991
Committed: https://crrev.com/96eb96d3903c9326cc797502c2d685177497069f
Cr-Commit-Position: refs/heads/master@{#390320}

[falken, 2016-04-27 07:26:11]

falken@chromium.org changed reviewers:
+ nhiroki@chromium.org

[falken, 2016-04-27 07:26:12]

nhiroki can you review this?

[nhiroki, 2016-04-28 05:51:46]

LGTM

https://codereview.chromium.org/1928443003/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_job_unittest.cc (right):

https://codereview.chromium.org/1928443003/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_job_unittest.cc:1135:
TEST_F(ServiceWorkerJobTest, Update_NewestVersionChanged) {
Can you add a comment about what we want to check by this test like
Update_EvictedIncumbent?

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

https://codereview.chromium.org/1928443003/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_register_job.cc:53:
ServiceWorkerRegisterJob::ServiceWorkerRegisterJob(
Can you remove DCHECK(registration->GetNewestVersion()) from callers of this
constructor (ServiceWorkerJobCoordinator::Update)?

https://codereview.chromium.org/1928443003/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_register_job.cc:134:
DCHECK(job_type_ == REGISTRATION_JOB);
DCHECK_EQ(REGISTRATION_JOB, job_type_);

https://codereview.chromium.org/1928443003/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_register_job.cc:269: script_url_ =
registration()->GetNewestVersion()->script_url();
Can we have DCHECK(script_url_.is_empty()) immediately before this assignment?

[nhiroki, 2016-04-28 05:51:47]

LGTM

[falken, 2016-04-28 06:25:17]

https://codereview.chromium.org/1928443003/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_job_unittest.cc (right):

https://codereview.chromium.org/1928443003/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_job_unittest.cc:1135:
TEST_F(ServiceWorkerJobTest, Update_NewestVersionChanged) {
On 2016/04/28 05:51:46, nhiroki wrote:
> Can you add a comment about what we want to check by this test like
> Update_EvictedIncumbent?

Done.

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

https://codereview.chromium.org/1928443003/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_register_job.cc:53:
ServiceWorkerRegisterJob::ServiceWorkerRegisterJob(
On 2016/04/28 05:51:46, nhiroki wrote:
> Can you remove DCHECK(registration->GetNewestVersion()) from callers of this
> constructor (ServiceWorkerJobCoordinator::Update)?

Great catch. Done.

https://codereview.chromium.org/1928443003/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_register_job.cc:134:
DCHECK(job_type_ == REGISTRATION_JOB);
On 2016/04/28 05:51:46, nhiroki wrote:
> DCHECK_EQ(REGISTRATION_JOB, job_type_);

Done.

https://codereview.chromium.org/1928443003/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_register_job.cc:269: script_url_ =
registration()->GetNewestVersion()->script_url();
On 2016/04/28 05:51:46, nhiroki wrote:
> Can we have DCHECK(script_url_.is_empty()) immediately before this assignment?

Good idea. Done.

[falken, 2016-04-28 06:25:28]

The CQ bit was checked by falken@chromium.org

[falken, 2016-04-28 06:25:29]

The patchset sent to the CQ was uploaded after l-g-t-m from nhiroki@chromium.org
Link to the patchset: https://codereview.chromium.org/1928443003/#ps20001
(title: "review comments")

[commit-bot: I haz the power, 2016-04-28 06:25:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1928443003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1928443003/20001

[commit-bot: I haz the power, 2016-04-28 07:28:51]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-04-30 17:16:46]

Patchset 2 (id:??) landed as
https://crrev.com/96eb96d3903c9326cc797502c2d685177497069f
Cr-Commit-Position: refs/heads/master@{#390320}

[commit-bot: I haz the power, 2016-04-30 17:54:33]

Description was changed from

==========
service worker: Update job gets the script url when the job starts

Previously, the update job would use the script url provided when the
job was scheduled.

This change fixes the crash when the update job starts after the registration
has no versions, and also matches the spec.

BUG=604991
==========

to

==========
service worker: Update job gets the script url when the job starts

Previously, the update job would use the script url provided when the
job was scheduled.

This change fixes the crash when the update job starts after the registration
has no versions, and also matches the spec.

BUG=604991

Committed: https://crrev.com/96eb96d3903c9326cc797502c2d685177497069f
Cr-Commit-Position: refs/heads/master@{#390320}
==========