Allow origin trials to be enabled by script

Allow origin trials to be enabled by script

Design doc:
https://docs.google.com/a/chromium.org/document/d/1oGM6VErHxxvA4JsjstNa6NizC4S5d6f-VrcjHMVYJyE/edit?usp=sharing

BUG=667301
Review-Url: https://codereview.chromium.org/2640823004
Cr-Commit-Position: refs/heads/master@{#447471}
Committed: https://chromium.googlesource.com/chromium/src/+/8206eacd2be4650deb6d9c706533ee6473d6dd3d

[chasej, 2017-01-18 22:15:00]

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

[commit-bot: I haz the power, 2017-01-18 22:15:40]

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, 2017-01-19 00:11:02]

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

[commit-bot: I haz the power, 2017-01-19 00:11:03]

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

[chasej, 2017-01-19 22:53:06]

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

[commit-bot: I haz the power, 2017-01-19 22:53:58]

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, 2017-01-20 00:40:39]

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

[commit-bot: I haz the power, 2017-01-20 00:40:40]

Dry run: This issue passed the CQ dry run.

[chasej, 2017-01-20 04:42:44]

Description was changed from

==========
Allow origin trials to be enabled by script

BUG=667301
==========

to

==========
Allow origin trials to be enabled by script

Design doc:
https://docs.google.com/a/chromium.org/document/d/1oGM6VErHxxvA4JsjstNa6NizC4...

BUG=667301
==========

[chasej, 2017-01-20 04:42:44]

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

[chasej, 2017-01-20 04:46:46]

iclelland, please take a look.

This is still a WIP, but I'd appreciate a review of the overall approach.

Outstanding:
- Probably going to reinstate the special handling for Window. That is, using
installPendingConditionalFeaturesOnWindow
- Cleaner approach to allow overrides, instead of the three static function
pointers
- Handling for all the affected types/trials. So far, tested WebUSB,
LongTaskObserver, and ServiceWorkerNavigationPreload

[iclelland, 2017-01-20 14:31:19]

I think I need to take a closer look at some of the logic, but first pass
comments are below.

https://codereview.chromium.org/2640823004/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/origin_trials/sample-api-script-added-before-access.html
(right):

https://codereview.chromium.org/2640823004/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/origin_trials/sample-api-script-added-before-access.html:16:
// Do not access the target object(s) before adding the token. This is to test
The target object in this case is the value returned by
internals.OriginTrials(), right? We should call out specifically that *that* is
the object not to instantiate, just so that nobody is put off by the vaguely
worded warning.

https://codereview.chromium.org/2640823004/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/origin_trials/sample-api-script-added-before-access.html:19:
// Add the token to enable the trial
We could factor this out into origin_trials.js, maybe, as a method like
addToken(tokenString, location)

https://codereview.chromium.org/2640823004/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/origin_trials/sample-api-script-added.html
(right):

https://codereview.chromium.org/2640823004/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/origin_trials/sample-api-script-added.html:17:
//  - Skip the worker tests, as they will likely fail due to running after the
Can we chain these tests, so that the worker tests are allowed to run (and fail)
before we continue by adding the token?

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

https://codereview.chromium.org/2640823004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp:55: LOG(INFO)
<< "Just install, don't look for existing instance for type: "
Is this a debug message to be removed?

