Minor cleanups for WebXr //component

This change has a few cleanups that were discovered or postponed in the
process of moving the ArCoreInstallHelper to //components. Primarily:

1) Use a delegate rather than wrapper pattern for supplying the
   installer with an InfoBarManager.
2) Move android_vr_utils to //components/webxr/android, where the logic
   can be shared, thus reducing code duplication.
3) Add class-level comments to install types.

Change-Id: I4a4273b51a2b99bec5f2c899b082690d5c4a44f9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2463768
Commit-Queue: Alexander Cooper <alcooper@chromium.org>
Auto-Submit: Alexander Cooper <alcooper@chromium.org>
Reviewed-by: Piotr Bialecki <bialpio@chromium.org>
Cr-Commit-Position: refs/heads/master@{#816369}

chrome/browser/BUILD.gn

@@ -6075,7 +6075,10 @@ static_library("browser") {
 
     if (is_android) {
       if (enable_arcore) {
-        deps += [ "//device/vr/android/arcore" ]
+        deps += [
+          "//components/webxr/android",
+          "//device/vr/android/arcore",
+        ]
       }
     } else {
       deps += [ "//device/vr/public/mojom" ]

chrome/browser/android/vr/BUILD.gn

@@ -16,8 +16,6 @@ static_library("vr_android") {
   sources = [
     "android_ui_gesture_target.cc",
     "android_ui_gesture_target.h",
-    "android_vr_utils.cc",
-    "android_vr_utils.h",
     "android_vsync_helper.cc",
     "android_vsync_helper.h",
     "autocomplete_controller.cc",
@@ -70,8 +68,6 @@ static_library("vr_android") {
       "arcore_device/arcore_device_utils.cc",
       "arcore_device/arcore_java_utils.cc",
       "arcore_device/arcore_java_utils.h",
-      "chrome_arcore_install_helper.cc",
-      "chrome_arcore_install_helper.h",
     ]
   }
 
@@ -94,6 +90,7 @@ static_library("vr_android") {
     "//components/search_engines:search_engines",
     "//components/webxr:webxr",
     "//components/webxr/android",
+    "//components/webxr/android:android_utils",
     "//content/public/browser",
     "//content/public/common",
     "//device/vr",

chrome/browser/android/vr/arcore_device/arcore_java_utils.cc

@@ -8,8 +8,8 @@
 #include <utility>
 
 #include "base/android/jni_string.h"
-#include "chrome/browser/android/vr/android_vr_utils.h"
 #include "chrome/browser/android/vr/ar_jni_headers/ArCoreJavaUtils_jni.h"
+#include "components/webxr/android/webxr_utils.h"
 #include "device/vr/android/arcore/arcore_shim.h"
 
 using base::android::AttachCurrentThread;
@@ -49,7 +49,8 @@ void ArCoreJavaUtils::RequestArSession(
 
   Java_ArCoreJavaUtils_startSession(
       env, j_arcore_java_utils_,
-      GetJavaWebContents(render_process_id, render_frame_id), use_overlay);
+      webxr::GetJavaWebContents(render_process_id, render_frame_id),
+      use_overlay);
 }
 
 void ArCoreJavaUtils::EndSession() {

chrome/browser/android/vr/chrome_arcore_install_helper.cc

-// Copyright 2020 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/chrome_arcore_install_helper.h"
-
-#include "chrome/browser/android/vr/android_vr_utils.h"
-#include "chrome/browser/infobars/infobar_service.h"
-#include "components/webxr/android/arcore_install_helper.h"
-
-using base::android::AttachCurrentThread;
-
-namespace vr {
-ChromeArCoreInstallHelper::ChromeArCoreInstallHelper() = default;
-ChromeArCoreInstallHelper::~ChromeArCoreInstallHelper() = default;
-
-void ChromeArCoreInstallHelper::EnsureInstalled(
-    int render_process_id,
-    int render_frame_id,
-    base::OnceCallback<void(bool)> install_callback) {
-  auto* infobar_manager = InfoBarService::FromWebContents(
-      GetWebContents(render_process_id, render_frame_id));
-  DCHECK(infobar_manager);
-  arcore_install_helper_.EnsureInstalled(render_process_id, render_frame_id,
-                                         infobar_manager,
-                                         std::move(install_callback));
-}
-
-}  // namespace vr

chrome/browser/android/vr/chrome_arcore_install_helper.h

-// Copyright 2020 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_CHROME_ARCORE_INSTALL_HELPER_H_
-#define CHROME_BROWSER_ANDROID_VR_CHROME_ARCORE_INSTALL_HELPER_H_
-
-#include "base/callback.h"
-#include "chrome/browser/vr/vr_export.h"
-#include "components/webxr/android/arcore_install_helper.h"
-#include "content/public/browser/xr_install_helper.h"
-
-namespace vr {
-
-// The Actual ArCoreInstallHelper needs an InfoBarManager interface, which
-// content/browser is unable to provide, as it has no means of accessing the
-// embedder specific factory method, to that end we make a small wrapper class
-// to extract the relevant InfoBarManager.
-class VR_EXPORT ChromeArCoreInstallHelper : public content::XrInstallHelper {
- public:
-  ChromeArCoreInstallHelper();
-  ~ChromeArCoreInstallHelper() override;
-  ChromeArCoreInstallHelper(const ChromeArCoreInstallHelper&) = delete;
-  ChromeArCoreInstallHelper& operator=(const ChromeArCoreInstallHelper&) =
-      delete;
-  void EnsureInstalled(
-      int render_process_id,
-      int render_frame_id,
-      base::OnceCallback<void(bool)> install_callback) override;
-
- private:
-  webxr::ArCoreInstallHelper arcore_install_helper_;
-};
-
-}  // namespace vr
-
-#endif  // CHROME_BROWSER_ANDROID_VR_CHROME_ARCORE_INSTALL_HELPER_H_

chrome/browser/android/vr/vrcore_install_helper.cc

@@ -9,8 +9,8 @@
 
 #include "base/bind.h"
 #include "chrome/android/features/vr/jni_headers/VrCoreInstallUtils_jni.h"
-#include "chrome/browser/android/vr/android_vr_utils.h"
 #include "chrome/browser/android/vr/vr_module_provider.h"
+#include "components/webxr/android/webxr_utils.h"
 
 using base::android::AttachCurrentThread;
 
@@ -65,7 +65,7 @@ void VrCoreInstallHelper::EnsureInstalled(
     // When completed, java will call: OnInstallResult
     Java_VrCoreInstallUtils_requestInstallVrCore(
         env, java_install_utils_,
-        GetJavaWebContents(render_process_id, render_frame_id));
+        webxr::GetJavaWebContents(render_process_id, render_frame_id));
     return;
   }
 

chrome/browser/vr/DEPS

@@ -4,6 +4,7 @@ include_rules = [
   "+cc/test",
   "+cc/trees",
   "+chrome/android/features/vr/jni_headers",
+  "+components/webxr",
   "+device/vr",
 ]
 

chrome/browser/vr/chrome_xr_integration_client.cc

@@ -23,13 +23,34 @@
 #include "chrome/browser/android/vr/gvr_install_helper.h"
 #include "device/vr/android/gvr/gvr_device_provider.h"
 #if BUILDFLAG(ENABLE_ARCORE)
-#include "chrome/browser/android/vr/chrome_arcore_install_helper.h"
+#include "chrome/browser/infobars/infobar_service.h"
+#include "components/webxr/android/arcore_install_helper.h"
+#include "components/webxr/android/xr_install_helper_delegate.h"
 #include "device/vr/android/arcore/arcore_device_provider_factory.h"
 #endif  // ENABLE_ARCORE
 #endif  // OS_WIN/OS_ANDROID
 
 namespace vr {
 
+// Note that this doesn't technically need to be behind this buildflag, but
+// ArCore is the only thing that's using it right now.
+#if BUILDFLAG(ENABLE_ARCORE)
+class ChromeXrInstallHelperDelegate : public webxr::XrInstallHelperDelegate {
+ public:
+  ChromeXrInstallHelperDelegate() = default;
+  ~ChromeXrInstallHelperDelegate() override = default;
+
+  ChromeXrInstallHelperDelegate(const ChromeXrInstallHelperDelegate&) = delete;
+  ChromeXrInstallHelperDelegate& operator=(
+      const ChromeXrInstallHelperDelegate&) = delete;
+
+  infobars::InfoBarManager* GetInfoBarManager(
+      content::WebContents* web_contents) override {
+    return InfoBarService::FromWebContents(web_contents);
+  }
+};
+#endif
+
 std::unique_ptr<content::XrInstallHelper>
 ChromeXrIntegrationClient::GetInstallHelper(
     device::mojom::XRDeviceId device_id) {
@@ -39,7 +60,8 @@ ChromeXrIntegrationClient::GetInstallHelper(
       return std::make_unique<GvrInstallHelper>();
 #if BUILDFLAG(ENABLE_ARCORE)
     case device::mojom::XRDeviceId::ARCORE_DEVICE_ID:
-      return std::make_unique<ChromeArCoreInstallHelper>();
+      return std::make_unique<webxr::ArCoreInstallHelper>(
+          std::make_unique<ChromeXrInstallHelperDelegate>());
 #endif  // ENABLE_ARCORE
 #endif  // OS_ANDROID
     default:

components/webxr/android/BUILD.gn

@@ -7,8 +7,21 @@ import("//device/vr/buildflags/buildflags.gni")
 
 assert(enable_vr)
 
+source_set("android_utils") {
+  sources = [
+    "webxr_utils.cc",
+    "webxr_utils.h",
+  ]
+
+  deps = [
+    "//base",
+    "//content/public/browser",
+  ]
+}
+
 source_set("android") {
   sources = [
+    "xr_install_helper_delegate.h",
     "xr_install_infobar.cc",
     "xr_install_infobar.h",
   ]
@@ -21,6 +34,7 @@ source_set("android") {
   }
 
   deps = [
+    ":android_utils",
     "//base",
     "//components/infobars/core",
     "//components/resources:android_resources",

components/webxr/android/arcore_install_helper.cc

@@ -15,6 +15,8 @@
 #include "components/resources/android/theme_resources.h"
 #include "components/strings/grit/components_strings.h"
 #include "components/webxr/android/ar_jni_headers/ArCoreInstallUtils_jni.h"
+#include "components/webxr/android/webxr_utils.h"
+#include "components/webxr/android/xr_install_helper_delegate.h"
 #include "components/webxr/android/xr_install_infobar.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/web_contents.h"
@@ -22,29 +24,12 @@
 using base::android::AttachCurrentThread;
 
 namespace webxr {
-namespace {
-content::WebContents* GetWebContents(int render_process_id,
-                                     int render_frame_id) {
-  content::RenderFrameHost* render_frame_host =
-      content::RenderFrameHost::FromID(render_process_id, render_frame_id);
-  DCHECK(render_frame_host);
-
-  content::WebContents* web_contents =
-      content::WebContents::FromRenderFrameHost(render_frame_host);
-  DCHECK(web_contents);
-
-  return web_contents;
-}
 
-base::android::ScopedJavaLocalRef<jobject> GetJavaWebContents(
-    int render_process_id,
-    int render_frame_id) {
-  return GetWebContents(render_process_id, render_frame_id)
-      ->GetJavaWebContents();
-}
-}  // namespace
+ArCoreInstallHelper::ArCoreInstallHelper(
+    std::unique_ptr<XrInstallHelperDelegate> install_delegate)
+    : install_delegate_(std::move(install_delegate)) {
+  DCHECK(install_delegate_);
 
-ArCoreInstallHelper::ArCoreInstallHelper() {
   // As per documentation, it's recommended to issue a call to
   // ArCoreApk.checkAvailability() early in application lifecycle & ignore the
   // result so that subsequent calls can return cached result:
@@ -71,7 +56,6 @@ ArCoreInstallHelper::~ArCoreInstallHelper() {
 void ArCoreInstallHelper::EnsureInstalled(
     int render_process_id,
     int render_frame_id,
-    infobars::InfoBarManager* infobar_manager,
     base::OnceCallback<void(bool)> install_callback) {
   DCHECK(!install_finished_callback_);
   install_finished_callback_ = std::move(install_callback);
@@ -84,7 +68,7 @@ void ArCoreInstallHelper::EnsureInstalled(
   // ARCore is not installed or requires an update.
   if (Java_ArCoreInstallUtils_shouldRequestInstallSupportedArCore(
           AttachCurrentThread())) {
-    ShowInfoBar(render_process_id, render_frame_id, infobar_manager);
+    ShowInfoBar(render_process_id, render_frame_id);
     return;
   }
 
@@ -93,10 +77,12 @@ void ArCoreInstallHelper::EnsureInstalled(
   OnRequestInstallSupportedArCoreResult(nullptr, true);
 }
 
-void ArCoreInstallHelper::ShowInfoBar(
-    int render_process_id,
-    int render_frame_id,
-    infobars::InfoBarManager* infobar_manager) {
+void ArCoreInstallHelper::ShowInfoBar(int render_process_id,
+                                      int render_frame_id) {
+  infobars::InfoBarManager* infobar_manager =
+      install_delegate_->GetInfoBarManager(
+          GetWebContents(render_process_id, render_frame_id));
+
   // We can't show an infobar without an |infobar_manager|, so if it's null,
   // report that we are not installed and stop processing.
   if (!infobar_manager) {

components/webxr/android/arcore_install_helper.h

@@ -5,16 +5,17 @@
 #ifndef COMPONENTS_WEBXR_ANDROID_ARCORE_INSTALL_HELPER_H_
 #define COMPONENTS_WEBXR_ANDROID_ARCORE_INSTALL_HELPER_H_
 
+#include <memory>
+
 #include "base/android/jni_android.h"
 #include "base/android/scoped_java_ref.h"
 #include "base/callback.h"
 #include "base/memory/weak_ptr.h"
-
-namespace infobars {
-class InfoBarManager;
-}
+#include "content/public/browser/xr_install_helper.h"
 
 namespace webxr {
+class XrInstallHelperDelegate;
+
 // Equivalent of ArCoreApk.Availability enum.
 // For detailed description, please see:
 // https://developers.google.com/ar/reference/java/arcore/reference/com/google/ar/core/ArCoreApk.Availability
@@ -29,26 +30,30 @@ enum class ArCoreAvailability : int {
   kUnsupportedDeviceNotCapable = 6,
 };
 
-class ArCoreInstallHelper {
+// Helper class to ensure that ArCore has been properly installed from the
+// store, per the ArCore Apk's installation implementation. Inherits from
+// content::XrInstallHelper so that it may be returned by the
+// XrIntegrationClient.
+class ArCoreInstallHelper : public content::XrInstallHelper {
  public:
-  ArCoreInstallHelper();
-  virtual ~ArCoreInstallHelper();
+  explicit ArCoreInstallHelper(
+      std::unique_ptr<XrInstallHelperDelegate> install_delegate);
+  ~ArCoreInstallHelper() override;
 
   ArCoreInstallHelper(const ArCoreInstallHelper&) = delete;
   ArCoreInstallHelper& operator=(const ArCoreInstallHelper&) = delete;
 
-  void EnsureInstalled(int render_process_id,
-                       int render_frame_id,
-                       infobars::InfoBarManager* infobar_manager,
-                       base::OnceCallback<void(bool)> install_callback);
+  // content::XrInstallHelper implementation.
+  void EnsureInstalled(
+      int render_process_id,
+      int render_frame_id,
+      base::OnceCallback<void(bool)> install_callback) override;
 
   // Called from Java end.
   void OnRequestInstallSupportedArCoreResult(JNIEnv* env, bool success);
 
  private:
-  void ShowInfoBar(int render_process_id,
-                   int render_frame_id,
-                   infobars::InfoBarManager* infobar_manager);
+  void ShowInfoBar(int render_process_id, int render_frame_id);
   void OnInfoBarResponse(int render_process_id,
                          int render_frame_id,
                          bool try_install);
@@ -56,6 +61,7 @@ class ArCoreInstallHelper {
 
   base::OnceCallback<void(bool)> install_finished_callback_;
   base::android::ScopedJavaGlobalRef<jobject> java_install_utils_;
+  std::unique_ptr<XrInstallHelperDelegate> install_delegate_;
 
   // Must be last.
   base::WeakPtrFactory<ArCoreInstallHelper> weak_ptr_factory_{this};

chrome/browser/android/vr/android_vr_utils.cc → components/webxr/android/webxr_utils.cc

@@ -2,12 +2,12 @@
 // 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/android_vr_utils.h"
+#include "components/webxr/android/webxr_utils.h"
 
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/web_contents.h"
 
-namespace vr {
+namespace webxr {
 
 content::WebContents* GetWebContents(int render_process_id,
                                      int render_frame_id) {
@@ -29,4 +29,4 @@ base::android::ScopedJavaLocalRef<jobject> GetJavaWebContents(
       ->GetJavaWebContents();
 }
 
-}  // namespace vr
+}  // namespace webxr

chrome/browser/android/vr/android_vr_utils.h → components/webxr/android/webxr_utils.h

@@ -2,8 +2,8 @@
 // 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_ANDROID_VR_UTILS_H_
-#define CHROME_BROWSER_ANDROID_VR_ANDROID_VR_UTILS_H_
+#ifndef COMPONENTS_WEBXR_ANDROID_WEBXR_UTILS_H_
+#define COMPONENTS_WEBXR_ANDROID_WEBXR_UTILS_H_
 
 #include "base/android/jni_android.h"
 
@@ -13,8 +13,8 @@ class WebContents;
 
 // Functions in this file are currently GVR/ArCore specific functions. If other
 // platforms need the same function here, please move it to
-// chrome/browser/vr/*util.cc|h
-namespace vr {
+// components/webxr/*util.cc|h
+namespace webxr {
 
 content::WebContents* GetWebContents(int render_process_id,
                                      int render_frame_id);
@@ -23,6 +23,6 @@ base::android::ScopedJavaLocalRef<jobject> GetJavaWebContents(
     int render_process_id,
     int render_frame_id);
 
-}  // namespace vr
+}  // namespace webxr
 
-#endif  // CHROME_BROWSER_ANDROID_VR_ANDROID_VR_UTILS_H_
+#endif  // COMPONENTS_WEBXR_ANDROID_WEBXR_UTILS_H_

components/webxr/android/xr_install_helper_delegate.h

+// Copyright 2020 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 COMPONENTS_WEBXR_ANDROID_XR_INSTALL_HELPER_DELEGATE_H_
+#define COMPONENTS_WEBXR_ANDROID_XR_INSTALL_HELPER_DELEGATE_H_
+
+namespace content {
+class WebContents;
+}
+
+namespace infobars {
+class InfoBarManager;
+}
+
+namespace webxr {
+
+// Helper class for InstallHelpers to gain access to other components. Since the
+// InstallHelpers are intended to be plumbed through content/public's
+// XrIntegrationClient, they may not contain direct references to other
+// component types. This provides a means for embedders to pass accessors for
+// other component types to the InstallHelpers.
+class XrInstallHelperDelegate {
+ public:
+  virtual ~XrInstallHelperDelegate() = default;
+
+  XrInstallHelperDelegate(const XrInstallHelperDelegate&) = delete;
+  XrInstallHelperDelegate& operator=(const XrInstallHelperDelegate&) = delete;
+
+  // Gets the InfoBarManager for the provided web_contents, which can be used to
+  // display the installation confirmation message. It is acceptable to return
+  // a null InfoBarManager, but installation may fail after doing so.
+  virtual infobars::InfoBarManager* GetInfoBarManager(
+      content::WebContents* web_contents) = 0;
+
+ protected:
+  XrInstallHelperDelegate() = default;
+};
+
+}  // namespace webxr
+
+#endif  // COMPONENTS_WEBXR_ANDROID_XR_INSTALL_HELPER_DELEGATE_H_

components/webxr/android/xr_install_infobar.h

@@ -16,6 +16,9 @@ namespace webxr {
 // control a ConfirmInfoBar.
 class XrInstallInfoBar : public ConfirmInfoBarDelegate {
  public:
+  // Constructor for XrInstallInfoBar, the callback is guaranteed to be called,
+  // if the InfoBar is accepted, cancelled, or dismissed. The Callback will be
+  // passed a bool indicating whether the result of the InfoBar was "accepted."
   XrInstallInfoBar(InfoBarIdentifier identifier,
                    int icon_id,
                    int message_id,