Lazily install origin trial features on V8 objects

Lazily install origin trial features on V8 objects.

This patch speeds up the creation of new execution contexts, by not requiring V8 objects to be already instantiated in order to attach origin-trial-controlled attributes. Instead, any attributes which need to be bound are attached to the interface and prototype when the constructor for the object is first accessed.

BUG=637148
Committed: https://crrev.com/b4791c071033d4cd85242cc531febf121ef196ea
Cr-Commit-Position: refs/heads/master@{#421166}

[iclelland, 2016-08-19 04:51:01]

The CQ bit was checked by iclelland@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-19 04:51:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-19 08:45:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-19 08:45:04]

Dry run: This issue passed the CQ dry run.

[iclelland, 2016-08-19 17:19:15]

The CQ bit was checked by iclelland@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-19 17:19:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[iclelland, 2016-08-19 17:25:02]

Description was changed from

==========
Lazily install orign trial features to navigator object

BUG=327148
==========

to

==========
Lazily install origin trial features on Navigator object.

This patch speeds up the creation of new execution contexts, by not requiring
any V8 objects other than the global object to be created in order to attach
origin-trial-controlled attributes. Instead, any attributes which need to be
bound are attached to the interface and prototype when the constructor for the
object is first accessed. The global objects (and any other objects which
require an actual instance to be present in order to apply origin trials) are
not subjected to this treatment.

BUG=327148
==========

[chasej, 2016-08-19 20:14:01]

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

[chasej, 2016-08-19 20:14:02]

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

https://codereview.chromium.org/2260113002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp:20: if
(RuntimeEnabledFeatures::durableStorageEnabled() || (originTrialContext &&
originTrialContext->isFeatureEnabled("ForeignFetch"))) {
Based on removed code, this probably shouldn't be durableStorageEnabled()?
Should this be linkServiceWorkerEnabled()?

[commit-bot: I haz the power, 2016-08-19 21:59:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-19 21:59:06]

Dry run: This issue passed the CQ dry run.

[haraken, 2016-08-22 02:05:01]

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

[haraken, 2016-08-22 02:05:02]

https://codereview.chromium.org/2260113002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp
(left):

https://codereview.chromium.org/2260113002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp:568:
V8NavigatorPartial::installWebBluetooth(scriptState);

It's a bit unfortunate that we need to have different paths for global objects
and other objects (which is adding complexity to the binding system)...

Would you help me understand why you cannot use the lazy-installation logic for
global objects as well? I think global objects are calling
constructorForTypeSlowCase as well.

[iclelland, 2016-08-22 12:56:56]

On 2016/08/22 02:05:02, haraken wrote:
>
https://codereview.chromium.org/2260113002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp
> (left):
> 
>
https://codereview.chromium.org/2260113002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp:568:
> V8NavigatorPartial::installWebBluetooth(scriptState);
> 
> It's a bit unfortunate that we need to have different paths for global objects
> and other objects (which is adding complexity to the binding system)...
> 
> Would you help me understand why you cannot use the lazy-installation logic
for
> global objects as well? I think global objects are calling
> constructorForTypeSlowCase as well.

I ran some experiments this weekend, and it actually turns out that I can --
even though the globals do instantiate some objects OnInstance, there is already
an instance even before the constructor is added to the map.

The only issue I had at that point was for testing; it does not appear to be
easily possible to apply the same logic to the Internals object (it is eagerly
created and injected into the JS context before we parse the document). I might
need to move all of the origin-trial test interfaces to a new interface for
testing, or change the structure of the layout tests to instantiate
window.internals / self.internals lazily as well.

[haraken, 2016-08-22 14:06:54]

On 2016/08/22 12:56:56, iclelland wrote:
> On 2016/08/22 02:05:02, haraken wrote:
> >
>
https://codereview.chromium.org/2260113002/diff/20001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp
> > (left):
> > 
> >
>
https://codereview.chromium.org/2260113002/diff/20001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp:568:
> > V8NavigatorPartial::installWebBluetooth(scriptState);
> > 
> > It's a bit unfortunate that we need to have different paths for global
objects
> > and other objects (which is adding complexity to the binding system)...
> > 
> > Would you help me understand why you cannot use the lazy-installation logic
> for
> > global objects as well? I think global objects are calling
> > constructorForTypeSlowCase as well.
> 
> I ran some experiments this weekend, and it actually turns out that I can --
> even though the globals do instantiate some objects OnInstance, there is
already
> an instance even before the constructor is added to the map.

If you can do that, that will be the best :)