https://codereview.chromium.org/2640823004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp:57:
installConditionalFeatures(wrapperTypeInfo, scriptState,
V8Window is a global object, which is the one case where we typically *do*
generally need the instance variable, in order to expose the constructor
interfaces. It's easy to get, as scriptState->context()->Global(), and it's a
singleton, so we don't need to search for multiple instances; is it actually
unnecessary in this case?

https://codereview.chromium.org/2640823004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp:65: void
getAffectedTypesForConditionalFeatureCore(
Rather than modifying its input, could this return a static variable, created
with CR_DEFINE_STATIC_LOCAL? It would save the overhead of creating a set and
inserting into it every time this is called.

https://codereview.chromium.org/2640823004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp:146:
LOG(INFO) << "Install pending feature: " << feature;
Debug, right?

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

https://codereview.chromium.org/2640823004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.h:45: CORE_EXPORT
InstallConditionalFeaturesIfNeededFunction
One way to get rid of the three parallel function pointers might be to
encapsulate them in a struct, which could be statically initialized in each
file, and then have one "registerOriginTrialsHandler(const struct)" method to
install all three at the same time. That would make it clear that all three (or
more in the future) are required, and ensure that either all or none are called.

If we store the struct explicitly in ConditionalFeatures.cpp, then there is one
additional lookup required for each call; one of the reasons for the function
pointers was to minimize the overhead of calling these for each prototype
instantiation; it's probably a micro-optimization to insist on it, though.

That would still require cooperative chaining of function calls. If we want to
avoid that, and didn't care how long it took to run the installation functions,
we could store a vector of structs (or three parallel vectors) and have a method
that iterates through it each time. I'm not sure if that's worth the complexity,
though, especially if these methods are going to be generated anyway.

https://codereview.chromium.org/2640823004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.h:64: // or
deferred until object creation).
I'm confused about what returning false from this function means.It looks like
it would be an error, and we should be DCHECKing that it never happens, but
maybe there are other possibilities with different kinds of trials that I'm not
seeing.

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

https://codereview.chromium.org/2640823004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8PerContextData.h:88:
v8::Local<v8::Function> constructorForTypeFromCache(
I'd document the return value, mostly for the case where the constructor is not
found in the map. (I hope the other case is obvious :) )

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

https://codereview.chromium.org/2640823004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/modules/v8/ConditionalFeaturesForModules.cpp:153:
// Only install features here, if the target type has an existing instance
I think rather than instance, you mean prototype/constructor here.

https://codereview.chromium.org/2640823004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/modules/v8/ConditionalFeaturesForModules.cpp:180:
if (feature == "ServiceWorkerNavigationPreload") {
Is this method eventually intended to cover all of the trial features which are
handled in installConditionalFeaturesForModules, or is this going to be a
hand-curated list?

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

https://codereview.chromium.org/2640823004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:223: return
valid;
I'd return false explicitly here for clarity

https://codereview.chromium.org/2640823004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:229: return
valid;
Ditto

[chasej, 2017-01-25 19:58:20]

iclelland, please take another look.

I've significantly changed the approach in ConditionalFeatures*.
From your comments, I realized I could simplify things by 
assuming the methods would be code-generated in the future.

The result is two families of methods. The first family (existing)
of methods handles installing features on a given type. The second
family (new) of methods handles installing a given feature on all
affected types. Each family of methods can eventually be code
generated.

I also added a bunch of layout tests. Still pending are tests for
WebShare and WebVR.

https://codereview.chromium.org/2640823004/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/origin_trials/sample-api-script-added-before-access.html
(right):

https://codereview.chromium.org/2640823004/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/origin_trials/sample-api-script-added-before-access.html:16:
// Do not access the target object(s) before adding the token. This is to test
On 2017/01/20 14:31:19, iclelland wrote:
> The target object in this case is the value returned by
> internals.OriginTrials(), right? We should call out specifically that *that*
is
> the object not to instantiate, just so that nobody is put off by the vaguely
> worded warning.

Done.

https://codereview.chromium.org/2640823004/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/origin_trials/sample-api-script-added-before-access.html:19:
// Add the token to enable the trial
On 2017/01/20 14:31:19, iclelland wrote:
> We could factor this out into origin_trials.js, maybe, as a method like
> addToken(tokenString, location)

I factored this out into a function, but it's not clear what you mean by the
location parameter. Is that intended to be where in the document the token would
be added, e.g. addToken(token, document.head)?

https://codereview.chromium.org/2640823004/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/origin_trials/sample-api-script-added.html
(right):

https://codereview.chromium.org/2640823004/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/origin_trials/sample-api-script-added.html:17:
//  - Skip the worker tests, as they will likely fail due to running after the
On 2017/01/20 14:31:19, iclelland wrote:
> Can we chain these tests, so that the worker tests are allowed to run (and
fail)
> before we continue by adding the token?

I looked into that, but wasn't able to find a simple way to do so. I then
figured that the disabled worker tests don't provide much value here. It would
be testing that a trial was unexpectedly enabled in a worker, when it didn't
inherit tokens from the containing document.

I would think it's possible to wait on the worker tests, but it doesn't seem
worth the additional effort. Is there a reason why this is important that I'm
missing?

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

https://codereview.chromium.org/2640823004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp:55: LOG(INFO)
<< "Just install, don't look for existing instance for type: "
On 2017/01/20 14:31:19, iclelland wrote:
> Is this a debug message to be removed?

This method is gone in the new, simplified approach.

https://codereview.chromium.org/2640823004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp:57:
installConditionalFeatures(wrapperTypeInfo, scriptState,
On 2017/01/20 14:31:19, iclelland wrote:
> V8Window is a global object, which is the one case where we typically *do*
> generally need the instance variable, in order to expose the constructor
> interfaces. It's easy to get, as scriptState->context()->Global(), and it's a
> singleton, so we don't need to search for multiple instances; is it actually
> unnecessary in this case?

This method is gone in the new, simplified approach.

https://codereview.chromium.org/2640823004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp:65: void
getAffectedTypesForConditionalFeatureCore(
On 2017/01/20 14:31:19, iclelland wrote:
> Rather than modifying its input, could this return a static variable, created
> with CR_DEFINE_STATIC_LOCAL? It would save the overhead of creating a set and
> inserting into it every time this is called.

This method is gone in the new, simplified approach.

https://codereview.chromium.org/2640823004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp:146:
LOG(INFO) << "Install pending feature: " << feature;
On 2017/01/20 14:31:19, iclelland wrote:
> Debug, right?

Removed (along with this method being rewritten).

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

https://codereview.chromium.org/2640823004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.h:45: CORE_EXPORT
InstallConditionalFeaturesIfNeededFunction
On 2017/01/20 14:31:19, iclelland wrote:
> One way to get rid of the three parallel function pointers might be to
> encapsulate them in a struct, which could be statically initialized in each
> file, and then have one "registerOriginTrialsHandler(const struct)" method to
> install all three at the same time. That would make it clear that all three
(or
> more in the future) are required, and ensure that either all or none are
called.
> 
> If we store the struct explicitly in ConditionalFeatures.cpp, then there is
one
> additional lookup required for each call; one of the reasons for the function
> pointers was to minimize the overhead of calling these for each prototype
> instantiation; it's probably a micro-optimization to insist on it, though.
> 
> That would still require cooperative chaining of function calls. If we want to
> avoid that, and didn't care how long it took to run the installation
functions,
> we could store a vector of structs (or three parallel vectors) and have a
method
> that iterates through it each time. I'm not sure if that's worth the
complexity,
> though, especially if these methods are going to be generated anyway.

I've simplified the approach, so that there's only two function
pointers (existing + new one). For just two pointers, I don't see
much point in doing anything like you suggested.

https://codereview.chromium.org/2640823004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.h:64: // or
deferred until object creation).
On 2017/01/20 14:31:19, iclelland wrote:
> I'm confused about what returning false from this function means.It looks like
> it would be an error, and we should be DCHECKing that it never happens, but
> maybe there are other possibilities with different kinds of trials that I'm
not
> seeing.

You're right. Ultimately, this function should never return false. It does now
in some cases, but only because I haven't implemented the handling for all the
various trials/features/types. I'll change it to void.

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

https://codereview.chromium.org/2640823004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8PerContextData.h:88:
v8::Local<v8::Function> constructorForTypeFromCache(
On 2017/01/20 14:31:19, iclelland wrote:
> I'd document the return value, mostly for the case where the constructor is
not
> found in the map. (I hope the other case is obvious :) )

Done.

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

https://codereview.chromium.org/2640823004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/modules/v8/ConditionalFeaturesForModules.cpp:153:
// Only install features here, if the target type has an existing instance
On 2017/01/20 14:31:19, iclelland wrote:
> I think rather than instance, you mean prototype/constructor here.

This method is gone in the new, simplified approach.

https://codereview.chromium.org/2640823004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/modules/v8/ConditionalFeaturesForModules.cpp:180:
if (feature == "ServiceWorkerNavigationPreload") {
On 2017/01/20 14:31:19, iclelland wrote:
> Is this method eventually intended to cover all of the trial features which
are
> handled in installConditionalFeaturesForModules, or is this going to be a
> hand-curated list?

This method is gone in the new, simplified approach.

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

https://codereview.chromium.org/2640823004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:223: return
valid;
On 2017/01/20 14:31:19, iclelland wrote:
> I'd return false explicitly here for clarity

Done.

https://codereview.chromium.org/2640823004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:229: return
valid;
On 2017/01/20 14:31:19, iclelland wrote:
> Ditto

Done.

[chasej, 2017-01-25 22:46:39]

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

[commit-bot: I haz the power, 2017-01-25 22:48:00]

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, 2017-01-25 22:52:11]

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

[commit-bot: I haz the power, 2017-01-25 22:52:12]

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-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[chasej, 2017-01-26 15:29:23]

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

[commit-bot: I haz the power, 2017-01-26 15:29:40]

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, 2017-01-26 17:49:43]

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

[commit-bot: I haz the power, 2017-01-26 17:49:44]

Dry run: This issue passed the CQ dry run.

[iclelland, 2017-01-26 20:14:12]

I haven't reviewed all of the test changes yet, but I've left comments below on
the code.

> iclelland, please take another look.
>
> I've significantly changed the approach in ConditionalFeatures*.
> From your comments, I realized I could simplify things by 
> assuming the methods would be code-generated in the future.
>
> The result is two families of methods. The first family (existing)
> of methods handles installing features on a given type. The second
> family (new) of methods handles installing a given feature on all
> affected types. Each family of methods can eventually be code
> generated.
>
> I also added a bunch of layout tests. Still pending are tests for
> WebShare and WebVR.

I like this approach, generally, thanks for changing it. It is more apparent
than ever that we should generate this :)

https://codereview.chromium.org/2640823004/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/origin_trials/sample-api-script-added-before-access.html
(right):

https://codereview.chromium.org/2640823004/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/origin_trials/sample-api-script-added-before-access.html:19:
// Add the token to enable the trial
On 2017/01/25 19:58:20, chasej wrote:
> On 2017/01/20 14:31:19, iclelland wrote:
> > We could factor this out into origin_trials.js, maybe, as a method like
> > addToken(tokenString, location)
> 
> I factored this out into a function, but it's not clear what you mean by the
> location parameter. Is that intended to be where in the document the token
would
> be added, e.g. addToken(token, document.head)?

Yes, that's what I meant -- the DOM node to append the token meta tag to. (Then
we could also test appending to <body>, if we need to)

