Eagerly install Origin Trial features on window

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 1f7d79ad77138ed6a703d0afcebaaea6af7ebb0f..9227686b3b291001b7399a63f7173b16d92af28e 100644
--- a/third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp
+++ b/third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp
@@ -29,6 +29,7 @@
 #include "bindings/core/v8/V8DOMConfiguration.h"
 
 #include "bindings/core/v8/GeneratedCodeHelper.h"  // just for DCHECK
+#include "bindings/core/v8/ScriptState.h"
 #include "bindings/core/v8/V8ObjectConstructor.h"
 #include "bindings/core/v8/V8PerContextData.h"
 #include "platform/tracing/TraceEvent.h"
@@ -78,11 +79,13 @@ void installAttributeInternal(
 }
 
 void installAttributeInternal(
-    v8::Isolate* isolate,
+    const ScriptState* scriptState,
     v8::Local<v8::Object> instance,

[Yuki, 2016/11/01 11:37:54]

Shall we change the function name and argument name and/or order drastically?  I
was misunderstanding what this function is expected to do.

Other utility functions in this file are designed to take
    instance = instance object or template of type X
    prototype = prototype object or template of type X
    interface = interface object or template of type X
however, this function takes
    instance = the global object (or navigator object?)
    prototype = prototype object or template of type X
    interface = interface object or template of type X

This is quite confusing.  We should make it clear that this function is so
special, and it's the global object.

IIUC, this function always installs the interface object on the global object
(or window.navigator?), right?  Then, I'd propose
    exposeInterfaceObject(const WrapperTypeInfo* interface,
v8::Local<v8::Object> target)
or something like that.

[iclelland, 2016/11/01 17:06:27]

I have no problem renaming, or even restructuring the file. I'd like to make
sure that we're clear what it's doing, though.

The install*internal methods in this file seem to take two forms, depending on
whether they are modifying a template or an instance. InstallAccessorInternal is
the only exception, and is templated to take one form or the other.

The actual parameters depend on what is being installed: attributes take
instance and prototype, constants take prototype and interface, and accessors
and methods need all three (instance, prototype and interface).

This file contains an implementation for every combination of member
{accessor,attribute,constant,method} and target {object,template}.

This method, in particular, installs attributes from the generated binding code
onto any instantiated v8 object instance (or optionally its prototype, if that's
how the attribute is defined).


Now, in practice:

This method is only used to expose constructor interfaces on the global objects
-- Window as well as ServiceWorkerGlobalScope. It has been used on
WorkerGlobalScope before, by DurableStorage, but not currently.

Other origin-trial-enabled features, on objects like Navigator (and
HTMLLinkElement, Gamepad, etc) all use installMethod and installAccessor
instead.

I'm not certain if there are IDL configurations that would cause
installAttribute to be used on objects other than the global objects.

Given that, we could rename this one method, and leave the other seven as-is. If
we're breaking the orthogonality there, then we can also just move this code
into installAttribute, and rename that instead. In that case, we'd need to
change the bindings code generator to know that the method to call is different
in that one case (installing an attribute in an origin trial), but that should
be possible.

     v8::Local<v8::Object> prototype,
-    const V8DOMConfiguration::AttributeConfiguration& attribute,
-    const DOMWrapperWorld& world) {
+    const V8DOMConfiguration::AttributeConfiguration& attribute) {
+  v8::Isolate* isolate = scriptState->isolate();
+  const DOMWrapperWorld& world = scriptState->world();
+
   if (attribute.exposeConfiguration ==
           V8DOMConfiguration::OnlyExposedToPrivateScript &&
       !world.isPrivateScriptIsolatedWorld())

[Yuki, 2016/11/01 11:37:54]

Given that this function is so special, do you need |world| at all?
Do you have any plan to do something depends on a world?

[iclelland, 2016/11/01 17:06:27]

Not for origin trials. Again, this method was written to be a general mechanism
to install attribute-type members on instantiated objects.

I don't think there are any plans to conditionally install constructor
interfaces in extensions. It's possible that with Feature Policy, we may be
conditionally installing constructors like RTCPeerConnection, but that's not a
final design decision yet. If that interface is currently accessible from
isolated worlds, then we'd need to be able to install it there as well, when not
disabled. But again, it's not final, and may be premature to say that it will be
needed.

@@ -98,7 +101,7 @@ void installAttributeInternal(
   DCHECK_EQ(&v8ConstructorAttributeGetter, attribute.getter);
 
   V8PerContextData* perContextData =
-      V8PerContextData::from(isolate->GetCurrentContext());
+      V8PerContextData::from(scriptState->context());
   v8::Local<v8::Function> data =

[Yuki, 2016/11/01 11:37:54]

nit: s/data/value/ since this is used the value of the property.

[iclelland, 2016/11/02 13:49:37]

Sure, done. Should this also be the case for installAttributeInternal(Isolate,
Template, Template, ...) above?

[Yuki, 2016/11/04 08:12:31]

No.  Data property setting on V8 takes a pair of (key, value) and an additional
|data| field.
The above installAttributeInternal is setting the additional data field.  So, I
think it's okay to call it |data|.

But, in this function, this is the value of (key, value) pair.

       perContextData->constructorForType(attribute.data);
 
@@ -106,13 +109,13 @@ void installAttributeInternal(
   if (attribute.propertyLocationConfiguration & V8DOMConfiguration::OnInstance)
     instance
         ->DefineOwnProperty(
-            isolate->GetCurrentContext(), name, data,
+            scriptState->context(), name, data,

[Yuki, 2016/11/01 11:37:54]

The context here must always be the current context regardless of what you're
doing.  Thus isolate->GetCurrentContext() is preferred.  I know your assumption
is scriptState->context() == isolate->GetCurrentContext(), but it's not clear
just from this function definition.  So, I prefer isolate->GetCurrentContext().

[iclelland, 2016/11/01 17:06:27]

I mentioned this in another comment as well; I ran into a problem while testing
this, that after a navigation, the context is reused, but when we encounter the
<meta> tags for the origin trial, the scriptstate's context is initialized, but
isolate->GetCurrentContext() is empty.

This was actually the reason that I switched this file to use ScriptState rather
than the pair (Isolate*,DOMWrapperWorld&).

If that's incorrect, I can change it, but I may need to do something in
OriginTrialContext::initializePendingFeatures to initialize or enter that
context.

[haraken, 2016/11/02 02:35:45]

As shiino-san mentioned, scriptState->context() == isolate->GetCurrentContext()
must hold.

Specifically, in what situation are you calling installDOMXXX when
isolate->GetCurrentContext() is null? It means that you're doing V8 operations
before entering any v8::Context, which seems wrong.

[iclelland, 2016/11/02 13:49:37]

Thanks for your patience. I've reverted this to isolate->GetCurrentContext(). To
be clear, is it still correct to use scriptState->context) when getting
perContextData? (a few lines above this)

[Yuki, 2016/11/04 08:12:31]

Yes, it's correct to use scriptState->context() to get perContextData().

             static_cast<v8::PropertyAttribute>(attribute.attribute))
         .ToChecked();
   if (attribute.propertyLocationConfiguration & V8DOMConfiguration::OnPrototype)
     prototype
         ->DefineOwnProperty(
-            isolate->GetCurrentContext(), name, data,
+            scriptState->context(), name, data,

[Yuki, 2016/11/01 11:37:54]

ditto.

[iclelland, 2016/11/02 13:49:37]

Done.

             static_cast<v8::PropertyAttribute>(attribute.attribute))
         .ToChecked();
   if (attribute.propertyLocationConfiguration & V8DOMConfiguration::OnInterface)
@@ -415,12 +418,11 @@ void V8DOMConfiguration::installAttribute(
 }
 
 void V8DOMConfiguration::installAttribute(
-    v8::Isolate* isolate,
-    const DOMWrapperWorld& world,
+    const ScriptState* scriptState,
     v8::Local<v8::Object> instance,
     v8::Local<v8::Object> prototype,
     const AttributeConfiguration& attribute) {
-  installAttributeInternal(isolate, instance, prototype, attribute, world);
+  installAttributeInternal(scriptState, instance, prototype, attribute);
 }
 
 void V8DOMConfiguration::installAccessors(