Make document.rootScroller into an origin trial

Make document.rootScroller into an origin trial

Intent-to-experiment:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/COF0YKkjZVQ

BUG=680190
Review-Url: https://codereview.chromium.org/2628923002
Cr-Commit-Position: refs/heads/master@{#443956}
Committed: https://chromium.googlesource.com/chromium/src/+/f2b6b4c6b17f90d48d986c3856f4a422ddbdbc2c

[bokan, 2017-01-11 18:30:57]

Description was changed from

==========
Make document.rootScroller into an origin trial

BUG=505516
==========

to

==========
Make document.rootScroller into an origin trial

BUG=680190
==========

[bokan, 2017-01-11 18:33:44]

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

[bokan, 2017-01-11 18:33:45]

Hey Ian,

Two questions:

The OriginTrials documentation says I may also need run V8Binding.cpp's
installOriginTrials but I couldn't find it, either in V8Binding.cpp nor in
CodeSearch more broadly. Are the instructions
(https://github.com/jpchase/OriginTrials/blob/gh-pages/ship-as-trial.md#how-to...)
out of date?

I also couldn't keep both RuntimeEnabled and OriginTrialEnabled on the idl. Does
that mean there's no way for a user to turn my feature on now that it's an
origin trial?

[bokan, 2017-01-12 00:29:39]

Description was changed from

==========
Make document.rootScroller into an origin trial

BUG=680190
==========

to

==========
Make document.rootScroller into an origin trial

Intent-to-experiment:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/COF0YKkjZVQ

BUG=680190
==========

[iclelland, 2017-01-12 15:50:59]

On 2017/01/11 18:33:45, bokan wrote:
> Hey Ian,
> 
> Two questions:
> 
> The OriginTrials documentation says I may also need run V8Binding.cpp's
> installOriginTrials but I couldn't find it, either in V8Binding.cpp nor in
> CodeSearch more broadly. Are the instructions
>
(https://github.com/jpchase/OriginTrials/blob/gh-pages/ship-as-trial.md#how-to...)
> out of date?

Yep. Those docs are out of date. The right methods to update for IDL trials are
in
bindings/core/v8/ConditionalFeatures.cpp and
bindings/modules/v8/ConditionalFeaturesForModules.cpp, depending on whether the
property definitions are in core or modules.


> I also couldn't keep both RuntimeEnabled and OriginTrialEnabled on the idl.
Does
> that mean there's no way for a user to turn my feature on now that it's an
> origin trial?

The bindings compiler doesn't support both attributes -- they're mutually
exclusive, but if you enable an origin trial on the feature, then it will
respect the state of the runtime flag as well -- if the flag is on, then the
feature will be installed unconditionally, and if it's off, then the origin
trial logic will apply (per renderer).

[iclelland, 2017-01-12 15:58:13]

https://codereview.chromium.org/2628923002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/Document.idl (right):

https://codereview.chromium.org/2628923002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Document.idl:81: [RaisesException=Setter,
OriginTrialEnabled=RootScroller, Measure] attribute Element? rootScroller;
You'll also need to add a block in installConditionalFeaturesForModules to add
the property to Document when the trial is enabled (this is the annoying manual
bit that we need to eventually generate).

When it sees a document object, it needs to run the generated
installRootScroller method on the prototype. I haven't checked this code too
carefully, but it should probably look something like:

  if (wrapperTypeInfo == &V8Document::wrapperTypeInfo) {
    if (OriginTrials::rootScrollerEnabled(executionContext)) {
      V8DocumentPartial::installRootScroller(isolate, world,
                                          v8::Local<v8::Object>(),
                                          prototypeObject, interfaceObject);
    }
  }

[bokan, 2017-01-12 16:11:30]

On 2017/01/12 15:50:59, iclelland wrote:
> On 2017/01/11 18:33:45, bokan wrote:
> > I also couldn't keep both RuntimeEnabled and OriginTrialEnabled on the idl.
> Does
> > that mean there's no way for a user to turn my feature on now that it's an
> > origin trial?
> 
> The bindings compiler doesn't support both attributes -- they're mutually
> exclusive, but if you enable an origin trial on the feature, then it will
> respect the state of the runtime flag as well -- if the flag is on, then the
> feature will be installed unconditionally, and if it's off, then the origin
> trial logic will apply (per renderer).

Ah, ok, when I tried it locally it wasn't working but that was because I hadn't
added it to installConditionalFeatures. Works for me now, thanks.

[bokan, 2017-01-12 16:11:59]

Added the attribute to installConditionalFeatures. PTAL

https://codereview.chromium.org/2628923002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/Document.idl (right):

https://codereview.chromium.org/2628923002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Document.idl:81: [RaisesException=Setter,
OriginTrialEnabled=RootScroller, Measure] attribute Element? rootScroller;
On 2017/01/12 15:58:13, iclelland wrote:
> You'll also need to add a block in installConditionalFeaturesForModules to add
> the property to Document when the trial is enabled (this is the annoying
manual
> bit that we need to eventually generate).
> 
> When it sees a document object, it needs to run the generated
> installRootScroller method on the prototype. I haven't checked this code too
> carefully, but it should probably look something like:
> 
>   if (wrapperTypeInfo == &V8Document::wrapperTypeInfo) {
>     if (OriginTrials::rootScrollerEnabled(executionContext)) {
>       V8DocumentPartial::installRootScroller(isolate, world,
>                                           v8::Local<v8::Object>(),
>                                           prototypeObject, interfaceObject);
>     }
>   }

Do you mean in installConditionalFeatures? My feature isn't in a module but in
core.

[iclelland, 2017-01-12 19:26:41]

https://codereview.chromium.org/2628923002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/Document.idl (right):

https://codereview.chromium.org/2628923002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Document.idl:81: [RaisesException=Setter,
OriginTrialEnabled=RootScroller, Measure] attribute Element? rootScroller;
On 2017/01/12 16:11:59, bokan wrote:
> On 2017/01/12 15:58:13, iclelland wrote:
> > You'll also need to add a block in installConditionalFeaturesForModules to
add
> > the property to Document when the trial is enabled (this is the annoying
> manual
> > bit that we need to eventually generate).
> > 
> > When it sees a document object, it needs to run the generated
> > installRootScroller method on the prototype. I haven't checked this code too
> > carefully, but it should probably look something like:
> > 
> >   if (wrapperTypeInfo == &V8Document::wrapperTypeInfo) {
> >     if (OriginTrials::rootScrollerEnabled(executionContext)) {
> >       V8DocumentPartial::installRootScroller(isolate, world,
> >                                           v8::Local<v8::Object>(),
> >                                           prototypeObject, interfaceObject);
> >     }
> >   }
> 
> Do you mean in installConditionalFeatures? My feature isn't in a module but in
> core.

