Ensure VR UI does not miss controller button clicks

chrome/browser/android/vr_shell/vr_shell_gl.cc

Index: chrome/browser/android/vr_shell/vr_shell_gl.cc
diff --git a/chrome/browser/android/vr_shell/vr_shell_gl.cc b/chrome/browser/android/vr_shell/vr_shell_gl.cc
index 07c45b0fd3f3de6a30b0ffca439c0b316688e63d..587c233fa5b55a6d3916c2c89d31676ba5df54af 100644
--- a/chrome/browser/android/vr_shell/vr_shell_gl.cc
+++ b/chrome/browser/android/vr_shell/vr_shell_gl.cc
@@ -493,8 +493,6 @@ void VrShellGl::HandleControllerInput(const gvr::Vec3f& forward_vector) {
       SendGesture(InputTarget::CONTENT, std::move(gesture));
       DVLOG(1) << __FUNCTION__ << ": sent CLICK gesture";
     }
-
-    return;

[cjgrant, 2017/03/23 21:24:47]

I'm not sure why we were returning here.

[mthiesse, 2017/03/23 21:34:23]

Fine to remove, as long as the check below stays.

   }
 
   gvr::Vec3f ergo_neutral_pose;
@@ -511,6 +509,12 @@ void VrShellGl::HandleControllerInput(const gvr::Vec3f& forward_vector) {
   gvr::Mat4f mat = QuatToMatrix(controller_quat_);
   gvr::Vec3f controller_direction = MatrixVectorMul(mat, ergo_neutral_pose);
 
+  HandleControllerAppButtonActivity(controller_direction);
+
+  if (ShouldDrawWebVr()) {

[cjgrant, 2017/03/23 21:24:47]

On second thought, this is bad, and I'll revert this check.

[mthiesse, 2017/03/23 21:34:23]

On third thought, don't revert this check.

[cjgrant, 2017/03/23 21:35:35]

On third thought, lets keep this, as it moves the original return down here. 
However, the fact that we early-return at all is a bug - we can have UI elements
with active mouse activity over them, and when we return to "drawing webvr", we
need to send "mouse is gone" events.

I don't know how big a deal this will actually be, since we're only now letting
people pop between webvr and menu with the app button.  I'll file a bug for this
too.

+    return;
+  }
+
   // If we place the reticle based on elements intersecting the controller beam,
   // we can end up with the reticle hiding behind elements, or jumping laterally
   // in the field of view. This is physically correct, but hard to use. For
@@ -530,36 +534,6 @@ void VrShellGl::HandleControllerInput(const gvr::Vec3f& forward_vector) {
   gvr::Vec3f eye_to_target = target_point_;
   NormalizeVector(eye_to_target);
 
-  // Note that button up/down state is transient, so ButtonDownHappened only

[cjgrant, 2017/03/23 21:24:47]

Broken out to a function for readability.

-  // returns true for a single frame (and we're guaranteed not to miss it).
-  if (controller_->ButtonDownHappened(
-          gvr::ControllerButton::GVR_CONTROLLER_BUTTON_APP)) {
-    controller_start_direction_ = controller_direction;
-  }
-  if (controller_->ButtonUpHappened(
-          gvr::ControllerButton::GVR_CONTROLLER_BUTTON_APP)) {
-    // A gesture is a movement of the controller while holding the App button.
-    // If the angle of the movement is within a threshold, the action is
-    // considered a regular click
-    // TODO(asimjour1): We need to refactor the gesture recognition outside of
-    // VrShellGl.
-    UiInterface::Direction direction = UiInterface::NONE;
-    float gesture_xz_angle;
-    bool valid_angle = XZAngle(controller_start_direction_,
-                               controller_direction, &gesture_xz_angle);
-    DCHECK(valid_angle);
-    if (fabs(gesture_xz_angle) > kMinAppButtonGestureAngleRad) {
-      direction = gesture_xz_angle < 0 ? UiInterface::LEFT : UiInterface::RIGHT;
-      main_thread_task_runner_->PostTask(
-          FROM_HERE, base::Bind(&VrShell::AppButtonGesturePerformed,
-                                weak_vr_shell_, direction));
-    }
-    if (direction == UiInterface::NONE) {
-      main_thread_task_runner_->PostTask(
-          FROM_HERE, base::Bind(&VrShell::AppButtonPressed, weak_vr_shell_));
-    }
-  }
-
   // Determine which UI element (if any) intersects the line between the eyes
   // and the controller target position.
   float closest_element_distance = VectorLength(target_point_);
@@ -627,6 +601,40 @@ void VrShellGl::HandleControllerInput(const gvr::Vec3f& forward_vector) {
   SendEventsToTarget(input_target, pixel_x, pixel_y);
 }
 
+void VrShellGl::HandleControllerAppButtonActivity(
+    const gvr::Vec3f& controller_direction) {
+  // Note that button up/down state is transient, so ButtonDownHappened only
+  // returns true for a single frame (and we're guaranteed not to miss it).
+  if (controller_->ButtonDownHappened(
+          gvr::ControllerButton::GVR_CONTROLLER_BUTTON_APP)) {
+    controller_start_direction_ = controller_direction;
+  }
+  if (controller_->ButtonUpHappened(
+          gvr::ControllerButton::GVR_CONTROLLER_BUTTON_APP)) {
+    // A gesture is a movement of the controller while holding the App button.
+    // If the angle of the movement is within a threshold, the action is
+    // considered a regular click
+    // TODO(asimjour1): We need to refactor the gesture recognition outside of
+    // VrShellGl.
+    UiInterface::Direction direction = UiInterface::NONE;
+    float gesture_xz_angle;
+    if (XZAngle(controller_start_direction_, controller_direction,
+                &gesture_xz_angle)) {
+      if (fabs(gesture_xz_angle) > kMinAppButtonGestureAngleRad) {
+        direction =
+            gesture_xz_angle < 0 ? UiInterface::LEFT : UiInterface::RIGHT;
+        main_thread_task_runner_->PostTask(
+            FROM_HERE, base::Bind(&VrShell::AppButtonGesturePerformed,
+                                  weak_vr_shell_, direction));
+      }
+    }
+    if (direction == UiInterface::NONE) {
+      main_thread_task_runner_->PostTask(
+          FROM_HERE, base::Bind(&VrShell::AppButtonPressed, weak_vr_shell_));
+    }
+  }
+}
+
 void VrShellGl::SendEventsToTarget(InputTarget input_target,
                                    int pixel_x,
                                    int pixel_y) {
@@ -817,7 +825,10 @@ void VrShellGl::DrawFrame(int16_t frame_index) {
   scene_->UpdateTransforms(TimeInMicroseconds());
 
   {
+    // TODO(crbug.com/704690): Acquire controller state in a way that's timely
+    // for both the gamepad API and UI input handling.
     TRACE_EVENT0("gpu", "VrShellGl::UpdateController");
+    UpdateController();
     HandleControllerInput(GetForwardVector(head_pose));
   }
 
@@ -1196,10 +1207,6 @@ void VrShellGl::OnVSync() {
   task_runner_->PostDelayedTask(FROM_HERE, vsync_task_.callback(),
                                 target - now);
 
-  // Get controller data now so that it's ready for both WebVR's
-  // gamepad API input and VrShell's own processing.
-  UpdateController();
-
   base::TimeDelta time = intervals * vsync_interval_;
   if (!callback_.is_null()) {
     SendVSync(time, base::ResetAndReturn(&callback_));