Fixes 2D-to-WebVR site transitions when browsing in VR.

Fixes 2D-to-WebVR site transitions when browsing in VR.

- Renames GetVRDevice(...) to CreateVRDisplayInfo(...) for better understandability.
- Refactores the addition for services in VRDeviceManager to simplify the code.
- Makes the client of VRDisplayImpl and VRServiceImpl private. Added delegate methods where calls to the client are necessary.
- Makes sure that VRDisplayImpl's client is initialized in the constructor. A call to an uninitialized client was the actual source of the bug.
- Moves the instantiation of VRDisplayImpl objects to VRServiceImpl since VRServiceImpl is the owner of the VRDisplayImpls.
- Adds class comments to the touched classes.
- Adaptes the unit tests accordingly.

BUG=703318
Review-Url: https://codereview.chromium.org/2746233002
Cr-Commit-Position: refs/heads/master@{#458483}
Committed: https://chromium.googlesource.com/chromium/src/+/e3db9aa8aa3160f96599766955bf286a64a072e2

[tiborg, 2017-03-13 20:10:46]

Description was changed from

==========
Fixes 2D-to-WebVR site transitions when browsing in VR.

- Renames GetVRDevice(...) to CreateVRDisplayInfo(...) for better
understandability.
- Refactores the addition for services in VRDeviceManager to simplify the code.
- Makes the client of VRDisplayImpl and VRServiceImpl private. Added delegate
methods where calls to the client are necessary.
- Makes sure that VRDisplayImpl's client is initialized in the constructor. A
call to an uninitialized client was the actual source of the bug.
- Moves the instantiation of VRDisplayImpl objects to VRServiceImpl since
VRServiceImpl is the owner of the VRDisplayImpls.
- Adds class comments to the touched classes.
- Adaptes the unit tests accordingly.

BUG=689139
==========

to

==========
Fixes 2D-to-WebVR site transitions when browsing in VR.

- Renames GetVRDevice(...) to CreateVRDisplayInfo(...) for better
understandability.
- Refactores the addition for services in VRDeviceManager to simplify the code.
- Makes the client of VRDisplayImpl and VRServiceImpl private. Added delegate
methods where calls to the client are necessary.
- Makes sure that VRDisplayImpl's client is initialized in the constructor. A
call to an uninitialized client was the actual source of the bug.
- Moves the instantiation of VRDisplayImpl objects to VRServiceImpl since
VRServiceImpl is the owner of the VRDisplayImpls.
- Adds class comments to the touched classes.
- Adaptes the unit tests accordingly.

BUG=689139
==========

[tiborg, 2017-03-13 20:10:46]

tiborg@chromium.org changed reviewers:
+ mthiesse@chromium.org

[tiborg, 2017-03-13 20:15:24]

tiborg@chromium.org changed reviewers:
+ bajones@chromium.org, bshe@chromium.org

[mthiesse, 2017-03-13 21:51:39]

(FYI you have to send out a message for us to get emails about CLs)

[tiborg, 2017-03-13 21:56:49]

Please take a look

[mthiesse, 2017-03-13 22:17:30]

lgtm, left a bunch of nits.

Should probably wait for Brandon to review too.

https://codereview.chromium.org/2746233002/diff/1/device/vr/vr_device.h
File device/vr/vr_device.h (right):

https://codereview.chromium.org/2746233002/diff/1/device/vr/vr_device.h#newco...
device/vr/vr_device.h:19: // Represents a VR device in the system. Owned by the
respective
What does "a VR device in the system" mean?

https://codereview.chromium.org/2746233002/diff/1/device/vr/vr_device.h#newco...
device/vr/vr_device.h:28: // Queries VR device for display info and calls
onCreated once the  display
nit: s/  display/ display/

https://codereview.chromium.org/2746233002/diff/1/device/vr/vr_device_manager.h
File device/vr/vr_device_manager.h (right):

https://codereview.chromium.org/2746233002/diff/1/device/vr/vr_device_manager...
device/vr/vr_device_manager.h:27: // Singleton used to provide the system's VR
devices to VRServiceImpl instances.
nit: s/system/platform/?

https://codereview.chromium.org/2746233002/diff/1/device/vr/vr_display_impl_u...
File device/vr/vr_display_impl_unittest.cc (right):

https://codereview.chromium.org/2746233002/diff/1/device/vr/vr_display_impl_u...
device/vr/vr_display_impl_unittest.cc:63: VRDisplayImpl*
GetVRDisplayImpl(VRServiceImpl* service, VRDevice* device) {
Why add this function?

https://codereview.chromium.org/2746233002/diff/1/device/vr/vr_service.mojom
File device/vr/vr_service.mojom (right):

https://codereview.chromium.org/2746233002/diff/1/device/vr/vr_service.mojom#...
device/vr/vr_service.mojom:75: // TODO(crbug/701027): Use a factory function
which took a
Keep TODO(shaobo.yan@intel.com) and add the crbug link to the comment (or use
"TODO(shaobo.yan@intel.com,crbug/701027)"

https://codereview.chromium.org/2746233002/diff/1/device/vr/vr_service_impl.h
File device/vr/vr_service_impl.h (right):

https://codereview.chromium.org/2746233002/diff/1/device/vr/vr_service_impl.h...
device/vr/vr_service_impl.h:21: // Mojo once the user navigates to a web site
containing WebVR.
nit: s/navigates to a web site/loads a page/

[amp, 2017-03-13 22:26:04]

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

[amp, 2017-03-13 22:26:06]

Some high level thoughts and nits on comments.

https://codereview.chromium.org/2746233002/diff/1/device/vr/test/fake_vr_devi...
File device/vr/test/fake_vr_device.cc (right):

https://codereview.chromium.org/2746233002/diff/1/device/vr/test/fake_vr_devi...
device/vr/test/fake_vr_device.cc:53: void FakeVRDevice::SetVRDevice(const
mojom::VRDisplayInfoPtr& device) {
Any reason not to rename the param here to display_info to be consistent?

https://codereview.chromium.org/2746233002/diff/1/device/vr/vr_device.h
File device/vr/vr_device.h (right):

https://codereview.chromium.org/2746233002/diff/1/device/vr/vr_device.h#newco...
device/vr/vr_device.h:29: // info object is created. May or may not call
onCreated before this
The last sentence here makes it sound like the creation can fail sometime and
then won't callback (and doesn't explain those scenarios).  Is this instead
saying that the callback is asynchronous?

https://codereview.chromium.org/2746233002/diff/1/device/vr/vr_device_manager.h
File device/vr/vr_device_manager.h (right):

https://codereview.chromium.org/2746233002/diff/1/device/vr/vr_device_manager...
device/vr/vr_device_manager.h:36: // this object. Automatically connects all
currently available VR devices to
That's quite an additional comment here.  Can we add some clarification on how
the automation of adding available devices work?

https://codereview.chromium.org/2746233002/diff/1/device/vr/vr_service_impl.cc
File device/vr/vr_service_impl.cc (right):

https://codereview.chromium.org/2746233002/diff/1/device/vr/vr_service_impl.c...
device/vr/vr_service_impl.cc:46: void VRServiceImpl::ConnectDevice(VRDevice*
device) {
There still seems to be a mismatch between displays and devices.  When do we use
one term vs the other?

https://codereview.chromium.org/2746233002/diff/1/device/vr/vr_service_impl.h
File device/vr/vr_service_impl.h (right):

https://codereview.chromium.org/2746233002/diff/1/device/vr/vr_service_impl.h...
device/vr/vr_service_impl.h:25: class VRServiceImpl : public mojom::VRService {
Not required for this change, but VRServiceImpl seems so generic as to be
useless (in fact rather confusing given the explanation comment above) and I
recommend we try to think of a better alternative (if that's not already being
done).

VRSession would make more sense to me.

[tiborg, 2017-03-13 23:00:58]

https://codereview.chromium.org/2746233002/diff/1/device/vr/test/fake_vr_devi...
File device/vr/test/fake_vr_device.cc (right):

https://codereview.chromium.org/2746233002/diff/1/device/vr/test/fake_vr_devi...
device/vr/test/fake_vr_device.cc:53: void FakeVRDevice::SetVRDevice(const
mojom::VRDisplayInfoPtr& device) {
On 2017/03/13 22:26:05, amp wrote:
> Any reason not to rename the param here to display_info to be consistent?

Good call! Changed it.

https://codereview.chromium.org/2746233002/diff/1/device/vr/vr_device.h
File device/vr/vr_device.h (right):

https://codereview.chromium.org/2746233002/diff/1/device/vr/vr_device.h#newco...
device/vr/vr_device.h:19: // Represents a VR device in the system. Owned by the
respective
On 2017/03/13 22:17:29, mthiesse wrote:
> What does "a VR device in the system" mean?

I mean one of all the available VR devices. Changed it to platform.

https://codereview.chromium.org/2746233002/diff/1/device/vr/vr_device.h#newco...
device/vr/vr_device.h:28: // Queries VR device for display info and calls
onCreated once the  display
On 2017/03/13 22:17:29, mthiesse wrote:
> nit: s/  display/ display/

Done.

https://codereview.chromium.org/2746233002/diff/1/device/vr/vr_device.h#newco...
device/vr/vr_device.h:29: // info object is created. May or may not call
onCreated before this
On 2017/03/13 22:26:05, amp wrote:
> The last sentence here makes it sound like the creation can fail sometime and
> then won't callback (and doesn't explain those scenarios).  Is this instead
> saying that the callback is asynchronous?

I wanted to say that the callback can be called on top of the stack of this
function even though you kinda assume the callback would be called sometimes
later (that's why the DisplayInfoPtr is not the return type). Changed it a bit.
Does this make sense?

https://codereview.chromium.org/2746233002/diff/1/device/vr/vr_device_manager.h
File device/vr/vr_device_manager.h (right):

https://codereview.chromium.org/2746233002/diff/1/device/vr/vr_device_manager...
device/vr/vr_device_manager.h:27: // Singleton used to provide the system's VR
devices to VRServiceImpl instances.
On 2017/03/13 22:17:29, mthiesse wrote:
> nit: s/system/platform/?

Done.

https://codereview.chromium.org/2746233002/diff/1/device/vr/vr_device_manager...
device/vr/vr_device_manager.h:36: // this object. Automatically connects all
currently available VR devices to
On 2017/03/13 22:26:05, amp wrote:
> That's quite an additional comment here.  Can we add some clarification on how
> the automation of adding available devices work?

Elaborated a bit. Is this more understandable?

https://codereview.chromium.org/2746233002/diff/1/device/vr/vr_display_impl_u...
File device/vr/vr_display_impl_unittest.cc (right):

https://codereview.chromium.org/2746233002/diff/1/device/vr/vr_display_impl_u...
device/vr/vr_display_impl_unittest.cc:63: VRDisplayImpl*
GetVRDisplayImpl(VRServiceImpl* service, VRDevice* device) {
On 2017/03/13 22:17:29, mthiesse wrote:
> Why add this function?

In C++ friend declarations are not inheritable. TEST_F makes a subclass of
VRDisplayImplTest and overrides the TestBody() function with the test content.
In the overriding TestBody() I cannot call private functions of VRDisplayImpl
anymore. This function works around this limitation.

https://codereview.chromium.org/2746233002/diff/1/device/vr/vr_service.mojom
File device/vr/vr_service.mojom (right):

https://codereview.chromium.org/2746233002/diff/1/device/vr/vr_service.mojom#...
device/vr/vr_service.mojom:75: // TODO(crbug/701027): Use a factory function
which took a
On 2017/03/13 22:17:29, mthiesse wrote:
> Keep mailto:TODO(shaobo.yan@intel.com) and add the crbug link to the comment
(or use
> mailto:"TODO(shaobo.yan@intel.com,crbug/701027)"

Done.

https://codereview.chromium.org/2746233002/diff/1/device/vr/vr_service_impl.cc
File device/vr/vr_service_impl.cc (right):

https://codereview.chromium.org/2746233002/diff/1/device/vr/vr_service_impl.c...
device/vr/vr_service_impl.cc:46: void VRServiceImpl::ConnectDevice(VRDevice*
device) {
On 2017/03/13 22:26:05, amp wrote:
> There still seems to be a mismatch between displays and devices.  When do we
use
> one term vs the other?

I agree, it's a bit confusing. Currently a display is a representation of a
device in the current WebVR session. So, if there are multiple sessions, there
will be a display in each session for each device.

https://codereview.chromium.org/2746233002/diff/1/device/vr/vr_service_impl.h
File device/vr/vr_service_impl.h (right):

https://codereview.chromium.org/2746233002/diff/1/device/vr/vr_service_impl.h...
device/vr/vr_service_impl.h:21: // Mojo once the user navigates to a web site
containing WebVR.
On 2017/03/13 22:17:29, mthiesse wrote:
> nit: s/navigates to a web site/loads a page/

Done.

https://codereview.chromium.org/2746233002/diff/1/device/vr/vr_service_impl.h...
device/vr/vr_service_impl.h:25: class VRServiceImpl : public mojom::VRService {
On 2017/03/13 22:26:05, amp wrote:
> Not required for this change, but VRServiceImpl seems so generic as to be
> useless (in fact rather confusing given the explanation comment above) and I
> recommend we try to think of a better alternative (if that's not already being
> done).
> 
> VRSession would make more sense to me.

Yep, agreed. VRSession sounds good to me. Should be a follow-up CL. There will
be quite some renaming if done properly because you would also have to change
the Mojo interfaces etc.

[bajones, 2017-03-13 23:04:58]

Broadly looking good, though I agree with a lot of the previous nits. One thing
to point out, though: There's a fair amount of (good) refactoring here, and it
made it difficult for me to reason about which parts of the patch were actually
fixing the 2D->VR transition. Any chance we could split this into a cleanup
patch and the feature fix?

https://codereview.chromium.org/2746233002/diff/1/device/vr/vr_service_impl.cc
File device/vr/vr_service_impl.cc (right):

https://codereview.chromium.org/2746233002/diff/1/device/vr/vr_service_impl.c...
device/vr/vr_service_impl.cc:46: void VRServiceImpl::ConnectDevice(VRDevice*
device) {
Oh, and just to make things MORE confusing we're gonna be changing some names in
the upcoming spec version: https://github.com/w3c/webvr/issues/192 (The primary
troublemaker there being Display->Device.)

I'm sorry. Really just so very sorry. :(

https://codereview.chromium.org/2746233002/diff/1/device/vr/vr_service_impl.h
File device/vr/vr_service_impl.h (right):

https://codereview.chromium.org/2746233002/diff/1/device/vr/vr_service_impl.h...
device/vr/vr_service_impl.h:25: class VRServiceImpl : public mojom::VRService {
On 2017/03/13 22:26:05, amp wrote:
> VRSession would make more sense to me.

May want to be careful with that. VRSession will have a specific meaning in the
upcoming spec refactor (an interface created from the VRDevice which enables
presentation and pose polling). Doesn't seem like this class maps to that
concept very well.

[amp, 2017-03-14 17:05:51]

lgtm

https://codereview.chromium.org/2746233002/diff/1/device/vr/vr_device.h
File device/vr/vr_device.h (right):

https://codereview.chromium.org/2746233002/diff/1/device/vr/vr_device.h#newco...
device/vr/vr_device.h:29: // info object is created. May or may not call
onCreated before this
On 2017/03/13 23:00:58, tiborg wrote:
> On 2017/03/13 22:26:05, amp wrote:
> > The last sentence here makes it sound like the creation can fail sometime
and
> > then won't callback (and doesn't explain those scenarios).  Is this instead
> > saying that the callback is asynchronous?
> 
> I wanted to say that the callback can be called on top of the stack of this
> function even though you kinda assume the callback would be called sometimes
> later (that's why the DisplayInfoPtr is not the return type). Changed it a
bit.
> Does this make sense?

Yes that clarifies the scenarios I was wondering about, thanks.

https://codereview.chromium.org/2746233002/diff/1/device/vr/vr_device_manager.h
File device/vr/vr_device_manager.h (right):

https://codereview.chromium.org/2746233002/diff/1/device/vr/vr_device_manager...
device/vr/vr_device_manager.h:36: // this object. Automatically connects all
currently available VR devices to
On 2017/03/13 23:00:58, tiborg wrote:
> On 2017/03/13 22:26:05, amp wrote:
> > That's quite an additional comment here.  Can we add some clarification on
how
> > the automation of adding available devices work?
> 
> Elaborated a bit. Is this more understandable?

Yes, it doesn't sound like magic now. :)

https://codereview.chromium.org/2746233002/diff/1/device/vr/vr_service_impl.cc
File device/vr/vr_service_impl.cc (right):

https://codereview.chromium.org/2746233002/diff/1/device/vr/vr_service_impl.c...
device/vr/vr_service_impl.cc:46: void VRServiceImpl::ConnectDevice(VRDevice*
device) {
On 2017/03/13 23:04:57, bajones wrote:
> Oh, and just to make things MORE confusing we're gonna be changing some names
in
> the upcoming spec version: https://github.com/w3c/webvr/issues/192 (The
primary
> troublemaker there being Display->Device.)
> 
> I'm sorry. Really just so very sorry. :(

Yea let's handle cleaning this up in a future change, I think I'm starting to
understand how they all work together, but it should be a lot easier for anyone
who looks at the code for the first time to be able to figure out what is going
on.

https://codereview.chromium.org/2746233002/diff/1/device/vr/vr_service_impl.h
File device/vr/vr_service_impl.h (right):

https://codereview.chromium.org/2746233002/diff/1/device/vr/vr_service_impl.h...
device/vr/vr_service_impl.h:25: class VRServiceImpl : public mojom::VRService {
On 2017/03/13 23:04:57, bajones wrote:
> On 2017/03/13 22:26:05, amp wrote:
> > VRSession would make more sense to me.
> 
> May want to be careful with that. VRSession will have a specific meaning in
the
> upcoming spec refactor (an interface created from the VRDevice which enables
> presentation and pose polling). Doesn't seem like this class maps to that
> concept very well.

Thanks for the clarification Brandon.  I thought I remembered something about VR
Session from the spec (which is why I had it in mind probably), but I agree we
should line up the naming and functionality correctly with the spec (so use
something other than VR session here if it doesn't line up).  Either way we
should handle this later and after some more discussion it sounds like.

https://codereview.chromium.org/2746233002/diff/20001/device/vr/vr_service.mojom
File device/vr/vr_service.mojom (right):

https://codereview.chromium.org/2746233002/diff/20001/device/vr/vr_service.mo...
device/vr/vr_service.mojom:75: // TODO(shaobo.yan@intel.com, crbug/701027): Use
a factory function which took
nit since you are already changing this line: s/took/takes/ and s/would/will/ on
the next line.

[tiborg, 2017-03-14 17:45:39]

On 2017/03/13 23:04:58, bajones wrote:
> Broadly looking good, though I agree with a lot of the previous nits. One
thing
> to point out, though: There's a fair amount of (good) refactoring here, and it
> made it difficult for me to reason about which parts of the patch were
actually
> fixing the 2D->VR transition. Any chance we could split this into a cleanup
> patch and the feature fix?
> 
> https://codereview.chromium.org/2746233002/diff/1/device/vr/vr_service_impl.cc
> File device/vr/vr_service_impl.cc (right):
> 
>
https://codereview.chromium.org/2746233002/diff/1/device/vr/vr_service_impl.c...
> device/vr/vr_service_impl.cc:46: void VRServiceImpl::ConnectDevice(VRDevice*
> device) {
> Oh, and just to make things MORE confusing we're gonna be changing some names
in
> the upcoming spec version: https://github.com/w3c/webvr/issues/192 (The
primary
> troublemaker there being Display->Device.)
> 
> I'm sorry. Really just so very sorry. :(
> 
> https://codereview.chromium.org/2746233002/diff/1/device/vr/vr_service_impl.h
> File device/vr/vr_service_impl.h (right):
> 
>
https://codereview.chromium.org/2746233002/diff/1/device/vr/vr_service_impl.h...
> device/vr/vr_service_impl.h:25: class VRServiceImpl : public mojom::VRService
{
> On 2017/03/13 22:26:05, amp wrote:
> > VRSession would make more sense to me.
> 
> May want to be careful with that. VRSession will have a specific meaning in
the
> upcoming spec refactor (an interface created from the VRDevice which enables
> presentation and pose polling). Doesn't seem like this class maps to that
> concept very well.

Discussed offline. Will not split this CL but will do for future CLs.

[tiborg, 2017-03-14 17:45:41]

On 2017/03/13 23:04:58, bajones wrote:
> Broadly looking good, though I agree with a lot of the previous nits. One
thing
> to point out, though: There's a fair amount of (good) refactoring here, and it
> made it difficult for me to reason about which parts of the patch were
actually
> fixing the 2D->VR transition. Any chance we could split this into a cleanup
> patch and the feature fix?
> 
> https://codereview.chromium.org/2746233002/diff/1/device/vr/vr_service_impl.cc
> File device/vr/vr_service_impl.cc (right):
> 
>
https://codereview.chromium.org/2746233002/diff/1/device/vr/vr_service_impl.c...
> device/vr/vr_service_impl.cc:46: void VRServiceImpl::ConnectDevice(VRDevice*
> device) {
> Oh, and just to make things MORE confusing we're gonna be changing some names
in
> the upcoming spec version: https://github.com/w3c/webvr/issues/192 (The
primary
> troublemaker there being Display->Device.)
> 
> I'm sorry. Really just so very sorry. :(
> 
> https://codereview.chromium.org/2746233002/diff/1/device/vr/vr_service_impl.h
> File device/vr/vr_service_impl.h (right):
> 
>
https://codereview.chromium.org/2746233002/diff/1/device/vr/vr_service_impl.h...
> device/vr/vr_service_impl.h:25: class VRServiceImpl : public mojom::VRService
{
> On 2017/03/13 22:26:05, amp wrote:
> > VRSession would make more sense to me.
> 
> May want to be careful with that. VRSession will have a specific meaning in
the
> upcoming spec refactor (an interface created from the VRDevice which enables
> presentation and pose polling). Doesn't seem like this class maps to that
> concept very well.

Discussed offline. Will not split this CL but will do for future CLs.

[tiborg, 2017-03-14 17:46:00]

https://codereview.chromium.org/2746233002/diff/20001/device/vr/vr_service.mojom
File device/vr/vr_service.mojom (right):

https://codereview.chromium.org/2746233002/diff/20001/device/vr/vr_service.mo...
device/vr/vr_service.mojom:75: // TODO(shaobo.yan@intel.com, crbug/701027): Use
a factory function which took
On 2017/03/14 17:05:51, amp wrote:
> nit since you are already changing this line: s/took/takes/ and s/would/will/
on
> the next line.

Done.

[bajones, 2017-03-14 17:49:18]

device/vr lgtm

[tiborg, 2017-03-14 19:25:47]

The CQ bit was checked by tiborg@chromium.org

[tiborg, 2017-03-14 19:25:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
mthiesse@chromium.org, amp@chromium.org
Link to the patchset: https://codereview.chromium.org/2746233002/#ps40001
(title: "nit")

[commit-bot: I haz the power, 2017-03-14 19:26:32]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-14 19:38:35]

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

[commit-bot: I haz the power, 2017-03-14 19:38:37]

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

[tiborg, 2017-03-14 19:45:53]

tiborg@chromium.org changed reviewers:
+ dcheng@chromium.org

[tiborg, 2017-03-14 19:45:54]

dcheng@: Please review changes in //src/device/vr/vr_service.mojom. Thanks!

[dcheng, 2017-03-16 05:38:22]

https://codereview.chromium.org/2746233002/diff/40001/device/vr/android/gvr/g...
File device/vr/android/gvr/gvr_device.cc (right):

https://codereview.chromium.org/2746233002/diff/40001/device/vr/android/gvr/g...
device/vr/android/gvr/gvr_device.cc:33:
onCreated.Run(mojom::VRDisplayInfoPtr(nullptr));
Unrelated, but this is typically written:

on_created.Run(nullptr);  // nullptr_t overload is purposely implicit

or

on_created.Run(mojom::VRDisplayInfoPtr());

https://codereview.chromium.org/2746233002/diff/40001/device/vr/android/gvr/g...
File device/vr/android/gvr/gvr_device.h (right):

https://codereview.chromium.org/2746233002/diff/40001/device/vr/android/gvr/g...
device/vr/android/gvr/gvr_device.h:23: const
base::Callback<void(mojom::VRDisplayInfoPtr)>& onCreated) override;
Nit: on_created, to follow Chromium naming conventions

https://codereview.chromium.org/2746233002/diff/40001/device/vr/vr_service_im...
File device/vr/vr_service_impl.cc (right):

https://codereview.chromium.org/2746233002/diff/40001/device/vr/vr_service_im...
device/vr/vr_service_impl.cc:50: DCHECK(client_);
This can't be a DCHECK: if the renderer process intentionally omits the call to
SetClient(), this will crash later when we try to make a call through an unbound
InterfacePtr in VRDisplayImpl's constructor.

https://codereview.chromium.org/2746233002/diff/40001/device/vr/vr_service_im...
File device/vr/vr_service_impl.h (right):

https://codereview.chromium.org/2746233002/diff/40001/device/vr/vr_service_im...
device/vr/vr_service_impl.h:62: VRDisplayImpl* GetVRDisplayImpl(VRDevice*
device);
There's a weak convention of suffixing this with 'ForTest' or 'ForTesting',
which PRESUBMIT.py will attempt to check is only used in test files.

[tiborg, 2017-03-16 15:02:40]

https://codereview.chromium.org/2746233002/diff/40001/device/vr/android/gvr/g...
File device/vr/android/gvr/gvr_device.cc (right):

https://codereview.chromium.org/2746233002/diff/40001/device/vr/android/gvr/g...
device/vr/android/gvr/gvr_device.cc:33:
onCreated.Run(mojom::VRDisplayInfoPtr(nullptr));
On 2017/03/16 05:38:22, dcheng wrote:
> Unrelated, but this is typically written:
> 
> on_created.Run(nullptr);  // nullptr_t overload is purposely implicit
> 
> or
> 
> on_created.Run(mojom::VRDisplayInfoPtr());

Done.

https://codereview.chromium.org/2746233002/diff/40001/device/vr/android/gvr/g...
File device/vr/android/gvr/gvr_device.h (right):

https://codereview.chromium.org/2746233002/diff/40001/device/vr/android/gvr/g...
device/vr/android/gvr/gvr_device.h:23: const
base::Callback<void(mojom::VRDisplayInfoPtr)>& onCreated) override;
On 2017/03/16 05:38:22, dcheng wrote:
> Nit: on_created, to follow Chromium naming conventions

Done.

https://codereview.chromium.org/2746233002/diff/40001/device/vr/vr_service_im...
File device/vr/vr_service_impl.cc (right):

https://codereview.chromium.org/2746233002/diff/40001/device/vr/vr_service_im...
device/vr/vr_service_impl.cc:50: DCHECK(client_);
On 2017/03/16 05:38:22, dcheng wrote:
> This can't be a DCHECK: if the renderer process intentionally omits the call
to
> SetClient(), this will crash later when we try to make a call through an
unbound
> InterfacePtr in VRDisplayImpl's constructor.

Ok, changed it so that it does not crash for now. In the future we should change
it so that the client_ is initialized in the constructor and, thus, never null.

https://codereview.chromium.org/2746233002/diff/40001/device/vr/vr_service_im...
File device/vr/vr_service_impl.h (right):

https://codereview.chromium.org/2746233002/diff/40001/device/vr/vr_service_im...
device/vr/vr_service_impl.h:62: VRDisplayImpl* GetVRDisplayImpl(VRDevice*
device);
On 2017/03/16 05:38:22, dcheng wrote:
> There's a weak convention of suffixing this with 'ForTest' or 'ForTesting',
> which PRESUBMIT.py will attempt to check is only used in test files.

Oh, perfect! Done.

[dcheng, 2017-03-16 21:39:12]

mojo lgtm

https://codereview.chromium.org/2746233002/diff/60001/device/vr/vr_service_im...
File device/vr/vr_service_impl.cc (right):

https://codereview.chromium.org/2746233002/diff/60001/device/vr/vr_service_im...
device/vr/vr_service_impl.cc:68: LOG(ERROR) << "Cannot create VR display because
connection to render "
Nit: DLOG() please.

[tiborg, 2017-03-16 23:15:10]

The CQ bit was checked by tiborg@chromium.org

[tiborg, 2017-03-16 23:15:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
mthiesse@chromium.org, bajones@chromium.org, amp@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2746233002/#ps80001
(title: "nit")

[commit-bot: I haz the power, 2017-03-16 23:16:07]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-16 23:38:50]

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

[commit-bot: I haz the power, 2017-03-16 23:38:51]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tiborg, 2017-03-17 00:20:50]

The CQ bit was checked by tiborg@chromium.org

[tiborg, 2017-03-17 00:20:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
mthiesse@chromium.org, bajones@chromium.org, dcheng@chromium.org,
amp@chromium.org
Link to the patchset: https://codereview.chromium.org/2746233002/#ps100001
(title: "Fixed tests")

[commit-bot: I haz the power, 2017-03-17 00:21:44]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-17 01:00:02]

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

[commit-bot: I haz the power, 2017-03-17 01:00:03]

Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[tiborg, 2017-03-17 14:49:29]

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

[commit-bot: I haz the power, 2017-03-17 14:49:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tiborg, 2017-03-17 15:05:23]

The CQ bit was unchecked by tiborg@chromium.org

[tiborg, 2017-03-17 15:07:04]

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

[commit-bot: I haz the power, 2017-03-17 15:07:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-17 16:25:23]

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

[commit-bot: I haz the power, 2017-03-17 16:25:24]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tiborg, 2017-03-20 21:22:27]

Description was changed from

==========
Fixes 2D-to-WebVR site transitions when browsing in VR.

- Renames GetVRDevice(...) to CreateVRDisplayInfo(...) for better
understandability.
- Refactores the addition for services in VRDeviceManager to simplify the code.
- Makes the client of VRDisplayImpl and VRServiceImpl private. Added delegate
methods where calls to the client are necessary.
- Makes sure that VRDisplayImpl's client is initialized in the constructor. A
call to an uninitialized client was the actual source of the bug.
- Moves the instantiation of VRDisplayImpl objects to VRServiceImpl since
VRServiceImpl is the owner of the VRDisplayImpls.
- Adds class comments to the touched classes.
- Adaptes the unit tests accordingly.

BUG=689139
==========

to

==========
Fixes 2D-to-WebVR site transitions when browsing in VR.

- Renames GetVRDevice(...) to CreateVRDisplayInfo(...) for better
understandability.
- Refactores the addition for services in VRDeviceManager to simplify the code.
- Makes the client of VRDisplayImpl and VRServiceImpl private. Added delegate
methods where calls to the client are necessary.
- Makes sure that VRDisplayImpl's client is initialized in the constructor. A
call to an uninitialized client was the actual source of the bug.
- Moves the instantiation of VRDisplayImpl objects to VRServiceImpl since
VRServiceImpl is the owner of the VRDisplayImpls.
- Adds class comments to the touched classes.
- Adaptes the unit tests accordingly.

BUG=703318
==========

[tiborg, 2017-03-20 22:51:48]

The CQ bit was checked by tiborg@chromium.org

[tiborg, 2017-03-20 22:51:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
mthiesse@chromium.org, bajones@chromium.org, dcheng@chromium.org,
amp@chromium.org
Link to the patchset: https://codereview.chromium.org/2746233002/#ps120001
(title: "Fixed export")

[commit-bot: I haz the power, 2017-03-20 22:53:05]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-21 00:26:12]

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

[commit-bot: I haz the power, 2017-03-21 00:26:13]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[tiborg, 2017-03-21 17:22:29]

The CQ bit was checked by tiborg@chromium.org

[commit-bot: I haz the power, 2017-03-21 17:23:16]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-21 18:31:02]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1490116949399280,
"parent_rev": "728296503744502f3460a11022f0e4281477f3c5", "commit_rev":
"e3db9aa8aa3160f96599766955bf286a64a072e2"}

[commit-bot: I haz the power, 2017-03-21 18:31:43]

Description was changed from

==========
Fixes 2D-to-WebVR site transitions when browsing in VR.

- Renames GetVRDevice(...) to CreateVRDisplayInfo(...) for better
understandability.
- Refactores the addition for services in VRDeviceManager to simplify the code.
- Makes the client of VRDisplayImpl and VRServiceImpl private. Added delegate
methods where calls to the client are necessary.
- Makes sure that VRDisplayImpl's client is initialized in the constructor. A
call to an uninitialized client was the actual source of the bug.
- Moves the instantiation of VRDisplayImpl objects to VRServiceImpl since
VRServiceImpl is the owner of the VRDisplayImpls.
- Adds class comments to the touched classes.
- Adaptes the unit tests accordingly.

BUG=703318
==========

to

==========
Fixes 2D-to-WebVR site transitions when browsing in VR.

- Renames GetVRDevice(...) to CreateVRDisplayInfo(...) for better
understandability.
- Refactores the addition for services in VRDeviceManager to simplify the code.
- Makes the client of VRDisplayImpl and VRServiceImpl private. Added delegate
methods where calls to the client are necessary.
- Makes sure that VRDisplayImpl's client is initialized in the constructor. A
call to an uninitialized client was the actual source of the bug.
- Moves the instantiation of VRDisplayImpl objects to VRServiceImpl since
VRServiceImpl is the owner of the VRDisplayImpls.
- Adds class comments to the touched classes.
- Adaptes the unit tests accordingly.

BUG=703318

Review-Url: https://codereview.chromium.org/2746233002
Cr-Commit-Position: refs/heads/master@{#458483}
Committed:
https://chromium.googlesource.com/chromium/src/+/e3db9aa8aa3160f96599766955bf...
==========

[commit-bot: I haz the power, 2017-03-21 18:31:46]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/e3db9aa8aa3160f96599766955bf...