App button click exits WebVR presentation.

App button click exits WebVR presentation.

BUG=715730
Review-Url: https://codereview.chromium.org/2867783002
Cr-Commit-Position: refs/heads/master@{#470605}
Committed: https://chromium.googlesource.com/chromium/src/+/ea6cba18fb2e07477a7992cc027677f37a9b772e

[tiborg, 2017-05-08 15:20:35]

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

[tiborg, 2017-05-08 15:20:35]

Hi Chris,
Please take a look.
Thanks,
Tibor

[cjgrant, 2017-05-08 15:29:41]

lgtm

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

https://codereview.chromium.org/2867783002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_scene_manager.cc:189:
browser_->ExitPresent();
The UI should know it's presenting WebVR.  But I guess no point in checking?

[amp, 2017-05-08 17:48:13]

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

https://codereview.chromium.org/2867783002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_scene_manager.cc:189:
browser_->ExitPresent();
On 2017/05/08 15:29:40, cjgrant wrote:
> The UI should know it's presenting WebVR.  But I guess no point in checking?

What happens when you press the app button in vr shell (and/or cinema mode)?

Are we going to expand this method to cover those use cases later?

[tiborg, 2017-05-08 18:59:37]

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

https://codereview.chromium.org/2867783002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_scene_manager.cc:189:
browser_->ExitPresent();
On 2017/05/08 17:48:13, amp wrote:
> On 2017/05/08 15:29:40, cjgrant wrote:
> > The UI should know it's presenting WebVR.  But I guess no point in checking?
> 
> What happens when you press the app button in vr shell (and/or cinema mode)?
> 
> Are we going to expand this method to cover those use cases later?

cjgrant@: I think it is better to let the delegate handle an exit request even
if we are not currently presenting. The rare case can happen that you click the
app button twice before we actually exit presentation. So the delegate must be
able to deal with an exit request while not presenting anyway.

amp@: You mean VR Browsing Mode? Currently, nothing happens in VR Browsing Mode.
Would like to hook up the app button to something for v0? Like exiting
fullscreen?

[amp, 2017-05-08 19:55:23]

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

https://codereview.chromium.org/2867783002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_scene_manager.cc:189:
browser_->ExitPresent();
On 2017/05/08 18:59:37, tiborg wrote:
> On 2017/05/08 17:48:13, amp wrote:
> > On 2017/05/08 15:29:40, cjgrant wrote:
> > > The UI should know it's presenting WebVR.  But I guess no point in
checking?
> > 
> > What happens when you press the app button in vr shell (and/or cinema mode)?
> > 
> > Are we going to expand this method to cover those use cases later?
> 
> cjgrant@: I think it is better to let the delegate handle an exit request even
> if we are not currently presenting. The rare case can happen that you click
the
> app button twice before we actually exit presentation. So the delegate must be
> able to deal with an exit request while not presenting anyway.
> 
> amp@: You mean VR Browsing Mode? Currently, nothing happens in VR Browsing
Mode.
> Would like to hook up the app button to something for v0? Like exiting
> fullscreen?

Yes exiting full screen is what I had in mind.  All we need to do is send a
message back to the web contents (see line 427 of vr_shell.cc in the change).

If we renamed it to something like ExitExclusiveScreenMode (I'm sure we can do
better than that mouthful) then it could perhaps work for both cases.

If we can't get it in for v0 I think it's fine though, as cinema mode wasn't a
critical requirement, just a nice to have.

[tiborg, 2017-05-08 20:14:02]

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

https://codereview.chromium.org/2867783002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_scene_manager.cc:189:
browser_->ExitPresent();
On 2017/05/08 19:55:22, amp wrote:
> On 2017/05/08 18:59:37, tiborg wrote:
> > On 2017/05/08 17:48:13, amp wrote:
> > > On 2017/05/08 15:29:40, cjgrant wrote:
> > > > The UI should know it's presenting WebVR.  But I guess no point in
> checking?
> > > 
> > > What happens when you press the app button in vr shell (and/or cinema
mode)?
> > > 
> > > Are we going to expand this method to cover those use cases later?
> > 
> > cjgrant@: I think it is better to let the delegate handle an exit request
even
> > if we are not currently presenting. The rare case can happen that you click
> the
> > app button twice before we actually exit presentation. So the delegate must
be
> > able to deal with an exit request while not presenting anyway.
> > 
> > amp@: You mean VR Browsing Mode? Currently, nothing happens in VR Browsing
> Mode.
> > Would like to hook up the app button to something for v0? Like exiting
> > fullscreen?
> 
> Yes exiting full screen is what I had in mind.  All we need to do is send a
> message back to the web contents (see line 427 of vr_shell.cc in the change).
> 
> If we renamed it to something like ExitExclusiveScreenMode (I'm sure we can do
> better than that mouthful) then it could perhaps work for both cases.
> 
> If we can't get it in for v0 I think it's fine though, as cinema mode wasn't a
> critical requirement, just a nice to have.

Yeah, exiting fullscreen should be very easy. What do you think about adding an
ExitFullscreen function to VrShell and call it from here? I would do this in a
follow up CL though.

[amp, 2017-05-08 20:37:01]

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

[amp, 2017-05-08 20:37:02]

lgtm

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

https://codereview.chromium.org/2867783002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_scene_manager.cc:189:
browser_->ExitPresent();
On 2017/05/08 20:14:01, tiborg wrote:
> On 2017/05/08 19:55:22, amp wrote:
> > On 2017/05/08 18:59:37, tiborg wrote:
> > > On 2017/05/08 17:48:13, amp wrote:
> > > > On 2017/05/08 15:29:40, cjgrant wrote:
> > > > > The UI should know it's presenting WebVR.  But I guess no point in
> > checking?
> > > > 
> > > > What happens when you press the app button in vr shell (and/or cinema
> mode)?
> > > > 
> > > > Are we going to expand this method to cover those use cases later?
> > > 
> > > cjgrant@: I think it is better to let the delegate handle an exit request
> even
> > > if we are not currently presenting. The rare case can happen that you
click
> > the
> > > app button twice before we actually exit presentation. So the delegate
must
> be
> > > able to deal with an exit request while not presenting anyway.
> > > 
> > > amp@: You mean VR Browsing Mode? Currently, nothing happens in VR Browsing
> > Mode.
> > > Would like to hook up the app button to something for v0? Like exiting
> > > fullscreen?
> > 
> > Yes exiting full screen is what I had in mind.  All we need to do is send a
> > message back to the web contents (see line 427 of vr_shell.cc in the
change).
> > 
> > If we renamed it to something like ExitExclusiveScreenMode (I'm sure we can
do
> > better than that mouthful) then it could perhaps work for both cases.
> > 
> > If we can't get it in for v0 I think it's fine though, as cinema mode wasn't
a
> > critical requirement, just a nice to have.
> 
> Yeah, exiting fullscreen should be very easy. What do you think about adding
an
> ExitFullscreen function to VrShell and call it from here? I would do this in a
> follow up CL though.

Follow up cl is fine.  We will need to wire up the app button, the back button,
and a new exit button that will all do the same thing for full screen eventually
in vr shell so having a specific method for it sounds good.

[tiborg, 2017-05-08 20:43:56]

The CQ bit was checked by tiborg@chromium.org

[commit-bot: I haz the power, 2017-05-08 20:44:56]

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-08 20:48:46]

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

