Adding WebVR interface to Blink

Adding WebVR interface to Blink

For the Chrome half of this CL, see https://codereview.chromium.org/829803003.

WebVR exposes an interface to Virtual Reality hardware, like Google Cardboard or the Oculus Rift, to Javascript. The intention is to allow developers to create Virtual Reality experiences on the web that are performant as possible while abstracting away the differences between various types of hardware.

The API is being developed in conjunction with Mozilla (http://mozvr.com/content/2014/11/10/mozvr-launches.html).

BUG=389343
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=192045

[bajones, 2015-01-15 00:47:48]

bajones@chromium.org changed reviewers:
+ haraken@chromium.org, mkwst@chromium.org

[bajones, 2015-01-15 00:47:49]

After living in an experimental branch for multiple months I feel like WebVR is
at a state where it can be merged into ToT behind a flag. (Or is pretty close to
that point, in any case.) The initial commits cover a subset of the eventual
desired functionality, only covering orientation output and HMD metrics and
ignoring rendering features, since I anticipate those will be more disruptive.
I'm also only adding support for Android via the Cardboard SDK at this time:
Oculus Rift support is deferred to a future point when I have more time to dive
into licensing minutia.

I'm quite certain that I'm overlooking some "can't see the forest for the
trees"-style problems in this CL, so forgive me if there's anything horribly
off. I'm super happy to answer any questions and provide some insight into where
follow-up CLs to this one will be going.

Even after this lands I anticipate it will remain behind a flag for a while as
the VR landscape settles, but it would be great to more easily get this into the
hands of developers who are eager to apply their web development skills to an
exciting new tech!

http://i.imgur.com/EIlIkWM.gif

[Mike West, 2015-01-15 05:32:10]

Hey Brandon! I'll take a look at the CL later today.

That said, given the lukewarm response to the intent to implement last year, I
think it would be a good idea to reping the list with the proposal to see if
folks' objections still stand. Given that this lives entirely in a module (but
is going to tangentially touch /core, right?), I suspect that experimentation in
trunk should be ok, but I don't want to ignore the opinions other folks have
expressed.

-mike

[bajones, 2015-01-30 23:06:32]

Updated the mailing list a couple of weeks ago (I've been OOO most of this week
or I would have pinged sooner.) The consensus on the mailing list appears to be
that this is OK to land in ToT, with the understanding that it may evolve into
more generic, non-VR-specific functionality with time.

Could you please give the CL another look? Thanks!

[bajones, 2015-02-03 22:22:02]

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

[bajones, 2015-02-03 22:22:02]

Adding more reviewers for non-modules code.

[jochen (gone - plz use gerrit), 2015-02-04 12:52:07]

jochen@chromium.org changed reviewers:
+ dglazkov@chromium.org, philipj@opera.com, rbyers@chromium.org

[jochen (gone - plz use gerrit), 2015-02-04 12:52:07]

+some ppl more involved in this discussion

[Ken Russell (switch to Gerrit), 2015-02-04 19:14:00]

Very nice; the code looks very straightforward and regular. Looks good overall.
A few comments.

https://codereview.chromium.org/848053002/diff/40001/Source/modules/vr/HMDVRD...
File Source/modules/vr/HMDVRDevice.cpp (right):

https://codereview.chromium.org/848053002/diff/40001/Source/modules/vr/HMDVRD...
Source/modules/vr/HMDVRDevice.cpp:42: return VREyeLeft;
This should explicitly check for "left" and return an error if the input string
doesn't match either.

https://codereview.chromium.org/848053002/diff/40001/Source/modules/vr/HMDVRD...
Source/modules/vr/HMDVRDevice.cpp:66: return m_eyeTranslationLeft;
Throughout, all of the code returning the value for the left eye by default
should explicitly check for "left" and throw a JavaScript exception if the input
string is invalid. The spec should be updated to define this behavior for all
entry points.

https://codereview.chromium.org/848053002/diff/40001/Source/modules/vr/HMDVRD...
Source/modules/vr/HMDVRDevice.cpp:114: // TODO: This doesn't change unless you
reset the FOV. Cache it.
Add owner for TODOs throughout.

https://codereview.chromium.org/848053002/diff/40001/public/platform/WebVR.h
File public/platform/WebVR.h (right):

https://codereview.chromium.org/848053002/diff/40001/public/platform/WebVR.h#...
public/platform/WebVR.h:94: // Device identifier (based on manufacturer, model,
etc.).
Is there any reason not to use WebString for both the deviceId and deviceName?
If not, please use them. If they can't be WebString, then document that these
must be null-terminated.

