Enable WebVR origin trial experiments.

Enable WebVR origin trial experiments.

Intent to Experiment discussion:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/zGAzqfi0e00/atXaz8UCAQAJ

BUG=652926
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/96e5488ff8ee752b774b20c3eea3ffd0227a70d6
Cr-Commit-Position: refs/heads/master@{#423925}

[amp, 2016-10-05 00:49:31]

Description was changed from

==========
Enable WebVR origin trial experiments.

BUG=652926
==========

to

==========
Enable WebVR origin trial experiments.

BUG=652926
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[iclelland, 2016-10-05 21:55:56]

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

[iclelland, 2016-10-05 21:55:57]

Looks pretty good -- I left a couple of comments that should help you get it up
and running.

https://codereview.chromium.org/2385123005/diff/40001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2385123005/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:2187: // What to do here? 
This is behind a build flag any way so maybe wait until
If WebVR ships as an origin trial, then I think that you'll need to enable the
browser-side components unconditionally, since you can't tell whether any given
renderer is going to need it until it actually navigates to the page.

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

https://codereview.chromium.org/2385123005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/modules/v8/ConditionalFeaturesForModules.cpp:81:
V8WindowPartial::installWebVR(isolate, world, global, prototypeObject,
This should probably be V8NavigatorPartial::installWebVR, by the look of it.

https://codereview.chromium.org/2385123005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right):

https://codereview.chromium.org/2385123005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:104:
GamepadExtensions implied_by=WebVR
You can add origin_trial_feature_name=WebVR here, and then the same token will
enable both. Origin trials doesn't handle implied_by dynamically.

[amp, 2016-10-05 22:04:43]

Thanks for the pointers.  I'm trying out the updates now.

https://codereview.chromium.org/2385123005/diff/40001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2385123005/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:2187: // What to do here? 
This is behind a build flag any way so maybe wait until
On 2016/10/05 21:55:57, iclelland wrote:
> If WebVR ships as an origin trial, then I think that you'll need to enable the
> browser-side components unconditionally, since you can't tell whether any
given
> renderer is going to need it until it actually navigates to the page.

I think we are going for having the components always there, but I'm not exactly
sure what the VRService here does so I'll dig a bit more.

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

https://codereview.chromium.org/2385123005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/modules/v8/ConditionalFeaturesForModules.cpp:81:
V8WindowPartial::installWebVR(isolate, world, global, prototypeObject,
On 2016/10/05 21:55:57, iclelland wrote:
> This should probably be V8NavigatorPartial::installWebVR, by the look of it.

Ah, yes it should.  I wasn't paying enough attention when I copied. :)

https://codereview.chromium.org/2385123005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right):

https://codereview.chromium.org/2385123005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:104:
GamepadExtensions implied_by=WebVR
On 2016/10/05 21:55:57, iclelland wrote:
> You can add origin_trial_feature_name=WebVR here, and then the same token will
> enable both. Origin trials doesn't handle implied_by dynamically.

That sounds great.  Will I need to update the idl's for the GamepadExtensions to
refer to the WebVR extensions or can those remain as is?  (for example
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/gamepa...)

[chasej, 2016-10-05 22:46:47]

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

[chasej, 2016-10-05 22:46:47]

https://codereview.chromium.org/2385123005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right):

https://codereview.chromium.org/2385123005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:104:
GamepadExtensions implied_by=WebVR
On 2016/10/05 22:04:43, amp wrote:
> On 2016/10/05 21:55:57, iclelland wrote:
> > You can add origin_trial_feature_name=WebVR here, and then the same token
will
> > enable both. Origin trials doesn't handle implied_by dynamically.
> 
> That sounds great.  Will I need to update the idl's for the GamepadExtensions
to
> refer to the WebVR extensions or can those remain as is?  (for example
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/gamepa...)

You will need to update the IDLs for the GamepadExtensions as well. Note that
you'll need to change it from [RuntimeEnabled=GamepadExtensions] to
[OriginTrialEnabled=GamepadExtensions]. The point is to use the runtime flag
name, not the origin trial feature name.

[amp, 2016-10-05 23:11:00]

On 2016/10/05 22:46:47, chasej wrote:
>
https://codereview.chromium.org/2385123005/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right):
> 
>
https://codereview.chromium.org/2385123005/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:104:
> GamepadExtensions implied_by=WebVR
> On 2016/10/05 22:04:43, amp wrote:
> > On 2016/10/05 21:55:57, iclelland wrote:
> > > You can add origin_trial_feature_name=WebVR here, and then the same token
> will
> > > enable both. Origin trials doesn't handle implied_by dynamically.
> > 
> > That sounds great.  Will I need to update the idl's for the
GamepadExtensions
> to
> > refer to the WebVR extensions or can those remain as is?  (for example
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/gamepa...)
> 
> You will need to update the IDLs for the GamepadExtensions as well. Note that
> you'll need to change it from [RuntimeEnabled=GamepadExtensions] to
> [OriginTrialEnabled=GamepadExtensions]. The point is to use the runtime flag
> name, not the origin trial feature name.

Latest patch includes those, but I can't get the api to show up (the WebVR one
is working great now).  I also tried to add to the ConditionalFeatures (but I
may again be doing something wrong).

[iclelland, 2016-10-06 03:03:16]

On 2016/10/05 23:11:00, amp wrote:
> On 2016/10/05 22:46:47, chasej wrote:
> >
>
https://codereview.chromium.org/2385123005/diff/40001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right):
> > 
> >
>
https://codereview.chromium.org/2385123005/diff/40001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:104:
> > GamepadExtensions implied_by=WebVR
> > On 2016/10/05 22:04:43, amp wrote:
> > > On 2016/10/05 21:55:57, iclelland wrote:
> > > > You can add origin_trial_feature_name=WebVR here, and then the same
token
> > will
> > > > enable both. Origin trials doesn't handle implied_by dynamically.
> > > 
> > > That sounds great.  Will I need to update the idl's for the
> GamepadExtensions
> > to
> > > refer to the WebVR extensions or can those remain as is?  (for example
> > >
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/gamepa...)
> > 
> > You will need to update the IDLs for the GamepadExtensions as well. Note
that
> > you'll need to change it from [RuntimeEnabled=GamepadExtensions] to
> > [OriginTrialEnabled=GamepadExtensions]. The point is to use the runtime flag
> > name, not the origin trial feature name.
> 
> Latest patch includes those, but I can't get the api to show up (the WebVR one
> is working great now).  I also tried to add to the ConditionalFeatures (but I
> may again be doing something wrong).

No problem -- you've added origin-trial-controlled features to Gamepad and
GamepadButton now as well, so we need to detect those interfaces in
ConditionalFeatures and install the attributes there.

(I ran "grep -rl installGamepadExtensions out/Default/gen/blink/bindings" to see
everywhere that this needed to be added)

You'll have to import V8Gamepad.h and V8GamepadButton.h, and then add a block
like

  } else if (wrapperTypeInfo == &V8Gamepad::wrapperTypeInfo) {
    if (RuntimeEnabledFeatures::gamepadExtensionsEnabled() ||
        (originTrialContext &&
         originTrialContext->isFeatureEnabled("GamedpadExtensions"))) {
      V8Gamepad::installGamepadExtensions(isolate, world, global,
prototypeObject,
                                     interfaceObject);
    }
  } else if (wrapperTypeInfo == &V8GamepadButton::wrapperTypeInfo) {
    if (RuntimeEnabledFeatures::gamepadExtensionsEnabled() ||
        (originTrialContext &&
         originTrialContext->isFeatureEnabled("GamedpadExtensions"))) {
      V8GamepadButton::installGamepadExtensions(isolate, world, global,
prototypeObject,
                                     interfaceObject);
    }
  
at the end of the installConditionalFeaturesForModules function.

(And I apologize for making you go through all that; this code needs to be
auto-generated, and will be very soon)

[chasej, 2016-10-06 13:08:33]

On 2016/10/06 03:03:16, iclelland wrote:
> On 2016/10/05 23:11:00, amp wrote:
> > On 2016/10/05 22:46:47, chasej wrote:
> > >
> >
>
https://codereview.chromium.org/2385123005/diff/40001/third_party/WebKit/Sour...
> > > File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2385123005/diff/40001/third_party/WebKit/Sour...
> > > third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:104:
> > > GamepadExtensions implied_by=WebVR
> > > On 2016/10/05 22:04:43, amp wrote:
> > > > On 2016/10/05 21:55:57, iclelland wrote:
> > > > > You can add origin_trial_feature_name=WebVR here, and then the same
> token
> > > will
> > > > > enable both. Origin trials doesn't handle implied_by dynamically.
> > > > 
> > > > That sounds great.  Will I need to update the idl's for the
> > GamepadExtensions
> > > to
> > > > refer to the WebVR extensions or can those remain as is?  (for example
> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/gamepa...)
> > > 
> > > You will need to update the IDLs for the GamepadExtensions as well. Note
> that
> > > you'll need to change it from [RuntimeEnabled=GamepadExtensions] to
> > > [OriginTrialEnabled=GamepadExtensions]. The point is to use the runtime
flag
> > > name, not the origin trial feature name.
> > 
> > Latest patch includes those, but I can't get the api to show up (the WebVR
one
> > is working great now).  I also tried to add to the ConditionalFeatures (but
I
> > may again be doing something wrong).
> 
> No problem -- you've added origin-trial-controlled features to Gamepad and
> GamepadButton now as well, so we need to detect those interfaces in
> ConditionalFeatures and install the attributes there.
> 
> (I ran "grep -rl installGamepadExtensions out/Default/gen/blink/bindings" to
see
> everywhere that this needed to be added)
> 
> You'll have to import V8Gamepad.h and V8GamepadButton.h, and then add a block
> like
> 
>   } else if (wrapperTypeInfo == &V8Gamepad::wrapperTypeInfo) {
>     if (RuntimeEnabledFeatures::gamepadExtensionsEnabled() ||
>         (originTrialContext &&
>          originTrialContext->isFeatureEnabled("GamedpadExtensions"))) {
>       V8Gamepad::installGamepadExtensions(isolate, world, global,
> prototypeObject,
>                                      interfaceObject);
>     }
>   } else if (wrapperTypeInfo == &V8GamepadButton::wrapperTypeInfo) {
>     if (RuntimeEnabledFeatures::gamepadExtensionsEnabled() ||
>         (originTrialContext &&
>          originTrialContext->isFeatureEnabled("GamedpadExtensions"))) {
>       V8GamepadButton::installGamepadExtensions(isolate, world, global,
> prototypeObject,
>                                      interfaceObject);
>     }
>   
> at the end of the installConditionalFeaturesForModules function.
> 
> (And I apologize for making you go through all that; this code needs to be
> auto-generated, and will be very soon)

Subtle, but important, correction. In all cases, it should be
originTrialContext->isFeatureEnabled("WebVR"). It is a bit confusing, which is
why we want all this code to be generated.

The point is that isFeatureEnabled() is checking the origin_trial_feature_name
value from RuntimeEnabledFeatures.in. It is not checking the runtime feature
name.

[amp, 2016-10-06 17:16:55]

Thanks for pointing all the issues out.  It makes sense now.  Reiterating just
to make sure I got it.

The origin trial name needs to refer to the new trial just created, but the
runtime feature name stays the same.

V8 conditional features need to include any API's we are updating with our
feature, so we need to add references to the Gamepad and Buttons API's which we
modify.

Trying this out now. :)

