Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2426723004:
ServiceWorker: Modernize postMessage tests for cleanup (Closed)
|
Created:
4 years, 2 months ago by nhiroki Modified:
4 years, 2 months ago CC:
chromium-reviews, michaeln, jsbell+serviceworker_chromium.org, kenjibaheux+watch_chromium.org, shimazu+serviceworker_chromium.org, serviceworker-reviews, nhiroki, kinuko+serviceworker, blink-reviews, horo+watch_chromium.org, falken+watch_chromium.org, tzik Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionServiceWorker: Modernize postMessage tests for cleanup
These tests still use old helpers/syntax like async_test, and they prevent from
changing/adding tests. This CL replaces them with more modern helpers/syntax and
simplifies the tests.
BUG=n/a
Committed: https://crrev.com/8b24bed363581a960b3a756bf09d420a926f8641
Cr-Commit-Position: refs/heads/master@{#426147}
Patch Set 1 #
Total comments: 13
Patch Set 2 : address comments #
Total comments: 2
Patch Set 3 : address review comments #
Messages
Total messages: 21 (11 generated)
Description was changed from ========== ServiceWorker: Modernize postMessage tests for cleanup These tests still use old helpers/syntax like async_test, and they prevent from changing/adding tests. This CL replaces them with more modern helpers/syntax. BUG=n/a ========== to ========== ServiceWorker: Modernize postMessage tests for cleanup These tests still use old helpers/syntax like async_test, and they prevent from changing/adding tests. This CL replaces them with more modern helpers/syntax and simplifies the tests. BUG=n/a ==========
The CQ bit was checked by nhiroki@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
nhiroki@chromium.org changed reviewers: + falken@chromium.org
Hi, can you review this? I'd like to have this cleanup so that I can easily add more tests for https://codereview.chromium.org/2414333003/ Thanks! https://codereview.chromium.org/2426723004/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/postmessage-msgport-to-client-worker.js (right): https://codereview.chromium.org/2426723004/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/postmessage-msgport-to-client-worker.js:13: function onMessageViaMessagePort(port, e) { I changed a communication path from "client" to "port" because, according to an expectation on postmessage-msgport-to-client.html, this test expects that a message is routed via MessagePort (but wasn't).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
shimazu@chromium.org changed reviewers: + shimazu@chromium.org
Drive-by comment: https://codereview.chromium.org/2426723004/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/http/tests/serviceworker/postmessage-msgport-to-client.html (right): https://codereview.chromium.org/2426723004/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/serviceworker/postmessage-msgport-to-client.html:20: add_completion_callback(() => frame.remove()); It's not necessary to remove |frame| explicitly because with_iframe removes the frame by default. https://codereview.chromium.org/2426723004/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/http/tests/serviceworker/postmessage.html (right): https://codereview.chromium.org/2426723004/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/serviceworker/postmessage.html:17: add_completion_callback(() => registration.unregister()); How about using |r| directly? add_completion_callback(() => r.unregister());
https://codereview.chromium.org/2426723004/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/http/tests/serviceworker/postmessage-msgport-to-client.html (right): https://codereview.chromium.org/2426723004/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/serviceworker/postmessage-msgport-to-client.html:28: port = e.data.port; Same here, this can just be e.ports[0]? https://codereview.chromium.org/2426723004/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/http/tests/serviceworker/postmessage.html (right): https://codereview.chromium.org/2426723004/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/serviceworker/postmessage.html:26: [messageChannel.port2]); Why do we send port manually in an object and parse it out on the other end instead of just using ExtendableMessageEvent.ports[0]? https://codereview.chromium.org/2426723004/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/postmessage-msgport-to-client-worker.js (right): https://codereview.chromium.org/2426723004/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/postmessage-msgport-to-client-worker.js:18: port.postMessage({done: true}); Not sure if this works but can it just be e.ports[0].postMessage?
Thank you for your comments. PTAL. https://codereview.chromium.org/2426723004/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/http/tests/serviceworker/postmessage-msgport-to-client.html (right): https://codereview.chromium.org/2426723004/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/serviceworker/postmessage-msgport-to-client.html:20: add_completion_callback(() => frame.remove()); On 2016/10/19 01:18:58, shimazu wrote: > It's not necessary to remove |frame| explicitly because with_iframe removes the > frame by default. Done. https://codereview.chromium.org/2426723004/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/serviceworker/postmessage-msgport-to-client.html:28: port = e.data.port; On 2016/10/19 01:55:24, falken wrote: > Same here, this can just be e.ports[0]? Done. https://codereview.chromium.org/2426723004/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/http/tests/serviceworker/postmessage.html (right): https://codereview.chromium.org/2426723004/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/serviceworker/postmessage.html:17: add_completion_callback(() => registration.unregister()); On 2016/10/19 01:18:58, shimazu wrote: > How about using |r| directly? > add_completion_callback(() => r.unregister()); Done. https://codereview.chromium.org/2426723004/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/serviceworker/postmessage.html:26: [messageChannel.port2]); On 2016/10/19 01:55:24, falken wrote: > Why do we send port manually in an object and parse it out on the other end > instead of just using ExtendableMessageEvent.ports[0]? Good point. We could use ExtendableMessageEvent.source instead of passing a MessagePort. Other tests also depend on this worker script and this change affects them, so I'd like to make this change in a following CL. https://codereview.chromium.org/2426723004/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/postmessage-msgport-to-client-worker.js (right): https://codereview.chromium.org/2426723004/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/postmessage-msgport-to-client-worker.js:18: port.postMessage({done: true}); On 2016/10/19 01:55:24, falken wrote: > Not sure if this works but can it just be e.ports[0].postMessage? In my understanding, 'ports' array is filled only when a port is passed as transferable in the sender-side, so e.ports[0] should be undefined here.
The CQ bit was checked by nhiroki@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2426723004/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/http/tests/serviceworker/postmessage.html (right): https://codereview.chromium.org/2426723004/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/serviceworker/postmessage.html:26: [messageChannel.port2]); On 2016/10/19 04:00:50, nhiroki (slow) wrote: > On 2016/10/19 01:55:24, falken wrote: > > Why do we send port manually in an object and parse it out on the other end > > instead of just using ExtendableMessageEvent.ports[0]? > > Good point. We could use ExtendableMessageEvent.source instead of passing a > MessagePort. Other tests also depend on this worker script and this change > affects them, so I'd like to make this change in a following CL. Thanks, yes I think a lot of our tests copy/pasted the original pattern so they are doing this confusing passing and parsing of the data object. https://codereview.chromium.org/2426723004/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/postmessage-msgport-to-client-worker.js (right): https://codereview.chromium.org/2426723004/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/postmessage-msgport-to-client-worker.js:18: port.postMessage({done: true}); On 2016/10/19 04:00:50, nhiroki (slow) wrote: > On 2016/10/19 01:55:24, falken wrote: > > Not sure if this works but can it just be e.ports[0].postMessage? > > In my understanding, 'ports' array is filled only when a port is passed as > transferable in the sender-side, so e.ports[0] should be undefined here. Acknowledged. https://codereview.chromium.org/2426723004/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/postmessage-msgport-to-client-worker.js (right): https://codereview.chromium.org/2426723004/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/postmessage-msgport-to-client-worker.js:7: client.postMessage(messageChannel.port2, [messageChannel.port2]); is it needed to pass messageChannel.port2 as the message? can this just be 'hi' instead?
Thanks. Updated. https://codereview.chromium.org/2426723004/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/postmessage-msgport-to-client-worker.js (right): https://codereview.chromium.org/2426723004/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/postmessage-msgport-to-client-worker.js:7: client.postMessage(messageChannel.port2, [messageChannel.port2]); On 2016/10/19 04:06:27, falken wrote: > is it needed to pass messageChannel.port2 as the message? can this just be 'hi' > instead? Yes, it's feasible. Done.
lgtm
The CQ bit was checked by nhiroki@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== ServiceWorker: Modernize postMessage tests for cleanup These tests still use old helpers/syntax like async_test, and they prevent from changing/adding tests. This CL replaces them with more modern helpers/syntax and simplifies the tests. BUG=n/a ========== to ========== ServiceWorker: Modernize postMessage tests for cleanup These tests still use old helpers/syntax like async_test, and they prevent from changing/adding tests. This CL replaces them with more modern helpers/syntax and simplifies the tests. BUG=n/a Committed: https://crrev.com/8b24bed363581a960b3a756bf09d420a926f8641 Cr-Commit-Position: refs/heads/master@{#426147} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/8b24bed363581a960b3a756bf09d420a926f8641 Cr-Commit-Position: refs/heads/master@{#426147} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
