[OriginTrials] Support OriginTrialEnabled IDL attribute on constants

[OriginTrials] Support OriginTrialEnabled IDL attribute on constants

BUG=619092
Committed: https://crrev.com/9500e40f0af7830f1024693b715d5abb6a7e37d4
Cr-Commit-Position: refs/heads/master@{#400390}

[iclelland, 2016-06-10 17:37:33]

Description was changed from

==========
[OriginTrials] Support OriginTrialEnabled IDL attribute on constants

BUG=619092
==========

to

==========
[OriginTrials] Support OriginTrialEnabled IDL attribute on constants

BUG=619092
==========

[iclelland, 2016-06-10 17:37:33]

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

[iclelland, 2016-06-10 17:38:06]

+r chasej: for origin trials, but PTAL at everything
+r haraken for bindings generation

Thanks!

[iclelland, 2016-06-10 17:39:04]

This will be the starting point for allowing v8 methods to be enabled through
Origin Trials as well, in a follow-up CL

[chasej, 2016-06-10 20:09:30]

https://codereview.chromium.org/2057273002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/scripts/v8_utilities.py (right):

https://codereview.chromium.org/2057273002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/scripts/v8_utilities.py:126: def
for_feature(items, feature_name):
Should this have more specific name like 'for_origin_trial_feature'. With
runtime-enabled features, this seems ambiguous.

https://codereview.chromium.org/2057273002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/templates/interface_base.cpp (right):

https://codereview.chromium.org/2057273002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/templates/interface_base.cpp:387: {#
Runtime-enabled constants #}
Should this be origin trial enabled constants?

[iclelland, 2016-06-10 20:24:01]

https://codereview.chromium.org/2057273002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/scripts/v8_utilities.py (right):

https://codereview.chromium.org/2057273002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/scripts/v8_utilities.py:126: def
for_feature(items, feature_name):
On 2016/06/10 20:09:30, chasej wrote:
> Should this have more specific name like 'for_origin_trial_feature'. With
> runtime-enabled features, this seems ambiguous.

The intention was to eventually also include runtime features as well, and split
them off so that RTE features could be initialized one-at-a-time.

However, that's for another day, and I can change the function name at that
point, or do something different, maybe.

Changed.

https://codereview.chromium.org/2057273002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/templates/interface_base.cpp (right):

https://codereview.chromium.org/2057273002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/templates/interface_base.cpp:387: {#
Runtime-enabled constants #}
On 2016/06/10 20:09:30, chasej wrote:
> Should this be origin trial enabled constants?

Yes. Yes it should. Thanks :)

[chasej, 2016-06-10 20:26:44]

lgtm

[haraken, 2016-06-13 01:12:58]

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

[haraken, 2016-06-13 01:12:59]

https://codereview.chromium.org/2057273002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp (right):

https://codereview.chromium.org/2057273002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp:201: void
installConstantInternal(v8::Isolate* isolate, v8::Local<v8::Object> prototype,
const V8DOMConfiguration::ConstantConfiguration& constant, const
DOMWrapperWorld& world)

Why do you need to pass DOMWrapperWorld? It's unused.

https://codereview.chromium.org/2057273002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp:205:
v8CallOrCrash(prototype->DefineOwnProperty(isolate->GetCurrentContext(), name,
value));

Per the spec, constant must be defined on both prototype object and instance
object (See line 197 and 198). But this CL is defining constants only on
prototype objects.

https://codereview.chromium.org/2057273002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/scripts/v8_interface.py (right):

https://codereview.chromium.org/2057273002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/scripts/v8_interface.py:101: def
origin_trial_feature_names(constants, attributes):

Are you going to support methods in the future?

[Yuki, 2016-06-13 06:47:55]

https://codereview.chromium.org/2057273002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/templates/interface_base.cpp (right):

https://codereview.chromium.org/2057273002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface_base.cpp:369: {% for
origin_trial_feature_name in origin_trial_feature_names %}{{newline}}
Do we really need this {{newline}}?

https://codereview.chromium.org/2057273002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface_base.cpp:373:
v8::Local<v8::Signature> signature;
Passing the empty signature to V8DOMConfiguration::installAccessor() is really
dangerous.  You must pass the effective signature as follows.

    v8::Local<v8::Signature> signature = v8::Signature::New(isolate,
interfaceTemplate);

