Add the ability to configure feedback prompt frequency

Add the ability to configure feedback prompt frequency

BUG=722971
Review-Url: https://codereview.chromium.org/2899513002
Cr-Commit-Position: refs/heads/master@{#474043}
Committed: https://chromium.googlesource.com/chromium/src/+/d2a2b5263102748b3884a1c19b1faaed9e8ae58a

[ymalik, 2017-05-19 23:05:43]

ymalik@chromium.org changed reviewers:
+ amp@chromium.org, mthiesse@chromium.org

[ymalik, 2017-05-19 23:05:43]

@amp, PTAL :)

[amp, 2017-05-20 00:12:05]

lgtm.  You're going to want rkaplow@ to review this as well (he's our finch
ambassador and has been reviewing our other finch experiment implementations and
config).

https://codereview.chromium.org/2899513002/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/2899513002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:414:
mFeedbackFrequency = VrFeedbackStatus.getFeedbackFrequency();
Have you verified this works?  I think native libraries need to be loaded before
you can check a feature (or get params from them).

I'm not sure they will be ready when VrShellDelegate is being constructed.

But if it worked for you then it is probably fine.

[amp, 2017-05-20 00:13:09]

https://codereview.chromium.org/2899513002/diff/1/chrome/browser/android/chro...
File chrome/browser/android/chrome_feature_list.cc (right):

https://codereview.chromium.org/2899513002/diff/1/chrome/browser/android/chro...
chrome/browser/android/chrome_feature_list.cc:228:
base::FEATURE_DISABLED_BY_DEFAULT};
Don't we want this to be default enabled and only use the experiment for a kill
switch and paremeterization?

[mthiesse, 2017-05-20 01:52:45]

lgtm % amps comment about default being resolved.

https://codereview.chromium.org/2899513002/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/2899513002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:414:
mFeedbackFrequency = VrFeedbackStatus.getFeedbackFrequency();
On 2017/05/20 00:12:04, amp wrote:
> Have you verified this works?  I think native libraries need to be loaded
before
> you can check a feature (or get params from them).
> 
> I'm not sure they will be ready when VrShellDelegate is being constructed.
> 
> But if it worked for you then it is probably fine.

VrShellDelegate can only be constructed after the native libraries are loaded
(you'll notice the nativeInit() call right above this one)

[amp, 2017-05-22 21:12:04]

Thanks for updating the default.  lgtm

[ymalik, 2017-05-22 22:35:01]

https://codereview.chromium.org/2899513002/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/2899513002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:414:
mFeedbackFrequency = VrFeedbackStatus.getFeedbackFrequency();
On 2017/05/20 01:52:44, mthiesse wrote:
> On 2017/05/20 00:12:04, amp wrote:
> > Have you verified this works?  I think native libraries need to be loaded
> before
> > you can check a feature (or get params from them).
> > 
> > I'm not sure they will be ready when VrShellDelegate is being constructed.
> > 
> > But if it worked for you then it is probably fine.
> 
> VrShellDelegate can only be constructed after the native libraries are loaded
> (you'll notice the nativeInit() call right above this one)

+1,  native libraries should be loaded at this point.

https://codereview.chromium.org/2899513002/diff/1/chrome/browser/android/chro...
File chrome/browser/android/chrome_feature_list.cc (right):

https://codereview.chromium.org/2899513002/diff/1/chrome/browser/android/chro...
chrome/browser/android/chrome_feature_list.cc:228:
base::FEATURE_DISABLED_BY_DEFAULT};
On 2017/05/20 00:13:09, amp wrote:
> Don't we want this to be default enabled and only use the experiment for a
kill
> switch and paremeterization?

Yes, you're right. Done.

[ymalik, 2017-05-22 22:46:32]

The CQ bit was checked by ymalik@chromium.org

[ymalik, 2017-05-22 22:46:33]

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

[commit-bot: I haz the power, 2017-05-22 22:47:06]

CQ is trying da patch.

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

[ymalik, 2017-05-22 22:56:56]

The CQ bit was unchecked by ymalik@chromium.org

[ymalik, 2017-05-23 18:03:12]

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

[commit-bot: I haz the power, 2017-05-23 18:04:04]

Dry run: CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-05-23 19:01:14]

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

[commit-bot: I haz the power, 2017-05-23 19:01:15]

Dry run: This issue passed the CQ dry run.

[ymalik, 2017-05-23 20:42:52]

The CQ bit was checked by ymalik@chromium.org

[ymalik, 2017-05-23 20:42:52]

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

[commit-bot: I haz the power, 2017-05-23 20:43:26]

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-05-23 20:49:32]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1495572172117740,
"parent_rev": "711b5cd6696a3ee46c38e2032f0014e63a0b0f0b", "commit_rev":
"d2a2b5263102748b3884a1c19b1faaed9e8ae58a"}

[commit-bot: I haz the power, 2017-05-23 20:49:44]

Description was changed from

==========
Add the ability to configure feedback prompt frequency

BUG=722971
==========

to

==========
Add the ability to configure feedback prompt frequency

BUG=722971

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

[commit-bot: I haz the power, 2017-05-23 20:49:45]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/d2a2b5263102748b3884a1c19b1f...