Separate VR DFM install logic from Consent Flow

Currently GvrConsentHelper also ensures that the VR DFM is or gets
installed if needed. However, the permissions API bypasses this flow. In
order to enable the Permissions API logic by default, this logic needs
to be separated out.

This mimics the pattern of the ArCoreInstallHelper; however, there is an
outstanding bug which blocks installing VrCore at this time. That will
be addressed in a separate, future, change.

Also drops the FPS timer in gvr_scheduler_delegate down to DVLOG(2), as
it's rate of logging proved a bit difficult to debug during development.

Fixed: 1043223
Change-Id: I2475403b4df62fa2b9db0ca15642be2ff7d6b4ea
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2036643
Auto-Submit: Alexander Cooper <alcooper@chromium.org>
Reviewed-by: Klaus Weidner <klausw@chromium.org>
Commit-Queue: Klaus Weidner <klausw@chromium.org>
Commit-Queue: Alexander Cooper <alcooper@chromium.org>
Cr-Commit-Position: refs/heads/master@{#738046}

chrome/browser/android/vr/BUILD.gn

@@ -33,6 +33,8 @@ static_library("vr_android") {
     "gvr_graphics_delegate.h",
     "gvr_input_delegate.cc",
     "gvr_input_delegate.h",
+    "gvr_install_helper.cc",
+    "gvr_install_helper.h",
     "gvr_keyboard_delegate.cc",
     "gvr_keyboard_delegate.h",
     "gvr_keyboard_shim.cc",

chrome/browser/android/vr/gvr_consent_helper.cc

@@ -6,10 +6,6 @@
 
 #include <utility>
 
-#include "base/android/jni_string.h"
-#include "base/bind.h"
-#include "base/callback.h"
-#include "base/macros.h"
 #include "chrome/android/features/vr/jni_headers/VrConsentDialog_jni.h"
 #include "chrome/browser/android/vr/android_vr_utils.h"
 
@@ -32,14 +28,12 @@ void GvrConsentHelper::ShowConsentPrompt(
     OnUserConsentCallback on_user_consent_callback) {
   DCHECK(!on_user_consent_callback_);
   on_user_consent_callback_ = std::move(on_user_consent_callback);
-  render_process_id_ = render_process_id;
-  render_frame_id_ = render_frame_id;
   consent_level_ = consent_level;
 
   JNIEnv* env = AttachCurrentThread();
   jdelegate_ = Java_VrConsentDialog_promptForUserConsent(
       env, reinterpret_cast<jlong>(this),
-      GetTabFromRenderer(render_process_id_, render_frame_id_),
+      GetTabFromRenderer(render_process_id, render_frame_id),
       static_cast<jint>(consent_level));
   if (jdelegate_.is_null()) {
     std::move(on_user_consent_callback_).Run(consent_level_, false);
@@ -49,43 +43,8 @@ void GvrConsentHelper::ShowConsentPrompt(
 
 void GvrConsentHelper::OnUserConsentResult(JNIEnv* env, jboolean is_granted) {
   jdelegate_.Reset();
-  if (!on_user_consent_callback_)
-    return;
-
-  if (!is_granted) {
-    std::move(on_user_consent_callback_).Run(consent_level_, false);
-    return;
-  }
-
-  // Now that we know consent was granted, check if the VRModule is installed,
-  // and install it if not. Treat failing to install the module the same as if
-  // consent were denied.
-  if (!module_delegate_) {
-    module_delegate_ = VrModuleProviderFactory::CreateModuleProvider(
-        render_process_id_, render_frame_id_);
-  }
-
-  if (!module_delegate_) {
-    std::move(on_user_consent_callback_).Run(consent_level_, false);
-    return;
-  }
-
-  if (!module_delegate_->ModuleInstalled()) {
-    module_delegate_->InstallModule(base::BindOnce(
-        &GvrConsentHelper::OnModuleInstalled, weak_ptr_.GetWeakPtr()));
-    return;
-  }
-
-  std::move(on_user_consent_callback_).Run(consent_level_, true);
-}
-
-void GvrConsentHelper::OnModuleInstalled(bool success) {
-  if (!success) {
-    std::move(on_user_consent_callback_).Run(consent_level_, false);
-    return;
-  }
-
-  std::move(on_user_consent_callback_).Run(consent_level_, true);
+  if (on_user_consent_callback_)
+    std::move(on_user_consent_callback_).Run(consent_level_, is_granted);
 }
 
 }  // namespace vr

chrome/browser/android/vr/gvr_consent_helper.h

@@ -5,13 +5,9 @@
 #ifndef CHROME_BROWSER_ANDROID_VR_GVR_CONSENT_HELPER_H_
 #define CHROME_BROWSER_ANDROID_VR_GVR_CONSENT_HELPER_H_
 
-#include <jni.h>
-#include <memory>
-
 #include "base/android/jni_android.h"
 #include "base/callback.h"
 #include "base/memory/weak_ptr.h"
-#include "chrome/browser/android/vr/vr_module_provider.h"
 #include "chrome/browser/vr/service/xr_consent_helper.h"
 
 namespace vr {
@@ -30,11 +26,6 @@ class GvrConsentHelper : public XrConsentHelper {
   void OnUserConsentResult(JNIEnv* env, jboolean is_granted);
 
  private:
-  void OnModuleInstalled(bool success);
-
-  std::unique_ptr<VrModuleProvider> module_delegate_;
-  int render_process_id_;
-  int render_frame_id_;
   XrConsentPromptLevel consent_level_;
 
   OnUserConsentCallback on_user_consent_callback_;

chrome/browser/android/vr/gvr_install_helper.cc

+// Copyright 2019 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/gvr_install_helper.h"
+
+#include <utility>
+
+#include "base/bind.h"
+#include "chrome/browser/android/vr/android_vr_utils.h"
+
+using base::android::AttachCurrentThread;
+
+namespace vr {
+
+GvrInstallHelper::GvrInstallHelper() : XrInstallHelper() {}
+
+GvrInstallHelper::~GvrInstallHelper() {
+  RunInstallFinishedCallback(false);
+}
+
+void GvrInstallHelper::EnsureInstalled(
+    int render_process_id,
+    int render_frame_id,
+    OnInstallFinishedCallback install_callback) {
+  // Callers should ensure they only prompt for install once.
+  DCHECK(!install_finished_callback_);
+
+  install_finished_callback_ = std::move(install_callback);
+
+  // By ensuring that there is no outstanding callback, we also ensure that we
+  // don't care about any value that may currently be in module_delegate_.
+  // Unfortunately, the only place we have to null this value out (so that we
+  // could also DCHECK on this being null), is OnModuleInstalled. Turns out,
+  // it's a pretty bad idea to delete the object that's currently processing
+  // your callback, as it may still have outstanding work it wants to do.
+  module_delegate_ = VrModuleProviderFactory::CreateModuleProvider(
+      render_process_id, render_frame_id);
+
+  // If we failed to get the module delegate, then report failure to install.
+  if (!module_delegate_) {
+    RunInstallFinishedCallback(false);
+    return;
+  }
+
+  // If the module is already installed, then skip to the next step, asserting
+  // that install succeeded.
+  if (module_delegate_->ModuleInstalled()) {
+    OnModuleInstalled(true);
+    return;
+  }
+
+  // Prompt for module installation.
+  module_delegate_->InstallModule(base::BindOnce(
+      &GvrInstallHelper::OnModuleInstalled, weak_ptr_.GetWeakPtr()));
+}
+
+void GvrInstallHelper::OnModuleInstalled(bool success) {
+  if (!success) {
+    // If we failed to install the DFM, then abort the flow.
+    RunInstallFinishedCallback(false);
+    return;
+  }
+
+  // TODO(crbug.com/1037935): Currently when spinning up VrShellDelegate will
+  // detect that VrCore is not installed and prompt for it to be installed.
+  // However, since we don't know about it here, this causes the session to get
+  // rejected when the prompt comes up. It would be better if we could do this
+  // installation here.
+  RunInstallFinishedCallback(success);
+}
+
+void GvrInstallHelper::RunInstallFinishedCallback(bool success) {
+  if (install_finished_callback_)
+    std::move(install_finished_callback_).Run(success);
+}
+
+}  // namespace vr

chrome/browser/android/vr/gvr_install_helper.h

+// Copyright 2019 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.
+
+#ifndef CHROME_BROWSER_ANDROID_VR_GVR_INSTALL_HELPER_H_
+#define CHROME_BROWSER_ANDROID_VR_GVR_INSTALL_HELPER_H_
+
+#include <memory>
+
+#include "base/callback.h"
+#include "base/memory/weak_ptr.h"
+#include "chrome/browser/android/vr/vr_module_provider.h"
+#include "chrome/browser/vr/service/xr_install_helper.h"
+#include "chrome/browser/vr/vr_export.h"
+
+namespace vr {
+
+class VR_EXPORT GvrInstallHelper : public XrInstallHelper {
+ public:
+  GvrInstallHelper();
+  ~GvrInstallHelper() override;
+
+  GvrInstallHelper(const GvrInstallHelper&) = delete;
+  GvrInstallHelper& operator=(const GvrInstallHelper&) = delete;
+
+  void EnsureInstalled(int render_process_id,
+                       int render_frame_id,
+                       OnInstallFinishedCallback install_callback) override;
+
+ private:
+  void OnModuleInstalled(bool success);
+  void RunInstallFinishedCallback(bool succeeded);
+
+  OnInstallFinishedCallback install_finished_callback_;
+  std::unique_ptr<VrModuleProvider> module_delegate_;
+
+  base::WeakPtrFactory<GvrInstallHelper> weak_ptr_{this};
+};
+
+}  // namespace vr
+
+#endif  // CHROME_BROWSER_ANDROID_VR_GVR_INSTALL_HELPER_H_

chrome/browser/android/vr/gvr_scheduler_delegate.cc

@@ -675,7 +675,7 @@ void GvrSchedulerDelegate::DrawFrameSubmitNow(FrameType frame_type,
   // After saving the timestamp, fps will be available via GetFPS().
   // TODO(vollick): enable rendering of this framerate in a HUD.
   vr_ui_fps_meter_.AddFrame(base::TimeTicks::Now());
-  DVLOG(1) << "fps: " << vr_ui_fps_meter_.GetFPS();
+  DVLOG(2) << "fps: " << vr_ui_fps_meter_.GetFPS();
   TRACE_COUNTER1("gpu", "VR UI FPS", vr_ui_fps_meter_.GetFPS());
 
   if (frame_type == kWebXrFrame) {

chrome/browser/vr/service/browser_xr_runtime.cc

@@ -5,6 +5,7 @@
 #include "chrome/browser/vr/service/browser_xr_runtime.h"
 
 #include <algorithm>
+#include <memory>
 #include <utility>
 
 #include "base/bind_helpers.h"
@@ -25,6 +26,7 @@
 #include "chrome/browser/vr/service/xr_session_request_consent_manager.h"
 #elif defined(OS_ANDROID)
 #include "chrome/browser/android/vr/gvr_consent_helper.h"
+#include "chrome/browser/android/vr/gvr_install_helper.h"
 #if BUILDFLAG(ENABLE_ARCORE)
 #include "chrome/browser/android/vr/arcore_device/arcore_consent_prompt.h"
 #include "chrome/browser/android/vr/arcore_device/arcore_install_helper.h"
@@ -251,6 +253,7 @@ BrowserXRRuntime::BrowserXRRuntime(
 #if defined(OS_ANDROID)
   if (id_ == device::mojom::XRDeviceId::GVR_DEVICE_ID) {
     consent_helper_ = std::make_unique<GvrConsentHelper>();
+    install_helper_ = std::make_unique<GvrInstallHelper>();
   }
 #if BUILDFLAG(ENABLE_ARCORE)
   if (id_ == device::mojom::XRDeviceId::ARCORE_DEVICE_ID) {