Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2657923002:
Add a Kill switch for WebVR cardboard support through finch. (Closed)
DescriptionAdd a Kill switch for WebVR cardboard support through finch.
BUG=685267
Review-Url: https://codereview.chromium.org/2657923002
Cr-Commit-Position: refs/heads/master@{#447354}
Committed: https://chromium.googlesource.com/chromium/src/+/4a636520c5c1a351628ce7e469cc3603fcb939e8
Patch Set 1 #Patch Set 2 : Use build version check #Patch Set 3 : (IN PROGRESS) Switch to variations and rewire to support that on android. Also removes content as … #Patch Set 4 : move check to only be for non-daydream devices #Patch Set 5 : rebase #Patch Set 6 : fix library check #
Total comments: 3
Patch Set 7 : Fix comment. #Patch Set 8 : rebase #Patch Set 9 : Fix javadoc #
Total comments: 4
Patch Set 10 : Switch to calling base code from feature list directly. #
Total comments: 4
Patch Set 11 : Formating #
Messages
Total messages: 48 (32 generated)
amp@chromium.org changed reviewers: + bshe@chromium.org, creis@chromium.org, rkaplow@chromium.org
bshe please review VR code creis please review content/ rkaplow as finch ambassador. This won't be used as an experiement directly, only as a kill switch if there are issues. The chrome_switch change is a clean up from a previous removal of old vr switches (I can remove that file if it is preferred to clean it up separately). I'm not sure who is the owner for the android feature list parts.
content/ LGTM
The CQ bit was checked by amp@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by amp@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by amp@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by amp@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
amp@chromium.org changed reviewers: + tedchoc@chromium.org - creis@chromium.org
PTAL, switched to using finch variations which required adding in some extra wiring and cleaned up references so content is no longer include (-creis for that reason). +tedchoc please review android changes I have verified the variation params will work correctly (though not yet on a KLM device as VrCore isn't working for them at the moment).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks. lgtm https://codereview.chromium.org/2657923002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2657923002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:135: // still be using cardboard. hmm, do we need to check if current headset is DD headset here? We only want to disable Cardboard support if VrCore somehow doesn't work, right? I would expect if VrCore works for DD headset, it should work for Cardboard headset too.
https://codereview.chromium.org/2657923002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2657923002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:135: // still be using cardboard. On 2017/01/30 01:41:29, bshe wrote: > hmm, do we need to check if current headset is DD headset here? We only want to > disable Cardboard support if VrCore somehow doesn't work, right? I would expect > if VrCore works for DD headset, it should work for Cardboard headset too. I'm not sure. The assumption I was using is that Daydream ready devices are much more thoroughly vetted (for both daydream and cardboard) and that any changes to VrCore which could negatively impact those devices would be rolled back or stopped before they go out, so we don't need a kill switch in those scenarios. It's only the older Android versions that might pick up a change which didn't get caught in testing/deployment that we would need the quick kill switch. I'm happy to change it if my assumptions are wrong though.
https://codereview.chromium.org/2657923002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2657923002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:135: // still be using cardboard. On 2017/01/30 17:59:06, amp wrote: > On 2017/01/30 01:41:29, bshe wrote: > > hmm, do we need to check if current headset is DD headset here? We only want > to > > disable Cardboard support if VrCore somehow doesn't work, right? I would > expect > > if VrCore works for DD headset, it should work for Cardboard headset too. > > I'm not sure. The assumption I was using is that Daydream ready devices are > much more thoroughly vetted (for both daydream and cardboard) and that any > changes to VrCore which could negatively impact those devices would be rolled > back or stopped before they go out, so we don't need a kill switch in those > scenarios. > > It's only the older Android versions that might pick up a change which didn't > get caught in testing/deployment that we would need the quick kill switch. > > I'm happy to change it if my assumptions are wrong though. I was referring to the comment that you have. Your comment seem to suggest that we should check if headset is DD headset. But I think checking DaydreamReadyDevice is the correct approach for reason that you said in your reply. So removing the comment sounds good.
On 2017/01/30 18:19:20, bshe wrote: > https://codereview.chromium.org/2657923002/diff/100001/chrome/android/java/sr... > File > chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java > (right): > > https://codereview.chromium.org/2657923002/diff/100001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:135: > // still be using cardboard. > On 2017/01/30 17:59:06, amp wrote: > > On 2017/01/30 01:41:29, bshe wrote: > > > hmm, do we need to check if current headset is DD headset here? We only want > > to > > > disable Cardboard support if VrCore somehow doesn't work, right? I would > > expect > > > if VrCore works for DD headset, it should work for Cardboard headset too. > > > > I'm not sure. The assumption I was using is that Daydream ready devices are > > much more thoroughly vetted (for both daydream and cardboard) and that any > > changes to VrCore which could negatively impact those devices would be rolled > > back or stopped before they go out, so we don't need a kill switch in those > > scenarios. > > > > It's only the older Android versions that might pick up a change which didn't > > get caught in testing/deployment that we would need the quick kill switch. > > > > I'm happy to change it if my assumptions are wrong though. > > I was referring to the comment that you have. Your comment seem to suggest that > we > should check if headset is DD headset. But I think checking DaydreamReadyDevice > is > the correct approach for reason that you said in your reply. So removing the > comment > sounds good. Ah good point. I went back and forth during the implementation and didn't update the comment. Updating now. Thanks!
The CQ bit was checked by amp@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by amp@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2657923002/diff/160001/components/variations/... File components/variations/android/variations_associated_data_android.cc (right): https://codereview.chromium.org/2657923002/diff/160001/components/variations/... components/variations/android/variations_associated_data_android.cc:37: this recently got deprecated to the base:: version, you could switch if you want: https://codesearch.chromium.org/chromium/src/components/variations/variations...
https://codereview.chromium.org/2657923002/diff/160001/chrome/browser/android... File chrome/browser/android/chrome_feature_list.cc (right): https://codereview.chromium.org/2657923002/diff/160001/chrome/browser/android... chrome/browser/android/chrome_feature_list.cc:164: static jlong GetFeature(JNIEnv* env, as a general practice, we really, really do not want to pass raw pointers back to java (too easy to get wrong, incorrectly modified, etc.., even harder to rationalize lifetime than raw native pointers IMO) I think if we wanted to make this more java friendly, we'd create a Feature.java class that wraps the raw pointer and maybe has the necessary apis to pull params out of it. But....that might be overkill for right now. I'd suggest that instead of making the changes to VariationsAssociatedData that you just add the method here that calls GetFieldTrialParamByFeatureAsInt give a field trial name and param name. I'm basically happy with any approach that doesn't pass a pointer.
https://codereview.chromium.org/2657923002/diff/160001/chrome/browser/android... File chrome/browser/android/chrome_feature_list.cc (right): https://codereview.chromium.org/2657923002/diff/160001/chrome/browser/android... chrome/browser/android/chrome_feature_list.cc:164: static jlong GetFeature(JNIEnv* env, On 2017/01/31 18:24:30, Ted C wrote: > as a general practice, we really, really do not want to pass raw pointers back > to java (too easy to get wrong, incorrectly modified, etc.., even harder to > rationalize lifetime than raw native pointers IMO) > > I think if we wanted to make this more java friendly, we'd create a Feature.java > class that wraps the raw pointer and maybe has the necessary apis to pull params > out of it. > > But....that might be overkill for right now. I'd suggest that instead of making > the changes to VariationsAssociatedData that you just add the method here that > calls GetFieldTrialParamByFeatureAsInt give a field trial name and param name. > > I'm basically happy with any approach that doesn't pass a pointer. Duly noted. I was trying to work out how to keep variation related code in the /components/variations while still being able to get a reference to the Feature and passing a pointer was all I could come up with. I'm fine with adding the call directly here in chrome_feature_list. It looks like a lot of the methods in /components/variations files are being deprecated in favor of /base/metrics anyway. @rkaplow, any concerns with this path? https://codereview.chromium.org/2657923002/diff/160001/components/variations/... File components/variations/android/variations_associated_data_android.cc (right): https://codereview.chromium.org/2657923002/diff/160001/components/variations/... components/variations/android/variations_associated_data_android.cc:37: On 2017/01/31 18:17:44, rkaplow wrote: > this recently got deprecated to the base:: version, you could switch if you > want: > https://codesearch.chromium.org/chromium/src/components/variations/variations... Acknowledged. I'll update to the non-deprecated call, but per tedchoc@ comment I'll use it from the feature_list file instead of changing anything in the variations component (unless you have a concern about that).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by amp@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL Removed changes to /components/varations/ and switched to calling from chrome_feature_list.cc directly so no pointer casting is required.
lgtm thanks! https://codereview.chromium.org/2657923002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeFeatureList.java (right): https://codereview.chromium.org/2657923002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeFeatureList.java:61: * specified parameter does not exist. align with "The parameter above" https://codereview.chromium.org/2657923002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2657923002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:140: ChromeFeatureList.WEBVR_CARDBOARD_SUPPORT, +4 indent for these next few lines
Thanks for all the reviews! https://codereview.chromium.org/2657923002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeFeatureList.java (right): https://codereview.chromium.org/2657923002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeFeatureList.java:61: * specified parameter does not exist. On 2017/01/31 21:53:43, Ted C wrote: > align with "The parameter above" Done. https://codereview.chromium.org/2657923002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2657923002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:140: ChromeFeatureList.WEBVR_CARDBOARD_SUPPORT, On 2017/01/31 21:53:43, Ted C wrote: > +4 indent for these next few lines Done.
The CQ bit was checked by amp@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org, bshe@chromium.org, rkaplow@chromium.org, tedchoc@chromium.org Link to the patchset: https://codereview.chromium.org/2657923002/#ps200001 (title: "Formating")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 200001, "attempt_start_ts": 1485900052968260,
"parent_rev": "f446721d35e784512fe9913e14842a8876309ea4", "commit_rev":
"4a636520c5c1a351628ce7e469cc3603fcb939e8"}
Message was sent while issue was closed.
Description was changed from ========== Add a Kill switch for WebVR cardboard support through finch. BUG=685267 ========== to ========== Add a Kill switch for WebVR cardboard support through finch. BUG=685267 Review-Url: https://codereview.chromium.org/2657923002 Cr-Commit-Position: refs/heads/master@{#447354} Committed: https://chromium.googlesource.com/chromium/src/+/4a636520c5c1a351628ce7e469cc... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as https://chromium.googlesource.com/chromium/src/+/4a636520c5c1a351628ce7e469cc... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
