First cut at API for ES6 ArrayBuffers

First cut at API for ES6 ArrayBuffers

R=rossberg@chromium.org
BUG=

Committed: https://code.google.com/p/v8/source/detail?r=14438

[Dmitry Lomov (no reviews), 2013-04-23 17:46:07]

Please take a look

[rossberg, 2013-04-25 11:00:55]

Mostly nits.

https://codereview.chromium.org/13958007/diff/3001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/13958007/diff/3001/include/v8.h#newcode1971
include/v8.h:1971: 
Style: add empty line

https://codereview.chromium.org/13958007/diff/3001/include/v8.h#newcode1973
include/v8.h:1973: * An instance of the built-in ArrayBuffer construnctor (ES6
draft 15.13.5).
Add a comment that this is still "experimental".

https://codereview.chromium.org/13958007/diff/3001/include/v8.h#newcode1994
include/v8.h:1994: * Create a new ArrayBuffer over existing memory block.
Nit: "over an existing..."

https://codereview.chromium.org/13958007/diff/3001/include/v8.h#newcode5138
include/v8.h:5138: 
STyle: add empty line

https://codereview.chromium.org/13958007/diff/3001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/13958007/diff/3001/src/api.cc#newcode5810
src/api.cc:5810: return i::Smi::cast(byte_length)->value();
The case distinction shouldn't be needed, it's done by Object::Number.

https://codereview.chromium.org/13958007/diff/3001/src/heap.h
File src/heap.h (right):

https://codereview.chromium.org/13958007/diff/3001/src/heap.h#newcode696
src/heap.h:696: 
Style: remove extra line

https://codereview.chromium.org/13958007/diff/3001/src/heap.h#newcode697
src/heap.h:697: // Allocates the JS ArrayBuffer object.
Nit: "Allocate a JS ArrayBuffer..."

