VrShell: Exit fullscreen on app button press.

VrShell: Exit fullscreen on app button press.

BUG=719997
Review-Url: https://codereview.chromium.org/2872143002
Cr-Commit-Position: refs/heads/master@{#470794}
Committed: https://chromium.googlesource.com/chromium/src/+/ce32493baa1dde9fe3976b80bd128e5560d424d8

[amp, 2017-05-10 01:24:50]

amp@chromium.org changed reviewers:
+ cjgrant@chromium.org, tiborg@chromium.org

[amp, 2017-05-10 01:24:51]

This will need to be rebased with tiborg@ change
https://codereview.chromium.org/2867783002/ (which looks to be failing to patch
due to a missing test mock).

[billorr, 2017-05-10 01:31:58]

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

[billorr, 2017-05-10 01:31:59]

lgtm

[tiborg, 2017-05-10 15:18:50]

https://codereview.chromium.org/2872143002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/ui_scene_manager.cc (right):

https://codereview.chromium.org/2872143002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_scene_manager.cc:228:
browser_->ExitFullscreen();
What happens if you exit fullscreen on a presenting WebVR page?

[amp, 2017-05-10 15:40:08]

https://codereview.chromium.org/2872143002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/ui_scene_manager.cc (right):

https://codereview.chromium.org/2872143002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_scene_manager.cc:228:
browser_->ExitFullscreen();
On 2017/05/10 15:18:50, tiborg wrote:
> What happens if you exit fullscreen on a presenting WebVR page?

It will check if the web contents are fullscreened and if they are tell it to
exit fullscreen.

I'm pretty sure that when in WebVR the web contents are not fullscreen and so it
should be a no-op, but perhaps we need to be more intelligent about this?

[tiborg, 2017-05-10 15:48:43]

https://codereview.chromium.org/2872143002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/ui_scene_manager.cc (right):

https://codereview.chromium.org/2872143002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_scene_manager.cc:228:
browser_->ExitFullscreen();
On 2017/05/10 15:40:08, amp wrote:
> On 2017/05/10 15:18:50, tiborg wrote:
> > What happens if you exit fullscreen on a presenting WebVR page?
> 
> It will check if the web contents are fullscreened and if they are tell it to
> exit fullscreen.
> 
> I'm pretty sure that when in WebVR the web contents are not fullscreen and so
it
> should be a no-op, but perhaps we need to be more intelligent about this?

I thought WebVR presentation includes being in fullscreen. But I may be wrong.
If web_contents_->IsFullscreen() returns false for presenting WebVR we are good.

[amp, 2017-05-10 16:15:01]

https://codereview.chromium.org/2872143002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/ui_scene_manager.cc (right):

https://codereview.chromium.org/2872143002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_scene_manager.cc:228:
browser_->ExitFullscreen();
On 2017/05/10 15:48:43, tiborg wrote:
> On 2017/05/10 15:40:08, amp wrote:
> > On 2017/05/10 15:18:50, tiborg wrote:
> > > What happens if you exit fullscreen on a presenting WebVR page?
> > 
> > It will check if the web contents are fullscreened and if they are tell it
to
> > exit fullscreen.
> > 
> > I'm pretty sure that when in WebVR the web contents are not fullscreen and
so
> it
> > should be a no-op, but perhaps we need to be more intelligent about this?
> 
> I thought WebVR presentation includes being in fullscreen. But I may be wrong.
> If web_contents_->IsFullscreen() returns false for presenting WebVR we are
good.

I just verified with this patch (your comment was a good reminder that I hadn't
checked WebVR).  The app button paused WebVR and pushing it again started it
back up.  If fullscreen had been used then pushing the app button again wouldn't
have worked, so I think we are good. (but let me know if you want me to actually
debug it and check the values).

[bsheedy, 2017-05-10 16:36:15]

FYI, whenever https://codereview.chromium.org/2867243002/ lands and adds support
for pressing the app button during tests, probably want to add an e2e test for
this.

[tiborg, 2017-05-10 17:01:44]

@bsheedy: That's great! That's very handy for more transition e2e tests
(crbug/715663).

https://codereview.chromium.org/2872143002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/ui_scene_manager.cc (right):

https://codereview.chromium.org/2872143002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_scene_manager.cc:228:
browser_->ExitFullscreen();
On 2017/05/10 16:15:01, amp wrote:
> On 2017/05/10 15:48:43, tiborg wrote:
> > On 2017/05/10 15:40:08, amp wrote:
> > > On 2017/05/10 15:18:50, tiborg wrote:
> > > > What happens if you exit fullscreen on a presenting WebVR page?
> > > 
> > > It will check if the web contents are fullscreened and if they are tell it
> to
> > > exit fullscreen.
> > > 
> > > I'm pretty sure that when in WebVR the web contents are not fullscreen and
> so
> > it
> > > should be a no-op, but perhaps we need to be more intelligent about this?
> > 
> > I thought WebVR presentation includes being in fullscreen. But I may be
wrong.
> > If web_contents_->IsFullscreen() returns false for presenting WebVR we are
> good.
> 
> I just verified with this patch (your comment was a good reminder that I
hadn't
> checked WebVR).  The app button paused WebVR and pushing it again started it
> back up.  If fullscreen had been used then pushing the app button again
wouldn't
> have worked, so I think we are good. (but let me know if you want me to
actually
> debug it and check the values).

