Ensure the address bar continues to show after dismissing WebVR.

Ensure the address bar continues to show after dismissing WebVR.

When WebVR presentation exits, the address bar is sometimes hidden.  This is because when we enter VR, the browser controls try to animate out to hidden.  The animation doesn't actually render any frames because we block vsync.

We were trying to make the controls visible by calling mActivity.getFullscreenManager().setPositionsForTabToNonFullscreen, which targets pretty low in the stack for whether the controls should be visible (for the next rendered frame), but higher level components still think the controls are not visible.  We should call mTab.updateBrowserControlsState(BrowserControlsState.SHOWN, true); instead to mark the controls visible at a higher level in the stack so components throughout the stack agree on control visibility.

BUG=715182
Review-Url: https://codereview.chromium.org/2903443002
Cr-Commit-Position: refs/heads/master@{#474007}
Committed: https://chromium.googlesource.com/chromium/src/+/931b0527cd26875c0b6bc5645b9ae29f484ce6b1

[ymalik, 2017-05-22 23:47:46]

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

[ymalik, 2017-05-22 23:47:46]

https://codereview.chromium.org/2903443002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java
(left):

https://codereview.chromium.org/2903443002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:881:
mActivity.getFullscreenManager().setPositionsForTabToNonFullscreen();
I am assuming a combination of removing this and calling
mTab.updateBrowserControlsState from pauseVr fixes this issue?

Could you elaborate on why removing this is required in the CL description?

[billorr, 2017-05-22 23:56:44]

Description was changed from

==========
Ensure the address bar continues to show after dismissing WebVR.

When WebVR presentation exits, the address bar is sometimes hidden.

BUG=715182
==========

to

==========
Ensure the address bar continues to show after dismissing WebVR.

When WebVR presentation exits, the address bar is sometimes hidden.  This is
because when we enter VR, the browser controls try to animate out to hidden. 
The animation doesn't actually render any frames because we block vsync.

We were trying to make the controls visible by calling
mActivity.getFullscreenManager().setPositionsForTabToNonFullscreen, which
targets pretty low in the stack for whether the controls should be visible (for
the next rendered frame), but higher level components still think the controls
are not visible.  We should call
mTab.updateBrowserControlsState(BrowserControlsState.SHOWN, true); instead to
mark the controls visible at a higher level in the stack so components
throughout the stack agree on control visibility.

BUG=715182
==========

[billorr, 2017-05-22 23:56:53]

https://codereview.chromium.org/2903443002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java
(left):

https://codereview.chromium.org/2903443002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:881:
mActivity.getFullscreenManager().setPositionsForTabToNonFullscreen();
On 2017/05/22 23:47:46, ymalik wrote:
> I am assuming a combination of removing this and calling
> mTab.updateBrowserControlsState from pauseVr fixes this issue?
> 
> Could you elaborate on why removing this is required in the CL description?
> 

This just tells the browser to render the next frame with the address
bar/controls in the right place (targetting pretty low-level in the stack).
mTab.updateBrowserControlsState sets the controls as visible at a higher level
in the stack.  

The bug is that different layers of the stack disagree on whether the controls
should be visible, and actually are animating them out after this line runs.

[ymalik, 2017-05-22 23:59:50]

lgtm, Thanks for the verbose explanation!

[Ted C, 2017-05-23 00:00:34]

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

[Ted C, 2017-05-23 00:00:35]

lgtm

[mthiesse, 2017-05-23 00:07:15]

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

[mthiesse, 2017-05-23 00:07:16]

https://codereview.chromium.org/2903443002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java
(right):

https://codereview.chromium.org/2903443002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java:458:
mTab.updateBrowserControlsState(BrowserControlsState.SHOWN, true);
Why is this here and not in restoreTabFromVR()?

Also I think you should be able to call this with BrowserControlsState.BOTH so
that we don't change what the ControlsState previously was? Maybe SHOWN is
desired though, so feel free to leave that as is. (We only need to call this at
all to get the TabbedModeBrowserControlsVisibilityDelegate in
ChromeTabbedActivity to update).

[billorr, 2017-05-23 00:48:12]

https://codereview.chromium.org/2903443002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java
(right):

https://codereview.chromium.org/2903443002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java:458:
mTab.updateBrowserControlsState(BrowserControlsState.SHOWN, true);
On 2017/05/23 00:07:16, mthiesse wrote:
> Why is this here and not in restoreTabFromVR()?

User interactions like swapping tabs and scrolling update the controls
visibility, so I don't anticipate bugs with this approach.

onPause/Pause are called as one of the last steps of VrShellDelegate#shutdownVr,
in particular after mInVr is set to false.  mInVr is used in
ChromeTabbedActivity to determine whether to hide controls, so this is slightly
safer against race conditions.

I could see an argument for making a new explicit function instead of going
through vrshell#pause/OnPause.  I'm trying to keep it roughly in the same place
as the setPositionsForTabToNonFullscreen().

> 
> Also I think you should be able to call this with BrowserControlsState.BOTH so
> that we don't change what the ControlsState previously was? Maybe SHOWN is
> desired though, so feel free to leave that as is. (We only need to call this
at
> all to get the TabbedModeBrowserControlsVisibilityDelegate in
> ChromeTabbedActivity to update).
I was worried about this as well - for example are we going to show controls
over full-screen content.  Ted claims that this still checks for whether the
page state supports controls.  During testing, I was seeing that exiting VRShell
with fullscreen video exits fullscreen.

[mthiesse, 2017-05-23 04:37:44]

lgtm % comment

https://codereview.chromium.org/2903443002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java
(right):

https://codereview.chromium.org/2903443002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java:458:
mTab.updateBrowserControlsState(BrowserControlsState.SHOWN, true);
So there are also some cases now, and probably more in the future, where we
pause without shutting down VR. So I think semantically, putting this in onPause
isn't right, even though it will work.

I think this should probably live in shutdown() then, as it's one of the things
we need to do when tearing down VR UI, and runs last in the teardown process.

[billorr, 2017-05-23 16:20:10]

On 2017/05/23 04:37:44, mthiesse wrote:
> lgtm % comment
> 
>
https://codereview.chromium.org/2903443002/diff/1/chrome/android/java/src/org...
> File
> chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java
> (right):
> 
>
https://codereview.chromium.org/2903443002/diff/1/chrome/android/java/src/org...
>
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java:458:
> mTab.updateBrowserControlsState(BrowserControlsState.SHOWN, true);
> So there are also some cases now, and probably more in the future, where we
> pause without shutting down VR. So I think semantically, putting this in
onPause
> isn't right, even though it will work.
> 
> I think this should probably live in shutdown() then, as it's one of the
things
> we need to do when tearing down VR UI, and runs last in the teardown process.

Sounds good - this works as well.

[billorr, 2017-05-23 16:20:12]

On 2017/05/23 04:37:44, mthiesse wrote:
> lgtm % comment
> 
>
https://codereview.chromium.org/2903443002/diff/1/chrome/android/java/src/org...
> File
> chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java
> (right):
> 
>
https://codereview.chromium.org/2903443002/diff/1/chrome/android/java/src/org...
>
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java:458:
> mTab.updateBrowserControlsState(BrowserControlsState.SHOWN, true);
> So there are also some cases now, and probably more in the future, where we
> pause without shutting down VR. So I think semantically, putting this in
onPause
> isn't right, even though it will work.
> 
> I think this should probably live in shutdown() then, as it's one of the
things
> we need to do when tearing down VR UI, and runs last in the teardown process.

Sounds good - this works as well.

[billorr, 2017-05-23 16:20:46]

The CQ bit was checked by billorr@chromium.org

[billorr, 2017-05-23 16:20:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
tedchoc@chromium.org, mthiesse@chromium.org, ymalik@chromium.org
Link to the patchset: https://codereview.chromium.org/2903443002/#ps20001
(title: "cr feedback")

[commit-bot: I haz the power, 2017-05-23 16:21:24]

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-23 17:18:35]

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

[commit-bot: I haz the power, 2017-05-23 17:18:36]

Failed to apply patch for
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:
While running git apply --index -3 -p1;
  error: patch failed:
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:878
  Falling back to three-way merge...
  Applied patch to
'chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java'
with conflicts.
  U
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java

Patch:      
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java
Index:
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java
diff --git
a/chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java
b/chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java
index
5fb62c6b7ed6d4f663d5be92efdbeb21d986ed81..1798155e0fa726217123f3e5fe8a0e415023810e
100644
---
a/chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java
+++
b/chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java
@@ -878,7 +878,6 @@ public class VrShellDelegate implements
ApplicationStatus.ActivityStateListener,
         mVrShell.pause();
         removeVrViews();
         destroyVrShell();
-        mActivity.getFullscreenManager().setPositionsForTabToNonFullscreen();
         if (setVrMode) mVrClassesWrapper.setVrModeEnabled(mActivity, false);
     }

