Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2699163003:
Use VrCoreCompatibility check for whether to show an infobar instead of Android package manager. (Closed)
DescriptionUse VrCoreCompatibility check for whether to show an infobar instead of Android package manager.
BUG=670166
Review-Url: https://codereview.chromium.org/2699163003
Cr-Commit-Position: refs/heads/master@{#452503}
Committed: https://chromium.googlesource.com/chromium/src/+/95ffecbdb167104df9c8c8f86e1826cb17122078
Patch Set 1 #
Total comments: 16
Patch Set 2 : address comments #Patch Set 3 : Check for OS support before vr core state #
Messages
Total messages: 17 (8 generated)
amp@chromium.org changed reviewers: + bshe@chromium.org
https://codereview.chromium.org/2699163003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrCoreVersionChecker.java (right): https://codereview.chromium.org/2699163003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrCoreVersionChecker.java:19: public static final int VR_CORE_READY = 2; optional nit: VR_CORE prefix is probably not needed? https://codereview.chromium.org/2699163003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrCoreVersionChecker.java:26: * Test if VrCore version installed is compatible with Chromium. Update this comment https://codereview.chromium.org/2699163003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrCoreVersionChecker.java:28: int isVrCoreCompatible(); It probably make sense to rename this function too. Something like getVrCoreCompatibility? https://codereview.chromium.org/2699163003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrCoreVersionCheckerImpl.java (right): https://codereview.chromium.org/2699163003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrCoreVersionCheckerImpl.java:29: Log.i(TAG, "Unable to find a compatible VrCore."); This is probably not needed in production? https://codereview.chromium.org/2699163003/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/2699163003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:560: Build.VERSION_CODES.KITKAT)) { it might be better to move this check outside of VR_CORE_NOT_AVAILABLE check. It is possible that we want to kill support on platforms that was supported. In that case, we don't want user to upgrade VrCore even they have VrCore installed. Also, how long can we keep this field trail? I would imagine that we want to keep it for a long time since we are now depending on it to decide if certain UI should show. If we can't use the field trail for this use case, we might just want to hard code it for now. https://codereview.chromium.org/2699163003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:563: // Supported, but not installed. Ask user to install instead of upgrade. nit: one space between period and "Ask". https://codereview.chromium.org/2699163003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:570: Log.w(TAG, "Unknown VrCore compatibility: " + vrCoreCompatibility); probably worth a Log.e?
https://codereview.chromium.org/2699163003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrCoreVersionChecker.java (right): https://codereview.chromium.org/2699163003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrCoreVersionChecker.java:19: public static final int VR_CORE_READY = 2; On 2017/02/21 15:53:08, bshe wrote: > optional nit: VR_CORE prefix is probably not needed? Done. https://codereview.chromium.org/2699163003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrCoreVersionChecker.java:26: * Test if VrCore version installed is compatible with Chromium. On 2017/02/21 15:53:08, bshe wrote: > Update this comment Done. https://codereview.chromium.org/2699163003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrCoreVersionChecker.java:28: int isVrCoreCompatible(); On 2017/02/21 15:53:08, bshe wrote: > It probably make sense to rename this function too. Something like > getVrCoreCompatibility? Done. https://codereview.chromium.org/2699163003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrCoreVersionCheckerImpl.java (right): https://codereview.chromium.org/2699163003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrCoreVersionCheckerImpl.java:29: Log.i(TAG, "Unable to find a compatible VrCore."); On 2017/02/21 15:53:08, bshe wrote: > This is probably not needed in production? Done. I was trying to maintain parity with what would have been logged before, but I agree it's not very useful. Removed. https://codereview.chromium.org/2699163003/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/2699163003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:560: Build.VERSION_CODES.KITKAT)) { On 2017/02/21 15:53:09, bshe wrote: > it might be better to move this check outside of VR_CORE_NOT_AVAILABLE check. It > is possible that we want to kill support on platforms that was supported. In > that case, we don't want user to upgrade VrCore even they have VrCore installed. > I think we would have to handle that specifically if that case arrived. > Also, how long can we keep this field trail? I would imagine that we want to > keep it for a long time since we are now depending on it to decide if certain UI > should show. If we can't use the field trail for this use case, we might just > want to hard code it for now. I originally had it hard coded to KITKAT, but then thought that it would match exactly whatever the current experiment is for support. I'm not sure there is a limit on how long a field trial can go. As long as we have to support multiple OS versions with potentially different capabilities I think we can maintain this field trial. Leaving as is for now, but ping me to discuss further if you think we should just hard code it. https://codereview.chromium.org/2699163003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:563: // Supported, but not installed. Ask user to install instead of upgrade. On 2017/02/21 15:53:09, bshe wrote: > nit: one space between period and "Ask". Done. https://codereview.chromium.org/2699163003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:570: Log.w(TAG, "Unknown VrCore compatibility: " + vrCoreCompatibility); On 2017/02/21 15:53:09, bshe wrote: > probably worth a Log.e? Done.
https://codereview.chromium.org/2699163003/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/2699163003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:560: Build.VERSION_CODES.KITKAT)) { On 2017/02/21 19:30:18, amp wrote: > On 2017/02/21 15:53:09, bshe wrote: > > it might be better to move this check outside of VR_CORE_NOT_AVAILABLE check. > It > > is possible that we want to kill support on platforms that was supported. In > > that case, we don't want user to upgrade VrCore even they have VrCore > installed. > > > I think we would have to handle that specifically if that case arrived. I am not sure what do you mean by specifically? If you move the code out of this if block. This should be all we need, right? Or is there something else that you are thinking of? > > > Also, how long can we keep this field trail? I would imagine that we want to > > keep it for a long time since we are now depending on it to decide if certain > UI > > should show. If we can't use the field trail for this use case, we might just > > want to hard code it for now. > > I originally had it hard coded to KITKAT, but then thought that it would match > exactly whatever the current experiment is for support. > > I'm not sure there is a limit on how long a field trial can go. As long as we > have to support multiple OS versions with potentially different capabilities I > think we can maintain this field trial. > > Leaving as is for now, but ping me to discuss further if you think we should > just hard code it.
https://codereview.chromium.org/2699163003/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/2699163003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:560: Build.VERSION_CODES.KITKAT)) { On 2017/02/21 22:03:55, bshe wrote: > On 2017/02/21 19:30:18, amp wrote: > > On 2017/02/21 15:53:09, bshe wrote: > > > it might be better to move this check outside of VR_CORE_NOT_AVAILABLE > check. > > It > > > is possible that we want to kill support on platforms that was supported. In > > > that case, we don't want user to upgrade VrCore even they have VrCore > > installed. > > > > > I think we would have to handle that specifically if that case arrived. > I am not sure what do you mean by specifically? If you move the code out of this > if block. This should be all we need, right? Or is there something else that you > are thinking of? > > > > > Also, how long can we keep this field trail? I would imagine that we want to > > > keep it for a long time since we are now depending on it to decide if > certain > > UI > > > should show. If we can't use the field trail for this use case, we might > just > > > want to hard code it for now. > > > > I originally had it hard coded to KITKAT, but then thought that it would match > > exactly whatever the current experiment is for support. > > > > I'm not sure there is a limit on how long a field trial can go. As long as we > > have to support multiple OS versions with potentially different capabilities I > > think we can maintain this field trial. > > > > Leaving as is for now, but ping me to discuss further if you think we should > > just hard code it. > Done. Sorry, I thought you were referring to a scenario where we were blacklisting specific platforms and confused myself. Reading the original comment again I think I understand now. :) We don't want to show an infobar to users who are on a platform that is no longer supported (even if they currently have vr core that is just out of date).
PTAL
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: No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
On 2017/02/22 17:42:43, amp wrote: > PTAL lgtm. thanks!
The CQ bit was checked by amp@chromium.org
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": 40001, "attempt_start_ts": 1487863089344660,
"parent_rev": "81b9da503821e8480bcaa8c3cb91414aa09d7f33", "commit_rev":
"95ffecbdb167104df9c8c8f86e1826cb17122078"}
Message was sent while issue was closed.
Description was changed from ========== Use VrCoreCompatibility check for whether to show an infobar instead of Android package manager. BUG=670166 ========== to ========== Use VrCoreCompatibility check for whether to show an infobar instead of Android package manager. BUG=670166 Review-Url: https://codereview.chromium.org/2699163003 Cr-Commit-Position: refs/heads/master@{#452503} Committed: https://chromium.googlesource.com/chromium/src/+/95ffecbdb167104df9c8c8f86e18... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/95ffecbdb167104df9c8c8f86e18... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