> The only issue I had at that point was for testing; it does not appear to be
> easily possible to apply the same logic to the Internals object (it is eagerly
> created and injected into the JS context before we parse the document). I
might
> need to move all of the origin-trial test interfaces to a new interface for
> testing, or change the structure of the layout tests to instantiate
> window.internals / self.internals lazily as well.

I don't have any strong opinion on how the tests are written, but creating
window.internals.originTrialTest would makes sense. We already have
window.internals.unionTypesTest.

[iclelland, 2016-08-23 15:05:46]

The CQ bit was checked by iclelland@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-23 15:06:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-23 16:43:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-23 16:43:14]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[iclelland, 2016-08-24 19:15:40]

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

https://codereview.chromium.org/2260113002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp:20: if
(RuntimeEnabledFeatures::durableStorageEnabled() || (originTrialContext &&
originTrialContext->isFeatureEnabled("ForeignFetch"))) {
On 2016/08/19 20:14:02, chasej wrote:
> Based on removed code, this probably shouldn't be durableStorageEnabled()?
> Should this be linkServiceWorkerEnabled()?

Cut / Paste error; thanks for spotting it. Done.

[haraken, 2016-08-25 01:17:13]

Mostly looks good.

https://codereview.chromium.org/2260113002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp (right):

https://codereview.chromium.org/2260113002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp:15: void
installConditionalFeaturesCore(const WrapperTypeInfo* type,
v8::Local<v8::Context> context, v8::Isolate* isolate, const DOMWrapperWorld&
world, v8::Local<v8::Object> prototypeObject, v8::Local<v8::Function>
interfaceObject)

Add a TODO and mention that the function should be auto-generated.

https://codereview.chromium.org/2260113002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp:26: namespace
{

Add an empty line below.

https://codereview.chromium.org/2260113002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.h (right):

https://codereview.chromium.org/2260113002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.h:20: //
installed, but may be called multiple times, as trial tokens are

Update this comment. Now the origin-trials are installed when a wrapper is
instantiated.

https://codereview.chromium.org/2260113002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.h:23: CORE_EXPORT
void installOriginTrials(ScriptState*);

You sometimes use "ConditionalFeatures" but sometimes use "OriginTrials". Maybe
do you want to unify the terms somehow?

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

https://codereview.chromium.org/2260113002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/modules/v8/ConditionalFeaturesForModules.cpp:27:
void installConditionalFeaturesForModules(const WrapperTypeInfo* type,
v8::Local<v8::Context> context, v8::Isolate* isolate, const DOMWrapperWorld&
world, v8::Local<v8::Object> prototypeObject, v8::Local<v8::Function>
interfaceObject)

Add a TODO and mention that the function should be auto-generated.

https://codereview.chromium.org/2260113002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/modules/v8/ConditionalFeaturesForModules.cpp:27:
void installConditionalFeaturesForModules(const WrapperTypeInfo* type,
v8::Local<v8::Context> context, v8::Isolate* isolate, const DOMWrapperWorld&
world, v8::Local<v8::Object> prototypeObject, v8::Local<v8::Function>
interfaceObject)

type => wrapperTypeInfo

https://codereview.chromium.org/2260113002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/modules/v8/ConditionalFeaturesForModules.cpp:71:


Remove the empty line.

https://codereview.chromium.org/2260113002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/modules/v8/ConditionalFeaturesForModules.cpp:78:
} // namespace blink

Add an empty line.

https://codereview.chromium.org/2260113002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp (left):

https://codereview.chromium.org/2260113002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:159:
initializePendingFeatures();

Would you help me understand why we can remove this? What happens if tokens are
added after a wrapper is instantiated?

[iclelland, 2016-08-25 04:06:11]

https://codereview.chromium.org/2260113002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp (left):

https://codereview.chromium.org/2260113002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:159:
initializePendingFeatures();
On 2016/08/25 01:17:13, haraken wrote:
> 
> Would you help me understand why we can remove this? What happens if tokens
are
> added after a wrapper is instantiated?

My reasoning is this: The change in this CL avoids adding any features until the
point where the object (window, navigator, document, etc) is accessed for the
first time, at which point all features applying to that object are added at
once. Now that *all* features are added in this way, even to window, this call
isn't needed.

If tokens are added to the document after we have already started executing
scripts, then they might not be added (the constructor for the relevant objects
may already be cached, in V8PerContextData::m_construtorMap)

It's really too late at that point to be interpreting tokens anyway, I think --
in all of the origin trials documentation, we say to put the tokens early in the
document, before any <script> or <link> tags.

(The secondary reason was that with the last patch, all of the code had been
removed from that method -- or rather, from the
V8Binding::installOriginTrialsCore and
V8BindingForModules::installOriginTrialsForModules methods -- so it had nothing
left to do.)

[haraken, 2016-08-25 04:54:06]

On 2016/08/25 04:06:11, iclelland wrote:
>
https://codereview.chromium.org/2260113002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp
(left):
> 
>
https://codereview.chromium.org/2260113002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:159:
> initializePendingFeatures();
> On 2016/08/25 01:17:13, haraken wrote:
> > 
> > Would you help me understand why we can remove this? What happens if tokens
> are
> > added after a wrapper is instantiated?
> 
> My reasoning is this: The change in this CL avoids adding any features until
the
> point where the object (window, navigator, document, etc) is accessed for the
> first time, at which point all features applying to that object are added at
> once. Now that *all* features are added in this way, even to window, this call
> isn't needed.
> 
> If tokens are added to the document after we have already started executing
> scripts, then they might not be added (the constructor for the relevant
objects
> may already be cached, in V8PerContextData::m_construtorMap)
> 
> It's really too late at that point to be interpreting tokens anyway, I think
--
> in all of the origin trials documentation, we say to put the tokens early in
the
> document, before any <script> or <link> tags.

If that is your intention, removing initializePendingFeatures() totally makes
sense :)

