Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1268663003:
Service Worker: Make ServiceWorkerRegistration.update() return a promise. (Blink Layout 3/3) (Closed)
|
Created:
5 years, 4 months ago by jungkees Modified:
5 years, 4 months ago CC:
blink-reviews, jsbell, tzik, serviceworker-reviews, dglazkov+blink, kinuko+serviceworker, horo Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionService Worker: Make ServiceWorkerRegistration.update() return a promise. (Blink Layout 3/3)
As per the resolution of f2f, ServiceWorkerRegistration.update() should return a
promise that transforms the promise returned by Update algorithm.
In this CL, ServiceWorkerContextCore::UpdateServiceWorker method has been
overloaded to cover two invocation paths: a scheduled update without a
provider_host and a callback (Soft Update in the spec) and a call initiated from
the script surface using ServiceWorkerRegistration.update().
Spec: https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#service-worker-registration-update
Spec discussion: https://github.com/slightlyoff/ServiceWorker/issues/311
Companion CL (Blink): https://codereview.chromium.org/1260833003/
Companion CL (Chromium): https://codereview.chromium.org/1270513002/
BUG=513655
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=200300
Patch Set 1 #
Total comments: 2
Patch Set 2 : #
Total comments: 7
Patch Set 3 : Merge test files. #Patch Set 4 : Nit: correct the file name in comment. #Patch Set 5 : Remove output set before setcookie. #
Total comments: 7
Patch Set 6 : Simplify the php script structure; remove uncessary catch block. #Patch Set 7 : Rebase. #
Messages
Total messages: 33 (10 generated)
jungkee.song@samsung.com changed reviewers: + jinho.bang@samsung.com, l.gombos@samsung.com
Blink layout test side of the registration.update() change. PTAL.
jungkee.song@samsung.com changed reviewers: + falken@chromium.org, kinuko@chromium.org, michaeln@chromium.org, nhiroki@chromium.org
This CL should include the change described in the step 3 of https://codereview.chromium.org/1267703003/#msg4 after Blink-side patch and Chromium-side patch land. SW OWNERs, PTAL.
The CQ bit was checked by jinho.bang@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1268663003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1268663003/1
The CQ bit was unchecked by jinho.bang@samsung.com
https://codereview.chromium.org/1268663003/diff/1/LayoutTests/http/tests/serv... File LayoutTests/http/tests/serviceworker/update.html (left): https://codereview.chromium.org/1268663003/diff/1/LayoutTests/http/tests/serv... LayoutTests/http/tests/serviceworker/update.html:28: return wait_for_update(t, registration); Looks good to me but would you please clarify one more thing? Can we remove the |wait_for_update()| function? If so, please remove it. Otherwise, l-g-t-m.
On 2015/07/31 05:55:00, jungkees wrote: > This CL should include the change described in the step 3 of > https://codereview.chromium.org/1267703003/#msg4 after Blink-side patch and > Chromium-side patch land. > > SW OWNERs, PTAL. Could you update this CL? I'll review this after that.
My opinion about keeping wait_for_update() helper function. Thanks for review. https://codereview.chromium.org/1268663003/diff/1/LayoutTests/http/tests/serv... File LayoutTests/http/tests/serviceworker/update.html (left): https://codereview.chromium.org/1268663003/diff/1/LayoutTests/http/tests/serv... LayoutTests/http/tests/serviceworker/update.html:28: return wait_for_update(t, registration); wait_for_update() helper function is more generally used for a control to wait until the ServiceWorkerRegistration object is dispatched "updatefound" event, which actually means almost the samething as registration.update() promise would imply. But a thing is that "updatefound" is dispatched for the other *update* scenarios including .register() and UA-initiated updates. So, retaining the helper function would be useful for those contexts I guess. E.g. registration-service-worker-attributes.html now uses the function to catch the "updatefound" event for .register() steps. Thanks for review.
On 2015/08/05 02:09:31, jungkees wrote: > My opinion about keeping wait_for_update() helper function. Thanks for review. > > https://codereview.chromium.org/1268663003/diff/1/LayoutTests/http/tests/serv... > File LayoutTests/http/tests/serviceworker/update.html (left): > > https://codereview.chromium.org/1268663003/diff/1/LayoutTests/http/tests/serv... > LayoutTests/http/tests/serviceworker/update.html:28: return wait_for_update(t, > registration); > wait_for_update() helper function is more generally used for a control to wait > until the ServiceWorkerRegistration object is dispatched "updatefound" event, > which actually means almost the samething as registration.update() promise would > imply. But a thing is that "updatefound" is dispatched for the other *update* > scenarios including .register() and UA-initiated updates. So, retaining the > helper function would be useful for those contexts I guess. E.g. > registration-service-worker-attributes.html now uses the function to catch the > "updatefound" event for .register() steps. Thanks for review. Thank you for detailed explanation. non-owner lgtm but please wait for owner's review. :) Thank you.
zino@, nhiroki@, I haven't update it with a new snapshot including the part that removes the temporary WebServiceWorkerRegistration changes. I'll let you know once I'm done with it. Thanks.
jungkee.song@samsung.com changed reviewers: + tkent@chromium.org
nhiroki@, tkent@, I made WebServiceWorkerRegistration.h to the final shape and added a new layout test for update failure scenario. While working on this CL, I found I missed adding a method in the previous chromium-side patch. So I also created a CL to address that issue: https://codereview.chromium.org/1278713002/. PTAL.
https://codereview.chromium.org/1268663003/diff/20001/LayoutTests/http/tests/... File LayoutTests/http/tests/serviceworker/resources/update-worker-error.php (right): https://codereview.chromium.org/1268663003/diff/20001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/resources/update-worker-error.php:1: <?php Can you rename this file to "update-error-worker.php"? Using "-worker" suffix for worker scripts is our convention (sorry for our non-explicit rules...) https://codereview.chromium.org/1268663003/diff/20001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/resources/update-worker-error.php:11: header("Pragma: no-cache"); Let me confirm my understanding... in the latest spec (after [1]), update() does not always bypass the HTTP Cache, so we need to set these "no-cache" parameters here. Is this correct? (In the current implementation, update() always bypasses the HTTP Cache[2]) [1] https://github.com/slightlyoff/ServiceWorker/commit/8ea4c1e2f0ebb612962fffed4... [2] https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/se... https://codereview.chromium.org/1268663003/diff/20001/LayoutTests/http/tests/... File LayoutTests/http/tests/serviceworker/update-error.html (right): https://codereview.chromium.org/1268663003/diff/20001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/update-error.html:7: promise_test(function(t) { How about merging this test into serviceworker/update.html? I'd prefer to push tests for the same API into one file.
Updated the comments. Thanks for review. https://codereview.chromium.org/1268663003/diff/20001/LayoutTests/http/tests/... File LayoutTests/http/tests/serviceworker/resources/update-worker-error.php (right): https://codereview.chromium.org/1268663003/diff/20001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/resources/update-worker-error.php:1: <?php Sure, I'll change it. https://codereview.chromium.org/1268663003/diff/20001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/resources/update-worker-error.php:11: header("Pragma: no-cache"); On 2015/08/06 09:57:28, nhiroki wrote: > Let me confirm my understanding... in the latest spec (after [1]), update() does > not always bypass the HTTP Cache, so we need to set these "no-cache" parameters > here. Is this correct? > Yes, that's my understanding and intention. I noticed that we might need to set the |force_bypass_cache| to false there. I'll double check it with the other editors before changing that. > (In the current implementation, update() always bypasses the HTTP Cache[2]) > > [1] > https://github.com/slightlyoff/ServiceWorker/commit/8ea4c1e2f0ebb612962fffed4... > [2] > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/se... Right, I'll take care of it. https://codereview.chromium.org/1268663003/diff/20001/LayoutTests/http/tests/... File LayoutTests/http/tests/serviceworker/update-error.html (right): https://codereview.chromium.org/1268663003/diff/20001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/update-error.html:7: promise_test(function(t) { Okay. I'll upload a new snapshot when I'm ready with the change.
Uploaded a new snapshot having addressed the comments. PTAL. https://codereview.chromium.org/1268663003/diff/20001/LayoutTests/http/tests/... File LayoutTests/http/tests/serviceworker/resources/update-worker-error.php (right): https://codereview.chromium.org/1268663003/diff/20001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/resources/update-worker-error.php:11: header("Pragma: no-cache"); I've double-checked this with jakearchibald@. It's the decision from the latest f2f that update() does not always bypass the HTTP cache. It should follow the same rule (i.e. min(max-age, 86400) has passed since the last update) to bypass the cache. I guess that max-age change (https://github.com/slightlyoff/ServiceWorker/commit/8ea4c1e2f0ebb612962fffed4...) hasn't been tracked yet. If so and nobody's already working on it, I'm happy to take on it as a separate CL.
Sorry. Just wait. I found ServiceWorkerGlobalScope/update.html is affected by the merged update-worker.php file. I'll upload a new snapshot addressing this issue.
Please ignore the #18 comment and go on a review. I confused myself with a local build setting the |force_bypass_cache| argument to false in service_worker_dispatcher.cc file for test. Acutally, ServiceWorkerGlobalScope/update.html does not even use the same update-worker.php file used in this CL. Yeah, ServiceWorkerGlobalScope/resources/update-worker.php might have to be updated when we work on the max-age update change.
I found update.html caused a regression in mac and win builds. It turned out that the headers set in update-worker.php script didn't work even for the first fetch of update-worker.php in those test environments. The reason I found is the content set by the echo statement before setcookie() is called seems not allowed. (http://php.net/manual/kr/function.setcookie.php). By removing the "echo" statements, I verified update.html is correctly running. PTAL. (A regression caught in the mac_blink_rel trybot result of the patch set #5 is not related to update.html.)
LGTM with some nits https://codereview.chromium.org/1268663003/diff/80001/LayoutTests/http/tests/... File LayoutTests/http/tests/serviceworker/resources/update-worker.php (right): https://codereview.chromium.org/1268663003/diff/80001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/resources/update-worker.php:10: header("Pragma: no-cache"); Can you move these cache-control headers to out of the if-elseif steatements? (line 7) https://codereview.chromium.org/1268663003/diff/80001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/resources/update-worker.php:16: } if ($mode == 'init') { // ... } else if ($mode == 'normal') { // ... } else if ($mode == 'error') { // ... } ... could be easier to understand the state transition. https://codereview.chromium.org/1268663003/diff/80001/LayoutTests/http/tests/... File LayoutTests/http/tests/serviceworker/update.html (right): https://codereview.chromium.org/1268663003/diff/80001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/update.html:29: return registration.update(); To make sure that the updatefound event is correctly fired when an updated worker is found, how about like this? return Promise.all([registration.update(), wait_for_update(t, registration)]); https://codereview.chromium.org/1268663003/diff/80001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/update.html:73: .catch(unreached_rejection(t));; promise_test() does not have to have a 'catch' block because it internally catches a rejection.
Addressed the comments. I think I need a review from tkent@ for the WebServiceWorkerRegistration.h change to the final version? https://codereview.chromium.org/1268663003/diff/80001/LayoutTests/http/tests/... File LayoutTests/http/tests/serviceworker/resources/update-worker.php (right): https://codereview.chromium.org/1268663003/diff/80001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/resources/update-worker.php:16: } Re-organized the blocks as you commented. Looks much clearer! https://codereview.chromium.org/1268663003/diff/80001/LayoutTests/http/tests/... File LayoutTests/http/tests/serviceworker/update.html (right): https://codereview.chromium.org/1268663003/diff/80001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/update.html:29: return registration.update(); Addressed. https://codereview.chromium.org/1268663003/diff/80001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/update.html:73: .catch(unreached_rejection(t));; Removed it. Thanks.
tkent@, this CL is the last step for changing ServiceWorkerRegistration.update() behavior. Could you please take a look?
lgtm
The CQ bit was checked by jungkee.song@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from jinho.bang@samsung.com, nhiroki@chromium.org Link to the patchset: https://codereview.chromium.org/1268663003/#ps100001 (title: "Simplify the php script structure; remove uncessary catch block.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1268663003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1268663003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_blink_compile_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/android_blink_compile_re...) blink_presubmit on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/3...)
The CQ bit was checked by jungkee.song@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org, nhiroki@chromium.org, jinho.bang@samsung.com Link to the patchset: https://codereview.chromium.org/1268663003/#ps120001 (title: "Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1268663003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1268663003/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://src.chromium.org/viewvc/blink?view=rev&revision=200300 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
