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

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

This patch removes the call-time check for origin-trial-enabled features,
replacing it with code that installs features into the V8 context as trial
tokens are encountered. This means that experimental features which were previously polluting the global namespaces are now completely unavailable except when enabled by origin trials. (The specific case of pollution caused by Durable Storage is removed from test expectations in this patch)

Origin trials are now initialized after the V8 context, by calling initializeOriginTrials. This calls, through a series of function pointers, the bindings initialization code for core, modules, and optionally internals (for tests).

This patch implements the design at https://docs.google.com/document/d/1vwKD8sUrr5mtktvEwvO_f2KUsNhpcgLMixYXhD8K4KY

BUG=584367
Committed: https://crrev.com/e3f3c54cae7eea91719344411dbfe89a717f2e9f
Cr-Commit-Position: refs/heads/master@{#396860}

[iclelland, 2016-05-20 18:25:10]

Description was changed from

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

This patch removes the call-time check for origin-trial-enabled features,
replacing it with code that installs features into the V8 context as trial
tokens are encountered.

BUG=584367
==========

to

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

This patch removes the call-time check for origin-trial-enabled features,
replacing it with code that installs features into the V8 context as trial
tokens are encountered.

BUG=584367
==========

[iclelland, 2016-05-20 18:25:11]

iclelland@chromium.org changed reviewers:
+ chasej@chromium.org

[iclelland, 2016-05-20 18:25:53]

chasej, can you take a preliminary look? There is still some work to do on this
before it's complete, but I believe that the mechanism is sound.

[chasej, 2016-05-20 20:18:14]

Looking pretty good so far.

It would help to link to the design doc in the CL description.

https://codereview.chromium.org/2005433002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp
(right):

https://codereview.chromium.org/2005433002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp:560: if
(originTrialContext->isFeatureEnabled("DurableStorage", nullptr) &&
!originTrialContext->featureBindingsInstalled("DurableStorage")) {
Should this check if the bindings have been installed first? I would think that
check is cheap, and certainly faster than the isFeatureEnabled() check.  That
would be an optimization for the {enabled, already installed} scenario, but I
don't know how often that would occur (if ever). For the {not enabled, *}
scenarios it does mean it would always do the installed check, which the current
implementation avoids.

https://codereview.chromium.org/2005433002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp:560: if
(originTrialContext->isFeatureEnabled("DurableStorage", nullptr) &&
!originTrialContext->featureBindingsInstalled("DurableStorage")) {
Add a comment/TODO that this will eventually be generated code?

[iclelland, 2016-05-26 15:57:58]

Description was changed from

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

This patch removes the call-time check for origin-trial-enabled features,
replacing it with code that installs features into the V8 context as trial
tokens are encountered.

BUG=584367
==========

to

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

This patch removes the call-time check for origin-trial-enabled features,
replacing it with code that installs features into the V8 context as trial
tokens are encountered.

This implements the design at
https://docs.google.com/document/d/1vwKD8sUrr5mtktvEwvO_f2KUsNhpcgLMixYXhD8K4KY

Note that in this patch, origin-trial-enabled attributes, installed on V8
objects, are defined as accessors, because they require a custom getter, which
Object::DefineProperty does not support. An alternative would be to switch these
interface constructor attributes to accessors in all cases, but the comment at
crrev.com/8988a332 suggests that that may not be correct.

BUG=584367
==========

[iclelland, 2016-05-26 17:09:22]

Description updated as well.

https://codereview.chromium.org/2005433002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp
(right):

https://codereview.chromium.org/2005433002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp:560: if
(originTrialContext->isFeatureEnabled("DurableStorage", nullptr) &&
!originTrialContext->featureBindingsInstalled("DurableStorage")) {
On 2016/05/20 20:18:14, chasej wrote:
> Add a comment/TODO that this will eventually be generated code?

Done.

https://codereview.chromium.org/2005433002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp:560: if
(originTrialContext->isFeatureEnabled("DurableStorage", nullptr) &&
!originTrialContext->featureBindingsInstalled("DurableStorage")) {
On 2016/05/20 20:18:14, chasej wrote:
> Should this check if the bindings have been installed first? I would think
that
> check is cheap, and certainly faster than the isFeatureEnabled() check.  That
> would be an optimization for the {enabled, already installed} scenario, but I
> don't know how often that would occur (if ever). For the {not enabled, *}
> scenarios it does mean it would always do the installed check, which the
current
> implementation avoids.

Done. Also in WebCoreTestSupport.cpp

[iclelland, 2016-05-26 18:07:33]

Description was changed from

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

This patch removes the call-time check for origin-trial-enabled features,
replacing it with code that installs features into the V8 context as trial
tokens are encountered.

This implements the design at
https://docs.google.com/document/d/1vwKD8sUrr5mtktvEwvO_f2KUsNhpcgLMixYXhD8K4KY

Note that in this patch, origin-trial-enabled attributes, installed on V8
objects, are defined as accessors, because they require a custom getter, which
Object::DefineProperty does not support. An alternative would be to switch these
interface constructor attributes to accessors in all cases, but the comment at
crrev.com/8988a332 suggests that that may not be correct.

BUG=584367
==========

to

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

This patch removes the call-time check for origin-trial-enabled features,
replacing it with code that installs features into the V8 context as trial
tokens are encountered. This means that experimental features which were
previously polluting the global namespaces are now completely unavailable except
when enabled by origin trials. (The specific case of pollution caused by Durable
Storage is removed from test expectations in this patch)

Origin trials are now initialized after the V8 context, by calling
initializeOriginTrials. This calls, through a series of function pointers, the
bindings initialization code for core, modules, and optionally internals (for
tests).

This patch implements the design at
https://docs.google.com/document/d/1vwKD8sUrr5mtktvEwvO_f2KUsNhpcgLMixYXhD8K4KY

Note that in this patch, origin-trial-enabled attributes, installed on V8
objects, are defined as accessors, because they require a custom getter, which
Object::DefineProperty does not support. An alternative would be to switch these
interface constructor attributes to accessors in all cases, but the comment at
crrev.com/8988a332 suggests that that may not be correct.

BUG=584367
==========

[iclelland, 2016-05-26 18:24:25]

iclelland@chromium.org changed reviewers:
+ haraken@chromium.org

[iclelland, 2016-05-26 18:24:26]

+r haraken -- Can you PTAL at this? Thanks!

[haraken, 2016-05-27 00:01:43]

https://codereview.chromium.org/2005433002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/V8Binding.cpp (right):

https://codereview.chromium.org/2005433002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8Binding.cpp:821: void
initializeOriginTrialsCore(v8::Local<v8::Context> context, const
DOMWrapperWorld& world)

Who calls this method? I guess initializeOriginTrialsForModules need to call
initializeOriginTrialsCore?

Another option would be to follow the same pattern as
toExecutionContextForModules. You can register a function pointer of
initializeOriginTrialsForModules and make initializeOriginTrialsCore call
initializeOriginTrialsForModules. (I'd prefer this.)

https://codereview.chromium.org/2005433002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp (right):

https://codereview.chromium.org/2005433002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp:64: void
installAttributeInternal(v8::Isolate* isolate, v8::Local<v8::Object> instance,
v8::Local<v8::Object> prototype, const
V8DOMConfiguration::AttributeConfiguration& attribute, const DOMWrapperWorld&
world)

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?

https://codereview.chromium.org/2005433002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp
(right):

https://codereview.chromium.org/2005433002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp:545: void
initializeOriginTrialsForModules(v8::Local<v8::Context> context, const
DOMWrapperWorld& world)

This method should take ScriptState. ScriptState contains information about both
context and world.

https://codereview.chromium.org/2005433002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp:558:
context->Enter();

ScriptState::Scope scope(scriptState);

https://codereview.chromium.org/2005433002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp:568: if
(!originTrialContext->featureBindingsInstalled("DurableStorage") &&
(RuntimeEnabledFeatures::durableStorageEnabled() ||
originTrialContext->isFeatureEnabled("DurableStorage", nullptr))) {

Let's drop RuntimeEnableFeatures::durableStorageEnabled() for now, because the
installation of runtime-enabled attributes is still handled by the IDL compiler.

https://codereview.chromium.org/2005433002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp:581:
originTrialContext->setFeatureBindingsInstalled("DurableStorage");

Before this CL, how was DurableStorage installed?

https://codereview.chromium.org/2005433002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/templates/interface_base.cpp (right):

https://codereview.chromium.org/2005433002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/interface_base.cpp:371: void
{{v8_class_or_partial}}::install{{group.grouper}}(v8::Isolate* isolate, const
DOMWrapperWorld& world, v8::Local<v8::Object> instance, v8::Local<v8::Function>
interface)

Sorry, I got lost. Who calls the install methods?

https://codereview.chromium.org/2005433002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp
(right):

https://codereview.chromium.org/2005433002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:263:
ScriptState* state = ScriptState::forMainWorld(frame);

Do you have a plan to enable the origin-trials attributes on isolated worlds?

https://codereview.chromium.org/2005433002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/testing/v8/WebCoreTestSupport.cpp (right):

https://codereview.chromium.org/2005433002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/testing/v8/WebCoreTestSupport.cpp:85: void
initializeOriginTrialsForTests(v8::Local<v8::Context> context, const
blink::DOMWrapperWorld& world)

Hmm, this adds a couple of complexity.

Alternately, can you write tests in layout tests? You can add origin-trials
attributes to window.internals (see Internal.idl) and enable the attributes
using <meta> tags.

https://codereview.chromium.org/2005433002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/2005433002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:352: // If Origin Trials
have been registered before the V8 context was ready,

I'm just curious but how can we enable origin-trials attributes on workers?
Workers won't have <meta> tags... Is there any special syntax for HTTP headers?

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

Thanks for the quick review! I think I've explained everything in comments, but
please ask if it's still unclear.

https://codereview.chromium.org/2005433002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/V8Binding.cpp (right):

https://codereview.chromium.org/2005433002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8Binding.cpp:821: void
initializeOriginTrialsCore(v8::Local<v8::Context> context, const
DOMWrapperWorld& world)
On 2016/05/27 00:01:42, haraken wrote:
> 
> Who calls this method? I guess initializeOriginTrialsForModules need to call
> initializeOriginTrialsCore?

Yes, when registerInitializeOriginTrialsForModules changes the
s_initializeOriginTrials function pointer, it receives a pointer to this
function, which it stores, and then calls as its first action.

> 
> Another option would be to follow the same pattern as
> toExecutionContextForModules. You can register a function pointer of
> initializeOriginTrialsForModules and make initializeOriginTrialsCore call
> initializeOriginTrialsForModules. (I'd prefer this.)

I originally wrote it that way here (mimicking that pattern).

The problem, (besides getting the core/modules dependency backwards) is that it
was difficult to imagine a good way to extend it, and I need to extend it for
tests.

If this file stores a "forModules" pointer, and V8BindingForModules doesn't
store anything, and I want the code that manages the internals object to insert
its own initialization function into the chain, then I'd need to break the
layering even further: Either *core* needs to also call an initializeForTests
pointer, or *modules* needs to have an initializeForTests pointer, when neither
of them should, in general, even be aware of tests.

https://codereview.chromium.org/2005433002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp (right):

https://codereview.chromium.org/2005433002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp:64: void
installAttributeInternal(v8::Isolate* isolate, v8::Local<v8::Object> instance,
v8::Local<v8::Object> prototype, const
V8DOMConfiguration::AttributeConfiguration& attribute, const DOMWrapperWorld&
world)
On 2016/05/27 00:01:42, haraken wrote:
> 
> 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?

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.

https://codereview.chromium.org/2005433002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp
(right):

https://codereview.chromium.org/2005433002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp:545: void
initializeOriginTrialsForModules(v8::Local<v8::Context> context, const
DOMWrapperWorld& world)
On 2016/05/27 00:01:42, haraken wrote:
> 
> This method should take ScriptState. ScriptState contains information about
both
> context and world.

That's a great idea -- thank you for that!

https://codereview.chromium.org/2005433002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp:558:
context->Enter();
On 2016/05/27 00:01:42, haraken wrote:
> 
> ScriptState::Scope scope(scriptState);

Done.

https://codereview.chromium.org/2005433002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp:568: if
(!originTrialContext->featureBindingsInstalled("DurableStorage") &&
(RuntimeEnabledFeatures::durableStorageEnabled() ||
originTrialContext->isFeatureEnabled("DurableStorage", nullptr))) {
On 2016/05/27 00:01:42, haraken wrote:
> 
> Let's drop RuntimeEnableFeatures::durableStorageEnabled() for now, because the
> installation of runtime-enabled attributes is still handled by the IDL
compiler.

DurableStorage isn't a runtime-enabled attribute anymore, though. It's
origin-trial-enabled. The way the bindings work with this CL, there is only one
place that the DurableStorage bindings will be installed -- through the
::installDurableStorage() methods which are intended to be called here. This
check is to ensure that if the runtime flag is set, that the trial will be
installed here unconditionally.

https://codereview.chromium.org/2005433002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp:581:
originTrialContext->setFeatureBindingsInstalled("DurableStorage");
On 2016/05/27 00:01:42, haraken wrote:
> 
> Before this CL, how was DurableStorage installed?

In early development, it was [RuntimeEnabled=DurableStorage] in the IDL, and so
would be installed with the DOM template. When it was moved to an origin trial
(and before this CL), its attributes would have been installed unconditionally
in the DOM template, but contained code in the getters to check for the trial
tokens at call time.

https://codereview.chromium.org/2005433002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/templates/interface_base.cpp (right):

https://codereview.chromium.org/2005433002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/interface_base.cpp:371: void
{{v8_class_or_partial}}::install{{group.grouper}}(v8::Isolate* isolate, const
DOMWrapperWorld& world, v8::Local<v8::Object> instance, v8::Local<v8::Function>
interface)
On 2016/05/27 00:01:42, haraken wrote:
> 
> Sorry, I got lost. Who calls the install methods?

These are called by the installOriginTrialsFor{Core/Modules/Tests} functions in
V8Bindings.cpp/V8BindingsForModules.cpp/WebCoreTestSupport.cpp

https://codereview.chromium.org/2005433002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp
(right):

https://codereview.chromium.org/2005433002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:263:
ScriptState* state = ScriptState::forMainWorld(frame);
On 2016/05/27 00:01:42, haraken wrote:
> 
> Do you have a plan to enable the origin-trials attributes on isolated worlds?

Not at the moment; there's definitely no plan right now to allow origin trials
in extensions; any use of the feature should really be under the control of the
origin. If there are other use cases where it would be useful for an isolated
world to have trial features enabled, I'm definitely open to allowing them.

https://codereview.chromium.org/2005433002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/testing/v8/WebCoreTestSupport.cpp (right):

https://codereview.chromium.org/2005433002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/testing/v8/WebCoreTestSupport.cpp:85: void
initializeOriginTrialsForTests(v8::Local<v8::Context> context, const
blink::DOMWrapperWorld& world)
On 2016/05/27 00:01:42, haraken wrote:
> 
> Hmm, this adds a couple of complexity.
> 
> Alternately, can you write tests in layout tests? You can add origin-trials
> attributes to window.internals (see Internal.idl) and enable the attributes
> using <meta> tags.

That's what we do -- see LayoutTests/http/origin_trials/ . The trouble is that
the tokens are encountered *before* the internals object is instantiated (It's
created very late) and so there is nothing to attach the attributes to at the
time that we would normally attach them.

What I do instead here is to add one more initialization function to the chain,
and then call initializeOriginTrials() one more time to add any new attributes,
*after* window.internals is instantiated.

https://codereview.chromium.org/2005433002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/2005433002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:352: // If Origin Trials
have been registered before the V8 context was ready,
On 2016/05/27 00:01:42, haraken wrote:
> 
> I'm just curious but how can we enable origin-trials attributes on workers?
> Workers won't have <meta> tags... Is there any special syntax for HTTP
headers?
> 

Yes, all resources can now be sent with an HTTP header "origin-trial", which
will also define any tokens to be used for that resource. This is the only way
right now to enable trials on service workers or shared workers.

Dedicated workers can use the HTTP header, and they also inherit any trials that
were enabled by their parent document.

[Yuki, 2016-05-27 09:59:41]

yukishiino@chromium.org changed reviewers:
+ yukishiino@chromium.org

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

https://codereview.chromium.org/2005433002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp (right):

https://codereview.chromium.org/2005433002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp:64: void
installAttributeInternal(v8::Isolate* isolate, v8::Local<v8::Object> instance,
v8::Local<v8::Object> prototype, const
V8DOMConfiguration::AttributeConfiguration& attribute, const DOMWrapperWorld&
world)
On 2016/05/27 03:19:43, iclelland wrote:
> On 2016/05/27 00:01:42, haraken wrote:
> > 
> > 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?
> 
> 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.

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:27]

https://codereview.chromium.org/2005433002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp (right):

https://codereview.chromium.org/2005433002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp:64: void
installAttributeInternal(v8::Isolate* isolate, v8::Local<v8::Object> instance,
v8::Local<v8::Object> prototype, const
V8DOMConfiguration::AttributeConfiguration& attribute, const DOMWrapperWorld&
world)
On 2016/05/27 09:59:42, Yuki wrote:
> On 2016/05/27 03:19:43, iclelland wrote:
> > On 2016/05/27 00:01:42, haraken wrote:
> > > 
> > > 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?
> > 
> > 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.
> 
> 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.

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]

https://codereview.chromium.org/2005433002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp (right):

https://codereview.chromium.org/2005433002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp:64: void
installAttributeInternal(v8::Isolate* isolate, v8::Local<v8::Object> instance,
v8::Local<v8::Object> prototype, const
V8DOMConfiguration::AttributeConfiguration& attribute, const DOMWrapperWorld&
world)
On 2016/05/27 13:51:26, iclelland wrote:
> On 2016/05/27 09:59:42, Yuki wrote:
> > On 2016/05/27 03:19:43, iclelland wrote:
> > > On 2016/05/27 00:01:42, haraken wrote:
> > > > 
> > > > 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?
> > > 
> > > 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.
> > 
> > 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.
> 
> 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)

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]

https://codereview.chromium.org/2005433002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp (right):

https://codereview.chromium.org/2005433002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp:64: void
installAttributeInternal(v8::Isolate* isolate, v8::Local<v8::Object> instance,
v8::Local<v8::Object> prototype, const
V8DOMConfiguration::AttributeConfiguration& attribute, const DOMWrapperWorld&
world)
On 2016/05/27 15:09:42, Yuki wrote:
> On 2016/05/27 13:51:26, iclelland wrote:
> > On 2016/05/27 09:59:42, Yuki wrote:
> > > On 2016/05/27 03:19:43, iclelland wrote:
> > > > On 2016/05/27 00:01:42, haraken wrote:
> > > > > 
> > > > > 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?
> > > > 
> > > > 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.
> > > 
> > > 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.
> > 
> > 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)
> 
> 1. This function is currently NOT used.  So it's okay to rewrite as you like
(or
> remove it).
(I presume here that you're talking about this new version of the
installAttributeInternal function)

> 
> 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.
> 

That you for that clarification; it's making a little bit more sense now.

> 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().

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)

> 
> 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).

[haraken, 2016-05-27 22:25:17]

https://codereview.chromium.org/2005433002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/templates/interface_base.cpp (right):

https://codereview.chromium.org/2005433002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/interface_base.cpp:371: void
{{v8_class_or_partial}}::install{{group.grouper}}(v8::Isolate* isolate, const
DOMWrapperWorld& world, v8::Local<v8::Object> instance, v8::Local<v8::Function>
interface)
On 2016/05/27 03:19:43, iclelland wrote:
> On 2016/05/27 00:01:42, haraken wrote:
> > 
> > Sorry, I got lost. Who calls the install methods?
> 
> These are called by the installOriginTrialsFor{Core/Modules/Tests} functions
in
> V8Bindings.cpp/V8BindingsForModules.cpp/WebCoreTestSupport.cpp

installOriginTrialsForCore doesn't call any installXXX functions, so I was
wondering why. But I now realized that this is just because we don't yet have
any origin-enabled attributes/methods. (DurableStorage is the only use case,
which is manually handled in installOriginTrialsForModules.)

https://codereview.chromium.org/2005433002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp
(right):

https://codereview.chromium.org/2005433002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:263:
ScriptState* state = ScriptState::forMainWorld(frame);
On 2016/05/27 03:19:43, iclelland wrote:
> On 2016/05/27 00:01:42, haraken wrote:
> > 
> > Do you have a plan to enable the origin-trials attributes on isolated
worlds?
> 
> Not at the moment; there's definitely no plan right now to allow origin trials
> in extensions; any use of the feature should really be under the control of
the
> origin. If there are other use cases where it would be useful for an isolated
> world to have trial features enabled, I'm definitely open to allowing them.

Not supporting isolated worlds at the moment makes sense to me.

https://codereview.chromium.org/2005433002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/testing/v8/WebCoreTestSupport.cpp (right):

https://codereview.chromium.org/2005433002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/testing/v8/WebCoreTestSupport.cpp:85: void
initializeOriginTrialsForTests(v8::Local<v8::Context> context, const
blink::DOMWrapperWorld& world)
On 2016/05/27 03:19:43, iclelland wrote:
> On 2016/05/27 00:01:42, haraken wrote:
> > 
> > Hmm, this adds a couple of complexity.
> > 
> > Alternately, can you write tests in layout tests? You can add origin-trials
> > attributes to window.internals (see Internal.idl) and enable the attributes
> > using <meta> tags.
> 
> That's what we do -- see LayoutTests/http/origin_trials/ . The trouble is that
> the tokens are encountered *before* the internals object is instantiated (It's
> created very late) and so there is nothing to attach the attributes to at the
> time that we would normally attach them.

I'm not sure if it's too late. In common cases, a window object is created after
a <head> tag is closed, where all tokens should have already been encountered.

If you really want to test a case where a new token is encountered after that
happens, can you add a <meta> tag after a <head> tag is closed?

(Then we can remove initializeOriginTrialsForTests and simplify the logic to
register function pointers for initializeOriginTrials.)

https://codereview.chromium.org/2005433002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp
(right):

https://codereview.chromium.org/2005433002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp:566: //
trials on static attributes. (crbug.com/614352)

At which point is the placeholder (i.e., |interface|) replaced with the actual
interface (i.e., StorageManager interface)?

https://codereview.chromium.org/2005433002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp:573:
V8WindowPartial::installDurableStorage(isolate, world, global, interface);

You should pass in ScriptState. ScriptState knows both isolate and world.

https://codereview.chromium.org/2005433002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/testing/v8/WebCoreTestSupport.cpp (right):

https://codereview.chromium.org/2005433002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/testing/v8/WebCoreTestSupport.cpp:112: if
(!originTrialContext->featureBindingsInstalled("Frobulate") &&
originTrialContext->isFeatureEnabled("Frobulate", nullptr)) {

It would be nicer if we generate the if check inside the install method.

Nit: This CL mixes "initialize" and "install". Shall we unify them into
"install"?

[Yuki, 2016-05-30 08:14:20]

https://codereview.chromium.org/2005433002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp (right):

https://codereview.chromium.org/2005433002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp:64: void
installAttributeInternal(v8::Isolate* isolate, v8::Local<v8::Object> instance,
v8::Local<v8::Object> prototype, const
V8DOMConfiguration::AttributeConfiguration& attribute, const DOMWrapperWorld&
world)
On 2016/05/27 15:27:01, iclelland wrote:
> On 2016/05/27 15:09:42, Yuki wrote:
> > On 2016/05/27 13:51:26, iclelland wrote:
> > > On 2016/05/27 09:59:42, Yuki wrote:
> > > > On 2016/05/27 03:19:43, iclelland wrote:
> > > > > On 2016/05/27 00:01:42, haraken wrote:
> > > > > > 
> > > > > > 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?
> > > > > 
> > > > > 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.
> > > > 
> > > > 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.
> > > 
> > > 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)
> > 
> > 1. This function is currently NOT used.  So it's okay to rewrite as you like
> (or
> > remove it).
> (I presume here that you're talking about this new version of the
> installAttributeInternal function)
> 
> > 
> > 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.
> > 
> 
> That you for that clarification; it's making a little bit more sense now.
> 
> > 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().
> 
> 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)

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.


> > 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-30 12:15:35]