> 
> (The secondary reason was that with the last patch, all of the code had been
> removed from that method -- or rather, from the
> V8Binding::installOriginTrialsCore and
> V8BindingForModules::installOriginTrialsForModules methods -- so it had
nothing
> left to do.)

[iclelland, 2016-09-16 19:57:22]

The CQ bit was checked by iclelland@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-16 19:57:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

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

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-16 20:17:11]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[iclelland, 2016-09-16 20:39:22]

The CQ bit was checked by iclelland@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-16 20:39:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-16 22:23:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-16 22:23:41]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[iclelland, 2016-09-20 19:25:16]

The CQ bit was checked by iclelland@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-20 19:26:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-20 20:25:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-20 20:25:30]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[iclelland, 2016-09-22 15:54:51]

The CQ bit was checked by iclelland@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-22 15:55:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-22 15:57:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-22 15:57:30]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

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

The CQ bit was checked by iclelland@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-22 16:05:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-22 16:56:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-22 16:56:16]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[iclelland, 2016-09-23 16:36:20]

The CQ bit was checked by iclelland@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-23 16:36:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-23 16:39:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-23 16:39:29]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[iclelland, 2016-09-23 16:52:38]

The CQ bit was checked by iclelland@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-23 16:53:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-23 18:37:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-23 18:37:32]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[iclelland, 2016-09-23 19:42:25]

The CQ bit was checked by iclelland@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-23 19:43:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[iclelland, 2016-09-23 19:45:24]

https://codereview.chromium.org/2260113002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp (right):

https://codereview.chromium.org/2260113002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp:15: void
installConditionalFeaturesCore(const WrapperTypeInfo* type,
v8::Local<v8::Context> context, v8::Isolate* isolate, const DOMWrapperWorld&
world, v8::Local<v8::Object> prototypeObject, v8::Local<v8::Function>
interfaceObject)
On 2016/08/25 01:17:13, haraken wrote:
> 
> Add a TODO and mention that the function should be auto-generated.

Done.

