-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Window.postMessage override that takes a dictionary #3799
Comments
Related issue is #1983 |
Folks, what do you think? https://github.com/dtapuska/useractivation gives one motivation (see also #1983), wanting to add a new option to postMessage(). However, I'd argue that even without that motivation, this is a good idea for the platform and for web developers:
/ccing @smaug---- as I know he's been looking at https://github.com/dtapuska/useractivation. Then @travisleithead and @hober for Edge and Safari? (I realize some folks might not find the implementation cost of this change in and of itself compelling, and might need to first be convinced of the value and design choices in https://github.com/dtapuska/useractivation. That's fine too. But with my editor and web developer hats on, I really want this change, independent of any future options we add to this dictionary.) |
@smaug---- @travisleithead @hober Any thoughts on this proposal? |
So far I haven't seen good reasoning why to change the old API. |
Providing a consistent API is fairly compelling to me. I do wonder if requiring the transfer argument forever will turn out to be a mistake. That makes more sense as part of a dictionary to me. |
ok, fine, consistency would be good, and I agree transfer as a part of dictionary would be better, since it is often empty. |
@annevk @smaug---- how does the latest revision look? |
@dtapuska I haven't reviewed in detail, but that looks okay to me. I was worried that sequence and dictionary would not be https://heycam.github.io/webidl/#dfn-distinguishable but it seems they are so the other |
So is the latest proposal:
or
with The previous comment from @smaug---- seemed to be in favor of making Personally, I do not believe that the consistency argument alone would be worth the trouble to implement this. I get that switching to a dictionary would be good if we added more options. However, we are currently not adding any options, are we? My preference would be to hold off on making a switch to dictionary until we actually need it (because we're adding options). It is unclear to me what those options will be (all other postMessage APIs do not require any options). |
The current pull request is the later (transfer inside dictionary). If you see the user activation proposal that will be the dictionary entry we wish to add. But we've disassociated it from this post message change. |
If we do this, all the postMessages should have (any message, optional PostMessageOptions options) form. |
That's fine. I can make pull requests against the other specs tomorrow if we agree on this. |
@domenic I think I got the other spots. WDYT? |
Spec changes were requested and this change matches the spec changes. whatwg/html#3799 BUG=861735 Change-Id: Ie5f33e6d52eb5d904ae3c439ee488ab75a3b514d
Spec changes were requested and this change matches the spec changes. whatwg/html#3799 BUG=861735 Change-Id: Ie5f33e6d52eb5d904ae3c439ee488ab75a3b514d
Spec changes were requested and this change matches the spec changes. whatwg/html#3799 BUG=861735 Change-Id: Ie5f33e6d52eb5d904ae3c439ee488ab75a3b514d Reviewed-on: https://chromium-review.googlesource.com/1151388 Commit-Queue: Dave Tapuska <dtapuska@chromium.org> Reviewed-by: Mustaq Ahmed <mustaq@chromium.org> Cr-Commit-Position: refs/heads/master@{#579152}
Spec changes were requested and this change matches the spec changes. whatwg/html#3799 BUG=861735 Change-Id: Ie5f33e6d52eb5d904ae3c439ee488ab75a3b514d Reviewed-on: https://chromium-review.googlesource.com/1151388 Commit-Queue: Dave Tapuska <dtapuska@chromium.org> Reviewed-by: Mustaq Ahmed <mustaq@chromium.org> Cr-Commit-Position: refs/heads/master@{#579152}
Spec changes were requested and this change matches the spec changes. whatwg/html#3799 BUG=861735 Change-Id: Ie5f33e6d52eb5d904ae3c439ee488ab75a3b514d Reviewed-on: https://chromium-review.googlesource.com/1151388 Commit-Queue: Dave Tapuska <dtapuska@chromium.org> Reviewed-by: Mustaq Ahmed <mustaq@chromium.org> Cr-Commit-Position: refs/heads/master@{#579152}
…MessageOptions, a=testonly Automatic update from web-platform-testsMove the transfer object into WindowPostMessageOptions Spec changes were requested and this change matches the spec changes. whatwg/html#3799 BUG=861735 Change-Id: Ie5f33e6d52eb5d904ae3c439ee488ab75a3b514d Reviewed-on: https://chromium-review.googlesource.com/1151388 Commit-Queue: Dave Tapuska <dtapuska@chromium.org> Reviewed-by: Mustaq Ahmed <mustaq@chromium.org> Cr-Commit-Position: refs/heads/master@{#579152} -- wpt-commits: defe19d91c59decb804193cbc0137e6a8465367a wpt-pr: 12212
…MessageOptions, a=testonly Automatic update from web-platform-testsMove the transfer object into WindowPostMessageOptions Spec changes were requested and this change matches the spec changes. whatwg/html#3799 BUG=861735 Change-Id: Ie5f33e6d52eb5d904ae3c439ee488ab75a3b514d Reviewed-on: https://chromium-review.googlesource.com/1151388 Commit-Queue: Dave Tapuska <dtapuska@chromium.org> Reviewed-by: Mustaq Ahmed <mustaq@chromium.org> Cr-Commit-Position: refs/heads/master@{#579152} -- wpt-commits: defe19d91c59decb804193cbc0137e6a8465367a wpt-pr: 12212
The current pull request against the HTML spec is here: whatwg/html#3799 Add a PostMessgeOptions dictionary so that WindowPostMessageOptions is a subclass of that. BUG=861735 Change-Id: I66c6990de82b0d74efa7f31240b63d19d074945d Reviewed-on: https://chromium-review.googlesource.com/1165954 Reviewed-by: David Bokan <bokan@chromium.org> Commit-Queue: Dave Tapuska <dtapuska@chromium.org> Cr-Commit-Position: refs/heads/master@{#581417}
The current pull request against the HTML spec is here: whatwg/html#3799 Rename the RuntimeEnabledFeature from WindowPostMessageOptions to PostMessageOptions BUG=861735 Change-Id: Ia7980a85c10535f6d531c87f3790efcf6ed2d54d
In anticipation of adding a few more overloads to the postMessage APIs in whatwg/html#3799 ensure that the code doesn't use the PostMessage extended attribute. Since the extended attribute doesn't support overloading it was decided that these implementations should really be reading the transferable argument themselves not in the bindings code. Make two versions one that is used by service workers that causes transferables to be copied and another that is used by workers, and message_ports that treat them as moveable types. BUG=861735 Change-Id: I82720aae73a1285d74b5c8f10b244c719290e2d9 Reviewed-on: https://chromium-review.googlesource.com/1165835 Commit-Queue: Dave Tapuska <dtapuska@chromium.org> Reviewed-by: Hitoshi Yoshida <peria@chromium.org> Reviewed-by: Yuki Shiino <yukishiino@chromium.org> Reviewed-by: Kentaro Hara <haraken@chromium.org> Cr-Commit-Position: refs/heads/master@{#582127}
The current pull request against the HTML spec is here: whatwg/html#3799 Rename the RuntimeEnabledFeature from WindowPostMessageOptions to PostMessageOptions BUG=861735 Change-Id: Ia7980a85c10535f6d531c87f3790efcf6ed2d54d
The current pull request against the HTML spec is here: whatwg/html#3799 Rename the RuntimeEnabledFeature from WindowPostMessageOptions to PostMessageOptions BUG=861735 Change-Id: Ia7980a85c10535f6d531c87f3790efcf6ed2d54d
The current pull request against the HTML spec is here: whatwg/html#3799 Rename the RuntimeEnabledFeature from WindowPostMessageOptions to PostMessageOptions BUG=861735 Change-Id: Ia7980a85c10535f6d531c87f3790efcf6ed2d54d Reviewed-on: https://chromium-review.googlesource.com/1169304 Commit-Queue: Dave Tapuska <dtapuska@chromium.org> Reviewed-by: Marijn Kruisselbrink <mek@chromium.org> Reviewed-by: Kentaro Hara <haraken@chromium.org> Cr-Commit-Position: refs/heads/master@{#582619}
The current pull request against the HTML spec is here: whatwg/html#3799 Rename the RuntimeEnabledFeature from WindowPostMessageOptions to PostMessageOptions BUG=861735 Change-Id: Ia7980a85c10535f6d531c87f3790efcf6ed2d54d Reviewed-on: https://chromium-review.googlesource.com/1169304 Commit-Queue: Dave Tapuska <dtapuska@chromium.org> Reviewed-by: Marijn Kruisselbrink <mek@chromium.org> Reviewed-by: Kentaro Hara <haraken@chromium.org> Cr-Commit-Position: refs/heads/master@{#582619}
The current pull request against the HTML spec is here: whatwg/html#3799 Rename the RuntimeEnabledFeature from WindowPostMessageOptions to PostMessageOptions BUG=861735 Change-Id: Ia7980a85c10535f6d531c87f3790efcf6ed2d54d Reviewed-on: https://chromium-review.googlesource.com/1169304 Commit-Queue: Dave Tapuska <dtapuska@chromium.org> Reviewed-by: Marijn Kruisselbrink <mek@chromium.org> Reviewed-by: Kentaro Hara <haraken@chromium.org> Cr-Commit-Position: refs/heads/master@{#582619}
…and message_port, a=testonly Automatic update from web-platform-testsAdd a PostMessageOptions API to workers and message_port The current pull request against the HTML spec is here: whatwg/html#3799 Rename the RuntimeEnabledFeature from WindowPostMessageOptions to PostMessageOptions BUG=861735 Change-Id: Ia7980a85c10535f6d531c87f3790efcf6ed2d54d Reviewed-on: https://chromium-review.googlesource.com/1169304 Commit-Queue: Dave Tapuska <dtapuska@chromium.org> Reviewed-by: Marijn Kruisselbrink <mek@chromium.org> Reviewed-by: Kentaro Hara <haraken@chromium.org> Cr-Commit-Position: refs/heads/master@{#582619} -- wpt-commits: aaac36358ac55215fbb28d62d235470859001e91 wpt-pr: 12385
…and message_port, a=testonly Automatic update from web-platform-testsAdd a PostMessageOptions API to workers and message_port The current pull request against the HTML spec is here: whatwg/html#3799 Rename the RuntimeEnabledFeature from WindowPostMessageOptions to PostMessageOptions BUG=861735 Change-Id: Ia7980a85c10535f6d531c87f3790efcf6ed2d54d Reviewed-on: https://chromium-review.googlesource.com/1169304 Commit-Queue: Dave Tapuska <dtapuska@chromium.org> Reviewed-by: Marijn Kruisselbrink <mek@chromium.org> Reviewed-by: Kentaro Hara <haraken@chromium.org> Cr-Commit-Position: refs/heads/master@{#582619} -- wpt-commits: aaac36358ac55215fbb28d62d235470859001e91 wpt-pr: 12385
(I came here from Intent to Implement and Ship: PostMessageOptions and WindowPostMessageOptions dictionary for postMessage methods)
@cdumez, the intended addition is https://github.com/dtapuska/useractivation. The relevant bit there is the |
Late reply, but I support this--also glad to see transfer included in the dictionary as param 2. |
…MessageOptions, a=testonly Automatic update from web-platform-testsMove the transfer object into WindowPostMessageOptions Spec changes were requested and this change matches the spec changes. whatwg/html#3799 BUG=861735 Change-Id: Ie5f33e6d52eb5d904ae3c439ee488ab75a3b514d Reviewed-on: https://chromium-review.googlesource.com/1151388 Commit-Queue: Dave Tapuska <dtapuskachromium.org> Reviewed-by: Mustaq Ahmed <mustaqchromium.org> Cr-Commit-Position: refs/heads/master{#579152} -- wpt-commits: defe19d91c59decb804193cbc0137e6a8465367a wpt-pr: 12212 UltraBlame original commit: 5b3dd8affc580fc17e73cfc4ecc554069ae41d71
…MessageOptions, a=testonly Automatic update from web-platform-testsMove the transfer object into WindowPostMessageOptions Spec changes were requested and this change matches the spec changes. whatwg/html#3799 BUG=861735 Change-Id: Ie5f33e6d52eb5d904ae3c439ee488ab75a3b514d Reviewed-on: https://chromium-review.googlesource.com/1151388 Commit-Queue: Dave Tapuska <dtapuskachromium.org> Reviewed-by: Mustaq Ahmed <mustaqchromium.org> Cr-Commit-Position: refs/heads/master{#579152} -- wpt-commits: defe19d91c59decb804193cbc0137e6a8465367a wpt-pr: 12212 UltraBlame original commit: 5b3dd8affc580fc17e73cfc4ecc554069ae41d71
…and message_port, a=testonly Automatic update from web-platform-testsAdd a PostMessageOptions API to workers and message_port The current pull request against the HTML spec is here: whatwg/html#3799 Rename the RuntimeEnabledFeature from WindowPostMessageOptions to PostMessageOptions BUG=861735 Change-Id: Ia7980a85c10535f6d531c87f3790efcf6ed2d54d Reviewed-on: https://chromium-review.googlesource.com/1169304 Commit-Queue: Dave Tapuska <dtapuskachromium.org> Reviewed-by: Marijn Kruisselbrink <mekchromium.org> Reviewed-by: Kentaro Hara <harakenchromium.org> Cr-Commit-Position: refs/heads/master{#582619} -- wpt-commits: aaac36358ac55215fbb28d62d235470859001e91 wpt-pr: 12385 UltraBlame original commit: 8a49d6c83ccbfad19178a65c50d5feccfaa578d3
…and message_port, a=testonly Automatic update from web-platform-testsAdd a PostMessageOptions API to workers and message_port The current pull request against the HTML spec is here: whatwg/html#3799 Rename the RuntimeEnabledFeature from WindowPostMessageOptions to PostMessageOptions BUG=861735 Change-Id: Ia7980a85c10535f6d531c87f3790efcf6ed2d54d Reviewed-on: https://chromium-review.googlesource.com/1169304 Commit-Queue: Dave Tapuska <dtapuskachromium.org> Reviewed-by: Marijn Kruisselbrink <mekchromium.org> Reviewed-by: Kentaro Hara <harakenchromium.org> Cr-Commit-Position: refs/heads/master{#582619} -- wpt-commits: aaac36358ac55215fbb28d62d235470859001e91 wpt-pr: 12385 UltraBlame original commit: 8a49d6c83ccbfad19178a65c50d5feccfaa578d3
…MessageOptions, a=testonly Automatic update from web-platform-testsMove the transfer object into WindowPostMessageOptions Spec changes were requested and this change matches the spec changes. whatwg/html#3799 BUG=861735 Change-Id: Ie5f33e6d52eb5d904ae3c439ee488ab75a3b514d Reviewed-on: https://chromium-review.googlesource.com/1151388 Commit-Queue: Dave Tapuska <dtapuskachromium.org> Reviewed-by: Mustaq Ahmed <mustaqchromium.org> Cr-Commit-Position: refs/heads/master{#579152} -- wpt-commits: defe19d91c59decb804193cbc0137e6a8465367a wpt-pr: 12212 UltraBlame original commit: 5b3dd8affc580fc17e73cfc4ecc554069ae41d71
…and message_port, a=testonly Automatic update from web-platform-testsAdd a PostMessageOptions API to workers and message_port The current pull request against the HTML spec is here: whatwg/html#3799 Rename the RuntimeEnabledFeature from WindowPostMessageOptions to PostMessageOptions BUG=861735 Change-Id: Ia7980a85c10535f6d531c87f3790efcf6ed2d54d Reviewed-on: https://chromium-review.googlesource.com/1169304 Commit-Queue: Dave Tapuska <dtapuskachromium.org> Reviewed-by: Marijn Kruisselbrink <mekchromium.org> Reviewed-by: Kentaro Hara <harakenchromium.org> Cr-Commit-Position: refs/heads/master{#582619} -- wpt-commits: aaac36358ac55215fbb28d62d235470859001e91 wpt-pr: 12385 UltraBlame original commit: 8a49d6c83ccbfad19178a65c50d5feccfaa578d3
https://bugs.webkit.org/show_bug.cgi?id=191028 Reviewed by Alex Christensen. LayoutTests/imported/w3c: Rebaseline WPT tests now that we have more passes. * web-platform-tests/html/browsers/origin/cross-origin-objects/cross-origin-objects-expected.txt: * web-platform-tests/html/browsers/windows/document-access/document_access_parent_access.tentative-expected.txt: * web-platform-tests/service-workers/service-worker/clients-matchall-frozen.https-expected.txt: * web-platform-tests/service-workers/service-worker/postmessage.https-expected.txt: * web-platform-tests/webmessaging/message-channels/dictionary-transferrable-expected.txt: * web-platform-tests/webmessaging/message-channels/user-activation.tentative-expected.txt: * web-platform-tests/webmessaging/postMessage_MessagePorts_xsite.sub.window-expected.txt: * web-platform-tests/webmessaging/with-options/host-specific-origin-expected.txt: * web-platform-tests/webmessaging/with-options/message-channel-transferable-expected.txt: * web-platform-tests/webmessaging/with-options/no-target-origin-expected.txt: * web-platform-tests/webmessaging/with-options/null-transfer-expected.txt: * web-platform-tests/webmessaging/with-options/one-arg-expected.txt: * web-platform-tests/webmessaging/with-options/slash-origin-expected.txt: * web-platform-tests/webmessaging/with-options/undefined-transferable-expected.txt: * web-platform-tests/webmessaging/with-options/unknown-parameter-expected.txt: * web-platform-tests/webmessaging/without-ports/008-expected.txt: * web-platform-tests/webmessaging/worker_postMessage_user_activation.tentative-expected.txt: * web-platform-tests/workers/interfaces/DedicatedWorkerGlobalScope/postMessage/second-argument-dictionary-expected.txt: * web-platform-tests/workers/interfaces/DedicatedWorkerGlobalScope/postMessage/second-argument-null-expected.txt: Source/WebCore: Implement PostMessageOptions dictionary parameter for postMessage: - whatwg/html#3799 - w3c/ServiceWorker#1344 Blink and Gecko already support this. No new tests, rebaselined existing tests. * CMakeLists.txt: * DerivedSources-input.xcfilelist: * DerivedSources-output.xcfilelist: * DerivedSources.make: * Headers.cmake: * Sources.txt: * WebCore.xcodeproj/project.pbxproj: * dom/MessagePort.cpp: (WebCore::MessagePort::postMessage): * dom/MessagePort.h: * dom/MessagePort.idl: * page/DOMWindow.cpp: (WebCore::DOMWindow::postMessage): * page/DOMWindow.h: (WebCore::WindowPostMessageOptions::WindowPostMessageOptions): * page/DOMWindow.idl: * page/PostMessageOptions.h: Copied from Source/WebCore/workers/service/ServiceWorkerClient.idl. (WebCore::PostMessageOptions::PostMessageOptions): * page/PostMessageOptions.idl: Copied from Source/WebCore/workers/service/ServiceWorkerClient.idl. * workers/DedicatedWorkerGlobalScope.cpp: (WebCore::DedicatedWorkerGlobalScope::postMessage): * workers/DedicatedWorkerGlobalScope.h: * workers/DedicatedWorkerGlobalScope.idl: * workers/Worker.cpp: (WebCore::Worker::postMessage): * workers/Worker.h: * workers/Worker.idl: * workers/service/ServiceWorker.cpp: (WebCore::ServiceWorker::postMessage): * workers/service/ServiceWorker.h: * workers/service/ServiceWorker.idl: * workers/service/ServiceWorkerClient.cpp: (WebCore::ServiceWorkerClient::postMessage): * workers/service/ServiceWorkerClient.h: * workers/service/ServiceWorkerClient.idl: LayoutTests: Update a few existing tests due to the behavior change. * TestExpectations: * fast/dom/Window/post-message-crash.html: * fast/events/message-port-multi-expected.txt: * fast/events/resources/message-port-multi.js: * fast/workers/resources/worker-context-thread-multi-port.js: * fast/workers/resources/worker-multi-port.js: * fast/workers/worker-multi-port-expected.txt: * http/tests/security/postMessage/target-origin-expected.txt: * platform/mac-wk1/imported/w3c/web-platform-tests/html/dom/idlharness.https-expected.txt: * platform/mac-wk2/imported/w3c/web-platform-tests/html/dom/idlharness.https-expected.txt: git-svn-id: http://svn.webkit.org/repository/webkit/trunk@253497 268f45cc-cd09-0410-ab3c-d52691b4dbfc
It is not possible to specify additional options to the postMessage method. I propose that we should change it so that it takes a dictionary.
Proposal from: https://github.com/dtapuska/useractivation
The text was updated successfully, but these errors were encountered: