Clean up VR Shell mode transitions (and fix potential webvr startup race).

Clean up VR Shell mode transitions (and fix potential webvr startup race).

BUG=668541
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/27fd48978b4e4b24ce8e017cdc3036375f911a45
Cr-Commit-Position: refs/heads/master@{#435010}

[mthiesse, 2016-11-28 21:29:12]

mthiesse@chromium.org changed reviewers:
+ billorr@chromium.org, bshe@chromium.org, cjgrant@chromium.org

[mthiesse, 2016-11-28 21:29:12]

PTAL.

Bill, please review subtle metrics changes. I think we were potentially
double-counting SetWebVREnabled in some cases.

[mthiesse, 2016-11-28 21:31:29]

Description was changed from

==========
Clean up VR Shell mode transitions (and fix potential webvr startup race).

BUG=668541
==========

to

==========
Clean up VR Shell mode transitions (and fix potential webvr startup race).

BUG=668541
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[mthiesse, 2016-11-28 21:34:56]

cc amp@ because I left a TODO in here for him ;)

[bshe, 2016-11-28 22:15:07]

lgtm

https://codereview.chromium.org/2536873002/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_interface.h (right):

https://codereview.chromium.org/2536873002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_interface.h:28: STANDARD_MENU,
nit: can you add comments about the two new enum? It is not obvious why we need
STARND and WEBVR menu be different.

[cjgrant, 2016-11-28 22:21:09]

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