https://codereview.chromium.org/2640823004/diff/80001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/VirtualTestSuites (right):

https://codereview.chromium.org/2640823004/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/VirtualTestSuites:114: "args":
["--disable-blink-features=ForeignFetch,ImageCapture,LinkServiceWorker,LongTaskObserver,ServiceWorkerNavigationPreload,SetRootScroller,WebUSB,WebVR,WebShare",
This is a big list, and updating it unfortunately is going to have to be part of
the integration docs now. Do you have any idea what it would take to make this a
single command-line flag to disable all origin trial features?

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

https://codereview.chromium.org/2640823004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp:14: #include
"core/origin_trials/OriginTrialContext.h"
I think we can remove this one now

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

https://codereview.chromium.org/2640823004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.h:11: #include
"wtf/HashSet.h"
Unneeded here?

https://codereview.chromium.org/2640823004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.h:69: CORE_EXPORT
bool getExistingConstructorAndPrototypeForType(
Should this be in ConditionalFeatures, or is it more a method for
V8PerContextData?

Alternately, if that doesn't work, I'd put it in the anonymous namespace in the
.cpp file and avoid declaring it here, if it's not used outside that file.

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

https://codereview.chromium.org/2640823004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/modules/v8/ConditionalFeaturesForModules.cpp:176:
isolate, world, instanceObject, prototypeObject, interfaceObject);
Can you replace prototypeObject and interfaceObject with explicitly empty
objects here? (like in ImageCapture and WebUSB) -- I don't think there's any
real issue here, but it *looks* a lot like passing uninitialized locals, and I'd
rather be clear that these aren't just reusing values from earlier in the
method, or being used uninitialized.

https://codereview.chromium.org/2640823004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/modules/v8/ConditionalFeaturesForModules.cpp:219:
prototypeObject, interfaceObject);
ditto here and the next line.

https://codereview.chromium.org/2640823004/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/bindings/modules/v8/ConditionalFeaturesForModules.h
(right):