https://codereview.chromium.org/2057273002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface_base.cpp:376: {% for
attribute in attributes|for_origin_trial_feature(origin_trial_feature_name) |
unique_by('name') | sort %}
Put spaces on the both sides of | operator to be consistent.

[iclelland, 2016-06-15 12:12:04]

https://codereview.chromium.org/2057273002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp (right):

https://codereview.chromium.org/2057273002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp:205:
v8CallOrCrash(prototype->DefineOwnProperty(isolate->GetCurrentContext(), name,
value));
On 2016/06/13 01:12:59, haraken wrote:
> 
> Per the spec, constant must be defined on both prototype object and instance
> object (See line 197 and 198). But this CL is defining constants only on
> prototype objects.
> 

I'm adding them now on the object returned from constructorForType(); as far as
I can tell through reading the code, that is the interface object. Is that
correct?

Also, my test case for this would bind them to window.Internals, which according
to spec should be the defined interface object for the V8Internals, but that
doesn't appear to be defined on window. ("internals", the instance, is defined,
but not "Internals") Is that a bug?

[Yuki, 2016-06-15 12:32:11]

https://codereview.chromium.org/2057273002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp (right):

https://codereview.chromium.org/2057273002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp:205:
v8CallOrCrash(prototype->DefineOwnProperty(isolate->GetCurrentContext(), name,
value));
On 2016/06/15 12:12:04, iclelland wrote:
> On 2016/06/13 01:12:59, haraken wrote:
> > 
> > Per the spec, constant must be defined on both prototype object and instance
> > object (See line 197 and 198). But this CL is defining constants only on
> > prototype objects.
> > 
> 
> I'm adding them now on the object returned from constructorForType(); as far
as
> I can tell through reading the code, that is the interface object. Is that
> correct?

Yes, the function object returned by constructorForType() is an interface
object.


> Also, my test case for this would bind them to window.Internals, which
according
> to spec should be the defined interface object for the V8Internals, but that
> doesn't appear to be defined on window. ("internals", the instance, is
defined,
> but not "Internals") Is that a bug?

It's not a bug.  Not all the interface objects are required to be exposed.  The
spec defines which interface objects must be exposed and which must not.  Plus,
window.internals is Blink-specific object, not defined by the spec.  So, it's
fine.

[iclelland, 2016-06-15 14:07:04]

Patchset #3 (id:40001) has been deleted

[iclelland, 2016-06-15 14:08:41]

https://codereview.chromium.org/2057273002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp (right):

https://codereview.chromium.org/2057273002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp:201: void
installConstantInternal(v8::Isolate* isolate, v8::Local<v8::Object> prototype,
const V8DOMConfiguration::ConstantConfiguration& constant, const
DOMWrapperWorld& world)
On 2016/06/13 01:12:59, haraken wrote:
> 
> Why do you need to pass DOMWrapperWorld? It's unused.

Not needed. removed. (Replaced with WrapperTypeInfo, to resolve Yuki's comment
below)

https://codereview.chromium.org/2057273002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp:205:
v8CallOrCrash(prototype->DefineOwnProperty(isolate->GetCurrentContext(), name,
value));
On 2016/06/15 12:32:11, Yuki wrote:
> On 2016/06/15 12:12:04, iclelland wrote:
> > On 2016/06/13 01:12:59, haraken wrote:
> > > 
> > > Per the spec, constant must be defined on both prototype object and
instance
> > > object (See line 197 and 198). But this CL is defining constants only on
> > > prototype objects.
> > > 
> > 
> > I'm adding them now on the object returned from constructorForType(); as far
> as
> > I can tell through reading the code, that is the interface object. Is that
> > correct?
> 
> Yes, the function object returned by constructorForType() is an interface
> object.
> 
> 
> > Also, my test case for this would bind them to window.Internals, which
> according
> > to spec should be the defined interface object for the V8Internals, but that
> > doesn't appear to be defined on window. ("internals", the instance, is
> defined,
> > but not "Internals") Is that a bug?
> 
> It's not a bug.  Not all the interface objects are required to be exposed. 
The
> spec defines which interface objects must be exposed and which must not. 
Plus,
> window.internals is Blink-specific object, not defined by the spec.  So, it's
> fine.