https://codereview.chromium.org/2005433002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp (right):

https://codereview.chromium.org/2005433002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp:64: void
installAttributeInternal(v8::Isolate* isolate, v8::Local<v8::Object> instance,
v8::Local<v8::Object> prototype, const
V8DOMConfiguration::AttributeConfiguration& attribute, const DOMWrapperWorld&
world)
> > > 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().
> > 
> > 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)
> 
> 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.

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?

https://codereview.chromium.org/2005433002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/templates/interface_base.cpp (right):

https://codereview.chromium.org/2005433002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/interface_base.cpp:371: void
{{v8_class_or_partial}}::install{{group.grouper}}(v8::Isolate* isolate, const
DOMWrapperWorld& world, v8::Local<v8::Object> instance, v8::Local<v8::Function>
interface)
On 2016/05/27 22:25:17, haraken wrote:
> On 2016/05/27 03:19:43, iclelland wrote:
> > On 2016/05/27 00:01:42, haraken wrote:
> > > 
> > > Sorry, I got lost. Who calls the install methods?
> > 
> > These are called by the installOriginTrialsFor{Core/Modules/Tests} functions
> in
> > V8Bindings.cpp/V8BindingsForModules.cpp/WebCoreTestSupport.cpp
> 
> installOriginTrialsForCore doesn't call any installXXX functions, so I was
> wondering why. But I now realized that this is just because we don't yet have
> any origin-enabled attributes/methods. (DurableStorage is the only use case,
> which is manually handled in installOriginTrialsForModules.)