[bajones, 2015-02-05 00:06:14]

On 2015/02/04 at 19:14:00, kbr wrote:
>
https://codereview.chromium.org/848053002/diff/40001/Source/modules/vr/HMDVRD...
> Source/modules/vr/HMDVRDevice.cpp:42: return VREyeLeft;
> This should explicitly check for "left" and return an error if the input
string doesn't match either.

Thanks for the review! I'll have some code updates later but I wanted to remark
that I checked into this and as it turns out the fact that I'm using an enum in
the IDL means I get this behavior for free! If you pass in anything other than
"left" or "right" V8 throws a TypeError. That being said I still feel my code
could be cleaned up so I'll make sure to include that in the next patch.

[philipj_slow, 2015-02-06 01:57:13]

philipj@opera.com changed reviewers:
+ pdr@chromium.org

[philipj_slow, 2015-02-06 01:57:15]

I've taken a quick look at the IDL files and left a few comments, looks pretty
good to me.

Paging pdr@ to comment on the dependency on DOMRect and DOMPoint.

https://codereview.chromium.org/848053002/diff/40001/Source/modules/vr/HMDVRD...
File Source/modules/vr/HMDVRDevice.idl (right):

https://codereview.chromium.org/848053002/diff/40001/Source/modules/vr/HMDVRD...
Source/modules/vr/HMDVRDevice.idl:12: GarbageCollected,
Add TypeChecking=Interface to all interfaces where it would make a difference,
so that you'll get a TypeError for setFieldOfView(window) for example.

https://codereview.chromium.org/848053002/diff/40001/Source/modules/vr/HMDVRD...
Source/modules/vr/HMDVRDevice.idl:14: DOMPoint getEyeTranslation(VREye
whichEye);
I guess you know about
https://groups.google.com/a/chromium.org/d/msg/blink-dev/V_bJNtOg0oM/o4Mi41Yx...
?

https://codereview.chromium.org/848053002/diff/40001/Source/modules/vr/Positi...
File Source/modules/vr/PositionSensorVRDevice.idl (right):

https://codereview.chromium.org/848053002/diff/40001/Source/modules/vr/Positi...
Source/modules/vr/PositionSensorVRDevice.idl:10: void zeroSensor();
zero is an odd verb, and apparently you agree since you called it reset
internally. Change the spec to resetSensor, clearSensor, or something?

https://codereview.chromium.org/848053002/diff/40001/Source/modules/vr/VRFiel...
File Source/modules/vr/VRFieldOfView.idl (right):

https://codereview.chromium.org/848053002/diff/40001/Source/modules/vr/VRFiel...
Source/modules/vr/VRFieldOfView.idl:8: Constructor(),
Maybe have this constructor take a dictionary for developer convenience?

[bajones, 2015-02-06 21:37:49]

Thanks for the feedback! This is exactly the sort of conversation I was hoping
to have about the API. :)

On 2015/02/06 at 01:57:15, philipj wrote:
>
https://codereview.chromium.org/848053002/diff/40001/Source/modules/vr/HMDVRD...
> Source/modules/vr/HMDVRDevice.idl:12: GarbageCollected,
> Add TypeChecking=Interface to all interfaces where it would make a difference,
so that you'll get a TypeError for setFieldOfView(window) for example.

Good suggestion, thanks!

>
https://codereview.chromium.org/848053002/diff/40001/Source/modules/vr/HMDVRD...
> Source/modules/vr/HMDVRDevice.idl:14: DOMPoint getEyeTranslation(VREye
whichEye);
> I guess you know about
https://groups.google.com/a/chromium.org/d/msg/blink-dev/V_bJNtOg0oM/o4Mi41Yx...
?

I did not, thanks for the link. My use of DOMRect/Point here was simply to match
what the Firefox implementation is doing, and since the interfaces already
existed in Chrome it didn't occur to me that there was any concern about the
interfaces. I guess the alternative is to define VRRect and VRPoint types, but I
can't see that as being much better performance wise. It should be noted that
Rect/Point objects returned by this API are never considered live.

> 
>
https://codereview.chromium.org/848053002/diff/40001/Source/modules/vr/Positi...
> File Source/modules/vr/PositionSensorVRDevice.idl (right):
> 
>
https://codereview.chromium.org/848053002/diff/40001/Source/modules/vr/Positi...
> Source/modules/vr/PositionSensorVRDevice.idl:10: void zeroSensor();
> zero is an odd verb, and apparently you agree since you called it reset
internally. Change the spec to resetSensor, clearSensor, or something?

I do agree, and again this was to match the Firefox implementation. I'll ping
Vlad (Firefox WebVR implementor) and see if there's any severe opposition to a
name change.
 
