Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1286123004:
Ensure that Service Worker clients are always returned in MRU order (2) (Closed)
|
Created:
5 years, 4 months ago by jeremyarcher Modified:
5 years, 4 months ago CC:
blink-reviews, michaeln, jsbell+serviceworker_chromium.org, kenjibaheux+watch_chromium.org, tzik, serviceworker-reviews, falken, kinuko+serviceworker, horo+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionEnsure that Service Worker clients are always returned in MRU order (2)
Adds a layout test to ensure that client ordering is done properly.
Spec: https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#clients-matchall-method
1. (Chromium) https://codereview.chromium.org/1285373002/
2. (Blink) This CL.
BUG=461411
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=200875
Patch Set 1 : #
Total comments: 6
Patch Set 2 : Add semicolon. #Patch Set 3 : Ensure that tests actually test MRU ordering. #
Total comments: 10
Patch Set 4 : Switch to promise_test(...) and fix silly copypasta. #
Total comments: 2
Patch Set 5 : Restructure tests for readability. #
Messages
Total messages: 23 (9 generated)
jeremyarcher@google.com changed reviewers: + falken@chromium.org, nhiroki@chromium.org
Patchset #1 (id:1) has been deleted
https://codereview.chromium.org/1286123004/diff/20001/LayoutTests/http/tests/... File LayoutTests/http/tests/serviceworker/clients-matchall-ordering.html (right): https://codereview.chromium.org/1286123004/diff/20001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/clients-matchall-ordering.html:17: .then(function() { return with_popup(scope + '#1'); }) Why don't you use iframe? (iframe is a window-client, just in case) https://codereview.chromium.org/1286123004/diff/20001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/clients-matchall-ordering.html:37: ] nit: ';'
https://codereview.chromium.org/1286123004/diff/20001/LayoutTests/http/tests/... File LayoutTests/http/tests/serviceworker/clients-matchall-ordering.html (right): https://codereview.chromium.org/1286123004/diff/20001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/clients-matchall-ordering.html:17: .then(function() { return with_popup(scope + '#1'); }) On 2015/08/17 06:58:45, nhiroki wrote: > Why don't you use iframe? (iframe is a window-client, just in case) I too would prefer iframe if possible. Otherwise we should at least rename frame1,2 to window1,2. https://codereview.chromium.org/1286123004/diff/20001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/clients-matchall-ordering.html:34: var pageOrder = [ our wpt tests use snake_case for variables and functions https://codereview.chromium.org/1286123004/diff/20001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/clients-matchall-ordering.html:36: new URL(scope + '#1', location).toString() This test can be more discerning, now if the impl simply returns in FIFO order, the test would still pass. Can you do something like open three frames and focus the middle one? Maybe also test before and after focusing? https://codereview.chromium.org/1286123004/diff/20001/LayoutTests/http/tests/... File LayoutTests/http/tests/serviceworker/resources/clients-matchall-worker.js (right): https://codereview.chromium.org/1286123004/diff/20001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/resources/clients-matchall-worker.js:16: message.sort(function(a, b) { return a[2] > b[2] ? 1 : -1; }); This extra flag is kind of tricky, I'd just remove the sorting from this function and let the caller deal with whether they want to sort it or not. It looks like the only other test using this clients-matchall-include-uncontrolled.html is already sorting the expected urls so it's natural to also sort the actual urls.
On 2015/08/17 06:58:45, nhiroki wrote: > https://codereview.chromium.org/1286123004/diff/20001/LayoutTests/http/tests/... > File LayoutTests/http/tests/serviceworker/clients-matchall-ordering.html > (right): > > https://codereview.chromium.org/1286123004/diff/20001/LayoutTests/http/tests/... > LayoutTests/http/tests/serviceworker/clients-matchall-ordering.html:17: > .then(function() { return with_popup(scope + '#1'); }) > Why don't you use iframe? (iframe is a window-client, just in case) As it stands, this patch only orders on when a rendering context is made "active," rather than when it was "focused," as the spec technically requires; I wasn't sure how to have an "inactive" <iframe>. > https://codereview.chromium.org/1286123004/diff/20001/LayoutTests/http/tests/... > LayoutTests/http/tests/serviceworker/clients-matchall-ordering.html:37: ] > nit: ';' Done.
On 2015/08/17 07:11:30, jeremyarcher wrote: > On 2015/08/17 06:58:45, nhiroki wrote: > > > https://codereview.chromium.org/1286123004/diff/20001/LayoutTests/http/tests/... > > File LayoutTests/http/tests/serviceworker/clients-matchall-ordering.html > > (right): > > > > > https://codereview.chromium.org/1286123004/diff/20001/LayoutTests/http/tests/... > > LayoutTests/http/tests/serviceworker/clients-matchall-ordering.html:17: > > .then(function() { return with_popup(scope + '#1'); }) > > Why don't you use iframe? (iframe is a window-client, just in case) > > As it stands, this patch only orders on when a rendering context is made > "active," rather than when it was "focused," as the spec technically requires; I > wasn't sure how to have an "inactive" <iframe>. Hmm... apparently I misunderstood "active" and "focus" states. LastActiveTime in the previous patch is updated only when a window is activated (not focused), is this correct? If so, I'd prefer supporting LastFocusTime in the previous patch to supporting only an activation case.
Patchset #3 (id:60001) has been deleted
Patchset #3 (id:80001) has been deleted
On 2015/08/17 07:57:13, nhiroki wrote: > On 2015/08/17 07:11:30, jeremyarcher wrote: > > On 2015/08/17 06:58:45, nhiroki wrote: > > > > > > https://codereview.chromium.org/1286123004/diff/20001/LayoutTests/http/tests/... > > > File LayoutTests/http/tests/serviceworker/clients-matchall-ordering.html > > > (right): > > > > > > > > > https://codereview.chromium.org/1286123004/diff/20001/LayoutTests/http/tests/... > > > LayoutTests/http/tests/serviceworker/clients-matchall-ordering.html:17: > > > .then(function() { return with_popup(scope + '#1'); }) > > > Why don't you use iframe? (iframe is a window-client, just in case) > > > > As it stands, this patch only orders on when a rendering context is made > > "active," rather than when it was "focused," as the spec technically requires; > I > > wasn't sure how to have an "inactive" <iframe>. > > Hmm... apparently I misunderstood "active" and "focus" states. LastActiveTime in > the previous patch is updated only when a window is activated (not focused), is > this correct? If so, I'd prefer supporting LastFocusTime in the previous patch > to supporting only an activation case. I've updated the semantics of the property (now it works for frames, too!) and made the tests a bit more useful. PTAL
https://codereview.chromium.org/1286123004/diff/100001/LayoutTests/http/tests... File LayoutTests/http/tests/serviceworker/clients-matchall.html (right): https://codereview.chromium.org/1286123004/diff/100001/LayoutTests/http/tests... LayoutTests/http/tests/serviceworker/clients-matchall.html:10: t.step(function() { Can you make this promise_test? https://codereview.chromium.org/1286123004/diff/100001/LayoutTests/http/tests... LayoutTests/http/tests/serviceworker/clients-matchall.html:27: f.contentWindow.navigator.serviceWorker.controller.postMessage( For cleanup, you can retain |registration.installing| at line 14 and call postMessage() on that. https://codereview.chromium.org/1286123004/diff/100001/LayoutTests/http/tests... LayoutTests/http/tests/serviceworker/clients-matchall.html:36: frame1.focus(); This makes the same result with the first trial. frame2.focus() could be more meaningful? https://codereview.chromium.org/1286123004/diff/100001/LayoutTests/http/tests... LayoutTests/http/tests/serviceworker/clients-matchall.html:39: frame1.contentWindow.navigator.serviceWorker.controller. ditto (reg.installing) https://codereview.chromium.org/1286123004/diff/100001/LayoutTests/http/tests... File LayoutTests/http/tests/serviceworker/resources/test-helpers.js (right): https://codereview.chromium.org/1286123004/diff/100001/LayoutTests/http/tests... LayoutTests/http/tests/serviceworker/resources/test-helpers.js:70: var with_popup = function(url) { This looks no longer necessary.
https://codereview.chromium.org/1286123004/diff/100001/LayoutTests/http/tests... File LayoutTests/http/tests/serviceworker/clients-matchall.html (right): https://codereview.chromium.org/1286123004/diff/100001/LayoutTests/http/tests... LayoutTests/http/tests/serviceworker/clients-matchall.html:10: t.step(function() { On 2015/08/19 08:01:13, nhiroki wrote: > Can you make this promise_test? Done. https://codereview.chromium.org/1286123004/diff/100001/LayoutTests/http/tests... LayoutTests/http/tests/serviceworker/clients-matchall.html:27: f.contentWindow.navigator.serviceWorker.controller.postMessage( On 2015/08/19 08:01:13, nhiroki wrote: > For cleanup, you can retain |registration.installing| at line 14 and call > postMessage() on that. Done. https://codereview.chromium.org/1286123004/diff/100001/LayoutTests/http/tests... LayoutTests/http/tests/serviceworker/clients-matchall.html:36: frame1.focus(); On 2015/08/19 08:01:13, nhiroki wrote: > This makes the same result with the first trial. frame2.focus() could be more > meaningful? Oh dear- copy paste strikes again. PTAL https://codereview.chromium.org/1286123004/diff/100001/LayoutTests/http/tests... LayoutTests/http/tests/serviceworker/clients-matchall.html:39: frame1.contentWindow.navigator.serviceWorker.controller. On 2015/08/19 08:01:13, nhiroki wrote: > ditto (reg.installing) Done. https://codereview.chromium.org/1286123004/diff/100001/LayoutTests/http/tests... File LayoutTests/http/tests/serviceworker/resources/test-helpers.js (right): https://codereview.chromium.org/1286123004/diff/100001/LayoutTests/http/tests... LayoutTests/http/tests/serviceworker/resources/test-helpers.js:70: var with_popup = function(url) { On 2015/08/19 08:01:13, nhiroki wrote: > This looks no longer necessary. Done.
LGTM! https://codereview.chromium.org/1286123004/diff/140001/LayoutTests/http/tests... File LayoutTests/http/tests/serviceworker/clients-matchall.html (right): https://codereview.chromium.org/1286123004/diff/140001/LayoutTests/http/tests... LayoutTests/http/tests/serviceworker/clients-matchall.html:35: frame2 = f; nit: Can you move this line to line 33? https://codereview.chromium.org/1286123004/diff/140001/LayoutTests/http/tests... LayoutTests/http/tests/serviceworker/clients-matchall.html:46: assert_array_equals(message.data[1], expectedFirst[1]); nit: How about moving these assertions to line 43? This promise block is for the next trial, but these assertions are for the previous trial.
Patchset #4 (id:120001) has been deleted
On 2015/08/19 09:09:45, nhiroki wrote: > LGTM! > > https://codereview.chromium.org/1286123004/diff/140001/LayoutTests/http/tests... > File LayoutTests/http/tests/serviceworker/clients-matchall.html (right): > > https://codereview.chromium.org/1286123004/diff/140001/LayoutTests/http/tests... > LayoutTests/http/tests/serviceworker/clients-matchall.html:35: frame2 = f; > nit: Can you move this line to line 33? > > https://codereview.chromium.org/1286123004/diff/140001/LayoutTests/http/tests... > LayoutTests/http/tests/serviceworker/clients-matchall.html:46: > assert_array_equals(message.data[1], expectedFirst[1]); > nit: How about moving these assertions to line 43? This promise block is for the > next trial, but these assertions are for the previous trial. Fixed.
The CQ bit was checked by jeremyarcher@google.com
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/1286123004/#ps160001 (title: "Restructure tests for readability.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1286123004/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1286123004/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by jeremyarcher@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1286123004/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1286123004/160001
Message was sent while issue was closed.
Committed patchset #5 (id:160001) as https://src.chromium.org/viewvc/blink?view=rev&revision=200875 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
