Externalization API for ArrayBuffer

src/api.cc

Index: src/api.cc
diff --git a/src/api.cc b/src/api.cc
index 7099ca8ddd9de161110b359089a77a4bb2ff23e7..9d0ada594ed5ef96aec818f99ed68b77b498fed3 100644
--- a/src/api.cc
+++ b/src/api.cc
@@ -6019,19 +6019,41 @@ Local<Object> Array::CloneElementAt(uint32_t index) {
 }
 
 
-size_t v8::ArrayBuffer::ByteLength() const {
+bool v8::ArrayBuffer::IsExternal() const {
   i::Isolate* isolate = Utils::OpenHandle(this)->GetIsolate();

[Sven Panne, 2013/05/23 07:57:01]

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.

[Dmitry Lomov (no reviews), 2013/05/23 08:42:15]

Done.

-  if (IsDeadCheck(isolate, "v8::ArrayBuffer::ByteLength()")) return 0;
+  if (IsDeadCheck(isolate, "v8::ArrayBuffer::IsExternal()")) return 0;
   i::Handle<i::JSArrayBuffer> obj = Utils::OpenHandle(this);

[Sven Panne, 2013/05/23 07:57:01]

Nit: No need to name this temporary.

[Dmitry Lomov (no reviews), 2013/05/23 08:42:15]

Done.

-  return static_cast<size_t>(obj->byte_length()->Number());
+  return obj->is_external();
+}
+
+v8::ArrayBufferContents::~ArrayBufferContents() {
+  if (data_ != NULL) {

[Sven Panne, 2013/05/23 07:57:01]

No need for this guard, free(NULL) is by definition a no-op.

[Dmitry Lomov (no reviews), 2013/05/23 08:42:15]

Done.

+    free(data_);
+    data_ = NULL;
+  }
+  byte_length_ = 0;
+}
+
+
+void v8::ArrayBuffer::Externalize(ArrayBufferContents* contents) {
+  i::Isolate* isolate = Utils::OpenHandle(this)->GetIsolate();

[Sven Panne, 2013/05/23 07:57:01]

Kill the IsDeadCheck here, too.

[Dmitry Lomov (no reviews), 2013/05/23 08:42:15]

Done.

+  if (IsDeadCheck(isolate, "v8::ArrayBuffer::Data()")) return;
+  i::Handle<i::JSArrayBuffer> obj = Utils::OpenHandle(this);
+  ApiCheck(!obj->is_external(),
+            "v8::ArrayBuffer::Externalize",
+            "ArrayBuffer already externalized");
+  obj->set_is_external(true);
+  size_t byte_length = static_cast<size_t>(obj->byte_length()->Number());
+  contents->data_ = obj->backing_store();

[Sven Panne, 2013/05/23 07:57:01]

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?

[Dmitry Lomov (no reviews), 2013/05/23 08:42:15]

As discussed offline, added an ApiCheck
On 2013/05/23 07:57:01, Sven Panne wrote:

+  contents->byte_length_ = byte_length;
 }
 
 
-void* v8::ArrayBuffer::Data() const {
+size_t v8::ArrayBuffer::ByteLength() const {
   i::Isolate* isolate = Utils::OpenHandle(this)->GetIsolate();
-  if (IsDeadCheck(isolate, "v8::ArrayBuffer::Data()")) return 0;
+  if (IsDeadCheck(isolate, "v8::ArrayBuffer::ByteLength()")) return 0;
   i::Handle<i::JSArrayBuffer> obj = Utils::OpenHandle(this);
-  return obj->backing_store();
+  return static_cast<size_t>(obj->byte_length()->Number());
 }
 
 
@@ -6054,7 +6076,7 @@ Local<ArrayBuffer> v8::ArrayBuffer::New(void* data, size_t byte_length) {
   ENTER_V8(isolate);
   i::Handle<i::JSArrayBuffer> obj =
       isolate->factory()->NewJSArrayBuffer();
-  i::Runtime::SetupArrayBuffer(isolate, obj, data, byte_length);
+  i::Runtime::SetupArrayBuffer(isolate, obj, true, data, byte_length);
   return Utils::ToLocal(obj);
 }