Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2779223004:
Fix showing 2D chrome shortly after DON finished (Closed)
DescriptionFix showing 2D chrome shortly after DON finished
BUG=706629
Review-Url: https://codereview.chromium.org/2779223004
Cr-Commit-Position: refs/heads/master@{#461116}
Committed: https://chromium.googlesource.com/chromium/src/+/8b374988abdf6fe6d5976c08fb52144248cacb98
Patch Set 1 #Patch Set 2 : nit #
Total comments: 4
Patch Set 3 : rebase and comments #
Total comments: 4
Messages
Total messages: 23 (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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Fix showing 2D chrome shortly after DON finished BUG= ========== to ========== Fix showing 2D chrome shortly after DON finished BUG=706629 ==========
bshe@chromium.org changed reviewers: + mthiesse@chromium.org
PTAL
https://codereview.chromium.org/2779223004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2779223004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:411: prepareToEnterVR(); You don't want to call prepareToEnterVR() here because that sets sensor orientation now. This would now effectively loop forever as long as the user is holding the phone in portrait. We want to set landscape here explicitly and ensure that we're in landscape, not sensor. https://codereview.chromium.org/2779223004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:524: if (mRestoreOrientation != null) { Can you move these five restoring orientation and window flags lines out to a function call? I think we duplicate this 3x now.
Patchset #3 (id:40001) has been deleted
The CQ bit was checked by bshe@chromium.org to run a CQ dry run
https://codereview.chromium.org/2779223004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2779223004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:411: prepareToEnterVR(); On 2017/03/30 14:38:19, mthiesse wrote: > You don't want to call prepareToEnterVR() here because that sets sensor > orientation now. This would now effectively loop forever as long as the user is > holding the phone in portrait. > > We want to set landscape here explicitly and ensure that we're in landscape, not > sensor. Done. https://codereview.chromium.org/2779223004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:524: if (mRestoreOrientation != null) { On 2017/03/30 14:38:19, mthiesse wrote: > Can you move these five restoring orientation and window flags lines out to a > function call? I think we duplicate this 3x now. 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.
https://codereview.chromium.org/2779223004/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (left): https://codereview.chromium.org/2779223004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:437: public void onSystemUiVisibilityChange(int visibility) { Won't this get called immediately when you call setWindowModeForVr(ActivityInfo.SCREEN_ORIENTATION_SENSOR), and set Landscape instead of Sensor orientation?
https://codereview.chromium.org/2779223004/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (left): https://codereview.chromium.org/2779223004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:437: public void onSystemUiVisibilityChange(int visibility) { On 2017/03/30 18:22:48, mthiesse wrote: > Won't this get called immediately when you call > setWindowModeForVr(ActivityInfo.SCREEN_ORIENTATION_SENSOR), and set Landscape > instead of Sensor orientation? I think we have mInVr to protect against this from happening?
lgtm https://codereview.chromium.org/2779223004/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2779223004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:420: // Lock orientation to landscape after enter VR. Is this redundant with the call above to setWindowModeForVr(ActivityInfo.SCREEN_ORIENTATION_LANDSCAPE);?
Thanks https://codereview.chromium.org/2779223004/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2779223004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:420: // Lock orientation to landscape after enter VR. On 2017/03/30 18:40:14, mthiesse wrote: > Is this redundant with the call above to > setWindowModeForVr(ActivityInfo.SCREEN_ORIENTATION_LANDSCAPE);? you mean line 403? It might not be called if we requested SENSOR and then put phone in LANDSCAPE orientation. So this should make sure we lock to LANDSCAPE.
The CQ bit was checked by bshe@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": 60001, "attempt_start_ts": 1490969754094720,
"parent_rev": "7343e434fa3db655774d481c99e2f74f00306df7", "commit_rev":
"8b374988abdf6fe6d5976c08fb52144248cacb98"}
Message was sent while issue was closed.
Description was changed from ========== Fix showing 2D chrome shortly after DON finished BUG=706629 ========== to ========== Fix showing 2D chrome shortly after DON finished BUG=706629 Review-Url: https://codereview.chromium.org/2779223004 Cr-Commit-Position: refs/heads/master@{#461116} Committed: https://chromium.googlesource.com/chromium/src/+/8b374988abdf6fe6d5976c08fb52... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://chromium.googlesource.com/chromium/src/+/8b374988abdf6fe6d5976c08fb52... |
