Add VrShell feature flag and wire it up to VrShell delegate.

Add VrShell feature flag and wire it up to VrShell delegate.

This is in preparation for a finch experiment and also updates existing VrShell
command line switch, and about:flags language.

When the feature is enabled it will enable WebVR and GamepadExtension features.

BUG=655199
Committed: https://crrev.com/136c3e8264f81f9e41f71982bcbe8cffa080d90c
Cr-Commit-Position: refs/heads/master@{#429892}

[mthiesse, 2016-11-01 01:20:58]

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

[mthiesse, 2016-11-01 01:20:59]

https://codereview.chromium.org/2470583002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java
(right):

https://codereview.chromium.org/2470583002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:40:
private boolean mVrShellEnabled = false;
Clank style is to leave booleans default initialized when false.

https://codereview.chromium.org/2470583002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:114:
mVrShellEnabled = ChromeFeatureList.isEnabled(ChromeFeatureList.VR_SHELL);
You should make mVrShellEnabled a tri-state Boolean, defaulting to null, then
all accesses of it should go through isVrShellEnabled(), which initializes it if
it's null and jni is ready (LibraryLoader.isInitialized()).

https://codereview.chromium.org/2470583002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:395:
return mVrAvailable && mVrShellEnabled;
Is it necessary to check mVrAvailable? mVrShellEnabled runtime flag should only
be available when VrShell is getting compiled (either by WebVR or VrShell)
right?

[amp, 2016-11-01 18:45:31]

https://codereview.chromium.org/2470583002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java
(right):

https://codereview.chromium.org/2470583002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:40:
private boolean mVrShellEnabled = false;
On 2016/11/01 01:20:59, mthiesse wrote:
> Clank style is to leave booleans default initialized when false.

Done.  (Well, converted to a Boolean which I let default to null, but close
enough).

https://codereview.chromium.org/2470583002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:114:
mVrShellEnabled = ChromeFeatureList.isEnabled(ChromeFeatureList.VR_SHELL);
On 2016/11/01 01:20:59, mthiesse wrote:
> You should make mVrShellEnabled a tri-state Boolean, defaulting to null, then
> all accesses of it should go through isVrShellEnabled(), which initializes it
if
> it's null and jni is ready (LibraryLoader.isInitialized()).

Thanks!  I couldn't figure out what the best solution was.  I've updated to use
a tri-state Boolean as suggested and changed to access it through the method
here.

https://codereview.chromium.org/2470583002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:395:
return mVrAvailable && mVrShellEnabled;
On 2016/11/01 01:20:59, mthiesse wrote:
> Is it necessary to check mVrAvailable? mVrShellEnabled runtime flag should
only
> be available when VrShell is getting compiled (either by WebVR or VrShell)
> right?

The feature flag itself isn't dependent on the build flag so in theory it might
be possible.  Should we update the feature flag to only be available when the
build flag is present?  I don't think the current command line switches do that.

In practice even if someone manually sets the flag on a build with out vr in it
nothing will happen, so I've removed the available check.

[mthiesse, 2016-11-01 18:51:20]

lgtm

https://codereview.chromium.org/2470583002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java
(right):

https://codereview.chromium.org/2470583002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:395:
return mVrAvailable && mVrShellEnabled;
On 2016/11/01 18:45:31, amp wrote:
> On 2016/11/01 01:20:59, mthiesse wrote:
> > Is it necessary to check mVrAvailable? mVrShellEnabled runtime flag should
> only
> > be available when VrShell is getting compiled (either by WebVR or VrShell)
> > right?
> 
> The feature flag itself isn't dependent on the build flag so in theory it
might
> be possible.  Should we update the feature flag to only be available when the
> build flag is present?  I don't think the current command line switches do
that.
> 
> In practice even if someone manually sets the flag on a build with out vr in
it
> nothing will happen, so I've removed the available check.

Yeah, we should probably update the feature flag to only be available when the
build flag is present... otherwise there's no point in showing it.

https://codereview.chromium.org/2470583002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java
(right):

https://codereview.chromium.org/2470583002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:391:
if (!LibraryLoader.isInitialized()) {
nit: move this check inside of (mVrShellEnabled == null)?

[amp, 2016-11-01 22:19:26]

amp@chromium.org changed reviewers:
+ jochen@chromium.org, rkaplow@chromium.org

[amp, 2016-11-01 22:19:27]

jochen@: Can you review the content/feature list parts and the histograms
change?

rkaplow@: Finch ambassador

Launch bug for this is crbug/641038.

https://codereview.chromium.org/2470583002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java
(right):

https://codereview.chromium.org/2470583002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:391:
if (!LibraryLoader.isInitialized()) {
On 2016/11/01 18:51:20, mthiesse wrote:
> nit: move this check inside of (mVrShellEnabled == null)?

Done.

[amp, 2016-11-01 22:41:02]

https://codereview.chromium.org/2470583002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java
(right):

https://codereview.chromium.org/2470583002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:395:
return mVrAvailable && mVrShellEnabled;
On 2016/11/01 18:51:20, mthiesse wrote:
> On 2016/11/01 18:45:31, amp wrote:
> > On 2016/11/01 01:20:59, mthiesse wrote:
> > > Is it necessary to check mVrAvailable? mVrShellEnabled runtime flag should
> > only
> > > be available when VrShell is getting compiled (either by WebVR or VrShell)
> > > right?
> > 
> > The feature flag itself isn't dependent on the build flag so in theory it
> might
> > be possible.  Should we update the feature flag to only be available when
the
> > build flag is present?  I don't think the current command line switches do
> that.
> > 
> > In practice even if someone manually sets the flag on a build with out vr in
> it
> > nothing will happen, so I've removed the available check.
> 
> Yeah, we should probably update the feature flag to only be available when the
> build flag is present... otherwise there's no point in showing it.

Almost missed this one.  The flag itself will still be available (via command
line etc), but I wrapped it in a build flag check in about://flags so it won't
show up there unless it's in the build.

[jochen (gone - plz use gerrit), 2016-11-02 11:00:56]

I can only approve UseCounter additions to histograms.xml, please ask a metrics
owner to review this part

[rkaplow, 2016-11-02 15:45:51]

https://codereview.chromium.org/2470583002/diff/80001/content/child/runtime_f...
File content/child/runtime_features.cc (right):

https://codereview.chromium.org/2470583002/diff/80001/content/child/runtime_f...
content/child/runtime_features.cc:317: if
(base::FeatureList::IsEnabled(features::kVrShell)){
ws after )

https://codereview.chromium.org/2470583002/diff/80001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/2470583002/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:90173: <int value="-488779992"
label="blink-settings"/>
you need the entries associated with enabling/disabling the new flag. Running
AboutFlagsHistogramTest should show what is needed, but is basically
Feature:enabled Feature:disabled

https://codereview.chromium.org/2470583002/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:90175: -  <int value="-475049740"
label="disable-vr-shell"/>
you shouldn't delete entries from a histogram enum, we'd still like to track old
users with these flags.

[amp, 2016-11-02 18:31:24]

On 2016/11/02 11:00:56, jochen wrote:
> I can only approve UseCounter additions to histograms.xml, please ask a
metrics
> owner to review this part

Oops, sorry about that.  Please only review the content and features.  Thanks!

[amp, 2016-11-02 18:32:03]

https://codereview.chromium.org/2470583002/diff/80001/content/child/runtime_f...
File content/child/runtime_features.cc (right):

https://codereview.chromium.org/2470583002/diff/80001/content/child/runtime_f...
content/child/runtime_features.cc:317: if
(base::FeatureList::IsEnabled(features::kVrShell)){
On 2016/11/02 15:45:50, rkaplow wrote:
> ws after )

Done.

https://codereview.chromium.org/2470583002/diff/80001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/2470583002/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:90173: <int value="-488779992"
label="blink-settings"/>
On 2016/11/02 15:45:50, rkaplow wrote:
> you need the entries associated with enabling/disabling the new flag. Running
> AboutFlagsHistogramTest should show what is needed, but is basically
> Feature:enabled Feature:disabled

Done.

https://codereview.chromium.org/2470583002/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:90175: -  <int value="-475049740"
label="disable-vr-shell"/>
On 2016/11/02 15:45:50, rkaplow wrote:
> you shouldn't delete entries from a histogram enum, we'd still like to track
old
> users with these flags.
> 

Done.  Although I don't think anyone ever used the existing flag as it was never
wired up to anything.

[rkaplow, 2016-11-02 23:05:21]

lgtm

[jochen (gone - plz use gerrit), 2016-11-03 11:54:03]

lgtm

[amp, 2016-11-03 17:44:01]

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

[commit-bot: I haz the power, 2016-11-03 17:44:36]

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

[commit-bot: I haz the power, 2016-11-03 17:53:56]

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

[commit-bot: I haz the power, 2016-11-03 17:53:57]

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

[amp, 2016-11-03 18:39:01]

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

[commit-bot: I haz the power, 2016-11-03 18:39:33]

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-11-03 19:45:59]

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

[commit-bot: I haz the power, 2016-11-03 19:46:00]

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

[amp, 2016-11-03 22:13:08]

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

[commit-bot: I haz the power, 2016-11-03 22:13:42]

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-11-04 03:08:44]

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

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

Dry run: This issue passed the CQ dry run.

[amp, 2016-11-04 15:21:40]

The CQ bit was checked by amp@chromium.org

[amp, 2016-11-04 15:21:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
mthiesse@chromium.org, jochen@chromium.org, rkaplow@chromium.org
Link to the patchset: https://codereview.chromium.org/2470583002/#ps160001
(title: "rebase")

[commit-bot: I haz the power, 2016-11-04 15:21:57]

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-11-04 15:28:33]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-11-04 15:32:14]

Description was changed from

==========
Add VrShell feature flag and wire it up to VrShell delegate.

This is in preparation for a finch experiment and also updates existing VrShell
command line switch, and about:flags language.

When the feature is enabled it will enable WebVR and GamepadExtension features.

BUG=655199
==========

to

==========
Add VrShell feature flag and wire it up to VrShell delegate.

This is in preparation for a finch experiment and also updates existing VrShell
command line switch, and about:flags language.

When the feature is enabled it will enable WebVR and GamepadExtension features.

BUG=655199

Committed: https://crrev.com/136c3e8264f81f9e41f71982bcbe8cffa080d90c
Cr-Commit-Position: refs/heads/master@{#429892}
==========

[commit-bot: I haz the power, 2016-11-04 15:32:16]

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