Yes, that's correct. Currently, the only origin trial feature only binds to
modules.

https://codereview.chromium.org/2005433002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp
(right):

https://codereview.chromium.org/2005433002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp:566: //
trials on static attributes. (crbug.com/614352)
On 2016/05/27 22:25:17, haraken wrote:
> 
> At which point is the placeholder (i.e., |interface|) replaced with the actual
> interface (i.e., StorageManager interface)?

This is probably unnecessarily confusing, and I can move |interface| out of this
method. It doesn't represent the StorageManager interface (for instance); rather
it would be the Window or Navigator interface (or something similar) if an
origin trial was attached to a static attribute, since those generate
AttributeConfigurations where propertyLocationConfiguration is OnInterface.

For example, if the IDL had:

partial interface Navigator {
  [OriginTrialEnabled=Foo] static void createFoo();
}

then we would need to attach the method to the Navigator *interface* rather than
window.navigator, or its prototype. I'm just not supporting that in this CL, but
I wrote the install* methods to take an interface parameter which doesn't ever
get used. I'll remove it from here (and put something back in its place when I
address crbug.com/614352)

https://codereview.chromium.org/2005433002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp:573:
V8WindowPartial::installDurableStorage(isolate, world, global, interface);
On 2016/05/27 22:25:17, haraken wrote:
> 
> You should pass in ScriptState. ScriptState knows both isolate and world.

