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#newcode2253
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#newcode2265
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#newcode2277
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.
https://codereview.chromium.org/13958007/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.