https://codereview.chromium.org/2640823004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/modules/v8/ConditionalFeaturesForModules.h:18:
void getAffectedTypesForConditionalFeatureForModules(
Is this implemented anywhere?

https://codereview.chromium.org/2640823004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp
(right):

https://codereview.chromium.org/2640823004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:157: void
OriginTrialContext::addToken(const String& token) {
I wonder if we should rename this too, to something like
'installTrialFromToken'?

https://codereview.chromium.org/2640823004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/origin_trials/OriginTrialContext.h (right):

https://codereview.chromium.org/2640823004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/origin_trials/OriginTrialContext.h:80: // there
are no enabled trials, this method returns without doing anything.
Can this method be called safely multiple times, if no new trials have been
enabled? We should call that out here, if so.

https://codereview.chromium.org/2640823004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/origin_trials/OriginTrialContext.h:88: bool
validateToken(const String& token);
Maybe we should rename this to reflect its very relevant side effect --
something like "enableTrialFromToken", and document that it returns false if the
token was invalid?

https://codereview.chromium.org/2640823004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/testing/v8/WebCoreTestSupport.h (right):

https://codereview.chromium.org/2640823004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/testing/v8/WebCoreTestSupport.h:46: void
installPendingConditionalFeatureForTests(const String&,
nit: I think we should name the variable here, String is sufficiently vague.

https://codereview.chromium.org/2640823004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/testing/v8/WebCoreTestSupport.h:49: }  //
namespace WebCoreTestSupport
nit: Blank line before

[chasej, 2017-01-27 17:22:34]

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

[chasej, 2017-01-27 17:22:36]

iclelland, please take another look.

haraken, please take a look at everything for OWNER, but especially the addition
of V8PerContextData::getExistingConstructorAndPrototypeForType.

There's a lot of layout tests added, but they are mostly repetitions of one of
two patterns. That is, testing exposed interfaces with a token embedded in the
document, or testing exposed interfaces with a token added via script.

https://codereview.chromium.org/2640823004/diff/80001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/VirtualTestSuites (right):

https://codereview.chromium.org/2640823004/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/VirtualTestSuites:114: "args":
["--disable-blink-features=ForeignFetch,ImageCapture,LinkServiceWorker,LongTaskObserver,ServiceWorkerNavigationPreload,SetRootScroller,WebUSB,WebVR,WebShare",
On 2017/01/26 20:14:11, iclelland wrote:
> This is a big list, and updating it unfortunately is going to have to be part
of
> the integration docs now. Do you have any idea what it would take to make this
a
> single command-line flag to disable all origin trial features?

It should be straight-forward to add a new flag. Logged crbug.com/685788 to
track it.

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

https://codereview.chromium.org/2640823004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp:14: #include
"core/origin_trials/OriginTrialContext.h"
On 2017/01/26 20:14:11, iclelland wrote:
> I think we can remove this one now

Done.

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

https://codereview.chromium.org/2640823004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.h:11: #include
"wtf/HashSet.h"
On 2017/01/26 20:14:11, iclelland wrote:
> Unneeded here?

Yes, removed.

https://codereview.chromium.org/2640823004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.h:69: CORE_EXPORT
bool getExistingConstructorAndPrototypeForType(
On 2017/01/26 20:14:11, iclelland wrote:
> Should this be in ConditionalFeatures, or is it more a method for
> V8PerContextData?
> 
> Alternately, if that doesn't work, I'd put it in the anonymous namespace in
the
> .cpp file and avoid declaring it here, if it's not used outside that file.

Moved to V8PerContextData, as the method is also called in
ConditionalFeaturesForModules
and WebCoreTestSupport. By moving, I can remove the other method added to
V8PerContextData, constructorForTypeFromCache().

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

https://codereview.chromium.org/2640823004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/modules/v8/ConditionalFeaturesForModules.cpp:176:
isolate, world, instanceObject, prototypeObject, interfaceObject);
On 2017/01/26 20:14:11, iclelland wrote:
> Can you replace prototypeObject and interfaceObject with explicitly empty
> objects here? (like in ImageCapture and WebUSB) -- I don't think there's any
> real issue here, but it *looks* a lot like passing uninitialized locals, and
I'd
> rather be clear that these aren't just reusing values from earlier in the
> method, or being used uninitialized.

Done.

https://codereview.chromium.org/2640823004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/modules/v8/ConditionalFeaturesForModules.cpp:219:
prototypeObject, interfaceObject);
On 2017/01/26 20:14:11, iclelland wrote:
> ditto here and the next line.

Done.

https://codereview.chromium.org/2640823004/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/bindings/modules/v8/ConditionalFeaturesForModules.h
(right):

https://codereview.chromium.org/2640823004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/modules/v8/ConditionalFeaturesForModules.h:18:
void getAffectedTypesForConditionalFeatureForModules(
On 2017/01/26 20:14:11, iclelland wrote:
> Is this implemented anywhere?

Nope, I missed it in the previous cleanup. Removed.

https://codereview.chromium.org/2640823004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp
(right):

https://codereview.chromium.org/2640823004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:157: void
OriginTrialContext::addToken(const String& token) {
On 2017/01/26 20:14:11, iclelland wrote:
> I wonder if we should rename this too, to something like
> 'installTrialFromToken'?

I'm thinking we should leave it as 'addToken'. I see it as the public
interface, and the installation process is more of an internal
implementation detail (although an important side effect). In the
case of 'validateToken' -> 'enableTrialFromToken', I would argue
the enabling of trials is actually more of the main purpose of the
function.

https://codereview.chromium.org/2640823004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/origin_trials/OriginTrialContext.h (right):

https://codereview.chromium.org/2640823004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/origin_trials/OriginTrialContext.h:80: // there
are no enabled trials, this method returns without doing anything.
On 2017/01/26 20:14:12, iclelland wrote:
> Can this method be called safely multiple times, if no new trials have been
> enabled? We should call that out here, if so.

Done.

https://codereview.chromium.org/2640823004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/origin_trials/OriginTrialContext.h:88: bool
validateToken(const String& token);
On 2017/01/26 20:14:12, iclelland wrote:
> Maybe we should rename this to reflect its very relevant side effect --
> something like "enableTrialFromToken", and document that it returns false if
the
> token was invalid?

Good idea! Done.

https://codereview.chromium.org/2640823004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/testing/v8/WebCoreTestSupport.h (right):

https://codereview.chromium.org/2640823004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/testing/v8/WebCoreTestSupport.h:46: void
installPendingConditionalFeatureForTests(const String&,
On 2017/01/26 20:14:12, iclelland wrote:
> nit: I think we should name the variable here, String is sufficiently vague.

I realized the function declaration isn't needed at all in the header file.
I've made the three files consistent, in that the implementation functions
are only used in the static pointers, and not declared in the header.

https://codereview.chromium.org/2640823004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/testing/v8/WebCoreTestSupport.h:49: }  //
namespace WebCoreTestSupport
On 2017/01/26 20:14:12, iclelland wrote:
> nit: Blank line before

Done.

[chasej, 2017-01-27 17:39:21]

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

[commit-bot: I haz the power, 2017-01-27 17:39:57]

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, 2017-01-27 17:49:03]

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

[commit-bot: I haz the power, 2017-01-27 17:49:04]

Dry run: 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...)

[haraken, 2017-01-28 02:25:21]

Sorry about the review delay. Mostly looks good!

https://codereview.chromium.org/2640823004/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp (right):

https://codereview.chromium.org/2640823004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp:60: if
(feature == "ForeignFetch") {

How are the features listed in this method selected? In other words, when do
developers need to add a feature to this method?

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

https://codereview.chromium.org/2640823004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.h:54: // when the
V8 context is reused (i.e., after navigation)

Not related to this CL, would you again help me understand why we need a special
logic for window? I think you're worrying about a case where a window object is
reused when we navigate from an initial empty document to a same-origin
document, but I've forgotten why we need a special handling for that case.

https://codereview.chromium.org/2640823004/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/V8PerContextData.cpp (right):

https://codereview.chromium.org/2640823004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8PerContextData.cpp:182:
*prototypeObject = prototypeForType(type);

Shall we add DCHECK(!prototypeObject->IsEmpty())?

https://codereview.chromium.org/2640823004/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/bindings/modules/v8/ConditionalFeaturesForModules.cpp
(right):

https://codereview.chromium.org/2640823004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/modules/v8/ConditionalFeaturesForModules.cpp:157:
if (feature == "ForeignFetch") {

Just help me understand: You're adding installation code for ForeignFetch,
ImageCapture etc. How is the feature list selected? In other words, what
features need to be added here?

https://codereview.chromium.org/2640823004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/modules/v8/ConditionalFeaturesForModules.cpp:219:
v8::Local<v8::Object> instanceObject = scriptState->context()->Global();

You can factor out this statement out of the if clause. instanceObject is used
in many if clauses.

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

https://codereview.chromium.org/2640823004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/origin_trials/OriginTrialContext.h:79: // reused
(i.e., after navigation). If the V8 context is not initialized, or

I'd drop the ", or even if..." part. That's a very subtle case where we navigate
from an initial empty document to a same-origin document.

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

https://codereview.chromium.org/2640823004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/testing/v8/WebCoreTestSupport.cpp:122: void
installPendingConditionalFeatureForTests(const String& feature,

Nit: ForTests => ForTesting (due to naming convention)

https://codereview.chromium.org/2640823004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/testing/v8/WebCoreTestSupport.cpp:140: void
registerInstallConditionalFeaturesForTests() {

Ditto.

[chasej, 2017-01-30 19:15:56]

haraken, iclelland, please take another look.

https://codereview.chromium.org/2640823004/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp (right):

https://codereview.chromium.org/2640823004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp:60: if
(feature == "ForeignFetch") {
On 2017/01/28 02:25:20, haraken wrote:
> 
> How are the features listed in this method selected? In other words, when do
> developers need to add a feature to this method?

Blink developers will need to update this method upon first integrating their
feature with the origin trials framework. They will also need to update the
InstallConditionalFeatures* method. I will be making updates to the feature
author guide to make this clear, currently found at:
https://github.com/jpchase/OriginTrials/blob/gh-pages/ship-as-trial.md#how-to....

We will eventually automate this via code generation.

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

https://codereview.chromium.org/2640823004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.h:54: // when the
V8 context is reused (i.e., after navigation)
On 2017/01/28 02:25:20, haraken wrote:
> 
> Not related to this CL, would you again help me understand why we need a
special
> logic for window? I think you're worrying about a case where a window object
is
> reused when we navigate from an initial empty document to a same-origin
> document, but I've forgotten why we need a special handling for that case.

This method is called in LocalWindowProxy::initialize(). My understand is, in
that case, the trials are enabled before the context is initialized, so this
method is needed to install.

iclelland, please correct if I have that wrong.

https://codereview.chromium.org/2640823004/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/V8PerContextData.cpp (right):

https://codereview.chromium.org/2640823004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8PerContextData.cpp:182:
*prototypeObject = prototypeForType(type);
On 2017/01/28 02:25:20, haraken wrote:
> 
> Shall we add DCHECK(!prototypeObject->IsEmpty())?

Done.

https://codereview.chromium.org/2640823004/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/bindings/modules/v8/ConditionalFeaturesForModules.cpp
(right):

https://codereview.chromium.org/2640823004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/modules/v8/ConditionalFeaturesForModules.cpp:157:
if (feature == "ForeignFetch") {
On 2017/01/28 02:25:20, haraken wrote:
> 
> Just help me understand: You're adding installation code for ForeignFetch,
> ImageCapture etc. How is the feature list selected? In other words, what
> features need to be added here?
> 

See my comment above on ConditionalFeatures.cpp.

https://codereview.chromium.org/2640823004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/modules/v8/ConditionalFeaturesForModules.cpp:219:
v8::Local<v8::Object> instanceObject = scriptState->context()->Global();
On 2017/01/28 02:25:20, haraken wrote:
> 
> You can factor out this statement out of the if clause. instanceObject is used
> in many if clauses.
> 

Done.

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

https://codereview.chromium.org/2640823004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/origin_trials/OriginTrialContext.h:79: // reused
(i.e., after navigation). If the V8 context is not initialized, or
On 2017/01/28 02:25:20, haraken wrote:
> 
> I'd drop the ", or even if..." part. That's a very subtle case where we
navigate
> from an initial empty document to a same-origin document.

Done.

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

https://codereview.chromium.org/2640823004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/testing/v8/WebCoreTestSupport.cpp:122: void
installPendingConditionalFeatureForTests(const String& feature,
On 2017/01/28 02:25:20, haraken wrote:
> 
> Nit: ForTests => ForTesting (due to naming convention)

Done.

https://codereview.chromium.org/2640823004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/testing/v8/WebCoreTestSupport.cpp:140: void
registerInstallConditionalFeaturesForTests() {
On 2017/01/28 02:25:20, haraken wrote:
> 
> Ditto.

Done.

[iclelland, 2017-01-30 20:37:39]

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

https://codereview.chromium.org/2640823004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.h:54: // when the
V8 context is reused (i.e., after navigation)
On 2017/01/28 02:25:20, haraken wrote:
> 
> Not related to this CL, would you again help me understand why we need a
special
> logic for window? I think you're worrying about a case where a window object
is
> reused when we navigate from an initial empty document to a same-origin
> document, but I've forgotten why we need a special handling for that case.

As I recall, when navigating between documents (not just from the initial empty
document), we clear out the per-context-data, but the context object itself is
reused. At that point, we needed to install any new origin trial attributes on
Window, so that the constructor interfaces are exposed to scripts.

https://codereview.chromium.org/2640823004/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/bindings/modules/v8/ConditionalFeaturesForModules.cpp
(right):

https://codereview.chromium.org/2640823004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/modules/v8/ConditionalFeaturesForModules.cpp:219:
v8::Local<v8::Object> instanceObject = scriptState->context()->Global();
On 2017/01/28 02:25:20, haraken wrote:
> 
> You can factor out this statement out of the if clause. instanceObject is used
> in many if clauses.
> 

chasej -- I don't have really strong feelings one way or the other about this,
but if you do factor it out, please rename it to something like
"globalInstanceObject". The global object is only the instanceObject when we're
installing trials on the global. (There has been confusion over this before)

[chasej, 2017-01-30 20:53:14]

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

[commit-bot: I haz the power, 2017-01-30 20:53:40]

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

[chasej, 2017-01-30 20:55:50]

On 2017/01/30 20:37:39, iclelland wrote:
>
https://codereview.chromium.org/2640823004/diff/100001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.h (right):
> 
>
https://codereview.chromium.org/2640823004/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.h:54: // when
the
> V8 context is reused (i.e., after navigation)
> On 2017/01/28 02:25:20, haraken wrote:
> > 
> > Not related to this CL, would you again help me understand why we need a
> special
> > logic for window? I think you're worrying about a case where a window object
> is
> > reused when we navigate from an initial empty document to a same-origin
> > document, but I've forgotten why we need a special handling for that case.
> 
> As I recall, when navigating between documents (not just from the initial
empty
> document), we clear out the per-context-data, but the context object itself is
> reused. At that point, we needed to install any new origin trial attributes on
> Window, so that the constructor interfaces are exposed to scripts.
> 
>
https://codereview.chromium.org/2640823004/diff/100001/third_party/WebKit/Sou...
> File
>
third_party/WebKit/Source/bindings/modules/v8/ConditionalFeaturesForModules.cpp
> (right):
> 
>
https://codereview.chromium.org/2640823004/diff/100001/third_party/WebKit/Sou...
>
third_party/WebKit/Source/bindings/modules/v8/ConditionalFeaturesForModules.cpp:219:
> v8::Local<v8::Object> instanceObject = scriptState->context()->Global();
> On 2017/01/28 02:25:20, haraken wrote:
> > 
> > You can factor out this statement out of the if clause. instanceObject is
used
> > in many if clauses.
> > 
> 
> chasej -- I don't have really strong feelings one way or the other about this,
> but if you do factor it out, please rename it to something like
> "globalInstanceObject". The global object is only the instanceObject when
we're
> installing trials on the global. (There has been confusion over this before)

That's a good point about the confusion. Since I'd already refactored, I've
updated to rename the variable as you suggested.

[commit-bot: I haz the power, 2017-01-30 22:43:23]

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

[commit-bot: I haz the power, 2017-01-30 22:43:25]

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

[haraken, 2017-01-31 08:19:35]

LGTM

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

https://codereview.chromium.org/2640823004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.h:54: // when the
V8 context is reused (i.e., after navigation)
On 2017/01/30 20:37:39, iclelland wrote:
> On 2017/01/28 02:25:20, haraken wrote:
> > 
> > Not related to this CL, would you again help me understand why we need a
> special
> > logic for window? I think you're worrying about a case where a window object
> is
> > reused when we navigate from an initial empty document to a same-origin
> > document, but I've forgotten why we need a special handling for that case.
> 
> As I recall, when navigating between documents (not just from the initial
empty
> document), we clear out the per-context-data, but the context object itself is
> reused. At that point, we needed to install any new origin trial attributes on
> Window, so that the constructor interfaces are exposed to scripts.

Hmm, I'm a bit confused.

When we navigate from one document to another document, a global proxy is
reused. A global object is recreated. A per-context data is recreated. Are you
install anything on the global proxy? That shouldn't happen because it will
"leak" the origin trials on previous windows to future windows...

What's more confusing is:

- installConditionalFeatures calls s_installConditionalFeaturesFunction.
- install"Pending"ConditionalFeaturesOnWindow calls
s_installConditionalFeaturesFunction
- install"Pending"ConditionalFeature calls
s_install"Pending"ConditionalFeatureFunction

Why is install"Pending"ConditionalFeaturesOnWindow not calling
s_install"Pending"ConditionalFeatureFunction?

https://codereview.chromium.org/2640823004/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp (right):

https://codereview.chromium.org/2640823004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp:50: void
installPendingConditionalFeatureCore(const String& feature,

Would you help me understand why you need both installConditionalFeaturesCore
and installPendingConditionalFeatureCore? I'm wondering why we cannot use
installPendingConditionalFeatureCore unconditionally.

https://codereview.chromium.org/2640823004/diff/140001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/bindings/modules/v8/ConditionalFeaturesForModules.cpp
(right):

https://codereview.chromium.org/2640823004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/modules/v8/ConditionalFeaturesForModules.cpp:166:
return;

Nit: You're using an else-if pattern in installConditionalFeaturesForModules
whereas you're using an early return in
installPendingConditionalFeatureForModules. Unify the patterns.

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

https://codereview.chromium.org/2640823004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:200: for
(auto enabledTrial : m_enabledTrials) {

It might be a bit wasteful to iterate m_enabledTrials every time.

Can we replace m_enabledTrials and m_installedTrials with m_pendingTrials? Then
you just need to iterate m_pendingTrials here. Enabled trials are removed from
m_pendingTrials.

You can do that in a follow-up.

[iclelland, 2017-01-31 15:29:18]

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

https://codereview.chromium.org/2640823004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.h:54: // when the
V8 context is reused (i.e., after navigation)
On 2017/01/31 08:19:35, haraken wrote:
> On 2017/01/30 20:37:39, iclelland wrote:
> > On 2017/01/28 02:25:20, haraken wrote:
> > > 
> > > Not related to this CL, would you again help me understand why we need a
> > special
> > > logic for window? I think you're worrying about a case where a window
object
> > is
> > > reused when we navigate from an initial empty document to a same-origin
> > > document, but I've forgotten why we need a special handling for that case.
> > 
> > As I recall, when navigating between documents (not just from the initial
> empty
> > document), we clear out the per-context-data, but the context object itself
is
> > reused. At that point, we needed to install any new origin trial attributes
on
> > Window, so that the constructor interfaces are exposed to scripts.
> 
> Hmm, I'm a bit confused.
> 
> When we navigate from one document to another document, a global proxy is
> reused. A global object is recreated. A per-context data is recreated. Are you
> install anything on the global proxy? That shouldn't happen because it will
> "leak" the origin trials on previous windows to future windows...

The original issue that prompted that code was https://crbug.com/653671, which
was addressed in https://codereview.chromium.org/2458183002. What we were seeing
was exactly the opposite of that. Navigating from a page where the trial was not
installed to one where it was would not actually install it. (Navigating from a
page with the trial to one without wouldn't leak the trial; it was not present
on the new page).

If that behaviour is unexpected, we should look into why it's happening that
way.

> What's more confusing is:
> 
> - installConditionalFeatures calls s_installConditionalFeaturesFunction.
> - install"Pending"ConditionalFeaturesOnWindow calls
> s_installConditionalFeaturesFunction
> - install"Pending"ConditionalFeature calls
> s_install"Pending"ConditionalFeatureFunction
> 
> Why is install"Pending"ConditionalFeaturesOnWindow not calling
> s_install"Pending"ConditionalFeatureFunction?

chasej, is that just bad naming at this point?
installPendingConditionalFeaturesOnWindow pre-dates the new
installPendingConditionalFeatures functions. Perhaps it should just be
"installConditionalFeaturesOnWindow" now, for consistency.

[chasej, 2017-02-01 00:54:49]

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

[commit-bot: I haz the power, 2017-02-01 00:55:36]

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

[chasej, 2017-02-01 00:57:48]

On 2017/01/31 15:29:18, iclelland wrote:
> > What's more confusing is:
> > 
> > - installConditionalFeatures calls s_installConditionalFeaturesFunction.
> > - install"Pending"ConditionalFeaturesOnWindow calls
> > s_installConditionalFeaturesFunction
> > - install"Pending"ConditionalFeature calls
> > s_install"Pending"ConditionalFeatureFunction
> > 
> > Why is install"Pending"ConditionalFeaturesOnWindow not calling
> > s_install"Pending"ConditionalFeatureFunction?
> 
> chasej, is that just bad naming at this point?
> installPendingConditionalFeaturesOnWindow pre-dates the new
> installPendingConditionalFeatures functions. Perhaps it should just be
> "installConditionalFeaturesOnWindow" now, for consistency.

Yes, probably bad naming now. I've renamed to
"installConditionalFeaturesOnWindow".

[commit-bot: I haz the power, 2017-02-01 00:58:45]

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

[commit-bot: I haz the power, 2017-02-01 00:58:46]

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-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[chasej, 2017-02-01 00:59:52]

haraken, thanks for the detailed review and good questions!

I've addressed your remaining comments below. I believe they were all
questions/nits/suggestions for followup CLs.

Pending approval from iclelland, I'm hoping I'm good to land this.

https://codereview.chromium.org/2640823004/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp (right):

https://codereview.chromium.org/2640823004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp:50: void
installPendingConditionalFeatureCore(const String& feature,
On 2017/01/31 08:19:35, haraken wrote:
> 
> Would you help me understand why you need both installConditionalFeaturesCore
> and installPendingConditionalFeatureCore? I'm wondering why we cannot use
> installPendingConditionalFeatureCore unconditionally.

We need both methods to handle two different install scenarios:
1) On object creation, install all enabled trials for the type being created
2) On newly-enabling a trial, install the trial on all existing types that are
affected

For (1), we use installConditionalFeatures*, which checks which trials
are enabled, and affect the single type being created (i.e. driven by type).

For (2), we use installPendingConditionalFeature*, which checks which
types are affected, and do not yet exist, to install the single feature.

Conceptually, there's a single mapping of enabled trial to affected types.
However, the two scenarios need to slice that mapping in different ways.
In theory, we could implement one method to support both scenarios. I
don't think it's feasible to have one method with an efficient implementation
for both though.

https://codereview.chromium.org/2640823004/diff/140001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/bindings/modules/v8/ConditionalFeaturesForModules.cpp
(right):

https://codereview.chromium.org/2640823004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/modules/v8/ConditionalFeaturesForModules.cpp:166:
return;
On 2017/01/31 08:19:35, haraken wrote:
> 
> Nit: You're using an else-if pattern in installConditionalFeaturesForModules
> whereas you're using an early return in
> installPendingConditionalFeatureForModules. Unify the patterns.

The intent is to use code-generation for both of these methods. I'd prefer to
defer the suggested cleanup for that work.

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

https://codereview.chromium.org/2640823004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:200: for
(auto enabledTrial : m_enabledTrials) {
On 2017/01/31 08:19:35, haraken wrote:
> 
> It might be a bit wasteful to iterate m_enabledTrials every time.
> 
> Can we replace m_enabledTrials and m_installedTrials with m_pendingTrials?
Then
> you just need to iterate m_pendingTrials here. Enabled trials are removed from
> m_pendingTrials.
> 
> You can do that in a follow-up.

We can't get rid of m_enabledTrials, as that is needed for isTrialsEnabled()
(i.e. checking if a trial is enabled). That means we still need to separately
track the enabled trials vs pending trials (or conversely installed trials).

It will also be tricky to track pending trials correctly. A trial could be
enabled by multiple tokens, so I don't think can simply add every new token to a
list of pending trials. It seems to me that tracking which trials have been
explicitly installed is the best path.

We're assuming that the large majority of pages will only have a few trials
enabled, if any (i.e. < 5). Under this assumption, and the issues above, are you
still concern about the iterating m_enabledTrials?

[chasej, 2017-02-01 02:14:39]

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

[commit-bot: I haz the power, 2017-02-01 02:15:37]

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, 2017-02-01 03:59:26]

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

[commit-bot: I haz the power, 2017-02-01 03:59:27]

Dry run: This issue passed the CQ dry run.

[haraken, 2017-02-01 04:02:55]

Still LGTM

[iclelland, 2017-02-01 04:23:11]

LGTM

One more question -- it appears that the test expectations under the new
origin-trials virtual test suite are identical to those under virtual/stable --
is the environment sufficiently different in the two sets of flags that we need
to test both?

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

https://codereview.chromium.org/2640823004/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:180: // Only
install pending features if at least one of the provided token are
Super nit: "tokens"

[chasej, 2017-02-01 06:10:11]

On 2017/02/01 04:23:11, iclelland wrote:
> LGTM
> 
> One more question -- it appears that the test expectations under the new
> origin-trials virtual test suite are identical to those under virtual/stable
--
> is the environment sufficiently different in the two sets of flags that we
need
> to test both?
No, probably don't need to test both at this point. I added the disabled suite
to explicitly control
the disabled status of the features. But, you're correct, the identical
expectations suggest
that both aren't needed. I have crbug.com/685788 to address the command line for
the 
suite - I'll add a note to revisit if both virtual suites are needed.

> 
>
https://codereview.chromium.org/2640823004/diff/200001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp
> (right):
> 
>
https://codereview.chromium.org/2640823004/diff/200001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:180: //
Only
> install pending features if at least one of the provided token are
> Super nit: "tokens"
Done.

[chasej, 2017-02-01 06:13:55]

The CQ bit was checked by chasej@chromium.org

[chasej, 2017-02-01 06:13:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, iclelland@chromium.org
Link to the patchset: https://codereview.chromium.org/2640823004/#ps220001
(title: "Address nit")

[commit-bot: I haz the power, 2017-02-01 06:14:12]

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, 2017-02-01 07:49:29]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1485929635970610,
"parent_rev": "f635e7f7a4d7e1f62bab36d230183e499627b9f7", "commit_rev":
"8206eacd2be4650deb6d9c706533ee6473d6dd3d"}

[commit-bot: I haz the power, 2017-02-01 07:50:02]

Description was changed from

==========
Allow origin trials to be enabled by script

Design doc:
https://docs.google.com/a/chromium.org/document/d/1oGM6VErHxxvA4JsjstNa6NizC4...

BUG=667301
==========

to

==========
Allow origin trials to be enabled by script

Design doc:
https://docs.google.com/a/chromium.org/document/d/1oGM6VErHxxvA4JsjstNa6NizC4...

BUG=667301

Review-Url: https://codereview.chromium.org/2640823004
Cr-Commit-Position: refs/heads/master@{#447471}
Committed:
https://chromium.googlesource.com/chromium/src/+/8206eacd2be4650deb6d9c706533...
==========

[commit-bot: I haz the power, 2017-02-01 07:50:03]

Committed patchset #12 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/8206eacd2be4650deb6d9c706533...