Yes, I totally mean that :) Sorry -- that's what I meant about not having
checked too closely.

s/V8DocumentPartial/V8Document/ then, as well.

[bokan, 2017-01-12 19:28:10]

On 2017/01/12 19:26:41, iclelland wrote:
>
https://codereview.chromium.org/2628923002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/dom/Document.idl (right):
> 
>
https://codereview.chromium.org/2628923002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/dom/Document.idl:81: [RaisesException=Setter,
> OriginTrialEnabled=RootScroller, Measure] attribute Element? rootScroller;
> On 2017/01/12 16:11:59, bokan wrote:
> > On 2017/01/12 15:58:13, iclelland wrote:
> > > You'll also need to add a block in installConditionalFeaturesForModules to
> add
> > > the property to Document when the trial is enabled (this is the annoying
> > manual
> > > bit that we need to eventually generate).
> > > 
> > > When it sees a document object, it needs to run the generated
> > > installRootScroller method on the prototype. I haven't checked this code
too
> > > carefully, but it should probably look something like:
> > > 
> > >   if (wrapperTypeInfo == &V8Document::wrapperTypeInfo) {
> > >     if (OriginTrials::rootScrollerEnabled(executionContext)) {
> > >       V8DocumentPartial::installRootScroller(isolate, world,
> > >                                           v8::Local<v8::Object>(),
> > >                                           prototypeObject,
interfaceObject);
> > >     }
> > >   }
> > 
> > Do you mean in installConditionalFeatures? My feature isn't in a module but
in
> > core.
> 
> Yes, I totally mean that :) Sorry -- that's what I meant about not having
> checked too closely.
> 
> s/V8DocumentPartial/V8Document/ then, as well.

