[Origin Trials] Install origin trial bindings on V8 context conditionally

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

 /*
  * Copyright (C) 2012 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  *
  * 1.  Redistributions of source code must retain the above copyright
  *     notice, this list of conditions and the following disclaimer.
  * 2.  Redistributions in binary form must reproduce the above copyright
@@ skipping 43 matching lines @@
 
     DCHECK(attribute.propertyLocationConfiguration);
     if (attribute.propertyLocationConfiguration & V8DOMConfiguration::OnInstance)
         instanceTemplate->SetNativeDataProperty(name, getter, setter, data, static_cast<v8::PropertyAttribute>(attribute.attribute), v8::Local<v8::AccessorSignature>(), static_cast<v8::AccessControl>(attribute.settings));
     if (attribute.propertyLocationConfiguration & V8DOMConfiguration::OnPrototype)
         prototypeTemplate->SetNativeDataProperty(name, getter, setter, data, static_cast<v8::PropertyAttribute>(attribute.attribute), v8::Local<v8::AccessorSignature>(), static_cast<v8::AccessControl>(attribute.settings));
     if (attribute.propertyLocationConfiguration & V8DOMConfiguration::OnInterface)
         NOTREACHED();
 }
 
 void installAttributeInternal(v8::Isolate* isolate, v8::Local<v8::Object> instance, v8::Local<v8::Object> prototype, const V8DOMConfiguration::AttributeConfiguration& attribute, const DOMWrapperWorld& world)

[haraken, 2016/05/27 00:01:42]

Now you're using SetAccessor, this method should be renamed to
installAccessorInternal. But we already have installAccessorInternal. Does it
mean that we can just use the existing installAccessorInternal for your purpose?

[iclelland, 2016/05/27 03:19:43]

That's a really good question. I started reading through all of the
SetAccessor/DefineProperty/SetAccessorProperty code to find all of the
differences between them. I think we can, and we should.

The cleanest way I can see to do that, though, is to change the definition of
is_data_property in v8_attributes.py to make these origin trial interfaces into
accessors rather than attributes. Then it will be defined as an accessor in the
generated bindings code, and everything should work correctly.

The comment in this commit: https://crrev.com/8988a332 -- made me think that
maybe that wasn't correct, though. Constructors were deliberately *not* made
into accessors in that change, and I didn't want to reverse that explicit
decision without understanding it more.

[Yuki, 2016/05/27 09:59:42]

For historical reasons, "attribute" in this file (installAttribute, etc.) means
"data-type property" regardless of whether it has callback functions or not.

There are two different things.
- data-type property vs accessor-type property
- if a property has "getter" and "setter" callback in C++, or not

As long as this function installs data-type properties, the name should be
"attribute" for the time being for consistency (although it's wrong naming...)

For example, constants are data-type properties and usually implemented without
callback functions.  However, when we'd like to count their usage (UseCounter),
we use callback functions to check how many times it's called.  But it's still
data-type properties, and constants are constants.

[iclelland, 2016/05/27 13:51:26]

This file is very confusing :)

constants are data type properties, and as you say, could be called 'attributes'
in this file, but there is also a set of installConstant() methods, which use a
*different* attachment method again (ObjectTemplate::Set this time)

And then, https://crrev.com/8988a332 says that constructors should actually be
*methods*, but there's *yet another* set of functions here called
installMethod() which we also don't use.

Yuki, are you saying here that what I've done is as correct as we can do within
the constraints of consistency?

(The only actual reason to use SetAccessor here is that Object::DefineProperty
doesn't allow the getter to be overridden, and the constructors need to use
v8ConstructorAttributeGetter, otherwise, I think they should be just like any
other data-type property)

[Yuki, 2016/05/27 15:09:42]

1. This function is currently NOT used.  So it's okay to rewrite as you like (or
remove it).

2. Spec says that there are
a) DOM constants, which are data-type properties
b) DOM attributes, which are accessor-type properties
c) DOM operations (often called method), which are data-type properties
d) Interface objects that are callable (often called constructor), which are
data-type properties

Note that constructors are not attributes or methods.  They're totally different
things.  However, if we focus to data-type or accessor-type,
  - constants, methods, constructors are data-type properties
  - attributes are accessor-type properties
So, constructors look much more like methods than attributes, although
constructors are not methods.

3. Current implementaion
If it's data-type properties (and not constants nor methods),
installAttribute(s) is used.  Otherwise, it's accessor-type properties, then
installAccessor(s) is used.

So, as long as you'd like to install "interface objects that are callable" (i.e.
constructors), then it's consistent to use installAttribute(s).  For other DOM
attributes (which must be accessor-type properties unless it's installed on
Window or Location), please use installAccessor(s).

4. SetAccessor or DefineOwnProperty

You can use SetAccessor if necessary and it's fine.  However, there is another
way.  Since you've already entered a specific context, you can instantiate an
interface object in that context and create a corresponding data-type property
with the value of the interface object.
    // Suppose you instantiate the interface object.
    v8::Local<v8::Value> interfaceObject = ...;
    holder->DefineOwnProperty("YourInterface", interfaceObject);
In this way, you can use DefineOwnProperty, and no need to use SetAccessor().

DefineOwnProperty: No need to care about getter/setter callback, etc.  V8 takes
care of everything.  The access to the property should be faster than
SetAccesor().  If you have a huge number of interfaces, instantiating all the
interface objects may cost.

SetAccessor: You can install your own getter/setter callback functions, so you
can count the usage (UseCounter).

[iclelland, 2016/05/27 15:27:01]

(I presume here that you're talking about this new version of the
installAttributeInternal function)
That you for that clarification; it's making a little bit more sense now.
Will that work for interface objects, though? It seems to be a requirement that
getting the attribute goes through v8ConstructorAttributeGetter. (Without this,
the attribute appears like "[Object object]" and not like "function some_name()
{ [native code] }" in the console, and I get a CannotConvertToPrimitive error if
I try to manipulate it at all)

(The specific failures I would get are in
LayoutTests/fast/js/global-constructors.html)

[Yuki, 2016/05/30 08:14:19]

Should work for the interface objects because the interface objects are
data-type properties.

In console,
  x = window.AudioNode // => function AudioNode() { [native code] }
  Object.defineProperty(window, 'Foo', {value: x})
  window.Foo // => function AudioNode() { [native code] }

You can do the same thing in C++.  The interface object itself is a function
object implemented in C++ (so, printed as [native code]), and you can
instantiate it as |x|.  Then, you can define a data-type property with the value
|x| with using DefineOwnProperty.

[iclelland, 2016/05/30 12:15:35]

I'll take a closer look to see what was going on with the failures in PS#6 (see
https://storage.googleapis.com/chromium-layout-test-archives/win_chromium_rel...,
for instance). The JS example you give relies on AudioNode already being defined
correctly on window, so it makes sense that simply duplicating it would create a
correctly defined Foo. To define AudioNode correctly in the first place, it
needs to have the custom getter -- that's why the generated code for
V8WindowPartial looks like this:

https://code.google.com/p/chromium/codesearch#chromium/src/out/Debug/gen/blin...

isn't it?

[Yuki, 2016/05/30 13:07:58]

Is this where you were trying to install an interface object?
https://codereview.chromium.org/2005433002/diff/100001/third_party/WebKit/Sou...
Then, it looks weird to me.

I was thinking about the following way.

  // You should already have V8YourInterface like V8AudioNode.
  V8PerContextData* perContextData = ...;
  v8::Local<v8::Function> interfaceObject =
      perContextData->constructorForType(&V8YourInterface::wrapperTypeInfo);
  v8::Local<v8::Object> obj = ...;
  obj->DefineOwnProperty(context, v8String("YourInterface"), interfaceObject);

I'm writing this very quickly, so details may be wrong.  But we should be able
to expose your interface in this way, too.

[iclelland, 2016/05/30 15:07:46]

That method works, and I've switched installAttributeInternal to use it. I've
added an assertion, to ensure that we are only calling that code in a situation
where using constructorForType is appropriate. I will update the method if we
ever need to use it to install other kinds of data-type properties.

 {
     if (attribute.exposeConfiguration == V8DOMConfiguration::OnlyExposedToPrivateScript
         && !world.isPrivateScriptIsolatedWorld())
         return;
 
     v8::Local<v8::Name> name = v8AtomicString(isolate, attribute.name);
+    v8::AccessorNameGetterCallback getter = attribute.getter;
+    v8::AccessorNameSetterCallback setter = attribute.setter;
+    if (world.isMainWorld()) {
+        if (attribute.getterForMainWorld)
+            getter = attribute.getterForMainWorld;
+        if (attribute.setterForMainWorld)
+            setter = attribute.setterForMainWorld;
+    }
     v8::Local<v8::Value> data = v8::External::New(isolate, const_cast<WrapperTypeInfo*>(attribute.data));
 
     DCHECK(attribute.propertyLocationConfiguration);
     if (attribute.propertyLocationConfiguration & V8DOMConfiguration::OnInstance)
-        v8CallOrCrash(instance->DefineOwnProperty(isolate->GetCurrentContext(), name, data, static_cast<v8::PropertyAttribute>(attribute.attribute)));
+        v8CallOrCrash(instance->SetAccessor(isolate->GetCurrentContext(), name, getter, setter, data, static_cast<v8::AccessControl>(attribute.settings), static_cast<v8::PropertyAttribute>(attribute.attribute)));
     if (attribute.propertyLocationConfiguration & V8DOMConfiguration::OnPrototype)
-        v8CallOrCrash(prototype->DefineOwnProperty(isolate->GetCurrentContext(), name, data, static_cast<v8::PropertyAttribute>(attribute.attribute)));
+        v8CallOrCrash(prototype->SetAccessor(isolate->GetCurrentContext(), name, getter, setter, data, static_cast<v8::AccessControl>(attribute.settings), static_cast<v8::PropertyAttribute>(attribute.attribute)));
     if (attribute.propertyLocationConfiguration & V8DOMConfiguration::OnInterface)
         NOTREACHED();
 }
 
 template<class FunctionOrTemplate>
 v8::Local<FunctionOrTemplate> createAccessorFunctionOrTemplate(v8::Isolate*, v8::FunctionCallback, v8::Local<v8::Value> data, v8::Local<v8::Signature>, int length);
 
 template<>
 v8::Local<v8::FunctionTemplate> createAccessorFunctionOrTemplate<v8::FunctionTemplate>(v8::Isolate* isolate, v8::FunctionCallback callback, v8::Local<v8::Value> data, v8::Local<v8::Signature> signature, int length)
 {
@@ skipping 265 matching lines @@
     data->setInterfaceTemplate(world, wrapperTypeInfo, result);
     return result;
 }
 
 void V8DOMConfiguration::setClassString(v8::Isolate* isolate, v8::Local<v8::ObjectTemplate> objectTemplate, const char* classString)
 {
     objectTemplate->Set(v8::Symbol::GetToStringTag(isolate), v8AtomicString(isolate, classString), static_cast<v8::PropertyAttribute>(v8::ReadOnly | v8::DontEnum));
 }
 
 } // namespace blink