[OnionSoup] Moving VR service from content to Blink

third_party/WebKit/Source/modules/vr/VRController.cpp

Index: third_party/WebKit/Source/modules/vr/VRController.cpp
diff --git a/third_party/WebKit/Source/modules/vr/VRController.cpp b/third_party/WebKit/Source/modules/vr/VRController.cpp
index 36aa47c5b8dcc414b32a940ddd4bdb11a5a0442e..689f2f6123d66358e52f801c21ae57493b85f955 100644
--- a/third_party/WebKit/Source/modules/vr/VRController.cpp
+++ b/third_party/WebKit/Source/modules/vr/VRController.cpp
@@ -5,7 +5,10 @@
 #include "modules/vr/VRController.h"
 
 #include "core/frame/LocalFrame.h"
+#include "modules/vr/VRGetDevicesCallback.h"
 #include "platform/RuntimeEnabledFeatures.h"
+#include "platform/mojo/MojoHelper.h"
+#include "public/platform/ServiceRegistry.h"
 
 namespace blink {
 
@@ -13,11 +16,11 @@ VRController::~VRController()
 {
 }
 
-void VRController::provideTo(LocalFrame& frame, WebVRClient* client)
+void VRController::provideTo(LocalFrame& frame, ServiceRegistry* registry)
 {
     ASSERT(RuntimeEnabledFeatures::webVREnabled());
 
-    VRController* controller = new VRController(frame, client);
+    VRController* controller = new VRController(frame, registry);
     Supplement<LocalFrame>::provideTo(frame, supplementName(), controller);
 }
 
@@ -26,10 +29,12 @@ VRController* VRController::from(LocalFrame& frame)
     return static_cast<VRController*>(Supplement<LocalFrame>::from(frame, supplementName()));
 }
 
-VRController::VRController(LocalFrame& frame, WebVRClient* client)
+VRController::VRController(LocalFrame& frame, ServiceRegistry* registry)
     : LocalFrameLifecycleObserver(&frame)
-    , m_client(client)
 {
+    ASSERT(!m_service.is_bound());
+    ASSERT(registry);
+    registry->connectToRemoteService(mojo::GetProxy(&m_service));
 }
 
 const char* VRController::supplementName()
@@ -37,38 +42,62 @@ const char* VRController::supplementName()
     return "VRController";
 }
 
-void VRController::getDevices(WebVRGetDevicesCallback* callback)
+void VRController::getDevices(VRGetDevicesCallback* callback)

[haraken, 2016/04/27 15:52:12]

getDevices() should take PassOwnPtr. See my below comment.

[RaviKasibhatla, 2016/04/28 14:34:15]

Done.

 {
-    // When detached, the client is no longer valid.
-    if (!m_client) {
+    if (!m_service) {
         callback->onError();
         delete callback;
         return;
     }
 
-    // Client is expected to take ownership of the callback
-    m_client->getDevices(callback);
+    m_pendingGetDevicesCallbacks.append(callback);
+    m_service->GetDevices(sameThreadBindForMojo(&VRController::OnGetDevices, this));

[bajones, 2016/04/27 18:09:28]

If multiple getDevices requests are made while one is still outstanding we
should be able to just queue up the callbacks and fire them all off when the
outstanding request returns. I don't see any benefit to making a separate IPC
call for each request.

[RaviKasibhatla, 2016/04/28 06:05:13]

Isn't the current code doing the same? We are queuing callback received in each
request. Once the request comes back, we fire the callback from the queue.

Are you suggesting that we don't call m_service->GetDevices for each
VRController call and rather make a single call to GetDevices and in response
fire all the queued callbacks together? If yes, when exactly should we be
calling m_service->GetDevices? How do we know when to trigger the service call
knowing we have queued all incoming requests?

 }
 
 void VRController::getSensorState(unsigned index, WebHMDSensorState& into)
 {
-    // When detached, the client is no longer valid.
-    if (!m_client)
+    if (!m_service)
         return;
-    m_client->getSensorState(index, into);
+
+    m_pendingSensorStateRequest = &into;
+    m_service->GetSensorState(index, sameThreadBindForMojo(&VRController::OnGetSensorState, this));
+
+    // This call needs to return results synchronously in order to be useful and
+    // provide the lowest latency results possible.
+    m_service.WaitForIncomingResponse();
 }
 
 void VRController::resetSensor(unsigned index)
 {
-    // When detached, the client is no longer valid.
-    if (!m_client)
+    if (!m_service)
         return;
-    m_client->resetSensor(index);
+    m_service->ResetSensor(index);
 }
 
 void VRController::willDetachFrameHost()
 {
-    m_client = nullptr;
+    resetSensor(0);
+}
+
+void VRController::OnGetDevices(const mojo::Array<mojom::VRDeviceInfoPtr>& devices)
+{
+    WebVector<WebVRDevice> webDevices(devices.size());
+
+    VRGetDevicesCallback* callback = m_pendingGetDevicesCallbacks.takeFirst();
+    if (!callback)
+        return;
+
+    for (size_t i = 0; i < devices.size(); ++i) {
+        webDevices[i] = devices[i].To<WebVRDevice>();
+    }

[bajones, 2016/04/27 18:09:28]

Non-critical idea: Seems like this could benefit from using a converter similar
to the one at
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[RaviKasibhatla, 2016/04/28 06:05:13]

Yes, currently it uses the converter added in VRTypeConverter.h. In next
iteration, probably we can remove VRTypeConverter and add the relevant converter
in MojoHelper.h itself.

+
+    callback->onSuccess(webDevices);
+    delete callback;

[haraken, 2016/04/27 15:52:12]

We should avoid calling manual delete.

Can we make m_pendingGetDevicesCallbacks Deque<OwnPtr<VRGetDevicesCallback>>?
Then you won't need to call manual delete.

[RaviKasibhatla, 2016/04/28 06:05:13]

Since we were already calling manual delete in VRController::getDevices(), I
went in the same path. I will try moving all logic to using OwnPtr.

[RaviKasibhatla, 2016/04/28 14:34:15]

Done.

+}
+
+void VRController::OnGetSensorState(const mojom::VRSensorStatePtr& mojoState)
+{
+    *m_pendingSensorStateRequest = mojoState.To<WebHMDSensorState>();
 }
 
 DEFINE_TRACE(VRController)