Ensure landscape display updates propagate before entering VR.

Ensure landscape display updates propagate before entering VR.

BUG=681059
Review-Url: https://codereview.chromium.org/2641813003
Cr-Commit-Position: refs/heads/master@{#444748}
Committed: https://chromium.googlesource.com/chromium/src/+/70fa7ff1b2dc490e8f90ddb7b758118b1993c37e

[mthiesse, 2017-01-18 18:23:29]

Description was changed from

==========
Ensure landscape display updates propagate before entering VR.

BUG=681059
==========

to

==========
Ensure landscape display updates propagate before entering VR.

BUG=681059
==========

[mthiesse, 2017-01-18 18:23:29]

mthiesse@chromium.org changed reviewers:
+ bshe@chromium.org

[mthiesse, 2017-01-18 18:23:38]

PTAL

[cjgrant, 2017-01-18 18:36:18]

cjgrant@chromium.org changed reviewers:
+ cjgrant@chromium.org

[cjgrant, 2017-01-18 18:36:18]

https://codereview.chromium.org/2641813003/diff/10001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java
(right):

https://codereview.chromium.org/2641813003/diff/10001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:258:
if (requestedWebVR) nativeSetPresentResult(mNativeVrShellDelegate, true);
nit: This line doesn't have to be in both cases - can call it earlier with
"result" as an argument.

[mthiesse, 2017-01-18 19:16:40]

https://codereview.chromium.org/2641813003/diff/10001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java
(right):

https://codereview.chromium.org/2641813003/diff/10001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:258:
if (requestedWebVR) nativeSetPresentResult(mNativeVrShellDelegate, true);
On 2017/01/18 18:36:18, cjgrant wrote:
> nit: This line doesn't have to be in both cases - can call it earlier with
> "result" as an argument.

Done.

[bshe, 2017-01-18 19:44:53]

https://codereview.chromium.org/2641813003/diff/10001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java
(right):

https://codereview.chromium.org/2641813003/diff/10001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:222:
h.post(new Runnable() {
nit: new Handler().post

https://codereview.chromium.org/2641813003/diff/10001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:230:
void enterVRInLandscape(Tab tab, boolean requestedWebVR) {
nit: private void enterVRInLandscape.

Also, perhaps rename to enterVRInternal or enterVRLater? enterVRInLandscape make
it sounds that we will have enterVRInHorizontal

https://codereview.chromium.org/2641813003/diff/10001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:239:
mTab = tab;
do you expect tab being different from mActivity.getActivityTab at this moment?
If so, probably file a bug and add a TODO.

https://codereview.chromium.org/2641813003/diff/10001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:256:
private void setEnterVRResult(boolean result, boolean requestedWebVR) {
nit: rename result to success?
also, change the code to this?
if (requestedWebVR) nativeSetPresentResult(mNativeVrShellDelegate, success);
if (!success && !mVrDaydreamApi.exitFromVr(EXIT_VR_RESULT, new Intent())) {
    mVrShell.setVrModeEnabled(false);
}

[mthiesse, 2017-01-18 20:09:14]

https://codereview.chromium.org/2641813003/diff/10001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java
(right):

https://codereview.chromium.org/2641813003/diff/10001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:222:
h.post(new Runnable() {
On 2017/01/18 19:44:53, bshe wrote:
> nit: new Handler().post

Done.

https://codereview.chromium.org/2641813003/diff/10001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:230:
void enterVRInLandscape(Tab tab, boolean requestedWebVR) {
On 2017/01/18 19:44:53, bshe wrote:
> nit: private void enterVRInLandscape.
> 
> Also, perhaps rename to enterVRInternal or enterVRLater? enterVRInLandscape
make
> it sounds that we will have enterVRInHorizontal

Done.

https://codereview.chromium.org/2641813003/diff/10001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:239:
mTab = tab;
On 2017/01/18 19:44:53, bshe wrote:
> do you expect tab being different from mActivity.getActivityTab at this
moment?
> If so, probably file a bug and add a TODO.

Instead of passing the tab around, I'll just use the current tab after the
callback. It really shouldn't ever change when anything but a script is entering
VR, but this will avoid any complexity around possibly not using the active tab.

https://codereview.chromium.org/2641813003/diff/10001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:256:
private void setEnterVRResult(boolean result, boolean requestedWebVR) {
On 2017/01/18 19:44:53, bshe wrote:
> nit: rename result to success?
> also, change the code to this?
> if (requestedWebVR) nativeSetPresentResult(mNativeVrShellDelegate, success);
> if (!success && !mVrDaydreamApi.exitFromVr(EXIT_VR_RESULT, new Intent())) {
>     mVrShell.setVrModeEnabled(false);
> }

Done.

[bshe, 2017-01-19 15:00:26]

On 2017/01/18 20:09:14, mthiesse wrote:
>
https://codereview.chromium.org/2641813003/diff/10001/chrome/android/java/src...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java
> (right):
> 
>
https://codereview.chromium.org/2641813003/diff/10001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:222:
> h.post(new Runnable() {
> On 2017/01/18 19:44:53, bshe wrote:
> > nit: new Handler().post
> 
> Done.
> 
>
https://codereview.chromium.org/2641813003/diff/10001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:230:
> void enterVRInLandscape(Tab tab, boolean requestedWebVR) {
> On 2017/01/18 19:44:53, bshe wrote:
> > nit: private void enterVRInLandscape.
> > 
> > Also, perhaps rename to enterVRInternal or enterVRLater? enterVRInLandscape
> make
> > it sounds that we will have enterVRInHorizontal
> 
> Done.
> 
>
https://codereview.chromium.org/2641813003/diff/10001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:239:
> mTab = tab;
> On 2017/01/18 19:44:53, bshe wrote:
> > do you expect tab being different from mActivity.getActivityTab at this
> moment?
> > If so, probably file a bug and add a TODO.
> 
> Instead of passing the tab around, I'll just use the current tab after the
> callback. It really shouldn't ever change when anything but a script is
entering
> VR, but this will avoid any complexity around possibly not using the active
tab.
> 
>
https://codereview.chromium.org/2641813003/diff/10001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:256:
> private void setEnterVRResult(boolean result, boolean requestedWebVR) {
> On 2017/01/18 19:44:53, bshe wrote:
> > nit: rename result to success?
> > also, change the code to this?
> > if (requestedWebVR) nativeSetPresentResult(mNativeVrShellDelegate, success);
> > if (!success && !mVrDaydreamApi.exitFromVr(EXIT_VR_RESULT, new Intent())) {
> >     mVrShell.setVrModeEnabled(false);
> > }
> 
> Done.

lgtm

[mthiesse, 2017-01-19 15:15:43]

The CQ bit was checked by mthiesse@chromium.org

[commit-bot: I haz the power, 2017-01-19 15:16:02]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-19 16:05:24]

CQ is committing da patch.

Bot data: {"patchset_id": 70001, "attempt_start_ts": 1484838943337130,
"parent_rev": "03b2e0bdad2a3157ee1c1f8ef6278826f9ea6d4f", "commit_rev":
"70fa7ff1b2dc490e8f90ddb7b758118b1993c37e"}

[commit-bot: I haz the power, 2017-01-19 16:05:56]

Description was changed from

==========
Ensure landscape display updates propagate before entering VR.

BUG=681059
==========

to

==========
Ensure landscape display updates propagate before entering VR.

BUG=681059

Review-Url: https://codereview.chromium.org/2641813003
Cr-Commit-Position: refs/heads/master@{#444748}
Committed:
https://chromium.googlesource.com/chromium/src/+/70fa7ff1b2dc490e8f90ddb7b758...
==========

[commit-bot: I haz the power, 2017-01-19 16:05:57]

Committed patchset #5 (id:70001) as
https://chromium.googlesource.com/chromium/src/+/70fa7ff1b2dc490e8f90ddb7b758...