https://codereview.chromium.org/13958007/diff/3001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/13958007/diff/3001/src/runtime.cc#newcode674
src/runtime.cc:674: isolate->heap()->NumberFromDouble(
Use the handlified abstraction isolate->factory()->NewNumber (or perhaps
NewNumberFromUint).

https://codereview.chromium.org/13958007/diff/3001/src/runtime.cc#newcode684
src/runtime.cc:684: bool Runtime::AllocateAndSetupArrayBuffer(
Can we rename this to something like SetUpArrayBufferAllocatingData, to avoid
confusion?

https://codereview.chromium.org/13958007/diff/3001/src/runtime.cc#newcode692
src/runtime.cc:692: if (data == NULL) {
Nit: I'd put the return on the same line and remove the adjacent empty lines.

https://codereview.chromium.org/13958007/diff/3001/src/runtime.cc#newcode701
src/runtime.cc:701: if  (!SetupArrayBuffer(isolate, array_buffer, data,
allocated_length))
Nit: spurious extra space

https://codereview.chromium.org/13958007/diff/3001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/13958007/diff/3001/test/cctest/test-api.cc#ne...
test/cctest/test-api.cc:2253: 
Style: add extra line

https://codereview.chromium.org/13958007/diff/3001/test/cctest/test-api.cc#ne...
test/cctest/test-api.cc:2265: uint8_t* data = static_cast<uint8_t*>(ab->Data());
Maybe add an assertion that Data is non-null.

https://codereview.chromium.org/13958007/diff/3001/test/cctest/test-api.cc#ne...
test/cctest/test-api.cc:2277: CHECK_EQ(0xAA, data[1]);
Also test the inverse, i.e., writing to data, reading from JS (same below).

[Dmitry Lomov (no reviews), 2013-04-25 11:48:01]

Thanks for the review!
Addressed comments. PTAL.

https://codereview.chromium.org/13958007/diff/3001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/13958007/diff/3001/include/v8.h#newcode1971
include/v8.h:1971: 
On 2013/04/25 11:00:55, rossberg wrote:
> Style: add empty line

Done.

https://codereview.chromium.org/13958007/diff/3001/include/v8.h#newcode1973
include/v8.h:1973: * An instance of the built-in ArrayBuffer construnctor (ES6
draft 15.13.5).
On 2013/04/25 11:00:55, rossberg wrote:
> Add a comment that this is still "experimental".

Done.

https://codereview.chromium.org/13958007/diff/3001/include/v8.h#newcode1994
include/v8.h:1994: * Create a new ArrayBuffer over existing memory block.
On 2013/04/25 11:00:55, rossberg wrote:
> Nit: "over an existing..."

Done.

https://codereview.chromium.org/13958007/diff/3001/include/v8.h#newcode5138
include/v8.h:5138: 
On 2013/04/25 11:00:55, rossberg wrote:
> STyle: add empty line

Done.

https://codereview.chromium.org/13958007/diff/3001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/13958007/diff/3001/src/api.cc#newcode5810
src/api.cc:5810: return i::Smi::cast(byte_length)->value();
On 2013/04/25 11:00:55, rossberg wrote:
> The case distinction shouldn't be needed, it's done by Object::Number.

Done.

https://codereview.chromium.org/13958007/diff/3001/src/heap.h
File src/heap.h (right):

https://codereview.chromium.org/13958007/diff/3001/src/heap.h#newcode696
src/heap.h:696: 
On 2013/04/25 11:00:55, rossberg wrote:
> Style: remove extra line

Done.

https://codereview.chromium.org/13958007/diff/3001/src/heap.h#newcode697
src/heap.h:697: // Allocates the JS ArrayBuffer object.
On 2013/04/25 11:00:55, rossberg wrote:
> Nit: "Allocate a JS ArrayBuffer..."

Done.

https://codereview.chromium.org/13958007/diff/3001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/13958007/diff/3001/src/runtime.cc#newcode674
src/runtime.cc:674: isolate->heap()->NumberFromDouble(
On 2013/04/25 11:00:55, rossberg wrote:
> Use the handlified abstraction isolate->factory()->NewNumber (or perhaps
> NewNumberFromUint).

Done.

https://codereview.chromium.org/13958007/diff/3001/src/runtime.cc#newcode684
src/runtime.cc:684: bool Runtime::AllocateAndSetupArrayBuffer(
On 2013/04/25 11:00:55, rossberg wrote:
> Can we rename this to something like SetUpArrayBufferAllocatingData, to avoid
> confusion?

Done.

https://codereview.chromium.org/13958007/diff/3001/src/runtime.cc#newcode692
src/runtime.cc:692: if (data == NULL) {
On 2013/04/25 11:00:55, rossberg wrote:
> Nit: I'd put the return on the same line and remove the adjacent empty lines.

Done.

https://codereview.chromium.org/13958007/diff/3001/src/runtime.cc#newcode701
src/runtime.cc:701: if  (!SetupArrayBuffer(isolate, array_buffer, data,
allocated_length))
On 2013/04/25 11:00:55, rossberg wrote:
> Nit: spurious extra space

Done.

https://codereview.chromium.org/13958007/diff/3001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/13958007/diff/3001/test/cctest/test-api.cc#ne...
test/cctest/test-api.cc:2253: 
On 2013/04/25 11:00:55, rossberg wrote:
> Style: add extra line

Done.

https://codereview.chromium.org/13958007/diff/3001/test/cctest/test-api.cc#ne...
test/cctest/test-api.cc:2265: uint8_t* data = static_cast<uint8_t*>(ab->Data());
On 2013/04/25 11:00:55, rossberg wrote:
> Maybe add an assertion that Data is non-null.

Done.

https://codereview.chromium.org/13958007/diff/3001/test/cctest/test-api.cc#ne...
test/cctest/test-api.cc:2277: CHECK_EQ(0xAA, data[1]);
On 2013/04/25 11:00:55, rossberg wrote:
> Also test the inverse, i.e., writing to data, reading from JS (same below).

Done.

[rossberg, 2013-04-25 11:58:04]

lgtm

[Dmitry Lomov (no reviews), 2013-04-25 12:02:33]

Committed patchset #3 manually as r14438 (presubmit successful).