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

chrome/browser/android/vr_shell/vr_shell.cc

Index: chrome/browser/android/vr_shell/vr_shell.cc
diff --git a/chrome/browser/android/vr_shell/vr_shell.cc b/chrome/browser/android/vr_shell/vr_shell.cc
index 5c58081dcd53aee0a7ef018c996fff160a104ce2..365addd96b2b2ecb455b619261be1cced43ad33a 100644
--- a/chrome/browser/android/vr_shell/vr_shell.cc
+++ b/chrome/browser/android/vr_shell/vr_shell.cc
@@ -151,7 +151,8 @@ VrShell::VrShell(JNIEnv* env,
                  content::WebContents* main_contents,
                  ui::WindowAndroid* content_window,
                  content::WebContents* ui_contents,
-                 ui::WindowAndroid* ui_window)
+                 ui::WindowAndroid* ui_window,
+                 bool for_web_vr)
     : WebContentsObserver(ui_contents),
       main_contents_(main_contents),
       ui_contents_(ui_contents),
@@ -162,7 +163,11 @@ VrShell::VrShell(JNIEnv* env,
   g_instance = this;
   j_vr_shell_.Reset(env, obj);
   scene_.reset(new UiScene);
-  html_interface_.reset(new UiInterface);
+
+  if (for_web_vr)
+    metrics_helper_->SetWebVREnabled(true);

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

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?

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

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]

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

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

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.

+  html_interface_.reset(new UiInterface(
+      for_web_vr ? UiInterface::Mode::WEB_VR : UiInterface::Mode::STANDARD));
   content_compositor_.reset(new VrCompositor(content_window, false));
   ui_compositor_.reset(new VrCompositor(ui_window, true));
   vr_web_contents_observer_.reset(
@@ -230,7 +235,6 @@ void VrShell::GvrInit(JNIEnv* env,
   base::AutoLock lock(gvr_init_lock_);
 
   // set the initial webvr state
-  metrics_helper_->SetWebVREnabled(webvr_mode_);
   metrics_helper_->SetVRActive(true);
 
   gvr_api_ =
@@ -349,26 +353,31 @@ void VrShell::UpdateController(const gvr::Vec3f& forward_vector) {
   // Note that button up/down state is transient, so IsButtonUp only returns
   // true for a single frame (and we're guaranteed not to miss it).
   if (controller_->IsButtonUp(
-      gvr::ControllerButton::GVR_CONTROLLER_BUTTON_APP)) {
-    if (html_interface_->GetMode() == UiInterface::Mode::MENU) {
-      // Temporary: Hit app button a second time to exit menu mode.
-      if (webvr_mode_) {
+          gvr::ControllerButton::GVR_CONTROLLER_BUTTON_APP)) {
+    // TODO(mthiesse): The page is no longer visible when in menu mode. We
+    // should unfocus or otherwise let it know it's hidden.
+    switch (html_interface_->GetMode()) {

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

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:49]

+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]

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 ;))

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

LOL!

+      case UiInterface::Mode::WEB_VR_MENU:
         html_interface_->SetMode(UiInterface::Mode::WEB_VR);
         main_thread_task_runner_->PostTask(
             FROM_HERE, base::Bind(&device::GvrDeviceProvider::OnDisplayFocus,
                                   delegate_->GetDeviceProvider()));
-      } else {
+        break;
+      case UiInterface::Mode::STANDARD_MENU:
         html_interface_->SetMode(UiInterface::Mode::STANDARD);
-      }
-    } else {
-      if (html_interface_->GetMode() == UiInterface::Mode::WEB_VR) {
+        break;
+      case UiInterface::Mode::WEB_VR:
         main_thread_task_runner_->PostTask(
             FROM_HERE, base::Bind(&device::GvrDeviceProvider::OnDisplayBlur,
                                   delegate_->GetDeviceProvider()));
-      }
-      html_interface_->SetMode(UiInterface::Mode::MENU);
-      // TODO(mthiesse): The page is no longer visible here. We should unfocus
-      // or otherwise let it know it's hidden.
+        html_interface_->SetMode(UiInterface::Mode::WEB_VR_MENU);
+        break;
+      case UiInterface::Mode::CINEMA:
+        // TODO(amp): Handle returning to CINEMA mode when returning from the
+        // menu. For now fall through to STANDARD mode behavior.
+      case UiInterface::Mode::STANDARD:
+        html_interface_->SetMode(UiInterface::Mode::STANDARD_MENU);
+        break;
     }
   }
 #endif
@@ -903,8 +912,7 @@ void VrShell::OnDomContentsLoaded() {
 void VrShell::SetWebVrMode(JNIEnv* env,
                            const base::android::JavaParamRef<jobject>& obj,
                            bool enabled) {
-  webvr_mode_ = enabled;
-  metrics_helper_->SetWebVREnabled(webvr_mode_);
+  metrics_helper_->SetWebVREnabled(enabled);
   if (enabled) {
     html_interface_->SetMode(UiInterface::Mode::WEB_VR);
   } else {
@@ -916,8 +924,7 @@ void VrShell::SetWebVRSecureOrigin(bool secure_origin) {
   html_interface_->SetSecureOrigin(secure_origin);
 }
 
-void VrShell::SubmitWebVRFrame() {
-}
+void VrShell::SubmitWebVRFrame() {}
 
 void VrShell::UpdateWebVRTextureBounds(const gvr::Rectf& left_bounds,
                                        const gvr::Rectf& right_bounds) {
@@ -1018,8 +1025,7 @@ void VrShell::DoUiAction(const UiAction action) {
 #if defined(ENABLE_VR_SHELL_UI_DEV)
     case RELOAD_UI:
       ui_contents_->GetController().Reload(false);
-      html_interface_.reset(new UiInterface);
-      html_interface_->SetMode(UiInterface::Mode::STANDARD);
+      html_interface_.reset(new UiInterface(UiInterface::Mode::STANDARD));
       vr_web_contents_observer_->SetUiInterface(html_interface_.get());
       break;
 #endif
@@ -1045,12 +1051,14 @@ jlong Init(JNIEnv* env,
            const JavaParamRef<jobject>& content_web_contents,
            jlong content_window_android,
            const JavaParamRef<jobject>& ui_web_contents,
-           jlong ui_window_android) {
+           jlong ui_window_android,
+           jboolean for_web_vr) {
   return reinterpret_cast<intptr_t>(new VrShell(
       env, obj, content::WebContents::FromJavaWebContents(content_web_contents),
       reinterpret_cast<ui::WindowAndroid*>(content_window_android),
       content::WebContents::FromJavaWebContents(ui_web_contents),
-      reinterpret_cast<ui::WindowAndroid*>(ui_window_android)));
+      reinterpret_cast<ui::WindowAndroid*>(ui_window_android),
+      for_web_vr));
 }
 
 }  // namespace vr_shell