Bug 73054 – [V8][Chromium] Add list of transferred ArrayBuffers to SerializedScriptValue::create

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Dmitry Lomov]

SerializedScriptValue::create needs a list of ArrayBuffers from transfer list to implement their transfer.

[Dmitry Lomov]

Created attachment 116445 [details]
The fix

[WebKit Review Bot]

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.

[David Levin]

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.

[Dmitry Lomov]

(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.

[Dmitry Lomov]

Created attachment 116446 [details]
CR feedback

[David Levin]

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.

[David Levin]

Comment on attachment 116446 [details]
CR feedback

Only nits/preferences are left, so r+. Please consider addressing some of them.

[Dmitry Lomov]

(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

[Dmitry Lomov]

Created attachment 116464 [details]
Final

[WebKit Review Bot]

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.

[WebKit Review Bot]

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

[Dmitry Lomov]

Created attachment 116476 [details]
Rebaselined tests

David: I took the liberty to cq+ rebaseline unreviewed, please yell if that is not ok with you.

[WebKit Review Bot]

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.

[WebKit Review Bot]

Comment on attachment 116476 [details]
Rebaselined tests

Clearing flags on attachment: 116476

Committed r101118: <http://trac.webkit.org/changeset/101118>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.