|
|
Created:
3 years, 9 months ago by tiborg Modified:
3 years, 8 months ago CC:
chromium-reviews, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, feature-vr-reviews_chromium.org, darin (slow to review), billorr Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFixes 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
Patch Set 1 #
Total comments: 28
Patch Set 2 : Nits, comment clarifications #
Total comments: 2
Patch Set 3 : nit #
Total comments: 8
Patch Set 4 : Rebased on ToT, nits, removed DCHECK #
Total comments: 1
Patch Set 5 : nit #Patch Set 6 : Fixed tests #Patch Set 7 : Fixed export #
Messages
Total messages: 53 (28 generated)
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@chromium.org changed reviewers: + mthiesse@chromium.org
tiborg@chromium.org changed reviewers: + bajones@chromium.org, bshe@chromium.org
(FYI you have to send out a message for us to get emails about CLs)
Please take a look
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@chromium.org changed reviewers: + amp@chromium.org
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.
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.
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.
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.
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.
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.
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.
device/vr lgtm
The CQ bit was checked by tiborg@chromium.org
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")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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@chromium.org changed reviewers: + dcheng@chromium.org
dcheng@: Please review changes in //src/device/vr/vr_service.mojom. Thanks!
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.
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.
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.
The CQ bit was checked by tiborg@chromium.org
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")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by tiborg@chromium.org
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")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by tiborg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by tiborg@chromium.org
The CQ bit was checked by tiborg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
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 ==========
The CQ bit was checked by tiborg@chromium.org
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")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by tiborg@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 120001, "attempt_start_ts": 1490116949399280, "parent_rev": "728296503744502f3460a11022f0e4281477f3c5", "commit_rev": "e3db9aa8aa3160f96599766955bf286a64a072e2"}
Message was sent while issue was closed.
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... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/e3db9aa8aa3160f96599766955bf... |