https://codereview.chromium.org/2260113002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp:26: namespace
{
On 2016/08/25 01:17:13, haraken wrote:
> 
> Add an empty line below.

Done.

https://codereview.chromium.org/2260113002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.h (right):

https://codereview.chromium.org/2260113002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.h:20: //
installed, but may be called multiple times, as trial tokens are
On 2016/08/25 01:17:13, haraken wrote:
> 
> Update this comment. Now the origin-trials are installed when a wrapper is
> instantiated.

Done. (And moved to the correct function, installConditionalFeatures)

https://codereview.chromium.org/2260113002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.h:23: CORE_EXPORT
void installOriginTrials(ScriptState*);
On 2016/08/25 01:17:13, haraken wrote:
> 
> You sometimes use "ConditionalFeatures" but sometimes use "OriginTrials".
Maybe
> do you want to unify the terms somehow?
> 

Thanks; this line should actually have been removed. I'm trying to converge on
ConditionalFeatures everywhere here, to refer to any features that should be
added or not depending on context. This will include feature policy features as
well. I'll rename all references to OriginTrials except where I specifically
mean that framework.

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

https://codereview.chromium.org/2260113002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/modules/v8/ConditionalFeaturesForModules.cpp:27:
void installConditionalFeaturesForModules(const WrapperTypeInfo* type,
v8::Local<v8::Context> context, v8::Isolate* isolate, const DOMWrapperWorld&
world, v8::Local<v8::Object> prototypeObject, v8::Local<v8::Function>
interfaceObject)
On 2016/08/25 01:17:13, haraken wrote:
> 
> Add a TODO and mention that the function should be auto-generated.

Done.

https://codereview.chromium.org/2260113002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/modules/v8/ConditionalFeaturesForModules.cpp:27:
void installConditionalFeaturesForModules(const WrapperTypeInfo* type,
v8::Local<v8::Context> context, v8::Isolate* isolate, const DOMWrapperWorld&
world, v8::Local<v8::Object> prototypeObject, v8::Local<v8::Function>
interfaceObject)
On 2016/08/25 01:17:13, haraken wrote:
> 
> type => wrapperTypeInfo

Done. (Also in core/v8/ConditionalFeatures.cpp)

https://codereview.chromium.org/2260113002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/modules/v8/ConditionalFeaturesForModules.cpp:71:

On 2016/08/25 01:17:13, haraken wrote:
> 
> Remove the empty line.

Done.

https://codereview.chromium.org/2260113002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/modules/v8/ConditionalFeaturesForModules.cpp:78:
} // namespace blink
On 2016/08/25 01:17:13, haraken wrote:
> 
> Add an empty line.

Done.

[iclelland, 2016-09-23 20:34:37]

chasej, haraken, can you PTAL again?

I discovered this week that the global objects are *not* being added to the
constructor map automatically. They are presently not being added to that map
until the constructor is actually requested. ("Window", for instance, in the JS
console). I inserted an explicit call to add them so that the trial features
would be added to globals.

[commit-bot: I haz the power, 2016-09-23 21:20:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-23 21:20:53]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[chasej, 2016-09-23 21:22:41]

lgtm

[haraken, 2016-09-25 23:55:27]

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

https://codereview.chromium.org/2260113002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.h:20: CORE_EXPORT
void installConditionalFeatures(const WrapperTypeInfo*, v8::Local<v8::Context>,
v8::Isolate*, const DOMWrapperWorld&, v8::Local<v8::Object>,
v8::Local<v8::Function>);

Why did you change the parameter from ScriptState to DOMWrapperWorld.

Also the comment "a specific context and world" sounds strange to me because a
context (which is 1:1 with ScriptState) is a concept to refer to a pair of
(world, frame).

https://codereview.chromium.org/2260113002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp (right):

https://codereview.chromium.org/2260113002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp:265:
V8PerContextData::from(context)->constructorForType(&V8Window::wrapperTypeInfo);

Why do you not need to make this change for the !isMainWorld case?

[iclelland, 2016-09-26 03:30:15]

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

https://codereview.chromium.org/2260113002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.h:20: CORE_EXPORT
void installConditionalFeatures(const WrapperTypeInfo*, v8::Local<v8::Context>,
v8::Isolate*, const DOMWrapperWorld&, v8::Local<v8::Object>,
v8::Local<v8::Function>);
On 2016/09/25 23:55:27, haraken wrote:
> 
> Why did you change the parameter from ScriptState to DOMWrapperWorld.

