Bug 73054

Summary: [V8][Chromium] Add list of transferred ArrayBuffers to SerializedScriptValue::create
Product: WebKit Reporter: Dmitry Lomov <dslomov>
Component: WebCore JavaScriptAssignee: Dmitry Lomov <dslomov>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, barraclough, dglazkov, japhet, levin, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 66578    
Attachments:
Description Flags
The fix
levin: review-
CR feedback
levin: review+
Final
webkit.review.bot: commit-queue-
Rebaselined tests none

Description Dmitry Lomov 2011-11-23 15:22:07 PST
SerializedScriptValue::create needs a list of ArrayBuffers from transfer list to implement their transfer.
Comment 1 Dmitry Lomov 2011-11-23 15:36:14 PST
Created attachment 116445 [details]
The fix
Comment 2 WebKit Review Bot 2011-11-23 15:39:43 PST
Attachment 116445 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/bindings/v8/V8Utilities.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/bindings/v8/custom/V8WorkerCustom.cpp:38:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 David Levin 2011-11-23 15:52:29 PST
Comment on attachment 116445 [details]
The fix

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

Just a few comments. I think this will likely be ok with these changes, but I need to take more time to look at it to be sure.

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1992
> +                                                                MessagePortArray* messagePorts, ArrayBufferArray* arrayBuffers,

arrayBuffers is unused. I guess that is next?

Anyway omit the var name for now to avoid build errors on some platforms.

> Source/WebCore/bindings/v8/SerializedScriptValue.h:39
> +class ArrayBuffer;

I think in this one case the contents of the namespace is indented -- double check the style guide pls. (Yes stylebot will complain.)

> Source/WebCore/bindings/v8/V8Utilities.cpp:84
> +bool extractTransferables(v8::Local<v8::Value> value, MessagePortArray& ports, ArrayBufferArray& arrayBuffers)

make it static since it is local to a file.

>> Source/WebCore/bindings/v8/custom/V8WorkerCustom.cpp:38
>> +#include "ArrayBuffer.h"
> 
> Alphabetical sorting problem.  [build/include_order] [4]