No, this confirmed this reasonable well. FYI, crrev/2872143002 landed, which
exits WebVR presentation on app button click. I assume you have to rebase on it.
If that works well together (which I think it will) it's all good.

[tiborg, 2017-05-10 17:02:09]

lgtm if it works with exiting WebVR presentation

[amp, 2017-05-10 17:26:01]

On 2017/05/10 16:36:15, bsheedy wrote:
> FYI, whenever https://codereview.chromium.org/2867243002/ lands and adds
support
> for pressing the app button during tests, probably want to add an e2e test for
> this.

Yes, that sounds like something we want.  I was looking at adding a unit test
for this, but the app button call would need to get into the web contents and
make them call back into vr shell but they don't exist for unit tests (and I
don't really want to mock out the whole interaction), so a e2e test that
verifies this would be great.

I'll add a comment to this effect on the vr exit controls bug.

[amp, 2017-05-10 17:39:37]

PTAL.

Rebased and I verified that it works in both WebVR and fullscreen cases (without
any detrimental effects in either mode).

[tiborg, 2017-05-10 17:44:09]

lgtm

[cjgrant, 2017-05-10 21:00:51]

lgtm modulo a couple of test edits.

https://codereview.chromium.org/2872143002/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc (right):

https://codereview.chromium.org/2872143002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:69: InSequence s;
You don't want this here - this test doesn't care about the order of mock calls
(and based on the comment below, it must not).

https://codereview.chromium.org/2872143002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:72:
EXPECT_CALL(*browser_, ExitPresent()).Times(1);
Should also EXPECT the ExitFullscreen call?

[cjgrant, 2017-05-10 21:00:51]

lgtm modulo a couple of test edits.

https://codereview.chromium.org/2872143002/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc (right):

https://codereview.chromium.org/2872143002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:69: InSequence s;
You don't want this here - this test doesn't care about the order of mock calls
(and based on the comment below, it must not).

https://codereview.chromium.org/2872143002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:72:
EXPECT_CALL(*browser_, ExitPresent()).Times(1);
Should also EXPECT the ExitFullscreen call?

[amp, 2017-05-10 21:03:56]

https://codereview.chromium.org/2872143002/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc (right):

https://codereview.chromium.org/2872143002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:69: InSequence s;
On 2017/05/10 21:00:51, cjgrant wrote:
> You don't want this here - this test doesn't care about the order of mock
calls
> (and based on the comment below, it must not).

Done.

https://codereview.chromium.org/2872143002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:72:
EXPECT_CALL(*browser_, ExitPresent()).Times(1);
On 2017/05/10 21:00:51, cjgrant wrote:
> Should also EXPECT the ExitFullscreen call?

Done.

[amp, 2017-05-10 21:05:00]

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

[commit-bot: I haz the power, 2017-05-10 21:06:38]

Dry run: CQ is trying da patch.

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

[amp, 2017-05-10 21:26:11]

The CQ bit was unchecked by amp@chromium.org

[amp, 2017-05-10 21:26:16]

The CQ bit was checked by amp@chromium.org

[amp, 2017-05-10 21:26:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
billorr@chromium.org, tiborg@chromium.org, cjgrant@chromium.org
Link to the patchset: https://codereview.chromium.org/2872143002/#ps2 (title:
"update app button test")

[commit-bot: I haz the power, 2017-05-10 21:26:43]

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-11 03:29:27]

CQ is committing da patch.

Bot data: {"patchset_id": 2, "attempt_start_ts": 1494451576950050, "parent_rev":
"2a6f440fa84a584d916254f9a4c4e4d83c53435f", "commit_rev":
"ce32493baa1dde9fe3976b80bd128e5560d424d8"}

[commit-bot: I haz the power, 2017-05-11 03:29:37]

Description was changed from

==========
VrShell: Exit fullscreen on app button press.

BUG=719997
==========

to

==========
VrShell: Exit fullscreen on app button press.

BUG=719997

Review-Url: https://codereview.chromium.org/2872143002
Cr-Commit-Position: refs/heads/master@{#470794}
Committed:
https://chromium.googlesource.com/chromium/src/+/ce32493baa1dde9fe3976b80bd12...
==========

[commit-bot: I haz the power, 2017-05-11 03:29:38]

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