Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(42)

Issue 1381153004: Service Worker: Change the criteria for bumping the last update check time (Closed)

Created:
5 years, 2 months ago by jungkees
Modified:
4 years, 11 months ago
Reviewers:
falken, michaeln, zino, nhiroki
CC:
chromium-reviews
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Service Worker: Change the criteria for bumping the last update check time As per the latest spec change, when an update successfully loaded a script resource from the network (having bypassed the HTTP cache), the registration's last update check time should be updated regardless of whether the installation of the fetched resource succeeds or fails. While the existing code updates the time stamp only when the subsequent installation of the newly received script is successfully completed (or the loaded script is byte-for-byte match to the incumbent script resource), this CL makes it bump the time stamp as soon as the update succeeded. This CL added a condition for checking whether the script resource was served from the network: if (net_request_->response_info().network_accessed && !(net_request_->response_info().was_cached)) // This line is added. Spec discussion: https://github.com/slightlyoff/ServiceWorker/issues/514#issuecomment-135641718 BUG=539709 Committed: https://crrev.com/2c7aaaf09eab313a07c425c474256f92957da931 Cr-Commit-Position: refs/heads/master@{#371485}

Patch Set 1 #

Total comments: 6

Patch Set 2 : Avoid reading disk for first time registrations. #

Total comments: 6

Patch Set 3 : Update the unittest. #

Total comments: 4

Patch Set 4 : Rename a member variable; update the comment. #

Total comments: 5

Patch Set 5 : Rebase and subclassing UpdateJobTestHelper for the fail-to-start-worker case #

Patch Set 6 : Remove a comment #

Total comments: 14

Patch Set 7 : Rebase; Update the unit test #

Total comments: 2

Patch Set 8 : Change the condition to check if SWRegistration is stored #

Total comments: 10

Patch Set 9 : Assure |last_update_check| is set a value #

Total comments: 4

Patch Set 10 : Move dcheck #

Patch Set 11 : Rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+85 lines, -25 lines) Patch
M content/browser/service_worker/embedded_worker_instance.h View 1 2 3 4 5 6 7 8 1 chunk +4 lines, -0 lines 0 comments Download
M content/browser/service_worker/embedded_worker_test_helper.h View 1 2 3 4 5 6 7 8 9 10 2 chunks +7 lines, -1 line 0 comments Download
M content/browser/service_worker/embedded_worker_test_helper.cc View 1 2 3 4 5 6 7 8 9 10 2 chunks +6 lines, -5 lines 0 comments Download
M content/browser/service_worker/service_worker_job_unittest.cc View 1 2 3 4 5 6 7 8 6 chunks +50 lines, -7 lines 0 comments Download
M content/browser/service_worker/service_worker_register_job.cc View 1 2 3 4 5 6 7 8 9 3 chunks +15 lines, -11 lines 0 comments Download
M content/browser/service_worker/service_worker_write_to_cache_job.cc View 1 2 3 4 5 6 7 8 9 1 chunk +3 lines, -1 line 0 comments Download

Messages

