Add primitive WebGL ArrayBuffer() support to d8

Add primitive WebGL ArrayBuffer() support to d8


R=jkummerow@chromium.org
BUG=
TEST=

Committed: http://code.google.com/p/v8/source/detail?r=10389

[danno, 2012-01-11 08:49:44]

PTAL

[Jakob Kummerow, 2012-01-11 10:17:53]

http://codereview.chromium.org/9114050/diff/4001/src/d8.cc
File src/d8.cc (right):

http://codereview.chromium.org/9114050/diff/4001/src/d8.cc#newcode284
src/d8.cc:284: static int convertToInt(Local<Value> value_in, TryCatch*
try_catch) {
Rename to convertToUint?

http://codereview.chromium.org/9114050/diff/4001/src/d8.cc#newcode339
src/d8.cc:339: if (args.Length() > 3 || args.Length() == 0) {
Arguments.operator[] is benign enough to simply return 'undefined' for args[0]
if args.Length == 0, but nevertheless I'd prefer to check for Length() > 0
before doing anything else (you can print a better error message that way, too).

http://codereview.chromium.org/9114050/diff/4001/src/d8.cc#newcode350
src/d8.cc:350: : args[2];
args[2] is optional and could be undefined. As implemented, the code defaults to
0 in that case, which is not correct.

http://codereview.chromium.org/9114050/diff/4001/src/d8.cc#newcode363
src/d8.cc:363:
array->Get(String::New("BYTES_PER_ELEMENT"))->ToInt32()->Int32Value();
My reading of the spec at
http://www.khronos.org/registry/typedarray/specs/latest/#7 is that |array| must
be an ArrayBuffer (and cannot be another kind of TypedArray), so
BYTES_PER_ELEMENTS is always 1, and you'll have to throw an exception if a
TypedArray is passed in.
Further, IIUC, the "length" parameter specifies the new array's length in terms
of number of elements in the new array, so all this conversion-fu is
unnecessary.

http://codereview.chromium.org/9114050/diff/4001/src/d8.cc#newcode389
src/d8.cc:389: // Hold a reference to the ArrayBuffer so it's buffer doesn't get
collected.
s/it's/its/

http://codereview.chromium.org/9114050/diff/4001/src/d8.cc#newcode393
src/d8.cc:393: reinterpret_cast<uint8_t*>(data) + offset, type,
Whoa... please add checks that both |offset| and |offset + length*element_size|
are within the existing |data| buffer.

[danno, 2012-01-11 13:07:41]

Please take another look

http://codereview.chromium.org/9114050/diff/4001/src/d8.cc
File src/d8.cc (right):

http://codereview.chromium.org/9114050/diff/4001/src/d8.cc#newcode284
src/d8.cc:284: static int convertToInt(Local<Value> value_in, TryCatch*
try_catch) {
On 2012/01/11 10:17:53, Jakob wrote:
> Rename to convertToUint?

Done.

http://codereview.chromium.org/9114050/diff/4001/src/d8.cc#newcode339
src/d8.cc:339: if (args.Length() > 3 || args.Length() == 0) {
On 2012/01/11 10:17:53, Jakob wrote:
> Arguments.operator[] is benign enough to simply return 'undefined' for args[0]
> if args.Length == 0, but nevertheless I'd prefer to check for Length() > 0
> before doing anything else (you can print a better error message that way,
too).

Done.

http://codereview.chromium.org/9114050/diff/4001/src/d8.cc#newcode350
src/d8.cc:350: : args[2];
On 2012/01/11 10:17:53, Jakob wrote:
> args[2] is optional and could be undefined. As implemented, the code defaults
to
> 0 in that case, which is not correct.

Done.

http://codereview.chromium.org/9114050/diff/4001/src/d8.cc#newcode363
src/d8.cc:363:
array->Get(String::New("BYTES_PER_ELEMENT"))->ToInt32()->Int32Value();
On 2012/01/11 10:17:53, Jakob wrote:
> My reading of the spec at
> http://www.khronos.org/registry/typedarray/specs/latest/#7 is that |array|
must
> be an ArrayBuffer (and cannot be another kind of TypedArray), so
> BYTES_PER_ELEMENTS is always 1, and you'll have to throw an exception if a
> TypedArray is passed in.
> Further, IIUC, the "length" parameter specifies the new array's length in
terms
> of number of elements in the new array, so all this conversion-fu is
> unnecessary.

Done.

http://codereview.chromium.org/9114050/diff/4001/src/d8.cc#newcode389
src/d8.cc:389: // Hold a reference to the ArrayBuffer so it's buffer doesn't get
collected.
On 2012/01/11 10:17:53, Jakob wrote:
> s/it's/its/

Done.

http://codereview.chromium.org/9114050/diff/4001/src/d8.cc#newcode393
src/d8.cc:393: reinterpret_cast<uint8_t*>(data) + offset, type,
On 2012/01/11 10:17:53, Jakob wrote:
> Whoa... please add checks that both |offset| and |offset +
length*element_size|
> are within the existing |data| buffer.

Done.

[Jakob Kummerow, 2012-01-11 13:54:27]

LGTM with comments.

http://codereview.chromium.org/9114050/diff/9004/src/d8.cc
File src/d8.cc (right):

http://codereview.chromium.org/9114050/diff/9004/src/d8.cc#newcode399
src/d8.cc:399: if (offset + (length * element_size) > array_buffer_length) {
You need an equivalent check (offset > array_buffer_length) in the args.Length()
== 2 case.

http://codereview.chromium.org/9114050/diff/9004/src/d8.cc#newcode407
src/d8.cc:407: String::New("ArrayBuffer minus the byteOffset must be a "
nit: s/ArrayBuffer/ArrayBuffer length/

http://codereview.chromium.org/9114050/diff/9004/src/d8.cc#newcode456
src/d8.cc:456: return result;
nit: you don't need this line, could return directly instead (as the next couple
of functions do).

http://codereview.chromium.org/9114050/diff/9004/test/mjsunit/external-array.js
File test/mjsunit/external-array.js (right):

http://codereview.chromium.org/9114050/diff/9004/test/mjsunit/external-array....
test/mjsunit/external-array.js:1: // Copyright 2011 the V8 project authors. All
rights reserved.
2012

http://codereview.chromium.org/9114050/diff/9004/test/mjsunit/external-array....
test/mjsunit/external-array.js:75: new Uint32Array(ab,13);
s/13/16/ to avoid triggering the "offset must be multiple of element size" code
path.

http://codereview.chromium.org/9114050/diff/9004/test/mjsunit/external-array....
test/mjsunit/external-array.js:81: function abfunc5() {
nit: AFAICS, abfunc5 and abfunc2 test the same thing.

http://codereview.chromium.org/9114050/diff/9004/test/mjsunit/external-array....
test/mjsunit/external-array.js:91: new Uint32Array(ab,1);
This doesn't test what the comment says (13-1 is, in fact, a multiple of 4, but
an exception is thrown nevertheless, because the offset must be a multiple of
the element size too).
What you mean is "new Uint32Array(ab, 4);".

[danno, 2012-01-11 15:10:20]

http://codereview.chromium.org/9114050/diff/9004/src/d8.cc
File src/d8.cc (right):

http://codereview.chromium.org/9114050/diff/9004/src/d8.cc#newcode399
src/d8.cc:399: if (offset + (length * element_size) > array_buffer_length) {
On 2012/01/11 13:54:27, Jakob wrote:
> You need an equivalent check (offset > array_buffer_length) in the
args.Length()
> == 2 case.

Done.

http://codereview.chromium.org/9114050/diff/9004/src/d8.cc#newcode407
src/d8.cc:407: String::New("ArrayBuffer minus the byteOffset must be a "
On 2012/01/11 13:54:27, Jakob wrote:
> nit: s/ArrayBuffer/ArrayBuffer length/

Done.

http://codereview.chromium.org/9114050/diff/9004/src/d8.cc#newcode456
src/d8.cc:456: return result;
On 2012/01/11 13:54:27, Jakob wrote:
> nit: you don't need this line, could return directly instead (as the next
couple
> of functions do).

Done.

http://codereview.chromium.org/9114050/diff/9004/test/mjsunit/external-array.js
File test/mjsunit/external-array.js (right):

http://codereview.chromium.org/9114050/diff/9004/test/mjsunit/external-array....
test/mjsunit/external-array.js:75: new Uint32Array(ab,13);
On 2012/01/11 13:54:27, Jakob wrote:
> s/13/16/ to avoid triggering the "offset must be multiple of element size"
code
> path. 

Done.

http://codereview.chromium.org/9114050/diff/9004/test/mjsunit/external-array....
test/mjsunit/external-array.js:81: function abfunc5() {
On 2012/01/11 13:54:27, Jakob wrote:
> nit: AFAICS, abfunc5 and abfunc2 test the same thing.

Done.