[billorr, 2017-05-23 17:59:57]

The CQ bit was checked by billorr@chromium.org

[billorr, 2017-05-23 17:59:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
tedchoc@chromium.org, mthiesse@chromium.org, ymalik@chromium.org
Link to the patchset: https://codereview.chromium.org/2903443002/#ps40001
(title: "rebase")

[commit-bot: I haz the power, 2017-05-23 18:00:40]

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-23 19:00:24]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1495562397381520,
"parent_rev": "bdbf9cf9b95b27fe1cfaba0f1032db6ae8a3c4bf", "commit_rev":
"931b0527cd26875c0b6bc5645b9ae29f484ce6b1"}

[commit-bot: I haz the power, 2017-05-23 19:00:35]

Description was changed from

==========
Ensure the address bar continues to show after dismissing WebVR.

When WebVR presentation exits, the address bar is sometimes hidden.  This is
because when we enter VR, the browser controls try to animate out to hidden. 
The animation doesn't actually render any frames because we block vsync.

We were trying to make the controls visible by calling
mActivity.getFullscreenManager().setPositionsForTabToNonFullscreen, which
targets pretty low in the stack for whether the controls should be visible (for
the next rendered frame), but higher level components still think the controls
are not visible.  We should call
mTab.updateBrowserControlsState(BrowserControlsState.SHOWN, true); instead to
mark the controls visible at a higher level in the stack so components
throughout the stack agree on control visibility.

BUG=715182
==========

to

==========
Ensure the address bar continues to show after dismissing WebVR.

When WebVR presentation exits, the address bar is sometimes hidden.  This is
because when we enter VR, the browser controls try to animate out to hidden. 
The animation doesn't actually render any frames because we block vsync.

We were trying to make the controls visible by calling
mActivity.getFullscreenManager().setPositionsForTabToNonFullscreen, which
targets pretty low in the stack for whether the controls should be visible (for
the next rendered frame), but higher level components still think the controls
are not visible.  We should call
mTab.updateBrowserControlsState(BrowserControlsState.SHOWN, true); instead to
mark the controls visible at a higher level in the stack so components
throughout the stack agree on control visibility.

BUG=715182

Review-Url: https://codereview.chromium.org/2903443002
Cr-Commit-Position: refs/heads/master@{#474007}
Committed:
https://chromium.googlesource.com/chromium/src/+/931b0527cd26875c0b6bc5645b9a...
==========

[commit-bot: I haz the power, 2017-05-23 19:00:38]

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