Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1872243002:
ServiceWorker: Check the equality of JS-level SW/SWR objects (Closed)
|
Created:
4 years, 8 months ago by shimazu (google) Modified:
4 years, 8 months ago CC:
chromium-reviews, michaeln, jsbell+serviceworker_chromium.org, kenjibaheux+watch_chromium.org, tzik, serviceworker-reviews, nhiroki, falken, kinuko+serviceworker, blink-reviews, horo+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionServiceWorker: Check the equality of JS-level SW/SWR objects
This test checks if the series of |registration.installing|,
|registration.waiting| and |registration.active| are the same when
installing a new service worker. I update the equality check to compare
the object itself instead of scriptURL because the spec explicitly shows
object level equality now.
Additionally, this test fails on Firefox 45.0 due to mismatching of
JS-level equality of each service worker object.
BUG=594032
Committed: https://crrev.com/ece7c41ea8ca49b959f12576284e4966a25d3606
Cr-Commit-Position: refs/heads/master@{#387875}
Patch Set 1 #
Total comments: 9
Patch Set 2 : Remove unnecessary logs, fix messages and resolve the race condition #
Total comments: 8
Patch Set 3 : Use promise_test and make synced-state.html deterministic order #
Total comments: 4
Patch Set 4 : Fix indent and add add_completion_callback to reclaim the registration #
Messages
Total messages: 14 (3 generated)
shimazu@google.com changed reviewers: + falken@chromium.org, nhiroki@chromium.org
PTAL
https://codereview.chromium.org/1872243002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/http/tests/serviceworker/synced-state.html (right): https://codereview.chromium.org/1872243002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/serviceworker/synced-state.html:19: var state = e.currentTarget.state; unused? https://codereview.chromium.org/1872243002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/serviceworker/synced-state.html:21: console.log(registration); Can you remove console.log()s? https://codereview.chromium.org/1872243002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/serviceworker/synced-state.html:26: 'be kept all the time: ' + step); It's likely that the test step proceeds before this assertion is tested because the next statechange event can be fired before getRegistration() returns. I'm not sure how to stably test the equality of registrations here... just drop this check? https://codereview.chromium.org/1872243002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/serviceworker/synced-state.html:40: 'SW object should be kept the equality ' + ".waiting should be equal to the original SW." would sounds a bit simpler. https://codereview.chromium.org/1872243002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/serviceworker/synced-state.html:54: '(activating)'); ditto https://codereview.chromium.org/1872243002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/serviceworker/synced-state.html:67: '(activated)'); ditto.
PTAL again:) https://codereview.chromium.org/1872243002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/http/tests/serviceworker/synced-state.html (right): https://codereview.chromium.org/1872243002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/serviceworker/synced-state.html:19: var state = e.currentTarget.state; On 2016/04/11 06:15:24, nhiroki wrote: > unused? Done. https://codereview.chromium.org/1872243002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/serviceworker/synced-state.html:21: console.log(registration); On 2016/04/11 06:15:24, nhiroki wrote: > Can you remove console.log()s? Done. https://codereview.chromium.org/1872243002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/serviceworker/synced-state.html:26: 'be kept all the time: ' + step); On 2016/04/11 06:15:24, nhiroki wrote: > It's likely that the test step proceeds before this assertion is tested because > the next statechange event can be fired before getRegistration() returns. I'm > not sure how to stably test the equality of registrations here... just drop this > check? Done.
https://codereview.chromium.org/1872243002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/serviceworker/synced-state.html (right): https://codereview.chromium.org/1872243002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/synced-state.html:8: function multiple_resolver(num_of_waiter, resolve) { Optional: 'barrier' (barrier_resolver?) would be common name for such a feature: https://en.wikipedia.org/wiki/Barrier_(computer_science) https://code.google.com/p/chromium/codesearch#chromium/src/base/barrier_closu... num_of_waiter -> num_of_waiters https://codereview.chromium.org/1872243002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/synced-state.html:24: .then(function(registration) { Can you add this? add_completion_callback(function() { registration.unregister(); }); https://codereview.chromium.org/1872243002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/synced-state.html:34: 'be kept all the time: ' + step); Hmm... this should work, but I'd prefer not to have assertions that could be non-deterministic. I suspect this assertion could not be tested on some steps because the spec does not ensure the execution order of statechange events and getRegistration() calls. If you really want to check the equality of registrations before/after the statechange event, how about checking it only on statechange('activated') like this? var registration; register().then(r => { registration = r; return new Promise(resolve => { r.installing.addEventListener('statechange', e => { // Test the step 1, 2, 3; if (step == 3) // .state is 'activated' resolve(); }); }); }) .then(() => { // statechange event does not happen anymore, // so we can deterministically check the equality. return getRegistration(); }) .then(r => { assert_equals(r, registration); }) https://codereview.chromium.org/1872243002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/synced-state.html:84: return service_worker_unregister_and_done(t, scope); (Not related to your changes) We don't have to call done() here bacause promise_test() internally calls it. After you add "add_completion_callback" as my comment at line 24, you can just remove this .then() chain.
Incorporated the reviews. PTAL again:) https://codereview.chromium.org/1872243002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/serviceworker/synced-state.html (right): https://codereview.chromium.org/1872243002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/synced-state.html:8: function multiple_resolver(num_of_waiter, resolve) { On 2016/04/13 05:44:10, nhiroki wrote: > Optional: 'barrier' (barrier_resolver?) would be common name for such a feature: > https://en.wikipedia.org/wiki/Barrier_(computer_science) > https://code.google.com/p/chromium/codesearch#chromium/src/base/barrier_closu... > > num_of_waiter -> num_of_waiters Done. https://codereview.chromium.org/1872243002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/synced-state.html:24: .then(function(registration) { On 2016/04/13 05:44:10, nhiroki wrote: > Can you add this? > > add_completion_callback(function() { registration.unregister(); }); Done. https://codereview.chromium.org/1872243002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/synced-state.html:34: 'be kept all the time: ' + step); On 2016/04/13 05:44:10, nhiroki wrote: > Hmm... this should work, but I'd prefer not to have assertions that could be > non-deterministic. I suspect this assertion could not be tested on some steps > because the spec does not ensure the execution order of statechange events and > getRegistration() calls. > > If you really want to check the equality of registrations before/after the > statechange event, how about checking it only on statechange('activated') like > this? > > var registration; > register().then(r => { > registration = r; > return new Promise(resolve => { > r.installing.addEventListener('statechange', e => { > // Test the step 1, 2, 3; > > if (step == 3) // .state is 'activated' > resolve(); > }); > }); > }) > .then(() => { > // statechange event does not happen anymore, > // so we can deterministically check the equality. > return getRegistration(); > }) > .then(r => { > assert_equals(r, registration); > }) Done. https://codereview.chromium.org/1872243002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/synced-state.html:84: return service_worker_unregister_and_done(t, scope); On 2016/04/13 05:44:10, nhiroki wrote: > (Not related to your changes) > > We don't have to call done() here bacause promise_test() internally calls it. > After you add "add_completion_callback" as my comment at line 24, you can just > remove this .then() chain. Done.
Looks good. Minor comments only. https://codereview.chromium.org/1872243002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/serviceworker/registration-service-worker-attributes.html (right): https://codereview.chromium.org/1872243002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/registration-service-worker-attributes.html:15: registration = r; Can you add this here? add_completion_callback(function() { registration.unregister(); }); https://codereview.chromium.org/1872243002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/serviceworker/synced-state.html (right): https://codereview.chromium.org/1872243002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/synced-state.html:11: var scope = 'resources/synced-state'; Indents would be broken. These lines need 2 more spaces.
Thanks, I update. https://codereview.chromium.org/1872243002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/serviceworker/registration-service-worker-attributes.html (right): https://codereview.chromium.org/1872243002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/registration-service-worker-attributes.html:15: registration = r; On 2016/04/18 03:26:53, nhiroki wrote: > Can you add this here? > > add_completion_callback(function() { registration.unregister(); }); Done. https://codereview.chromium.org/1872243002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/serviceworker/synced-state.html (right): https://codereview.chromium.org/1872243002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/synced-state.html:11: var scope = 'resources/synced-state'; On 2016/04/18 03:26:53, nhiroki wrote: > Indents would be broken. These lines need 2 more spaces. Done.
lgtm
The CQ bit was checked by shimazu@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1872243002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1872243002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== ServiceWorker: Check the equality of JS-level SW/SWR objects This test checks if the series of |registration.installing|, |registration.waiting| and |registration.active| are the same when installing a new service worker. I update the equality check to compare the object itself instead of scriptURL because the spec explicitly shows object level equality now. Additionally, this test fails on Firefox 45.0 due to mismatching of JS-level equality of each service worker object. BUG=594032 ========== to ========== ServiceWorker: Check the equality of JS-level SW/SWR objects This test checks if the series of |registration.installing|, |registration.waiting| and |registration.active| are the same when installing a new service worker. I update the equality check to compare the object itself instead of scriptURL because the spec explicitly shows object level equality now. Additionally, this test fails on Firefox 45.0 due to mismatching of JS-level equality of each service worker object. BUG=594032 Committed: https://crrev.com/ece7c41ea8ca49b959f12576284e4966a25d3606 Cr-Commit-Position: refs/heads/master@{#387875} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/ece7c41ea8ca49b959f12576284e4966a25d3606 Cr-Commit-Position: refs/heads/master@{#387875} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