[iclelland, 2016-10-06 17:36:25]

On Thu, Oct 6, 2016 at 1:16 PM, <amp@chromium.org> wrote:

> Thanks for pointing all the issues out. It makes sense now. Reiterating
> just
> to make sure I got it.
>
> The origin trial name needs to refer to the new trial just created, but the
> runtime feature name stays the same.
>

Yes. I've created https://codereview.chromium.org/2398793003/ to address
the confusion there, but I'll hold off on submitting that until your CL is
landed, so you don't have to think about it again :)


>
> V8 conditional features need to include any API's we are updating with our
> feature, so we need to add references to the Gamepad and Buttons API's
> which we
> modify.
>

Also yes. Any interfaces which have [OriginTrialEnabled] attributes on them
need to be included in conditional features.

If the *entire* interface is marked as [OriginTrialEnabled], then it's not
needed (for instance, like in VRDisplay.idl), but if individual attributes
are tagged, then that's when it needs to be there.


>
> Trying this out now. :)
>
> https://codereview.chromium.org/2385123005/
>

Ian

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[iclelland, 2016-10-06 17:36:26]

On Thu, Oct 6, 2016 at 1:16 PM, <amp@chromium.org> wrote:

> Thanks for pointing all the issues out. It makes sense now. Reiterating
> just
> to make sure I got it.
>
> The origin trial name needs to refer to the new trial just created, but the
> runtime feature name stays the same.
>