>
https://codereview.chromium.org/848053002/diff/40001/Source/modules/vr/VRFiel...
> File Source/modules/vr/VRFieldOfView.idl (right):
> 
>
https://codereview.chromium.org/848053002/diff/40001/Source/modules/vr/VRFiel...
> Source/modules/vr/VRFieldOfView.idl:8: Constructor(),
> Maybe have this constructor take a dictionary for developer convenience?

Not a bad idea.

[bajones, 2015-02-20 21:21:22]

Apologies for the delay, but the CL has now been updated to take into account
philipj's feedback. I'm still looking forward to reviews on the modules/ code
from mkwest@ and haraken@ and further feedback from anyone else who is
interested in this API.

I am still using DOMRect and DOMPoint for the time being since they are
available for use at this point and if we decide to remove them it will be
trivial to replace them with a semantically similar VRRect/VRPoint. That said I
do feel it's worthwhile to have a standard mechanism for communicating basic
geometric values rather than each module inventing their own. No comment on the
utility of DOMMatrix, as I feel that's a different use case entirely.

[Rick Byers, 2015-02-21 01:57:04]

rbyers@chromium.org changed reviewers:
- rbyers@chromium.org

[Rick Byers, 2015-02-21 01:59:14]

Removing myself as a reviewer, I don't think we need this many reviewers on the
patch and I don't have any particular context (plus I'm swamped with reviews). 
If you need my OWNERS stamp on some directory in particular after someone with
context has given approval then feel free to add me back.

[philipj_slow, 2015-02-23 02:33:11]

https://codereview.chromium.org/848053002/diff/40001/Source/modules/vr/Positi...
File Source/modules/vr/PositionSensorVRDevice.idl (right):

https://codereview.chromium.org/848053002/diff/40001/Source/modules/vr/Positi...
Source/modules/vr/PositionSensorVRDevice.idl:10: void zeroSensor();
On 2015/02/06 01:57:14, philipj_UTC7 wrote:
> zero is an odd verb, and apparently you agree since you called it reset
> internally. Change the spec to resetSensor, clearSensor, or something?

resetSensor() it is.

https://codereview.chromium.org/848053002/diff/60001/Source/modules/vr/VRFiel...
File Source/modules/vr/VRFieldOfView.idl (right):

https://codereview.chromium.org/848053002/diff/60001/Source/modules/vr/VRFiel...
Source/modules/vr/VRFieldOfView.idl:8: Constructor(optional VRFieldOfViewInit
fov),
Now you may need TypeChecking=Interface so that null cannot be passed to the
constructor. Not entirely sure.

https://codereview.chromium.org/848053002/diff/60001/Source/modules/vr/VRFiel...
File Source/modules/vr/VRFieldOfViewInit.idl (right):

https://codereview.chromium.org/848053002/diff/60001/Source/modules/vr/VRFiel...
Source/modules/vr/VRFieldOfViewInit.idl:5: dictionary VRFieldOfViewInit {
Does adding RuntimeEnabled not make any difference to the generated code?

[Ken Russell (switch to Gerrit), 2015-02-23 19:39:32]

This LGTM overall with philipj@'s comments addressed. Note I'm not an owner in
public/.

https://codereview.chromium.org/848053002/diff/60001/Source/modules/vr/VRHard...
File Source/modules/vr/VRHardwareUnit.cpp (right):

https://codereview.chromium.org/848053002/diff/60001/Source/modules/vr/VRHard...
Source/modules/vr/VRHardwareUnit.cpp:49: void
VRHardwareUnit::addDevicesToVector(HeapVector<Member<VRDevice>>& vrDevices)
Please make sure to get a review from someone who works on Oilpan regarding the
HeapVector and Member use here and throughout.

[bajones, 2015-02-23 20:14:10]

bajones@chromium.org changed reviewers:
+ oilpan-reviews@chromium.org

[bajones, 2015-02-23 20:14:11]

+oilpan-reviews. Other feedback will be addressed shortly. Thanks for the
reviews thus far!

[sof, 2015-02-24 12:35:27]

sigbjornf@opera.com changed reviewers:
+ sigbjornf@opera.com

[sof, 2015-02-24 12:35:30]

Looks good from an Oilpan pov already.

https://codereview.chromium.org/848053002/diff/60001/Source/modules/vr/HMDVRD...
File Source/modules/vr/HMDVRDevice.cpp (right):

https://codereview.chromium.org/848053002/diff/60001/Source/modules/vr/HMDVRD...
Source/modules/vr/HMDVRDevice.cpp:24: }
// namespace

