mojo VR interface simplified

device/vr/android/gvr/gvr_device.cc

Index: device/vr/android/gvr/gvr_device.cc
diff --git a/device/vr/android/gvr/gvr_device.cc b/device/vr/android/gvr/gvr_device.cc
index 9238409817038e7f9ada4174f54f00923ae0bab6..b72693d5400eed49a7c4ad3ccaeece267322b4a1 100644
--- a/device/vr/android/gvr/gvr_device.cc
+++ b/device/vr/android/gvr/gvr_device.cc
@@ -29,26 +29,26 @@ GvrDevice::GvrDevice(GvrDeviceProvider* provider, GvrDelegate* delegate)
 
 GvrDevice::~GvrDevice() {}
 
-VRDisplayPtr GvrDevice::GetVRDevice() {
+mojom::VRDisplayInfoPtr GvrDevice::GetVRDevice() {
   TRACE_EVENT0("input", "GvrDevice::GetVRDevice");
 
-  VRDisplayPtr device = VRDisplay::New();
+  mojom::VRDisplayInfoPtr device = mojom::VRDisplayInfo::New();
 
   device->index = id();
 
-  device->capabilities = VRDisplayCapabilities::New();
+  device->capabilities = mojom::VRDisplayCapabilities::New();
   device->capabilities->hasOrientation = true;
   device->capabilities->hasPosition = false;
   device->capabilities->hasExternalDisplay = false;
   device->capabilities->canPresent = true;
 
-  device->leftEye = VREyeParameters::New();
-  device->rightEye = VREyeParameters::New();
-  VREyeParametersPtr& left_eye = device->leftEye;
-  VREyeParametersPtr& right_eye = device->rightEye;
+  device->leftEye = mojom::VREyeParameters::New();
+  device->rightEye = mojom::VREyeParameters::New();
+  mojom::VREyeParametersPtr& left_eye = device->leftEye;
+  mojom::VREyeParametersPtr& right_eye = device->rightEye;
 
-  left_eye->fieldOfView = VRFieldOfView::New();
-  right_eye->fieldOfView = VRFieldOfView::New();
+  left_eye->fieldOfView = mojom::VRFieldOfView::New();
+  right_eye->fieldOfView = mojom::VRFieldOfView::New();
 
   left_eye->offset = mojo::Array<float>::New(3);
   right_eye->offset = mojo::Array<float>::New(3);
@@ -127,10 +127,13 @@ VRDisplayPtr GvrDevice::GetVRDevice() {
   return device;
 }
 
-VRPosePtr GvrDevice::GetPose() {
+mojom::VRPosePtr GvrDevice::GetPose(VRServiceImpl* service) {
   TRACE_EVENT0("input", "GvrDevice::GetSensorState");
 
-  VRPosePtr pose = VRPose::New();
+  if (!IsAccessAllowed(service))
+    return nullptr;
+
+  mojom::VRPosePtr pose = mojom::VRPose::New();
 
   pose->timestamp = base::Time::Now().ToJsTime();
 
@@ -186,39 +189,61 @@ VRPosePtr GvrDevice::GetPose() {
   return pose;
 }
 
-void GvrDevice::ResetPose() {
+void GvrDevice::ResetPose(VRServiceImpl* service) {
+  if (!IsAccessAllowed(service))
+    return;
+
   gvr::GvrApi* gvr_api = GetGvrApi();
   if (gvr_api)
     gvr_api->ResetTracking();
 }
 
-bool GvrDevice::RequestPresent(bool secure_origin) {
+bool GvrDevice::RequestPresent(VRServiceImpl* service, bool secure_origin) {
+  if (!IsAccessAllowed(service))
+    return false;
+
+  // One service could present on several devices at the same time
+  // and different service could present on different devices the same time
+  if (presenting_service_ == nullptr)
+    presenting_service_ = service;

[dcheng, 2016/11/08 08:55:16]

This isn't critical, but I personally found this a bit hard to follow: if we get
here, it's guaranteed that we're already the presenting service for this device
or there is no presenting service associated with this device.

However, the name "IsAccessAllowed(service)" doesn't really convey that concept,
but I'm not sure if I have a better idea. Maybe bajones@ will have some
thoughts.

[shaobo.yan, 2016/11/08 11:33:44]

Yes, it means that if the presenting_service_ remains nullptr or it is the
service which request access that the access request would be approved.

[dcheng, 2016/11/08 18:15:44]

Right, I'm hoping we can rename this function (in a followup) to make this
clearer: otherwise, a reader like me needs to examine two functions to
understand that the presenting_service_ == service invariant will hold after
208.

[shaobo.yan, 2016/11/09 01:11:20]

Ok, it seems that give a property name is what I really need to improve :). I'll
update it in a followup and give it a TODO here for not block this issue.

+
   secure_origin_ = secure_origin;
   if (delegate_)
     delegate_->SetWebVRSecureOrigin(secure_origin_);
+
   return gvr_provider_->RequestPresent();
 }
 
-void GvrDevice::ExitPresent() {
+void GvrDevice::ExitPresent(VRServiceImpl* service) {
+  if (IsPresentingService(service))
+    presenting_service_ = nullptr;
+
   gvr_provider_->ExitPresent();
+  OnExitPresent(service);
 }
 
-void GvrDevice::SubmitFrame(VRPosePtr pose) {
-  if (delegate_)
-    delegate_->SubmitWebVRFrame();
+void GvrDevice::SubmitFrame(VRServiceImpl* service, mojom::VRPosePtr pose) {
+  if (IsPresentingService(service)) {
+    if (delegate_)
+      delegate_->SubmitWebVRFrame();
+  }
 }
 
-void GvrDevice::UpdateLayerBounds(VRLayerBoundsPtr leftBounds,
-                                  VRLayerBoundsPtr rightBounds) {
-  if (!delegate_)
-    return;
-
-  delegate_->UpdateWebVRTextureBounds(0,  // Left eye
-                                      leftBounds->left, leftBounds->top,
-                                      leftBounds->width, leftBounds->height);
-  delegate_->UpdateWebVRTextureBounds(1,  // Right eye
-                                      rightBounds->left, rightBounds->top,
-                                      rightBounds->width, rightBounds->height);
+void GvrDevice::UpdateLayerBounds(VRServiceImpl* service,
+                                  mojom::VRLayerBoundsPtr leftBounds,
+                                  mojom::VRLayerBoundsPtr rightBounds) {
+  if (IsAccessAllowed(service)) {

[dcheng, 2016/11/08 08:55:16]

FWIW, the usual error handling style in Chromium is to early return:

if (!IsAccessAllowed(service) || !delegate_)
  return;

(GvrDevice could also use some comments describing lifetime; for example, why
are we allowed to set a non-null delegate? It seems like this code would be
simpler if |delegate_| were never null. Doing this in a followup is OK)

[shaobo.yan, 2016/11/08 11:33:44]

I could explain about delegate_ null check. Since the delegate_ is related to a
vr_shell, and until the vr_shell related class call setDelgate, that the
delegate_ would be a non-null object.

[dcheng, 2016/11/08 18:15:43]

Sorry, I think I still don't understand the class design.

The constructor takes a Delegate* argument. Shouldn't this  just always be set
to some non-null value? Why would we allow null delegates at all?

[shaobo.yan, 2016/11/09 01:11:20]

Because in gvr implementation vr_shell is the truly backend which GvrDevice
account on. But vr_shell and gvr_device have different life-cycle. I think
bajones@ could give you some answer.

+    if (!delegate_)
+      return;
+
+    delegate_->UpdateWebVRTextureBounds(0,  // Left eye
+                                        leftBounds->left, leftBounds->top,
+                                        leftBounds->width, leftBounds->height);
+    delegate_->UpdateWebVRTextureBounds(1,  // Right eye
+                                        rightBounds->left, rightBounds->top,
+                                        rightBounds->width,
+                                        rightBounds->height);
+  }
 }
 
 void GvrDevice::SetDelegate(GvrDelegate* delegate) {
@@ -227,7 +252,7 @@ void GvrDevice::SetDelegate(GvrDelegate* delegate) {
   // Notify the clients that this device has changed
   if (delegate_) {
     delegate_->SetWebVRSecureOrigin(secure_origin_);
-    VRDeviceManager::GetInstance()->OnDeviceChanged(GetVRDevice());
+    OnDisplayChanged();
   }
 }