Create interface for VRDeviceProvider::Initialize

This replaces the four callbacks that were being passed into the
VRDeviceProvider::Initialize method with a VRDeviceProviderClient
interface. This helps to improve the readability of the methods, since
some of the callbacks being passed into Initialize had quite a few
parameters and were unwieldy. Further, this makes it simpler to provide
a means for the device providers to access data should another expansion
like that with the FrameSinkClient become necessary in the future.

The use of a raw pointer being passed into Initialize which some of the
VRDeviceProviders maintain a reference to is just as safe as the
existing callback code, which used base::Unretained in it's bindings.
This change should be a purely mechanical change, switching the use of
the callbacks with direct calls onto a cached client object.

Future improvements to this area could investigate a safer pattern, but
the fact that the XRRuntimeManagerImpl is effectively a singleton and
is guaranteed to outlive the providers is a fairly safe pattern.

There are some further potential issues with the Orientation device
provider, where if there is an already initialized device, we may not
trigger the initialized callback; but it is unclear if this is a case
that can actually be hit today, and was also left as-is in the interests
of trying to keep this change mechanical.

Bug: 864026
Change-Id: I9bf7648ff202cb5f9c0765aa31de707c0a72f936
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3343629
Reviewed-by: Klaus Weidner <klausw@chromium.org>
Commit-Queue: Alexander Cooper <alcooper@chromium.org>
Cr-Commit-Position: refs/heads/main@{#952531}

components/webxr/android/arcore_device_provider.cc

@@ -19,16 +19,7 @@ ArCoreDeviceProvider::ArCoreDeviceProvider(
 
 ArCoreDeviceProvider::~ArCoreDeviceProvider() = default;
 
-void ArCoreDeviceProvider::Initialize(
-    base::RepeatingCallback<void(device::mojom::XRDeviceId,
-                                 device::mojom::VRDisplayInfoPtr,
-                                 device::mojom::XRDeviceDataPtr,
-                                 mojo::PendingRemote<device::mojom::XRRuntime>)>
-        add_device_callback,
-    base::RepeatingCallback<void(device::mojom::XRDeviceId)>
-        remove_device_callback,
-    base::OnceClosure initialization_complete,
-    device::XrFrameSinkClientFactory xr_frame_sink_client_factory) {
+void ArCoreDeviceProvider::Initialize(device::VRDeviceProviderClient* client) {
   if (device::IsArCoreSupported()) {
     DVLOG(2) << __func__ << ": ARCore is supported, creating device";
 
@@ -37,14 +28,14 @@ void ArCoreDeviceProvider::Initialize(
         std::make_unique<device::ArImageTransportFactory>(),
         std::make_unique<webxr::MailboxToSurfaceBridgeFactoryImpl>(),
         std::make_unique<webxr::ArCoreJavaUtils>(compositor_delegate_provider_),
-        std::move(xr_frame_sink_client_factory));
+        client->GetXrFrameSinkClientFactory());
 
-    add_device_callback.Run(
+    client->AddRuntime(
         arcore_device_->GetId(), arcore_device_->GetVRDisplayInfo(),
         arcore_device_->GetDeviceData(), arcore_device_->BindXRRuntime());
   }
   initialized_ = true;
-  std::move(initialization_complete).Run();
+  client->OnProviderInitialized();
 }
 
 bool ArCoreDeviceProvider::Initialized() {

components/webxr/android/arcore_device_provider.h

@@ -28,16 +28,7 @@ class ArCoreDeviceProvider : public device::VRDeviceProvider {
   ArCoreDeviceProvider& operator=(const ArCoreDeviceProvider&) = delete;
 
   ~ArCoreDeviceProvider() override;
-  void Initialize(
-      base::RepeatingCallback<void(
-          device::mojom::XRDeviceId,
-          device::mojom::VRDisplayInfoPtr,
-          device::mojom::XRDeviceDataPtr,
-          mojo::PendingRemote<device::mojom::XRRuntime>)> add_device_callback,
-      base::RepeatingCallback<void(device::mojom::XRDeviceId)>
-          remove_device_callback,
-      base::OnceClosure initialization_complete,
-      device::XrFrameSinkClientFactory xr_frame_sink_client_factory) override;
+  void Initialize(device::VRDeviceProviderClient* client) override;
   bool Initialized() override;
 
  private:

content/browser/xr/service/isolated_device_provider.cc

@@ -16,19 +16,8 @@ constexpr int kMaxRetries = 3;
 namespace content {
 
 void IsolatedVRDeviceProvider::Initialize(
-    base::RepeatingCallback<void(device::mojom::XRDeviceId,
-                                 device::mojom::VRDisplayInfoPtr,
-                                 device::mojom::XRDeviceDataPtr,
-                                 mojo::PendingRemote<device::mojom::XRRuntime>)>
-        add_device_callback,
-    base::RepeatingCallback<void(device::mojom::XRDeviceId)>
-        remove_device_callback,
-    base::OnceClosure initialization_complete,
-    device::XrFrameSinkClientFactory xr_frame_sink_client_factory) {
-  add_device_callback_ = std::move(add_device_callback);
-  remove_device_callback_ = std::move(remove_device_callback);
-  initialization_complete_ = std::move(initialization_complete);
-
+    device::VRDeviceProviderClient* client) {
+  client_ = client;
   SetupDeviceProvider();
 }
 
@@ -41,8 +30,8 @@ void IsolatedVRDeviceProvider::OnDeviceAdded(
     mojo::PendingRemote<device::mojom::XRCompositorHost> compositor_host,
     device::mojom::XRDeviceDataPtr device_data,
     device::mojom::XRDeviceId device_id) {
-  add_device_callback_.Run(device_id, nullptr, std::move(device_data),
-                           std::move(device));
+  client_->AddRuntime(device_id, nullptr, std::move(device_data),
+                      std::move(device));
 
   auto* integration_client = GetXrIntegrationClient();
   if (!integration_client)
@@ -56,7 +45,7 @@ void IsolatedVRDeviceProvider::OnDeviceAdded(
 }
 
 void IsolatedVRDeviceProvider::OnDeviceRemoved(device::mojom::XRDeviceId id) {
-  remove_device_callback_.Run(id);
+  client_->RemoveRuntime(id);
   ui_host_map_.erase(id);
 }
 
@@ -65,7 +54,7 @@ void IsolatedVRDeviceProvider::OnServerError() {
   // should be removed.
   for (auto& entry : ui_host_map_) {
     auto id = entry.first;
-    remove_device_callback_.Run(id);
+    client_->RemoveRuntime(id);
   }
   ui_host_map_.clear();
 
@@ -88,7 +77,7 @@ void IsolatedVRDeviceProvider::OnServerError() {
 void IsolatedVRDeviceProvider::OnDevicesEnumerated() {
   if (!initialized_) {
     initialized_ = true;
-    std::move(initialization_complete_).Run();
+    client_->OnProviderInitialized();
   }
 
   // Either we've hit the max retries and given up (in which case we don't have

content/browser/xr/service/isolated_device_provider.h

@@ -26,16 +26,7 @@ class IsolatedVRDeviceProvider
   ~IsolatedVRDeviceProvider() override;
 
   // If the VR API requires initialization that should happen here.
-  void Initialize(
-      base::RepeatingCallback<void(
-          device::mojom::XRDeviceId,
-          device::mojom::VRDisplayInfoPtr,
-          device::mojom::XRDeviceDataPtr,
-          mojo::PendingRemote<device::mojom::XRRuntime>)> add_device_callback,
-      base::RepeatingCallback<void(device::mojom::XRDeviceId)>
-          remove_device_callback,
-      base::OnceClosure initialization_complete,
-      device::XrFrameSinkClientFactory xr_frame_sink_client_factory) override;
+  void Initialize(device::VRDeviceProviderClient* client) override;
 
   // Returns true if initialization is complete.
   bool Initialized() override;
@@ -56,15 +47,7 @@ class IsolatedVRDeviceProvider
   int retry_count_ = 0;
   mojo::Remote<device::mojom::IsolatedXRRuntimeProvider> device_provider_;
 
-  // TODO(crbug.com/1090029): Wrap XRDeviceId + VRDisplayInfo into XRDeviceData
-  base::RepeatingCallback<void(device::mojom::XRDeviceId,
-                               device::mojom::VRDisplayInfoPtr,
-                               device::mojom::XRDeviceDataPtr,
-                               mojo::PendingRemote<device::mojom::XRRuntime>)>
-      add_device_callback_;
-  base::RepeatingCallback<void(device::mojom::XRDeviceId)>
-      remove_device_callback_;
-  base::OnceClosure initialization_complete_;
+  device::VRDeviceProviderClient* client_ = nullptr;
   mojo::Receiver<device::mojom::IsolatedXRRuntimeProviderClient> receiver_{
       this};
 

content/browser/xr/service/xr_runtime_manager_impl.cc

@@ -503,14 +503,10 @@ void XRRuntimeManagerImpl::InitializeProviders() {
       continue;
     }
 
-    provider->Initialize(
-        base::BindRepeating(&XRRuntimeManagerImpl::AddRuntime,
-                            base::Unretained(this)),
-        base::BindRepeating(&XRRuntimeManagerImpl::RemoveRuntime,
-                            base::Unretained(this)),
-        base::BindOnce(&XRRuntimeManagerImpl::OnProviderInitialized,
-                       base::Unretained(this)),
-        base::BindRepeating(&FrameSinkClientFactory));
+    // It is acceptable for the providers to potentially take/keep a reference
+    // to ourselves here, since we own the providers and can guarantee that they
+    // will not outlive us.
+    provider->Initialize(this);
   }
 
   providers_initialized_ = true;
@@ -573,6 +569,11 @@ void XRRuntimeManagerImpl::RemoveRuntime(device::mojom::XRDeviceId id) {
     service->RuntimesChanged();
 }
 
+device::XrFrameSinkClientFactory
+XRRuntimeManagerImpl::GetXrFrameSinkClientFactory() {
+  return base::BindRepeating(&FrameSinkClientFactory);
+}
+
 void XRRuntimeManagerImpl::ForEachRuntime(
     base::RepeatingCallback<void(BrowserXRRuntime*)> fn) {
   for (auto& runtime : runtimes_) {

content/browser/xr/service/xr_runtime_manager_impl.h

@@ -23,6 +23,7 @@
 #include "content/public/browser/gpu_data_manager_observer.h"
 #include "content/public/browser/xr_integration_client.h"
 #include "content/public/browser/xr_runtime_manager.h"
+#include "device/vr/public/cpp/vr_device_provider.h"
 #include "device/vr/public/mojom/vr_service.mojom-forward.h"
 #include "mojo/public/cpp/bindings/pending_remote.h"
 
@@ -38,7 +39,8 @@ class XRRuntimeManagerTest;
 class CONTENT_EXPORT XRRuntimeManagerImpl
     : public XRRuntimeManager,
       public base::RefCounted<XRRuntimeManagerImpl>,
-      public content::GpuDataManagerObserver {
+      public content::GpuDataManagerObserver,
+      public device::VRDeviceProviderClient {
  public:
   friend base::RefCounted<XRRuntimeManagerImpl>;
   static constexpr auto kRefCountPreference =
@@ -92,6 +94,16 @@ class CONTENT_EXPORT XRRuntimeManagerImpl
   void ForEachRuntime(
       base::RepeatingCallback<void(BrowserXRRuntime*)> fn) override;
 
+  // VRDeviceProviderClient implementation
+  void AddRuntime(
+      device::mojom::XRDeviceId id,
+      device::mojom::VRDisplayInfoPtr info,
+      device::mojom::XRDeviceDataPtr device_data,
+      mojo::PendingRemote<device::mojom::XRRuntime> runtime) override;
+  void RemoveRuntime(device::mojom::XRDeviceId id) override;
+  void OnProviderInitialized() override;
+  device::XrFrameSinkClientFactory GetXrFrameSinkClientFactory() override;
+
  private:
   // Constructor also used by tests to supply an arbitrary list of providers
   static scoped_refptr<XRRuntimeManagerImpl> CreateInstance(
@@ -108,15 +120,8 @@ class CONTENT_EXPORT XRRuntimeManagerImpl
   ~XRRuntimeManagerImpl() override;
 
   void InitializeProviders();
-  void OnProviderInitialized();
   bool AreAllProvidersInitialized();
 
-  void AddRuntime(device::mojom::XRDeviceId id,
-                  device::mojom::VRDisplayInfoPtr info,
-                  device::mojom::XRDeviceDataPtr device_data,
-                  mojo::PendingRemote<device::mojom::XRRuntime> runtime);
-  void RemoveRuntime(device::mojom::XRDeviceId id);
-
   bool IsInitializedOnCompatibleAdapter(BrowserXRRuntimeImpl* runtime);
 
   // Gets the system default immersive-vr runtime if available.

device/vr/android/gvr/gvr_device_provider.cc

@@ -14,28 +14,20 @@ namespace device {
 GvrDeviceProvider::GvrDeviceProvider() = default;
 GvrDeviceProvider::~GvrDeviceProvider() = default;
 
-void GvrDeviceProvider::Initialize(
-    base::RepeatingCallback<void(mojom::XRDeviceId,
-                                 mojom::VRDisplayInfoPtr,
-                                 mojom::XRDeviceDataPtr,
-                                 mojo::PendingRemote<mojom::XRRuntime>)>
-        add_device_callback,
-    base::RepeatingCallback<void(mojom::XRDeviceId)> remove_device_callback,
-    base::OnceClosure initialization_complete,
-    XrFrameSinkClientFactory xr_frame_sink_client_factory) {
+void GvrDeviceProvider::Initialize(VRDeviceProviderClient* client) {
   // We only expose GvrDevice if
   //  - we could potentially install VRServices to support presentation, and
   //  - this build is a bundle and, thus, supports installing the VR module.
   if (base::android::BundleUtils::IsBundle()) {
     vr_device_ = base::WrapUnique(new GvrDevice());
   }
   if (vr_device_) {
-    add_device_callback.Run(vr_device_->GetId(), vr_device_->GetVRDisplayInfo(),
-                            vr_device_->GetDeviceData(),
-                            vr_device_->BindXRRuntime());
+    client->AddRuntime(vr_device_->GetId(), vr_device_->GetVRDisplayInfo(),
+                       vr_device_->GetDeviceData(),
+                       vr_device_->BindXRRuntime());
   }
   initialized_ = true;
-  std::move(initialization_complete).Run();
+  client->OnProviderInitialized();
 }
 
 bool GvrDeviceProvider::Initialized() {

device/vr/android/gvr/gvr_device_provider.h

@@ -24,16 +24,7 @@ class DEVICE_VR_EXPORT GvrDeviceProvider : public VRDeviceProvider {
 
   ~GvrDeviceProvider() override;
 
-  void Initialize(
-      base::RepeatingCallback<void(mojom::XRDeviceId,
-                                   mojom::VRDisplayInfoPtr,
-                                   mojom::XRDeviceDataPtr,
-                                   mojo::PendingRemote<mojom::XRRuntime>)>
-          add_device_callback,
-      base::RepeatingCallback<void(mojom::XRDeviceId)> remove_device_callback,
-      base::OnceClosure initialization_complete,
-      XrFrameSinkClientFactory xr_frame_sink_client_factory) override;
-
+  void Initialize(VRDeviceProviderClient* client) override;
   bool Initialized() override;
 
  private:

device/vr/orientation/orientation_device_provider.cc

@@ -19,28 +19,19 @@ VROrientationDeviceProvider::VROrientationDeviceProvider(
 
 VROrientationDeviceProvider::~VROrientationDeviceProvider() = default;
 
-void VROrientationDeviceProvider::Initialize(
-    base::RepeatingCallback<void(mojom::XRDeviceId,
-                                 mojom::VRDisplayInfoPtr,
-                                 mojom::XRDeviceDataPtr,
-                                 mojo::PendingRemote<mojom::XRRuntime>)>
-        add_device_callback,
-    base::RepeatingCallback<void(mojom::XRDeviceId)> remove_device_callback,
-    base::OnceClosure initialization_complete,
-    XrFrameSinkClientFactory xr_frame_sink_client_factory) {
+void VROrientationDeviceProvider::Initialize(VRDeviceProviderClient* client) {
   if (device_ && device_->IsAvailable()) {
-    add_device_callback.Run(device_->GetId(), device_->GetVRDisplayInfo(),
-                            device_->GetDeviceData(), device_->BindXRRuntime());
+    client->AddRuntime(device_->GetId(), device_->GetVRDisplayInfo(),
+                       device_->GetDeviceData(), device_->BindXRRuntime());
     return;
   }
 
   if (!device_) {
+    client_ = client;
     device_ = std::make_unique<VROrientationDevice>(
         sensor_provider_.get(),
         base::BindOnce(&VROrientationDeviceProvider::DeviceInitialized,
                        base::Unretained(this)));
-    add_device_callback_ = add_device_callback;
-    initialized_callback_ = std::move(initialization_complete);
   }
 }
 
@@ -56,13 +47,12 @@ void VROrientationDeviceProvider::DeviceInitialized() {
 
   // If the device successfully connected to the orientation APIs, provide it.
   if (device_->IsAvailable()) {
-    add_device_callback_.Run(device_->GetId(), device_->GetVRDisplayInfo(),
-                             device_->GetDeviceData(),
-                             device_->BindXRRuntime());
+    client_->AddRuntime(device_->GetId(), device_->GetVRDisplayInfo(),
+                        device_->GetDeviceData(), device_->BindXRRuntime());
   }
 
   initialized_ = true;
-  std::move(initialized_callback_).Run();
+  client_->OnProviderInitialized();
 }
 
 }  // namespace device

device/vr/orientation/orientation_device_provider.h

@@ -29,15 +29,7 @@ class COMPONENT_EXPORT(VR_ORIENTATION) VROrientationDeviceProvider
 
   ~VROrientationDeviceProvider() override;
 
-  void Initialize(
-      base::RepeatingCallback<void(mojom::XRDeviceId,
-                                   mojom::VRDisplayInfoPtr,
-                                   mojom::XRDeviceDataPtr,
-                                   mojo::PendingRemote<mojom::XRRuntime>)>
-          add_device_callback,
-      base::RepeatingCallback<void(mojom::XRDeviceId)> remove_device_callback,
-      base::OnceClosure initialization_complete,
-      XrFrameSinkClientFactory xr_frame_sink_client_factory) override;
+  void Initialize(VRDeviceProviderClient* client) override;
 
   bool Initialized() override;
 
@@ -49,13 +41,7 @@ class COMPONENT_EXPORT(VR_ORIENTATION) VROrientationDeviceProvider
   mojo::Remote<device::mojom::SensorProvider> sensor_provider_;
 
   std::unique_ptr<VROrientationDevice> device_;
-
-  base::RepeatingCallback<void(mojom::XRDeviceId,
-                               mojom::VRDisplayInfoPtr,
-                               mojom::XRDeviceDataPtr,
-                               mojo::PendingRemote<mojom::XRRuntime>)>
-      add_device_callback_;
-  base::OnceClosure initialized_callback_;
+  VRDeviceProviderClient* client_ = nullptr;
 };
 
 }  // namespace device