Yes. I've created https://codereview.chromium.org/2398793003/ to address
the confusion there, but I'll hold off on submitting that until your CL is
landed, so you don't have to think about it again :)


>
> V8 conditional features need to include any API's we are updating with our
> feature, so we need to add references to the Gamepad and Buttons API's
> which we
> modify.
>

Also yes. Any interfaces which have [OriginTrialEnabled] attributes on them
need to be included in conditional features.

If the *entire* interface is marked as [OriginTrialEnabled], then it's not
needed (for instance, like in VRDisplay.idl), but if individual attributes
are tagged, then that's when it needs to be there.


>
> Trying this out now. :)
>
> https://codereview.chromium.org/2385123005/
>

Ian

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[amp, 2016-10-06 18:13:58]

On 2016/10/06 17:36:26, iclelland wrote:
> On Thu, Oct 6, 2016 at 1:16 PM, <mailto:amp@chromium.org> wrote:
> 
> > Thanks for pointing all the issues out. It makes sense now. Reiterating
> > just
> > to make sure I got it.
> >
> > The origin trial name needs to refer to the new trial just created, but the
> > runtime feature name stays the same.
> >
> 
> Yes. I've created https://codereview.chromium.org/2398793003/ to address
> the confusion there, but I'll hold off on submitting that until your CL is
> landed, so you don't have to think about it again :)