Yup, my latest patch uses V8Document and just cargo-culted from the surrounding
code.

[iclelland, 2017-01-12 19:31:06]

On 2017/01/12 19:28:10, bokan wrote:
> On 2017/01/12 19:26:41, iclelland wrote:
> >
>
https://codereview.chromium.org/2628923002/diff/1/third_party/WebKit/Source/c...
> > File third_party/WebKit/Source/core/dom/Document.idl (right):
> > 
> >
>
https://codereview.chromium.org/2628923002/diff/1/third_party/WebKit/Source/c...
> > third_party/WebKit/Source/core/dom/Document.idl:81: [RaisesException=Setter,
> > OriginTrialEnabled=RootScroller, Measure] attribute Element? rootScroller;
> > On 2017/01/12 16:11:59, bokan wrote:
> > > On 2017/01/12 15:58:13, iclelland wrote:
> > > > You'll also need to add a block in installConditionalFeaturesForModules
to
> > add
> > > > the property to Document when the trial is enabled (this is the annoying
> > > manual
> > > > bit that we need to eventually generate).
> > > > 
> > > > When it sees a document object, it needs to run the generated
> > > > installRootScroller method on the prototype. I haven't checked this code
> too
> > > > carefully, but it should probably look something like:
> > > > 
> > > >   if (wrapperTypeInfo == &V8Document::wrapperTypeInfo) {
> > > >     if (OriginTrials::rootScrollerEnabled(executionContext)) {
> > > >       V8DocumentPartial::installRootScroller(isolate, world,
> > > >                                           v8::Local<v8::Object>(),
> > > >                                           prototypeObject,
> interfaceObject);
> > > >     }
> > > >   }
> > > 
> > > Do you mean in installConditionalFeatures? My feature isn't in a module
but
> in
> > > core.
> > 
> > Yes, I totally mean that :) Sorry -- that's what I meant about not having
> > checked too closely.
> > 
> > s/V8DocumentPartial/V8Document/ then, as well.
> 
> Yup, my latest patch uses V8Document and just cargo-culted from the
surrounding
> code.

Perfect :) This LGTM to enable the feature as a trial.

[bokan, 2017-01-12 19:38:06]

Thanks, so will this automatically kick off all that's needed for origins to
sign up or will someone have to manually go and populate the frontend for
signups? Basically, when can I tell a site author to go get a token?

[bokan, 2017-01-12 19:39:37]

bokan@chromium.org changed reviewers:
+ esprehn@chromium.org

[bokan, 2017-01-12 19:39:37]

+esprehn for platform and bindings OWNER.

[iclelland, 2017-01-12 19:43:59]

On 2017/01/12 19:38:06, bokan wrote:
> Thanks, so will this automatically kick off all that's needed for origins to
> sign up or will someone have to manually go and populate the frontend for
> signups? Basically, when can I tell a site author to go get a token?

You overestimate our current level of automation :)

If you send a note to experimentation-dev@chromium.org with trial start and end
dates, etc, we can add all of the info that we need to the signup form. We
wouldn't normally add it to the form before this code is at least in Canary, but
we can generate tokens manually for you before that. chasej@ should have all of
the policy-ish details if you need them.

[bokan, 2017-01-12 19:45:15]

