Add menu mode plumbing for WebVR mode.

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 c0210b93c8f98dc21ea8228412b3e9cfbd978938..047a694d7b1b6a891afae0a0b4be84c20be49470 100644
--- a/chrome/browser/android/vr_shell/vr_shell_gl.cc
+++ b/chrome/browser/android/vr_shell/vr_shell_gl.cc
@@ -486,7 +486,7 @@ void VrShellGl::UpdateController(const gvr::Vec3f& forward_vector) {
         FROM_HERE, base::Bind(&VrShell::AppButtonPressed, weak_vr_shell_));
   }
 
-  if (web_vr_mode_) {
+  if (ShouldDrawWebVr()) {
     // Process screen touch events for Cardboard button compatibility.
     // Also send tap events for controller "touchpad click" events.
     if (touch_pending_ ||
@@ -699,9 +699,10 @@ void VrShellGl::DrawFrame(int16_t frame_index) {
 
   // If needed, resize the primary buffer for use with WebVR. Resizing
   // needs to happen before acquiring a frame.
-  if (web_vr_mode_) {
+  if (ShouldDrawWebVr()) {

[cjgrant, 2017/03/16 15:44:36]

Seeing the number of spots we call this, a couple things come to mind:
- Do we set web_vr_mode_ on this thread, such that it can't change
mid-render-loop.
- Will skipping WebVR rendering while in the menu cause any pose queues (or
related) to get out of sync?  

[tiborg, 2017/03/16 19:40:22]

Yes, web_vr_mode_ should only be set by the GL thread. So, it can't change in
the middle of the render loop. The second question I don't know.

[cjgrant, 2017/03/20 15:34:57]

So it turns out that WebVR changes have fully broken the app button press when
WebVR is rendering, anyway.  I made a bug to work on that, after this plumbing
lands.

[tiborg, 2017/03/20 15:56:08]

Acknowledged.

     // Process all pending_bounds_ changes targeted for before this
     // frame, being careful of wrapping frame indices.
+    // TODO(cjgrant): Do not compare the constants on every frame.

[tiborg, 2017/03/16 19:40:22]

Make a bug for this?

[cjgrant, 2017/03/16 21:21:51]

For something this trivial, I don't think a bug is justified - nobody would ever
prioritize it.

[tiborg, 2017/03/17 15:13:44]

Yes, it was just a suggestion. You could make a bug and assign it to yourself.
Than it is tracked. But I will not fight on this.

[cjgrant, 2017/03/20 15:34:57]

So I've tied this cleanup to the UI math optimization bug.  It's not really
related, but that seems like a great time to circle back and clean up this kind
of thing (there may be other cleanups too of course).

[tiborg, 2017/03/20 15:56:08]

Great, thanks.

     static constexpr unsigned max =
         std::numeric_limits<decltype(frame_index_)>::max();
     static_assert(max > kPoseRingBufferSize * 2,
@@ -761,7 +762,7 @@ void VrShellGl::DrawFrame(int16_t frame_index) {
   }
   frame.BindBuffer(kFramePrimaryBuffer);
 
-  if (web_vr_mode_) {
+  if (ShouldDrawWebVr()) {
     DrawWebVr();
   }
 
@@ -772,7 +773,8 @@ void VrShellGl::DrawFrame(int16_t frame_index) {
   // submitting. Technically we don't need a pose if not reprojecting,
   // but keeping it uninitialized seems likely to cause problems down
   // the road. Copying it is cheaper than fetching a new one.
-  if (web_vr_mode_) {
+  if (ShouldDrawWebVr()) {
+    // TODO(cjgrant): Do not compare the constants on every frame.

[tiborg, 2017/03/16 19:40:22]

Here, too.

     static_assert(!((kPoseRingBufferSize - 1) & kPoseRingBufferSize),
                   "kPoseRingBufferSize must be a power of 2");
     head_pose = webvr_head_pose_[frame_index % kPoseRingBufferSize];
@@ -833,7 +835,7 @@ void VrShellGl::DrawVrShellAndUnbind(const gvr::Mat4f& head_pose,
     }
   }
 
-  if (web_vr_mode_) {
+  if (ShouldDrawWebVr()) {
     // WebVR is incompatible with 3D world compositing since the
     // depth buffer was already populated with unknown scaling - the
     // WebVR app has full control over zNear/zFar. Just leave the
@@ -853,8 +855,8 @@ void VrShellGl::DrawVrShellAndUnbind(const gvr::Mat4f& head_pose,
     glClear(GL_COLOR_BUFFER_BIT | GL_DEPTH_BUFFER_BIT);
   }
   if (!world_elements.empty()) {
-    DrawUiView(&head_pose, world_elements, render_size_primary_,
-               kViewportListPrimaryOffset);
+    DrawUiView(head_pose, world_elements, render_size_primary_,
+               kViewportListPrimaryOffset, scene_->GetCursorEnabled());
   }
   frame.Unbind();  // Done with the primary buffer.
 
@@ -872,32 +874,29 @@ void VrShellGl::DrawVrShellAndUnbind(const gvr::Mat4f& head_pose,
     frame.BindBuffer(kFrameHeadlockedBuffer);
     glClearColor(0.0f, 0.0f, 0.0f, 0.0f);
     glClear(GL_COLOR_BUFFER_BIT | GL_DEPTH_BUFFER_BIT);
-    DrawUiView(nullptr, head_locked_elements, render_size_headlocked_,
-               kViewportListHeadlockedOffset);
+    gvr::Mat4f identity_matrix;
+    SetIdentityM(identity_matrix);
+    DrawUiView(identity_matrix, head_locked_elements, render_size_headlocked_,
+               kViewportListHeadlockedOffset, false);
     frame.Unbind();  // Done with the headlocked buffer.
   }
 }
 
-void VrShellGl::DrawUiView(const gvr::Mat4f* head_pose,
+void VrShellGl::DrawUiView(const gvr::Mat4f& head_pose,
                            const std::vector<const ContentRectangle*>& elements,
                            const gvr::Sizei& render_size,
-                           int viewport_offset) {
+                           int viewport_offset,
+                           bool draw_cursor) {
   TRACE_EVENT0("gpu", "VrShellGl::DrawUiView");
 
-  gvr::Mat4f view_matrix;
-  if (head_pose) {
-    view_matrix = *head_pose;
-  } else {
-    SetIdentityM(view_matrix);
-  }
-  auto elementsInDrawOrder = GetElementsInDrawOrder(view_matrix, elements);
+  auto elementsInDrawOrder = GetElementsInDrawOrder(head_pose, elements);
 
   for (auto eye : {GVR_LEFT_EYE, GVR_RIGHT_EYE}) {
     buffer_viewport_list_->GetBufferViewport(eye + viewport_offset,
                                              buffer_viewport_.get());
 
     const gvr::Mat4f eye_view_matrix =
-        MatrixMul(gvr_api_->GetEyeFromHeadMatrix(eye), view_matrix);
+        MatrixMul(gvr_api_->GetEyeFromHeadMatrix(eye), head_pose);
 
     gvr::Recti pixel_rect =
         CalculatePixelSpaceRect(render_size, buffer_viewport_->GetSourceUv());
@@ -910,16 +909,14 @@ void VrShellGl::DrawUiView(const gvr::Mat4f* head_pose,
                                             kZNear, kZFar),
                   eye_view_matrix);
 
-    DrawElements(render_matrix, eye_view_matrix, elementsInDrawOrder);
-    if (head_pose != nullptr && !web_vr_mode_) {
+    DrawElements(render_matrix, elementsInDrawOrder);
+    if (draw_cursor)

[tiborg, 2017/03/16 19:40:22]

Isn't it better, i.e. less error prone, to use curly braces even though there is
only one line in the if?

[cjgrant, 2017/03/16 21:21:51]

I prefer to, but senior reviewers keep telling me not to use braces in the
one-line case.

[amp, 2017/03/17 00:26:46]

Interesting.  The style guide says you can do either:
https://google.github.io/styleguide/cppguide.html

Even the chromium style guide doesn't say anything about it:
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md

I would probably do whatever senior reviewers say as well, but I think you could
make a case for always using braces (which I prefer as well) as long as it
didn't feel like it changed the conventions in an existing file.

[tiborg, 2017/03/17 15:13:43]

If we all prefer it why not embrace it in our code? Unless senior reviewers
would not let us through then.

[cjgrant, 2017/03/20 15:34:57]

We're the owners, so we can go either way.  And, we already mix styles in this
module.  But I like braces too, so lets go for it.

       DrawCursor(render_matrix);
-    }
   }
 }
 
 void VrShellGl::DrawElements(
     const gvr::Mat4f& view_proj_matrix,
-    const gvr::Mat4f& view_matrix,
     const std::vector<const ContentRectangle*>& elements) {
   for (const auto* rect : elements) {
     gvr::Mat4f transform = MatrixMul(view_proj_matrix, rect->TransformMatrix());
@@ -1074,6 +1071,10 @@ void VrShellGl::DrawCursor(const gvr::Mat4f& render_matrix) {
   }
 }
 
+bool VrShellGl::ShouldDrawWebVr() {
+  return web_vr_mode_ && scene_->GetWebVrRenderingEnabled();
+}
+
 void VrShellGl::DrawWebVr() {
   TRACE_EVENT0("gpu", "VrShellGl::DrawWebVr");
   // Don't need face culling, depth testing, blending, etc. Turn it all off.
@@ -1190,7 +1191,7 @@ void VrShellGl::OnVSync() {
     pending_vsync_ = true;
     pending_time_ = time;
   }
-  if (!web_vr_mode_) {
+  if (!ShouldDrawWebVr()) {
     DrawFrame(-1);
   }
 }