You can go ahead and submit that and I'll rebase.  I just found out that we are
blocked on 55 for a different reason, so the time pressure to get this in by
branch point is reduced and I will take some more time to verify everything is
working without rushing. We'll be targeting 56 it looks like now.
> 
> 
> >
> > V8 conditional features need to include any API's we are updating with our
> > feature, so we need to add references to the Gamepad and Buttons API's
> > which we
> > modify.
> >
> 
> Also yes. Any interfaces which have [OriginTrialEnabled] attributes on them
> need to be included in conditional features.
> 
> If the *entire* interface is marked as [OriginTrialEnabled], then it's not
> needed (for instance, like in VRDisplay.idl), but if individual attributes
> are tagged, then that's when it needs to be there.
> 
> 
> >
> > Trying this out now. :)

It looks like it works, but it only picks up the trial when I kill chrome and
load the page with the META tag as the first page Chrome loads.  If I just
navigate to a page with the tag it doesn't get enabled.  Any ideas about why
that might happen?
> >
> > https://codereview.chromium.org/2385123005/
> >
> 
> Ian
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[iclelland, 2016-10-06 20:28:14]

On 2016/10/06 18:13:58, amp wrote:
> On 2016/10/06 17:36:26, iclelland wrote:
> > On Thu, Oct 6, 2016 at 1:16 PM, <mailto:amp@chromium.org> wrote:
> > 
> > > Thanks for pointing all the issues out. It makes sense now. Reiterating
> > > just
> > > to make sure I got it.
> > >
> > > The origin trial name needs to refer to the new trial just created, but
the
> > > runtime feature name stays the same.
> > >
> > 
> > Yes. I've created https://codereview.chromium.org/2398793003/ to address
> > the confusion there, but I'll hold off on submitting that until your CL is
> > landed, so you don't have to think about it again :)
> 
> You can go ahead and submit that and I'll rebase.  I just found out that we
are
> blocked on 55 for a different reason, so the time pressure to get this in by
> branch point is reduced and I will take some more time to verify everything is
> working without rushing. We'll be targeting 56 it looks like now.
> > 
> > 
> > >
> > > V8 conditional features need to include any API's we are updating with our
> > > feature, so we need to add references to the Gamepad and Buttons API's
> > > which we
> > > modify.
> > >
> > 
> > Also yes. Any interfaces which have [OriginTrialEnabled] attributes on them
> > need to be included in conditional features.
> > 
> > If the *entire* interface is marked as [OriginTrialEnabled], then it's not
> > needed (for instance, like in VRDisplay.idl), but if individual attributes
> > are tagged, then that's when it needs to be there.
> > 
> > 
> > >
> > > Trying this out now. :)
> 
> It looks like it works, but it only picks up the trial when I kill chrome and
> load the page with the META tag as the first page Chrome loads.  If I just
> navigate to a page with the tag it doesn't get enabled.  Any ideas about why
> that might happen?

I need to investigate that -- that may be a recent regression in the framework
that I"ll need to sort out before 55.

Try these pages:
   https://clelland.github.io/GHPage/ot/vr.html
   https://clelland.github.io/GHPage/ot/novr.html

navigator.getVRDisplays works consistently, regardless of which page I start
with (there are links between them)
window.VRDisplay shows the symptom you describe; it only works if you start with
that page, and not if you navigate to it.