I see, thanks. I'm not sure how to write a proper layout test for this then,
without explicitly exposing this interface. I've done manual testing, adding
additional constants to other origin trials, and verified that it works.

https://codereview.chromium.org/2057273002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/scripts/v8_interface.py (right):

https://codereview.chromium.org/2057273002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/scripts/v8_interface.py:101: def
origin_trial_feature_names(constants, attributes):
On 2016/06/13 01:12:59, haraken wrote:
> 
> Are you going to support methods in the future?

Yes, that will be in a follow-up CL to this.

https://codereview.chromium.org/2057273002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/templates/interface_base.cpp (right):

https://codereview.chromium.org/2057273002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface_base.cpp:369: {% for
origin_trial_feature_name in origin_trial_feature_names %}{{newline}}
On 2016/06/13 06:47:55, Yuki wrote:
> Do we really need this {{newline}}?

It's not strictly needed for correctness, but it provides the expected blank
line between function definitions.

https://codereview.chromium.org/2057273002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface_base.cpp:373:
v8::Local<v8::Signature> signature;
On 2016/06/13 06:47:55, Yuki wrote:
> Passing the empty signature to V8DOMConfiguration::installAccessor() is really
> dangerous.  You must pass the effective signature as follows.
> 
>     v8::Local<v8::Signature> signature = v8::Signature::New(isolate,
> interfaceTemplate);

Done.

https://codereview.chromium.org/2057273002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface_base.cpp:376: {% for
attribute in attributes|for_origin_trial_feature(origin_trial_feature_name) |
unique_by('name') | sort %}
On 2016/06/13 06:47:55, Yuki wrote:
> Put spaces on the both sides of | operator to be consistent.

Thanks, done.

[haraken, 2016-06-16 00:48:15]

LGTM on my side.

[Yuki, 2016-06-16 08:01:08]

https://codereview.chromium.org/2057273002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp (right):

https://codereview.chromium.org/2057273002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp:204:
v8::Local<v8::Function> interface = perContextData->constructorForType(type);
I'd prefer that the caller passes |interface| to installConstant() as an
argument, because a) it's consistent with other installXxx family, and b) if
you're going to install multiple constants, you can reduce the number of calls
to isolate->GetCurrentContext(), that is not cheap.

https://codereview.chromium.org/2057273002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp:207:
v8CallOrCrash(interface->DefineOwnProperty(isolate->GetCurrentContext(), name,
value));
You need to specify the fourth argument |attributes| of DefineOwnProperty to
make the property v8::ReadOnly and v8::DontDelete (i.e. [[Writable]]: false,
[[Configurable]]: false).

https://codereview.chromium.org/2057273002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/templates/interface_base.cpp (right):

https://codereview.chromium.org/2057273002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface_base.cpp:382:
v8::Local<v8::FunctionTemplate> interfaceTemplate =
{{v8_class}}::wrapperTypeInfo.domTemplate(scriptState->isolate(),
scriptState->world());
I'm very sorry, I should have recommended the following way.

V8PerIsolateData* perIsolateData =
V8PerIsolateData::from(scriptState->isolate());
v8::Local<v8::FunctionTemplate> interfaceTemplate =
perIsolateData->findInterfaceTemplate(scriptState->world(),
&{{v8_class}}::wrapperTypeInfo);

This should be more efficient.

https://codereview.chromium.org/2057273002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/testing/WorkerInternals.idl (right):

https://codereview.chromium.org/2057273002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/testing/WorkerInternals.idl:6:
DoNotCheckConstants
Why do you need this extended attribute?  This disables tests about constant
values.

[iclelland, 2016-06-16 22:55:37]

https://codereview.chromium.org/2057273002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp (right):

https://codereview.chromium.org/2057273002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp:204:
v8::Local<v8::Function> interface = perContextData->constructorForType(type);
On 2016/06/16 08:01:08, Yuki wrote:
> I'd prefer that the caller passes |interface| to installConstant() as an
> argument, because a) it's consistent with other installXxx family, and b) if
> you're going to install multiple constants, you can reduce the number of calls
> to isolate->GetCurrentContext(), that is not cheap.

Done. I've made the interface consistent, and reduced the number of calls in
V8DOMConfiguration.cpp as well.

