Bug 73589 - [V8][Chromium] Adjust postMessage to the latest "implementation-ready" spec
Summary: [V8][Chromium] Adjust postMessage to the latest "implementation-ready" spec
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:
 
Reported: 2011-12-01 14:19 PST by Dmitry Lomov
Modified: 2011-12-07 20:38 PST (History)
10 users (show)

See Also:


Attachments
Fix (15.38 KB, patch)
2011-12-01 14:22 PST, Dmitry Lomov
levin: review+
webkit.review.bot: commit-queue-
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-01 14:19:11 PST
1. postMessage should support transfer of MessagePorts
2. the order of arguments to Window::postMessage and Window::webkitPostMessage should be (msg, targetOrigin [, transfer])
Comment 1 Dmitry Lomov 2011-12-01 14:22:34 PST
Created attachment 117487 [details]
Fix
Comment 2 David Levin 2011-12-01 16:05:15 PST
Comment on attachment 117487 [details]
Fix

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

> Source/WebCore/bindings/v8/custom/V8DOMWindowCustom.cpp:306
> +            if (!extractTransferables(args[2], portArray, arrayBufferArray))

You could combine with the previous if instead of having
if...
 if ...
Comment 3 WebKit Review Bot 2011-12-02 05:49:21 PST
Comment on attachment 117487 [details]
Fix

Attachment 117487 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10690360

New failing tests:
http/tests/inspector/extensions-headers.html
css2.1/20110323/abspos-containing-block-initial-004b.htm
http/tests/inspector/extensions-ignore-cache.html
inspector/extensions/extensions-audits-api.html
inspector/extensions/extensions-events.html
fast/forms/number/spin-in-multi-column.html
inspector/extensions/extensions-network.html
fast/replaced/width100percent-textarea.html
http/tests/inspector/extensions-network-redirect.html
inspector/extensions/extensions-audits.html
inspector/extensions/extensions-console.html
css2.1/20110323/abspos-containing-block-initial-004d.htm
inspector/extensions/extensions-eval.html
http/tests/inspector/extensions-useragent.html
inspector/extensions/extensions-panel.html
inspector/extensions/extensions-api.html
Comment 4 Dmitry Lomov 2011-12-02 09:38:43 PST
(In reply to comment #3)
> (From update of attachment 117487 [details])
> Attachment 117487 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/10690360
> 
> New failing tests:
> http/tests/inspector/extensions-headers.html
> css2.1/20110323/abspos-containing-block-initial-004b.htm
> http/tests/inspector/extensions-ignore-cache.html
> inspector/extensions/extensions-audits-api.html
> inspector/extensions/extensions-events.html
> fast/forms/number/spin-in-multi-column.html
> inspector/extensions/extensions-network.html
> fast/replaced/width100percent-textarea.html
> http/tests/inspector/extensions-network-redirect.html
> inspector/extensions/extensions-audits.html
> inspector/extensions/extensions-console.html
> css2.1/20110323/abspos-containing-block-initial-004d.htm
> inspector/extensions/extensions-eval.html
> http/tests/inspector/extensions-useragent.html
> inspector/extensions/extensions-panel.html
> inspector/extensions/extensions-api.html

These tests timeout on my machine even without a patch.
Comment 5 Dmitry Lomov 2011-12-02 11:03:28 PST
Landed in http://trac.webkit.org/changeset/101831.
Comment 7 Andrey Kosyakov 2011-12-07 16:28:11 PST
First, this indeed broke all inspector extensions tests (and extensions themselves), as extensions rely on an order of arguments in postMessage(). Doing a code search for postMessage() and paying attention to test results would probably save us from this, but I think this will also break quite a few pages in the internet that rely on postMessage().

Second, the actual order of the arguments, as processed by V8DOMWindowCustom::handlePostMessageCallback() is different from that in DOMWindow.idl -- I find this misleading to say least.

Third, the order of arguments is now different for JSC and V8 bindings -- so getting a code to work in two different WebKit browser is going to be tricky.

I believe the above justifies the roll back for this change.
Comment 8 Dmitry Lomov 2011-12-07 16:37:48 PST
(In reply to comment #7)
> First, this indeed broke all inspector extensions tests (and extensions themselves), as extensions rely on an order of arguments in postMessage(). Doing a code search for postMessage() and paying attention to test results would probably save us from this, but I think this will also break quite a few pages in the internet that rely on postMessage().
> 
> Second, the actual order of the arguments, as processed by V8DOMWindowCustom::handlePostMessageCallback() is different from that in DOMWindow.idl -- I find this misleading to say least.
> 
> Third, the order of arguments is now different for JSC and V8 bindings -- so getting a code to work in two different WebKit browser is going to be tricky.
> 
> I believe the above justifies the roll back for this change.

I am working on a fix that will return the support for legacy argument order.
Comment 9 Dmitry Lomov 2011-12-07 16:44:15 PST
(In reply to comment #8)
> (In reply to comment #7)
> > First, this indeed broke all inspector extensions tests (and extensions themselves), as extensions rely on an order of arguments in postMessage(). Doing a code search for postMessage() and paying attention to test results would probably save us from this, but I think this will also break quite a few pages in the internet that rely on postMessage().
> > 
> > Second, the actual order of the arguments, as processed by V8DOMWindowCustom::handlePostMessageCallback() is different from that in DOMWindow.idl -- I find this misleading to say least.
> > 
> > Third, the order of arguments is now different for JSC and V8 bindings -- so getting a code to work in two different WebKit browser is going to be tricky.
> > 
> > I believe the above justifies the roll back for this change.
> 
> I am working on a fix that will return the support for legacy argument order.

The bug is https://bugs.webkit.org/show_bug.cgi?id=74038