Bug 70658 - [JSC] Implement MessagePort transfer in JSC bindings implementation of webkitPostMessage
Summary: [JSC] Implement MessagePort transfer in JSC bindings implementation of webkit...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dmitry Lomov
URL:
Keywords:
Depends on:
Blocks: 65209
  Show dependency tree / graph
 
Reported: 2011-10-21 15:58 PDT by Dmitry Lomov
Modified: 2011-10-31 14:07 PDT (History)
4 users (show)

See Also:

Attachments
Fix + tests (10.90 KB, patch)
2011-10-24 20:10 PDT, Dmitry Lomov 		no flags Details | Formatted Diff | Diff
CR comments addressed (thanks David!) (10.87 KB, patch)
2011-10-25 11:20 PDT, Dmitry Lomov 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Dmitry Lomov 2011-10-21 15:58:23 PDT 
Corresponding bug for chromium: https://bugs.webkit.org/show_bug.cgi?id=70580
Comment 1 Dmitry Lomov 2011-10-24 20:10:25 PDT 
Created attachment 112296 [details]
Fix + tests
Comment 2 David Levin 2011-10-25 09:56:45 PDT 
Comment on attachment 112296 [details]
Fix + tests

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

Looks good to me (with a few minor nits). (I don't know about the cast static_cast<JSDOMGlobalObject*>(m_exec->lexicalGlobalObject())).


Hopefully Oliver will weigh in on it too!

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:290
> + 

extra blank line.

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:1212
> +            if (!m_messagePorts || index >= m_messagePorts->size()) {

Why not just combine this with the previous if ||...
Comment 3 Dmitry Lomov 2011-10-25 11:19:15 PDT 
(In reply to comment #2)
> (From update of attachment 112296 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=112296&action=review
> 
> Looks good to me (with a few minor nits). (I don't know about the cast static_cast<JSDOMGlobalObject*>(m_exec->lexicalGlobalObject())).

From what I see, the assumption of this cast holds, but an opinion of an experienced JSC engineer is most welcome.
Comment 4 Dmitry Lomov 2011-10-25 11:20:00 PDT 
Created attachment 112364 [details]
CR comments addressed (thanks David!)
Comment 5 David Levin 2011-10-31 10:09:55 PDT 
Comment on attachment 112364 [details]
CR comments addressed (thanks David!)

Looks good. Thanks!
Comment 6 WebKit Review Bot 2011-10-31 14:07:38 PDT 
Comment on attachment 112364 [details]
CR comments addressed (thanks David!)

Clearing flags on attachment: 112364

Committed r98879: <http://trac.webkit.org/changeset/98879>
Comment 7 WebKit Review Bot 2011-10-31 14:07:42 PDT 
All reviewed patches have been landed.  Closing bug.