Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2556093002:
Enable cardboard support for non-Daydream devices (Closed)
DescriptionEnable cardboard support for non-Daydream devices
If VrCore exists on non-Daydream devices, WebVR Cardboard support should be enabled.
BUG=671370
Committed: https://crrev.com/a22648aca08ffd0812837e124fe4074b4572ed5e
Cr-Commit-Position: refs/heads/master@{#437962}
Patch Set 1 #Patch Set 2 : Minor nits #
Total comments: 6
Patch Set 3 : rebase and reviews #Patch Set 4 : nit #
Total comments: 10
Patch Set 5 : reviews #
Total comments: 10
Patch Set 6 : comments #Messages
Total messages: 46 (27 generated)
The CQ bit was checked by bshe@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...
bshe@chromium.org changed reviewers: + mthiesse@chromium.org
Hi Micheal. Can you pleas take a look?
On 2016/12/07 16:40:53, bshe wrote: > Hi Micheal. > Can you pleas take a look? LGTM, FWIW.
On 2016/12/07 16:40:53, bshe wrote: > Hi Micheal. > Can you pleas take a look? LGTM, FWIW.
On 2016/12/07 16:40:53, bshe wrote: > Hi Micheal. > Can you pleas take a look? LGTM, FWIW.
On 2016/12/07 16:51:19, bajones wrote: > On 2016/12/07 16:40:53, bshe wrote: > > Hi Micheal. > > Can you pleas take a look? > > LGTM, FWIW. Derp, please excuse the triple comment. I blame GBus WiFi.
On 2016/12/07 16:52:20, bajones wrote: > On 2016/12/07 16:51:19, bajones wrote: > > On 2016/12/07 16:40:53, bshe wrote: > > > Hi Micheal. > > > Can you pleas take a look? > > > > LGTM, FWIW. > > Derp, please excuse the triple comment. I blame GBus WiFi. I take it as you really want this CL to submit :)
https://codereview.chromium.org/2556093002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2556093002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:101: mTabObserver = new EmptyTabObserver() { I think we still need this for cardboard only support. We still need to exit VR Shell if the page is unsupported. https://codereview.chromium.org/2556093002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:159: if (mCardboardSupportOnly) return; assert that we don't get here on cardboard, rather than returning. Something very wrong happened if we get here. https://codereview.chromium.org/2556093002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:385: exitVRIfNecessary(mCardboardSupportOnly /* returnTo2D */); This should still always be false. returnTo2D is whether or not we're returning to 2D within chrome (and are therefore still the foreground app). When in onPause we're returning to 2D with another app in the foreground, so returnTo2D should be false. Feel free to find a better name for these parameters if they're confusing.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by bshe@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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
Patchset #5 (id:80001) has been deleted
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
Patchset #3 (id:100001) has been deleted
The CQ bit was checked by bshe@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...
https://codereview.chromium.org/2556093002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2556093002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:101: mTabObserver = new EmptyTabObserver() { On 2016/12/07 17:00:31, mthiesse wrote: > I think we still need this for cardboard only support. We still need to exit VR > Shell if the page is unsupported. Done. https://codereview.chromium.org/2556093002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:159: if (mCardboardSupportOnly) return; On 2016/12/07 17:00:31, mthiesse wrote: > assert that we don't get here on cardboard, rather than returning. Something > very wrong happened if we get here. Done. https://codereview.chromium.org/2556093002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:385: exitVRIfNecessary(mCardboardSupportOnly /* returnTo2D */); On 2016/12/07 17:00:31, mthiesse wrote: > This should still always be false. returnTo2D is whether or not we're returning > to 2D within chrome (and are therefore still the foreground app). When in > onPause we're returning to 2D with another app in the foreground, so returnTo2D > should be false. > > Feel free to find a better name for these parameters if they're confusing. Addressed in https://codereview.chromium.org/2560843003/
https://codereview.chromium.org/2556093002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2556093002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:95: mVrAvailable = createVrDaydreamApi() && mVrAvailable; &= ? did you mean to set mVrAvailable twice? https://codereview.chromium.org/2556093002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:163: assert mVrAvailable; Actually, on second thought, maybe we don't want to assert here... Could this lead to other apps faking a VR intent to crash chrome? I don't know that we have any guarantee that VR intents come from daydream. Maybe for now, return early instead of asserting, and file a bug to verify that VR intents are only coming from daydream? If they're correctly only ever calling us back with the pendingIntents we give them, we should be able to verify that somehow. https://codereview.chromium.org/2556093002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:299: // Transition screen is not available for Cardboard only(non-Daydream) devices. nit: s/only(non/only (non/ https://codereview.chromium.org/2556093002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:300: shutdownVR(false, false); Maybe file a bug for a cardboard DOFF screen? In error cases it would still be nice to show some sort of "take your headset off now" screen. https://codereview.chromium.org/2556093002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:576: // Disable VR Shell for Cardboard only(non-Daydream) devices. nit: space
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 bshe@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...
https://codereview.chromium.org/2556093002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2556093002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:95: mVrAvailable = createVrDaydreamApi() && mVrAvailable; On 2016/12/09 19:30:14, mthiesse wrote: > &= ? > > did you mean to set mVrAvailable twice? made it more readable. This should be better once I refactored createVrDaydreamApi. https://codereview.chromium.org/2556093002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:163: assert mVrAvailable; On 2016/12/09 19:30:14, mthiesse wrote: > Actually, on second thought, maybe we don't want to assert here... > > Could this lead to other apps faking a VR intent to crash chrome? I don't know > that we have any guarantee that VR intents come from daydream. > > Maybe for now, return early instead of asserting, and file a bug to verify that > VR intents are only coming from daydream? If they're correctly only ever calling > us back with the pendingIntents we give them, we should be able to verify that > somehow. Good catch. Looks like we don't, not if someone send an intent with DAYDREAM_VR_EXTRA set to true. https://codereview.chromium.org/2556093002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:299: // Transition screen is not available for Cardboard only(non-Daydream) devices. On 2016/12/09 19:30:14, mthiesse wrote: > nit: s/only(non/only (non/ Done. https://codereview.chromium.org/2556093002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:300: shutdownVR(false, false); On 2016/12/09 19:30:14, mthiesse wrote: > Maybe file a bug for a cardboard DOFF screen? In error cases it would still be > nice to show some sort of "take your headset off now" screen. Done. https://codereview.chromium.org/2556093002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:576: // Disable VR Shell for Cardboard only(non-Daydream) devices. On 2016/12/09 19:30:14, mthiesse wrote: > nit: space Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2556093002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2556093002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:96: if (mVrDaydreamApi == null) createVrDaydreamApi(); What if createVrClassesBuilder() failed? https://codereview.chromium.org/2556093002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:577: if (!mVrAvailable || mCardboardSupportOnly || !LibraryLoader.isInitialized()) { I know you didn't add this, but while you're here we should probably assert LibraryLoader.isInitialized() instead of checking it and returning false. We could end up misleading callers into thinking the device doesn't support VrShell, when it just hasn't loaded yet.
bshe@chromium.org changed reviewers: + amp@chromium.org
https://codereview.chromium.org/2556093002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2556093002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:96: if (mVrDaydreamApi == null) createVrDaydreamApi(); On 2016/12/12 19:30:47, mthiesse wrote: > What if createVrClassesBuilder() failed? Good question. In that case, mVrAailable will be false. And if mVrAvailable is false, we wont use mCardboardSupportOnly since its value doesn't make sense. All places that use mCardboardSupportOnly is gated by mVrAvailable. Ideally, we should create a Support level enum (NO_VR, CARDBAROD_ONLY and DAYDREAM) instead of using boolean that depends on other boolean . But since we might still want to merge this back to M56. I didn't refactor the code to make potential merge easier. https://codereview.chromium.org/2556093002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:577: if (!mVrAvailable || mCardboardSupportOnly || !LibraryLoader.isInitialized()) { On 2016/12/12 19:30:47, mthiesse wrote: > I know you didn't add this, but while you're here we should probably assert > LibraryLoader.isInitialized() instead of checking it and returning false. We > could end up misleading callers into thinking the device doesn't support > VrShell, when it just hasn't loaded yet. +amp is there any scenario that we might call this function before native library is ready? If so, we probably want to remove these calls to a place after the native library is initialized. If not, I think we can just remove it.
https://codereview.chromium.org/2556093002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2556093002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:577: if (!mVrAvailable || mCardboardSupportOnly || !LibraryLoader.isInitialized()) { On 2016/12/12 20:55:59, bshe wrote: > On 2016/12/12 19:30:47, mthiesse wrote: > > I know you didn't add this, but while you're here we should probably assert > > LibraryLoader.isInitialized() instead of checking it and returning false. We > > could end up misleading callers into thinking the device doesn't support > > VrShell, when it just hasn't loaded yet. > > +amp is there any scenario that we might call this function before native > library is ready? > If so, we probably want to remove these calls to a place after the native > library is initialized. > If not, I think we can just remove it. I seem to recall hitting a scenario where things failed if the check wasn't in there, but don't remember what it was specifically. It wasn't a rare failure IIRC so maybe try and remove it and if things still work call it good? I agree that it feels odd to say it doesn't support daydream if it just hasn't loaded yet.
lgtm https://codereview.chromium.org/2556093002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2556093002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:96: if (mVrDaydreamApi == null) createVrDaydreamApi(); On 2016/12/12 20:55:59, bshe wrote: > On 2016/12/12 19:30:47, mthiesse wrote: > > What if createVrClassesBuilder() failed? > > Good question. In that case, mVrAailable will be false. And if mVrAvailable is > false, we wont use mCardboardSupportOnly since its value doesn't make sense. All > places that use mCardboardSupportOnly is gated by mVrAvailable. > > Ideally, we should create a Support level enum (NO_VR, CARDBAROD_ONLY and > DAYDREAM) instead of using boolean that depends on other boolean . But since we > might still want to merge this back to M56. I didn't refactor the code to make > potential merge easier. Add a TODO? https://codereview.chromium.org/2556093002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:577: if (!mVrAvailable || mCardboardSupportOnly || !LibraryLoader.isInitialized()) { On 2016/12/12 21:04:40, amp wrote: > On 2016/12/12 20:55:59, bshe wrote: > > On 2016/12/12 19:30:47, mthiesse wrote: > > > I know you didn't add this, but while you're here we should probably assert > > > LibraryLoader.isInitialized() instead of checking it and returning false. We > > > could end up misleading callers into thinking the device doesn't support > > > VrShell, when it just hasn't loaded yet. > > > > +amp is there any scenario that we might call this function before native > > library is ready? > > If so, we probably want to remove these calls to a place after the native > > library is initialized. > > If not, I think we can just remove it. > > I seem to recall hitting a scenario where things failed if the check wasn't in > there, but don't remember what it was specifically. It wasn't a rare failure > IIRC so maybe try and remove it and if things still work call it good? > > I agree that it feels odd to say it doesn't support daydream if it just hasn't > loaded yet. You should rename it to isVrShellEnabledWithNative, to make it clear that native is also expected to be available.
https://codereview.chromium.org/2556093002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2556093002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:577: if (!mVrAvailable || mCardboardSupportOnly || !LibraryLoader.isInitialized()) { On 2016/12/12 21:27:18, mthiesse wrote: > On 2016/12/12 21:04:40, amp wrote: > > On 2016/12/12 20:55:59, bshe wrote: > > > On 2016/12/12 19:30:47, mthiesse wrote: > > > > I know you didn't add this, but while you're here we should probably > assert > > > > LibraryLoader.isInitialized() instead of checking it and returning false. > We > > > > could end up misleading callers into thinking the device doesn't support > > > > VrShell, when it just hasn't loaded yet. > > > > > > +amp is there any scenario that we might call this function before native > > > library is ready? > > > If so, we probably want to remove these calls to a place after the native > > > library is initialized. > > > If not, I think we can just remove it. > > > > I seem to recall hitting a scenario where things failed if the check wasn't in > > there, but don't remember what it was specifically. It wasn't a rare failure > > IIRC so maybe try and remove it and if things still work call it good? > > > > I agree that it feels odd to say it doesn't support daydream if it just hasn't > > loaded yet. > > You should rename it to isVrShellEnabledWithNative, to make it clear that native > is also expected to be available. Also, if you just want to merge this to M56, feel free to make this change in a followup.
https://codereview.chromium.org/2556093002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2556093002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:96: if (mVrDaydreamApi == null) createVrDaydreamApi(); On 2016/12/12 21:27:18, mthiesse wrote: > On 2016/12/12 20:55:59, bshe wrote: > > On 2016/12/12 19:30:47, mthiesse wrote: > > > What if createVrClassesBuilder() failed? > > > > Good question. In that case, mVrAailable will be false. And if mVrAvailable is > > false, we wont use mCardboardSupportOnly since its value doesn't make sense. > All > > places that use mCardboardSupportOnly is gated by mVrAvailable. > > > > Ideally, we should create a Support level enum (NO_VR, CARDBAROD_ONLY and > > DAYDREAM) instead of using boolean that depends on other boolean . But since > we > > might still want to merge this back to M56. I didn't refactor the code to make > > potential merge easier. > > Add a TODO? Done. https://codereview.chromium.org/2556093002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:577: if (!mVrAvailable || mCardboardSupportOnly || !LibraryLoader.isInitialized()) { On 2016/12/12 21:28:14, mthiesse wrote: > On 2016/12/12 21:27:18, mthiesse wrote: > > On 2016/12/12 21:04:40, amp wrote: > > > On 2016/12/12 20:55:59, bshe wrote: > > > > On 2016/12/12 19:30:47, mthiesse wrote: > > > > > I know you didn't add this, but while you're here we should probably > > assert > > > > > LibraryLoader.isInitialized() instead of checking it and returning > false. > > We > > > > > could end up misleading callers into thinking the device doesn't support > > > > > VrShell, when it just hasn't loaded yet. > > > > > > > > +amp is there any scenario that we might call this function before native > > > > library is ready? > > > > If so, we probably want to remove these calls to a place after the native > > > > library is initialized. > > > > If not, I think we can just remove it. > > > > > > I seem to recall hitting a scenario where things failed if the check wasn't > in > > > there, but don't remember what it was specifically. It wasn't a rare > failure > > > IIRC so maybe try and remove it and if things still work call it good? > > > > > > I agree that it feels odd to say it doesn't support daydream if it just > hasn't > > > loaded yet. > > > > You should rename it to isVrShellEnabledWithNative, to make it clear that > native > > is also expected to be available. > > Also, if you just want to merge this to M56, feel free to make this change in a > followup. yeah. rename would make it even harder to merge. I think the reason we need native ready here is because ChromeFeatureList.isEnabled needs native. Otherwise, it will crash. I am hesitant to change the name since I think we really shouldn't call the function before native ready. So keep it as it is for the sake of merge. Follow up CL will test if we can remove it.
The CQ bit was checked by bshe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bajones@chromium.org, mthiesse@chromium.org Link to the patchset: https://codereview.chromium.org/2556093002/#ps180001 (title: "comments")
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": 180001, "attempt_start_ts": 1481581005136110,
"parent_rev": "62e24726ce13701d11e1709ac5336a47df6b4f23", "commit_rev":
"754426b698e78c8e456b10f99e64a98087b8ead0"}
Message was sent while issue was closed.
Description was changed from ========== Enable cardboard support for non-Daydream devices If VrCore exists on non-Daydream devices, WebVR Cardboard support should be enabled. BUG=671370 ========== to ========== Enable cardboard support for non-Daydream devices If VrCore exists on non-Daydream devices, WebVR Cardboard support should be enabled. BUG=671370 Review-Url: https://codereview.chromium.org/2556093002 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Enable cardboard support for non-Daydream devices If VrCore exists on non-Daydream devices, WebVR Cardboard support should be enabled. BUG=671370 Review-Url: https://codereview.chromium.org/2556093002 ========== to ========== Enable cardboard support for non-Daydream devices If VrCore exists on non-Daydream devices, WebVR Cardboard support should be enabled. BUG=671370 Committed: https://crrev.com/a22648aca08ffd0812837e124fe4074b4572ed5e Cr-Commit-Position: refs/heads/master@{#437962} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/a22648aca08ffd0812837e124fe4074b4572ed5e Cr-Commit-Position: refs/heads/master@{#437962} |
