[Bindings] Use SetNativeDataProperty to install features on instances

third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp

Index: third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp
diff --git a/third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp b/third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp
index 288633ca3b04386091e2453ed33aebd4b18f857d..de7496dc20d998d68ce6921645c3251be4d1331a 100644
--- a/third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp
+++ b/third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp
@@ -95,35 +95,31 @@ void InstallAttributeInternal(
     const DOMWrapperWorld& world) {
   if (!WorldConfigurationApplies(attribute, world))
     return;
-  v8::Local<v8::Name> name = V8AtomicString(isolate, attribute.name);
-
-  // This method is only being used for installing interfaces which are
-  // enabled through origin trials. Assert here that it is being called with
-  // an attribute configuration for a constructor.
-  // TODO(iclelland): Relax this constraint and allow arbitrary data-type
-  // properties to be added here.
-  DCHECK_EQ(&V8ConstructorAttributeGetter, attribute.getter);

[haraken, 2017/04/13 08:06:09]

Can we keep this assert?

This assert is important since non-data properties should not use
InstallAttributeInternal (which should actually be renamed to
InstallConstructorDataProperty).

Not related to this CL, but we should really consider renaming methods in this
file. They are confusing...

[peria, 2017/04/13 09:51:21]

done
Agreed. I'll rename these functions (and also local variables) in another CL.

 
-  V8PerContextData* per_context_data =
-      V8PerContextData::From(isolate->GetCurrentContext());
-  v8::Local<v8::Function> data =
-      per_context_data->ConstructorForType(attribute.data);
+  v8::Local<v8::Name> name = V8AtomicString(isolate, attribute.name);
+  v8::AccessorNameGetterCallback getter = attribute.getter;
+  v8::AccessorNameSetterCallback setter = attribute.setter;
+  v8::Local<v8::Value> data =
+      v8::External::New(isolate, const_cast<WrapperTypeInfo*>(attribute.data));
 
+  v8::Local<v8::Context> context = isolate->GetCurrentContext();
   DCHECK(attribute.property_location_configuration);
   if (attribute.property_location_configuration &
-      V8DOMConfiguration::kOnInstance)
-    instance
-        ->DefineOwnProperty(
-            isolate->GetCurrentContext(), name, data,
-            static_cast<v8::PropertyAttribute>(attribute.attribute))
-        .ToChecked();
+      V8DOMConfiguration::kOnInstance) {
+    DCHECK(instance

[bashi, 2017/04/13 08:08:36]

Why do we need DCHECK()? ToChecked() should crash on failures.

[peria, 2017/04/13 09:51:21]

Hmm, I thought returning false can be independent from internal exceptions, but
as far as I read V8 code, SetAccessor seems to return iff it has a pending
exception.
So removed.

+               ->SetAccessor(
+                   context, name, getter, setter, data, v8::DEFAULT,
+                   static_cast<v8::PropertyAttribute>(attribute.attribute))
+               .ToChecked());
+  }
   if (attribute.property_location_configuration &
-      V8DOMConfiguration::kOnPrototype)
-    prototype
-        ->DefineOwnProperty(
-            isolate->GetCurrentContext(), name, data,
-            static_cast<v8::PropertyAttribute>(attribute.attribute))
-        .ToChecked();
+      V8DOMConfiguration::kOnPrototype) {
+    DCHECK(prototype
+               ->SetAccessor(
+                   context, name, getter, setter, data, v8::DEFAULT,
+                   static_cast<v8::PropertyAttribute>(attribute.attribute))
+               .ToChecked());
+  }
   if (attribute.property_location_configuration &
       V8DOMConfiguration::kOnInterface)
     NOTREACHED();