https://codereview.chromium.org/2536873002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:359: switch
(html_interface_->GetMode()) {
This block looks odd to me for a few reasons:
- We're asking the HTML interface what mode it's in, when we set its mode (I
think this is for convenience).
- I don't see why there should be separate modes for menu-in-WebVR and
menu-standard.  Why not independently tell the UI about (1) webVR mode, (2) menu
active, and (3) cinema mode?  Then this logic ends up as "invert menu mode state
in the UI".  I think the same may be true for CINEMA MODE - it feels more like a
boolean that would influence how the UI draws Standard mode, not a mode of its
own.  Otherwise, won't we have CINEMA_MENU as well?

I picture vrShell keeping track of these states, and then:
html_interface_->SetMode(STANDARD);
html_interface_->SetCinema(true/false);
html_interface_->SetMenuMode(true/false);

[amp, 2016-11-28 23:09:48]

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

[amp, 2016-11-28 23:09:49]

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

https://codereview.chromium.org/2536873002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:168:
metrics_helper_->SetWebVREnabled(true);
Doesn't this need to be set to false in the non-webvr case?  Why put it behind
an if block instead of just passing in for_web_vr?

https://codereview.chromium.org/2536873002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:359: switch
(html_interface_->GetMode()) {
On 2016/11/28 22:21:09, cjgrant wrote:
> This block looks odd to me for a few reasons:
> - We're asking the HTML interface what mode it's in, when we set its mode (I
> think this is for convenience).
> - I don't see why there should be separate modes for menu-in-WebVR and
> menu-standard.  Why not independently tell the UI about (1) webVR mode, (2)
menu
> active, and (3) cinema mode?  Then this logic ends up as "invert menu mode
state
> in the UI".  I think the same may be true for CINEMA MODE - it feels more like
a
> boolean that would influence how the UI draws Standard mode, not a mode of its
> own.  Otherwise, won't we have CINEMA_MENU as well?
> 
> I picture vrShell keeping track of these states, and then:
> html_interface_->SetMode(STANDARD);
> html_interface_->SetCinema(true/false);
> html_interface_->SetMenuMode(true/false);

+1 to trying to model this differently.  The TODO below on line 376 looks like
it would be addressed by creating a CINEMA_MENU mode with the implementation in
this patch.

[mthiesse, 2016-11-28 23:28:34]

https://codereview.chromium.org/2536873002/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_interface.h (right):

https://codereview.chromium.org/2536873002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_interface.h:28: STANDARD_MENU,
On 2016/11/28 22:15:07, bshe wrote:
> nit: can you add comments about the two new enum? It is not obvious why we
need
> STARND and WEBVR menu be different.

Acknowledged.

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

https://codereview.chromium.org/2536873002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:359: switch
(html_interface_->GetMode()) {
On 2016/11/28 22:21:09, cjgrant wrote:
> This block looks odd to me for a few reasons:
> - We're asking the HTML interface what mode it's in, when we set its mode (I
> think this is for convenience).
> - I don't see why there should be separate modes for menu-in-WebVR and
> menu-standard.  Why not independently tell the UI about (1) webVR mode, (2)
menu
> active, and (3) cinema mode?  Then this logic ends up as "invert menu mode
state
> in the UI".  I think the same may be true for CINEMA MODE - it feels more like
a
> boolean that would influence how the UI draws Standard mode, not a mode of its
> own.  Otherwise, won't we have CINEMA_MENU as well?
> 
> I picture vrShell keeping track of these states, and then:
> html_interface_->SetMode(STANDARD);
> html_interface_->SetCinema(true/false);
> html_interface_->SetMenuMode(true/false);

Well okay Mr. "I care about long term health", I guuuessss if you insist on
writing good code.

(Chris was worried he was lobbing a grenade here ;))

[mthiesse, 2016-11-28 23:30:12]

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

https://codereview.chromium.org/2536873002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:168:
metrics_helper_->SetWebVREnabled(true);
On 2016/11/28 23:09:49, amp wrote:
> Doesn't this need to be set to false in the non-webvr case?  Why put it behind
> an if block instead of just passing in for_web_vr?

I would think not, because we're not setting WebVR mode at all here. Presumably
a SetWebVREnabled(false) should only ever follow a SetWebVREnabled(true), or
else what are we actually trying to measure here?

[billorr, 2016-11-29 00:05:31]

lgtm

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

https://codereview.chromium.org/2536873002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:168:
metrics_helper_->SetWebVREnabled(true);
On 2016/11/28 23:30:12, mthiesse wrote:
> On 2016/11/28 23:09:49, amp wrote:
> > Doesn't this need to be set to false in the non-webvr case?  Why put it
behind
> > an if block instead of just passing in for_web_vr?
> 
> I would think not, because we're not setting WebVR mode at all here.
Presumably
> a SetWebVREnabled(false) should only ever follow a SetWebVREnabled(true), or
> else what are we actually trying to measure here?

It defaults to false, so we should only need this when we enter webvr mode. 
That said, passing it in won't hurt.

[cjgrant, 2016-11-29 02:26:35]

lgtm

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

https://codereview.chromium.org/2536873002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:359: switch
(html_interface_->GetMode()) {
On 2016/11/28 23:28:34, mthiesse wrote:
> On 2016/11/28 22:21:09, cjgrant wrote:
> > This block looks odd to me for a few reasons:
> > - We're asking the HTML interface what mode it's in, when we set its mode (I
> > think this is for convenience).
> > - I don't see why there should be separate modes for menu-in-WebVR and
> > menu-standard.  Why not independently tell the UI about (1) webVR mode, (2)
> menu
> > active, and (3) cinema mode?  Then this logic ends up as "invert menu mode
> state
> > in the UI".  I think the same may be true for CINEMA MODE - it feels more
like
> a
> > boolean that would influence how the UI draws Standard mode, not a mode of
its
> > own.  Otherwise, won't we have CINEMA_MENU as well?
> > 
> > I picture vrShell keeping track of these states, and then:
> > html_interface_->SetMode(STANDARD);
> > html_interface_->SetCinema(true/false);
> > html_interface_->SetMenuMode(true/false);
> 
> Well okay Mr. "I care about long term health", I guuuessss if you insist on
> writing good code.
> 
> (Chris was worried he was lobbing a grenade here ;))

LOL!

[mthiesse, 2016-11-29 03:53:15]

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

https://codereview.chromium.org/2536873002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:168:
metrics_helper_->SetWebVREnabled(true);
On 2016/11/29 00:05:31, billorr wrote:
> On 2016/11/28 23:30:12, mthiesse wrote:
> > On 2016/11/28 23:09:49, amp wrote:
> > > Doesn't this need to be set to false in the non-webvr case?  Why put it
> behind
> > > an if block instead of just passing in for_web_vr?
> > 
> > I would think not, because we're not setting WebVR mode at all here.
> Presumably
> > a SetWebVREnabled(false) should only ever follow a SetWebVREnabled(true), or
> > else what are we actually trying to measure here?
> 
> It defaults to false, so we should only need this when we enter webvr mode. 
> That said, passing it in won't hurt.

Ah, I didn't know that was how it worked. I'm going to leave this as-is for now,
unless somebody prefers "metrics_helper_->SetWebVREnabled(for_web_vr)", but I
think that's slightly misleading.

That said, it seems we're not calling metrics_helper_->SetWebVREnabled(false) in
all cases where we exit webVR. Do we also want to call it when we exit webVR by
tearing down VR Shell (i.e. when we press the system back button to exit VR)?
Right now it's only called when the site explicitly fires a stop presenting
call, or something else blink-side kicks us out of presentation.

[mthiesse, 2016-11-29 15:13:21]

The CQ bit was checked by mthiesse@chromium.org

[mthiesse, 2016-11-29 15:13:22]

The patchset sent to the CQ was uploaded after l-g-t-m from bshe@chromium.org
Link to the patchset: https://codereview.chromium.org/2536873002/#ps40001
(title: "Address comments")

[commit-bot: I haz the power, 2016-11-29 15:13:38]

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, 2016-11-29 16:35:11]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1480432401743860,
"parent_rev": "84e2fa6a0258d85d64bce3d9a78522cc3c4ec252", "commit_rev":
"37bafb423179be29d16ea18d9574a46040d1c5f9"}

[commit-bot: I haz the power, 2016-11-29 16:35:40]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-11-29 16:37:18]

Description was changed from

==========
Clean up VR Shell mode transitions (and fix potential webvr startup race).

BUG=668541
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Clean up VR Shell mode transitions (and fix potential webvr startup race).

BUG=668541
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/27fd48978b4e4b24ce8e017cdc3036375f911a45
Cr-Commit-Position: refs/heads/master@{#435010}
==========

[commit-bot: I haz the power, 2016-11-29 16:37:19]

Patchset 3 (id:??) landed as
https://crrev.com/27fd48978b4e4b24ce8e017cdc3036375f911a45
Cr-Commit-Position: refs/heads/master@{#435010}