Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
362123003:
ServiceWorker: unregister's scope argument should be optional (Closed)
|
Created:
6 years, 5 months ago by jsbell Modified:
6 years, 5 months ago CC:
blink-reviews, michaeln, jsbell+serviceworker_chromium.org, tzik, serviceworker-reviews, nhiroki, falken, kinuko+serviceworker, horo+watch_chromium.org, alecflett+watch_chromium.org Base URL:
svn://svn.chromium.org/blink/trunk Project:
blink Visibility:
Public. |
DescriptionServiceWorker: unregister's scope argument should be optional
Bring implementation in line with the latest spec edits: the |scope|
argument to unregister() should be optional and default to '/*'.
R=dominicc@chromium.org,michaeln@chromium.org,falken@chromium.org
BUG=388044
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=178034
Patch Set 1 #
Total comments: 9
Patch Set 2 : Rework test #
Total comments: 7
Patch Set 3 : Reformated test #
Messages
Total messages: 19 (0 generated)
dominicc@, falken@, michael@ - please take a look? Feel free to CQ if you like it, but no rush.
https://codereview.chromium.org/362123003/diff/1/LayoutTests/http/tests/servi... File LayoutTests/http/tests/serviceworker/unregister.html (right): https://codereview.chromium.org/362123003/diff/1/LayoutTests/http/tests/servi... LayoutTests/http/tests/serviceworker/unregister.html:84: 'unregistered worker should not override content'); i think this tests may be hitting on much more than the default param value, i think its hitting on things that aren't yet implemented, about unregistering sw that are inuse vs notinuse (kinda complicated things). a different way to verify unreg understand the default as "/*" could be to verify that .state transitions to 'deprecated' in the sw instance returned by register?
https://codereview.chromium.org/362123003/diff/1/LayoutTests/http/tests/servi... File LayoutTests/http/tests/serviceworker/unregister.html (right): https://codereview.chromium.org/362123003/diff/1/LayoutTests/http/tests/servi... LayoutTests/http/tests/serviceworker/unregister.html:84: 'unregistered worker should not override content'); On 2014/07/01 21:14:03, michaeln wrote: > i think this tests may be hitting on much more than the default param value, i > think its hitting on things that aren't yet implemented, about unregistering sw > that are inuse vs notinuse (kinda complicated things). > > a different way to verify unreg understand the default as "/*" could be to > verify that .state transitions to 'deprecated' in the sw instance returned by > register? Makes sense - I'll give that a whirl.
https://codereview.chromium.org/362123003/diff/1/LayoutTests/http/tests/servi... File LayoutTests/http/tests/serviceworker/unregister.html (right): https://codereview.chromium.org/362123003/diff/1/LayoutTests/http/tests/servi... LayoutTests/http/tests/serviceworker/unregister.html:66: service_worker_unregister_and_register(t, 'resources/simple-intercept-worker.js', '/*') The line wrapping in this addition is inconsistent; it mostly adheres to 80 columns, except where it doesn't. https://codereview.chromium.org/362123003/diff/1/LayoutTests/http/tests/servi... LayoutTests/http/tests/serviceworker/unregister.html:67: .then(t.step_func(function() { I think we should not indent here. asanka wrote: mumble(). then( which also works out OK when you get used to the . and then being split up. Maybe we should adopt his style? He also made abstemious use of step_func FWIW, and I am inclined to think that's more readable. We could make with_iframe return a function if it doesn't already. https://codereview.chromium.org/362123003/diff/1/LayoutTests/http/tests/servi... LayoutTests/http/tests/serviceworker/unregister.html:71: assert_true(contains(iframe.contentDocument.body.innerHTML, 'intercepted'), Maybe putting that as the whole title would make this smaller (no contains; document.title instead of document.body.innerHTML)
https://codereview.chromium.org/362123003/diff/1/LayoutTests/http/tests/servi... File LayoutTests/http/tests/serviceworker/unregister.html (right): https://codereview.chromium.org/362123003/diff/1/LayoutTests/http/tests/servi... LayoutTests/http/tests/serviceworker/unregister.html:66: service_worker_unregister_and_register(t, 'resources/simple-intercept-worker.js', '/*') On 2014/07/02 02:10:51, dominicc wrote: > The line wrapping in this addition is inconsistent; it mostly adheres to 80 > columns, except where it doesn't. Done. https://codereview.chromium.org/362123003/diff/1/LayoutTests/http/tests/servi... LayoutTests/http/tests/serviceworker/unregister.html:67: .then(t.step_func(function() { On 2014/07/02 02:10:51, dominicc wrote: > I think we should not indent here. > > asanka wrote: > > mumble(). > then( > > which also works out OK when you get used to the . and then being split up. > Maybe we should adopt his style? Hrm. Putting the '.' on the leading line is fine. Without the indent there's nothing that makes it apparent this is a promise chain - it's much less readable to me. > He also made abstemious use of step_func FWIW, and I am inclined to think that's > more readable. I agree it's more readable without, but leaving out step_func is just asking for pain later. For example, when running the IDB tests submitted to the W3C there were a few tests Chrome failed due to timeouts. It took grovelling through the test to find the missing step_func wrappers to get a meaningful failure from the test. If we leave out step_func then we're making more work for those who want to use our tests to validate their impls. https://codereview.chromium.org/362123003/diff/1/LayoutTests/http/tests/servi... LayoutTests/http/tests/serviceworker/unregister.html:84: 'unregistered worker should not override content'); On 2014/07/02 00:18:50, jsbell wrote: > On 2014/07/01 21:14:03, michaeln wrote: > > i think this tests may be hitting on much more than the default param value, i > > think its hitting on things that aren't yet implemented, about unregistering > sw > > that are inuse vs notinuse (kinda complicated things). > > > > a different way to verify unreg understand the default as "/*" could be to > > verify that .state transitions to 'deprecated' in the sw instance returned by > > register? > > Makes sense - I'll give that a whirl. It appears that our [[Unregister]] implementation matches the spec in that the [[Registration]]'s activeVersion is not set to "redundant". That seems like a spec bug, but maybe there's some subtlety I'm missing. The latest patch on this issue works if I make this addition on the Chromium side in service_worker_unregister_job.cc: + if (registration->active_version()) { + registration->active_version()->SetStatus( + ServiceWorkerVersion::REDUNDANT); + } ... but there's no-one around at the moment to bounce this off of. :P
https://codereview.chromium.org/362123003/diff/1/LayoutTests/http/tests/servi... File LayoutTests/http/tests/serviceworker/unregister.html (right): https://codereview.chromium.org/362123003/diff/1/LayoutTests/http/tests/servi... LayoutTests/http/tests/serviceworker/unregister.html:84: 'unregistered worker should not override content'); > It appears that our [[Unregister]] implementation matches the spec in that the > [[Registration]]'s activeVersion is not set to "redundant". > > That seems like a spec bug, but maybe there's some subtlety I'm missing. hmmm, i certainly had the expectation that any versions associated with an unregisterted registration would claim to be redundant thereafter, that doesn't mean they shouldn't continue to function for pages that are 'controlled' by them, just that the .state should change to 'redundant'. wdyt, open an issue to possibly tweak that in the spec?
On 2014/07/02 19:03:34, michaeln wrote: > https://codereview.chromium.org/362123003/diff/1/LayoutTests/http/tests/servi... > File LayoutTests/http/tests/serviceworker/unregister.html (right): > > https://codereview.chromium.org/362123003/diff/1/LayoutTests/http/tests/servi... > LayoutTests/http/tests/serviceworker/unregister.html:84: 'unregistered worker > should not override content'); > > It appears that our [[Unregister]] implementation matches the spec in that the > > [[Registration]]'s activeVersion is not set to "redundant". > > > > That seems like a spec bug, but maybe there's some subtlety I'm missing. > > hmmm, i certainly had the expectation that any versions associated with an > unregisterted registration would claim to be redundant thereafter, that doesn't > mean they shouldn't continue to function for pages that are 'controlled' by > them, just that the .state should change to 'redundant'. > > wdyt, open an issue to possibly tweak that in the spec? Thanks for sanity checking - opened: https://github.com/slightlyoff/ServiceWorker/issues/353
Also, uploaded the chromium fix (and test) to: https://codereview.chromium.org/369693002 ... assuming we get consensus that this is a spec glitch.
A few comments inline. I'm not sold the spec is wrong on the redundant-but-controlling thing, but maybe I just don't understand the issue. Re: indenting... FWIW Jake's H5R article and rsvp.js does it Google-style--trailing .then( Q does left-aligned .then, break before the . WinJS & cujos/when.js does indented .then, break before the . I guess I'm fine with your/WinJS/when.js style. Let's give Asanka feedback to that effect. Re: test_step, why can't we just end Promise chains with .catch(generic test failing handler) ? https://codereview.chromium.org/362123003/diff/20001/LayoutTests/http/tests/s... File LayoutTests/http/tests/serviceworker/resources/test-helpers.js (right): https://codereview.chromium.org/362123003/diff/20001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/resources/test-helpers.js:69: return new Promise(test.step_func(function(resolve, reject) { Would the test fail more quickly/incisively if you rejected when the worker advances "past" the desired state? https://codereview.chromium.org/362123003/diff/20001/LayoutTests/http/tests/s... File LayoutTests/http/tests/serviceworker/unregister.html (right): https://codereview.chromium.org/362123003/diff/20001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/unregister.html:65: // FIXME; 'deactivated' should be 'redundant' crrev.com/352423005 FIXME; -> FIXME: https://codereview.chromium.org/362123003/diff/20001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/unregister.html:65: // FIXME; 'deactivated' should be 'redundant' crrev.com/352423005 You could map redundant to deactivated inside wait_for_state and have the FIXME in one place for everyone.
Long weekend here in the US so not touching this for several more days. Thanks for the feedback! https://codereview.chromium.org/362123003/diff/20001/LayoutTests/http/tests/s... File LayoutTests/http/tests/serviceworker/resources/test-helpers.js (right): https://codereview.chromium.org/362123003/diff/20001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/resources/test-helpers.js:69: return new Promise(test.step_func(function(resolve, reject) { On 2014/07/03 04:44:35, dominicc wrote: > Would the test fail more quickly/incisively if you rejected when the worker > advances "past" the desired state? That would imply encoding the valid state transitions into this helper, which seems fragile. https://codereview.chromium.org/362123003/diff/20001/LayoutTests/http/tests/s... File LayoutTests/http/tests/serviceworker/unregister.html (right): https://codereview.chromium.org/362123003/diff/20001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/unregister.html:65: // FIXME; 'deactivated' should be 'redundant' crrev.com/352423005 On 2014/07/03 04:44:35, dominicc wrote: > You could map redundant to deactivated inside wait_for_state and have the FIXME > in one place for everyone. The CL to rename the states will land before this CL does, and this will need to be rebased The FIXMEs are really just a hint for reviewers.
On 2014/07/03 04:44:35, dominicc wrote: > I'm not sold the spec is wrong on the redundant-but-controlling thing, but maybe > I just don't understand the issue. Would you expect the test to work as written? If so, there's a spec issue, because our implementation matches the spec and the test fails. If not, can you explain how you'd correct the test?
On 2014/07/03 05:19:16, jsbell wrote: > On 2014/07/03 04:44:35, dominicc wrote: > > I'm not sold the spec is wrong on the redundant-but-controlling thing, but > maybe > > I just don't understand the issue. > > Would you expect the test to work as written? Yes. I think so. > If so, there's a spec issue, because our implementation matches the spec and the > test fails. I'm looking at https://github.com/slightlyoff/ServiceWorker/issues/353 from your Chromium patch and it looks like you have worked this out? I don't see anything in that issue about optional parameters to unregister which is what this patch is about. > If not, can you explain how you'd correct the test?
Oops. Plus some comments. https://codereview.chromium.org/362123003/diff/20001/LayoutTests/http/tests/s... File LayoutTests/http/tests/serviceworker/unregister.html (right): https://codereview.chromium.org/362123003/diff/20001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/unregister.html:62: var statePromise; state_promise https://codereview.chromium.org/362123003/diff/20001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/unregister.html:63: service_worker_unregister_and_register(t, 'resources/empty-worker.js'). Break before .
Yay, falken's change to make unregister move the state of an active worker to 'redundant' unblocks this. I went ahead and updated all of the tests in the file to match our new style guide (I hope!). dominicc@ - please take a look? (and CQ if it looks good)
On 2014/07/11 19:03:09, jsbell wrote: > Yay, falken's change to make unregister move the state of an active worker to > 'redundant' unblocks this. > > I went ahead and updated all of the tests in the file to match our new style > guide (I hope!). > > dominicc@ - please take a look? (and CQ if it looks good) I think the style guide hasn't quite set yet but this LGTM++
The CQ bit was checked by dominicc@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jsbell@chromium.org/362123003/40001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_blink_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
Message was sent while issue was closed.
Change committed as 178034 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
