Bug 70580 - [Chromium] Implement MessagePort transfer in chromium port of webkit
Summary: [Chromium] Implement MessagePort transfer in chromium port of webkit
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: 65209
  Show dependency treegraph
 
Reported: 2011-10-20 21:47 PDT by Dmitry Lomov
Modified: 2011-10-21 17:37 PDT (History)
5 users (show)

See Also:


Attachments
Fix (16.03 KB, patch)
2011-10-20 21:50 PDT, Dmitry Lomov
levin: review+
levin: commit-queue-
Details | Formatted Diff | Diff
Fixes in tests + testdata for JSC (18.44 KB, patch)
2011-10-21 16:00 PDT, 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-10-20 21:47:15 PDT
SerializedScriptValue should support transferring message ports
Comment 1 Dmitry Lomov 2011-10-20 21:50:58 PDT
Created attachment 111901 [details]
Fix
Comment 2 David Levin 2011-10-20 23:16:31 PDT
Comment on attachment 111901 [details]
Fix

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

Looks good (except I just want to understand the wrap issue before I r+).

> LayoutTests/fast/events/resources/message-port-multi.js:58
> +            if(event.ports && event.ports.length > 0 && event.ports[0] === event.data.port)

add space after if

> LayoutTests/fast/events/resources/message-port-multi.js:79
> +            testFailed("Unexpexcted message " + event.data);

typo: Unexpexcted

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1065
> +            uint32_t messagePortIndex;

indent is incorrect here and below

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1801
> +        *object = V8MessagePort::wrap(m_transferredMessagePorts->at(index).get());

It is unintuitive to me that both sides of this use "wrap". (line 598 and 1801). It feels like one of them should be unwrap.
Comment 3 Dmitry Lomov 2011-10-20 23:18:14 PDT
(In reply to comment #2)

> It is unintuitive to me that both sides of this use "wrap". (line 598 and 1801). It feels like one of them should be unwrap.

On both sides we go from MessagePort (webkit object) to corresponding V8 object. Hence the wrap on both ends.
Comment 4 David Levin 2011-10-20 23:19:40 PDT
Comment on attachment 111901 [details]
Fix

ok. Please fix up the nits mentioned before landing. Thanks!
Comment 5 Dmitry Lomov 2011-10-21 16:00:33 PDT
Created attachment 112039 [details]
Fixes in tests + testdata for JSC
Comment 6 WebKit Review Bot 2011-10-21 17:37:23 PDT
Comment on attachment 112039 [details]
Fixes in tests + testdata for JSC

Clearing flags on attachment: 112039

Committed r98175: <http://trac.webkit.org/changeset/98175>
Comment 7 WebKit Review Bot 2011-10-21 17:37:28 PDT
All reviewed patches have been landed.  Closing bug.