Stay in VR while showing DOFF flow and connect CCT exit button to this.

Stay in VR while showing DOFF flow and connect CCT exit button to this.

This CL does a bunch of complicated things like allowing us to stay in VR when paused as long as we're showing the DOFF flow and expect to return to chrome while still in VR.

There are some gotchas I had to be careful to address, like if the user hits the home button during the DOFF flow then returns to any instance of chrome (CTA/CCT/WebApk) we need to make sure to exit VR in the activity that's still showing the DOFF flow.

This also fixes a possible Activity leak if the nonpresentingdelegate remains alive when switching activities.

BUG=719661
Review-Url: https://codereview.chromium.org/2889693005
Cr-Commit-Position: refs/heads/master@{#473193}
Committed: https://chromium.googlesource.com/chromium/src/+/2dc94cbcc999cade8a3a9a9b2118be81a403e54b

[mthiesse, 2017-05-17 20:37:48]

mthiesse@chromium.org changed reviewers:
+ cjgrant@chromium.org, vollick@chromium.org, ymalik@chromium.org

[mthiesse, 2017-05-17 20:37:49]

PTAL.

[billorr1, 2017-05-17 21:14:10]

billorr@google.com changed reviewers:
+ billorr@google.com

[billorr1, 2017-05-17 21:14:10]

https://codereview.chromium.org/2889693005/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/2889693005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:799:
return;
What is the state here?  Should this record the user action for exiting VR?

[mthiesse, 2017-05-17 22:48:27]

https://codereview.chromium.org/2889693005/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/2889693005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:799:
return;
On 2017/05/17 21:14:10, billorr1 wrote:
> What is the state here?  Should this record the user action for exiting VR?

No, onExitVrResult will call shutdownVr, but with the correct state set.

This is just to safety against something shutting down VR when we didn't expect
it so we don't get into a weird state where we're expecting to exit VR but have
already exited VR.

[ymalik, 2017-05-18 14:37:10]

https://codereview.chromium.org/2889693005/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/2889693005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:432:
assert !mInVr || mShowingDaydreamDoff;
This assert says that we should not be inVr unless we're showing DOFF. The above
IF says that if we're in VR, then if we're not showing DOFF, do something. We
should get rid of the else above and move this assert to the very beginning?

https://codereview.chromium.org/2889693005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:442:
private void swapHostActivity(ChromeActivity activity) {
nit: could you add some comments explaining why we may need to
"swapHostActivity"?

https://codereview.chromium.org/2889693005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:686:
if (mInVr) {
nit: do you think it's more clear to say if mShowingDaydreamDoff here?

https://codereview.chromium.org/2889693005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:750:
if (!mShowingDaydreamDoff) return;
should this be an assert?

[mthiesse, 2017-05-18 15:11:44]

https://codereview.chromium.org/2889693005/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/2889693005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:432:
assert !mInVr || mShowingDaydreamDoff;
On 2017/05/18 14:37:10, ymalik wrote:
> This assert says that we should not be inVr unless we're showing DOFF. The
above
> IF says that if we're in VR, then if we're not showing DOFF, do something. We
> should get rid of the else above and move this assert to the very beginning?

Done. Sorry, that was a forward-looking else branch that shouldn't be hit at the
moment. Added an assertion.

https://codereview.chromium.org/2889693005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:442:
private void swapHostActivity(ChromeActivity activity) {
On 2017/05/18 14:37:10, ymalik wrote:
> nit: could you add some comments explaining why we may need to
> "swapHostActivity"?

Done.

https://codereview.chromium.org/2889693005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:686:
if (mInVr) {
On 2017/05/18 14:37:10, ymalik wrote:
> nit: do you think it's more clear to say if mShowingDaydreamDoff here?

While in this case the two flags are equivalent, I think mInVr is technically
more correct and less confusing in the future.

https://codereview.chromium.org/2889693005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:750:
if (!mShowingDaydreamDoff) return;
On 2017/05/18 14:37:10, ymalik wrote:
> should this be an assert?

No, added a comment.

[mthiesse, 2017-05-18 23:52:16]

Ping, I'd like to get this in by tomorrow if possible as I'm gone next week.

[mthiesse, 2017-05-19 14:10:43]

The CQ bit was checked by mthiesse@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-19 14:10:58]

Dry run: CQ is trying da patch.

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

[ymalik, 2017-05-19 14:45:24]

lgtm

The CL makes sense at a high-level, but as you pointed out, its quite
complicated. Its hard to be sure that it won't break other states in
VrShellDelegate. We should figure out how to simplify / clean this up in the
future and add tests.

[commit-bot: I haz the power, 2017-05-19 15:09:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-19 15:09:56]

Dry run: This issue passed the CQ dry run.

[mthiesse, 2017-05-19 15:10:49]

The CQ bit was checked by mthiesse@chromium.org

[commit-bot: I haz the power, 2017-05-19 15:11:13]

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-05-19 15:17:01]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1495206649277860,
"parent_rev": "3fb17e012c431c830c857bdd9e350603137d11b3", "commit_rev":
"2dc94cbcc999cade8a3a9a9b2118be81a403e54b"}

[commit-bot: I haz the power, 2017-05-19 15:17:16]

Description was changed from

==========
Stay in VR while showing DOFF flow and connect CCT exit button to this.

This CL does a bunch of complicated things like allowing us to stay in VR when
paused as long as we're showing the DOFF flow and expect to return to chrome
while still in VR.

There are some gotchas I had to be careful to address, like if the user hits the
home button during the DOFF flow then returns to any instance of chrome
(CTA/CCT/WebApk) we need to make sure to exit VR in the activity that's still
showing the DOFF flow.

This also fixes a possible Activity leak if the nonpresentingdelegate remains
alive when switching activities.

BUG=719661
==========

to

==========
Stay in VR while showing DOFF flow and connect CCT exit button to this.

This CL does a bunch of complicated things like allowing us to stay in VR when
paused as long as we're showing the DOFF flow and expect to return to chrome
while still in VR.

There are some gotchas I had to be careful to address, like if the user hits the
home button during the DOFF flow then returns to any instance of chrome
(CTA/CCT/WebApk) we need to make sure to exit VR in the activity that's still
showing the DOFF flow.

This also fixes a possible Activity leak if the nonpresentingdelegate remains
alive when switching activities.

BUG=719661

Review-Url: https://codereview.chromium.org/2889693005
Cr-Commit-Position: refs/heads/master@{#473193}
Committed:
https://chromium.googlesource.com/chromium/src/+/2dc94cbcc999cade8a3a9a9b2118...
==========

[commit-bot: I haz the power, 2017-05-19 15:17:17]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/2dc94cbcc999cade8a3a9a9b2118...