Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2560843003:
Don't try to enter VR onResume for Cardboard mode (Closed)
DescriptionDon't try to enter VR onResume for Cardboard mode
BUG=672273
Committed: https://crrev.com/c759a4044aa42e34c8ab4a0d1bffe60d0a58eda6
Cr-Commit-Position: refs/heads/master@{#437306}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Reviews #
Total comments: 4
Patch Set 3 : nits #
Messages
Total messages: 24 (14 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 please take a look? This is kind of respond to one of your comments in https://codereview.chromium.org/2556093002/. So returnTo2D is not what is used to mean anymore. It looks like when returnTo2D is false, this is the only path that might trigger re-enter to VR on resume. So I have renamed it to a better name to reflect the change. I also fixed the issue in Cardboard mode due to an oversight which kind of associated with this.
bshe@chromium.org changed reviewers: + dtrainor@chromium.org
+dtrainor@chromium.org for owner Thanks
lgtm!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2560843003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (right): https://codereview.chromium.org/2560843003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1239: if (mVrShellDelegate.exitVRIfNecessary(false/* maybeEnterVrOnResume */)) return true; nit: space before /* https://codereview.chromium.org/2560843003/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/2560843003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:371: public boolean exitVRIfNecessary(boolean maybeEnterVrOnResume) { maybeEnterVrOnResume is not correct. "isPausing" or "isLeavingClank" would be more accurate.
https://codereview.chromium.org/2560843003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (right): https://codereview.chromium.org/2560843003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1239: if (mVrShellDelegate.exitVRIfNecessary(false/* maybeEnterVrOnResume */)) return true; On 2016/12/08 16:22:14, mthiesse wrote: > nit: space before /* Done. https://codereview.chromium.org/2560843003/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/2560843003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:371: public boolean exitVRIfNecessary(boolean maybeEnterVrOnResume) { On 2016/12/08 16:22:14, mthiesse wrote: > maybeEnterVrOnResume is not correct. "isPausing" or "isLeavingClank" would be > more accurate. Done. And changed the logic that prevent reenter vr in to onResume.
lgtm % nits https://codereview.chromium.org/2560843003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (right): https://codereview.chromium.org/2560843003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1239: if (mVrShellDelegate.exitVRIfNecessary(false /* maybeEnterVrOnResume */)) return true; nit: s/maybeEnterVrOnResume/isPausing https://codereview.chromium.org/2560843003/diff/20001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrShellTest.java (right): https://codereview.chromium.org/2560843003/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrShellTest.java:56: mDelegate.exitVRIfNecessary(true /* maybeEnterVrOnResume */); This probably actually makes more sense as false... (also update maybeEnterVrOnResume)
The CQ bit was checked by bshe@chromium.org to run a CQ dry run
https://codereview.chromium.org/2560843003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (right): https://codereview.chromium.org/2560843003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1239: if (mVrShellDelegate.exitVRIfNecessary(false /* maybeEnterVrOnResume */)) return true; On 2016/12/08 17:57:02, mthiesse wrote: > nit: s/maybeEnterVrOnResume/isPausing Done. https://codereview.chromium.org/2560843003/diff/20001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrShellTest.java (right): https://codereview.chromium.org/2560843003/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrShellTest.java:56: mDelegate.exitVRIfNecessary(true /* maybeEnterVrOnResume */); On 2016/12/08 17:57:02, mthiesse wrote: > This probably actually makes more sense as false... (also update > maybeEnterVrOnResume) Done.
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 bshe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dtrainor@chromium.org, mthiesse@chromium.org Link to the patchset: https://codereview.chromium.org/2560843003/#ps40001 (title: "nits")
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": 1481224644928460,
"parent_rev": "6788623a0a0b51635086481a1c684749f377319a", "commit_rev":
"4180deb5130f4793cbece3531eb62ce94d40d3ac"}
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Don't try to enter VR onResume for Cardboard mode BUG=672273 ========== to ========== Don't try to enter VR onResume for Cardboard mode BUG=672273 Committed: https://crrev.com/c759a4044aa42e34c8ab4a0d1bffe60d0a58eda6 Cr-Commit-Position: refs/heads/master@{#437306} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/c759a4044aa42e34c8ab4a0d1bffe60d0a58eda6 Cr-Commit-Position: refs/heads/master@{#437306} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