Yes, I'll do that -- Somehow, I was thinking that the generated bindings code
shouldn't depend on things like ScriptState, but it's all in /bindings/, so it
makes sense to use it.

Thanks

https://codereview.chromium.org/2005433002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/testing/v8/WebCoreTestSupport.cpp (right):

https://codereview.chromium.org/2005433002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/testing/v8/WebCoreTestSupport.cpp:112: if
(!originTrialContext->featureBindingsInstalled("Frobulate") &&
originTrialContext->isFeatureEnabled("Frobulate", nullptr)) {
On 2016/05/27 22:25:17, haraken wrote:
> 
> It would be nicer if we generate the if check inside the install method.

We'd have to pass the OriginTrialContext into the install method, though, to do
that, which seems ugly. I can do it, though, if you think it makes more sense
that way.
Question: There are two parts to the `if` -- the policy check (isFeatureEnabled)
and the only-install-this-once check
(featureBindingsInstalled/setFeatureBindingsInstalled). Should both of them go
into the install method, or just the featureBindingsInstalled calls?

> 
> Nit: This CL mixes "initialize" and "install". Shall we unify them into
> "install"?
Yes, install is the better of the two words. I'll change that.

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

https://codereview.chromium.org/2005433002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp (right):

https://codereview.chromium.org/2005433002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp:64: void
installAttributeInternal(v8::Isolate* isolate, v8::Local<v8::Object> instance,
v8::Local<v8::Object> prototype, const
V8DOMConfiguration::AttributeConfiguration& attribute, const DOMWrapperWorld&
world)
On 2016/05/30 12:15:35, iclelland wrote:
> > > > 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().
> > > 
> > > 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)
> > 
> > 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.
> 
> 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?

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:47]