I'll investigate, but I think you've done everything right.


> > >
> > > https://codereview.chromium.org/2385123005/
> > >
> > 
> > Ian
> > 
> > -- 
> > You received this message because you are subscribed to the Google Groups
> > "Chromium-reviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.

[amp, 2016-10-06 20:46:10]

On 2016/10/06 20:28:14, iclelland wrote:
> On 2016/10/06 18:13:58, amp wrote:
> > On 2016/10/06 17:36:26, iclelland wrote:
> > > On Thu, Oct 6, 2016 at 1:16 PM, <mailto:amp@chromium.org> wrote:
> > > 
> > > > Thanks for pointing all the issues out. It makes sense now. Reiterating
> > > > just
> > > > to make sure I got it.
> > > >
> > > > The origin trial name needs to refer to the new trial just created, but
> the
> > > > runtime feature name stays the same.
> > > >
> > > 
> > > Yes. I've created https://codereview.chromium.org/2398793003/ to address
> > > the confusion there, but I'll hold off on submitting that until your CL is
> > > landed, so you don't have to think about it again :)
> > 
> > You can go ahead and submit that and I'll rebase.  I just found out that we
> are
> > blocked on 55 for a different reason, so the time pressure to get this in by
> > branch point is reduced and I will take some more time to verify everything
is
> > working without rushing. We'll be targeting 56 it looks like now.
> > > 
> > > 
> > > >
> > > > V8 conditional features need to include any API's we are updating with
our
> > > > feature, so we need to add references to the Gamepad and Buttons API's
> > > > which we
> > > > modify.
> > > >
> > > 
> > > Also yes. Any interfaces which have [OriginTrialEnabled] attributes on
them
> > > need to be included in conditional features.
> > > 
> > > If the *entire* interface is marked as [OriginTrialEnabled], then it's not
> > > needed (for instance, like in VRDisplay.idl), but if individual attributes
> > > are tagged, then that's when it needs to be there.
> > > 
> > > 
> > > >
> > > > Trying this out now. :)
> > 
> > It looks like it works, but it only picks up the trial when I kill chrome
and
> > load the page with the META tag as the first page Chrome loads.  If I just
> > navigate to a page with the tag it doesn't get enabled.  Any ideas about why
> > that might happen?
> 
> I need to investigate that -- that may be a recent regression in the framework
> that I"ll need to sort out before 55.
> 
> Try these pages:
>    https://clelland.github.io/GHPage/ot/vr.html
>    https://clelland.github.io/GHPage/ot/novr.html
> 
> navigator.getVRDisplays works consistently, regardless of which page I start
> with (there are links between them)
> window.VRDisplay shows the symptom you describe; it only works if you start
with
> that page, and not if you navigate to it.
> 
> I'll investigate, but I think you've done everything right.

Thanks.  I tried these pages, but I couldn't get them to work either way.  Are
these using test tokens or official tokens? (I have the
--origin-trial-public-key= flag set on my instance).

> 
> 
> > > >
> > > > https://codereview.chromium.org/2385123005/
> > > >
> > > 
> > > Ian
> > > 
> > > -- 
> > > You received this message because you are subscribed to the Google Groups
> > > "Chromium-reviews" group.
> > > To unsubscribe from this group and stop receiving emails from it, send an
> > email
> > > to mailto:chromium-reviews+unsubscribe@chromium.org.

[amp, 2016-10-06 20:49:48]

amp@chromium.org changed reviewers:
+ bajones@chromium.org

[amp, 2016-10-06 20:49:49]

+bajones for vr owners (no rush as we aren't trying to hit the branch point
anymore, so timing as paternity leave allows is fine). Any suggestions for
content and third party owners to include?

This should be all the changes required to start the origin trial.

[amp, 2016-10-07 00:13:12]

amp@chromium.org changed reviewers:
+ clamy@chromium.org, haraken@chromium.org

[amp, 2016-10-07 00:13:13]

PTAL

+clamy for content/browser/frame_host/render_frame_host_impl.cc
+haraken for everything else

Everything is working now. I verified that the gamepad extensions are good with
the origin trial as well (using a test token on localhost).

[haraken, 2016-10-07 02:19:52]

LGTM

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

