Refactor v8::Object bindings (#243)

* Rename `Object::new2()` to `Object::with_prototype_and_properties()`.
* Make `Object::with_prototype_and_properties()` take a slice of keys
  and a slice of values as arguments, instead of using
  `Vec<v8::Local<v8::Name>>` and `Vec<v8::Local<v8::Value>>>`.
* Remove type `MaybeBool` from the public interface. These methods now
  return `Option<bool>` instead.
* Fix parameter type mismatches between Rust and C++ APIs.

src/binding.cc

@@ -658,10 +658,9 @@ v8::Object* v8__Object__New(v8::Isolate* isolate) {
   return local_to_ptr(v8::Object::New(isolate));
 }
 
-v8::Object* v8__Object__New2(v8::Isolate* isolate,
-                             v8::Local<v8::Value> prototype_or_null,
-                             v8::Local<v8::Name>* names,
-                             v8::Local<v8::Value>* values, size_t length) {
+v8::Object* v8__Object__New__with_prototype_and_properties(
+    v8::Isolate* isolate, v8::Local<v8::Value> prototype_or_null,
+    v8::Local<v8::Name>* names, v8::Local<v8::Value>* values, size_t length) {
   return local_to_ptr(
       v8::Object::New(isolate, prototype_or_null, names, values, length));
 }

src/lib.rs

@@ -162,7 +162,6 @@ pub use snapshot::OwnedStartupData;
 pub use snapshot::SnapshotCreator;
 pub use snapshot::StartupData;
 pub use string::NewStringType;
-pub use support::MaybeBool;
 pub use support::SharedRef;
 pub use support::UniquePtr;
 pub use support::UniqueRef;

src/object.rs

@@ -14,42 +14,41 @@ use crate::Value;
 
 extern "C" {
   fn v8__Object__New(isolate: *mut Isolate) -> *mut Object;
-  fn v8__Object__New2(
+  fn v8__Object__New__with_prototype_and_properties(
     isolate: *mut Isolate,
-    prototype_or_null: *mut Value,
-    names: *mut *mut Name,
-    values: *mut *mut Value,
+    prototype_or_null: Local<Value>,
+    names: *mut Local<Name>,
+    values: *mut Local<Value>,
     length: usize,
   ) -> *mut Object;
   fn v8__Object__SetAccessor(
     self_: &Object,
-    context: *const Context,
-    name: *const Name,
+    context: Local<Context>,
+    name: Local<Name>,
     getter: AccessorNameGetterCallback,
   ) -> MaybeBool;
-
   fn v8__Object__Get(
     object: &Object,
-    context: *const Context,
-    key: *const Value,
+    context: Local<Context>,
+    key: Local<Value>,
   ) -> *mut Value;
   fn v8__Object__Set(
     object: &Object,
-    context: *const Context,
-    key: *const Value,
-    value: *const Value,
+    context: Local<Context>,
+    key: Local<Value>,
+    value: Local<Value>,
   ) -> MaybeBool;
   fn v8__Object__CreateDataProperty(
     object: &Object,
-    context: *const Context,
-    key: *const Name,
-    value: *const Value,
+    context: Local<Context>,
+    key: Local<Name>,
+    value: Local<Value>,
   ) -> MaybeBool;
   fn v8__Object__DefineOwnProperty(
     object: &Object,
-    context: *const Context,
-    key: *const Name,
-    value: *const Value,
+    context: Local<Context>,
+    key: Local<Name>,
+    value: Local<Value>,
     attr: PropertyAttribute,
   ) -> MaybeBool;
   fn v8__Object__GetIdentityHash(object: &Object) -> int;
@@ -70,35 +69,23 @@ impl Object {
   /// a prototype at all). This is similar to Object.create().
   /// All properties will be created as enumerable, configurable
   /// and writable properties.
-  pub fn new2<'sc>(
+  pub fn with_prototype_and_properties<'sc>(
     scope: &mut impl ToLocal<'sc>,
-    mut prototype_or_null: Local<'sc, Value>,
-    names: Vec<Local<'sc, Name>>,
-    values: Vec<Local<'sc, Value>>,
+    prototype_or_null: Local<'sc, Value>,
+    names: &[Local<Name>],
+    values: &[Local<Value>],
   ) -> Local<'sc, Object> {
-    let length = names.len();
-    assert_eq!(length, values.len());
-    let mut names_: Vec<*mut Name> = vec![];
-    for mut name in names {
-      let n = &mut *name;
-      names_.push(n);
-    }
-
-    let mut values_: Vec<*mut Value> = vec![];
-    for mut value in values {
-      let n = &mut *value;
-      values_.push(n);
-    }
-    let ptr = unsafe {
-      v8__Object__New2(
+    assert_eq!(names.len(), values.len());
+    unsafe {
+      let object = v8__Object__New__with_prototype_and_properties(
         scope.isolate(),
-        &mut *prototype_or_null,
-        names_.as_mut_ptr(),
-        values_.as_mut_ptr(),
-        length,
-      )
-    };
-    unsafe { scope.to_local(ptr) }.unwrap()
+        prototype_or_null,
+        names.as_ptr() as *mut Local<Name>,
+        values.as_ptr() as *mut Local<Value>,
+        names.len(),
+      );
+      scope.to_local(object).unwrap()
+    }
   }
 
   /// Set only return Just(true) or Empty(), so if it should never fail, use
@@ -108,8 +95,8 @@ impl Object {
     context: Local<Context>,
     key: Local<Value>,
     value: Local<Value>,
-  ) -> MaybeBool {
-    unsafe { v8__Object__Set(self, &*context, &*key, &*value) }
+  ) -> Option<bool> {
+    unsafe { v8__Object__Set(self, context, key, value) }.into()
   }
 
   /// Implements CreateDataProperty (ECMA-262, 7.3.4).
@@ -124,8 +111,8 @@ impl Object {
     context: Local<Context>,
     key: Local<Name>,
     value: Local<Value>,
-  ) -> MaybeBool {
-    unsafe { v8__Object__CreateDataProperty(self, &*context, &*key, &*value) }
+  ) -> Option<bool> {
+    unsafe { v8__Object__CreateDataProperty(self, context, key, value) }.into()
   }
 
   /// Implements DefineOwnProperty.
@@ -140,10 +127,9 @@ impl Object {
     key: Local<Name>,
     value: Local<Value>,
     attr: PropertyAttribute,
-  ) -> MaybeBool {
-    unsafe {
-      v8__Object__DefineOwnProperty(self, &*context, &*key, &*value, attr)
-    }
+  ) -> Option<bool> {
+    unsafe { v8__Object__DefineOwnProperty(self, context, key, value, attr) }
+      .into()
   }
 
   pub fn get<'a>(
@@ -153,7 +139,7 @@ impl Object {
     key: Local<Value>,
   ) -> Option<Local<'a, Value>> {
     unsafe {
-      let ptr = v8__Object__Get(self, &*context, &*key);
+      let ptr = v8__Object__Get(self, context, key);
       scope.to_local(ptr)
     }
   }
@@ -164,10 +150,9 @@ impl Object {
     context: Local<Context>,
     name: Local<Name>,
     getter: impl for<'s> MapFnTo<AccessorNameGetterCallback<'s>>,
-  ) -> MaybeBool {
-    unsafe {
-      v8__Object__SetAccessor(self, &*context, &*name, getter.map_fn_to())
-    }
+  ) -> Option<bool> {
+    unsafe { v8__Object__SetAccessor(self, context, name, getter.map_fn_to()) }
+      .into()
   }
 
   /// Returns the identity hash for this object. The current implementation

src/support.rs

@@ -236,7 +236,7 @@ where
 
 #[repr(C)]
 #[derive(Debug, PartialEq)]
-pub enum MaybeBool {
+pub(crate) enum MaybeBool {
   JustFalse = 0,
   JustTrue = 1,
   Nothing = 2,

tests/test_api.rs

@@ -1013,15 +1013,16 @@ fn object() {
     let mut cs = v8::ContextScope::new(scope, context);
     let scope = cs.enter();
     let null: v8::Local<v8::Value> = v8::null(scope).into();
-    let s1 = v8::String::new(scope, "a").unwrap();
-    let s2 = v8::String::new(scope, "b").unwrap();
-    let name1 = s1.into();
-    let name2 = s2.into();
-    let names = vec![name1, name2];
+    let n1: v8::Local<v8::Name> = v8::String::new(scope, "a").unwrap().into();
+    let n2: v8::Local<v8::Name> = v8::String::new(scope, "b").unwrap().into();
     let v1: v8::Local<v8::Value> = v8::Number::new(scope, 1.0).into();
     let v2: v8::Local<v8::Value> = v8::Number::new(scope, 2.0).into();
-    let values = vec![v1, v2];
-    let object = v8::Object::new2(scope, null, names, values);
+    let object = v8::Object::with_prototype_and_properties(
+      scope,
+      null,
+      &[n1, n2],
+      &[v1, v2],
+    );
     assert!(!object.is_null_or_undefined());
 
     let object_ = v8::Object::new(scope);
@@ -1088,18 +1089,14 @@ fn create_data_property() {
     let obj = obj.to_object(scope).unwrap();
     let key = v8_str(scope, "foo");
     let value = v8_str(scope, "bar");
-    assert_eq!(
-      obj.create_data_property(context, key.into(), value.into()),
-      v8::MaybeBool::JustTrue
-    );
+    assert!(obj
+      .create_data_property(context, key.into(), value.into())
+      .unwrap());
     let actual = obj.get(scope, context, key.into()).unwrap();
     assert!(value.strict_equals(actual));
 
     let key2 = v8_str(scope, "foo2");
-    assert_eq!(
-      obj.set(context, key2.into(), value.into()),
-      v8::MaybeBool::JustTrue
-    );
+    assert!(obj.set(context, key2.into(), value.into()).unwrap());
     let actual = obj.get(scope, context, key2.into()).unwrap();
     assert!(value.strict_equals(actual));
   }
@@ -1893,12 +1890,10 @@ fn shared_array_buffer() {
     }
 
     let global = context.global(scope);
-    let r = global.create_data_property(
-      context,
-      v8_str(scope, "shared").into(),
-      sab.into(),
-    );
-    assert_eq!(r, v8::MaybeBool::JustTrue);
+    let r = global
+      .create_data_property(context, v8_str(scope, "shared").into(), sab.into())
+      .unwrap();
+    assert!(r);
     let source = v8::String::new(
       scope,
       r"sharedBytes = new Uint8Array(shared);