Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2903573004:
Fix exiting WebVR present. (Closed)
DescriptionFix exiting WebVR present.
- Previously, exiting WebVR present would stop WebVR rendering but not tell VrShell to exit presenting mode.
BUG=725649
Review-Url: https://codereview.chromium.org/2903573004
Cr-Commit-Position: refs/heads/master@{#474361}
Committed: https://chromium.googlesource.com/chromium/src/+/d8955f1c68c20bc97db9d800be8a13bcc6631b38
Patch Set 1 #
Total comments: 3
Patch Set 2 : Removed duplicate, made Cardboard support work #
Total comments: 2
Patch Set 3 : Rebased on ToT #
Dependent Patchsets: Messages
Total messages: 20 (9 generated)
Description was changed from ========== Fix exiting WebVR present. - Previously, exiting WebVR present would stop WebVR rendering but not tell VrShell to exit presenting mode. BUG=725649 ========== to ========== Fix exiting WebVR present. - Previously, exiting WebVR present would stop WebVR rendering but not tell VrShell to exit presenting mode. BUG=725649 ==========
tiborg@chromium.org changed reviewers: + mthiesse@chromium.org
Hi Micheal, Please have a look at this CL. crrev/2889693005 broke exiting WebVR presentation. We may just be able to move mVrShell.setWebVrModeEnabled(false); out of the if. But I'm not sure how exiting WebVR presentation interacts with CCT. This CL makes exiting WebVR presentation for CTA and the DD headset work. Cheers, Tibor
lgtm % comment https://codereview.chromium.org/2903573004/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/2903573004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:713: mVrShell.setWebVrModeEnabled(false); I think there was probably some confusion here about what this block of code meant. There should be no need to set the webvr mode if we're shutting down VR, so this line is probably not necessary. Alternatively if you think it's still necessary, move mVrShell.setWebVrModeEnabled(false); above "if (!isVrShellEnabled(mVrSupportLevel)..." and avoid duplicating.
ymalik@chromium.org changed reviewers: + ymalik@chromium.org
https://codereview.chromium.org/2903573004/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/2903573004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:713: mVrShell.setWebVrModeEnabled(false); On 2017/05/24 05:24:01, mthiesse wrote: > I think there was probably some confusion here about what this block of code > meant. There should be no need to set the webvr mode if we're shutting down VR, > so this line is probably not necessary. > > Alternatively if you think it's still necessary, move > mVrShell.setWebVrModeEnabled(false); above "if > (!isVrShellEnabled(mVrSupportLevel)..." and avoid duplicating. I think every time we're exiting WebVr presentation, we should be calling mVrShell.setWebVrModeEnabled(false). I imagine this is probably for cases where we exit presentation where VR browsing is not allowed, for e.g. cardboard? Maybe worth adding a test base for this (e.g we never attempt to go into VR browsing when we're not supposed to). So we should move this outside the if block as Mike suggested. Also, this CL will have merge conflicts with https://codereview.chromium.org/2887383002/
PTAL https://codereview.chromium.org/2903573004/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/2903573004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:713: mVrShell.setWebVrModeEnabled(false); On 2017/05/24 14:43:41, ymalik wrote: > On 2017/05/24 05:24:01, mthiesse wrote: > > I think there was probably some confusion here about what this block of code > > meant. There should be no need to set the webvr mode if we're shutting down > VR, > > so this line is probably not necessary. > > > > Alternatively if you think it's still necessary, move > > mVrShell.setWebVrModeEnabled(false); above "if > > (!isVrShellEnabled(mVrSupportLevel)..." and avoid duplicating. > > I think every time we're exiting WebVr presentation, we should be calling > mVrShell.setWebVrModeEnabled(false). > > I imagine this is probably for cases where we exit presentation where VR > browsing is not allowed, for e.g. cardboard? Maybe worth adding a test base for > this (e.g we never attempt to go into VR browsing when we're not supposed to). > > So we should move this outside the if block as Mike suggested. > > Also, this CL will have merge conflicts with > https://codereview.chromium.org/2887383002/ Removed it from the case where we want to exit VR. It works now with CTA and CCT on both DD and Cardboard. I think it is better to not call mVrShell.setWebVrModeEnabled(false); in the exit VR case to avoid some kind of flicker if we exit presentation and then exit VR right after.
lgtm https://codereview.chromium.org/2903573004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2903573004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:710: if (mVrDaydreamApi.isDaydreamCurrentViewer() I think these API calls are pretty slow, we should move this check to a function and cache it until chrome is paused.
lgtm
https://codereview.chromium.org/2903573004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2903573004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:710: if (mVrDaydreamApi.isDaydreamCurrentViewer() On 2017/05/24 15:42:29, mthiesse wrote: > I think these API calls are pretty slow, we should move this check to a function > and cache it until chrome is paused. Let's move this to a follow-up CL.
The CQ bit was checked by tiborg@chromium.org
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
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-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...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by tiborg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mthiesse@chromium.org, ymalik@chromium.org Link to the patchset: https://codereview.chromium.org/2903573004/#ps40001 (title: "Rebased on ToT")
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": 1495646245241420,
"parent_rev": "4b60ccfb7c7c5769005610e94c602dbbbf5f4616", "commit_rev":
"d8955f1c68c20bc97db9d800be8a13bcc6631b38"}
Message was sent while issue was closed.
Description was changed from ========== Fix exiting WebVR present. - Previously, exiting WebVR present would stop WebVR rendering but not tell VrShell to exit presenting mode. BUG=725649 ========== to ========== Fix exiting WebVR present. - Previously, exiting WebVR present would stop WebVR rendering but not tell VrShell to exit presenting mode. BUG=725649 Review-Url: https://codereview.chromium.org/2903573004 Cr-Commit-Position: refs/heads/master@{#474361} Committed: https://chromium.googlesource.com/chromium/src/+/d8955f1c68c20bc97db9d800be8a... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/d8955f1c68c20bc97db9d800be8a... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