I changed this when I changed the call site -- this is called from
V8PerContextData::constructorForTypeSlowCase, which doesn't use ScriptState;
it's easy enough to change it, though.

> Also the comment "a specific context and world" sounds strange to me because a
> context (which is 1:1 with ScriptState) is a concept to refer to a pair of
> (world, frame).

I'll see if I can re-word that to be clearer, thanks.

https://codereview.chromium.org/2260113002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp (right):

https://codereview.chromium.org/2260113002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp:265:
V8PerContextData::from(context)->constructorForType(&V8Window::wrapperTypeInfo);
On 2016/09/25 23:55:27, haraken wrote:
> 
> Why do you not need to make this change for the !isMainWorld case?

The original decision behind this (from way back in crrev.com/a9b70a69) was that
OriginTrials were not desired in extensions, and so there was never a need to
install them there.

This may have to change if we use the same mechanism for FeaturePolicy, so that
may have to be revisited, but here I was just applying the same logic as in the
code that was removed below.

[haraken, 2016-09-26 06:09:24]

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

https://codereview.chromium.org/2260113002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.h:20: CORE_EXPORT
void installConditionalFeatures(const WrapperTypeInfo*, v8::Local<v8::Context>,
v8::Isolate*, const DOMWrapperWorld&, v8::Local<v8::Object>,
v8::Local<v8::Function>);
On 2016/09/26 03:30:14, iclelland wrote:
> On 2016/09/25 23:55:27, haraken wrote:
> > 
> > Why did you change the parameter from ScriptState to DOMWrapperWorld.
> 
> I changed this when I changed the call site -- this is called from
> V8PerContextData::constructorForTypeSlowCase, which doesn't use ScriptState;
> it's easy enough to change it, though.
> 
> > Also the comment "a specific context and world" sounds strange to me because
a
> > context (which is 1:1 with ScriptState) is a concept to refer to a pair of
> > (world, frame).
> 
> I'll see if I can re-word that to be clearer, thanks.

Basically we want to unify all information about script execution to
ScriptState. So I'd prefer passing ScriptState here :)

https://codereview.chromium.org/2260113002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp (right):

https://codereview.chromium.org/2260113002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp:265:
V8PerContextData::from(context)->constructorForType(&V8Window::wrapperTypeInfo);
On 2016/09/26 03:30:14, iclelland wrote:
> On 2016/09/25 23:55:27, haraken wrote:
> > 
> > Why do you not need to make this change for the !isMainWorld case?
> 
> The original decision behind this (from way back in crrev.com/a9b70a69) was
that
> OriginTrials were not desired in extensions, and so there was never a need to
> install them there.
> 
> This may have to change if we use the same mechanism for FeaturePolicy, so
that
> may have to be revisited, but here I was just applying the same logic as in
the
> code that was removed below.

Makes sense.

That being said, in general, I'd prefer making the constructor mapping
consistent among all worlds. Let me make this change for all worlds in a
separate CL.

[iclelland, 2016-09-26 13:49:48]

The CQ bit was checked by iclelland@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-26 13:50:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[iclelland, 2016-09-26 13:51:20]

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

https://codereview.chromium.org/2260113002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.h:20: CORE_EXPORT
void installConditionalFeatures(const WrapperTypeInfo*, v8::Local<v8::Context>,
v8::Isolate*, const DOMWrapperWorld&, v8::Local<v8::Object>,
v8::Local<v8::Function>);
On 2016/09/26 06:09:24, haraken wrote:
> On 2016/09/26 03:30:14, iclelland wrote:
> > On 2016/09/25 23:55:27, haraken wrote:
> > > 
> > > Why did you change the parameter from ScriptState to DOMWrapperWorld.
> > 
> > I changed this when I changed the call site -- this is called from
> > V8PerContextData::constructorForTypeSlowCase, which doesn't use ScriptState;
> > it's easy enough to change it, though.
> > 
> > > Also the comment "a specific context and world" sounds strange to me
because
> a
> > > context (which is 1:1 with ScriptState) is a concept to refer to a pair of
> > > (world, frame).
> > 
> > I'll see if I can re-word that to be clearer, thanks.
> 
> Basically we want to unify all information about script execution to
> ScriptState. So I'd prefer passing ScriptState here :)
> 

