Bug 74038 - [V8][Chromium] Support legacy argument order in window.postMessage/window.webkitPostMessage
Summary: [V8][Chromium] Support legacy argument order in window.postMessage/window.web...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dmitry Lomov
URL:
Keywords:
Depends on:
Blocks: 74045
  Show dependency treegraph
 
Reported: 2011-12-07 16:43 PST by Dmitry Lomov
Modified: 2011-12-07 22:59 PST (History)
6 users (show)

See Also:


Attachments
Fix (8.99 KB, patch)
2011-12-07 17:35 PST, Dmitry Lomov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dmitry Lomov 2011-12-07 16:43:53 PST
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.
Comment 1 Dmitry Lomov 2011-12-07 17:09:31 PST
Link to the current (editor's draft) spec: http://dev.w3.org/html5/postmsg/#posting-messages
Comment 2 Dmitry Lomov 2011-12-07 17:35:15 PST
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.
Comment 3 Andrey Kosyakov 2011-12-07 18:00:10 PST
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.
Comment 4 Dmitry Lomov 2011-12-07 18:03:06 PST
(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?
Comment 5 Andrey Kosyakov 2011-12-07 18:14:09 PST
> > 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.
Comment 6 Dmitry Lomov 2011-12-07 18:16:38 PST
(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.
Comment 7 Dmitry Lomov 2011-12-07 18:34:06 PST
(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
Comment 8 Dmitry Lomov 2011-12-07 19:05:34 PST
(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.
Comment 9 WebKit Review Bot 2011-12-07 22:59:27 PST
Comment on attachment 118292 [details]
Fix

Clearing flags on attachment: 118292

Committed r102317: <http://trac.webkit.org/changeset/102317>
Comment 10 WebKit Review Bot 2011-12-07 22:59:32 PST
All reviewed patches have been landed.  Closing bug.