Adding Chrome-side WebVR interface

Adding Chrome-side WebVR interface

WebVR Spec: http://mozvr.github.io/webvr-spec/webvr.html

For the Blink half of this CL, see https://codereview.chromium.org/848053002

Provides the basic plumbing required to get data from Javascript to the right place in the browser process and back.

BUG=389343
Committed: https://crrev.com/4ab52def6d1861a55fd9864b47f56c16e4b423a0
Cr-Commit-Position: refs/heads/master@{#335062}

Committed: https://crrev.com/02443533743dd265b7a8dec4bee855b0e544a727
Cr-Commit-Position: refs/heads/master@{#335561}

[bajones, 2015-03-18 18:49:48]

bajones@chromium.org changed reviewers:
+ kbr@chromium.org, mkwst@chromium.org, sievers@chromium.org,
tedchoc@chromium.org

[bajones, 2015-03-18 18:49:49]

Blink side of this CL is now landed (https://codereview.chromium.org/848053002).

tedchoc@: Could you please review content/browser/android/ and
content/public/android/? (May also want to provide feedback on JNI usage in
cardboard_vr_device.cc)

mkwst@: Could you review the messages related code? (content/common/ and ipc/)

seivers@: Would you be able to review content/browser/ and content/renderer/? 

kbr@: I'd appreciate any general review you would be able to provide.

And if anyone is unable to provide a review or feels like they're not the
appropriate reviewer please let me know!

[Ted C, 2015-03-19 00:17:21]

On 2015/03/18 18:49:49, bajones wrote:
> Blink side of this CL is now landed
(https://codereview.chromium.org/848053002).
> 
> tedchoc@: Could you please review content/browser/android/ and
> content/public/android/? (May also want to provide feedback on JNI usage in
> cardboard_vr_device.cc)
> 
> mkwst@: Could you review the messages related code? (content/common/ and ipc/)
> 
> seivers@: Would you be able to review content/browser/ and content/renderer/? 
> 
> kbr@: I'd appreciate any general review you would be able to provide.
> 
> And if anyone is unable to provide a review or feels like they're not the
> appropriate reviewer please let me know!

Pre review question.  How big is cardboard.jar and how much does it increase our
APK
size?

[no sievers, 2015-03-19 01:24:53]

https://codereview.chromium.org/829803003/diff/100001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/829803003/diff/100001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:2269: SINGLE_VALUE_TYPE(switches::kEnableVR)
Should it be kEnableWebVR and 'enable-webvr'?

https://codereview.chromium.org/829803003/diff/100001/content/browser/android...
File content/browser/android/browser_jni_registrar.cc (right):

https://codereview.chromium.org/829803003/diff/100001/content/browser/android...
content/browser/android/browser_jni_registrar.cc:53:
content::CardboardVRDevice::RegisterCardboardVRDevice},
nit: indent

https://codereview.chromium.org/829803003/diff/100001/content/browser/rendere...
File content/browser/renderer_host/vr_message_filter.cc (right):

https://codereview.chromium.org/829803003/diff/100001/content/browser/rendere...
content/browser/renderer_host/vr_message_filter.cc:17:
DCHECK_CURRENTLY_ON(BrowserThread::IO);
I'm not sure this is guaranteed (although it might be, but it's not obvious).
But filters are refcounted across threads. The RenderProcessHost holds a
reference to the channel on the UI thread, the channel holds a reference to the
filter... then there is the internal Context I think which shuts down on the IO
thread.

https://codereview.chromium.org/829803003/diff/100001/content/browser/vr/vr_d...
File content/browser/vr/vr_device.h (right):

https://codereview.chromium.org/829803003/diff/100001/content/browser/vr/vr_d...
content/browser/vr/vr_device.h:22: 
So all of this - device, service, provider - gets called into exclusively from
the IO thread?
That should probably be commented on in the headers. And should we have a few
thread checks in the right places?

https://codereview.chromium.org/829803003/diff/100001/content/browser/vr/vr_d...
File content/browser/vr/vr_device_provider.h (right):

https://codereview.chromium.org/829803003/diff/100001/content/browser/vr/vr_d...
content/browser/vr/vr_device_provider.h:27: // necessary, but should be able to
start up again if GetDevices is called.
Hmmm, so this is actually not the counterpart to Initialize(). Maybe a different
name would be better then. ReleaseDevices()?
Because IIUC you don't have to call Initialize() after you called Shutdown(),
but you do have to release all devices although you don't specifically open
them. (The comment almost suggest that there should be OpenDevice, CloseDevice.)

https://codereview.chromium.org/829803003/diff/100001/content/browser/vr/vr_s...
File content/browser/vr/vr_service.cc (right):

https://codereview.chromium.org/829803003/diff/100001/content/browser/vr/vr_s...
content/browser/vr/vr_service.cc:16: void
DeleteVRDeviceProvider(VRDeviceProvider* provider) {
nit: anonymous namespace around this

Or better: you can actually use a lambda below:
std::for_each(providers_.begin(), providers_.end(), [](VRDeviceProvider* p){
delete p; });

https://codereview.chromium.org/829803003/diff/100001/content/browser/vr/vr_s...
content/browser/vr/vr_service.cc:64: LeakySingletonTraits<VRService> >::get();
Ah wait, so why even bother... VRDeviceProviders never get deleted?

https://codereview.chromium.org/829803003/diff/100001/content/browser/vr/vr_s...
File content/browser/vr/vr_service.h (right):

https://codereview.chromium.org/829803003/diff/100001/content/browser/vr/vr_s...
content/browser/vr/vr_service.h:29: void Shutdown();
It's unclear who is supposed to call these. Initialize() gets called internally
(so could be a private method), and Shutdown() does not get called.

https://codereview.chromium.org/829803003/diff/100001/content/browser/vr/vr_s...
content/browser/vr/vr_service.h:40: VRService(VRDeviceProvider* provider);
nit: explicit

https://codereview.chromium.org/829803003/diff/100001/content/browser/vr/vr_s...
content/browser/vr/vr_service.h:45: DISALLOW_COPY_AND_ASSIGN(VRService);
nit: move this to the bottom

https://codereview.chromium.org/829803003/diff/100001/content/browser/vr/vr_t...
File content/browser/vr/vr_test_helpers.h (right):

https://codereview.chromium.org/829803003/diff/100001/content/browser/vr/vr_t...
content/browser/vr/vr_test_helpers.h:15: class MockVRDevice : public VRDevice {
nit: might be nice to have these two mock classes in their own files.

https://codereview.chromium.org/829803003/diff/100001/content/browser/vr/vr_t...
content/browser/vr/vr_test_helpers.h:19: 
Since you are not using gmock, should these classes be called
FakeMockVRDevice(Provider)?
And then go under vr/test/...

https://codereview.chromium.org/829803003/diff/100001/content/common/vr_messa...
File content/common/vr_messages.h (right):

https://codereview.chromium.org/829803003/diff/100001/content/common/vr_messa...
content/common/vr_messages.h:1: // Copyright 2014 The Chromium Authors. All
rights reserved.
nit: 2015

https://codereview.chromium.org/829803003/diff/100001/content/common/vr_messa...
content/common/vr_messages.h:51: IPC_SYNC_MESSAGE_CONTROL0_1(VRMsg_GetVRDevices,
nit: should these be named VRHostMsg_*?

https://codereview.chromium.org/829803003/diff/100001/content/common/vr_messa...
content/common/vr_messages.h:52: std::vector<blink::WebVRDevice>  /* handle */)
nit: /* devices */

https://codereview.chromium.org/829803003/diff/100001/content/common/vr_messa...
content/common/vr_messages.h:58: blink::WebHMDSensorState /* handle */)
nit: /* state */

https://codereview.chromium.org/829803003/diff/100001/content/common/vr_param...
File content/common/vr_param_traits.cc (right):

https://codereview.chromium.org/829803003/diff/100001/content/common/vr_param...
content/common/vr_param_traits.cc:1: // Copyright 2014 The Chromium Authors. All
rights reserved.
nit: 2015

https://codereview.chromium.org/829803003/diff/100001/content/common/vr_param...
File content/common/vr_param_traits.h (right):

https://codereview.chromium.org/829803003/diff/100001/content/common/vr_param...
content/common/vr_param_traits.h:1: // Copyright 2014 The Chromium Authors. All
rights reserved.
nit: 2015

https://codereview.chromium.org/829803003/diff/100001/content/content_jni.gypi
File content/content_jni.gypi (right):

https://codereview.chromium.org/829803003/diff/100001/content/content_jni.gyp...
content/content_jni.gypi:50: 'cardboard_javalib',
Can we depend on this from the android section in content_browser.gypi instead?

https://codereview.chromium.org/829803003/diff/100001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/input/CardboardVRDevice.java
(right):

https://codereview.chromium.org/829803003/diff/100001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/CardboardVRDevice.java:1:
// Copyright 2013 The Chromium Authors. All rights reserved.
nit: 2015

https://codereview.chromium.org/829803003/diff/100001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/CardboardVRDevice.java:105:
Log.d(TAG, "onCardboardTrigger");
that's spammy

https://codereview.chromium.org/829803003/diff/100001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/CardboardVRDevice.java:130:
final CardboardVRDevice listener = this;
unused var

https://codereview.chromium.org/829803003/diff/100001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/CardboardVRDevice.java:131:
mMagnetSensor.stop();
Is it ok to call this from a thread different than the observer thread created
above?

https://codereview.chromium.org/829803003/diff/100001/content/renderer/vr_dis...
File content/renderer/vr_dispatcher.cc (right):

https://codereview.chromium.org/829803003/diff/100001/content/renderer/vr_dis...
content/renderer/vr_dispatcher.cc:22: CHECK(RenderThread::Get()->Send(new
VRMsg_GetVRDevices(
not sure about the CHECKs(). or why we would do this here and not elsewhere
(which also seems very odd).

https://codereview.chromium.org/829803003/diff/100001/content/renderer/vr_dis...
content/renderer/vr_dispatcher.cc:31: CHECK(RenderThread::Get()->Send(new
VRMsg_GetHMDSensorState(index, &state)));
On a related note... for GetSensorState and GetRendertargetRects, should they
return some sort of result? Ie. do we need to worry about this failing, and when
could this happen?

https://codereview.chromium.org/829803003/diff/100001/content/renderer/vr_dis...
content/renderer/vr_dispatcher.cc:50: return false;
Why is this a RenderProcessObserver then?

https://codereview.chromium.org/829803003/diff/100001/content/renderer/vr_dis...
File content/renderer/vr_dispatcher.h (right):

https://codereview.chromium.org/829803003/diff/100001/content/renderer/vr_dis...
content/renderer/vr_dispatcher.h:36: bool OnControlMessageReceived(const
IPC::Message& message) override;
private: DISALLOW_COPY_AND_ASSIGN(VRDispatcher)

[no sievers, 2015-03-19 01:26:23]

https://codereview.chromium.org/829803003/diff/100001/content/browser/vr/vr_t...
File content/browser/vr/vr_test_helpers.h (right):

https://codereview.chromium.org/829803003/diff/100001/content/browser/vr/vr_t...
content/browser/vr/vr_test_helpers.h:19: 
On 2015/03/19 01:24:52, sievers wrote:
> Since you are not using gmock, should these classes be called
> FakeMockVRDevice(Provider)?
> And then go under vr/test/...

err FakeVRDevice etc. I meant

[bajones, 2015-03-19 02:44:58]

On 2015/03/19 at 00:17:21, tedchoc wrote:
> Pre review question.  How big is cardboard.jar and how much does it increase
our APK
> size?

Good questions! The canonical Cardboard.jar is 167Kb and distributed by the
Cardboard team here:
https://github.com/googlesamples/cardboard-java/tree/master/CardboardSample/libs

I was asked the APK size question in my intent to implement email
(https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/m32Yy87Ggt0)
and at the time it added 74Kb to ChromeShell.apk. Cardboard.jar has been updated
since that number was checked, and I'm not sure what effect that has on the
final build size. I'm not at an android-build-capable machine right now, so I'll
have to give you updated numbers later.

sievers@: Thanks for the quick and detailed review! I'll get on these ASAP, and
might have some questions for you along the way.

[Ted C, 2015-03-19 16:30:11]

On 2015/03/19 02:44:58, bajones wrote:
> On 2015/03/19 at 00:17:21, tedchoc wrote:
> > Pre review question.  How big is cardboard.jar and how much does it increase
> our APK
> > size?
> 
> Good questions! The canonical Cardboard.jar is 167Kb and distributed by the
> Cardboard team here:
>
https://github.com/googlesamples/cardboard-java/tree/master/CardboardSample/libs
> 
> I was asked the APK size question in my intent to implement email
> (https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/m32Yy87Ggt0)
> and at the time it added 74Kb to ChromeShell.apk. Cardboard.jar has been
updated
> since that number was checked, and I'm not sure what effect that has on the
> final build size. I'm not at an android-build-capable machine right now, so
I'll
> have to give you updated numbers later.

Make sure you test on release as proguard "should" remove any unused java bits
from the jar, so it might end up being even smaller in practice.  Would be good
to know final numbers though.

> 
> sievers@: Thanks for the quick and detailed review! I'll get on these ASAP,
and
> might have some questions for you along the way.

[bajones, 2015-03-19 23:33:28]

Updated patch with a bunch of fixes, though there's still some things that need
addressing (threading concerns being the primary one.)

On 2015/03/19 at 16:30:11, tedchoc wrote:
> Make sure you test on release as proguard "should" remove any unused java bits
> from the jar, so it might end up being even smaller in practice.  Would be
good
> to know final numbers though.

Release build ToT right now: 33,500,377 bytes
Release build w/ Patch 6: 33,571,341 bytes

So this patch currently adds 69Kb (70,964 bytes) to ChromeShell.apk

On 2015/03/19 at 01:24:53, sievers wrote:
>
https://codereview.chromium.org/829803003/diff/100001/content/content_jni.gyp...
> content/content_jni.gypi:50: 'cardboard_javalib',
> Can we depend on this from the android section in content_browser.gypi
instead?

Tried this and it didn't work, because CardboardVRDevice.java (included in
content_jni.gypi) is what actually needs it. I was able to move it to
content.gyp, though, where it's grouped with some existing android dependencies.
Feels cleaner.

I've still got some work to do, so I'll let you know when it's in a good place
to review again! Thanks!

[Mike West, 2015-03-20 16:46:17]

On 2015/03/19 at 23:33:28, bajones wrote:
> I've still got some work to do, so I'll let you know when it's in a good place
to review again! Thanks!

The IPC work here looks pretty reasonable (it's a lot of synchronous getters,
but I suppose there's no way around blocking on the data for your particular
use-case). Do you expect the messages to change based on the work that you still
have left?

https://codereview.chromium.org/829803003/diff/120001/content/common/vr_messa...
File content/common/vr_messages.h (right):

https://codereview.chromium.org/829803003/diff/120001/content/common/vr_messa...
content/common/vr_messages.h:51:
IPC_SYNC_MESSAGE_CONTROL0_1(VRHostMsg_GetVRDevices,
Nit: Can you add some comments about what exactly these messages are doing?

[bajones, 2015-03-20 18:20:11]

On 2015/03/20 at 16:46:17, mkwst wrote:
> The IPC work here looks pretty reasonable (it's a lot of synchronous getters,
but I suppose there's no way around blocking on the data for your particular
use-case).

Yeah, the synchronous nature of the state poll is unfortunate but ultimately
probably the best way to approach it since sensor response latency is a big deal
for VR.

> Do you expect the messages to change based on the work that you still have
left?

No, not for this CL. I also have the next two CL's worth of code for this
feature waiting in the wings, and the messages stay stable there as well. The
only upcoming change is either a new message to notify a device that it's the
target of a fullscreen request or an update to existing fullscreen plumbing to
communicate the same thing. Haven't decided which is cleaner. 

>
https://codereview.chromium.org/829803003/diff/120001/content/common/vr_messa...
> File content/common/vr_messages.h (right):
> 
>
https://codereview.chromium.org/829803003/diff/120001/content/common/vr_messa...
> content/common/vr_messages.h:51:
IPC_SYNC_MESSAGE_CONTROL0_1(VRHostMsg_GetVRDevices,
> Nit: Can you add some comments about what exactly these messages are doing?

Done!

[Ken Russell (switch to Gerrit), 2015-03-23 21:34:14]

This LGTM overall. Great work getting it hooked up. Only one small comment.

https://codereview.chromium.org/829803003/diff/140001/content/browser/vr/card...
File content/browser/vr/cardboard/cardboard_vr_device.cc (right):

https://codereview.chromium.org/829803003/diff/140001/content/browser/vr/card...
content/browser/vr/cardboard/cardboard_vr_device.cc:148: JNIEnv* env =
AttachCurrentThread();
I'm not sure what the conventions are for code interacting with the Java APIs,
but I think you should consider caching the JNIEnv* for the current thread, and
DCHECKing that the thread on which these methods are called is the same every
time.

[bajones, 2015-03-25 20:39:34]

On 2015/03/23 21:34:14, Ken Russell wrote:
>
https://codereview.chromium.org/829803003/diff/140001/content/browser/vr/card...
> content/browser/vr/cardboard/cardboard_vr_device.cc:148: JNIEnv* env =
> AttachCurrentThread();
> I'm not sure what the conventions are for code interacting with the Java APIs,
> but I think you should consider caching the JNIEnv* for the current thread,
and
> DCHECKing that the thread on which these methods are called is the same every
> time.

I haven't found any other code in Chrome that uses that pattern. Calling
AttachCurrentThread for every function that interfaces with Java appears to be
the standard way of doing it, though I don't know enough about JNI to tell you
why.

Examples:
https://code.google.com/p/chromium/codesearch#chromium/src/ui/gl/android/surf...
https://code.google.com/p/chromium/codesearch#chromium/src/net/android/networ...

[Ken Russell (switch to Gerrit), 2015-03-25 21:37:11]

On 2015/03/25 20:39:34, bajones wrote:
> On 2015/03/23 21:34:14, Ken Russell wrote:
> >
>
https://codereview.chromium.org/829803003/diff/140001/content/browser/vr/card...
> > content/browser/vr/cardboard/cardboard_vr_device.cc:148: JNIEnv* env =
> > AttachCurrentThread();
> > I'm not sure what the conventions are for code interacting with the Java
APIs,
> > but I think you should consider caching the JNIEnv* for the current thread,
> and
> > DCHECKing that the thread on which these methods are called is the same
every
> > time.
> 
> I haven't found any other code in Chrome that uses that pattern. Calling
> AttachCurrentThread for every function that interfaces with Java appears to be
> the standard way of doing it, though I don't know enough about JNI to tell you
> why.
> 
> Examples:
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/gl/android/surf...
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/android/networ...

All good. Thanks for double-checking.

[bajones, 2015-03-25 23:54:11]

sievers@: Latest patch should address all of your feedback. Please take another
look!

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

https://codereview.chromium.org/829803003/diff/160001/content/browser/vr/card...
File content/browser/vr/cardboard/cardboard_vr_device.cc (right):

https://codereview.chromium.org/829803003/diff/160001/content/browser/vr/card...
content/browser/vr/cardboard/cardboard_vr_device.cc:82:
base::TruncateUTF8ToByteSize(tmp, blink::WebVRDevice::deviceIdLengthCap-1,
nit: spaces around operator here and in line 85 and 100

https://codereview.chromium.org/829803003/diff/160001/content/browser/vr/card...
File content/browser/vr/cardboard/cardboard_vr_device.h (right):

https://codereview.chromium.org/829803003/diff/160001/content/browser/vr/card...
content/browser/vr/cardboard/cardboard_vr_device.h:40: unsigned int
frame_index_;
DISALLOW_COPY_AND_ASSIGN()

https://codereview.chromium.org/829803003/diff/160001/content/browser/vr/card...
File content/browser/vr/cardboard/cardboard_vr_device_provider.h (right):

https://codereview.chromium.org/829803003/diff/160001/content/browser/vr/card...
content/browser/vr/cardboard/cardboard_vr_device_provider.h:28: VRDevice*
cardboard_device_;
can this be scoped_ptr since it takes ownership?

https://codereview.chromium.org/829803003/diff/160001/content/browser/vr/card...
content/browser/vr/cardboard/cardboard_vr_device_provider.h:29: };
DISALLOW_COPY_AND_ASSIGN()

https://codereview.chromium.org/829803003/diff/160001/content/browser/vr/vr_s...
File content/browser/vr/vr_service.cc (right):

https://codereview.chromium.org/829803003/diff/160001/content/browser/vr/vr_s...
content/browser/vr/vr_service.cc:29: VRService::~VRService() {
Should this also DCHECK(thread_checker_.CalledOnValidThread())?

https://codereview.chromium.org/829803003/diff/160001/content/browser/vr/vr_s...
content/browser/vr/vr_service.cc:36: LeakySingletonTraits<VRService> >::get();
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).

https://codereview.chromium.org/829803003/diff/160001/content/browser/vr/vr_s...
content/browser/vr/vr_service.cc:94: if (it != consumers_.end()) {
Should this DCHECK() or log an error if not found?

https://codereview.chromium.org/829803003/diff/160001/content/browser/vr/vr_s...
content/browser/vr/vr_service.cc:103: if(vr_initialized_ == true) {
nit: space after 'if'

https://codereview.chromium.org/829803003/diff/160001/content/browser/vr/vr_s...
content/browser/vr/vr_service.cc:116: if(vr_initialized_ == false) {
nit: space after 'if' (also !vr_initalized?)

https://codereview.chromium.org/829803003/diff/160001/content/browser/vr/vr_s...
content/browser/vr/vr_service.cc:130: void
VRService::RegisterProvider(VRDeviceProvider* provider) {
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.

https://codereview.chromium.org/829803003/diff/160001/content/browser/vr/vr_s...
File content/browser/vr/vr_service.h (right):

https://codereview.chromium.org/829803003/diff/160001/content/browser/vr/vr_s...
content/browser/vr/vr_service.h:40: // Constructor for testing. Takes ownership
of the given provider.
If so, then you can pass scoped_ptr

https://codereview.chromium.org/829803003/diff/160001/content/browser/vr/vr_s...
File content/browser/vr/vr_service_unittest.cc (right):

https://codereview.chromium.org/829803003/diff/160001/content/browser/vr/vr_s...
content/browser/vr/vr_service_unittest.cc:47: delete service_;
Can you make |service_| scoped_ptr then?

https://codereview.chromium.org/829803003/diff/160001/content/browser/vr/vr_s...
content/browser/vr/vr_service_unittest.cc:51: provider_ = new
FakeVRDeviceProvider();
Who deletes the provider?

https://codereview.chromium.org/829803003/diff/160001/content/browser/vr/vr_s...
content/browser/vr/vr_service_unittest.cc:118: EXPECT_EQ((size_t)0,
webvr_devices.size());
nit: 0u (or static_cast, style guide) here and below

https://codereview.chromium.org/829803003/diff/160001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/input/CardboardVRDevice.java
(right):

https://codereview.chromium.org/829803003/diff/160001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/CardboardVRDevice.java:1:
// Copyright 2014 The Chromium Authors. All rights reserved.
nit: 2015

https://codereview.chromium.org/829803003/diff/160001/content/renderer/render...
File content/renderer/renderer_blink_platform_impl.cc (right):

https://codereview.chromium.org/829803003/diff/160001/content/renderer/render...
content/renderer/renderer_blink_platform_impl.cc:847:
blink::WebVector<blink::WebVRDevice>* devices) {
Looks like all the APIs below check RenderThreadImpl::current() for NULL and
early-out. Although I'm not fully sure why (maybe to accomodate some standalone
tests?). But maybe we should do it here too for consistency.

https://codereview.chromium.org/829803003/diff/160001/content/renderer/vr_dis...
File content/renderer/vr_dispatcher.h (right):

https://codereview.chromium.org/829803003/diff/160001/content/renderer/vr_dis...
content/renderer/vr_dispatcher.h:8: #include "base/memory/scoped_ptr.h"
nit: unneeded?

https://codereview.chromium.org/829803003/diff/160001/content/renderer/vr_dis...
content/renderer/vr_dispatcher.h:9: #include "content/common/vr_messages.h"
nit: can this move to the .cc file?

https://codereview.chromium.org/829803003/diff/160001/content/renderer/vr_dis...
content/renderer/vr_dispatcher.h:10: #include
"content/public/renderer/render_process_observer.h"
nit: unneeded?

https://codereview.chromium.org/829803003/diff/160001/content/renderer/vr_dis...
content/renderer/vr_dispatcher.h:11: #include
"third_party/WebKit/public/platform/WebSize.h"
nit: unneeded? and maybe some can be fwd-declared (state, vector, maybe
device?).

[bajones, 2015-03-26 21:22:55]

Thanks again for the detailed review! Addressed all the new feedback with one
exception, which I'd like your advice on:

On 2015/03/26 19:55:15, sievers wrote:
>
https://codereview.chromium.org/829803003/diff/160001/content/browser/vr/vr_s...
> content/browser/vr/vr_service.cc:36: LeakySingletonTraits<VRService> >::get();
> 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?

Agreed that this isn't great. I don't think making it owned by the message
filter is the right move, though, since the idea is that we want all renderers
to share a single instance of the VR service and related classes. (Also, in an
upcoming patch the compositor will need to have some basic interaction with the
service, so hiding it in the MessageFilter is a bad idea.)

I could take the newly reworked ServiceConsumer tracking and have it actually
delete the instance when the active consumer count hits 0, but I'm wondering if
there's a well established pattern for these sort of classes I should be
modeling this after. I originally built this to mimick GamepadService at the
time, but that has since been refactored. It still appears that they're leaking
the service, though. Thoughts?

[no sievers, 2015-03-26 22:13:13]

On 2015/03/26 21:22:55, bajones wrote:
> Thanks again for the detailed review! Addressed all the new feedback with one
> exception, which I'd like your advice on:
> 
> On 2015/03/26 19:55:15, sievers wrote:
> >
>
https://codereview.chromium.org/829803003/diff/160001/content/browser/vr/vr_s...
> > content/browser/vr/vr_service.cc:36: LeakySingletonTraits<VRService>
>::get();
> > 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?
> 
> Agreed that this isn't great. I don't think making it owned by the message
> filter is the right move, though, since the idea is that we want all renderers
> to share a single instance of the VR service and related classes. (Also, in an
> upcoming patch the compositor will need to have some basic interaction with
the
> service, so hiding it in the MessageFilter is a bad idea.)
> 
> I could take the newly reworked ServiceConsumer tracking and have it actually
> delete the instance when the active consumer count hits 0, but I'm wondering
if
> there's a well established pattern for these sort of classes I should be
> modeling this after. I originally built this to mimick GamepadService at the
> time, but that has since been refactored. It still appears that they're
leaking
> the service, though. Thoughts?

I like the idea of refcounting it through the consumers.

And you want to definitely share the same cardboard instance between different
renderers?

[bajones, 2015-03-26 22:26:29]

On 2015/03/26 22:13:13, sievers wrote:
> I like the idea of refcounting it through the consumers.

Okay then, I'll do that.

> And you want to definitely share the same cardboard instance between different
> renderers?

Yup. The VRDevices shouldn't have any per-renderer state, they're only used to
query values from the hardware. Other potential backends (Oculus, Valve) may
have non-trivial initialization costs and may not be well behaved if multiple
threads are accessing them at once. Ensuring that each hardware interface is
instantiated exactly once also produces the lowest memory impact possible.

[bajones, 2015-03-26 23:59:21]

On 2015/03/26 22:26:29, bajones wrote:
> On 2015/03/26 22:13:13, sievers wrote:
> > I like the idea of refcounting it through the consumers.
> 
> Okay then, I'll do that.
> 

And done! This also cleaned up the concept of "Releasing" DeviceProviders, since
they now have the same lifespan as the service.

[no sievers, 2015-03-27 01:18:27]

ok just a few more nits

https://codereview.chromium.org/829803003/diff/200001/content/browser/vr/vr_s...
File content/browser/vr/vr_service.cc (right):

https://codereview.chromium.org/829803003/diff/200001/content/browser/vr/vr_s...
content/browser/vr/vr_service.cc:37:
DCHECK(thread_checker_.CalledOnValidThread());
Can you add the OnDestruct() override in the msg filter I was mentioning earlier
to make sure this is always the case?

https://codereview.chromium.org/829803003/diff/200001/content/browser/vr/vr_s...
content/browser/vr/vr_service.cc:50: CHECK(!!instance != !!g_vr_service);
nit: CHECK_NE(!!instance, !!g_vr_service)

https://codereview.chromium.org/829803003/diff/200001/content/browser/vr/vr_s...
content/browser/vr/vr_service.cc:137:
providers_.push_back(linked_ptr<VRDeviceProvider>(provider.release()));
nit: make_linked_ptr(provider.release())

https://codereview.chromium.org/829803003/diff/200001/content/browser/vr/vr_s...
File content/browser/vr/vr_service.h (right):

https://codereview.chromium.org/829803003/diff/200001/content/browser/vr/vr_s...
content/browser/vr/vr_service.h:43: explicit
VRService(linked_ptr<VRDeviceProvider> provider);
Can this take scoped_ptr and take ownership?

https://codereview.chromium.org/829803003/diff/200001/content/browser/vr/vr_s...
File content/browser/vr/vr_service_consumer.cc (right):

https://codereview.chromium.org/829803003/diff/200001/content/browser/vr/vr_s...
content/browser/vr/vr_service_consumer.cc:12: service_(NULL),
|service_| is unused

https://codereview.chromium.org/829803003/diff/200001/content/browser/vr/vr_s...
File content/browser/vr/vr_service_unittest.cc (right):

https://codereview.chromium.org/829803003/diff/200001/content/browser/vr/vr_s...
content/browser/vr/vr_service_unittest.cc:29: linked_ptr<FakeVRDeviceProvider>
provider_;
nit: can you just use scoped_ptr for clear ownership?

[bajones, 2015-03-27 19:34:53]

On 2015/03/27 01:18:27, sievers wrote:
> ok just a few more nits

Thanks for being such a responsive reviewer! I'm really happy with the
improvements made to the code as a result.

Addressed the nits with one point of contention:

>
https://codereview.chromium.org/829803003/diff/200001/content/browser/vr/vr_s...
> content/browser/vr/vr_service.h:43: explicit
> VRService(linked_ptr<VRDeviceProvider> provider);
> Can this take scoped_ptr and take ownership?

Done, but...

>
https://codereview.chromium.org/829803003/diff/200001/content/browser/vr/vr_s...
> content/browser/vr/vr_service_unittest.cc:29: linked_ptr<FakeVRDeviceProvider>
> provider_;
> nit: can you just use scoped_ptr for clear ownership?

I need to have access to this object (as a FakeVRDeviceProvider) in the tests,
and changing the VRService test constructor to take a scoped_ptr would mean
using a scoped_ptr here would immediately remove my access to it. Instead I've
passed it as a scoped_ptr but retained a raw pointer to the object for use in
the tests. Hopefully the ownership semantics are still clear in this case.

[no sievers, 2015-03-27 20:45:38]

lgtm

https://codereview.chromium.org/829803003/diff/220001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/829803003/diff/220001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:918: AddFilter(new
VRMessageFilter());
Can you add the filter only if the cmdline is set?

[Ted C, 2015-03-28 00:56:04]

Silly higher level questions for the uninformed.

https://codereview.chromium.org/829803003/diff/140001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/input/CardboardVRDevice.java
(right):

https://codereview.chromium.org/829803003/diff/140001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/CardboardVRDevice.java:1:
// Copyright 2014 The Chromium Authors. All rights reserved.
2015

https://codereview.chromium.org/829803003/diff/240001/content/browser/vr/card...
File content/browser/vr/cardboard/cardboard_vr_device.cc (right):

https://codereview.chromium.org/829803003/diff/240001/content/browser/vr/card...
content/browser/vr/cardboard/cardboard_vr_device.cc:26: float fTrace = m[0] +
m[5] + m[10];
this block looks indented 2 spaces too many

https://codereview.chromium.org/829803003/diff/240001/content/browser/vr/vr_s...
File content/browser/vr/vr_service.cc (right):

https://codereview.chromium.org/829803003/diff/240001/content/browser/vr/vr_s...
content/browser/vr/vr_service.cc:66: if(webvr_devices == NULL)
space after if...more often I would see !webvr_devices instead of == NULL

https://codereview.chromium.org/829803003/diff/240001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/input/CardboardVRDevice.java
(right):

https://codereview.chromium.org/829803003/diff/240001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/CardboardVRDevice.java:37:
mHMDManager = new HeadMountedDisplayManager(context);
Should we be calling things like this:
https://developers.google.com/cardboard/android/latest/reference/com/google/v...

Are we wasting battery if we don't?

And I tried digging through parts of this change and I never see how we actually
determine if a cardboard device is actually available.

What happens if it becomes unavailable?  I don't see anything in the API that
signals that nor do I see handling.

To me, this logic doesn't mimic any of the other external device APIs were you
have to handle devices appearing and disappearing, so I feel like I'm missing
something.

https://codereview.chromium.org/829803003/diff/240001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/CardboardVRDevice.java:56:
private float getIPD() {
java says to not capitalize acronyms

https://source.android.com/source/code-style.html#treat-acronyms-as-words

[bajones, 2015-03-28 17:43:35]

On 2015/03/28 00:56:04, Ted C wrote:
> Silly higher level questions for the uninformed.

No worries! This is fairly new territory so I don't expect a lot of domain
expertise 

>
https://codereview.chromium.org/829803003/diff/140001/content/public/android/...
>
content/public/android/java/src/org/chromium/content/browser/input/CardboardVRDevice.java:1:
> // Copyright 2014 The Chromium Authors. All rights reserved.
> 2015

This is already fixed in the latest patch. (All your other comments are against
the newest patch, so I'll address them soon.)

>
https://codereview.chromium.org/829803003/diff/240001/content/public/android/...
>
content/public/android/java/src/org/chromium/content/browser/input/CardboardVRDevice.java:37:
> mHMDManager = new HeadMountedDisplayManager(context);
> Should we be calling things like this:
>
https://developers.google.com/cardboard/android/latest/reference/com/google/v...
> 
> Are we wasting battery if we don't?

At the moment this object will get shut down and stop tracking once all
renderers referencing it have closed. We can be more fine grained and pause when
the Blink objects are GCed, but I was leaving that to a future patch to try and
keep this CL manageable.  

> And I tried digging through parts of this change and I never see how we
actually
> determine if a cardboard device is actually available.

I think the basic assumption is that every Android device is a potential
Cardboard device all the time. In a limited number of circumstances you can
detect if the phone is actually in a Cardboard harness via the NFC tag, but
that's proven unreliable and the Cardboard team is moving away from it. There
may be some special cases for devices without accelerometers, I'd have to check
with the Cardboard team.

> What happens if it becomes unavailable?  I don't see anything in the API that
> signals that nor do I see handling.
> 
> To me, this logic doesn't mimic any of the other external device APIs were you
> have to handle devices appearing and disappearing, so I feel like I'm missing
> something.

See above: There's not a reliable way to detect if a phone is in or out of a
Cardboard harness, so "connected" and "disconnected" states aren't meaningful.
That won't be the case for other potential VR devices on the desktop (Oculus
Rift, etc.) and I was planning on handling that when adding support for those
devices.

[Mike West, 2015-03-30 05:07:37]

IPC LGTM.

[jdduke (slow), 2015-03-30 15:19:46]

jdduke@chromium.org changed reviewers:
+ jdduke@chromium.org

[jdduke (slow), 2015-03-30 15:19:47]

https://codereview.chromium.org/829803003/diff/240001/content/browser/rendere...
File content/browser/renderer_host/vr_message_filter.cc (right):

https://codereview.chromium.org/829803003/diff/240001/content/browser/rendere...
content/browser/renderer_host/vr_message_filter.cc:49:
device->GetSensorState(state);
So, these methods appear to be getting called on the IO thread. Are Android's
HeadMountedDisplay and HeadMountedDisplayManager both completely thread safe?

https://codereview.chromium.org/829803003/diff/240001/content/browser/rendere...
content/browser/renderer_host/vr_message_filter.cc:68:
device->GetRenderTargetRects(leftFov, rightFov, leftRect, rightRect);
Apologies if you've already gone over this, but I don't see where these
variables are getting consumed? Where do we forward the fetched data?

[bajones, 2015-03-30 16:41:44]

tedchoc@: Addressed your nits, thanks! Let me know if you have any remaining
concerns over sensor lifetime that you want addressed in this CL.

On 2015/03/30 15:19:47, jdduke wrote:
>
https://codereview.chromium.org/829803003/diff/240001/content/browser/rendere...
> File content/browser/renderer_host/vr_message_filter.cc (right):
> 
>
https://codereview.chromium.org/829803003/diff/240001/content/browser/rendere...
> content/browser/renderer_host/vr_message_filter.cc:49:
> device->GetSensorState(state);
> So, these methods appear to be getting called on the IO thread. Are Android's
> HeadMountedDisplay and HeadMountedDisplayManager both completely thread safe?

I've searched through the Cardboard SDK documentation
(https://developers.google.com/cardboard/android/latest/reference/com/google/v...)
and haven't found anything that explicitly mentions the thread safety of these
objects. There is one class, DistortionRenderer
(https://developers.google.com/cardboard/android/latest/reference/com/google/v...),
that explicitly points out that it is not thread safe. One would hope this
implies that the other classes are thread safe due to omission of any language
to the contrary. I'll ping the Cardboard team about it, though. 

>
https://codereview.chromium.org/829803003/diff/240001/content/browser/rendere...
> content/browser/renderer_host/vr_message_filter.cc:68:
> device->GetRenderTargetRects(leftFov, rightFov, leftRect, rightRect);
> Apologies if you've already gone over this, but I don't see where these
> variables are getting consumed? Where do we forward the fetched data?

No apologies needed. :)

In this case leftRect and rightRect are the outputs, populated by the device for
the given field of view and returned to Blink where they are exposed as the
renderRect for each eye.
(http://mozvr.github.io/webvr-spec/webvr.html#dom-vreyeparameters-renderrect).

[bajones, 2015-03-30 16:43:55]

achuith@: Could you give a quick thumbs up to the single line
chrome_restart_request.cc change?

[bajones, 2015-03-30 16:45:24]

bajones@chromium.org changed reviewers:
+ achuith@chromium.org

[bajones, 2015-03-30 16:45:25]

Hm... My reviewers list didn't update properly for some reason. Let's try this
again:

achuith@: Could you give a quick thumbs up to the single line
chrome_restart_request.cc change?

[jdduke (slow), 2015-03-30 16:47:48]

On 2015/03/30 16:41:44, bajones wrote:
> On 2015/03/30 15:19:47, jdduke wrote:
>
https://codereview.chromium.org/829803003/diff/240001/content/browser/rendere...
> > content/browser/renderer_host/vr_message_filter.cc:49:
> > device->GetSensorState(state);
> > So, these methods appear to be getting called on the IO thread. Are
Android's
> > HeadMountedDisplay and HeadMountedDisplayManager both completely thread
safe?
> 
> I've searched through the Cardboard SDK documentation
>
(https://developers.google.com/cardboard/android/latest/reference/com/google/v...)
> and haven't found anything that explicitly mentions the thread safety of these
> objects. There is one class, DistortionRenderer
>
(https://developers.google.com/cardboard/android/latest/reference/com/google/v...),
> that explicitly points out that it is not thread safe. One would hope this
> implies that the other classes are thread safe due to omission of any language
> to the contrary. I'll ping the Cardboard team about it, though. 

Thanks, yes, let's talk to them, and we can also look at the source directly
(the API docs are very inconsistent in their discussions of thread safety, often
making assumptions about behavior only taking place on the UI thread).

[achuithb, 2015-03-30 17:40:33]

On 2015/03/30 16:45:25, bajones wrote:
> Hm... My reviewers list didn't update properly for some reason. Let's try this
> again:
> 
> achuith@: Could you give a quick thumbs up to the single line
> chrome_restart_request.cc change?

lgtm for chrome_restart_request.cc

[jdduke (slow), 2015-04-01 17:13:35]

On 2015/03/30 16:47:48, jdduke wrote:
> Thanks, yes, let's talk to them, and we can also look at the source directly
> (the API docs are very inconsistent in their discussions of thread safety,
often
> making assumptions about behavior only taking place on the UI thread).

Just an update here for completeness, dcoz@ communicated to us that the
HeadMountedDisplay-related classes are not currently thread-safe. We'll either
need to rethink the synchronous API here (which we'll probably have to do
regardless), or push the SDK devs to make the API thread-safe.

[bajones, 2015-04-09 20:50:14]

On 2015/04/01 17:13:35, jdduke wrote:
> Just an update here for completeness, dcoz@ communicated to us that the
> HeadMountedDisplay-related classes are not currently thread-safe. We'll either
> need to rethink the synchronous API here (which we'll probably have to do
> regardless), or push the SDK devs to make the API thread-safe.

I'd like to clarify this a bit: The current code already asserts that all access
to the VR backend is done on a single thread, the IO thread in this case. In
that sense it should be thread safe. Is your primary concern that the SDK may
assume it's being accessed from a UI thread?

On the Synchronous API front I agree that it needs some refactoring. The
getVRDevices call is already specced on the Blink side as a Promise, I had just
put off making it actually async "till I got the initial functionality working".
Shouldn't be a problem to do that now. I can also probably avoid the
getRenderTargetRects call entirely for now by setting a couple of limits on the
Blink side. The problematic call is getSensorState. The entire point is that it
gets the state of the headset with as little latency as possible to be used in a
render loop. Making it evented or async will introduce problems for this use
case that we already see with Orientation events such as missing frame
schedules. Not sure what the right resolution is for that.

[jdduke (slow), 2015-04-09 21:35:26]

On 2015/04/09 20:50:14, bajones wrote:
> On 2015/04/01 17:13:35, jdduke wrote:
> > Just an update here for completeness, dcoz@ communicated to us that the
> > HeadMountedDisplay-related classes are not currently thread-safe. We'll
either
> > need to rethink the synchronous API here (which we'll probably have to do
> > regardless), or push the SDK devs to make the API thread-safe.
> 
> I'd like to clarify this a bit: The current code already asserts that all
access
> to the VR backend is done on a single thread, the IO thread in this case. In
> that sense it should be thread safe. Is your primary concern that the SDK may
> assume it's being accessed from a UI thread?

Right =/. I haven't looked at their source, so I haven't a clue what the current
assumptions are, but "let's just assume we're on the UI thread" is not
unprecedented in Android code.

[bajones, 2015-04-16 21:16:26]

bajones@chromium.org changed reviewers:
+ brettw@chromium.org

[bajones, 2015-04-16 21:16:27]

Adding brettw@ to reviewers to approve documentation files in third_party.

Just landed some updates to the Blink side of this, updated this CL to match.
Along with several other bits of cleanup VRHostMsg_GetVRDevices is now
asynchronous and the previously synchronous VRHostMsg_GetRenderRects has been
removed. Should alleviate some of the API concerns. Also in an email thread
David Coz confirmed that the Cardboard SDK doesn't make assumptions about
running on any particular thread. Still haven't a definitive answer on the
Cardboard.jar security review, so I'll keep following up on that.

jdduke@: Aside from the pending security review are there any lingering
questions/concerns that you feel should block landing this initial patch?

[jdduke (slow), 2015-04-16 22:24:50]

No objections here, thanks for addressing the synchronous API concerns.

https://codereview.chromium.org/829803003/diff/340001/content/browser/vr/card...
File content/browser/vr/cardboard/cardboard_vr_device.cc (right):

https://codereview.chromium.org/829803003/diff/340001/content/browser/vr/card...
content/browser/vr/cardboard/cardboard_vr_device.cc:63: , frame_index_(0)
Nit: This looks like Blink constructor formatting, and the brace shouldn't be on
a newline.

https://codereview.chromium.org/829803003/diff/340001/content/browser/vr/card...
content/browser/vr/cardboard/cardboard_vr_device.cc:159: JNIEnv* env =
AttachCurrentThread();
How requently is this called? It may not be worth optimizing now, but it looks
like we incur ~5 JNI hops here. Again, probably not a big deal for now.

https://codereview.chromium.org/829803003/diff/340001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/input/CardboardVRDevice.java
(right):

https://codereview.chromium.org/829803003/diff/340001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/CardboardVRDevice.java:25:
private HeadMountedDisplayManager mHMDManager;
Nit: All of these member variables can be "final".

https://codereview.chromium.org/829803003/diff/340001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/CardboardVRDevice.java:27:
private long mVrDevicePtr;
Hmm, this pointer looks unused? Can we remove it?

[bajones, 2015-04-17 00:38:42]

Nits addressed, thanks again!

https://codereview.chromium.org/829803003/diff/340001/content/browser/vr/card...
File content/browser/vr/cardboard/cardboard_vr_device.cc (right):

https://codereview.chromium.org/829803003/diff/340001/content/browser/vr/card...
content/browser/vr/cardboard/cardboard_vr_device.cc:159: JNIEnv* env =
AttachCurrentThread();
On 2015/04/16 22:24:50, jdduke wrote:
> How requently is this called? It may not be worth optimizing now, but it looks
> like we incur ~5 JNI hops here. Again, probably not a big deal for now.

It'll get called once per frame for most VR apps, which isn't a ton but also
isn't trivial. I'll look at optimizing it in an upcoming patch. May be that
future changes to the Cardboard SDK also help with this.

[jdduke (slow), 2015-04-17 00:43:00]

On 2015/04/17 00:38:42, bajones wrote:
> Nits addressed, thanks again!
> 
>
https://codereview.chromium.org/829803003/diff/340001/content/browser/vr/card...
> File content/browser/vr/cardboard/cardboard_vr_device.cc (right):
> 
>
https://codereview.chromium.org/829803003/diff/340001/content/browser/vr/card...
> content/browser/vr/cardboard/cardboard_vr_device.cc:159: JNIEnv* env =
> AttachCurrentThread();
> On 2015/04/16 22:24:50, jdduke wrote:
> > How requently is this called? It may not be worth optimizing now, but it
looks
> > like we incur ~5 JNI hops here. Again, probably not a big deal for now.
> 
> It'll get called once per frame for most VR apps, which isn't a ton but also
> isn't trivial. I'll look at optimizing it in an upcoming patch. May be that
> future changes to the Cardboard SDK also help with this.

Yeah, it's definitely fine as-is.

Is there a .jar you meant to add to this patch? I don't see it, is it added
separately? 

Ted gets back next Tuesday, ideally we could wait for his final approval if you
don't mind waiting a few more days.

[bajones, 2015-04-17 06:11:21]

On 2015/04/17 00:43:00, jdduke wrote:
> Is there a .jar you meant to add to this patch? I don't see it, is it added
> separately? 

It's added by the update to DEPS. We have a proper mirror for the cardboard SDK
repo now.

> 
> Ted gets back next Tuesday, ideally we could wait for his final approval if
you
> don't mind waiting a few more days.

Not at all! A lot of this is new territory for me within Chrome so I appreciate
all the feedback I can get.

[bajones, 2015-04-22 23:26:02]

tedchoc@: Would you mind taking another look at this as jdduke@ suggested? I'd
appreciate your feedback.

[Ted C, 2015-04-24 01:06:07]

android side lgtm

Got a bit carried away and there are some style nits else where (still the
original things you asked me for seem good).

https://codereview.chromium.org/829803003/diff/380001/content/browser/rendere...
File content/browser/renderer_host/vr_message_filter.cc (right):

https://codereview.chromium.org/829803003/diff/380001/content/browser/rendere...
content/browser/renderer_host/vr_message_filter.cc:51: if (device) {
typically you don't need braces unless it's confusing without them for these one
liners...this applies throughout the change.

also fun fact.  in java braces are always required unless the statement and
condition can all fit on the same line.  in c++, the condition and statement go
on separate lines.  Unless it's a thursday of an odd month.

https://codereview.chromium.org/829803003/diff/380001/content/browser/vr/card...
File content/browser/vr/cardboard/cardboard_vr_device.cc (right):

https://codereview.chromium.org/829803003/diff/380001/content/browser/vr/card...
content/browser/vr/cardboard/cardboard_vr_device.cc:28: if ( fTrace > 0.0f ) {
spaces around "fTrace > 0.0f" not needed

https://codereview.chromium.org/829803003/diff/380001/content/browser/vr/card...
content/browser/vr/cardboard/cardboard_vr_device.cc:55: }
I would add
"  // namespace"
here for clarity

https://codereview.chromium.org/829803003/diff/380001/content/browser/vr/card...
content/browser/vr/cardboard/cardboard_vr_device.cc:71:
Java_CardboardVRDevice_stopTracking(AttachCurrentThread(),
in stopTracking, there is no corresponding unregister head tracker or anything
in the cardboard API right?

Just making sure we're not leaking the java side of things unexpectedly.

https://codereview.chromium.org/829803003/diff/380001/content/browser/vr/card...
File content/browser/vr/cardboard/cardboard_vr_device.h (right):

https://codereview.chromium.org/829803003/diff/380001/content/browser/vr/card...
content/browser/vr/cardboard/cardboard_vr_device.h:30: blink::WebVRVector4&
orientation,
+2 indent of both these lines

https://codereview.chromium.org/829803003/diff/380001/content/browser/vr/card...
File content/browser/vr/cardboard/cardboard_vr_device_provider.h (right):

https://codereview.chromium.org/829803003/diff/380001/content/browser/vr/card...
content/browser/vr/cardboard/cardboard_vr_device_provider.h:15: struct
ovrHmdDesc_;
what is this for?

https://codereview.chromium.org/829803003/diff/380001/content/browser/vr/test...
File content/browser/vr/test/fake_vr_device.cc (right):

https://codereview.chromium.org/829803003/diff/380001/content/browser/vr/test...
content/browser/vr/test/fake_vr_device.cc:39: void
FakeVRDevice::GetRenderTargetRects(
blank line above.

https://codereview.chromium.org/829803003/diff/380001/content/browser/vr/test...
File content/browser/vr/test/fake_vr_device.h (right):

https://codereview.chromium.org/829803003/diff/380001/content/browser/vr/test...
content/browser/vr/test/fake_vr_device.h:1: // Copyright (c) 2015 The Chromium
Authors. All rights reserved.
no (c) here or any other places.

https://codereview.chromium.org/829803003/diff/380001/content/browser/vr/test...
File content/browser/vr/test/fake_vr_device_provider.cc (right):

https://codereview.chromium.org/829803003/diff/380001/content/browser/vr/test...
content/browser/vr/test/fake_vr_device_provider.cc:34: for(iter =
devices_.begin(); iter != devices_.end(); ++iter) {
space after for

Also, could this just be?
for (auto device : devices_) {
  devices.push_back(device);
}

https://codereview.chromium.org/829803003/diff/380001/content/browser/vr/test...
content/browser/vr/test/fake_vr_device_provider.cc:35: devices.push_back(*iter);
-2 indent

https://codereview.chromium.org/829803003/diff/380001/content/browser/vr/vr_d...
File content/browser/vr/vr_device_provider.h (right):

https://codereview.chromium.org/829803003/diff/380001/content/browser/vr/vr_d...
content/browser/vr/vr_device_provider.h:27: #endif  //
CONTENT_BROWSER_VR_DEVICE_VR_DEVICE_PROVIDER_H
doesn't match above (extra device after the first VR)

https://codereview.chromium.org/829803003/diff/380001/content/browser/vr/vr_s...
File content/browser/vr/vr_service.cc (right):

https://codereview.chromium.org/829803003/diff/380001/content/browser/vr/vr_s...
content/browser/vr/vr_service.cc:100: return NULL;
nullptr (and other places instead of NULL)

https://codereview.chromium.org/829803003/diff/380001/content/browser/vr/vr_s...
content/browser/vr/vr_service.cc:131: if (vr_initialized_ == true) {
no need for == true

https://codereview.chromium.org/829803003/diff/380001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/829803003/diff/380001/content/renderer/render...
content/renderer/render_frame_impl.cc:3800: vr_dispatcher_ = new
VRDispatcher(this);
who deletes this?  should it be a scoped_ptr like the permission_client_ above?

https://codereview.chromium.org/829803003/diff/380001/content/renderer/render...
content/renderer/render_frame_impl.cc:3804: 
extra blank line here.

[bajones, 2015-04-24 21:25:53]

Thanks for the review! Addressed most everything, with a couple of comments
below:

On 2015/04/24 01:06:07, Ted C wrote:
>
https://codereview.chromium.org/829803003/diff/380001/content/browser/vr/card...
> content/browser/vr/cardboard/cardboard_vr_device.cc:71:
> Java_CardboardVRDevice_stopTracking(AttachCurrentThread(),
> in stopTracking, there is no corresponding unregister head tracker or anything
> in the cardboard API right?

Not that I've been able to find. 

>
https://codereview.chromium.org/829803003/diff/380001/content/browser/vr/card...
> File content/browser/vr/cardboard/cardboard_vr_device_provider.h (right):
> 
>
https://codereview.chromium.org/829803003/diff/380001/content/browser/vr/card...
> content/browser/vr/cardboard/cardboard_vr_device_provider.h:15: struct
> ovrHmdDesc_;
> what is this for?

It's for embarrassing me by exposing my sloppy copy/pasting ways, of course. :)
It's a leftover from a second VRDevice implementation I have waiting in the
wings for Oculus Rift devices. Thanks for catching that!

>
https://codereview.chromium.org/829803003/diff/380001/content/renderer/render...
> content/renderer/render_frame_impl.cc:3800: vr_dispatcher_ = new
> VRDispatcher(this);
> who deletes this?  should it be a scoped_ptr like the permission_client_
above?

A better reference would be the geolocation_dispatcher_ or
screen_orientation_dispatcher_, which both have similar behavior. VRDispatcher
inherits RenderFrameObserver, which is implicitly deleted when the
RenderFrameImpl is destroyed. I've added a comment to that effect, since I was
also confused by it at first. (My first implementation did have it as a
scoped_ptr, but that caused all sorts of crashing.)

[jdduke (slow), 2015-04-24 21:46:08]

lgtm too for what it's worth.

https://codereview.chromium.org/829803003/diff/400001/content/browser/vr/card...
File content/browser/vr/cardboard/cardboard_vr_device.cc (right):

https://codereview.chromium.org/829803003/diff/400001/content/browser/vr/card...
content/browser/vr/cardboard/cardboard_vr_device.cc:63: frame_index_(0) {
Would it be worth having a ThreadChecker for this class, and we can DCHECK that
each of the JNI methods are all called on the same thread?

[bajones, 2015-04-24 22:03:47]

On 2015/04/24 21:46:08, jdduke wrote:
>
https://codereview.chromium.org/829803003/diff/400001/content/browser/vr/card...
> content/browser/vr/cardboard/cardboard_vr_device.cc:63: frame_index_(0) {
> Would it be worth having a ThreadChecker for this class, and we can DCHECK
that
> each of the JNI methods are all called on the same thread?

The only thing that accesses this object is the VRService and it already has a
ThreadChecker to do exactly that.

[mdempsky, 2015-05-11 19:24:59]

mdempsky@chromium.org changed reviewers:
+ mdempsky@chromium.org

[mdempsky, 2015-05-11 19:25:02]

(Bunch of quick feedback while trying to familiarize myself with the code for
security review.)

https://codereview.chromium.org/829803003/diff/400001/content/browser/vr/card...
File content/browser/vr/cardboard/cardboard_vr_device.cc (right):

https://codereview.chromium.org/829803003/diff/400001/content/browser/vr/card...
content/browser/vr/cardboard/cardboard_vr_device.cc:25: void
MatrixToOrientationQuat(const float m[16], blink::WebVRVector4& out) {
Nit: Google C++ style guide disallows non-const reference parameters:
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Reference_Ar...

Also, are these formulas taken from somewhere?  They're non-trivial enough that
it might be nice to reference somewhere authoritative.

Lastly, I think the formulas assume m[15] == 1.0f.  Is that guaranteed to always
be true?  (I suspect it is in practice, but briefly scanning the Cardboard SDK
documentation, it doesn't seem promised.)  Is it worth adding a DCHECK or
something to verify?

https://codereview.chromium.org/829803003/diff/400001/content/browser/vr/card...
content/browser/vr/cardboard/cardboard_vr_device.cc:155: blink::WebVRVector4&
orientation,
Nit: Non-const reference parameters.

https://codereview.chromium.org/829803003/diff/400001/content/browser/vr/card...
File content/browser/vr/cardboard/cardboard_vr_device.h (right):

https://codereview.chromium.org/829803003/diff/400001/content/browser/vr/card...
content/browser/vr/cardboard/cardboard_vr_device.h:21:
CardboardVRDevice(VRDeviceProvider* provider);
Nit: Add "explicit" for single-argument constructors:
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Explicit_Con...

https://codereview.chromium.org/829803003/diff/400001/content/browser/vr/test...
File content/browser/vr/test/fake_vr_device.cc (right):

https://codereview.chromium.org/829803003/diff/400001/content/browser/vr/test...
content/browser/vr/test/fake_vr_device.cc:10: : VRDevice(provider) {
Nit: Need explicit initialization for device_, state_, width_, and height_. 
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Initialization

https://codereview.chromium.org/829803003/diff/400001/content/browser/vr/test...
File content/browser/vr/test/fake_vr_device.h (right):

https://codereview.chromium.org/829803003/diff/400001/content/browser/vr/test...
content/browser/vr/test/fake_vr_device.h:16: FakeVRDevice(VRDeviceProvider*
provider);
Nit: Add explicit.

https://codereview.chromium.org/829803003/diff/400001/content/browser/vr/test...
content/browser/vr/test/fake_vr_device.h:37: };
Nit: Use DISALLOW_COPY_AND_ASSIGN:
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Copyable_Mov...

https://codereview.chromium.org/829803003/diff/400001/content/browser/vr/vr_d...
File content/browser/vr/vr_device.cc (right):

https://codereview.chromium.org/829803003/diff/400001/content/browser/vr/vr_d...
content/browser/vr/vr_device.cc:15: next_id_++;
Are VRDevices guaranteed to be created in a single thread?

What happens if next_id_ wraps around?  Is it possible for an attacker on the
web to create and destroy 2^32 VRDevice objects so that IDs are reused?

https://codereview.chromium.org/829803003/diff/400001/content/browser/vr/vr_d...
File content/browser/vr/vr_device.h (right):

https://codereview.chromium.org/829803003/diff/400001/content/browser/vr/vr_d...
content/browser/vr/vr_device.h:25: VRDevice(VRDeviceProvider* provider);
Nit: Add "explicit".

https://codereview.chromium.org/829803003/diff/400001/content/browser/vr/vr_d...
content/browser/vr/vr_device.h:40: };
Nit: DISALLOW_COPY_AND_ASSIGN.

https://codereview.chromium.org/829803003/diff/400001/content/browser/vr/vr_d...
File content/browser/vr/vr_device_provider.h (right):

https://codereview.chromium.org/829803003/diff/400001/content/browser/vr/vr_d...
content/browser/vr/vr_device_provider.h:19: virtual void
GetDevices(std::vector<VRDevice*>& devices) = 0;
Nit: Non-const reference parameter.

https://codereview.chromium.org/829803003/diff/400001/content/browser/vr/vr_s...
File content/browser/vr/vr_service.h (right):

https://codereview.chromium.org/829803003/diff/400001/content/browser/vr/vr_s...
content/browser/vr/vr_service.h:27: class CONTENT_EXPORT VRService {
Are base::LazyInstance or Singleton not appropriate here?

https://codereview.chromium.org/829803003/diff/400001/content/browser/vr/vr_s...
content/browser/vr/vr_service.h:54: typedef
std::vector<linked_ptr<VRDeviceProvider>> ProviderList;
FYI: I believe Chromium style is to prefer "using ProviderList =
std::vector<linked_ptr<VRDeviceProvider>>" for new code:
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/8dOAMzgR4ao/G0zaw...

(Below too.)

https://codereview.chromium.org/829803003/diff/400001/content/common/vr_param...
File content/common/vr_param_traits.cc (right):

https://codereview.chromium.org/829803003/diff/400001/content/common/vr_param...
content/common/vr_param_traits.cc:17: void LogWebUCharString(
(I'm surprised this functionality doesn't already exist somewhere in Chromium?)

https://codereview.chromium.org/829803003/diff/400001/content/common/vr_param...
content/common/vr_param_traits.cc:29: }
Nit: "// namespace".

https://codereview.chromium.org/829803003/diff/400001/content/common/vr_param...
content/common/vr_param_traits.cc:57: LogWebUCharString(p.deviceName,
WebVRDevice::deviceNameLengthCap, l);
Should there be delimiters between the logged fields?

https://codereview.chromium.org/829803003/diff/400001/content/renderer/vr_dis...
File content/renderer/vr_dispatcher.h (right):

https://codereview.chromium.org/829803003/diff/400001/content/renderer/vr_dis...
content/renderer/vr_dispatcher.h:18: NON_EXPORTED_BASE(public
blink::WebVRClient){
Nit: Missing a space between ")" and "{".  Please run clang-format.

https://codereview.chromium.org/829803003/diff/400001/content/renderer/vr_dis...
content/renderer/vr_dispatcher.h:20: VRDispatcher(RenderFrame* render_frame);
Nit: Add "explicit".

https://codereview.chromium.org/829803003/diff/400001/content/renderer/vr_dis...
content/renderer/vr_dispatcher.h:26: // blink::WebVRClient implementation.
Since blink::WebVRClient is a private base class, you can make these member
function overrides private too.

[mdempsky, 2015-05-11 19:34:47]

https://codereview.chromium.org/829803003/diff/400001/content/renderer/vr_dis...
File content/renderer/vr_dispatcher.h (right):

https://codereview.chromium.org/829803003/diff/400001/content/renderer/vr_dis...
content/renderer/vr_dispatcher.h:26: // blink::WebVRClient implementation.
On 2015/05/11 19:25:01, mdempsky wrote:
> Since blink::WebVRClient is a private base class, you can make these member
> function overrides private too.

Oops, disregard this.  I misunderstood the purpose of NON_EXPORTED_BASE and
missed the "public" keyword.

[mdempsky, 2015-05-11 22:11:09]

https://codereview.chromium.org/829803003/diff/400001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/input/CardboardVRDevice.java
(right):

https://codereview.chromium.org/829803003/diff/400001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/CardboardVRDevice.java:14:
import com.google.vrtoolkit.cardboard.sensors.HeadTracker;
This appears to be a (publicly) undocumented part of the Cardboard API. If you
haven't already, it's probably worth pinging the Cardboard folks to make sure
they know you're using it like this.

https://codereview.chromium.org/829803003/diff/400001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/CardboardVRDevice.java:25:
private final HeadMountedDisplayManager mHMDManager;
According to the documentation and examples I can find, it looks you're supposed
to call onPause and onResume when the ambient Activity has been paused/resumed. 
But I don't see that happening here.

[bajones, 2015-05-12 00:13:39]

Thanks for the review, mdempsky@! I've got a patch in progress that I'll
probably upload tomorrow, but in the meantime I've got a few responses to your
comments:

https://codereview.chromium.org/829803003/diff/400001/content/browser/vr/card...
File content/browser/vr/cardboard/cardboard_vr_device.cc (right):

https://codereview.chromium.org/829803003/diff/400001/content/browser/vr/card...
content/browser/vr/cardboard/cardboard_vr_device.cc:25: void
MatrixToOrientationQuat(const float m[16], blink::WebVRVector4& out) {
On 2015/05/11 19:25:01, mdempsky wrote:
> Also, are these formulas taken from somewhere?  They're non-trivial enough
that
> it might be nice to reference somewhere authoritative.

Ooh... that's a really good question. This was ported from my Javascript matrix
library which was ported from one of my older C++ libraries which was evolved
from several previous versions of the same library which was referenced from I
don't even remember what anymore. Carmack is probably at the bottom of the pile
somewhere. :)

I'm happy to dig around and find a reasonably authoritative source for the
algorithm and point to it. Certainly didn't spring fully formed from my head. 
 
> Lastly, I think the formulas assume m[15] == 1.0f.  Is that guaranteed to
always
> be true?  (I suspect it is in practice, but briefly scanning the Cardboard SDK
> documentation, it doesn't seem promised.)  Is it worth adding a DCHECK or
> something to verify?

Good catch! I'll throw a check into here.

https://codereview.chromium.org/829803003/diff/400001/content/browser/vr/card...
content/browser/vr/cardboard/cardboard_vr_device.cc:63: frame_index_(0) {
On 2015/04/24 21:46:07, jdduke wrote:
> Would it be worth having a ThreadChecker for this class, and we can DCHECK
that
> each of the JNI methods are all called on the same thread?

This class is only called from the VRService, which already has a ThreadChecker
for exactly that purpose.

https://codereview.chromium.org/829803003/diff/400001/content/browser/vr/vr_d...
File content/browser/vr/vr_device.cc (right):

https://codereview.chromium.org/829803003/diff/400001/content/browser/vr/vr_d...
content/browser/vr/vr_device.cc:15: next_id_++;
On 2015/05/11 19:25:01, mdempsky wrote:
> Are VRDevices guaranteed to be created in a single thread?
> 
> What happens if next_id_ wraps around?  Is it possible for an attacker on the
> web to create and destroy 2^32 VRDevice objects so that IDs are reused?

Theoretically, I suppose if you opened a tab, queried VR devices, closed the
tab, and then repeated that a couple billion times, sure. I think at that point
I'm happy to stop returning VR devices, though. I'll add some simple logic to
handle/prevent the wraparound case.

https://codereview.chromium.org/829803003/diff/400001/content/browser/vr/vr_d...
File content/browser/vr/vr_device.h (right):

https://codereview.chromium.org/829803003/diff/400001/content/browser/vr/vr_d...
content/browser/vr/vr_device.h:40: };
On 2015/05/11 19:25:01, mdempsky wrote:
> Nit: DISALLOW_COPY_AND_ASSIGN.

Can't DISALLOW_COPY_AND_ASSIGN on an abstract class.

https://codereview.chromium.org/829803003/diff/400001/content/browser/vr/vr_s...
File content/browser/vr/vr_service.h (right):

https://codereview.chromium.org/829803003/diff/400001/content/browser/vr/vr_s...
content/browser/vr/vr_service.h:27: class CONTENT_EXPORT VRService {
On 2015/05/11 19:25:01, mdempsky wrote:
> Are base::LazyInstance or Singleton not appropriate here?

This was a Singleton in a previous patchset. sievers@ suggested refactoring it
in https://codereview.chromium.org/829803003/#msg15 to prevent leaking the
singleton for the lifetime of the process.

[mdempsky_google, 2015-05-12 00:25:13]

mdempsky@google.com changed reviewers:
+ mdempsky@google.com

[mdempsky_google, 2015-05-12 00:25:16]

https://codereview.chromium.org/829803003/diff/400001/content/browser/vr/vr_d...
File content/browser/vr/vr_device.h (right):

https://codereview.chromium.org/829803003/diff/400001/content/browser/vr/vr_d...
content/browser/vr/vr_device.h:40: };
On 2015/05/12 00:13:39, bajones wrote:
> On 2015/05/11 19:25:01, mdempsky wrote:
> > Nit: DISALLOW_COPY_AND_ASSIGN.
> 
> Can't DISALLOW_COPY_AND_ASSIGN on an abstract class.

Can you elaborate?  Abstract classes still have implicit copy constructors and
assignment operators, and there are other abstract classes in Chromium that have
DISALLOW_COPY_AND_ASSIGN.  It's not obvious to me why it wouldn't work here too.

[mdempsky, 2015-05-12 00:25:52]

mdempsky@chromium.org changed reviewers:
- mdempsky@google.com

[bajones, 2015-05-12 16:24:57]

On 2015/05/12 00:25:16, mdempsky_google wrote:
>
https://codereview.chromium.org/829803003/diff/400001/content/browser/vr/vr_d...
> File content/browser/vr/vr_device.h (right):
> 
>
https://codereview.chromium.org/829803003/diff/400001/content/browser/vr/vr_d...
> content/browser/vr/vr_device.h:40: };
> On 2015/05/12 00:13:39, bajones wrote:
> > On 2015/05/11 19:25:01, mdempsky wrote:
> > > Nit: DISALLOW_COPY_AND_ASSIGN.
> > 
> > Can't DISALLOW_COPY_AND_ASSIGN on an abstract class.
> 
> Can you elaborate?  Abstract classes still have implicit copy constructors and
> assignment operators, and there are other abstract classes in Chromium that
have
> DISALLOW_COPY_AND_ASSIGN.  It's not obvious to me why it wouldn't work here
too.

Attempting to use DISSALLOW_COPY_AND_ASSIGN with VRDevice yields the following
errors:

../../content/browser/vr/vr_device.h:41:36: error: ISO C++ forbids declaration
of 'DISALLOW_COPY_AND_ASSIGN' with no type [-fpermissive]
   DISALLOW_COPY_AND_ASSIGN(VRDevice);
                                    ^
../../content/browser/vr/vr_device.h:41:28: error: cannot declare parameter to
be of abstract type 'content::VRDevice'
   DISALLOW_COPY_AND_ASSIGN(VRDevice);
                            ^

[bajones, 2015-05-12 16:38:18]

On 2015/05/12 16:24:57, bajones wrote:
> Attempting to use DISSALLOW_COPY_AND_ASSIGN with VRDevice yields the following
> errors:
> 
> ../../content/browser/vr/vr_device.h:41:36: error: ISO C++ forbids declaration
> of 'DISALLOW_COPY_AND_ASSIGN' with no type [-fpermissive]
>    DISALLOW_COPY_AND_ASSIGN(VRDevice);
>                                     ^
> ../../content/browser/vr/vr_device.h:41:28: error: cannot declare parameter to
> be of abstract type 'content::VRDevice'
>    DISALLOW_COPY_AND_ASSIGN(VRDevice);
>                             ^

Wait... scratch that. I'm mis-reading the error message. I don't have the right
include to expose DISALLOW_COPY_AND_ASSIGN. :P Fixing now.

[kerz_chromium, 2015-05-13 05:14:44]

kerz@chromium.org changed reviewers:
+ kerz@chromium.org

[kerz_chromium, 2015-05-13 05:14:46]

How much size will this add to android builds with the new SDK?

[bajones, 2015-05-13 18:32:17]

On 2015/05/13 05:14:46, kerz_chromium wrote:
> How much size will this add to android builds with the new SDK?

I've run these numbers before (comment #8:
https://codereview.chromium.org/829803003/#msg8) but since the SDK and Chrome
have changed over time I just did the comparison again:

ChromeShell Top of Tree: 33,233,217 bytes
ChromeShell with WebVR: 33,286,974 bytes

For a difference of 52kb (53,757 bytes)

This is a drop from the previous number (69Kb), which I believe is accounted for
by my more recent ChromeShell builds using ProGuard.

[cpu_(ooo_6.6-7.5), 2015-05-15 23:28:05]

cpu@chromium.org changed reviewers:
+ cpu@chromium.org

[cpu_(ooo_6.6-7.5), 2015-05-15 23:28:07]

https://codereview.chromium.org/829803003/diff/420001/content/browser/vr/card...
File content/browser/vr/cardboard/cardboard_vr_device.h (right):

https://codereview.chromium.org/829803003/diff/420001/content/browser/vr/card...
content/browser/vr/cardboard/cardboard_vr_device.h:18: public:
this is android / clang only?

Seems strange that there is no indication in the path it is

https://codereview.chromium.org/829803003/diff/420001/third_party/cardboard-j...
File third_party/cardboard-java/BUILD.gn (left):

https://codereview.chromium.org/829803003/diff/420001/third_party/cardboard-j...
third_party/cardboard-java/BUILD.gn:4: 
adding a third party library is done in a separate CL and requires an
specialized review. 

Start with https://www.chromium.org/developers/adding-3rd-party-libraries

[bajones, 2015-05-29 21:36:59]

bajones@chromium.org changed reviewers:
+ rockot@chromium.org, tsepez@chromium.org

[bajones, 2015-05-29 21:37:01]

+tsepez@ For .mojom review, and +rockot@ for thoughts on my use of Mojo.

At the request of the eng review I've converted the IPC for this feature to
Mojo. It's the first time I've used Mojo, so I'm happy to fix anything that may
look awkward or non-standard. Additionally I'm not sure if content/browser/vr is
the most appropriate place for the bulk of this features code now that it's been
Mojofied. If it would fit better somewhere else I'm happy to relocate!

Quick renaming note for reviewers: What used to be VRService is now
VRDeviceManager and the class previously known as VRMessageFilter is now
VRService (to better match Mojo's verbiage).

Adding the Cardboard library has been moved out to it's own CL.
(https://codereview.chromium.org/1158413004/) That depends on a roll of
android_protobuf (https://codereview.chromium.org/1149283005/), which depends on
a small change to third_party/cacheinvalidation. That change will probably have
to wait until that library is merged into the chrome repo.
(https://code.google.com/p/chromium/issues/detail?id=472898) Yay for dependency
chains! I'd still appreciate feedback on this CL while that gets sorted so that
I have plenty of time to address any design concerns.

[Ken Rockot(use gerrit already), 2015-05-31 23:43:47]

https://codereview.chromium.org/829803003/diff/440001/content/browser/rendere...
File content/browser/renderer_host/vr_message_filter.cc (right):

https://codereview.chromium.org/829803003/diff/440001/content/browser/rendere...
content/browser/renderer_host/vr_message_filter.cc:1: // Copyright 2015 The
Chromium Authors. All rights reserved.
so should this file be gone now?

https://codereview.chromium.org/829803003/diff/440001/content/browser/vr/vr_s...
File content/browser/vr/vr_service_impl.h (right):

https://codereview.chromium.org/829803003/diff/440001/content/browser/vr/vr_s...
content/browser/vr/vr_service_impl.h:17: class VRServiceImpl : public
mojo::InterfaceImpl<VRService> {
InterfaceImpl is deprecated, so please don't use it. You can implement VRService
directly, and then there are a few options for how to bind |request| to your
impl: you can use a mojo::Binding, or a mojo::StrongBinding.

A StrongBinding owns its bound impl and deletes the impl when the bound
MessagePipe is closed. Both Binding and StrongBinding allow you to
set_error_handler to some implementation of mojo::ErrorHandler to handle pipe
closing if you have a need for that. In this case a StrongBinding would work
fine and would effectively mirror the same behavior you get with InterfaceImpl.

However... I don't really see why VRDeviceManager and VRServiceImpl have to be
two separate classes. You don't appear to need a different service
implementation for each request, (i.e. your only per-impl state is "am i
registered with the device manager?"). So alternatively you could merge the two
classes and use a single mojo::WeakBindingSet<VRService> (defined in
//mojo/common/weak_binding_set.h) to bind all incoming requests to a lazy
singleton instance.

To get the same ref-counting behavior as you have now (i.e. cleaning up the
singleton when the last connection closes), you would need to add an accessor to
WeakBindingSet to test if the bindings set is empty (that really should exist; I
guess nobody's needed it yet). Then in addition to VRService you'd also
implement mojo::ErrorHandler and set yourself as the error handler for the
WeakBindingSet; your OnConnectionError impl would delete |this| if the
WeakBindingSet is empty.

https://codereview.chromium.org/829803003/diff/440001/content/renderer/vr/vr_...
File content/renderer/vr/vr_dispatcher.cc (right):

https://codereview.chromium.org/829803003/diff/440001/content/renderer/vr/vr_...
content/renderer/vr/vr_dispatcher.cc:20: if (!vr_service_.get()) {
nit: vr_service_ is testable, no need to get()

https://codereview.chromium.org/829803003/diff/440001/content/renderer/vr/vr_...
content/renderer/vr/vr_dispatcher.cc:37:
base::Bind(&VRDispatcher::OnGetSensorState,
nit: OnGetSensorState doesn't actually need |this| and could just be static or
defined in an anonymous namespace

https://codereview.chromium.org/829803003/diff/440001/content/renderer/vr/vr_...
content/renderer/vr/vr_dispatcher.cc:41: // TODO: Justify this.
What do you mean? Justify the call to WaitForIncomingResponse (which is easy to
justify if the API must be synchronous), or justify that the API is synchronous?
:)

https://codereview.chromium.org/829803003/diff/440001/content/renderer/vr/vr_...
content/renderer/vr/vr_dispatcher.cc:60: const VRDeviceInfoPtr& mojo_device =
devices[i];
nit: maybe just do devices[i].To<blink::WebVRDevice()?

[Tom Sepez, 2015-06-01 18:26:28]

https://codereview.chromium.org/829803003/diff/440001/content/renderer/vr/vr_...
File content/renderer/vr/vr_type_converters.cc (right):

https://codereview.chromium.org/829803003/diff/440001/content/renderer/vr/vr_...
content/renderer/vr/vr_type_converters.cc:103: memcpy(output.deviceName,
input->deviceName.data(),
blink::WebVRDevice::deviceName is of type unsigned short[128], (e.g. UTF-16) but
I thought that Mojo strings were UTF-8.

[bajones, 2015-06-12 20:16:43]

Okay, I think this might just possibly be "the patch". If it passes review
muster I'll land third-party/cardboard-java and then this patch. Uncomfortably
excited right now! :D

On 2015/05/31 23:43:47, Ken Rockot wrote:
> content/browser/renderer_host/vr_message_filter.cc:1: // Copyright 2015 The
> Chromium Authors. All rights reserved.
> so should this file be gone now?

Indeed. Git always screws me up on file removals. Made doubly sure they're gone
this time.
 
> InterfaceImpl is deprecated, so please don't use it.

Good to know! Fixed.
 
> However... I don't really see why VRDeviceManager and VRServiceImpl have to be
> two separate classes.

Great observation! Updated the code as suggested and I'm much happier with the
result.

> you would need to add an accessor to
> WeakBindingSet to test if the bindings set is empty (that really should exist;
I
> guess nobody's needed it yet).

Added.

> content/renderer/vr/vr_dispatcher.cc:20: if (!vr_service_.get()) {
> nit: vr_service_ is testable, no need to get()

Fixed.
 
> nit: OnGetSensorState doesn't actually need |this| and could just be static or
> defined in an anonymous namespace

Fixed.

> content/renderer/vr/vr_dispatcher.cc:41: // TODO: Justify this.
> What do you mean? Justify the call to WaitForIncomingResponse (which is easy
to
> justify if the API must be synchronous), or justify that the API is
synchronous?
> :)

It was to remind me to add a comment explaining why the API was synchronous.
Updated.

> nit: maybe just do devices[i].To<blink::WebVRDevice()?

Done.

On 2015/06/01 18:26:28, Tom Sepez wrote:
> blink::WebVRDevice::deviceName is of type unsigned short[128], (e.g. UTF-16)
but
> I thought that Mojo strings were UTF-8.

Correct, thanks for catching that! Fixed in the latest patch.

[bajones, 2015-06-15 17:58:46]

Post-weekend review ping, just to make sure I don't get buried in your inboxes.
:)

[Tom Sepez, 2015-06-15 18:03:36]

Mojom LGTM

[Ken Rockot(use gerrit already), 2015-06-15 18:22:55]

mojo-ish bits lgtm

https://codereview.chromium.org/829803003/diff/540001/content/browser/vr/vr_d...
File content/browser/vr/vr_device_manager.cc (right):

https://codereview.chromium.org/829803003/diff/540001/content/browser/vr/vr_d...
content/browser/vr/vr_device_manager.cc:52: delete g_vr_device_manager;
Probably want to null out g_vr_device_manager here so a future GetInstance will
do the right thing?

https://codereview.chromium.org/829803003/diff/540001/content/browser/vr/vr_d...
content/browser/vr/vr_device_manager.cc:82: for (iter = providers_.begin(); iter
!= providers_.end(); ++iter) {
nit: you could simplify a bit with ranged-based loops and auto, here and
elsewhere e.g.:

for (const auto& provider : providers_)
    provider->GetDevice(devices);

https://codereview.chromium.org/829803003/diff/540001/content/browser/vr/vr_d...
File content/browser/vr/vr_device_manager.h (right):

https://codereview.chromium.org/829803003/diff/540001/content/browser/vr/vr_d...
content/browser/vr/vr_device_manager.h:61: DeviceMap devices_;
May be worth noting that the VRDevice objects here are owned by their providers
(and the providers in turn owned by the manager), and therefore this is safe and
not leaky.

[bajones, 2015-06-15 20:46:50]

bajones@chromium.org changed reviewers:
+ jochen@chromium.org, sky@chromium.org

[bajones, 2015-06-15 20:46:52]

jochen@: Could you take a look at the changes to build/common.gypi?

sky@: Made a small change to mojo/common/weak_binding_set.h at rockot@'s
suggestion. Could you review?

On 2015/06/15 18:22:55, Ken Rockot wrote:
> content/browser/vr/vr_device_manager.cc:52: delete g_vr_device_manager;
> Probably want to null out g_vr_device_manager here so a future GetInstance
will
> do the right thing?

nulling out g_vr_device_manager is done in the destructor.
 
>
https://codereview.chromium.org/829803003/diff/540001/content/browser/vr/vr_d...
> content/browser/vr/vr_device_manager.cc:82: for (iter = providers_.begin();
iter
> != providers_.end(); ++iter) {
> nit: you could simplify a bit with ranged-based loops and auto

I keep forgetting we allow that now. Done.

> May be worth noting that the VRDevice objects here are owned by their
providers
> (and the providers in turn owned by the manager), and therefore this is safe
and
> not leaky.

Done. Thanks for the continued feedback!

[sky, 2015-06-15 21:55:56]

LGTM

[jochen (gone - plz use gerrit), 2015-06-16 13:30:31]

build/ lgtm with nit

https://codereview.chromium.org/829803003/diff/560001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/829803003/diff/560001/build/common.gypi#newco...
build/common.gypi:3042: 'defines': ['ENABLE_WEBVR=1'],
nit. just ENABLE_WEBVR, we usually don't check the value of feature defines

[bajones, 2015-06-16 19:29:06]

The CQ bit was checked by bajones@chromium.org

[bajones, 2015-06-16 19:29:07]

The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org,
sievers@chromium.org, mkwst@chromium.org, achuith@chromium.org,
tedchoc@chromium.org, jdduke@chromium.org, rockot@chromium.org,
tsepez@chromium.org, jochen@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/829803003/#ps580001
(title: "Rebase")

[commit-bot: I haz the power, 2015-06-16 19:29:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/829803003/580001

[commit-bot: I haz the power, 2015-06-16 19:43:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-16 19:43:53]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[bajones, 2015-06-17 03:32:57]

The CQ bit was checked by bajones@chromium.org

[bajones, 2015-06-17 03:32:58]

The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org,
jdduke@chromium.org, jochen@chromium.org, sky@chromium.org, rockot@chromium.org,
tedchoc@chromium.org, mkwst@chromium.org, sievers@chromium.org,
tsepez@chromium.org, achuith@chromium.org
Link to the patchset: https://codereview.chromium.org/829803003/#ps790001
(title: "Fix GN, attempt 11")

[commit-bot: I haz the power, 2015-06-17 03:33:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/829803003/790001

[commit-bot: I haz the power, 2015-06-17 05:05:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-17 05:05:41]

Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)

[bajones, 2015-06-17 21:00:15]

bajones@chromium.org changed reviewers:
+ isherman@chromium.org

[bajones, 2015-06-17 21:00:17]

isherman@: Could you please review the one line change to histograms.xml?

[Ilya Sherman, 2015-06-17 21:16:11]

histograms.xml lgtm

[bajones, 2015-06-17 22:36:20]

The CQ bit was checked by bajones@chromium.org

[bajones, 2015-06-17 22:36:22]

The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org,
jdduke@chromium.org, jochen@chromium.org, sky@chromium.org, rockot@chromium.org,
tedchoc@chromium.org, mkwst@chromium.org, sievers@chromium.org,
tsepez@chromium.org, achuith@chromium.org
Link to the patchset: https://codereview.chromium.org/829803003/#ps830001
(title: "Testing")

[commit-bot: I haz the power, 2015-06-17 22:38:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/829803003/830001

[commit-bot: I haz the power, 2015-06-18 00:51:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-18 00:51:10]

Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)

[bajones, 2015-06-18 01:38:57]

The CQ bit was checked by bajones@chromium.org

[commit-bot: I haz the power, 2015-06-18 01:39:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/829803003/830001

[commit-bot: I haz the power, 2015-06-18 03:59:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-18 03:59:32]

Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)

[bajones, 2015-06-18 16:01:11]

The CQ bit was checked by bajones@chromium.org

[commit-bot: I haz the power, 2015-06-18 16:04:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/829803003/830001

[commit-bot: I haz the power, 2015-06-18 16:39:28]

Committed patchset #43 (id:830001)

[commit-bot: I haz the power, 2015-06-18 16:40:35]

Patchset 43 (id:??) landed as
https://crrev.com/4ab52def6d1861a55fd9864b47f56c16e4b423a0
Cr-Commit-Position: refs/heads/master@{#335062}

[bajones, 2015-06-19 23:34:42]

The CQ bit was checked by bajones@chromium.org to run a CQ dry run

[bajones, 2015-06-19 23:34:43]

The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org,
jdduke@chromium.org, jochen@chromium.org, sky@chromium.org, rockot@chromium.org,
tedchoc@chromium.org, mkwst@chromium.org, sievers@chromium.org,
tsepez@chromium.org, achuith@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/829803003/#ps850001
(title: "Stripped out Cardboard code to land this in more bite sized pieces")

[commit-bot: I haz the power, 2015-06-19 23:35:52]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/829803003/850001

[commit-bot: I haz the power, 2015-06-19 23:45:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-19 23:45:28]

Dry run: Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[bajones, 2015-06-22 18:27:51]

Okay, here's my current plan for this CL:

After landing the first time we found a minor conflict with the Cardboard
library and Android M. Sounds like the problem will be addressed in the next
public cardboard-java release, so I'm holding off on landing the android
implementation until then. Shouldn't be too long.

In the meantime, the non-cardboard plumbing bits of this CL didn't seem to have
any problems landing, so I've stripped out the Cardboard portions of this CL and
am going to land the Mojo plumbing bits independently (turned off in the build
files till there's a platform implementation in place.) This will reduce the
scope of the cardboard patch when it lands, making another revert easier if
necessary, and will prevent conflicts/rebasing with this patch in the meantime.

[bajones, 2015-06-22 20:07:08]

The CQ bit was checked by bajones@chromium.org

[bajones, 2015-06-22 20:07:10]

The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org,
jdduke@chromium.org, jochen@chromium.org, sky@chromium.org, rockot@chromium.org,
tedchoc@chromium.org, mkwst@chromium.org, sievers@chromium.org,
tsepez@chromium.org, achuith@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/829803003/#ps890001
(title: "Missed one cardboard reference")

[commit-bot: I haz the power, 2015-06-22 20:08:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/829803003/890001

[commit-bot: I haz the power, 2015-06-22 21:17:55]

Committed patchset #46 (id:890001)

[commit-bot: I haz the power, 2015-06-22 21:19:03]

Patchset 46 (id:??) landed as
https://crrev.com/02443533743dd265b7a8dec4bee855b0e544a727
Cr-Commit-Position: refs/heads/master@{#335561}