[commit-bot: I haz the power, 2017-05-08 20:48:47]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[cjgrant, 2017-05-09 12:59:16]

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

[commit-bot: I haz the power, 2017-05-09 12:59:46]

Dry run: 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-09 13:32:50]

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

[commit-bot: I haz the power, 2017-05-09 13:32:51]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[tiborg, 2017-05-09 18:01:11]

The CQ bit was checked by tiborg@chromium.org

[tiborg, 2017-05-09 18:01:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
cjgrant@chromium.org, amp@chromium.org
Link to the patchset: https://codereview.chromium.org/2867783002/#ps60001
(title: "Rebased on ToT")

[commit-bot: I haz the power, 2017-05-09 18:02:49]

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-09 18:41:40]

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

[commit-bot: I haz the power, 2017-05-09 18:41:41]

Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[tiborg, 2017-05-10 15:16:53]

The CQ bit was checked by tiborg@chromium.org

[tiborg, 2017-05-10 15:16:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
cjgrant@chromium.org, amp@chromium.org
Link to the patchset: https://codereview.chromium.org/2867783002/#ps80001
(title: "Rebased on ToT, fixed unit tests")

[commit-bot: I haz the power, 2017-05-10 15:18:00]

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-10 16:41:03]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1494429413954570,
"parent_rev": "17d386e6b5412b5ade8103c66200ced8f0e06e7c", "commit_rev":
"ea6cba18fb2e07477a7992cc027677f37a9b772e"}

[commit-bot: I haz the power, 2017-05-10 16:41:10]

Description was changed from

==========
App button click exits WebVR presentation.

BUG=715730
==========

to

==========
App button click exits WebVR presentation.

BUG=715730

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

[commit-bot: I haz the power, 2017-05-10 16:41:12]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/ea6cba18fb2e07477a7992cc0276...

[yhirano, 2017-05-11 03:10:22]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/2874103003/ by yhirano@chromium.org.

The reason for reverting is: Causes failures on a MSAN bot.
https://uberchromegw.corp.google.com/i/chromium.memory/builders/Linux%20Chrom....

[yhirano, 2017-05-11 03:17:22]

On 2017/05/11 03:10:22, yhirano wrote:
> A revert of this CL (patchset #5 id:80001) has been created in
> https://codereview.chromium.org/2874103003/ by mailto:yhirano@chromium.org.
> 
> The reason for reverting is: Causes failures on a MSAN bot.
>
https://uberchromegw.corp.google.com/i/chromium.memory/builders/Linux%20Chrom....

Oops, I chose a wrong CL. Relanding.