https://codereview.chromium.org/2057273002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp:207:
v8CallOrCrash(interface->DefineOwnProperty(isolate->GetCurrentContext(), name,
value));
On 2016/06/16 08:01:08, Yuki wrote:
> You need to specify the fourth argument |attributes| of DefineOwnProperty to
> make the property v8::ReadOnly and v8::DontDelete (i.e. [[Writable]]: false,
> [[Configurable]]: false).

Done.

https://codereview.chromium.org/2057273002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/templates/interface_base.cpp (right):

https://codereview.chromium.org/2057273002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface_base.cpp:382:
v8::Local<v8::FunctionTemplate> interfaceTemplate =
{{v8_class}}::wrapperTypeInfo.domTemplate(scriptState->isolate(),
scriptState->world());
On 2016/06/16 08:01:08, Yuki wrote:
> I'm very sorry, I should have recommended the following way.
> 
> V8PerIsolateData* perIsolateData =
> V8PerIsolateData::from(scriptState->isolate());
> v8::Local<v8::FunctionTemplate> interfaceTemplate =
> perIsolateData->findInterfaceTemplate(scriptState->world(),
> &{{v8_class}}::wrapperTypeInfo);
> 
> This should be more efficient.

I'm happy to switch it to this way. Thanks, Yuki! Done.

https://codereview.chromium.org/2057273002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/testing/WorkerInternals.idl (right):

https://codereview.chromium.org/2057273002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/testing/WorkerInternals.idl:6:
DoNotCheckConstants
On 2016/06/16 08:01:08, Yuki wrote:
> Why do you need this extended attribute?  This disables tests about constant
> values.

I don't, now that I discovered the [Reflect] IDL attribute. The trouble was that
the constant I was adding would fail blink style checks, as it is all uppercase.
DoNotCheckConstants is already  present on the Internals interface, so I felt
that adding it here was also justified.

[Yuki, 2016-06-17 05:37:47]

lgtm

https://codereview.chromium.org/2057273002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/templates/interface_base.cpp (right):

https://codereview.chromium.org/2057273002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface_base.cpp:386:
v8::Local<v8::Signature> signature = v8::Signature::New(scriptState->isolate(),
interfaceTemplate);
You don't need to create |signature| every time in for-loop.
Let's move this out of the for-loop.

[iclelland, 2016-06-17 06:08:49]

https://codereview.chromium.org/2057273002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/templates/interface_base.cpp (right):

https://codereview.chromium.org/2057273002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface_base.cpp:386:
v8::Local<v8::Signature> signature = v8::Signature::New(scriptState->isolate(),
interfaceTemplate);
On 2016/06/17 05:37:47, Yuki wrote:
> You don't need to create |signature| every time in for-loop.
> Let's move this out of the for-loop.

Done.

[iclelland, 2016-06-17 06:08:59]

The CQ bit was checked by iclelland@chromium.org

[iclelland, 2016-06-17 06:09:00]

The patchset sent to the CQ was uploaded after l-g-t-m from chasej@chromium.org,
haraken@chromium.org, yukishiino@chromium.org
Link to the patchset: https://codereview.chromium.org/2057273002/#ps100001
(title: "Move signature creation out of for loop")

[commit-bot: I haz the power, 2016-06-17 06:09:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2057273002/100001

[Yuki, 2016-06-17 06:10:26]

LGTM.  Thank you for all your work and cooperations.

[commit-bot: I haz the power, 2016-06-17 07:44:11]

Description was changed from

==========
[OriginTrials] Support OriginTrialEnabled IDL attribute on constants

BUG=619092
==========

to

==========
[OriginTrials] Support OriginTrialEnabled IDL attribute on constants

BUG=619092
==========

[commit-bot: I haz the power, 2016-06-17 07:44:12]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-06-17 07:45:26]

Description was changed from

==========
[OriginTrials] Support OriginTrialEnabled IDL attribute on constants

BUG=619092
==========

to

==========
[OriginTrials] Support OriginTrialEnabled IDL attribute on constants

BUG=619092

Committed: https://crrev.com/9500e40f0af7830f1024693b715d5abb6a7e37d4
Cr-Commit-Position: refs/heads/master@{#400390}
==========

[commit-bot: I haz the power, 2016-06-17 07:45:28]

Patchset 5 (id:??) landed as
https://crrev.com/9500e40f0af7830f1024693b715d5abb6a7e37d4
Cr-Commit-Position: refs/heads/master@{#400390}