https://codereview.chromium.org/2005433002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp (right):

https://codereview.chromium.org/2005433002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp:64: void
installAttributeInternal(v8::Isolate* isolate, v8::Local<v8::Object> instance,
v8::Local<v8::Object> prototype, const
V8DOMConfiguration::AttributeConfiguration& attribute, const DOMWrapperWorld&
world)
On 2016/05/30 13:07:58, Yuki wrote:
> On 2016/05/30 12:15:35, iclelland wrote:
> > > > > 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().
> > > > 
> > > > 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)
> > > 
> > > 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.
> > 
> > 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?
> 
> 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.

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.

https://codereview.chromium.org/2005433002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp
(right):

https://codereview.chromium.org/2005433002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp:566: //
trials on static attributes. (crbug.com/614352)
On 2016/05/30 12:15:35, iclelland wrote:
> On 2016/05/27 22:25:17, haraken wrote:
> > 
> > At which point is the placeholder (i.e., |interface|) replaced with the
actual
> > interface (i.e., StorageManager interface)?
> 
> This is probably unnecessarily confusing, and I can move |interface| out of
this
> method. It doesn't represent the StorageManager interface (for instance);
rather
> it would be the Window or Navigator interface (or something similar) if an
> origin trial was attached to a static attribute, since those generate
> AttributeConfigurations where propertyLocationConfiguration is OnInterface.
> 
> For example, if the IDL had:
> 
> partial interface Navigator {
>   [OriginTrialEnabled=Foo] static void createFoo();
> }
> 
> then we would need to attach the method to the Navigator *interface* rather
than
> window.navigator, or its prototype. I'm just not supporting that in this CL,
but
> I wrote the install* methods to take an interface parameter which doesn't ever
> get used. I'll remove it from here (and put something back in its place when I
> address crbug.com/614352) 

