Bug 74038 – [V8][Chromium] Support legacy argument order in window.postMessage/window.webkitPostMessage

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Dmitry Lomov]

Current spec for window.postMessage states the following argument list for postMessage:
postMessage(msg, targetOrigin [, transferables])
However, legacy implementation in WebKit used a different order:
postMessage(msg [, transferables], targetOrigin).

Changing this in https://bugs.webkit.org/show_bug.cgi?id=73589 was a backwards-incompatible change.

[Dmitry Lomov]

Link to the current (editor's draft) spec: http://dev.w3.org/html5/postmsg/#posting-messages

[Dmitry Lomov]

Created attachment 118292 [details]
Fix

This fix accepts legacy argument order (postMessage(msg, transferables, targetOrigin)) when third argument is a string.

This is not 100% backward-compatible (previous version was also working for objects whose toString returned a valid target origin), but I think it is close enough so as not to break the Internet.

[Andrey Kosyakov]

Comment on attachment 118292 [details]
Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=118292&action=review

Why are we doing this for V8 bindings only?

> Source/WebCore/bindings/v8/custom/V8DOMWindowCustom.cpp:318
> +            if (isLegacyTargetOriginDesignation(args[2])) {

It might be worth issuing a deprecation warning message to the console when the compatibility logic kicks in.

[Dmitry Lomov]

(In reply to comment #3)
> (From update of attachment 118292 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=118292&action=review
> 
> Why are we doing this for V8 bindings only?

JSC patch will follow (https://bugs.webkit.org/show_bug.cgi?id=73691).

> 
> > Source/WebCore/bindings/v8/custom/V8DOMWindowCustom.cpp:318
> > +            if (isLegacyTargetOriginDesignation(args[2])) {
> 
> It might be worth issuing a deprecation warning message to the console when the compatibility logic kicks in.

How to do that? Any examples? 
Will issuing message to the console cause perf regression?

[Andrey Kosyakov]

> > Why are we doing this for V8 bindings only?
> 
> JSC patch will follow (https://bugs.webkit.org/show_bug.cgi?id=73691).

I think it logically belongs to a single patch, along with DOMWindow.idl change.

> > It might be worth issuing a deprecation warning message to the console when the compatibility logic kicks in.
> 
> How to do that? Any examples? 

Check out this:

http://www.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/dom/UIEvent.cpp&exact_package=chromium&l=108

Or this:
http://www.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/webaudio/AudioContext.cpp&exact_package=chromium&ct=rc&cd=4&l=392

> Will issuing message to the console cause perf regression?

Not sure if this is critical, but if it is, we could limit the number of time we issue the message.

[Dmitry Lomov]

(In reply to comment #5)
> > > Why are we doing this for V8 bindings only?
> > 
> > JSC patch will follow (https://bugs.webkit.org/show_bug.cgi?id=73691).
> 
> I think it logically belongs to a single patch, along with DOMWindow.idl change.
> 
> > > It might be worth issuing a deprecation warning message to the console when the compatibility logic kicks in.
> > 
> > How to do that? Any examples? 
> 
> Check out this:
> 
> http://www.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/dom/UIEvent.cpp&exact_package=chromium&l=108

Thank you - looks really simple. Will add.

[Dmitry Lomov]

(In reply to comment #6)
> (In reply to comment #5)
> > > > Why are we doing this for V8 bindings only?
> > > 
> > > JSC patch will follow (https://bugs.webkit.org/show_bug.cgi?id=73691).
> > 
> > I think it logically belongs to a single patch, along with DOMWindow.idl change.
> > 
> > > > It might be worth issuing a deprecation warning message to the console when the compatibility logic kicks in.
> > > 
> > > How to do that? Any examples? 
> > 
> > Check out this:
> > 
> > http://www.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/dom/UIEvent.cpp&exact_package=chromium&l=108
> 
> Thank you - looks really simple. Will add.

On second thoughts, this will cause deprecation warnings in (shipping) chrome extensions, so coordinated changes in them will be required. In the interest of making this patch as small as possible, this will be done in a separate patch: https://bugs.webkit.org/show_bug.cgi?id=74045

[Dmitry Lomov]

(In reply to comment #6)
> (In reply to comment #5)
> > > > Why are we doing this for V8 bindings only?
> > > 
> > > JSC patch will follow (https://bugs.webkit.org/show_bug.cgi?id=73691).
> > 
> > I think it logically belongs to a single patch, along with DOMWindow.idl change.
> > 

I do not think this is necessary. JSC bindings as in the tree today implement legacy behavior, following DOMWindow.idl. Let us keep patches small.

[WebKit Review Bot]

Comment on attachment 118292 [details]
Fix

Clearing flags on attachment: 118292

Committed r102317: <http://trac.webkit.org/changeset/102317>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.