Skip to content
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 postMessage override that provides WindowPostMessageOptions. #3800

Merged
merged 4 commits into from
Oct 26, 2018

Conversation

dtapuska
Copy link
Contributor

@dtapuska dtapuska commented Jul 5, 2018

@dtapuska
Copy link
Contributor Author

dtapuska commented Jul 5, 2018

@domenic WDYT?

Tentative WPT tests are here for review: https://chromium-review.googlesource.com/c/chromium/src/+/1118850

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some issues, but the basic idea is clear. I'll follow up with a more general comment in #3799.

I wonder whether we should update all of the spec examples to use this overload. If we believe it to be the right pattern in general, that might be a good idea. You can Ctrl+F for postMessage( in the rendered output to find a lot.

On the other hand, maybe spec examples should be a bit more conservative, not using newer, less-supported features.

source Outdated
callback <dfn>FrameRequestCallback</dfn> = void (<span>DOMHighResTimeStamp</span> time);

dictionary <dfn>WindowPostMessageOptions</dfn>
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: curly on previous line

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

source Outdated
{
USVString targetOrigin = "/";
};
</pre>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: no \n before closing tag

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

source Outdated
@@ -95391,6 +95397,9 @@ function receiver(e) {
<p>Objects listed in <var>transfer</var> are transferred, not just cloned, meaning that
they are no longer usable on the sending side.</p>

<p>Target origin is specified as a member of <var>options</var>. If options is not provided
targetOrigin of the dictionary is initialized to "<code data-x="">/</code>".</p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would phrase this as:

A target origin may be specified using the <code data-x="">targetOrigin</code> member of <var>options</var>. If not provided, it defaults to "<code data-x="">/</code>". This restricts the message to same-origin targets only.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

source Outdated
@@ -95391,6 +95397,9 @@ function receiver(e) {
<p>Objects listed in <var>transfer</var> are transferred, not just cloned, meaning that
they are no longer usable on the sending side.</p>

<p>Target origin is specified as a member of <var>options</var>. If options is not provided
targetOrigin of the dictionary is initialized to "<code data-x="">/</code>".</p>

<p>If the origin of the target window doesn't match the given origin, the message is discarded,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should change "given" to "target" here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

source Outdated
@@ -95391,6 +95397,9 @@ function receiver(e) {
<p>Objects listed in <var>transfer</var> are transferred, not just cloned, meaning that
they are no longer usable on the sending side.</p>

<p>Target origin is specified as a member of <var>options</var>. If options is not provided
targetOrigin of the dictionary is initialized to "<code data-x="">/</code>".</p>

<p>If the origin of the target window doesn't match the given origin, the message is discarded,
to avoid information leakage. To send the message to the target regardless of origin, set the
target origin to "<code data-x="">*</code>". To restrict the message to same-origin targets only,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can remove the "To restrict the message..." sentence now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

source Outdated

<p>This is an alternate version of postMessage where the target origin is specified as a parameter.
Calling <code data-x="">window.postMessage(message, target, transfer);</code> is equivalent
to <code data-x="">window.postMessage(message, transfer, {targetOrigin: target});</code>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nits:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

source Outdated
@@ -95417,7 +95435,7 @@ function receiver(e) {
<div w-nodev>

<p>The <dfn><code data-x="dom-window-postMessage">postMessage(<var>message</var>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You lost the definition for postMessage(message, targetOrigin, transfer). It will need a second data-x, since it's a second method.

See https://html.spec.whatwg.org/#dom-document-open for the general pattern here. You'll need to define something like "window message-posting steps" which takes message, transfer, targetOrigin, then define the two methods each of which call the window message-posting steps.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

source Outdated
@@ -95433,6 +95451,8 @@ function receiver(e) {
window, in same-origin cases. See discussion at
https://github.com/whatwg/html/issues/1542#issuecomment-233502636 -->

<li><p>Let <var>targetOrigin</var> be <var>options</var>'s targetOrigin.</p></li>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use either "the value of options's targetOrigin member" or (newer, rarer style) options["targetOrigin"].

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@domenic domenic added addition/proposal New features or enhancements needs implementer interest Moving the issue forward requires implementers to express interest labels Jul 10, 2018
@dtapuska dtapuska force-pushed the post_message branch 2 times, most recently from a2fdfb3 to c9bf3a9 Compare July 24, 2018 16:25
@dtapuska
Copy link
Contributor Author

Ok; transfer moved to the dictionary. It digresses from the consistency we were after but I think the argument to put it in the dictionary is reasonable.

@wanderview
Copy link
Member

wanderview commented Jul 31, 2018

If we're updating all versions of postMessage(), then please also update ServiceWorker.postMessage() and Client.postMessage() in the service worker spec.

https://w3c.github.io/ServiceWorker/#service-worker-postmessage
https://w3c.github.io/ServiceWorker/#client-postmessage

@dtapuska
Copy link
Contributor Author

Yes that is the intent. We have to agree on it first though.. See #3799 where the main discussion is happening.

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lots of editorial work to do, but normative aspects all look good, thanks!

source Outdated
<var>options</var>. If not provided, it defaults to "<code data-x="">/</code>". This restricts
the message to same-origin targets only.</p>

<p>This is an alternate version of postMessage where the target origin is specified as a
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stray paragraph; delete

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

source Outdated
@@ -95356,29 +95361,41 @@ function receiver(e) {

<dl class="domintro">

<dt><var>window</var> . <code subdfn data-x="dom-window-postMessage">postMessage</code>(<var>message</var>, <var>targetOrigin</var> [, <var>transfer</var> ] )</dt>
<dt><var>window</var> . <code subdfn data-x="dom-window-postMessage">postMessage</code>(<var>message</var>[, <var>options</var> ])</dt>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: space between ] and ), I guess? Spec is inconsistent, but we should be consistent within this block, at least. I'm also OK with deleting the space from the below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

source Outdated

<dd>

<p>This is an alternate version of postMessage where the target origin is specified as a
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

postMessage -> <code data-x="dom-window-postMessage-options">postMessage()</code>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

source Outdated
<p>This is an alternate version of postMessage where the target origin is specified as a
parameter. Calling <code data-x="">window.postMessage(message, target, transfer)</code> is
equivalent to <code data-x="">window.postMessage(message, transfer, {targetOrigin:
target})</code>.</p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs updating since transfer moved into the options dictionary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

source Outdated
<p>The <dfn><code data-x="dom-window-postMessage">postMessage(<var>message</var>,
<var>targetOrigin</var>, <var>transfer</var>)</code></dfn> method, when invoked on a
<code>Window</code> object must run the following steps:</p>
<p>The <dfn>window post message steps</dfn>, given a <var>targetWindow</var>, <var>message</var>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comma after message and after options

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

source Outdated
<li><p>Let <var>targetPort</var> be the port with which this <code>MessagePort</code> is
entangled, if any; otherwise let it be null.</p></li>

<li><p>Let <var>options</var> be a new instance of <code>PostMessageOptions</code>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same simplification as above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

source Outdated
@@ -96914,6 +97012,13 @@ interface <dfn>DedicatedWorkerGlobalScope</dfn> : <span>WorkerGlobalScope</span>
<var>dedicatedWorkerGlobal</var>. <var>transfer</var> can be passed as a list of objects that are
to be transferred rather than cloned.</dd>

<dt><var>dedicatedWorkerGlobal</var> . <code subdfn
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same dt/dt/dd structure as before.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

source Outdated
and <dfn><code data-x="dom-DedicatedWorkerGlobalScope-postMessage-options">postMessage()</code></dfn>
methods on <code>DedicatedWorkerGlobalScope</code> objects act as if, when invoked, it
immediately invoked the respective <span data-x="dom-MessagePort-postMessage">postMessage</span>
and <span data-x="dom-MessagePort-postMessage-options">postMessage (with options)</span> on the port,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use code, not span.

Insert parameter lists for all four methods.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

source Outdated
@@ -97653,6 +97761,12 @@ enum <dfn>WorkerType</dfn> { "classic", "module" };
<dd>Clones <var>message</var> and transmits it to <var>worker</var>'s global environment.
<var>transfer</var> can be passed as a list of objects that are to be transferred rather than
cloned.</dd>

<dt><var>worker</var> . <code subdfn data-x="dom-Worker-postMessage-options">postMessage</code>(<var>message</var> [, <var>options</var> ])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same dt/dt/dd structure as before.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

source Outdated
@@ -97668,10 +97782,12 @@ enum <dfn>WorkerType</dfn> { "classic", "module" };
<p>All messages received by that port must immediately be retargeted at the <code>Worker</code>
object.</p>

<p>The <dfn><code data-x="dom-Worker-postMessage">postMessage()</code></dfn> method on
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use span, not code; add parameter lists to all four.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

dtapuska added a commit to dtapuska/ServiceWorker that referenced this pull request Aug 10, 2018
@foolip
Copy link
Member

foolip commented Aug 17, 2018

Even though it shows up in the thread, I had a hard time spotting the corresponding changes for Service Workers. Here they are: w3c/ServiceWorker#1344

@dtapuska dtapuska mentioned this pull request Sep 6, 2018
@sideshowbarker sideshowbarker added normative change impacts documentation Used by documentation communities, such as MDN, to track changes that impact documentation labels Sep 11, 2018
source Outdated
callback <dfn>FrameRequestCallback</dfn> = void (<span>DOMHighResTimeStamp</span> time);

dictionary <dfn>WindowPostMessageOptions</dfn> : <span>PostMessageOptions</span> {
USVString targetOrigin = "/";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we name this simply "origin" for convenience?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I think the explicitness is good. This isn't "the origin of a posted message", as origin would indicate, it's "the target origin to which you are posting the message".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mkay, I don't like that it gets so long.

"<var>transfer</var>" → <var>transfer</var> ]».</p></li>

<li><p>Run the <span>window post message steps</span> providing
<var>targetWindow</var>, <var>message</var> and <var>options</var>.</p></li>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oxford comma.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

<li><p>Let <var>targetWindow</var> be this <code>Window</code> object.</p></li>

<li><p>Run the <span>window post message steps</span> providing <var>targetWindow</var>,
<var>message</var> and <var>options</var>.</p></li>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oxford comma.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@domenic domenic removed the needs implementer interest Moving the issue forward requires implementers to express interest label Sep 28, 2018
@domenic
Copy link
Member

domenic commented Sep 28, 2018

Removing "needs implementer interest" per #3799 (comment). Also please rebase on master, I guess there have been conflicts.

@dtapuska
Copy link
Contributor Author

dtapuska commented Oct 2, 2018

Commas addressed PTAL.

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if for the non-Window postMessage() methods we should mark the transfer argument as non-optional and non-defaulting. It probably doesn't matter, but it seems slightly cleaner to make overloading pick the dictionary-based version as much as possible.

I also pushed some formatting nits.

Are the tests updated as well? The link in the comment following OP is rather outdated.

@domenic
Copy link
Member

domenic commented Oct 26, 2018

Tests ended up in https://github.com/web-platform-tests/wpt/tree/5a792b6bf4e0d39f2b65301b592dffffa8bd9b15/webmessaging/with-options. I'll merge this, and file an issue to make them un-.tentative.

@domenic
Copy link
Member

domenic commented Oct 26, 2018

@dtapuska did you, or can you, file issues on the other implementers?

dtapuska added a commit to dtapuska/ServiceWorker that referenced this pull request Oct 29, 2018
@dtapuska dtapuska deleted the post_message branch October 29, 2018 15:58
dtapuska added a commit to dtapuska/ServiceWorker that referenced this pull request Mar 7, 2019
mfalken pushed a commit to w3c/ServiceWorker that referenced this pull request Mar 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition/proposal New features or enhancements impacts documentation Used by documentation communities, such as MDN, to track changes that impact documentation normative change
Development

Successfully merging this pull request may close these issues.

6 participants