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

chrome/browser/android/vr_shell/vr_shell.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/android/vr_shell/vr_shell.h"
 
 #include "base/metrics/histogram_macros.h"
 #include "chrome/browser/android/vr_shell/ui_elements.h"
 #include "chrome/browser/android/vr_shell/ui_interface.h"
 #include "chrome/browser/android/vr_shell/ui_scene.h"
@@ skipping 133 matching lines @@
 
 }  // namespace
 
 namespace vr_shell {
 
 VrShell::VrShell(JNIEnv* env,
                  jobject obj,
                  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),
       metrics_helper_(new VrMetricsHelper(main_contents)),
       main_thread_task_runner_(base::ThreadTaskRunnerHandle::Get()),
       weak_ptr_factory_(this) {
   DCHECK(g_instance == nullptr);
   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(
       new VrWebContentsObserver(main_contents, html_interface_.get()));
 
   LoadUIContent();
 
   gvr::Mat4f identity;
   SetIdentityM(identity);
   webvr_head_pose_.resize(kPoseRingBufferSize, identity);
@@ skipping 47 matching lines @@
   DAYDREAM = 2,
   VIEWER_TYPE_MAX,
 };
 
 void VrShell::GvrInit(JNIEnv* env,
                       const JavaParamRef<jobject>& obj,
                       jlong native_gvr_api) {
   base::AutoLock lock(gvr_init_lock_);
 
   // set the initial webvr state
-  metrics_helper_->SetWebVREnabled(webvr_mode_);
   metrics_helper_->SetVRActive(true);
 
   gvr_api_ =
       gvr::GvrApi::WrapNonOwned(reinterpret_cast<gvr_context*>(native_gvr_api));
   // TODO(klausw,crbug.com/655722): should report OnGvrDelegateReady here once
   // we switch to using a WebVR render surface. We currently need to wait for
   // the compositor window's size to be known first. See also
   // ContentSurfaceChanged.
   controller_.reset(
       new VrController(reinterpret_cast<gvr_context*>(native_gvr_api)));
@@ skipping 98 matching lines @@
   webvr_right_viewport_->SetSourceBufferIndex(kFramePrimaryBuffer);
 }
 
 void VrShell::UpdateController(const gvr::Vec3f& forward_vector) {
   controller_->UpdateState();
 
 #if defined(ENABLE_VR_SHELL)
   // 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
   if (html_interface_->GetMode() == UiInterface::Mode::WEB_VR) {
     // Process screen touch events for Cardboard button compatibility.
     // Also send tap events for controller "touchpad click" events.
     if (touch_pending_ || controller_->IsButtonUp(
             gvr::ControllerButton::GVR_CONTROLLER_BUTTON_CLICK)) {
       touch_pending_ = false;
       std::unique_ptr<WebGestureEvent> gesture(new WebGestureEvent());
@@ skipping 514 matching lines @@
 
 void VrShell::OnDomContentsLoaded() {
   html_interface_->SetURL(main_contents_->GetVisibleURL());
   html_interface_->SetLoading(main_contents_->IsLoading());
   html_interface_->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 {
     html_interface_->SetMode(UiInterface::Mode::STANDARD);
   }
 }
 
 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) {
   webvr_left_viewport_->SetSourceUv(left_bounds);
   webvr_right_viewport_->SetSourceUv(right_bounds);
 }
 
 gvr::GvrApi* VrShell::gvr_api() {
   return gvr_api_.get();
 }
@@ skipping 80 matching lines @@
     case HISTORY_FORWARD:
       if (controller.CanGoForward())
         controller.GoForward();
       break;
     case RELOAD:
       controller.Reload(false);
       break;
 #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
     case ZOOM_OUT:  // Not handled yet.
     case ZOOM_IN:  // Not handled yet.
       break;
     default:
       NOTREACHED();
   }
 }
 
 void VrShell::RenderViewHostChanged(content::RenderViewHost* old_host,
                                     content::RenderViewHost* new_host) {
   new_host->GetWidget()->GetView()->SetBackgroundColor(SK_ColorTRANSPARENT);
 }
 
 // ----------------------------------------------------------------------------
 // Native JNI methods
 // ----------------------------------------------------------------------------
 
 jlong Init(JNIEnv* env,
            const JavaParamRef<jobject>& obj,
            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