Adding Chrome-side WebVR interface

content/browser/vr/vr_service.cc

+// Copyright 2015 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 "content/browser/vr/vr_service.h"
+
+#include "base/memory/singleton.h"
+#include "third_party/WebKit/public/platform/WebVR.h"
+
+#if defined(OS_ANDROID)
+#include "content/browser/vr/cardboard/cardboard_vr_device_provider.h"
+#endif
+
+namespace content {
+
+VRService::VRService() :
+    vr_initialized_(false) {
+#if defined(OS_ANDROID)
+  RegisterProvider(new CardboardVRDeviceProvider());
+#endif
+}
+
+VRService::VRService(VRDeviceProvider* provider) :
+    vr_initialized_(false) {
+  thread_checker_.DetachFromThread();
+  RegisterProvider(provider);
+}
+
+VRService::~VRService() {

[no sievers, 2015/03/26 19:55:15]

Should this also DCHECK(thread_checker_.CalledOnValidThread())?

+  std::for_each(providers_.begin(), providers_.end(),
+      [](VRDeviceProvider* p){ delete p; });
+}
+
+VRService* VRService::GetInstance() {
+  return Singleton<VRService,
+                   LeakySingletonTraits<VRService> >::get();

[no sievers, 2015/03/26 19:55:15]

So you are still leaking this whole class, so the cleanup in the destructor
above is misleading.

Can we avoid making this a singleton?
Is the MessageFilter the only thing that needs to get to this?
If so, can it imply own this?

Note that you can also override OnDestruct() in the filter to make sure it gets
destructed on the right thread (and just call
BrowserThread::DeleteOnIOThread::Destruct(this) in there).

+}
+
+void VRService::GetVRDevices(
+    std::vector<blink::WebVRDevice>* webvr_devices) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  if(webvr_devices == NULL)
+    return;
+
+  InitializeProviders();
+
+  webvr_devices->clear();
+
+  ProviderList::iterator iter;
+
+  std::vector<VRDevice*> devices;
+
+  for (iter = providers_.begin(); iter != providers_.end(); ++iter) {
+    (*iter)->GetDevices(devices);
+  }
+
+  std::vector<VRDevice*>::iterator diter;
+  for (diter = devices.begin(); diter != devices.end(); ++diter) {
+    VRDevice* device = *diter;
+    if (devices_.find(device->id()) == devices_.end()) {
+      devices_[device->id()] = device;
+    }
+
+    blink::WebVRDevice webvr_device;
+    device->GetVRDevice(&webvr_device);
+    webvr_device.index = device->id();
+    webvr_devices->push_back(webvr_device);
+  }
+}
+
+VRDevice* VRService::GetDevice(unsigned int index) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  DeviceMap::iterator iter = devices_.find(index);
+  if (iter == devices_.end()) {
+    return NULL;
+  }
+  return iter->second;
+}
+
+void VRService::AddConsumer(VRServiceConsumer* consumer) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  // Explicitly does not initialize the providers. That's done lazily the first
+  // time GetVRDevices is called.
+  consumers_.insert(consumer);
+}
+
+void VRService::RemoveConsumer(VRServiceConsumer* consumer) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  ConsumerSet::iterator it = consumers_.find(consumer);
+  if (it != consumers_.end()) {

[no sievers, 2015/03/26 19:55:15]

Should this DCHECK() or log an error if not found?

+    consumers_.erase(it);
+    if (consumers_.size() == 0) {
+      ReleaseProviderDevices();
+    }
+  }
+}
+
+void VRService::InitializeProviders() {
+  if(vr_initialized_ == true) {

[no sievers, 2015/03/26 19:55:15]

nit: space after 'if'

+    return;
+  }
+
+  ProviderList::iterator iter;
+  for (iter = providers_.begin(); iter != providers_.end(); ++iter) {
+    (*iter)->Initialize();
+  }
+
+  vr_initialized_ = true;
+}
+
+void VRService::ReleaseProviderDevices() {
+  if(vr_initialized_ == false) {

[no sievers, 2015/03/26 19:55:15]

nit: space after 'if' (also !vr_initalized?)

+    return;
+  }
+
+  devices_.clear();
+
+  ProviderList::iterator iter;
+  for (iter = providers_.begin(); iter != providers_.end(); ++iter) {
+    (*iter)->ReleaseDevices();
+  }
+
+  vr_initialized_ = false;
+}
+
+void VRService::RegisterProvider(VRDeviceProvider* provider) {

[no sievers, 2015/03/26 19:55:15]

If |this| takes ownership, should it be passing a scoped_ptr?
Similarly, |providers_| could have scoped ownership (std::vector<linked_ptr<>>
or so).

But again, you are leaking the providers anyways.

+  providers_.push_back(provider);
+}
+
+}  // namespace content