Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1211253007:
ServiceWorker: Add LayoutTest for WindowClient.navigate(). (Closed)
|
Created:
5 years, 5 months ago by zino Modified:
4 years, 11 months ago CC:
blink-reviews, falken, horo+watch_chromium.org, jsbell+serviceworker_chromium.org, kenjibaheux+watch_chromium.org, kinuko+serviceworker, michaeln, serviceworker-reviews, tzik Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionServiceWorker: Add LayoutTest for WindowClient.navigate().
This is a test for http://crrev.com/1196193003 and http://crrev.com/1202453002.
BUG=500911
Patch Set 1 #Patch Set 2 : #
Total comments: 9
Patch Set 3 : #
Total comments: 6
Patch Set 4 : #
Total comments: 17
Messages
Total messages: 20 (5 generated)
jinho.bang@samsung.com changed reviewers: + falken@chromium.org, mkwst@chromium.org, nhiroki@chromium.org
Please take a look. Thank you.
LGTM.
Thank you for making tests! https://codereview.chromium.org/1211253007/diff/20001/LayoutTests/http/tests/... File LayoutTests/http/tests/serviceworker/chromium/resources/windowclient-navigate.js (right): https://codereview.chromium.org/1211253007/diff/20001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/chromium/resources/windowclient-navigate.js:38: self.postMessage('navigate() can navigate not controlled client'); Could you wrap this line at line 80? Our style guide[1] requires it. [1] http://www.chromium.org/blink/serviceworker/testing#TOC-Layout-Tests-Style https://codereview.chromium.org/1211253007/diff/20001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/chromium/resources/windowclient-navigate.js:49: client.navigate('https://test.com/').then(function(c) { "https://test.com/" is a real domain. Could you change this like https://codereview.chromium.org/1224083008/ ? https://codereview.chromium.org/1211253007/diff/20001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/chromium/resources/windowclient-navigate.js:50: self.postMessage('navigate() can navigate cross origin client'); "can navigate to a cross origin URL" ? https://codereview.chromium.org/1211253007/diff/20001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/chromium/resources/windowclient-navigate.js:60: self.postMessage('navigate() can not navigate without a user interaction'); I think that the spec doesn't require a user interaction for navigate(). Could you remove "synthesizeNotificationClick()" pattern from these tests? https://codereview.chromium.org/1211253007/diff/20001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/chromium/resources/windowclient-navigate.js:69: self.postMessage('navigate() can not navigate an invalid url'); "navigate to an invalid url" https://codereview.chromium.org/1211253007/diff/20001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/chromium/resources/windowclient-navigate.js:79: self.postMessage('navigate() can not navigate view-source scheme'); ditto. https://codereview.chromium.org/1211253007/diff/20001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/chromium/resources/windowclient-navigate.js:89: self.postMessage('navigate() can not navigate file scheme'); ditto. https://codereview.chromium.org/1211253007/diff/20001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/chromium/resources/windowclient-navigate.js:99: self.postMessage('navigate() can not navigate about:blank'); ditto. https://codereview.chromium.org/1211253007/diff/20001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/chromium/resources/windowclient-navigate.js:109: self.postMessage('navigate() can not navigate about:crash'); ditto.
I addressed your comments. Could you review again?
On 2015/07/13 08:59:20, zino wrote: > I addressed your comments. > > Could you review again? nhiroki@, This CL is also waiting for your confirm. :)
Sorry for my late reply. https://codereview.chromium.org/1211253007/diff/40001/LayoutTests/http/tests/... File LayoutTests/http/tests/serviceworker/chromium/resources/windowclient-navigate.js (right): https://codereview.chromium.org/1211253007/diff/40001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/chromium/resources/windowclient-navigate.js:34: function testNavigateNotControlledClient(client) { |client| ("/serviceworker/chromium/resources/blank.html") seems to be controlled. https://codereview.chromium.org/1211253007/diff/40001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/chromium/resources/windowclient-navigate.js:36: self.postMessage('navigate() can navigate not controlled client'); According to the spec, non-controlled clients should not be navigated: "4. If the context object's associated service worker client's active worker is not the incumbent settings object's global object's service worker, return a promise rejected with a TypeError." https://codereview.chromium.org/1211253007/diff/40001/LayoutTests/http/tests/... File LayoutTests/http/tests/serviceworker/chromium/windowclient-navigate.html (right): https://codereview.chromium.org/1211253007/diff/40001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/chromium/windowclient-navigate.html:11: testRunner.setPermission('notifications', 'granted', location.origin, location.origin); I think we can re-write these tests using iframes (eg. [1]) instead of openWindow() which requires a chromium-specific machinery (ie. window.testRunner). And then we can move this file and windowclient-navigate.js into http/tests/serviceworker/ so that we'll be able to upstream them to the W3C web-platform-tests later [2]. [1] https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... [2] https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
Patchset #4 (id:60001) has been deleted
Patchset #4 (id:80001) has been deleted
Reviewers, Please take a look again :) https://codereview.chromium.org/1211253007/diff/40001/LayoutTests/http/tests/... File LayoutTests/http/tests/serviceworker/chromium/resources/windowclient-navigate.js (right): https://codereview.chromium.org/1211253007/diff/40001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/chromium/resources/windowclient-navigate.js:34: function testNavigateNotControlledClient(client) { On 2015/07/15 08:53:14, nhiroki (ooo back on Sep. 28) wrote: > |client| ("/serviceworker/chromium/resources/blank.html") seems to be > controlled. Done. https://codereview.chromium.org/1211253007/diff/40001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/chromium/resources/windowclient-navigate.js:36: self.postMessage('navigate() can navigate not controlled client'); On 2015/07/15 08:53:14, nhiroki (ooo back on Sep. 28) wrote: > According to the spec, non-controlled clients should not be navigated: > > "4. If the context object's associated service worker client's active worker is > not the incumbent settings object's global object's service worker, return a > promise rejected with a TypeError." Done. https://codereview.chromium.org/1211253007/diff/40001/LayoutTests/http/tests/... File LayoutTests/http/tests/serviceworker/chromium/windowclient-navigate.html (right): https://codereview.chromium.org/1211253007/diff/40001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/chromium/windowclient-navigate.html:11: testRunner.setPermission('notifications', 'granted', location.origin, location.origin); On 2015/07/15 08:53:14, nhiroki (ooo back on Sep. 28) wrote: > I think we can re-write these tests using iframes (eg. [1]) instead of > openWindow() which requires a chromium-specific machinery (ie. > window.testRunner). And then we can move this file and windowclient-navigate.js > into http/tests/serviceworker/ so that we'll be able to upstream them to the W3C > web-platform-tests later [2]. > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > [2] > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... Done.
On 2015/09/22 07:53:31, zino wrote: > Reviewers, > > Please take a look again :) > > https://codereview.chromium.org/1211253007/diff/40001/LayoutTests/http/tests/... > File > LayoutTests/http/tests/serviceworker/chromium/resources/windowclient-navigate.js > (right): > > https://codereview.chromium.org/1211253007/diff/40001/LayoutTests/http/tests/... > LayoutTests/http/tests/serviceworker/chromium/resources/windowclient-navigate.js:34: > function testNavigateNotControlledClient(client) { > On 2015/07/15 08:53:14, nhiroki (ooo back on Sep. 28) wrote: > > |client| ("/serviceworker/chromium/resources/blank.html") seems to be > > controlled. > > Done. > > https://codereview.chromium.org/1211253007/diff/40001/LayoutTests/http/tests/... > LayoutTests/http/tests/serviceworker/chromium/resources/windowclient-navigate.js:36: > self.postMessage('navigate() can navigate not controlled client'); > On 2015/07/15 08:53:14, nhiroki (ooo back on Sep. 28) wrote: > > According to the spec, non-controlled clients should not be navigated: > > > > "4. If the context object's associated service worker client's active worker > is > > not the incumbent settings object's global object's service worker, return a > > promise rejected with a TypeError." > > Done. > > https://codereview.chromium.org/1211253007/diff/40001/LayoutTests/http/tests/... > File LayoutTests/http/tests/serviceworker/chromium/windowclient-navigate.html > (right): > > https://codereview.chromium.org/1211253007/diff/40001/LayoutTests/http/tests/... > LayoutTests/http/tests/serviceworker/chromium/windowclient-navigate.html:11: > testRunner.setPermission('notifications', 'granted', location.origin, > location.origin); > On 2015/07/15 08:53:14, nhiroki (ooo back on Sep. 28) wrote: > > I think we can re-write these tests using iframes (eg. [1]) instead of > > openWindow() which requires a chromium-specific machinery (ie. > > window.testRunner). And then we can move this file and > windowclient-navigate.js > > into http/tests/serviceworker/ so that we'll be able to upstream them to the > W3C > > web-platform-tests later [2]. > > > > [1] > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > [2] > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > Done. Please take a look. Thank you.
sorry for my late reply https://codereview.chromium.org/1211253007/diff/100001/LayoutTests/http/tests... File LayoutTests/http/tests/serviceworker/resources/windowclient-navigate.js (right): https://codereview.chromium.org/1211253007/diff/100001/LayoutTests/http/tests... LayoutTests/http/tests/serviceworker/resources/windowclient-navigate.js:20: }); promise-chain indent (see my other comment) client.navigate(url).then(function(c)) { ... }) .catch(function(e) { ... }); https://codereview.chromium.org/1211253007/diff/100001/LayoutTests/http/tests... File LayoutTests/http/tests/serviceworker/windowclient-navigate.html (right): https://codereview.chromium.org/1211253007/diff/100001/LayoutTests/http/tests... LayoutTests/http/tests/serviceworker/windowclient-navigate.html:21: { url : 'view-source://http://test.com' }, 'view-source:http://example.com'? - '//' before 'http' is not necessary. - Probably this test will be pushed to W3C web-platform-tests repository, so the test domain "example.com" would be better ("http://test.com" is a real domain). https://codereview.chromium.org/1211253007/diff/100001/LayoutTests/http/tests... LayoutTests/http/tests/serviceworker/windowclient-navigate.html:29: 'UnknownError', According to the spec, navigation for uncontrolled client should be rejected with TypeError. https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#cl... "4. If the context object's associated service worker client's active worker is not the incumbent settings object's global object's service worker, return a promise rejected with a TypeError." https://codereview.chromium.org/1211253007/diff/100001/LayoutTests/http/tests... LayoutTests/http/tests/serviceworker/windowclient-navigate.html:32: 'UnknownError', The spec does not mention 'view-source', but "TypeError" could be more appropriate like other invalid urls. https://codereview.chromium.org/1211253007/diff/100001/LayoutTests/http/tests... LayoutTests/http/tests/serviceworker/windowclient-navigate.html:43: test, 'resources/windowclient-navigate.js', scope) Could you add "-worker.js" suffix? "windowclient-navigate-worker.js" https://codereview.chromium.org/1211253007/diff/100001/LayoutTests/http/tests... LayoutTests/http/tests/serviceworker/windowclient-navigate.html:46: }) Could you insert more indents like this? return service_worker_unregister_and_register(...) .then(function(registration)) { // 4-spaces indent from ".then" return wait_for_state(test, registration.installing, 'activated'); // 2-spaces indent from ".then" }) .then(...) (see: http://www.chromium.org/blink/serviceworker/testing#TOC-Layout-Tests-Style) https://codereview.chromium.org/1211253007/diff/100001/LayoutTests/http/tests... LayoutTests/http/tests/serviceworker/windowclient-navigate.html:56: message_frame = f; Why doesn't this toplevel frame (not |message_frame|) directly communicate with service worker using MessageChannel? https://codereview.chromium.org/1211253007/diff/100001/LayoutTests/http/tests... LayoutTests/http/tests/serviceworker/windowclient-navigate.html:77: }); nit: indent with_iframe(...).then(function(f) { ... });
Patchset #5 (id:120001) has been deleted
Thank you for review. I addressed your comments and I've just reupload this patch to chromium repo. Please review: https://codereview.chromium.org/1390993002 https://codereview.chromium.org/1211253007/diff/100001/LayoutTests/http/tests... File LayoutTests/http/tests/serviceworker/resources/windowclient-navigate.js (right): https://codereview.chromium.org/1211253007/diff/100001/LayoutTests/http/tests... LayoutTests/http/tests/serviceworker/resources/windowclient-navigate.js:20: }); On 2015/09/28 08:59:21, nhiroki wrote: > promise-chain indent (see my other comment) > > client.navigate(url).then(function(c)) { > ... > }) > .catch(function(e) { > ... > }); Done. https://codereview.chromium.org/1211253007/diff/100001/LayoutTests/http/tests... File LayoutTests/http/tests/serviceworker/windowclient-navigate.html (right): https://codereview.chromium.org/1211253007/diff/100001/LayoutTests/http/tests... LayoutTests/http/tests/serviceworker/windowclient-navigate.html:21: { url : 'view-source://http://test.com' }, On 2015/09/28 08:59:21, nhiroki wrote: > 'view-source:http://example.com'? > > - '//' before 'http' is not necessary. > - Probably this test will be pushed to W3C web-platform-tests repository, so > the test domain "example.com" would be better ("http://test.com" is a real > domain). Done. https://codereview.chromium.org/1211253007/diff/100001/LayoutTests/http/tests... LayoutTests/http/tests/serviceworker/windowclient-navigate.html:29: 'UnknownError', I tried it but I faced some troubles. So, I filed bug and left a comment. Please see: http://crbug.com/540503 https://codereview.chromium.org/1211253007/diff/100001/LayoutTests/http/tests... LayoutTests/http/tests/serviceworker/windowclient-navigate.html:32: 'UnknownError', On 2015/09/28 08:59:21, nhiroki wrote: > The spec does not mention 'view-source', but "TypeError" could be more > appropriate like other invalid urls. ditto https://codereview.chromium.org/1211253007/diff/100001/LayoutTests/http/tests... LayoutTests/http/tests/serviceworker/windowclient-navigate.html:43: test, 'resources/windowclient-navigate.js', scope) On 2015/09/28 08:59:21, nhiroki wrote: > Could you add "-worker.js" suffix? > > "windowclient-navigate-worker.js" Done. https://codereview.chromium.org/1211253007/diff/100001/LayoutTests/http/tests... LayoutTests/http/tests/serviceworker/windowclient-navigate.html:46: }) On 2015/09/28 08:59:21, nhiroki wrote: > Could you insert more indents like this? > > return service_worker_unregister_and_register(...) > .then(function(registration)) { > // 4-spaces indent from ".then" > return wait_for_state(test, registration.installing, 'activated'); > // 2-spaces indent from ".then" > }) > .then(...) > > (see: > http://www.chromium.org/blink/serviceworker/testing#TOC-Layout-Tests-Style) Done. https://codereview.chromium.org/1211253007/diff/100001/LayoutTests/http/tests... LayoutTests/http/tests/serviceworker/windowclient-navigate.html:56: message_frame = f; On 2015/09/28 08:59:21, nhiroki wrote: > Why doesn't this toplevel frame (not |message_frame|) directly communicate with > service worker using MessageChannel? Done. https://codereview.chromium.org/1211253007/diff/100001/LayoutTests/http/tests... LayoutTests/http/tests/serviceworker/windowclient-navigate.html:77: }); On 2015/09/28 08:59:21, nhiroki wrote: > nit: indent > > with_iframe(...).then(function(f) { > ... > }); Done.
I addressed your comments and I've just reupload this patch to chromium repo. Please review: https://codereview.chromium.org/1390993002
On 2015/10/08 02:44:53, zino wrote: > I addressed your comments and I've just reupload this patch to chromium repo. > > Please review: > https://codereview.chromium.org/1390993002 Thank you for the heads up!
https://codereview.chromium.org/1211253007/diff/100001/LayoutTests/http/tests... File LayoutTests/http/tests/serviceworker/windowclient-navigate.html (right): https://codereview.chromium.org/1211253007/diff/100001/LayoutTests/http/tests... LayoutTests/http/tests/serviceworker/windowclient-navigate.html:29: 'UnknownError', On 2015/10/07 02:30:41, zino wrote: > I tried it but I faced some troubles. > > So, I filed bug and left a comment. > > Please see: http://crbug.com/540503 Acknowledged.
Any update on this patch?
On 2016/01/26 01:50:53, falken wrote: > Any update on this patch? This patch was already landed: https://codereview.chromium.org/1390993002/ This CL was uploaded to Blink repo. So, I'll close this.
Description was changed from ========== ServiceWorker: Add LayoutTest for WindowClient.navigate(). This is a test for http://crrev.com/1196193003 and http://crrev.com/1202453002. BUG=500911 ========== to ========== ServiceWorker: Add LayoutTest for WindowClient.navigate(). This is a test for http://crrev.com/1196193003 and http://crrev.com/1202453002. BUG=500911 ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