Done.

https://codereview.chromium.org/2005433002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp:573:
V8WindowPartial::installDurableStorage(isolate, world, global, interface);
On 2016/05/30 12:15:35, iclelland wrote:
> On 2016/05/27 22:25:17, haraken wrote:
> > 
> > You should pass in ScriptState. ScriptState knows both isolate and world.
> 
> Yes, I'll do that -- Somehow, I was thinking that the generated bindings code
> shouldn't depend on things like ScriptState, but it's all in /bindings/, so it
> makes sense to use it.
> 
> Thanks

Done.

[iclelland, 2016-05-30 15:08:37]

Description was changed from

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

This patch removes the call-time check for origin-trial-enabled features,
replacing it with code that installs features into the V8 context as trial
tokens are encountered. This means that experimental features which were
previously polluting the global namespaces are now completely unavailable except
when enabled by origin trials. (The specific case of pollution caused by Durable
Storage is removed from test expectations in this patch)

Origin trials are now initialized after the V8 context, by calling
initializeOriginTrials. This calls, through a series of function pointers, the
bindings initialization code for core, modules, and optionally internals (for
tests).

This patch implements the design at
https://docs.google.com/document/d/1vwKD8sUrr5mtktvEwvO_f2KUsNhpcgLMixYXhD8K4KY

Note that in this patch, origin-trial-enabled attributes, installed on V8
objects, are defined as accessors, because they require a custom getter, which
Object::DefineProperty does not support. An alternative would be to switch these
interface constructor attributes to accessors in all cases, but the comment at
crrev.com/8988a332 suggests that that may not be correct.

BUG=584367
==========

to

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

This patch removes the call-time check for origin-trial-enabled features,
replacing it with code that installs features into the V8 context as trial
tokens are encountered. This means that experimental features which were
previously polluting the global namespaces are now completely unavailable except
when enabled by origin trials. (The specific case of pollution caused by Durable
Storage is removed from test expectations in this patch)

Origin trials are now initialized after the V8 context, by calling
initializeOriginTrials. This calls, through a series of function pointers, the
bindings initialization code for core, modules, and optionally internals (for
tests).

This patch implements the design at
https://docs.google.com/document/d/1vwKD8sUrr5mtktvEwvO_f2KUsNhpcgLMixYXhD8K4KY

BUG=584367
==========

[iclelland, 2016-05-30 15:39:28]

https://codereview.chromium.org/2005433002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/testing/v8/WebCoreTestSupport.cpp (right):

https://codereview.chromium.org/2005433002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/testing/v8/WebCoreTestSupport.cpp:85: void
initializeOriginTrialsForTests(v8::Local<v8::Context> context, const
blink::DOMWrapperWorld& world)
On 2016/05/27 22:25:17, haraken wrote:
> On 2016/05/27 03:19:43, iclelland wrote:
> > On 2016/05/27 00:01:42, haraken wrote:
> > > 
> > > Hmm, this adds a couple of complexity.
> > > 
> > > Alternately, can you write tests in layout tests? You can add
origin-trials
> > > attributes to window.internals (see Internal.idl) and enable the
attributes
> > > using <meta> tags.
> > 
> > That's what we do -- see LayoutTests/http/origin_trials/ . The trouble is
that
> > the tokens are encountered *before* the internals object is instantiated
(It's
> > created very late) and so there is nothing to attach the attributes to at
the
> > time that we would normally attach them.
> 
> I'm not sure if it's too late. In common cases, a window object is created
after
> a <head> tag is closed, where all tokens should have already been encountered.

That's true, but it's not window that is a problem here -- it's
window.internals, which is injected some time after the window object is
created.