Total messages: 61 (13 generated)
jungkees
This CL is split out from https://codereview.chromium.org/1283273002/ to cover the timestamp behavior change. PTAL I ...
5 years, 2 months ago (2015-10-06 02:02:26 UTC) #3
jungkees
This CL conforms to the result of the f2f discussion in TPAC: https://github.com/slightlyoff/ServiceWorker/issues/514#issuecomment-151333617. PTAL
5 years, 1 month ago (2015-11-04 06:22:44 UTC) #4
michaeln
It makes sense to update the time when there's an application error during installation event ...
5 years, 1 month ago (2015-11-09 23:19:10 UTC) #5
jungkees
On 2015/11/09 23:19:10, michaeln wrote: > It makes sense to update the time when there's ...
5 years, 1 month ago (2015-11-10 02:43:16 UTC) #6
jungkees
Setting |last_update_time| for the first time registration seems fine. Checking/using the value for it seems ...
5 years, 1 month ago (2015-11-10 02:45:17 UTC) #7
michaeln
> I'm not sure if I missed your point here. If so, please let me ...
5 years, 1 month ago (2015-11-10 21:17:36 UTC) #8
jungkees
> - swaccessed causing an update check > - new version is downloaded bypassing http ...
5 years, 1 month ago (2015-11-11 08:10:05 UTC) #9
michaeln
> The desired behavior and the intent of the spec is to NOT bypass caches ...
5 years, 1 month ago (2015-11-11 20:57:03 UTC) #10
michaeln
> https://github.com/slightlyoff/ServiceWorker/issues/514#issuecomment-130664373. > > I'm not sure what you mean by the update succeeded? The ...
5 years, 1 month ago (2015-11-11 21:23:30 UTC) #11
jungkees
> I think I see, validating the network cache == update success. Right. That's clear.
5 years, 1 month ago (2015-11-12 11:25:25 UTC) #12
falken
Michael, it seems like your concerns are mostly resolved? If so, this patch seems mostly ...
5 years ago (2015-12-02 09:27:39 UTC) #13
michaeln
On 2015/12/02 09:27:39, falken (away back on Dec 4) wrote: > Michael, it seems like ...
5 years ago (2015-12-04 00:37:13 UTC) #14
falken
A comment or even better a unit test for that side effect would be good.
5 years ago (2015-12-04 02:18:10 UTC) #15
jungkees
A question about checking whether a registration is stored in the database. https://codereview.chromium.org/1381153004/diff/1/content/browser/service_worker/service_worker_register_job.cc File content/browser/service_worker/service_worker_register_job.cc ...
5 years ago (2015-12-04 11:46:38 UTC) #16
falken
https://codereview.chromium.org/1381153004/diff/1/content/browser/service_worker/service_worker_register_job.cc File content/browser/service_worker/service_worker_register_job.cc (right): https://codereview.chromium.org/1381153004/diff/1/content/browser/service_worker/service_worker_register_job.cc#newcode347 content/browser/service_worker/service_worker_register_job.cc:347: context_->storage()->UpdateLastUpdateCheckTime(registration()); On 2015/12/04 11:46:38, jungkees wrote: > Having inspected ...
5 years ago (2015-12-07 03:53:34 UTC) #17
jungkees
Avoided first time registrations write to disk. > Jungkee, service_worker_job_unittest was not uploaded. I'm again ...
5 years ago (2015-12-07 07:02:09 UTC) #18
falken
I see the uploaded unittest file, but it's just removing the existing unit test. Can ...
5 years ago (2015-12-07 07:40:33 UTC) #19
falken
I see now: "I removed TEST_F(ServiceWorkerJobTest, Update_BumpLastUpdateCheckTime) for now. Could you give me a comment ...
5 years ago (2015-12-07 07:47:41 UTC) #20
jungkees
Just uploaded a new snapshot having addressed the comments and updated the unit test. PTAL. ...
5 years ago (2015-12-15 13:07:46 UTC) #21
jungkees
service_worker_job_unittest was uploaded, but side-by-side diffs doesn't show again. I again encountered a 403 error ...
5 years ago (2015-12-15 13:11:00 UTC) #22
falken
I think this is looking good. https://codereview.chromium.org/1381153004/diff/40001/content/browser/service_worker/service_worker_job_unittest.cc File content/browser/service_worker/service_worker_job_unittest.cc (right): https://codereview.chromium.org/1381153004/diff/40001/content/browser/service_worker/service_worker_job_unittest.cc#newcode805 content/browser/service_worker/service_worker_job_unittest.cc:805: A bit ambiguous ...
5 years ago (2015-12-16 08:27:48 UTC) #23
jungkees
Uploaded a new snapshot. PTAL. Thanks for review! https://codereview.chromium.org/1381153004/diff/40001/content/browser/service_worker/service_worker_register_job.cc File content/browser/service_worker/service_worker_register_job.cc (right): https://codereview.chromium.org/1381153004/diff/40001/content/browser/service_worker/service_worker_register_job.cc#newcode356 content/browser/service_worker/service_worker_register_job.cc:356: // ...
5 years ago (2015-12-16 08:59:20 UTC) #24
falken
Sorry for the late review, I was off today. https://codereview.chromium.org/1381153004/diff/60001/content/browser/service_worker/embedded_worker_test_helper.h File content/browser/service_worker/embedded_worker_test_helper.h (right): https://codereview.chromium.org/1381153004/diff/60001/content/browser/service_worker/embedded_worker_test_helper.h#newcode102 content/browser/service_worker/embedded_worker_test_helper.h:102: ...
5 years ago (2015-12-17 13:23:43 UTC) #25
jungkees
Sorry for the late response. I just uploaded a new snapshot having rebased and addressed ...
5 years ago (2015-12-23 03:08:58 UTC) #27
falken
https://codereview.chromium.org/1381153004/diff/120001/content/browser/service_worker/embedded_worker_test_helper.h File content/browser/service_worker/embedded_worker_test_helper.h (right): https://codereview.chromium.org/1381153004/diff/120001/content/browser/service_worker/embedded_worker_test_helper.h#newcode78 content/browser/service_worker/embedded_worker_test_helper.h:78: } How about a GetNextThreadId() instead that takes care ...
4 years, 11 months ago (2016-01-05 04:21:16 UTC) #28
jungkees
I uploaded a new snapshot having addressed the comments. PTAL. https://codereview.chromium.org/1381153004/diff/120001/content/browser/service_worker/embedded_worker_test_helper.h File content/browser/service_worker/embedded_worker_test_helper.h (right): https://codereview.chromium.org/1381153004/diff/120001/content/browser/service_worker/embedded_worker_test_helper.h#newcode78 ...
4 years, 11 months ago (2016-01-06 02:57:55 UTC) #29
falken
https://codereview.chromium.org/1381153004/diff/140001/content/browser/service_worker/service_worker_register_job.cc File content/browser/service_worker/service_worker_register_job.cc (right): https://codereview.chromium.org/1381153004/diff/140001/content/browser/service_worker/service_worker_register_job.cc#newcode364 content/browser/service_worker/service_worker_register_job.cc:364: if (job_type_ == UPDATE_JOB) Ick, I made a mistake. ...
4 years, 11 months ago (2016-01-06 05:22:08 UTC) #30
jungkees
Addressed the comment on changing the condition to check if the SWRegistration is stored. https://codereview.chromium.org/1381153004/diff/140001/content/browser/service_worker/service_worker_register_job.cc ...
4 years, 11 months ago (2016-01-06 05:45:29 UTC) #31
jungkees
falken@, I think this CL is ready. Could you check it?
4 years, 11 months ago (2016-01-19 03:48:15 UTC) #32
michaeln
driveby comments/questions https://codereview.chromium.org/1381153004/diff/160001/content/browser/service_worker/service_worker_register_job.cc File content/browser/service_worker/service_worker_register_job.cc (left): https://codereview.chromium.org/1381153004/diff/160001/content/browser/service_worker/service_worker_register_job.cc#oldcode436 content/browser/service_worker/service_worker_register_job.cc:436: registration()->set_last_update_check(base::Time::Now()); Is there any chance we can ...
4 years, 11 months ago (2016-01-19 19:20:28 UTC) #33
jungkees
I updated the comments. Thanks for review. https://codereview.chromium.org/1381153004/diff/160001/content/browser/service_worker/service_worker_register_job.cc File content/browser/service_worker/service_worker_register_job.cc (left): https://codereview.chromium.org/1381153004/diff/160001/content/browser/service_worker/service_worker_register_job.cc#oldcode436 content/browser/service_worker/service_worker_register_job.cc:436: registration()->set_last_update_check(base::Time::Now()); On ...
4 years, 11 months ago (2016-01-20 05:38:11 UTC) #34
falken
I think this looks good unless Michael has objections. But when run locally (I wanted ...
4 years, 11 months ago (2016-01-20 09:31:54 UTC) #35
michaeln
https://codereview.chromium.org/1381153004/diff/160001/content/browser/service_worker/service_worker_register_job.cc File content/browser/service_worker/service_worker_register_job.cc (left): https://codereview.chromium.org/1381153004/diff/160001/content/browser/service_worker/service_worker_register_job.cc#oldcode436 content/browser/service_worker/service_worker_register_job.cc:436: registration()->set_last_update_check(base::Time::Now()); > This asserts you couldn't get to OnInstallFinished ...
4 years, 11 months ago (2016-01-21 20:55:31 UTC) #36
jungkees
I uploaded a new snapshot having addressed those comments. PTAL. Thanks. falken@, > I think ...
4 years, 11 months ago (2016-01-22 05:22:27 UTC) #37
michaeln
https://codereview.chromium.org/1381153004/diff/180001/content/browser/service_worker/service_worker_register_job.cc File content/browser/service_worker/service_worker_register_job.cc (right): https://codereview.chromium.org/1381153004/diff/180001/content/browser/service_worker/service_worker_register_job.cc#newcode368 content/browser/service_worker/service_worker_register_job.cc:368: DCHECK(!registration()->last_update_check().is_null()); This dcheck is very useless here. If the ...
4 years, 11 months ago (2016-01-22 19:48:13 UTC) #38
jungkees
I uploaded a new snapshot having addressed the comment. PTAL https://codereview.chromium.org/1381153004/diff/180001/content/browser/service_worker/service_worker_register_job.cc File content/browser/service_worker/service_worker_register_job.cc (right): https://codereview.chromium.org/1381153004/diff/180001/content/browser/service_worker/service_worker_register_job.cc#newcode368 ...
4 years, 11 months ago (2016-01-25 20:03:01 UTC) #39
michaeln
thnx... lgtm!
4 years, 11 months ago (2016-01-25 23:24:38 UTC) #40
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1381153004/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1381153004/200001
4 years, 11 months ago (2016-01-26 01:22:01 UTC) #42
commit-bot: I haz the power
Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm64_dbg_recipe/builds/12673) android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED, ...
4 years, 11 months ago (2016-01-26 01:29:20 UTC) #44
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1381153004/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1381153004/200001
4 years, 11 months ago (2016-01-26 01:56:49 UTC) #46
commit-bot: I haz the power
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator_ninja/builds/121650) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, ...
4 years, 11 months ago (2016-01-26 02:01:43 UTC) #48
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1381153004/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1381153004/200001
4 years, 11 months ago (2016-01-26 03:16:16 UTC) #50
commit-bot: I haz the power
Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm64_dbg_recipe/builds/12729) android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED, ...
4 years, 11 months ago (2016-01-26 03:25:02 UTC) #52
jungkees
I rebased it having resolving a conflict. PTAL
4 years, 11 months ago (2016-01-26 08:14:41 UTC) #53
falken
lgtm
4 years, 11 months ago (2016-01-26 08:32:56 UTC) #54
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1381153004/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1381153004/220001
4 years, 11 months ago (2016-01-26 08:34:46 UTC) #57
commit-bot: I haz the power
Committed patchset #11 (id:220001)
4 years, 11 months ago (2016-01-26 10:03:40 UTC) #59
commit-bot: I haz the power
4 years, 11 months ago (2016-01-26 10:05:13 UTC) #61
Message was sent while issue was closed.
Patchset 11 (id:??) landed as
https://crrev.com/2c7aaaf09eab313a07c425c474256f92957da931
Cr-Commit-Position: refs/heads/master@{#371485}

Powered by Google App Engine
This is Rietveld 408576698