Done.

https://codereview.chromium.org/2260113002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp (right):

https://codereview.chromium.org/2260113002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp:265:
V8PerContextData::from(context)->constructorForType(&V8Window::wrapperTypeInfo);
On 2016/09/26 06:09:24, haraken wrote:
> On 2016/09/26 03:30:14, iclelland wrote:
> > On 2016/09/25 23:55:27, haraken wrote:
> > > 
> > > Why do you not need to make this change for the !isMainWorld case?
> > 
> > The original decision behind this (from way back in crrev.com/a9b70a69) was
> that
> > OriginTrials were not desired in extensions, and so there was never a need
to
> > install them there.
> > 
> > This may have to change if we use the same mechanism for FeaturePolicy, so
> that
> > may have to be revisited, but here I was just applying the same logic as in
> the
> > code that was removed below.
> 
> Makes sense.
> 
> That being said, in general, I'd prefer making the constructor mapping
> consistent among all worlds. Let me make this change for all worlds in a
> separate CL.

SGTM! I've rebased this patch on top of yours now.

[iclelland, 2016-09-26 13:54:07]

The CQ bit was checked by iclelland@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-26 13:54:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[haraken, 2016-09-26 14:44:57]

On 2016/09/26 13:51:20, iclelland wrote:
>
https://codereview.chromium.org/2260113002/diff/240001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.h (right):
> 
>
https://codereview.chromium.org/2260113002/diff/240001/third_party/WebKit/Sou...
> third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.h:20:
CORE_EXPORT
> void installConditionalFeatures(const WrapperTypeInfo*,
v8::Local<v8::Context>,
> v8::Isolate*, const DOMWrapperWorld&, v8::Local<v8::Object>,
> v8::Local<v8::Function>);
> On 2016/09/26 06:09:24, haraken wrote:
> > On 2016/09/26 03:30:14, iclelland wrote:
> > > On 2016/09/25 23:55:27, haraken wrote:
> > > > 
> > > > Why did you change the parameter from ScriptState to DOMWrapperWorld.
> > > 
> > > I changed this when I changed the call site -- this is called from
> > > V8PerContextData::constructorForTypeSlowCase, which doesn't use
ScriptState;
> > > it's easy enough to change it, though.
> > > 
> > > > Also the comment "a specific context and world" sounds strange to me
> because
> > a
> > > > context (which is 1:1 with ScriptState) is a concept to refer to a pair
of
> > > > (world, frame).
> > > 
> > > I'll see if I can re-word that to be clearer, thanks.
> > 
> > Basically we want to unify all information about script execution to
> > ScriptState. So I'd prefer passing ScriptState here :)
> > 
> 
> Done.
> 
>
https://codereview.chromium.org/2260113002/diff/240001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp (right):
> 
>
https://codereview.chromium.org/2260113002/diff/240001/third_party/WebKit/Sou...
> third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp:265:
>
V8PerContextData::from(context)->constructorForType(&V8Window::wrapperTypeInfo);
> On 2016/09/26 06:09:24, haraken wrote:
> > On 2016/09/26 03:30:14, iclelland wrote:
> > > On 2016/09/25 23:55:27, haraken wrote:
> > > > 
> > > > Why do you not need to make this change for the !isMainWorld case?
> > > 
> > > The original decision behind this (from way back in crrev.com/a9b70a69)
was
> > that
> > > OriginTrials were not desired in extensions, and so there was never a need
> to
> > > install them there.
> > > 
> > > This may have to change if we use the same mechanism for FeaturePolicy, so
> > that
> > > may have to be revisited, but here I was just applying the same logic as
in
> > the
> > > code that was removed below.
> > 
> > Makes sense.
> > 
> > That being said, in general, I'd prefer making the constructor mapping
> > consistent among all worlds. Let me make this change for all worlds in a
> > separate CL.
> 
> SGTM! I've rebased this patch on top of yours now.

Thanks, LGTM

[commit-bot: I haz the power, 2016-09-26 14:56:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-26 14:56:56]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[iclelland, 2016-09-27 03:42:30]

