[webkit-reviews] review denied: [Bug 63644] [v8] Improve performance of typed array set() taking Array : [Attachment 106509] Implement the 'set' method in JS.

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Ulan Degenbaev
<ulan at chromium.org>'s request for review:
Bug 63644: [v8] Improve performance of typed array set() taking Array
https://bugs.webkit.org/show_bug.cgi?id=63644

Attachment 106509: Implement the 'set' method in JS.
https://bugs.webkit.org/attachment.cgi?id=106509&action=review

------- Additional Comments from David Levin <levin at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=106509&action=review


Just a few comments. Also thanks to dslomov who helped.

> ChangeLog:19
> +	   *
../../Source/WebCore/bindings/v8/custom/V8ArrayBufferViewCustomScript.js:
Added.

This ChangeLog is messed up. See other entries in this file.

> Source/WebCore/WebCore.gyp/WebCore.gyp:483
> +	     'action_name': 'generateV8ArrayBufferViewCustomScript.h',

Similar items are named like this: generateV8ArrayBufferViewCustomScript

> Source/WebCore/WebCore.gyp/WebCore.gyp:497
> +	     'message': 'Generating V8ArrayBufferViewCustomScript.h from
V8ArrayBuferViewCustomScript.js',

typo: V8ArrayBuferViewCustomScript

> Source/WebCore/bindings/v8/V8BindingScripts.cpp:32
> +namespace WebCore {

add blank line.

> Source/WebCore/bindings/v8/V8BindingScripts.cpp:33
> +void V8BindingScripts::runScriptsInGlobalContext(v8::Handle<v8::Context>
v8Context)

s/runScriptsInGlobalContext/runScript/ ?

> Source/WebCore/bindings/v8/V8BindingScripts.cpp:37
> +			   sizeof(V8ArrayBufferViewCustomScript_js));

indentation is wrong.

> Source/WebCore/bindings/v8/V8BindingScripts.cpp:40
> +}

add blank line.

> Source/WebCore/bindings/v8/V8BindingScripts.h:37
> +    static void runScriptsInGlobalContext(v8::Handle<v8::Context>
v8Context);

param name adds no info. Omit it.

> Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:2
>   * Copyright (C) 2008, 2009 Google Inc. All rights reserved.

Add 2011.

> Source/WebCore/bindings/v8/custom/V8ArrayBufferViewCustomScript.js:34
> +		   var n = source.length;

Please use descriptive variable names. (not "n")

> Source/WebCore/bindings/v8/custom/V8ArrayBufferViewCustomScript.js:41
> +	       } else {

Don't use {} for single line statements.

> LayoutTests/fast/canvas/webgl/array-unit-tests.html:647
> +function testSettingFromArrayWithMinusZeroOffset(type, name) {

This test isn't used.