https://codereview.chromium.org/848053002/diff/60001/Source/modules/vr/HMDVRD...
Source/modules/vr/HMDVRDevice.cpp:104: // TODO(bajones): This doesn't change
unless you reset the FOV. Cache it.
nit: FIXMEs in Blink.

https://codereview.chromium.org/848053002/diff/60001/Source/modules/vr/HMDVRD...
Source/modules/vr/HMDVRDevice.cpp:142: VRDevice::trace(visitor);
nit: preference is to trace the subclasses last.

https://codereview.chromium.org/848053002/diff/60001/Source/modules/vr/HMDVRD...
File Source/modules/vr/HMDVRDevice.h (right):

https://codereview.chromium.org/848053002/diff/60001/Source/modules/vr/HMDVRD...
Source/modules/vr/HMDVRDevice.h:1: // Copyright 2014 The Chromium Authors. All
rights reserved.
nit: s/2014/2015/g

https://codereview.chromium.org/848053002/diff/60001/Source/modules/vr/HMDVRD...
Source/modules/vr/HMDVRDevice.h:13: #include "wtf/RefCounted.h"
not needed (here and elsewhere in this directory, i think.)

( Only wtf/ include needed here is wtf/Forward.h )

https://codereview.chromium.org/848053002/diff/60001/Source/modules/vr/HMDVRD...
Source/modules/vr/HMDVRDevice.h:31: HMDVRDevice(VRHardwareUnit*);
this is an 'explicit' conversion, right?

https://codereview.chromium.org/848053002/diff/60001/Source/modules/vr/HMDVRD...
Source/modules/vr/HMDVRDevice.h:37: DOMPoint* getEyeTranslation(const String&
whichEye);
nit/suggestion: the use of 'whichEye' in these decls can perhaps be removed
without losing too much?

Also, Blink prefers not to prefix getters with "get" (i'd characterize these as
parameterized getters in terms of what they do) -- eyeTranslation() ?

https://codereview.chromium.org/848053002/diff/60001/Source/modules/vr/HMDVRD...
File Source/modules/vr/HMDVRDevice.idl (right):