Description was changed from

==========
Lazily install origin trial features on Navigator object.

This patch speeds up the creation of new execution contexts, by not requiring
any V8 objects other than the global object to be created in order to attach
origin-trial-controlled attributes. Instead, any attributes which need to be
bound are attached to the interface and prototype when the constructor for the
object is first accessed. The global objects (and any other objects which
require an actual instance to be present in order to apply origin trials) are
not subjected to this treatment.

BUG=327148
==========

to

==========
Lazily install origin trial features on V8 objects.

This patch speeds up the creation of new execution contexts, by not requiring V8
objects to be already instantiated in order to attach origin-trial-controlled
attributes. Instead, any attributes which need to be bound are attached to the
interface and prototype when the constructor for the object is first accessed.

BUG=327148
==========

[iclelland, 2016-09-27 03:42:44]

The CQ bit was checked by iclelland@chromium.org

[iclelland, 2016-09-27 03:42:44]

The patchset sent to the CQ was uploaded after l-g-t-m from chasej@chromium.org
Link to the patchset: https://codereview.chromium.org/2260113002/#ps280001
(title: "Actually rebase onto 2372473002")

[commit-bot: I haz the power, 2016-09-27 03:43:00]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-27 03:47:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-27 03:47:14]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[iclelland, 2016-09-27 04:25:56]

The CQ bit was checked by iclelland@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-27 04:26:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-27 06:34:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-27 06:34:51]

Dry run: This issue passed the CQ dry run.

[iclelland, 2016-09-27 10:56:01]

The CQ bit was checked by iclelland@chromium.org

[iclelland, 2016-09-27 10:56:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, chasej@chromium.org
Link to the patchset: https://codereview.chromium.org/2260113002/#ps300001
(title: "Rebase after parent CL landed")

[commit-bot: I haz the power, 2016-09-27 10:56:22]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-27 11:02:51]

Description was changed from

==========
Lazily install origin trial features on V8 objects.

This patch speeds up the creation of new execution contexts, by not requiring V8
objects to be already instantiated in order to attach origin-trial-controlled
attributes. Instead, any attributes which need to be bound are attached to the
interface and prototype when the constructor for the object is first accessed.

BUG=327148
==========

to

==========
Lazily install origin trial features on V8 objects.

This patch speeds up the creation of new execution contexts, by not requiring V8
objects to be already instantiated in order to attach origin-trial-controlled
attributes. Instead, any attributes which need to be bound are attached to the
interface and prototype when the constructor for the object is first accessed.

BUG=327148
==========

[commit-bot: I haz the power, 2016-09-27 11:02:54]

Committed patchset #16 (id:300001)

[commit-bot: I haz the power, 2016-09-27 11:05:06]

Description was changed from

==========
Lazily install origin trial features on V8 objects.

This patch speeds up the creation of new execution contexts, by not requiring V8
objects to be already instantiated in order to attach origin-trial-controlled
attributes. Instead, any attributes which need to be bound are attached to the
interface and prototype when the constructor for the object is first accessed.

BUG=327148
==========

to

==========
Lazily install origin trial features on V8 objects.

This patch speeds up the creation of new execution contexts, by not requiring V8
objects to be already instantiated in order to attach origin-trial-controlled
attributes. Instead, any attributes which need to be bound are attached to the
interface and prototype when the constructor for the object is first accessed.

BUG=327148

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

[commit-bot: I haz the power, 2016-09-27 11:05:08]

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

[chasej, 2017-01-11 20:48:34]

Description was changed from

==========
Lazily install origin trial features on V8 objects.

This patch speeds up the creation of new execution contexts, by not requiring V8
objects to be already instantiated in order to attach origin-trial-controlled
attributes. Instead, any attributes which need to be bound are attached to the
interface and prototype when the constructor for the object is first accessed.

BUG=327148

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

to

==========
Lazily install origin trial features on V8 objects.

This patch speeds up the creation of new execution contexts, by not requiring V8
objects to be already instantiated in order to attach origin-trial-controlled
attributes. Instead, any attributes which need to be bound are attached to the
interface and prototype when the constructor for the object is first accessed.

BUG=637148

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