On 2017/01/12 19:43:59, iclelland wrote:
> On 2017/01/12 19:38:06, bokan wrote:
> > Thanks, so will this automatically kick off all that's needed for origins to
> > sign up or will someone have to manually go and populate the frontend for
> > signups? Basically, when can I tell a site author to go get a token?
> 
> You overestimate our current level of automation :)
> 
> If you send a note to mailto:experimentation-dev@chromium.org with trial start
and end
> dates, etc, we can add all of the info that we need to the signup form. We
> wouldn't normally add it to the form before this code is at least in Canary,
but
> we can generate tokens manually for you before that. chasej@ should have all
of
> the policy-ish details if you need them.

Thanks, there's no rush so I'll send an mail to that address when this CL hits
Canary.

[bokan, 2017-01-13 21:25:31]

bokan@chromium.org changed reviewers:
- esprehn@chromium.org

[bokan, 2017-01-13 21:26:42]

bokan@chromium.org changed reviewers:
+ jam@chromium.org

[bokan, 2017-01-13 21:26:42]

+jam@ for OWNER stamp

[jam, 2017-01-13 23:08:01]

On 2017/01/13 21:26:42, bokan wrote:
> +jam@ for OWNER stamp

i'm not an owner in webkit, pick someone from there

[bokan, 2017-01-15 19:11:32]

bokan@chromium.org changed reviewers:
+ rbyers@chromium.org
- jam@chromium.org

[bokan, 2017-01-15 19:11:43]

+rbyers@

[Rick Byers, 2017-01-16 20:32:47]

LGTM

https://codereview.chromium.org/2628923002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.idl (right):

https://codereview.chromium.org/2628923002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.idl:81: [RaisesException=Setter,
OriginTrialEnabled=RootScroller, Measure] attribute Element? rootScroller;
I assume this can still be enabled by
--enable-experimental-web-platform-features, right?

[bokan, 2017-01-16 21:27:28]

On 2017/01/16 20:32:47, Rick Byers wrote:
> LGTM
> 
>
https://codereview.chromium.org/2628923002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/dom/Document.idl (right):
> 
>
https://codereview.chromium.org/2628923002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/dom/Document.idl:81: [RaisesException=Setter,
> OriginTrialEnabled=RootScroller, Measure] attribute Element? rootScroller;
> I assume this can still be enabled by
> --enable-experimental-web-platform-features, right?

Yes, Ian explained - the binding compiler can't handle having both attributes
but making it an OriginTrialEnabled means that
--enable-experimental-web-platform-features and
--enable-blink-features=SetRootScroller should both work. I've confirmed this
locally.

[bokan, 2017-01-16 21:29:03]

The CQ bit was checked by bokan@chromium.org

[bokan, 2017-01-16 21:29:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
iclelland@chromium.org, rbyers@chromium.org
Link to the patchset: https://codereview.chromium.org/2628923002/#ps40001
(title: "Rebase")

[commit-bot: I haz the power, 2017-01-16 21:29:21]

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-16 23:02:26]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1484602143176430,
"parent_rev": "a4eab9aa9b5cbeb5e9dde2ce1eb0ecd6313c8f6c", "commit_rev":
"f2b6b4c6b17f90d48d986c3856f4a422ddbdbc2c"}

[commit-bot: I haz the power, 2017-01-16 23:02:56]

Description was changed from

==========
Make document.rootScroller into an origin trial

Intent-to-experiment:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/COF0YKkjZVQ

BUG=680190
==========

to

==========
Make document.rootScroller into an origin trial

Intent-to-experiment:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/COF0YKkjZVQ

BUG=680190

Review-Url: https://codereview.chromium.org/2628923002
Cr-Commit-Position: refs/heads/master@{#443956}
Committed:
https://chromium.googlesource.com/chromium/src/+/f2b6b4c6b17f90d48d986c3856f4...
==========

[commit-bot: I haz the power, 2017-01-16 23:02:58]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/f2b6b4c6b17f90d48d986c3856f4...

[bokan, 2017-02-23 14:46:37]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/2707133008/ by bokan@chromium.org.

The reason for reverting is: Origin trial has been delayed for now.