Ensure VR UI does not miss controller button clicks

chrome/browser/android/vr_shell/vr_shell_gl.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_gl.h"
 
 #include <limits>
 #include <utility>
 
 #include "base/android/jni_android.h"
@@ skipping 475 matching lines @@
       touch_pending_ = false;
       std::unique_ptr<WebGestureEvent> gesture(new WebGestureEvent(
           WebInputEvent::GestureTapDown, WebInputEvent::NoModifiers,
           (base::TimeTicks::Now() - base::TimeTicks()).InSecondsF()));
       gesture->sourceDevice = blink::WebGestureDeviceTouchpad;
       gesture->x = 0;
       gesture->y = 0;
       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;
   if (!controller_->IsConnected()) {
     // No controller detected, set up a gaze cursor that tracks the
     // forward direction.
     ergo_neutral_pose = {0.0f, 0.0f, -1.0f};
     controller_quat_ = GetRotationFromZAxis(forward_vector);
   } else {
     ergo_neutral_pose = {0.0f, -sin(kErgoAngleOffset), -cos(kErgoAngleOffset)};
     controller_quat_ = controller_->Orientation();
   }
 
   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
   // usability, do the following instead:
   //
   // - Project the controller laser onto a distance-limiting sphere.
   // - Create a vector between the eyes and the outer surface point.
   // - If any UI elements intersect this vector, and is within the bounding
   //   sphere, choose the closest to the eyes, and place the reticle at the
   //   intersection point.
 
   // Compute the distance from the eyes to the distance limiting sphere. Note
   // that the sphere is centered at the controller, rather than the eye, for
   // simplicity.
   float distance = scene_->GetBackgroundDistance();
   target_point_ = GetRayPoint(kHandPosition, controller_direction, distance);
   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_);
   target_element_ = nullptr;
   float target_x;
   float target_y;
 
   for (const auto& plane : scene_->GetUiElements()) {
     if (!plane->IsHitTestable())
       continue;
@@ skipping 47 matching lines @@
       case Fill::SPRITE:
         input_target = InputTarget::UI;
         break;
       default:
         break;
     }
   }
   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) {
   std::vector<std::unique_ptr<WebGestureEvent>> gesture_list =
       controller_->DetectGestures();
   double timestamp = gesture_list.front()->timeStampSeconds();
 
   if (touch_pending_) {
     touch_pending_ = false;
     std::unique_ptr<WebGestureEvent> event(new WebGestureEvent(
@@ skipping 170 matching lines @@
     // object_from_reference_matrix, we're not updating position_external.
     // TODO: Not sure what object_from_reference_matrix is. The new api removed
     // it. For now, removing it seems working fine.
     gvr_api_->ApplyNeckModel(head_pose, 1.0f);
   }
 
   // Update the render position of all UI elements (including desktop).
   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));
   }
 
   DrawWorldElements(head_pose);
 
   frame.Unbind();
 
   // Draw head-locked elements to a separate, non-reprojected buffer.
   if (scene_->HasVisibleHeadLockedElements()) {
     frame.BindBuffer(kFrameHeadlockedBuffer);
@@ skipping 358 matching lines @@
 
   // Don't send VSyncs until we have a timebase/interval.
   if (vsync_interval_.is_zero())
     return;
   target = now + vsync_interval_;
   int64_t intervals = (target - vsync_timebase_) / vsync_interval_;
   target = vsync_timebase_ + intervals * vsync_interval_;
   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_));
   } else {
     pending_vsync_ = true;
     pending_time_ = time;
   }
   if (!ShouldDrawWebVr()) {
     DrawFrame(-1);
   }
@@ skipping 70 matching lines @@
   // appropriate recommended render resolution as the default size during
   // InitializeGl. Revisit if the initialization order changes.
   device::mojom::VRDisplayInfoPtr info = VrShell::CreateVRDisplayInfo(
       gvr_api_.get(), webvr_surface_size_, device_id);
   main_thread_task_runner_->PostTask(
       FROM_HERE,
       base::Bind(&RunVRDisplayInfoCallback, callback, base::Passed(&info)));
 }
 
 }  // namespace vr_shell