https://codereview.chromium.org/848053002/diff/60001/Source/modules/vr/HMDVRD...
Source/modules/vr/HMDVRDevice.idl:14: ] interface HMDVRDevice : VRDevice {
"HMD" - could you spell out that acronym here or in the header file?

https://codereview.chromium.org/848053002/diff/60001/Source/modules/vr/Naviga...
File Source/modules/vr/NavigatorVRDevice.cpp (right):

https://codereview.chromium.org/848053002/diff/60001/Source/modules/vr/Naviga...
Source/modules/vr/NavigatorVRDevice.cpp:55: HeapVector<Member<VRDevice>>
NavigatorVRDevice::updateVRHardwareUnits()
Hmm, naming doesn't suggest that it returns a result - currentVRHardwareUnits(),
getUpdatedVRHardwareDevices(), ..?

https://codereview.chromium.org/848053002/diff/60001/Source/modules/vr/Naviga...
Source/modules/vr/NavigatorVRDevice.cpp:83: for (iter = m_hardwareUnits.begin();
iter != m_hardwareUnits.end(); ++iter) {
Can we use a range-based for loop here?

https://codereview.chromium.org/848053002/diff/60001/Source/modules/vr/Naviga...
File Source/modules/vr/NavigatorVRDevice.h (right):

https://codereview.chromium.org/848053002/diff/60001/Source/modules/vr/Naviga...
Source/modules/vr/NavigatorVRDevice.h:33: explicit NavigatorVRDevice();
nit: no need for explicit.

https://codereview.chromium.org/848053002/diff/60001/Source/modules/vr/Naviga...
Source/modules/vr/NavigatorVRDevice.h:37: HeapVector<Member<VRDevice>>
updateVRHardwareUnits();
How about #including "VRDevice.h" and change this to VRDeviceVector, would that
work?

https://codereview.chromium.org/848053002/diff/60001/Source/modules/vr/Positi...
File Source/modules/vr/PositionSensorVRDevice.h (right):

https://codereview.chromium.org/848053002/diff/60001/Source/modules/vr/Positi...
Source/modules/vr/PositionSensorVRDevice.h:21:
PositionSensorVRDevice(VRHardwareUnit*);
nit: add 'explicit'

https://codereview.chromium.org/848053002/diff/60001/Source/modules/vr/VRDevi...
File Source/modules/vr/VRDevice.h (right):

https://codereview.chromium.org/848053002/diff/60001/Source/modules/vr/VRDevi...
Source/modules/vr/VRDevice.h:32: VRDevice(VRHardwareUnit*);
nit: add explicit.

https://codereview.chromium.org/848053002/diff/60001/Source/modules/vr/VRDevi...
Source/modules/vr/VRDevice.h:40: typedef HeapVector<Member<VRDevice>>
VRDeviceVector;
Styleguide recommends using 'using' for aliases in Chromium & Blink.

https://codereview.chromium.org/848053002/diff/60001/Source/modules/vr/VRDevi...
File Source/modules/vr/VRDevice.idl (right):

https://codereview.chromium.org/848053002/diff/60001/Source/modules/vr/VRDevi...
Source/modules/vr/VRDevice.idl:5: [
Could you add a spec url reference here, if available? (Same for the other .idl
files & their interfaces.)

https://codereview.chromium.org/848053002/diff/60001/Source/modules/vr/VRHard...
File Source/modules/vr/VRHardwareUnit.cpp (right):

https://codereview.chromium.org/848053002/diff/60001/Source/modules/vr/VRHard...
Source/modules/vr/VRHardwareUnit.cpp:33: if (!m_hmd.get())
get() is redundant (here and for other Member<> boolean uses here.)

https://codereview.chromium.org/848053002/diff/60001/Source/modules/vr/VRHard...
Source/modules/vr/VRHardwareUnit.cpp:78: }
Appropriate to have a default: case here?

https://codereview.chromium.org/848053002/diff/60001/Source/modules/vr/VRHard...
File Source/modules/vr/VRHardwareUnit.h (right):

https://codereview.chromium.org/848053002/diff/60001/Source/modules/vr/VRHard...
Source/modules/vr/VRHardwareUnit.h:36: void setFieldOfView(unsigned eyeIndex,
VRFieldOfView*);
Are we bounded two just left&right eyes here (now and later)? If so, an enum
seems preferable.

https://codereview.chromium.org/848053002/diff/60001/Source/modules/vr/VRPosi...
File Source/modules/vr/VRPositionState.cpp (right):

https://codereview.chromium.org/848053002/diff/60001/Source/modules/vr/VRPosi...
Source/modules/vr/VRPositionState.cpp:16: return 0;
nullptr

https://codereview.chromium.org/848053002/diff/60001/Source/modules/vr/VRPosi...
Source/modules/vr/VRPositionState.cpp:22: return 0;
nullptr

https://codereview.chromium.org/848053002/diff/60001/Source/modules/vr/VRPosi...
Source/modules/vr/VRPositionState.cpp:25: }
// namespace

https://codereview.chromium.org/848053002/diff/60001/Source/modules/vr/VRPosi...
File Source/modules/vr/VRPositionState.h (right):

https://codereview.chromium.org/848053002/diff/60001/Source/modules/vr/VRPosi...
Source/modules/vr/VRPositionState.h:17: class VRPositionState final : public
GarbageCollectedFinalized<VRPositionState>, public ScriptWrappable {
You can make this GarbageCollected<> and remove the dtor.

https://codereview.chromium.org/848053002/diff/60001/Source/web/FrameLoaderCl...
File Source/web/FrameLoaderClientImpl.cpp (right):

https://codereview.chromium.org/848053002/diff/60001/Source/web/FrameLoaderCl...
Source/web/FrameLoaderClientImpl.cpp:131: NavigatorVRDevice::from(*document);
Doesn't this need to be conditionally enabled?

https://codereview.chromium.org/848053002/diff/60001/public/platform/Platform.h
File public/platform/Platform.h (right):

https://codereview.chromium.org/848053002/diff/60001/public/platform/Platform...
public/platform/Platform.h:228: virtual void
getVRDevices(std::vector<blink::WebVRDevice>* devices) { }
Shouldn't WebVector<> be used here?

[bajones, 2015-02-24 23:19:24]

On 2015/02/24 at 12:35:30, sigbjornf wrote:
> Looks good from an Oilpan pov already.
> [snip epic amounts of detailed feedback]

Wow, thank you for the very thorough review! Addressed everything you pointed
out, with the exception of adding a spec reference to the IDLs. The spec is a
WIP at https://github.com/MozVR/webvr-spec and at this point there's no live
HTML file to link to. I'll be sure to update these interfaces with said link
once we figure out where it will be hosted.

Pinging mkwest@ and haraken@ again for a modules/ review. Sorry to keep hounding
you, but there's not any alternate owners for me to pester. :(

[sof, 2015-02-25 06:30:34]

https://codereview.chromium.org/848053002/diff/80001/public/platform/Platform.h
File public/platform/Platform.h (right):

https://codereview.chromium.org/848053002/diff/80001/public/platform/Platform...
public/platform/Platform.h:60: #include <vector>
You can remove this now.

https://codereview.chromium.org/848053002/diff/80001/public/platform/WebVR.h
File public/platform/WebVR.h (right):

https://codereview.chromium.org/848053002/diff/80001/public/platform/WebVR.h#...
public/platform/WebVR.h:14: namespace blink {
Could we have some comments, description, something here? i.e., a set of base
types that Blink defines to the outside world should be a bit more carefully
declared than what it looks right now.

[sof, 2015-02-25 07:15:16]

On 2015/02/24 23:19:24, bajones wrote:
> On 2015/02/24 at 12:35:30, sigbjornf wrote:
> > Looks good from an Oilpan pov already.
> > [snip epic amounts of detailed feedback]
> 
> Wow, thank you for the very thorough review! Addressed everything you pointed
> out, with the exception of adding a spec reference to the IDLs. The spec is a
> WIP at https://github.com/MozVR/webvr-spec and at this point there's no live
> HTML file to link to. I'll be sure to update these interfaces with said link
> once we figure out where it will be hosted.
> 

Sounds good (and thanks for all your WebVR efforts)

lgtm

[haraken, 2015-02-25 11:52:56]

Oilpan part LGTM.

https://codereview.chromium.org/848053002/diff/80001/Source/modules/vr/HMDVRD...
File Source/modules/vr/HMDVRDevice.idl (right):

https://codereview.chromium.org/848053002/diff/80001/Source/modules/vr/HMDVRD...
Source/modules/vr/HMDVRDevice.idl:15: GarbageCollected,

You don't need [GarbageCollected], since [GarbageCollected] is inherited.

https://codereview.chromium.org/848053002/diff/80001/Source/modules/vr/VRFiel...
File Source/modules/vr/VRFieldOfView.h (right):

https://codereview.chromium.org/848053002/diff/80001/Source/modules/vr/VRFiel...
Source/modules/vr/VRFieldOfView.h:17: class VRFieldOfView final : public
GarbageCollectedFinalized<VRFieldOfView>, public ScriptWrappable {

"Finalized" wouldn't be needed.

https://codereview.chromium.org/848053002/diff/80001/public/platform/WebVR.h
File public/platform/WebVR.h (right):

https://codereview.chromium.org/848053002/diff/80001/public/platform/WebVR.h#...
public/platform/WebVR.h:1: // Copyright 2014 The Chromium Authors. All rights
reserved.

2015

https://codereview.chromium.org/848053002/diff/80001/public/platform/WebVR.h#...
public/platform/WebVR.h:16: #pragma pack(push, 1)

Does this really matter? (Otherwise, we don't want to use unnecessary pragmas.)

[bajones, 2015-02-27 18:25:03]

haraken@ and sigbjornf@'s feedback has been addressed. Thanks for the reviews!

mkwst@: Although our presubmit will accept haraken@'s approval for modules/ I
get the impression from the OWNERS file that I still need your OK for everything
outside of the IDLs. Correct?

dglazkov: Could I get you to look at public/?

[bajones, 2015-02-27 21:10:10]

https://codereview.chromium.org/848053002/diff/100001/Source/platform/Runtime...
File Source/platform/RuntimeEnabledFeatures.in (right):

https://codereview.chromium.org/848053002/diff/100001/Source/platform/Runtime...
Source/platform/RuntimeEnabledFeatures.in:157: VRDevice
depends_on=GeometryInterfaces, status=stable
Ugh. Apologies for this oversight. These were switched on for local testing
only. I'll remove them in the next update.

[bajones, 2015-03-16 20:34:28]

Patchset #7 (id:120001) has been deleted

[bajones, 2015-03-16 21:30:35]

FYI, WebVR has a public spec now! http://mozvr.github.io/webvr-spec/webvr.html

I've updated this CL to match the spec behavior and taken the opportunity to
rebase and cleanup a bit to.

Still waiting on modules/ and public/ reviews.

[philipj_slow, 2015-03-17 02:27:28]

https://codereview.chromium.org/848053002/diff/140001/Source/modules/vr/OWNERS
File Source/modules/vr/OWNERS (right):

https://codereview.chromium.org/848053002/diff/140001/Source/modules/vr/OWNER...
Source/modules/vr/OWNERS:1: bajones@chromium.org
Maybe you can find a co-owner to review most/all Source/modules/vr/ CLs going
forward?

[Mike West, 2015-03-17 05:25:27]

public/ and modules/ LGTM % a few nits.

Sorry, this fell off my radar completely. Thanks for pinging me about it.

https://codereview.chromium.org/848053002/diff/140001/Source/modules/vr/HMDVR...
File Source/modules/vr/HMDVRDevice.cpp (right):

https://codereview.chromium.org/848053002/diff/140001/Source/modules/vr/HMDVR...
Source/modules/vr/HMDVRDevice.cpp:62: default:
Nit: Here and elsewhere (like VRHardwareUnit::getFieldOfView), why use 'default'
and not 'VREyeNone'? It doesn't seem like we'll be adding eyes anytime soon,
does it? :)

https://codereview.chromium.org/848053002/diff/140001/Source/modules/vr/HMDVR...
File Source/modules/vr/HMDVRDevice.h (right):

https://codereview.chromium.org/848053002/diff/140001/Source/modules/vr/HMDVR...
Source/modules/vr/HMDVRDevice.h:37: DOMRect* getRecommendedEyeRenderRect(const
String&);*/
Nit: Please drop commented-out code from the patch. We typically don't land
that.

https://codereview.chromium.org/848053002/diff/140001/Source/modules/vr/HMDVR...
File Source/modules/vr/HMDVRDevice.idl (right):

https://codereview.chromium.org/848053002/diff/140001/Source/modules/vr/HMDVR...
Source/modules/vr/HMDVRDevice.idl:12: // headset.
Nit: Can you add a link to the spec to this and other IDL files?

https://codereview.chromium.org/848053002/diff/140001/Source/modules/vr/OWNERS
File Source/modules/vr/OWNERS (right):

https://codereview.chromium.org/848053002/diff/140001/Source/modules/vr/OWNER...
Source/modules/vr/OWNERS:1: bajones@chromium.org
On 2015/03/17 at 02:27:27, philipj_UTC7 wrote:
> Maybe you can find a co-owner to review most/all Source/modules/vr/ CLs going
forward?

+1. :)

[bajones, 2015-03-17 20:07:45]

On 2015/03/17 at 05:25:27, mkwst wrote:
> public/ and modules/ LGTM % a few nits.
> 
> Sorry, this fell off my radar completely. Thanks for pinging me about it.

No worries! Thanks for the review!

>
https://codereview.chromium.org/848053002/diff/140001/Source/modules/vr/HMDVR...
> Source/modules/vr/HMDVRDevice.cpp:62: default:
> Nit: Here and elsewhere (like VRHardwareUnit::getFieldOfView), why use
'default' and not 'VREyeNone'? It doesn't seem like we'll be adding eyes anytime
soon, does it? :)

This caused some issues with the static code analysis, where it didn't recognize
the switch statements as covering all possible code paths. Threw errors about
some functions not returning. As such it felt cleaner to leave it as-is.

>
https://codereview.chromium.org/848053002/diff/140001/Source/modules/vr/HMDVR...
> Source/modules/vr/HMDVRDevice.h:37: DOMRect* getRecommendedEyeRenderRect(const
String&);*/
> Nit: Please drop commented-out code from the patch. We typically don't land
that.

Done

>
https://codereview.chromium.org/848053002/diff/140001/Source/modules/vr/HMDVR...
> Source/modules/vr/HMDVRDevice.idl:12: // headset.
> Nit: Can you add a link to the spec to this and other IDL files?

Done

>
https://codereview.chromium.org/848053002/diff/140001/Source/modules/vr/OWNER...
> Source/modules/vr/OWNERS:1: bajones@chromium.org
> On 2015/03/17 at 02:27:27, philipj_UTC7 wrote:
> > Maybe you can find a co-owner to review most/all Source/modules/vr/ CLs
going forward?
> 
> +1. :)

Ken Russel has graciously volunteered to be the directory's co-owner initially,
though I'll be on the lookout for another interested Chrome developer with
working knowledge of current VR implementations.

[bajones, 2015-03-17 20:08:05]

The CQ bit was checked by bajones@chromium.org

[bajones, 2015-03-17 20:08:06]

The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org,
sigbjornf@opera.com, haraken@chromium.org, mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/848053002/#ps160001
(title: "Addressed feedback from mkwst@, added kbr@ as a modules/vr owner")

[commit-bot: I haz the power, 2015-03-17 20:08:29]

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

[bajones, 2015-03-17 21:39:37]

The CQ bit was checked by bajones@chromium.org

[bajones, 2015-03-17 21:39:38]

The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org,
sigbjornf@opera.com, haraken@chromium.org, mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/848053002/#ps180001
(title: "Fixed compiler warning treated as an error on Windows")

[commit-bot: I haz the power, 2015-03-17 21:40:37]

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

[commit-bot: I haz the power, 2015-03-17 23:27:00]

Committed patchset #9 (id:180001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=192045

[mdempsky, 2015-05-11 22:48:39]

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

[mdempsky, 2015-05-11 22:48:40]

https://codereview.chromium.org/848053002/diff/180001/Source/modules/vr/Posit...
File Source/modules/vr/PositionSensorVRDevice.cpp (right):

https://codereview.chromium.org/848053002/diff/180001/Source/modules/vr/Posit...
Source/modules/vr/PositionSensorVRDevice.cpp:30:
blink::Platform::current()->resetVRSensor(index());
Should resetting a VR sensor be global like this?  It seems like if two frames
both have access to the same VR sensor, then one frame calling resetSensor would
reset the sensor as seen by both pages.  Is that intentional?

[bajones, 2015-05-11 23:01:02]

On 2015/05/11 22:48:40, mdempsky wrote:
>
https://codereview.chromium.org/848053002/diff/180001/Source/modules/vr/Posit...
> File Source/modules/vr/PositionSensorVRDevice.cpp (right):
> 
>
https://codereview.chromium.org/848053002/diff/180001/Source/modules/vr/Posit...
> Source/modules/vr/PositionSensorVRDevice.cpp:30:
> blink::Platform::current()->resetVRSensor(index());
> Should resetting a VR sensor be global like this?  It seems like if two frames
> both have access to the same VR sensor, then one frame calling resetSensor
would
> reset the sensor as seen by both pages.  Is that intentional?

Maybe not "intentional", but at least a known issue. Most of the time when using
these APIs the user will likely be fullscreen (I've got patches waiting in the
wings for VR/fullscreen integration), and thus implicitly only using it on one
page at a time. Even if the user wasn't using the API like that I'm not sure how
critical that separation will be, though. If needed I could handle the reset at
the Blink level by multiplying the HMD-provided orientation against an inverse
"forward" quaternion. I'll bring this up with the other spec author and see what
he thinks.

[mdempsky, 2015-05-11 23:14:44]

On 2015/05/11 23:01:02, bajones wrote:
> Maybe not "intentional", but at least a known issue. Most of the time when
using
> these APIs the user will likely be fullscreen (I've got patches waiting in the
> wings for VR/fullscreen integration), and thus implicitly only using it on one
> page at a time. Even if the user wasn't using the API like that I'm not sure
how
> critical that separation will be, though.

I guess my concern is just a page in the background continuously resetting the
sensor state to disrupt other pages.

Of course, the easy fix is to close annoying pages that do that (like if a
background page is playing unwanted audio or uselessly consuming CPU), so it's
certainly not an issue unique to WebVR.

> If needed I could handle the reset at
> the Blink level by multiplying the HMD-provided orientation against an inverse
> "forward" quaternion.

Yeah, that's what I was thinking too as a possible fix.

> I'll bring this up with the other spec author and see what
> he thinks.

SGTM

[esprehn, 2015-06-22 22:05:27]

https://codereview.chromium.org/848053002/diff/180001/Source/modules/vr/HMDVR...
File Source/modules/vr/HMDVRDevice.h (right):

https://codereview.chromium.org/848053002/diff/180001/Source/modules/vr/HMDVR...
Source/modules/vr/HMDVRDevice.h:26: typedef Vector<double> DoubleVector;
unused?

https://codereview.chromium.org/848053002/diff/180001/Source/modules/vr/HMDVR...
Source/modules/vr/HMDVRDevice.h:36: static VREye StringToVREye(const String&);
lowerCaseNames.

https://codereview.chromium.org/848053002/diff/180001/Source/modules/vr/VRPos...
File Source/modules/vr/VRPositionState.cpp (right):

https://codereview.chromium.org/848053002/diff/180001/Source/modules/vr/VRPos...
Source/modules/vr/VRPositionState.cpp:12: DOMPoint* VecToDomPoint(const
blink::WebVRVector4& vec, bool valid)
lowerCaseNames.

https://codereview.chromium.org/848053002/diff/180001/Source/modules/vr/VRPos...
Source/modules/vr/VRPositionState.cpp:18: DOMPoint* VecToDomPoint(const
blink::WebVRVector3& vec, bool valid)
ditto

[bajones, 2015-06-23 20:39:08]

Thanks for pointing out the casing inconsistencies! Patch up here:
https://codereview.chromium.org/1207513003/ 

On 2015/06/22 22:05:27, esprehn wrote:
> Source/modules/vr/HMDVRDevice.h:26: typedef Vector<double> DoubleVector;
> unused?

Already removed by a later patch.