This seems like an ok error to have.
Comment 4 Dmitry Lomov 2011-11-23 15:59:07 PST
(In reply to comment #3)
> (From update of attachment 116445 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=116445&action=review
> 
> Just a few comments. I think this will likely be ok with these changes, but I need to take more time to look at it to be sure.
> 
> > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1992
> > +                                                                MessagePortArray* messagePorts, ArrayBufferArray* arrayBuffers,
> 
> arrayBuffers is unused. I guess that is next?
Yes it is next
> 
> Anyway omit the var name for now to avoid build errors on some platforms.
Done

> 
> > Source/WebCore/bindings/v8/SerializedScriptValue.h:39
> > +class ArrayBuffer;
> 
> I think in this one case the contents of the namespace is indented -- double check the style guide pls. (Yes stylebot will complain.)

Checked the style guide - no exceptions.

> 
> > Source/WebCore/bindings/v8/V8Utilities.cpp:84
> > +bool extractTransferables(v8::Local<v8::Value> value, MessagePortArray& ports, ArrayBufferArray& arrayBuffers)
> 
> make it static since it is local to a file.

It is not local.

> 
> >> Source/WebCore/bindings/v8/custom/V8WorkerCustom.cpp:38
> >> +#include "ArrayBuffer.h"
> > 
> > Alphabetical sorting problem.  [build/include_order] [4]
> 
> This seems like an ok error to have.
Comment 5 Dmitry Lomov 2011-11-23 16:01:32 PST
Created attachment 116446 [details]
CR feedback
Comment 6 David Levin 2011-11-23 17:21:25 PST
Comment on attachment 116446 [details]
CR feedback

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

Last round.

> Source/WebCore/bindings/v8/V8Utilities.cpp:41
> +#include <v8.h>

< after " (sorry I wasn't clear about this)

> Source/WebCore/bindings/v8/V8Utilities.cpp:121
> +            ports.append(V8MessagePort::toNative(v8::Handle<v8::Object>::Cast(transferrable)));

Would be nice to move "v8::Handle<v8::Object>::Cast(transferrable)" out of the clauses.

> Source/WebCore/bindings/v8/V8Utilities.h:90
> +    bool getMessagePortArray(v8::Local<v8::Value>, MessagePortArray&);

This function is only used for the current postMessage support, right? (Might be nice to have a small comment to that affect.)

Who calls it anymore in fact?

> Source/WebCore/bindings/v8/custom/V8MessagePortCustom.cpp:56
> +        SerializedScriptValue::create(args[0],

I wish that we didn't have to do this same thing at every callsite. Should doTransfer be a parameter?

> Source/WebCore/bindings/v8/custom/V8WorkerCustom.cpp:63
> +                                      doTransfer ? &ports : 0, doTransfer ? &arrayBuffers : 0,

Might be nice to put the &arrayBuffer arg on a separate line as the current state makes it much harder to read quickly.
Comment 7 David Levin 2011-11-23 17:48:18 PST
Comment on attachment 116446 [details]
CR feedback

Only nits/preferences are left, so r+. Please consider addressing some of them.
Comment 8 Dmitry Lomov 2011-11-23 18:21:56 PST
(In reply to comment #6)
> (From update of attachment 116446 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=116446&action=review
> 
> Last round.
> 
> > Source/WebCore/bindings/v8/V8Utilities.cpp:41
> > +#include <v8.h>
> 
> < after " (sorry I wasn't clear about this)

Done.

> 
> > Source/WebCore/bindings/v8/V8Utilities.cpp:121
> > +            ports.append(V8MessagePort::toNative(v8::Handle<v8::Object>::Cast(transferrable)));
> 
> Would be nice to move "v8::Handle<v8::Object>::Cast(transferrable)" out of the clauses.

Can't. The conditions in ifs determine validity of the cast. 
> 
> > Source/WebCore/bindings/v8/V8Utilities.h:90
> > +    bool getMessagePortArray(v8::Local<v8::Value>, MessagePortArray&);
> 
> This function is only used for the current postMessage support, right? (Might be nice to have a small comment to that affect.)
> 
> Who calls it anymore in fact?

It is used elsewhere (OptionsObject and some other places)
> 
> > Source/WebCore/bindings/v8/custom/V8MessagePortCustom.cpp:56
> > +        SerializedScriptValue::create(args[0],
> 
> I wish that we didn't have to do this same thing at every callsite. Should doTransfer be a parameter?

Well, I want to keep it flexible for now (given the way specs are atm - message port transfer is in "ready for implementation" state, while array buffer transfer is a "strawman" - I think we will be in the state where postMessage allow transferring the former but not the latter)
> 
> > Source/WebCore/bindings/v8/custom/V8WorkerCustom.cpp:63
> > +                                      doTransfer ? &ports : 0, doTransfer ? &arrayBuffers : 0,
> 
> Might be nice to put the &arrayBuffer arg on a separate line as the current state makes it much harder to read quickly.

Done
Comment 9 Dmitry Lomov 2011-11-23 18:29:32 PST
Created attachment 116464 [details]
Final
Comment 10 WebKit Review Bot 2011-11-23 18:31:21 PST
Attachment 116464 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/bindings/v8/custom/V8WorkerCustom.cpp:38:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 WebKit Review Bot 2011-11-23 19:08:50 PST
Comment on attachment 116464 [details]
Final

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

New failing tests:
fast/events/constructors/message-event-constructor.html
fast/dom/Window/window-postmessage-args.html
Comment 12 Dmitry Lomov 2011-11-23 20:33:20 PST
Created attachment 116476 [details]
Rebaselined tests

David: I took the liberty to cq+ rebaseline unreviewed, please yell if that is not ok with you.
Comment 13 WebKit Review Bot 2011-11-23 20:36:12 PST
Attachment 116476 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1

Source/WebCore/bindings/v8/custom/V8WorkerCustom.cpp:38:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 14 WebKit Review Bot 2011-11-23 21:14:42 PST
Comment on attachment 116476 [details]
Rebaselined tests

Clearing flags on attachment: 116476

Committed r101118: <http://trac.webkit.org/changeset/101118>
Comment 15 WebKit Review Bot 2011-11-23 21:14:48 PST
All reviewed patches have been landed.  Closing bug.