[webkit-reviews] review denied: [Bug 73054] [V8][Chromium] Add list of transferred ArrayBuffers to SerializedScriptValue::create : [Attachment 116445] The fix

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Dmitry Lomov <dslomov at google.com>'s
request for review:
Bug 73054: [V8][Chromium] Add list of transferred ArrayBuffers to
SerializedScriptValue::create
https://bugs.webkit.org/show_bug.cgi?id=73054

Attachment 116445: The fix
https://bugs.webkit.org/attachment.cgi?id=116445&action=review

------- Additional Comments from David Levin <levin at chromium.org>
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.