https://codereview.chromium.org/2385123005/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/modules/v8/ConditionalFeaturesForModules.cpp:118:
(originTrialContext && originTrialContext->isFeatureEnabled("WebVR"))) {

Shouldn't this be "GamepadExtensions"?

https://codereview.chromium.org/2385123005/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/modules/v8/ConditionalFeaturesForModules.cpp:160:
(originTrialContext && originTrialContext->isFeatureEnabled("WebVR"))) {

Ditto.

https://codereview.chromium.org/2385123005/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/modules/v8/ConditionalFeaturesForModules.cpp:166:
(originTrialContext && originTrialContext->isFeatureEnabled("WebVR"))) {

Ditto.

[iclelland, 2016-10-07 02:20:48]

On 2016/10/06 20:46:10, amp wrote:
> On 2016/10/06 20:28:14, iclelland wrote:
> > On 2016/10/06 18:13:58, amp wrote:
> > > It looks like it works, but it only picks up the trial when I kill chrome
> and
> > > load the page with the META tag as the first page Chrome loads.  If I just
> > > navigate to a page with the tag it doesn't get enabled.  Any ideas about
why
> > > that might happen?
> > 
> > I need to investigate that -- that may be a recent regression in the
framework
> > that I"ll need to sort out before 55.
> > 
> > Try these pages:
> >    https://clelland.github.io/GHPage/ot/vr.html
> >    https://clelland.github.io/GHPage/ot/novr.html
> > 
> > navigator.getVRDisplays works consistently, regardless of which page I start
> > with (there are links between them)
> > window.VRDisplay shows the symptom you describe; it only works if you start
> with
> > that page, and not if you navigate to it.
> > 
> > I'll investigate, but I think you've done everything right.
> 
> Thanks.  I tried these pages, but I couldn't get them to work either way.  Are
> these using test tokens or official tokens? (I have the
> --origin-trial-public-key= flag set on my instance).

That page is using official tokens; they should work with chrome built with your
patch, with no other command line arguments.

[bajones, 2016-10-07 05:30:13]

modules/vr & gamepad LGTM

[haraken, 2016-10-07 05:58:51]

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

[haraken, 2016-10-07 05:58:52]

This will need an API owner review. +jochen

[jochen (gone - plz use gerrit), 2016-10-07 06:57:00]

lgtm

please add a link to the "intent to experiment" discussion to the CL description

[clamy, 2016-10-07 11:09:54]

Thanks!

https://codereview.chromium.org/2385123005/diff/120001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (left):

https://codereview.chromium.org/2385123005/diff/120001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:2207: if
(browser_command_line.HasSwitch(switches::kEnableWebVR)) {
Why not use directly "if
(base::CommandLine::ForCurrentProcess()->HasSwitch(switches::kEnableWebVR))"?
This is what's done everywhere in the code.

[iclelland, 2016-10-07 14:51:50]

https://codereview.chromium.org/2385123005/diff/120001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (left):

https://codereview.chromium.org/2385123005/diff/120001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:2207: if
(browser_command_line.HasSwitch(switches::kEnableWebVR)) {
On 2016/10/07 11:09:54, clamy wrote:
> Why not use directly "if
> (base::CommandLine::ForCurrentProcess()->HasSwitch(switches::kEnableWebVR))"?
> This is what's done everywhere in the code.

Isn't that line being deleted, though?

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

https://codereview.chromium.org/2385123005/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/modules/v8/ConditionalFeaturesForModules.cpp:118:
(originTrialContext && originTrialContext->isFeatureEnabled("WebVR"))) {
On 2016/10/07 02:19:52, haraken wrote:
> 
> Shouldn't this be "GamepadExtensions"?

No, chasej pointed out in #9 that this should be "WebVR"; it's the name of the
origin trial, rather than the name of the runtime feature. crrev.com/2398793003/
is going to change this to |isTrialEnabled| to try to avoid this confusion.

[clamy, 2016-10-07 14:54:40]

https://codereview.chromium.org/2385123005/diff/120001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (left):

https://codereview.chromium.org/2385123005/diff/120001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:2207: if
(browser_command_line.HasSwitch(switches::kEnableWebVR)) {
On 2016/10/07 14:51:50, iclelland wrote:
> On 2016/10/07 11:09:54, clamy wrote:
> > Why not use directly "if
> >
(base::CommandLine::ForCurrentProcess()->HasSwitch(switches::kEnableWebVR))"?
> > This is what's done everywhere in the code.
> 
> Isn't that line being deleted, though?

I'm not quite sure what you mean? If you like at
https://cs.chromium.org/chromium/src/content/common/site_isolation_policy.cc?...
for example, this is what we use. Note that the functions are used all over the
code, at various points in time.

[iclelland, 2016-10-07 15:11:16]

https://codereview.chromium.org/2385123005/diff/120001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (left):

https://codereview.chromium.org/2385123005/diff/120001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:2207: if
(browser_command_line.HasSwitch(switches::kEnableWebVR)) {
On 2016/10/07 14:54:40, clamy wrote:
> On 2016/10/07 14:51:50, iclelland wrote:
> > On 2016/10/07 11:09:54, clamy wrote:
> > > Why not use directly "if
> > >
> (base::CommandLine::ForCurrentProcess()->HasSwitch(switches::kEnableWebVR))"?
> > > This is what's done everywhere in the code.
> > 
> > Isn't that line being deleted, though?
> 
> I'm not quite sure what you mean? If you like at
>
https://cs.chromium.org/chromium/src/content/common/site_isolation_policy.cc?...
> for example, this is what we use. Note that the functions are used all over
the
> code, at various points in time.

I mean that this CL *deletes* the line you're talking about.

base::CommandLine::ForCurrentProcess()->HasSwitch(...) is definitely the idiom
of choice, but it's no longer required once this feature is enabled as an origin
trial.

[clamy, 2016-10-07 15:13:59]

Thanks! content/ lgtm.

https://codereview.chromium.org/2385123005/diff/120001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (left):

https://codereview.chromium.org/2385123005/diff/120001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:2207: if
(browser_command_line.HasSwitch(switches::kEnableWebVR)) {
On 2016/10/07 15:11:16, iclelland wrote:
> On 2016/10/07 14:54:40, clamy wrote:
> > On 2016/10/07 14:51:50, iclelland wrote:
> > > On 2016/10/07 11:09:54, clamy wrote:
> > > > Why not use directly "if
> > > >
> >
(base::CommandLine::ForCurrentProcess()->HasSwitch(switches::kEnableWebVR))"?
> > > > This is what's done everywhere in the code.
> > > 
> > > Isn't that line being deleted, though?
> > 
> > I'm not quite sure what you mean? If you like at
> >
>
https://cs.chromium.org/chromium/src/content/common/site_isolation_policy.cc?...
> > for example, this is what we use. Note that the functions are used all over
> the
> > code, at various points in time.
> 
> I mean that this CL *deletes* the line you're talking about.
> 
> base::CommandLine::ForCurrentProcess()->HasSwitch(...) is definitely the idiom
> of choice, but it's no longer required once this feature is enabled as an
origin
> trial.

Sorry my bad, I read too quickly :-/.

[amp, 2016-10-07 17:04:49]

On 2016/10/07 06:57:00, jochen wrote:
> lgtm
> 
> please add a link to the "intent to experiment" discussion to the CL
description

Done.

It also also linked from the bug as well.

[amp, 2016-10-07 17:05:36]

Description was changed from

==========
Enable WebVR origin trial experiments.

BUG=652926
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Enable WebVR origin trial experiments.

Intent to Experiment discussion:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/zGAzqfi0e00/atXaz8UC...

BUG=652926
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[amp, 2016-10-07 17:07:23]

On 2016/10/07 02:20:48, iclelland wrote:
> On 2016/10/06 20:46:10, amp wrote:
> > On 2016/10/06 20:28:14, iclelland wrote:
> > > On 2016/10/06 18:13:58, amp wrote:
> > > > It looks like it works, but it only picks up the trial when I kill
chrome
> > and
> > > > load the page with the META tag as the first page Chrome loads.  If I
just
> > > > navigate to a page with the tag it doesn't get enabled.  Any ideas about
> why
> > > > that might happen?
> > > 
> > > I need to investigate that -- that may be a recent regression in the
> framework
> > > that I"ll need to sort out before 55.
> > > 
> > > Try these pages:
> > >    https://clelland.github.io/GHPage/ot/vr.html
> > >    https://clelland.github.io/GHPage/ot/novr.html
> > > 
> > > navigator.getVRDisplays works consistently, regardless of which page I
start
> > > with (there are links between them)
> > > window.VRDisplay shows the symptom you describe; it only works if you
start
> > with
> > > that page, and not if you navigate to it.
> > > 
> > > I'll investigate, but I think you've done everything right.
> > 
> > Thanks.  I tried these pages, but I couldn't get them to work either way. 
Are
> > these using test tokens or official tokens? (I have the
> > --origin-trial-public-key= flag set on my instance).
> 
> That page is using official tokens; they should work with chrome built with
your
> patch, with no other command line arguments.

I tried these with no command line flags (just my patch) and saw the same thing.
 navigator.getVRDisplays() always works, but window.VRDisplay is only available
if I restart Chrome on the page.

Is there a bug filed for tracking this?

[amp, 2016-10-07 17:07:36]

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

[commit-bot: I haz the power, 2016-10-07 17:07:54]

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-10-07 17:10:26]

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

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

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  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-10-07 17:17:09]

On 2016/10/07 17:07:23, amp wrote:
> On 2016/10/07 02:20:48, iclelland wrote:
> > On 2016/10/06 20:46:10, amp wrote:
> > > On 2016/10/06 20:28:14, iclelland wrote:
> > > > On 2016/10/06 18:13:58, amp wrote:
> > > > > It looks like it works, but it only picks up the trial when I kill
> chrome
> > > and
> > > > > load the page with the META tag as the first page Chrome loads.  If I
> just
> > > > > navigate to a page with the tag it doesn't get enabled.  Any ideas
about
> > why
> > > > > that might happen?
> > > > 
> > > > I need to investigate that -- that may be a recent regression in the
> > framework
> > > > that I"ll need to sort out before 55.
> > > > 
> > > > Try these pages:
> > > >    https://clelland.github.io/GHPage/ot/vr.html
> > > >    https://clelland.github.io/GHPage/ot/novr.html
> > > > 
> > > > navigator.getVRDisplays works consistently, regardless of which page I
> start
> > > > with (there are links between them)
> > > > window.VRDisplay shows the symptom you describe; it only works if you
> start
> > > with
> > > > that page, and not if you navigate to it.
> > > > 
> > > > I'll investigate, but I think you've done everything right.
> > > 
> > > Thanks.  I tried these pages, but I couldn't get them to work either way. 
> Are
> > > these using test tokens or official tokens? (I have the
> > > --origin-trial-public-key= flag set on my instance).
> > 
> > That page is using official tokens; they should work with chrome built with
> your
> > patch, with no other command line arguments.
> 
> I tried these with no command line flags (just my patch) and saw the same
thing.
>  navigator.getVRDisplays() always works, but window.VRDisplay is only
available
> if I restart Chrome on the page.
> 
> Is there a bug filed for tracking this?

Yep - crbug.com/653671/ -- not specifically for WebVR; it's affecting other
origin trials as well.

[amp, 2016-10-07 17:31:29]

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

[commit-bot: I haz the power, 2016-10-07 17:31: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, 2016-10-07 19:06:46]

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

[commit-bot: I haz the power, 2016-10-07 19:06:48]

Dry run: This issue passed the CQ dry run.

[amp, 2016-10-07 19:11:29]

The CQ bit was checked by amp@chromium.org

[amp, 2016-10-07 19:11:30]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
haraken@chromium.org, bajones@chromium.org, clamy@chromium.org
Link to the patchset: https://codereview.chromium.org/2385123005/#ps160001
(title: "Rebase for conflict in ConditionalFeaturesForModules.cpp")

[commit-bot: I haz the power, 2016-10-07 19:12:03]

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-10-07 19:20:22]

Description was changed from

==========
Enable WebVR origin trial experiments.

Intent to Experiment discussion:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/zGAzqfi0e00/atXaz8UC...

BUG=652926
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Enable WebVR origin trial experiments.

Intent to Experiment discussion:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/zGAzqfi0e00/atXaz8UC...

BUG=652926
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-10-07 19:20:24]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-10-07 19:22:23]

Description was changed from

==========
Enable WebVR origin trial experiments.

Intent to Experiment discussion:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/zGAzqfi0e00/atXaz8UC...

BUG=652926
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Enable WebVR origin trial experiments.

Intent to Experiment discussion:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/zGAzqfi0e00/atXaz8UC...

BUG=652926
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/96e5488ff8ee752b774b20c3eea3ffd0227a70d6
Cr-Commit-Position: refs/heads/master@{#423925}
==========

[commit-bot: I haz the power, 2016-10-07 19:22:25]

Patchset 9 (id:??) landed as
https://crrev.com/96e5488ff8ee752b774b20c3eea3ffd0227a70d6
Cr-Commit-Position: refs/heads/master@{#423925}