> 
> If you really want to test a case where a new token is encountered after that
> happens, can you add a <meta> tag after a <head> tag is closed?
> 
> (Then we can remove initializeOriginTrialsForTests and simplify the logic to
> register function pointers for initializeOriginTrials.)

V8Internals is only included in testing binaries. Regardless of when we call
V8Internals::installOriginTrialsSampleAPI, we can't reference it in
Source/bindings/core/v8/V8Binding.cpp or
Source/bindings/modules/v8/V8BindingForModules.cpp.

[haraken, 2016-05-31 00:16:32]

LGTM on my side.

https://codereview.chromium.org/2005433002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp (right):

https://codereview.chromium.org/2005433002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp:75: // an
attribute conguration for a constructor.

configuration

https://codereview.chromium.org/2005433002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/templates/interface_base.cpp (right):

https://codereview.chromium.org/2005433002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/interface_base.cpp:382: // static
attributes to existing interfaces. This is not currently

to install static attributes on existing interfaces

[iclelland, 2016-05-31 03:17:31]

https://codereview.chromium.org/2005433002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp (right):

https://codereview.chromium.org/2005433002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp:75: // an
attribute conguration for a constructor.
On 2016/05/31 00:16:32, haraken wrote:
> 
> configuration

Done.

https://codereview.chromium.org/2005433002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/templates/interface_base.cpp (right):

https://codereview.chromium.org/2005433002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/interface_base.cpp:382: // static
attributes to existing interfaces. This is not currently
On 2016/05/31 00:16:32, haraken wrote:
> 
> to install static attributes on existing interfaces

Done. Also clarified that static attributes are only unsupported in origin
trials, not generally.

[Yuki, 2016-05-31 05:05:32]

https://codereview.chromium.org/2005433002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/V8Binding.h (right):

https://codereview.chromium.org/2005433002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8Binding.h:72: using
installOriginTrialsFunction = void (*)(ScriptState*);
Type name should starts with a capital letter, I think.

https://codereview.chromium.org/2005433002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp
(right):

https://codereview.chromium.org/2005433002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp:564:
v8::Local<v8::String> navigatorName = v8::String::NewFromUtf8(isolate,
"navigator", v8::NewStringType::kNormal).ToLocalChecked();
You can use NewFromOneByte, and it seems faster than NewFromUtf8.

https://codereview.chromium.org/2005433002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/templates/interface_base.cpp (right):

https://codereview.chromium.org/2005433002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/interface_base.cpp:378:
V8DOMConfiguration::installAttribute(scriptState->isolate(),
scriptState->world(), instance, prototype,
attribute{{attribute.name}}Configuration);
Note for the binding team: We may want to rename installAttribute to
installInterfaceObject or something like that.

https://codereview.chromium.org/2005433002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/interface_base.cpp:386: // trials
on static attributes. (crbug.com/614352)
I don't understand this comment.

Are you trying to install a static attribute?  Then, you really need an
interface object where the static attribute is going to be installed. 
Otherwise, it's meaningless.

If you don't have any static attribute yet.  Then, you don't need to create a
function object as an interface object.  Just use an empty handle, passing
v8::Local<v8::Function>() should work.

If you need an interface object, then you can use
perIsolateData->construtorForType().

[iclelland, 2016-05-31 05:34:52]

https://codereview.chromium.org/2005433002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/V8Binding.h (right):

https://codereview.chromium.org/2005433002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8Binding.h:72: using
installOriginTrialsFunction = void (*)(ScriptState*);
On 2016/05/31 05:05:32, Yuki wrote:
> Type name should starts with a capital letter, I think.

Done.

https://codereview.chromium.org/2005433002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp
(right):

https://codereview.chromium.org/2005433002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp:564:
v8::Local<v8::String> navigatorName = v8::String::NewFromUtf8(isolate,
"navigator", v8::NewStringType::kNormal).ToLocalChecked();
On 2016/05/31 05:05:32, Yuki wrote:
> You can use NewFromOneByte, and it seems faster than NewFromUtf8.

Done.

https://codereview.chromium.org/2005433002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/templates/interface_base.cpp (right):

https://codereview.chromium.org/2005433002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/interface_base.cpp:386: // trials
on static attributes. (crbug.com/614352)
On 2016/05/31 05:05:32, Yuki wrote:
> I don't understand this comment.
> 
> Are you trying to install a static attribute?  
No. 
> Then, you really need an
> interface object where the static attribute is going to be installed. 
Yes, that's what I was trying to say with the comment. I was trying to explain
why I was generating a useless function object to pass to installAccessor.
> Otherwise, it's meaningless.
> 
> If you don't have any static attribute yet.  Then, you don't need to create a
> function object as an interface object.  Just use an empty handle, passing
> v8::Local<v8::Function>() should work.
That's definitely cleaner than what I was doing. I've switch to that (and
removed the unfortunately confusing comment.) Thanks.

> 
> If you need an interface object, then you can use
> perIsolateData->construtorForType().

[Yuki, 2016-05-31 05:38:25]

LGTM on my side, too.

[iclelland, 2016-05-31 12:31:10]

iclelland@chromium.org changed reviewers:
+ rbyers@chromium.org

[iclelland, 2016-05-31 12:31:11]

+r rbyers for the cleanup in the webexposed test expectations

[Rick Byers, 2016-05-31 12:50:13]

On 2016/05/31 12:31:11, iclelland wrote:
> +r rbyers for the cleanup in the webexposed test expectations

webexposed LGTM - woot, thank you for the diligence here!

[chasej, 2016-05-31 15:42:13]

https://codereview.chromium.org/2005433002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/V8Binding.h (right):

https://codereview.chromium.org/2005433002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8Binding.h:869: // installd, but may
be called multiple times, as trial tokens are
Nit: spelling for "installd"

