Thanks for review, all comments addressed
https://codereview.chromium.org/15001041/diff/7001/include/v8.h
File include/v8.h (right):
https://codereview.chromium.org/15001041/diff/7001/include/v8.h#newcode2342
include/v8.h:2342: ArrayBufferContents() : data_(NULL), byte_length_(0)
{}
On 2013/05/23 07:57:01, Sven Panne wrote:
Nit: We usually put the constructor/destructor at the beginning of the
corresponding public/protected/private section.
Done.
https://codereview.chromium.org/15001041/diff/7001/include/v8.h#newcode2348
include/v8.h:2348: { }
On 2013/05/23 07:57:01, Sven Panne wrote:
Nit: Put this at the end of the initializer list.
Done.
https://codereview.chromium.org/15001041/diff/7001/src/api.cc
File src/api.cc (right):
https://codereview.chromium.org/15001041/diff/7001/src/api.cc#newcode6023
src/api.cc:6023: i::Isolate* isolate =
Utils::OpenHandle(this)->GetIsolate();
On 2013/05/23 07:57:01, Sven Panne wrote:
Just remove this line and the following IsDeadCheck. This idiom is
fundamentally
broken, anyway, and we want to remove it completely in the future, so
no need
for it in new code.
Done.
https://codereview.chromium.org/15001041/diff/7001/src/api.cc#newcode6025
src/api.cc:6025: i::Handle<i::JSArrayBuffer> obj =
Utils::OpenHandle(this);
On 2013/05/23 07:57:01, Sven Panne wrote:
Nit: No need to name this temporary.
Done.
https://codereview.chromium.org/15001041/diff/7001/src/api.cc#newcode6030
src/api.cc:6030: if (data_ != NULL) {
On 2013/05/23 07:57:01, Sven Panne wrote:
No need for this guard, free(NULL) is by definition a no-op.
Done.
https://codereview.chromium.org/15001041/diff/7001/src/api.cc#newcode6039
src/api.cc:6039: i::Isolate* isolate =
Utils::OpenHandle(this)->GetIsolate();
On 2013/05/23 07:57:01, Sven Panne wrote:
Kill the IsDeadCheck here, too.
Done.
https://codereview.chromium.org/15001041/diff/7001/src/api.cc#newcode6047
src/api.cc:6047: contents->data_ = obj->backing_store();
As discussed offline, added an ApiCheck
On 2013/05/23 07:57:01, Sven Panne wrote:
We silently overwrite any previous backing store in contents here.
This part of
the API looks very fragile and will easily lead to programming errors.
Can we do
this differently?
https://codereview.chromium.org/15001041/diff/7001/src/d8.cc
File src/d8.cc (right):
https://codereview.chromium.org/15001041/diff/7001/src/d8.cc#newcode1080
src/d8.cc:1080: weak_handle.MarkIndependent(isolate);
On 2013/05/23 07:57:01, Sven Panne wrote:
IIRC, dcarney wants to move to a non-Isolate version in the long run,
so we
should remove parameter here.
Done.
https://codereview.chromium.org/15001041/diff/7001/src/runtime.cc
File src/runtime.cc (right):
https://codereview.chromium.org/15001041/diff/7001/src/runtime.cc#newcode668
src/runtime.cc:668: if (data != NULL)
On 2013/05/23 07:57:01, Sven Panne wrote:
No need for this guard here, either.
Done.
https://codereview.chromium.org/15001041/diff/7001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):
https://codereview.chromium.org/15001041/diff/7001/test/cctest/test-api.cc#newcode2496
test/cctest/test-api.cc:2496: THREADED_TEST(ArrayBuffer) {
On 2013/05/23 07:57:01, Sven Panne wrote:
Can we please split this test into several tests? Currently it is very
hard to
see what the various parts are actually testing. Furthermore, do we
have simple
test to see what happens on the JavaScript side when an ArrayBuffer
moves to the
"externalized" state? If there is one, I can't see it...
Done.
https://codereview.chromium.org/15001041/diff/7001/test/cctest/test-api.cc#newcode15477
test/cctest/test-api.cc:15477: ElementType* backing_store = new
ElementType[kElementCount+2];
On 2013/05/23 07:57:01, Sven Panne wrote:
Using 'ScopedVector<ElementType> backing_store(kElementCount+2)'
simplifies
things here.
Done.
https://codereview.chromium.org/15001041/
--
--
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.
