[WebVR] Handle slow connecting devices.

device/vr/vr_service_impl.cc

Index: device/vr/vr_service_impl.cc
diff --git a/device/vr/vr_service_impl.cc b/device/vr/vr_service_impl.cc
index 81223facf67b42ffa95c448277be8149ab6201ce..5f51669c9c3ddd06ac08711ce1ead4e4485ad3c6 100644
--- a/device/vr/vr_service_impl.cc
+++ b/device/vr/vr_service_impl.cc
@@ -17,6 +17,7 @@ namespace device {
 VRServiceImpl::VRServiceImpl()
     : listening_for_activate_(false),
       connected_devices_(0),
+      handled_devices_(0),
       weak_ptr_factory_(this) {}
 
 VRServiceImpl::~VRServiceImpl() {
@@ -41,8 +42,18 @@ void VRServiceImpl::SetClient(mojom::VRServiceClientPtr service_client,
   // send ConnectDevice to it so that it's populated with the currently active
   // displays. Thereafter it will stay up to date by virtue of listening for new
   // connected events.
-  VRDeviceManager::GetInstance()->AddService(this);
-  std::move(callback).Run(connected_devices_);
+  VRDeviceManager* device_manager = VRDeviceManager::GetInstance();
+  device_manager->AddService(this);
+  unsigned expected_devices = device_manager->GetNumberOfConnectedDevices();
+  // TODO(amp): Remove this count based synchronization.
+  // If libraries are not loaded, new devices will immediatly be handled but not
+  // connect, return only those devices which have already connected.
+  // If libraries were loaded then all devices may not be handled yet so return
+  // the number we expect to eventually connect.
+  if (expected_devices == handled_devices_) {
+    expected_devices = connected_devices_;
+  }
+  std::move(callback).Run(expected_devices);

[mthiesse, 2017/06/05 16:18:07]

I'm confused. Doesn't this just re-introduce the bug where we report 1 device,
but that device never connects?

[amp, 2017/06/05 16:31:18]

Let me attempt a better explanation (let me know if you want this in the
comments).

If all the devices are immediately handled (probably only happens when we don't
have to load libraries and instantiate vr displays) then we can assume that all
devices that are going to connect have already connected and so we return that
value (line 54).  Usually this ends up being 0 and so the VRController on the
rendered side immediately resolves and returns 0 devices to the javascript side.

If we haven't handled all the devices, then something is still working on
loading or instantiating and so we return the actual number of devices that were
registered in the manager (from line 47).  Then when the device connects and
calls back (line 92) it will signal the rendered and resolve the promise when
the number of connected devices which called back equals the number passed back
here.

 }
 
 void VRServiceImpl::ConnectDevice(VRDevice* device) {
@@ -72,15 +83,16 @@ void VRServiceImpl::OnVRDisplayInfoCreated(
     return;
   }
 
-  if (!display_info) {
-    // If we get passed a null display info it means the device does not exist.
-    // This can happen for example if VR services are not installed. We will not
-    // instantiate a display in this case and don't count it as connected.
-    return;
+  // If we get passed a null display info it means the device does not exist.
+  // This can happen for example if VR services are not installed. We will not
+  // instantiate a display in this case and don't count it as connected, but we
+  // do mark that we have handled it.
+  if (display_info) {
+    displays_[device] = base::MakeUnique<VRDisplayImpl>(
+        device, this, client_.get(), std::move(display_info));
+    connected_devices_++;
   }

[mthiesse, 2017/06/05 17:36:27]

Can we DCHECK on the else case there we're inside of a SetClient call?

[amp, 2017/06/05 18:40:39]

Done.  Switched the if around (back to the original) and used an AutoReset to
verify the stack.

-  displays_[device] = base::MakeUnique<VRDisplayImpl>(
-      device, this, client_.get(), std::move(display_info));
-  connected_devices_++;
+  handled_devices_++;
 }
 
 VRDisplayImpl* VRServiceImpl::GetVRDisplayImplForTesting(VRDevice* device) {