https://codereview.chromium.org/2005433002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/tests/results/core/V8TestObject.cpp
(right):

https://codereview.chromium.org/2005433002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/tests/results/core/V8TestObject.cpp:11943:
v8::Local<v8::Signature> signature;
The signature variable is defined twice. At best, it isn't the cleanest code. At
worst it would cause compile issues?

https://codereview.chromium.org/2005433002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/origin_trials/OriginTrialContext.h (right):

https://codereview.chromium.org/2005433002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/origin_trials/OriginTrialContext.h:78: void
initializePendingTrials();
Nit: maybe this should be named "initializePendingFeatures"? While the class is
OriginTrialContext, the implementation generally refers to features, not trials.

[iclelland, 2016-05-31 16:03:44]

https://codereview.chromium.org/2005433002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/V8Binding.h (right):

https://codereview.chromium.org/2005433002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8Binding.h:869: // installd, but may
be called multiple times, as trial tokens are
On 2016/05/31 15:42:13, chasej wrote:
> Nit: spelling for "installd"

Done.

https://codereview.chromium.org/2005433002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/tests/results/core/V8TestObject.cpp
(right):

https://codereview.chromium.org/2005433002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/tests/results/core/V8TestObject.cpp:11943:
v8::Local<v8::Signature> signature;
On 2016/05/31 15:42:13, chasej wrote:
> The signature variable is defined twice. At best, it isn't the cleanest code.
At
> worst it would cause compile issues?

It certainly would; thanks for catching it. Fixed.

https://codereview.chromium.org/2005433002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/origin_trials/OriginTrialContext.h (right):

https://codereview.chromium.org/2005433002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/origin_trials/OriginTrialContext.h:78: void
initializePendingTrials();
On 2016/05/31 15:42:13, chasej wrote:
> Nit: maybe this should be named "initializePendingFeatures"? While the class
is
> OriginTrialContext, the implementation generally refers to features, not
trials.

Done.

[chasej, 2016-05-31 16:14:31]

LGTM.

[iclelland, 2016-05-31 17:34:21]

The CQ bit was checked by iclelland@chromium.org

[iclelland, 2016-05-31 17:34:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, rbyers@chromium.org, yukishiino@chromium.org
Link to the patchset: https://codereview.chromium.org/2005433002/#ps260001
(title: "Fix multiple definition of signature; addressing nits")

[commit-bot: I haz the power, 2016-05-31 17:35:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2005433002/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2005433002/260001

[commit-bot: I haz the power, 2016-05-31 18:04:40]

Description was changed from

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

This patch removes the call-time check for origin-trial-enabled features,
replacing it with code that installs features into the V8 context as trial
tokens are encountered. This means that experimental features which were
previously polluting the global namespaces are now completely unavailable except
when enabled by origin trials. (The specific case of pollution caused by Durable
Storage is removed from test expectations in this patch)

Origin trials are now initialized after the V8 context, by calling
initializeOriginTrials. This calls, through a series of function pointers, the
bindings initialization code for core, modules, and optionally internals (for
tests).

This patch implements the design at
https://docs.google.com/document/d/1vwKD8sUrr5mtktvEwvO_f2KUsNhpcgLMixYXhD8K4KY

BUG=584367
==========

to

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

This patch removes the call-time check for origin-trial-enabled features,
replacing it with code that installs features into the V8 context as trial
tokens are encountered. This means that experimental features which were
previously polluting the global namespaces are now completely unavailable except
when enabled by origin trials. (The specific case of pollution caused by Durable
Storage is removed from test expectations in this patch)

Origin trials are now initialized after the V8 context, by calling
initializeOriginTrials. This calls, through a series of function pointers, the
bindings initialization code for core, modules, and optionally internals (for
tests).

This patch implements the design at
https://docs.google.com/document/d/1vwKD8sUrr5mtktvEwvO_f2KUsNhpcgLMixYXhD8K4KY

BUG=584367
==========

[commit-bot: I haz the power, 2016-05-31 18:04:41]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2016-05-31 18:06:26]

Description was changed from

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

This patch removes the call-time check for origin-trial-enabled features,
replacing it with code that installs features into the V8 context as trial
tokens are encountered. This means that experimental features which were
previously polluting the global namespaces are now completely unavailable except
when enabled by origin trials. (The specific case of pollution caused by Durable
Storage is removed from test expectations in this patch)

Origin trials are now initialized after the V8 context, by calling
initializeOriginTrials. This calls, through a series of function pointers, the
bindings initialization code for core, modules, and optionally internals (for
tests).

This patch implements the design at
https://docs.google.com/document/d/1vwKD8sUrr5mtktvEwvO_f2KUsNhpcgLMixYXhD8K4KY

BUG=584367
==========

to

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

This patch removes the call-time check for origin-trial-enabled features,
replacing it with code that installs features into the V8 context as trial
tokens are encountered. This means that experimental features which were
previously polluting the global namespaces are now completely unavailable except
when enabled by origin trials. (The specific case of pollution caused by Durable
Storage is removed from test expectations in this patch)

Origin trials are now initialized after the V8 context, by calling
initializeOriginTrials. This calls, through a series of function pointers, the
bindings initialization code for core, modules, and optionally internals (for
tests).

This patch implements the design at
https://docs.google.com/document/d/1vwKD8sUrr5mtktvEwvO_f2KUsNhpcgLMixYXhD8K4KY

BUG=584367

Committed: https://crrev.com/e3f3c54cae7eea91719344411dbfe89a717f2e9f
Cr-Commit-Position: refs/heads/master@{#396860}
==========

[commit-bot: I haz the power, 2016-05-31 18:06:28]

Patchset 14 (id:??) landed as
https://crrev.com/e3f3c54cae7eea91719344411dbfe89a717f2e9f
Cr-Commit-Position: refs/heads/master@{#396860}