mojo VR interface simplified

mojo VR interface simplified

Since mojo interface prefer API without index transfer, current
implementation should be modified.

According to dcheng@ and bajones@ comments, WebVR implementation should
modified to let display/frame communicate directly so that index
parameters are no longer need.

This patch is based on the strategy above and will have three stages:
1(done). Modify event handler part;
2(current). Modify VRService Interface;
3. Modify unittest part;

BUG=652080
R=leon.han@intel.com,dcheng@chromium.org,bshe@chromium.org,bajones@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/91d0e2e835b7e000e432f3df690ed37a36cde1ec
Cr-Commit-Position: refs/heads/master@{#431210}

[shaobo.yan, 2016-10-14 17:25:12]

This patch has finished most of the mojo simplified work. And next patch set
will focused on mock hardware. leon@ PTAL.

[shaobo.yan, 2016-10-14 17:26:17]

Description was changed from

==========
[WIP] mojo VR interface simplified

Since mojo interface prefer API without index transfer, current
implementation should be modified.

According to dcheng@ and bajones@ comments, WebVR implementation should
modified to let display/frame communicate directly so that index
parameters are no longer need.

This patch is based on the strategy above and will have three stages:
1(done). Modify event handler part;
2(current). Modify VRService Interface;
3. Modify unittest part;

BUG=652080
R=leon.han@intel.com,dcheng@chromium.org,bshe@chromium.org,bajones@chromium.org
==========

to

==========
[WIP] mojo VR interface simplified

Since mojo interface prefer API without index transfer, current
implementation should be modified.

According to dcheng@ and bajones@ comments, WebVR implementation should
modified to let display/frame communicate directly so that index
parameters are no longer need.

This patch is based on the strategy above and will have three stages:
1(done). Modify event handler part;
2(current). Modify VRService Interface;
3. Modify unittest part;

BUG=652080
R=leon.han@intel.com,dcheng@chromium.org,bshe@chromium.org,bajones@chromium.org
==========

[shaobo.yan, 2016-10-14 17:27:59]

BTW the patch only passed compile and further test is on progress. Leon@ PTAL

[bajones, 2016-10-14 22:31:10]

There's an opportunity to make the access checks more robust with this patch.
The limit of one service and one device presenting at a time was an
oversimplification for the sake of getting something working. Ideally we would
want to allow two different services to present to two different devices are the
same time. (That's not a terribly likely scenario in the real world, but there's
no reason we can't support it.) As such, it makes sense to drop the
vr_device_helper and have the various "IsAccessAllowed" and similar checks
happen directly on the VRDevice. So switching:

    if (!helper()->IsAccessAllowed(service, this->id()))

to:

    if (!IsAccessAllowed(service))

with IsAccessAllowed being simply:

    VRDisplay::IsAccessAllowed(service) {
       return (!presenting_service_ || presenting_service_ == service);
    }

[shaobo.yan, 2016-10-15 02:52:06]

On 2016/10/14 22:31:10, bajones wrote:
> There's an opportunity to make the access checks more robust with this patch.
> The limit of one service and one device presenting at a time was an
> oversimplification for the sake of getting something working. Ideally we would
> want to allow two different services to present to two different devices are
the
> same time. (That's not a terribly likely scenario in the real world, but
there's
> no reason we can't support it.) As such, it makes sense to drop the
> vr_device_helper and have the various "IsAccessAllowed" and similar checks
> happen directly on the VRDevice. So switching:
> 
>     if (!helper()->IsAccessAllowed(service, this->id()))
> 
> to:
> 
>     if (!IsAccessAllowed(service))
> 
> with IsAccessAllowed being simply:
> 
>     VRDisplay::IsAccessAllowed(service) {
>        return (!presenting_service_ || presenting_service_ == service);
>     }

Thx for reviewing and congratulations to you and the lucky little one bajones@!
And the comments make sense, is there any other situations should I consider?

[shaobo.yan, 2016-10-17 05:06:47]

leon@ PTAL.

[leonhsl(Using Gerrit), 2016-10-17 09:03:30]

I have not made clear all about the code logic, but posted some comments to
drive. 
One suggestion: create a draft design to clarify your solution and get it
reviewed, then we can make a high level consensus firstly, and I suppose it will
also become easier for bajones@ and dcheng@ to give their opinions.

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

https://codereview.chromium.org/2420743003/diff/40001/device/vr/vr_service.mo...
device/vr/vr_service.mojom:52: uint32 index;
I suppose our target is to avoid transferring the index.

https://codereview.chromium.org/2420743003/diff/40001/device/vr/vr_service.mo...
device/vr/vr_service.mojom:61: VRDisplay? display;
Does it make sense when VRDisplay is null? For what scenario?

https://codereview.chromium.org/2420743003/diff/40001/device/vr/vr_service.mo...
device/vr/vr_service.mojom:62: VRDeviceService? service;
Same question with above, what scenario for the null VRDeviceService?

https://codereview.chromium.org/2420743003/diff/40001/device/vr/vr_service.mo...
device/vr/vr_service.mojom:76: interface VRDeviceService {
We should avoid such interface naming as XXXService, as it is not a real
service,, how about 'VRDevice'?

https://codereview.chromium.org/2420743003/diff/40001/device/vr/vr_service.mo...
device/vr/vr_service.mojom:90: interface VRDisplayService {
Naming change: to 'VRDisplay' or 'VRDeviceClient'

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

https://codereview.chromium.org/2420743003/diff/40001/device/vr/vr_service_im...
device/vr/vr_service_impl.cc:56: return std::move(device_services_[device]);
After std::move(), device_services_[device] became invalid and can not be
referred anymore.

https://codereview.chromium.org/2420743003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right):

https://codereview.chromium.org/2420743003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:472:
m_service->RegisterDisplayService(m_binding.CreateInterfacePtrAndBind());
As mentioned before, m_binding can only be bound once, but I suppose this
function would be called multiple times.

[shaobo.yan, 2016-10-18 01:10:48]

Thx for reviewing leon@ ! and I think a design draft is a great idea. I'll work
it out ASAP.

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

https://codereview.chromium.org/2420743003/diff/40001/device/vr/vr_service.mo...
device/vr/vr_service.mojom:52: uint32 index;
On 2016/10/17 09:03:29, leonhsl wrote:
> I suppose our target is to avoid transferring the index.

Since WebVR spec need each VRDisplay should have an id attribute, I thought the
index could be setted as id directly. But if mojo really doesn't want index be
transferred, I'll remove it and use another id generation in blink side.

https://codereview.chromium.org/2420743003/diff/40001/device/vr/vr_service.mo...
device/vr/vr_service.mojom:62: VRDeviceService? service;
On 2016/10/17 09:03:29, leonhsl wrote:
> Same question with above, what scenario for the null VRDeviceService?

The VRDisplay should not be nullptr but the VRDeviceService could be.

The display and service should not be nullptr. Since all of them are related to
an VRDevice object and the object has been checked not null. So it should not be
nullptr.

https://codereview.chromium.org/2420743003/diff/40001/device/vr/vr_service.mo...
device/vr/vr_service.mojom:76: interface VRDeviceService {
On 2016/10/17 09:03:29, leonhsl wrote:
> We should avoid such interface naming as XXXService, as it is not a real
> service,, how about 'VRDevice'?

Oh, thx for the info ! We already have a class named VRDevice. This interface is
used by VRDisplay object to communicate with VRDevice object on blink side,
maybe VRDeviceClient could be a name.

https://codereview.chromium.org/2420743003/diff/40001/device/vr/vr_service.mo...
device/vr/vr_service.mojom:90: interface VRDisplayService {
On 2016/10/17 09:03:29, leonhsl wrote:
> Naming change: to 'VRDisplay' or 'VRDeviceClient'

This interface is used by VRDevice to inform event to VRDisplay on blink side.
I'll change it to VRDisplayClient.

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

https://codereview.chromium.org/2420743003/diff/40001/device/vr/vr_service_im...
device/vr/vr_service_impl.cc:56: return std::move(device_services_[device]);
On 2016/10/17 09:03:29, leonhsl wrote:
> After std::move(), device_services_[device] became invalid and can not be
> referred anymore.

I'm so stupid. Thx for finding this. I'll update the logic and not referred to
deviceService any more.

https://codereview.chromium.org/2420743003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right):

https://codereview.chromium.org/2420743003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:472:
m_service->RegisterDisplayService(m_binding.CreateInterfacePtrAndBind());
On 2016/10/17 09:03:29, leonhsl wrote:
> As mentioned before, m_binding can only be bound once, but I suppose this
> function would be called multiple times.

Yes. Will update it.

[shaobo.yan, 2016-10-18 05:33:46]

leon@, dcheng@, bajones@ bshe@,
here is the design draft, pls comment.
https://docs.google.com/document/d/15vnRtmruDOMGG2A_FXInt3ueNfFmBylCjf5SSJicr...

[leonhsl(Using Gerrit), 2016-10-18 07:12:10]

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

https://codereview.chromium.org/2420743003/diff/40001/device/vr/vr_service.mo...
device/vr/vr_service.mojom:52: uint32 index;
On 2016/10/18 01:10:48, shaobo.yan wrote:
> On 2016/10/17 09:03:29, leonhsl wrote:
> > I suppose our target is to avoid transferring the index.
> 
> Since WebVR spec need each VRDisplay should have an id attribute, I thought
the
> index could be setted as id directly. But if mojo really doesn't want index be
> transferred, I'll remove it and use another id generation in blink side.

This is not requested by mojo. I think one motivate of the refactor is to avoid
specifying device<-->display pair by an index, and transferring index everywhere
seems be unnecessary at least for apis such as onDisplayChanged and
onDisplayConnected etc.

https://codereview.chromium.org/2420743003/diff/40001/device/vr/vr_service.mo...
device/vr/vr_service.mojom:76: interface VRDeviceService {
On 2016/10/18 01:10:48, shaobo.yan wrote:
> On 2016/10/17 09:03:29, leonhsl wrote:
> > We should avoid such interface naming as XXXService, as it is not a real
> > service,, how about 'VRDevice'?
> 
> Oh, thx for the info ! We already have a class named VRDevice. This interface
is
> used by VRDisplay object to communicate with VRDevice object on blink side,
> maybe VRDeviceClient could be a name.

You could change 'module device' to 'module device.mojom', this way generated
class will live under a nested namespace device::mojom, to avoid name collision.
I think (VRDevice, VRDeviceClient) pair is appropriate naming practice to align
with other mojo interfaces inside Chromium.

[shaobo.yan, 2016-10-18 07:25:11]

On 2016/10/18 07:12:10, leonhsl wrote:
>
https://codereview.chromium.org/2420743003/diff/40001/device/vr/vr_service.mojom
> File device/vr/vr_service.mojom (right):
> 
>
https://codereview.chromium.org/2420743003/diff/40001/device/vr/vr_service.mo...
> device/vr/vr_service.mojom:52: uint32 index;
> On 2016/10/18 01:10:48, shaobo.yan wrote:
> > On 2016/10/17 09:03:29, leonhsl wrote:
> > > I suppose our target is to avoid transferring the index.
> > 
> > Since WebVR spec need each VRDisplay should have an id attribute, I thought
> the
> > index could be setted as id directly. But if mojo really doesn't want index
be
> > transferred, I'll remove it and use another id generation in blink side.
> 
> This is not requested by mojo. I think one motivate of the refactor is to
avoid
> specifying device<-->display pair by an index, and transferring index
everywhere
> seems be unnecessary at least for apis such as onDisplayChanged and
> onDisplayConnected etc.
> 
>
https://codereview.chromium.org/2420743003/diff/40001/device/vr/vr_service.mo...
> device/vr/vr_service.mojom:76: interface VRDeviceService {
> On 2016/10/18 01:10:48, shaobo.yan wrote:
> > On 2016/10/17 09:03:29, leonhsl wrote:
> > > We should avoid such interface naming as XXXService, as it is not a real
> > > service,, how about 'VRDevice'?
> > 
> > Oh, thx for the info ! We already have a class named VRDevice. This
interface
> is
> > used by VRDisplay object to communicate with VRDevice object on blink side,
> > maybe VRDeviceClient could be a name.
> 
> You could change 'module device' to 'module device.mojom', this way generated
> class will live under a nested namespace device::mojom, to avoid name
collision.
> I think (VRDevice, VRDeviceClient) pair is appropriate naming practice to
align
> with other mojo interfaces inside Chromium.

Thx for the example! So I'll update it to VRDeviceClient

[shaobo.yan, 2016-10-24 01:40:18]

leon@ and bajones@. PTAL.
P.S: the design doc is updated and links here:
https://docs.google.com/document/d/15vnRtmruDOMGG2A_FXInt3ueNfFmBylCjf5SSJicr...

[leonhsl(Using Gerrit), 2016-10-24 02:29:22]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-24 02:29:32]

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, 2016-10-24 02:31:23]

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

[commit-bot: I haz the power, 2016-10-24 02:31:23]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[leonhsl(Using Gerrit), 2016-10-25 03:17:40]

One suggestion: It would be better to get all trybots green firstly.

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

https://codereview.chromium.org/2420743003/diff/80001/device/vr/vr_service.mo...
device/vr/vr_service.mojom:80: interface VRDeviceClient {
Still old comments about interface naming... : (VRDevice, VRDeviceClient) pair
should have the similar relationship with existing Chromium interface pairs, for
example (VRService, VRServiceClient) pair, but now seems both VRDevice and
VRDeviceClient live in Browser and have no any mojo connection.

[shaobo.yan, 2016-10-25 03:44:50]

The CQ bit was checked by shaobo.yan@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-25 03:45:04]

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, 2016-10-25 03:45:05]

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

[commit-bot: I haz the power, 2016-10-25 03:45:08]

Dry run: No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[shaobo.yan, 2016-10-25 03:47:25]

On 2016/10/25 03:17:40, leonhsl wrote:
> One suggestion: It would be better to get all trybots green firstly.
> 
>
https://codereview.chromium.org/2420743003/diff/80001/device/vr/vr_service.mojom
> File device/vr/vr_service.mojom (right):
> 
>
https://codereview.chromium.org/2420743003/diff/80001/device/vr/vr_service.mo...
> device/vr/vr_service.mojom:80: interface VRDeviceClient {
> Still old comments about interface naming... : (VRDevice, VRDeviceClient) pair
> should have the similar relationship with existing Chromium interface pairs,
for
> example (VRService, VRServiceClient) pair, but now seems both VRDevice and
> VRDeviceClient live in Browser and have no any mojo connection.

I've done rebase with patch set 6.

About VRDevice, VRDeviceClient would be held by VRDisplay which lives in blink.
But like VRService, its impl class will live in browser.
What's your suggestion about dealing with this situation?

[leonhsl(Using Gerrit), 2016-10-25 05:15:13]

On 2016/10/25 03:47:25, shaobo.yan wrote:
> On 2016/10/25 03:17:40, leonhsl wrote:
> > One suggestion: It would be better to get all trybots green firstly.
> > 
> >
>
https://codereview.chromium.org/2420743003/diff/80001/device/vr/vr_service.mojom
> > File device/vr/vr_service.mojom (right):
> > 
> >
>
https://codereview.chromium.org/2420743003/diff/80001/device/vr/vr_service.mo...
> > device/vr/vr_service.mojom:80: interface VRDeviceClient {
> > Still old comments about interface naming... : (VRDevice, VRDeviceClient)
pair
> > should have the similar relationship with existing Chromium interface pairs,
> for
> > example (VRService, VRServiceClient) pair, but now seems both VRDevice and
> > VRDeviceClient live in Browser and have no any mojo connection.
> 
> I've done rebase with patch set 6.
> 
> About VRDevice, VRDeviceClient would be held by VRDisplay which lives in
blink.
> But like VRService, its impl class will live in browser.
> What's your suggestion about dealing with this situation?

VRDevice lives in browser, VRDeviceClient lives in blink, just like (VRService,
VRServiceClient)

[leonhsl(Using Gerrit), 2016-10-25 05:15:37]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-25 05:15:49]

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, 2016-10-25 05:34:46]

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

[commit-bot: I haz the power, 2016-10-25 05:34:47]

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

[shaobo.yan, 2016-10-25 08:47:07]

Description was changed from

==========
[WIP] mojo VR interface simplified

Since mojo interface prefer API without index transfer, current
implementation should be modified.

According to dcheng@ and bajones@ comments, WebVR implementation should
modified to let display/frame communicate directly so that index
parameters are no longer need.

This patch is based on the strategy above and will have three stages:
1(done). Modify event handler part;
2(current). Modify VRService Interface;
3. Modify unittest part;

BUG=652080
R=leon.han@intel.com,dcheng@chromium.org,bshe@chromium.org,bajones@chromium.org
==========

to

==========
[WIP] mojo VR interface simplified

Since mojo interface prefer API without index transfer, current
implementation should be modified.

According to dcheng@ and bajones@ comments, WebVR implementation should
modified to let display/frame communicate directly so that index
parameters are no longer need.

This patch is based on the strategy above and will have three stages:
1(done). Modify event handler part;
2(current). Modify VRService Interface;
3. Modify unittest part;

BUG=652080
R=leon.han@intel.com,dcheng@chromium.org,bshe@chromium.org,bajones@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[shaobo.yan, 2016-10-25 08:48:09]

leon@ and bajones@ PTAL

[bajones, 2016-10-25 22:21:38]

Apologies again for the slowness of my reviews. Several comments/questions.

https://codereview.chromium.org/2420743003/diff/120001/device/vr/android/gvr/...
File device/vr/android/gvr/gvr_device.cc (right):

https://codereview.chromium.org/2420743003/diff/120001/device/vr/android/gvr/...
device/vr/android/gvr/gvr_device.cc:202: return false;
Any reason why CheckAccessAllowed isn't used here?

https://codereview.chromium.org/2420743003/diff/120001/device/vr/android/gvr/...
device/vr/android/gvr/gvr_device.cc:213: if (!gvr_provider_->RequestPresent()) {
This change is unnecessary.

https://codereview.chromium.org/2420743003/diff/120001/device/vr/android/gvr/...
device/vr/android/gvr/gvr_device.cc:257: this->OnDisplayChanged();
just OnDisplayChanged(), please.

https://codereview.chromium.org/2420743003/diff/120001/device/vr/android/gvr/...
device/vr/android/gvr/gvr_device.cc:268: void GvrDevice::OnDisplayChanged() {
There's nothing GvrDevice specific about any of these functions. Can we move the
implementation to vr_device.cc so they can be shared by all VRDevices?

https://codereview.chromium.org/2420743003/diff/120001/device/vr/vr_device.cc
File device/vr/vr_device.cc (right):

https://codereview.chromium.org/2420743003/diff/120001/device/vr/vr_device.cc...
device/vr/vr_device.cc:26: void VRDevice::SetClient(std::vector<VRServiceImpl*>*
clients) {
Weird mismatch in terminology here and elsewhere where we refer to these as
"clients" when adding them and "services" most other places. Let's decide on one
term and stick with it.

https://codereview.chromium.org/2420743003/diff/120001/device/vr/vr_device_cl...
File device/vr/vr_device_client_impl_unittest.cc (right):

https://codereview.chromium.org/2420743003/diff/120001/device/vr/vr_device_cl...
device/vr/vr_device_client_impl_unittest.cc:5: /*#include
"device/vr/vr_service_impl.h"
Let's not check this in commented out.

https://codereview.chromium.org/2420743003/diff/120001/device/vr/vr_service_i...
File device/vr/vr_service_impl_unittest.cc (right):

https://codereview.chromium.org/2420743003/diff/120001/device/vr/vr_service_i...
device/vr/vr_service_impl_unittest.cc:107: /*TEST_F(VRServiceImplTest,
DeviceChangedDispatched) {
Again, let's not commit commented out blocks like this. If it's absolutely
necessary, please include a TODO comment indicating why.

https://codereview.chromium.org/2420743003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/vr/VRController.cpp (right):

https://codereview.chromium.org/2420743003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/vr/VRController.cpp:60:
callback->onSuccess(outDisplays);
Probably shouldn't call onSuccess if success == false.

https://codereview.chromium.org/2420743003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right):

https://codereview.chromium.org/2420743003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:43: m_displayId(nextId),
I know that there's been discussion about the fact that we shouldn't be
transferring the id via Mojo but I'm concerned that this breaks integration with
the Gamepad API, which also uses the VRDisplay ID to identify which HMD it's
associated with.

I'm glad that this patch breaks the dependency on the ID for IPC calls, but I'd
prefer to continue using the ID assigned by the browser process for now for the
sake of gamepad. I'll look at refactoring that particular dependency soon.

[shaobo.yan, 2016-10-26 01:19:22]

https://codereview.chromium.org/2420743003/diff/120001/device/vr/android/gvr/...
File device/vr/android/gvr/gvr_device.cc (right):

https://codereview.chromium.org/2420743003/diff/120001/device/vr/android/gvr/...
device/vr/android/gvr/gvr_device.cc:257: this->OnDisplayChanged();
On 2016/10/25 22:21:38, bajones wrote:
> just OnDisplayChanged(), please.

Sorry about these three fault.

https://codereview.chromium.org/2420743003/diff/120001/device/vr/android/gvr/...
device/vr/android/gvr/gvr_device.cc:268: void GvrDevice::OnDisplayChanged() {
On 2016/10/25 22:21:38, bajones wrote:
> There's nothing GvrDevice specific about any of these functions. Can we move
the
> implementation to vr_device.cc so they can be shared by all VRDevices?

Sure and if any kind of devices need different behaviour, they could override
it.

https://codereview.chromium.org/2420743003/diff/120001/device/vr/vr_device.cc
File device/vr/vr_device.cc (right):

https://codereview.chromium.org/2420743003/diff/120001/device/vr/vr_device.cc...
device/vr/vr_device.cc:26: void VRDevice::SetClient(std::vector<VRServiceImpl*>*
clients) {
On 2016/10/25 22:21:38, bajones wrote:
> Weird mismatch in terminology here and elsewhere where we refer to these as
> "clients" when adding them and "services" most other places. Let's decide on
one
> term and stick with it.

Sorry about this but a little explain : since the name changed for several
times, I'm busy with modify all of this. However, the name seems to be settle
down yesterday and I'll review all the names and correct them.

https://codereview.chromium.org/2420743003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right):

https://codereview.chromium.org/2420743003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:43: m_displayId(nextId),
On 2016/10/25 22:21:38, bajones wrote:
> I know that there's been discussion about the fact that we shouldn't be
> transferring the id via Mojo but I'm concerned that this breaks integration
with
> the Gamepad API, which also uses the VRDisplay ID to identify which HMD it's
> associated with.
> 
> I'm glad that this patch breaks the dependency on the ID for IPC calls, but
I'd
> prefer to continue using the ID assigned by the browser process for now for
the
> sake of gamepad. I'll look at refactoring that particular dependency soon. 

Shall I take the chance to see if the gamepad part could be refactory in this
patch ? :)

[bajones, 2016-10-26 08:35:55]

> Shall I take the chance to see if the gamepad part could be refactory in this
> patch ? :)

This patch is already pretty large, let's not make it bigger. Gamepad changes
can come in a follow up CL.

[bajones, 2016-10-27 06:57:25]

FYI: This CL has become a blocker for me, so I tried to build against it today.
Found a few basic errors that were easy to fix, but the Chrome crashes when you
attempt to run with this patch because it uses a synchronous mojo call from the
browser process to the renderer, which is explicitly forbidden.

Because my work is now dependent on this refactor being completed I took the
opportunity to try and refactor the bad synchronous call out and, honestly, got
a little carried away. I found the new interface names felt confusing and the
flow for establishing the connection between the two ends of the
VRDisplay/Device to be hard to follow. So I began fiddling with a variant of
this patch that I feel cleans up the connection handshake quite a bit. Take a
look: https://codereview.chromium.org/2453973002/

Due to a technical glitch I haven't actually built that patch just yet, and I'm
sure it'll have some errors, but it generally shows my thoughts on the class
names and mojo flow. (Also haven't updated the tests at all, so those are
broken.) I'm happy to continue working on it tomorrow if that's easiest, or you
can feel free to grab some or all of the code and build off of it. I'm perfectly
happy to have this land under your name, even with my changes included. All I
really care about at this point is seeing the refactor land so that it unblocks
some dependent changes I've got coming.

--Brandon

[shaobo.yan, 2016-10-27 07:05:53]

On 2016/10/27 06:57:25, bajones wrote:
> FYI: This CL has become a blocker for me, so I tried to build against it
today.
> Found a few basic errors that were easy to fix, but the Chrome crashes when
you
> attempt to run with this patch because it uses a synchronous mojo call from
the
> browser process to the renderer, which is explicitly forbidden.
> 
> Because my work is now dependent on this refactor being completed I took the
> opportunity to try and refactor the bad synchronous call out and, honestly,
got
> a little carried away. I found the new interface names felt confusing and the
> flow for establishing the connection between the two ends of the
> VRDisplay/Device to be hard to follow. So I began fiddling with a variant of
> this patch that I feel cleans up the connection handshake quite a bit. Take a
> look: https://codereview.chromium.org/2453973002/
> 
> Due to a technical glitch I haven't actually built that patch just yet, and
I'm
> sure it'll have some errors, but it generally shows my thoughts on the class
> names and mojo flow. (Also haven't updated the tests at all, so those are
> broken.) I'm happy to continue working on it tomorrow if that's easiest, or
you
> can feel free to grab some or all of the code and build off of it. I'm
perfectly
> happy to have this land under your name, even with my changes included. All I
> really care about at this point is seeing the refactor land so that it
unblocks
> some dependent changes I've got coming.
> 
> --Brandon
Sorry for the blocking. Due to the change of the class name, I've not tested the
patch until now. And I also find the crash reason of sync call from browser
process. And I've finished fixed it out but not upload the patch set yet.
About the name, I think I'm really not good at it. I'm glad to see you have
ideas about them.
I'll update according to your change and test them ASAP. 
BR,
Yan,Shaobo

[shaobo.yan, 2016-10-28 03:01:48]

On 2016/10/27 07:05:53, shaobo.yan wrote:
> On 2016/10/27 06:57:25, bajones wrote:
> > FYI: This CL has become a blocker for me, so I tried to build against it
> today.
> > Found a few basic errors that were easy to fix, but the Chrome crashes when
> you
> > attempt to run with this patch because it uses a synchronous mojo call from
> the
> > browser process to the renderer, which is explicitly forbidden.
> > 
> > Because my work is now dependent on this refactor being completed I took the
> > opportunity to try and refactor the bad synchronous call out and, honestly,
> got
> > a little carried away. I found the new interface names felt confusing and
the
> > flow for establishing the connection between the two ends of the
> > VRDisplay/Device to be hard to follow. So I began fiddling with a variant of
> > this patch that I feel cleans up the connection handshake quite a bit. Take
a
> > look: https://codereview.chromium.org/2453973002/
> > 
> > Due to a technical glitch I haven't actually built that patch just yet, and
> I'm
> > sure it'll have some errors, but it generally shows my thoughts on the class
> > names and mojo flow. (Also haven't updated the tests at all, so those are
> > broken.) I'm happy to continue working on it tomorrow if that's easiest, or
> you
> > can feel free to grab some or all of the code and build off of it. I'm
> perfectly
> > happy to have this land under your name, even with my changes included. All
I
> > really care about at this point is seeing the refactor land so that it
> unblocks
> > some dependent changes I've got coming.
> > 
> > --Brandon
> Sorry for the blocking. Due to the change of the class name, I've not tested
the
> patch until now. And I also find the crash reason of sync call from browser
> process. And I've finished fixed it out but not upload the patch set yet.
> About the name, I think I'm really not good at it. I'm glad to see you have
> ideas about them.
> I'll update according to your change and test them ASAP. 
> BR,
> Yan,Shaobo

Hi, brandon,
the latest patch set has rebased on your latest change :) thx for your help with
this issue.
The test case is not updated.
Based on the chromium built out, getVRDisplay indeed has some problem to pop up
device info right when I first call it. But When second time, the info has been
poped up correctly.
I'll test and fix other problem based on it.

BR,
Yan,Shaobo

[shaobo.yan, 2016-11-01 08:05:57]

It seems the refactory could be settle down now.
The patch have been test through WebVR Sample and have the similar behaviour
with the one before refactory.
Thx for bajones@ help with the more clear name and more clear mojo connection
flow.
There is some TODO remains but I have some different ideas, so I just remain
them for discussion.

bajones@ and dcheng@ PTAL.

[bajones, 2016-11-01 18:36:10]

Doing the rest of the review now, but FYI: You'll need to update the service
name in
https://codereview.chromium.org/2463393002/diff/1/content/public/app/mojo/con...
to match the new mojom:: namespace. (Only VRService should be needed in that
file, everything else is communicated directly across an already established
connection.) We didn't know about the requirement to list the service in that
file until yesterday, so sorry for the confusion. :P

[bajones, 2016-11-01 18:48:42]

On 2016/11/01 18:36:10, bajones wrote:
> Doing the rest of the review now, but FYI: You'll need to update the service
> name in
>
https://codereview.chromium.org/2463393002/diff/1/content/public/app/mojo/con...
> to match the new mojom:: namespace. (Only VRService should be needed in that
> file, everything else is communicated directly across an already established
> connection.) We didn't know about the requirement to list the service in that
> file until yesterday, so sorry for the confusion. :P

Aside from that pending rename, LGTM! The outstanding TODOs are all items I feel
should be handled in a follow up CL.

Thanks!

[haraken, 2016-11-02 02:04:36]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org, reillyg@chromium.org, rockot@chromium.org

[haraken, 2016-11-02 02:04:37]

https://codereview.chromium.org/2420743003/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/vr/VRController.h (right):

https://codereview.chromium.org/2420743003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/vr/VRController.h:55:
mojo::Binding<device::mojom::blink::VRServiceClient> m_binding;

We'll need to add a pre-finalizer to VRController and clear m_binding on the
pre-finalizer.

reillyg@, rockot@: I think rockot@ has implemented some way that allows us to
avoid adding a pre-finalizer. How can we avoid the pre-finalizer? :)

[shaobo.yan, 2016-11-02 02:16:21]

On 2016/11/02 02:04:37, haraken wrote:
>
https://codereview.chromium.org/2420743003/diff/240001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/modules/vr/VRController.h (right):
> 
>
https://codereview.chromium.org/2420743003/diff/240001/third_party/WebKit/Sou...
> third_party/WebKit/Source/modules/vr/VRController.h:55:
> mojo::Binding<device::mojom::blink::VRServiceClient> m_binding;
> 
> We'll need to add a pre-finalizer to VRController and clear m_binding on the
> pre-finalizer.
> 
> reillyg@, rockot@: I think rockot@ has implemented some way that allows us to
> avoid adding a pre-finalizer. How can we avoid the pre-finalizer? :)

Thx for reviewing ! I forget to add pre-finailizer to VRController, which is
similar to VRDisplay :)
But if there is any possible way which could avoid pre-finailizer. I'd like to
update in that way :)

[Ken Rockot(use gerrit already), 2016-11-02 02:35:49]

On Tue, Nov 1, 2016 at 7:16 PM, <shaobo.yan@intel.com> wrote:

> On 2016/11/02 02:04:37, haraken wrote:
> >
> https://codereview.chromium.org/2420743003/diff/240001/
> third_party/WebKit/Source/modules/vr/VRController.h
> > File third_party/WebKit/Source/modules/vr/VRController.h (right):
> >
> >
> https://codereview.chromium.org/2420743003/diff/240001/
> third_party/WebKit/Source/modules/vr/VRController.h#newcode55
> > third_party/WebKit/Source/modules/vr/VRController.h:55:
> > mojo::Binding<device::mojom::blink::VRServiceClient> m_binding;
> >
> > We'll need to add a pre-finalizer to VRController and clear m_binding on
> the
> > pre-finalizer.
> >
> > reillyg@, rockot@: I think rockot@ has implemented some way that allows
> us to
> > avoid adding a pre-finalizer. How can we avoid the pre-finalizer? :)
>
> Thx for reviewing ! I forget to add pre-finailizer to VRController, which
> is
> similar to VRDisplay :)
> But if there is any possible way which could avoid pre-finailizer. I'd
> like to
> update in that way :)
>

It is possible to template mojo::Binding over an additional traits type to
control how the implementation reference is stored internally. Please see
raw_ptr_impl_ref_traits.h
<https://cs.chromium.org/chromium/src/mojo/public/cpp/bindings/raw_ptr_impl_re...>
for
the default traits, and BindingTest.CustomImplPointerType
<https://cs.chromium.org/chromium/src/testing/gtest/include/gtest/gtest.h?l=44...>
for
an example which uses base::WeakPtr instead of a raw pointer. You should be
able to mimic this behavior for blink::WeakPersistent.


>
> https://codereview.chromium.org/2420743003/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[Ken Rockot(use gerrit already), 2016-11-02 02:35:50]

On Tue, Nov 1, 2016 at 7:16 PM, <shaobo.yan@intel.com> wrote:

> On 2016/11/02 02:04:37, haraken wrote:
> >
> https://codereview.chromium.org/2420743003/diff/240001/
> third_party/WebKit/Source/modules/vr/VRController.h
> > File third_party/WebKit/Source/modules/vr/VRController.h (right):
> >
> >
> https://codereview.chromium.org/2420743003/diff/240001/
> third_party/WebKit/Source/modules/vr/VRController.h#newcode55
> > third_party/WebKit/Source/modules/vr/VRController.h:55:
> > mojo::Binding<device::mojom::blink::VRServiceClient> m_binding;
> >
> > We'll need to add a pre-finalizer to VRController and clear m_binding on
> the
> > pre-finalizer.
> >
> > reillyg@, rockot@: I think rockot@ has implemented some way that allows
> us to
> > avoid adding a pre-finalizer. How can we avoid the pre-finalizer? :)
>
> Thx for reviewing ! I forget to add pre-finailizer to VRController, which
> is
> similar to VRDisplay :)
> But if there is any possible way which could avoid pre-finailizer. I'd
> like to
> update in that way :)
>

It is possible to template mojo::Binding over an additional traits type to
control how the implementation reference is stored internally. Please see
raw_ptr_impl_ref_traits.h
<https://cs.chromium.org/chromium/src/mojo/public/cpp/bindings/raw_ptr_impl_re...>
for
the default traits, and BindingTest.CustomImplPointerType
<https://cs.chromium.org/chromium/src/testing/gtest/include/gtest/gtest.h?l=44...>
for
an example which uses base::WeakPtr instead of a raw pointer. You should be
able to mimic this behavior for blink::WeakPersistent.


>
> https://codereview.chromium.org/2420743003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[shaobo.yan, 2016-11-02 06:00:22]

haraken@, rockot@ : Since I've found there is already a function named
contextDestroy could be filled in PRE_FINALIZER, so I used the macro. thx for
your guide rockot@ and maybe we could modify this in next CLs :)

haraken@ PTAL.

[haraken, 2016-11-02 08:25:11]

LGTM on my side with comments.

https://codereview.chromium.org/2420743003/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/vr/VRController.cpp (right):

https://codereview.chromium.org/2420743003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/vr/VRController.cpp:28:
WTF::bind(&VRController::onDisplaysSynced, wrapPersistent(this))));

You need to call ThreadState::current()->registerPreFinalizer(). Otherwise, the
pre-finalizer won't get called.

https://codereview.chromium.org/2420743003/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/vr/VRController.h (right):

https://codereview.chromium.org/2420743003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/vr/VRController.h:28:
USING_PRE_FINALIZER(VRController, contextDestroyed);

It's a bit strange to register a function address of an overridden method
somewhere. So I'd prefer adding a helper method like dispose() and register it
to the pre-finalizer. contextDestroyed() can just call dispose().

https://codereview.chromium.org/2420743003/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right):

https://codereview.chromium.org/2420743003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:76:
m_display->SetClient(m_binding.CreateInterfacePtrAndBind());

You need to register the pre-finalizer.

(I think we should improve the GC plugin to check that
ThreadState::current()->registerPreFinalizer() is not missing...)

[shaobo.yan, 2016-11-02 09:22:38]

https://codereview.chromium.org/2420743003/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/vr/VRController.cpp (right):

https://codereview.chromium.org/2420743003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/vr/VRController.cpp:28:
WTF::bind(&VRController::onDisplaysSynced, wrapPersistent(this))));
On 2016/11/02 08:25:11, haraken wrote:
> 
> You need to call ThreadState::current()->registerPreFinalizer(). Otherwise,
the
> pre-finalizer won't get called.

Thx for telling me that!

https://codereview.chromium.org/2420743003/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/vr/VRController.h (right):

https://codereview.chromium.org/2420743003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/vr/VRController.h:28:
USING_PRE_FINALIZER(VRController, contextDestroyed);
On 2016/11/02 08:25:11, haraken wrote:
> 
> It's a bit strange to register a function address of an overridden method
> somewhere. So I'd prefer adding a helper method like dispose() and register it
> to the pre-finalizer. contextDestroyed() can just call dispose().

Done.

https://codereview.chromium.org/2420743003/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right):

https://codereview.chromium.org/2420743003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:76:
m_display->SetClient(m_binding.CreateInterfacePtrAndBind());
On 2016/11/02 08:25:11, haraken wrote:
> 
> You need to register the pre-finalizer.
> 
> (I think we should improve the GC plugin to check that
> ThreadState::current()->registerPreFinalizer() is not missing...)

Agree with that :)

[shaobo.yan, 2016-11-02 09:22:54]

The CQ bit was checked by shaobo.yan@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-02 09:23:07]

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

[shaobo.yan, 2016-11-02 09:52:32]

dcheng@ PTAL

[shaobo.yan, 2016-11-02 09:53:07]

Description was changed from

==========
[WIP] mojo VR interface simplified

Since mojo interface prefer API without index transfer, current
implementation should be modified.

According to dcheng@ and bajones@ comments, WebVR implementation should
modified to let display/frame communicate directly so that index
parameters are no longer need.

This patch is based on the strategy above and will have three stages:
1(done). Modify event handler part;
2(current). Modify VRService Interface;
3. Modify unittest part;

BUG=652080
R=leon.han@intel.com,dcheng@chromium.org,bshe@chromium.org,bajones@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
[WIP] mojo VR interface simplified

Since mojo interface prefer API without index transfer, current
implementation should be modified.

According to dcheng@ and bajones@ comments, WebVR implementation should
modified to let display/frame communicate directly so that index
parameters are no longer need.

This patch is based on the strategy above and will have three stages:
1(done). Modify event handler part;
2(current). Modify VRService Interface;
3. Modify unittest part;

BUG=652080
R=leon.han@intel.com,dcheng@chromium.org,bshe@chromium.org,bajones@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[shaobo.yan, 2016-11-02 09:53:07]

shaobo.yan@intel.com changed reviewers:
+ clamy@chromium.org

[commit-bot: I haz the power, 2016-11-02 10:20:42]

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

[commit-bot: I haz the power, 2016-11-02 10:20:43]

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

[shaobo.yan, 2016-11-03 00:49:58]

The CQ bit was checked by shaobo.yan@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-03 00:50:29]

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

[shaobo.yan, 2016-11-03 02:07:16]

Since it already became a block issue, ping dcheng@ again

[commit-bot: I haz the power, 2016-11-03 03:27:30]

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

[commit-bot: I haz the power, 2016-11-03 03:27:32]

Dry run: This issue passed the CQ dry run.

[shaobo.yan, 2016-11-03 04:08:04]

clamy@ PTAL content part

[shaobo.yan, 2016-11-03 04:08:29]

Description was changed from

==========
[WIP] mojo VR interface simplified

Since mojo interface prefer API without index transfer, current
implementation should be modified.

According to dcheng@ and bajones@ comments, WebVR implementation should
modified to let display/frame communicate directly so that index
parameters are no longer need.

This patch is based on the strategy above and will have three stages:
1(done). Modify event handler part;
2(current). Modify VRService Interface;
3. Modify unittest part;

BUG=652080
R=leon.han@intel.com,dcheng@chromium.org,bshe@chromium.org,bajones@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
mojo VR interface simplified

Since mojo interface prefer API without index transfer, current
implementation should be modified.

According to dcheng@ and bajones@ comments, WebVR implementation should
modified to let display/frame communicate directly so that index
parameters are no longer need.

This patch is based on the strategy above and will have three stages:
1(done). Modify event handler part;
2(current). Modify VRService Interface;
3. Modify unittest part;

BUG=652080
R=leon.han@intel.com,dcheng@chromium.org,bshe@chromium.org,bajones@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[shaobo.yan, 2016-11-04 01:49:02]

Sorry but ping clamy@ and dcheng@ , PTAL content part and mojo part

[dcheng, 2016-11-04 05:12:21]

https://codereview.chromium.org/2420743003/diff/300001/device/vr/android/gvr/...
File device/vr/android/gvr/gvr_device.cc (right):

https://codereview.chromium.org/2420743003/diff/300001/device/vr/android/gvr/...
device/vr/android/gvr/gvr_device.cc:235: if (!IsAccessAllowed(service)) {
Should this be an early return?

https://codereview.chromium.org/2420743003/diff/300001/device/vr/test/fake_vr...
File device/vr/test/fake_vr_device.h (right):

https://codereview.chromium.org/2420743003/diff/300001/device/vr/test/fake_vr...
device/vr/test/fake_vr_device.h:25: device_registered = !device_registered;
Why does this mutate device_registered?

https://codereview.chromium.org/2420743003/diff/300001/device/vr/test/fake_vr...
device/vr/test/fake_vr_device.h:46: unsigned updated;
Nit: fields should end with _

https://codereview.chromium.org/2420743003/diff/300001/device/vr/vr_device.cc
File device/vr/vr_device.cc (right):

https://codereview.chromium.org/2420743003/diff/300001/device/vr/vr_device.cc...
device/vr/vr_device.cc:30: std::pair<VRServiceImpl*, VRDisplayImpl*>(service,
display_impl));
Use std::make_pair

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

https://codereview.chromium.org/2420743003/diff/300001/device/vr/vr_device.h#...
device/vr/vr_device.h:59: // TODO : Since the VRDisplayImpl knows it's
VRServiceImpl we should only need
TODO should have a username or bug number associated.

Also a few typos:
it's => its
tp => to

https://codereview.chromium.org/2420743003/diff/300001/device/vr/vr_device_ma...
File device/vr/vr_device_manager.cc (right):

https://codereview.chromium.org/2420743003/diff/300001/device/vr/vr_device_ma...
device/vr/vr_device_manager.cc:100: if (!devices.size())
Use empty()

https://codereview.chromium.org/2420743003/diff/300001/device/vr/vr_device_ma...
File device/vr/vr_device_manager.h (right):

https://codereview.chromium.org/2420743003/diff/300001/device/vr/vr_device_ma...
device/vr/vr_device_manager.h:40: DEVICE_VR_EXPORT unsigned int GetDevicesNum();
Nit: don't use abbreviations like Num. GetNumberOfDevices is fine. Otherwise,
the method name reads ambiguously.

https://codereview.chromium.org/2420743003/diff/300001/device/vr/vr_service.m...
File device/vr/vr_service.mojom (right):

https://codereview.chromium.org/2420743003/diff/300001/device/vr/vr_service.m...
device/vr/vr_service.mojom:68: SetClient(VRServiceClient client) => (uint32
num);
In a followup, it'd be better if we had a factory function that took a
VRServiceClient so we would never have a half-initialized VRService: as it
stands, it's possible for the renderer to never set a client. The browser never
checks that client() is non-null.

Also, I'm not sure what |num| is: please make the name more descriptive.

https://codereview.chromium.org/2420743003/diff/300001/device/vr/vr_service.m...
device/vr/vr_service.mojom:76: SetClient(VRDisplayClient client);
Ditto: it'd be better for this to be created through some sort of factory
function, so that we're never in the half-initialized state. This one is easy to
fix: we can pass down a VRDisplayClient& in OnDisplayConnected().

https://codereview.chromium.org/2420743003/diff/300001/device/vr/vr_service_i...
File device/vr/vr_service_impl.cc (right):

https://codereview.chromium.org/2420743003/diff/300001/device/vr/vr_service_i...
device/vr/vr_service_impl.cc:35: displays_[device] =
std::unique_ptr<VRDisplayImpl>(display_impl);
Nit: use base::WrapUnique()

https://codereview.chromium.org/2420743003/diff/300001/device/vr/vr_service_i...
device/vr/vr_service_impl.cc:40: binding_.reset(new
mojo::Binding<mojom::VRService>(this, std::move(request)));
This should just be a mojo::Binding<mojom::VRServices>>: it can be closed and
rebound easily (this CL is pretty large so if you want to do this in a followup,
that's fine: just add a TODO)

https://codereview.chromium.org/2420743003/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/vr/VRController.cpp (right):

https://codereview.chromium.org/2420743003/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/vr/VRController.cpp:51: WTF::wrapUnique(new
VRGetDevicesCallback(resolver)));
Nit: no WTF:: prefix

https://codereview.chromium.org/2420743003/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right):

https://codereview.chromium.org/2420743003/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:141: if (!m_display)
Should !m_display be the first check in anything callable from JS? In things
like submitFrame(), we do a bunch of work before deciding that we can't do
anything since we're detached.

[clamy, 2016-11-04 15:35:56]

Thanks! content/ lgtm.

[shaobo.yan, 2016-11-05 08:11:39]

dcheng@ : Thx for reviewing and the faults you point out taught me a lot :)

Here are some updates and PTAL, thx!

https://codereview.chromium.org/2420743003/diff/300001/device/vr/android/gvr/...
File device/vr/android/gvr/gvr_device.cc (right):

https://codereview.chromium.org/2420743003/diff/300001/device/vr/android/gvr/...
device/vr/android/gvr/gvr_device.cc:235: if (!IsAccessAllowed(service)) {
On 2016/11/04 05:12:20, dcheng wrote:
> Should this be an early return?

Yes, and sorry for the fault.Updated to delete '!'

https://codereview.chromium.org/2420743003/diff/300001/device/vr/test/fake_vr...
File device/vr/test/fake_vr_device.h (right):

https://codereview.chromium.org/2420743003/diff/300001/device/vr/test/fake_vr...
device/vr/test/fake_vr_device.h:25: device_registered = !device_registered;
On 2016/11/04 05:12:21, dcheng wrote:
> Why does this mutate device_registered?

It means to reset device_registered status when calling this method.
It's an old method which will be updated by following CL.

https://codereview.chromium.org/2420743003/diff/300001/device/vr/test/fake_vr...
device/vr/test/fake_vr_device.h:46: unsigned updated;
On 2016/11/04 05:12:21, dcheng wrote:
> Nit: fields should end with _

thx and done.

https://codereview.chromium.org/2420743003/diff/300001/device/vr/vr_device_ma...
File device/vr/vr_device_manager.cc (right):

https://codereview.chromium.org/2420743003/diff/300001/device/vr/vr_device_ma...
device/vr/vr_device_manager.cc:100: if (!devices.size())
On 2016/11/04 05:12:21, dcheng wrote:
> Use empty()

done.

https://codereview.chromium.org/2420743003/diff/300001/device/vr/vr_device_ma...
File device/vr/vr_device_manager.h (right):

https://codereview.chromium.org/2420743003/diff/300001/device/vr/vr_device_ma...
device/vr/vr_device_manager.h:40: DEVICE_VR_EXPORT unsigned int GetDevicesNum();
On 2016/11/04 05:12:21, dcheng wrote:
> Nit: don't use abbreviations like Num. GetNumberOfDevices is fine. Otherwise,
> the method name reads ambiguously.

Thx for telling me this. And this method is for getting connected device number,
so I updated it to GetNumberOfConnectedDevices

https://codereview.chromium.org/2420743003/diff/300001/device/vr/vr_service.m...
File device/vr/vr_service.mojom (right):

https://codereview.chromium.org/2420743003/diff/300001/device/vr/vr_service.m...
device/vr/vr_service.mojom:68: SetClient(VRServiceClient client) => (uint32
num);
On 2016/11/04 05:12:21, dcheng wrote:
> In a followup, it'd be better if we had a factory function that took a
> VRServiceClient so we would never have a half-initialized VRService: as it
> stands, it's possible for the renderer to never set a client. The browser
never
> checks that client() is non-null.
> 
> Also, I'm not sure what |num| is: please make the name more descriptive.

num means connectedDeviceNumber I'll update it.
And I'll use a TODO to fix this.

https://codereview.chromium.org/2420743003/diff/300001/device/vr/vr_service.m...
device/vr/vr_service.mojom:76: SetClient(VRDisplayClient client);
On 2016/11/04 05:12:21, dcheng wrote:
> Ditto: it'd be better for this to be created through some sort of factory
> function, so that we're never in the half-initialized state. This one is easy
to
> fix: we can pass down a VRDisplayClient& in OnDisplayConnected().

Thx and I'll use TODO to follow this.

https://codereview.chromium.org/2420743003/diff/300001/device/vr/vr_service_i...
File device/vr/vr_service_impl.cc (right):

https://codereview.chromium.org/2420743003/diff/300001/device/vr/vr_service_i...
device/vr/vr_service_impl.cc:35: displays_[device] =
std::unique_ptr<VRDisplayImpl>(display_impl);
On 2016/11/04 05:12:21, dcheng wrote:
> Nit: use base::WrapUnique()

Thx for telling me that.Updated

https://codereview.chromium.org/2420743003/diff/300001/device/vr/vr_service_i...
device/vr/vr_service_impl.cc:40: binding_.reset(new
mojo::Binding<mojom::VRService>(this, std::move(request)));
On 2016/11/04 05:12:21, dcheng wrote:
> This should just be a mojo::Binding<mojom::VRServices>>: it can be closed and
> rebound easily (this CL is pretty large so if you want to do this in a
followup,
> that's fine: just add a TODO)

Does it means that we shouldn't abandon the mojo::Binding<mojom::VRServices> ?
TODO added.

https://codereview.chromium.org/2420743003/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right):

https://codereview.chromium.org/2420743003/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:141: if (!m_display)
On 2016/11/04 05:12:21, dcheng wrote:
> Should !m_display be the first check in anything callable from JS? In things
> like submitFrame(), we do a bunch of work before deciding that we can't do
> anything since we're detached.

Yes, thx for pointing out this. I've updated submitFrame() and
updateLayerBounds() function to reduce some useless work. The check condition
here and in requestPresent(), I think is ok.

[shaobo.yan, 2016-11-05 23:17:34]

ping dcheng@ PTAL

[shaobo.yan, 2016-11-07 00:53:15]

ping dcheng@ PTAL for this block issue

[shaobo.yan, 2016-11-07 14:06:35]

Sorry but ping dcheng@ PTAL :)

[bajones, 2016-11-07 21:48:46]

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

[bajones, 2016-11-07 21:48:48]

dcheng@ (or mkwst@, since he seems slow to respond), can you give this CL a
follow up SECURITY_OWNERS review?

[dcheng, 2016-11-07 22:08:25]

On 2016/11/07 21:48:48, bajones wrote:
> dcheng@ (or mkwst@, since he seems slow to respond), can you give this CL a
> follow up SECURITY_OWNERS review?

I'm located in Pacific time. I do sometimes review CLs over weekends, time
permitting, but that's not always possible. This CL is also somewhat non-trivial
to review, so I hope you can understand the delay. Thank you.

[shaobo.yan, 2016-11-08 01:25:13]

dcheng@ since I'm new to commit to chromium, one of my friends taught me some
code commit & review rules yesterday. And he points out I'd better not ping
reviewer in weekend (and consider the time zone). what's more I'd better just
reply done or comments below comments after I updated my patch. No need to reply
immediately. 
And sorry for my ping frequency because this patch blocks lots of other
peoples(like bajones@) follow up works for WebVR. Hope you understand and a nice
day :)

[dcheng, 2016-11-08 08:55:17]

lgtm

https://codereview.chromium.org/2420743003/diff/300001/device/vr/vr_service_i...
File device/vr/vr_service_impl.cc (right):

https://codereview.chromium.org/2420743003/diff/300001/device/vr/vr_service_i...
device/vr/vr_service_impl.cc:40: binding_.reset(new
mojo::Binding<mojom::VRService>(this, std::move(request)));
On 2016/11/05 08:11:39, shaobo.yan wrote:
> On 2016/11/04 05:12:21, dcheng wrote:
> > This should just be a mojo::Binding<mojom::VRServices>>: it can be closed
and
> > rebound easily (this CL is pretty large so if you want to do this in a
> followup,
> > that's fine: just add a TODO)
> 
> Does it means that we shouldn't abandon the mojo::Binding<mojom::VRServices> ?
> TODO added.

Actually, from what I can tell, we seem to create a new impl for each connection
request.

The standard way of doing this is just to pass the InterfaceRequest<> directly
to the VRServiceImpl constructor.

(Yes, this is fine to do an in followup)

https://codereview.chromium.org/2420743003/diff/320001/device/vr/android/gvr/...
File device/vr/android/gvr/gvr_device.cc (right):

https://codereview.chromium.org/2420743003/diff/320001/device/vr/android/gvr/...
device/vr/android/gvr/gvr_device.cc:208: presenting_service_ = service;
This isn't critical, but I personally found this a bit hard to follow: if we get
here, it's guaranteed that we're already the presenting service for this device
or there is no presenting service associated with this device.

However, the name "IsAccessAllowed(service)" doesn't really convey that concept,
but I'm not sure if I have a better idea. Maybe bajones@ will have some
thoughts.

https://codereview.chromium.org/2420743003/diff/320001/device/vr/android/gvr/...
device/vr/android/gvr/gvr_device.cc:235: if (IsAccessAllowed(service)) {
FWIW, the usual error handling style in Chromium is to early return:

if (!IsAccessAllowed(service) || !delegate_)
  return;

(GvrDevice could also use some comments describing lifetime; for example, why
are we allowed to set a non-null delegate? It seems like this code would be
simpler if |delegate_| were never null. Doing this in a followup is OK)

https://codereview.chromium.org/2420743003/diff/320001/device/vr/test/fake_vr...
File device/vr/test/fake_vr_device.h (right):

https://codereview.chromium.org/2420743003/diff/320001/device/vr/test/fake_vr...
device/vr/test/fake_vr_device.h:23: unsigned UpdatedNum() { return updated_; }
Nit: const

https://codereview.chromium.org/2420743003/diff/320001/device/vr/test/fake_vr...
device/vr/test/fake_vr_device.h:42: unsigned int updated_;
Nit: please name this field more descriptively. number_of_updated_devices_ is
what this means, I think?

Similar comment applies to the accessor.

https://codereview.chromium.org/2420743003/diff/320001/device/vr/vr_device.cc
File device/vr/vr_device.cc (right):

https://codereview.chromium.org/2420743003/diff/320001/device/vr/vr_device.cc...
device/vr/vr_device.cc:52: if (client)
If we always bind client (by changing the mojom to pass a VRDisplayClient&), it
shouldn't be necessary to null-check this: calling a disconnected client just
becomes a no-op.

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

https://codereview.chromium.org/2420743003/diff/320001/device/vr/vr_device.h#...
device/vr/vr_device.h:50: // TODO (Shaobo): Checks should be done against
VRDisplayImpl.
The convention for TODOs here and elsewhere should be:

TODO(shaobo.yan@intel.com): 

(Note there's no space between the paren and TODO)

https://codereview.chromium.org/2420743003/diff/320001/device/vr/vr_device.h#...
device/vr/vr_device.h:60: // only need
Nit: fix wrapping

https://codereview.chromium.org/2420743003/diff/320001/device/vr/vr_device_ma...
File device/vr/vr_device_manager.h (right):

https://codereview.chromium.org/2420743003/diff/320001/device/vr/vr_device_ma...
device/vr/vr_device_manager.h:40: DEVICE_VR_EXPORT unsigned int
GetNumberOfConnectedDevices();
Note that it might be easier to just export the entire class. (I don't feel
strongly about this though)

https://codereview.chromium.org/2420743003/diff/320001/device/vr/vr_service.m...
File device/vr/vr_service.mojom (right):

https://codereview.chromium.org/2420743003/diff/320001/device/vr/vr_service.m...
device/vr/vr_service.mojom:77: // TODO (Shaobo) : pass down a VRDisplayClient&
in OnDisplayConnected().
For this, can we just go ahead and address the TODO now?

From the C++ side, this gets passed down as an InterfaceRequest which the
receiving side is expected to bind to an impl:
https://www.chromium.org/developers/design-documents/mojo/mojom-syntax

https://codereview.chromium.org/2420743003/diff/320001/device/vr/vr_service_i...
File device/vr/vr_service_impl_unittest.cc (right):

https://codereview.chromium.org/2420743003/diff/320001/device/vr/vr_service_i...
device/vr/vr_service_impl_unittest.cc:23: void(mojom::VRDisplayClientPtr&
display_client));
I might be missing something, but I'm not seeing how this mock is used.

https://codereview.chromium.org/2420743003/diff/320001/device/vr/vr_service_i...
device/vr/vr_service_impl_unittest.cc:33: client_binding_.reset(new
mojo::Binding<mojom::VRServiceClient>(
Let's add a TODO to clean up this binding (this shouldn't need to be a
unique_ptr either).

It's also a bit unusual to have an external binding like this. I'd suggest just
not using gmock in this particular case.

https://codereview.chromium.org/2420743003/diff/320001/device/vr/vr_service_i...
device/vr/vr_service_impl_unittest.cc:44: void SetDeviceNum(unsigned int
device_num) { device_num_ = device_num; }
SetNumberOfDevices

number_of_devices_?

https://codereview.chromium.org/2420743003/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/vr/VRController.cpp (right):

https://codereview.chromium.org/2420743003/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/vr/VRController.cpp:51: wrapUnique(new
VRGetDevicesCallback(resolver)));
WTF just added makeUnique, so this can actually be written:

makeUnique<VRGetDevicesCallback>(resolver)

(Sorry for the churn)

https://codereview.chromium.org/2420743003/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/vr/VRController.cpp:73: void
VRController::onDisplaysSynced(unsigned num) {
Nit: please name |num| and |m_displaysSyncedNum| more descriptively

I'd suggest numberOfDevices and m_numberOfSyncedDevices or something along those
lines.

https://codereview.chromium.org/2420743003/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/vr/VRDisplay.h (right):

https://codereview.chromium.org/2420743003/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/vr/VRDisplay.h:70: m_display =
std::move(display);
I think this is unused.

[dcheng, 2016-11-08 09:00:24]

(Sorry, not lgtm yet: I hit the wrong button, I was typing up a reply in here)

On 2016/11/08 01:25:13, shaobo.yan wrote:
> dcheng@ since I'm new to commit to chromium, one of my friends taught me some
> code commit & review rules yesterday. And he points out I'd better not ping
> reviewer in weekend (and consider the time zone). what's more I'd better just
> reply done or comments below comments after I updated my patch. No need to
reply
> immediately. 
> And sorry for my ping frequency because this patch blocks lots of other
> peoples(like bajones@) follow up works for WebVR. Hope you understand and a
nice
> day :)

No worries, I sometimes check messages on weekends, but I didn't check most of
my labels this weekend (and Monday mornings are usually a bunch of catching up).

That being said, I'll prioritize any subsequent replies highly: I wasn't aware
that it was on the blocking path for other team members (as I thought this was
intended as a cleanup refactoring). If you have questions about Mojo, please
feel free to reach out on IRC via PM (or email me directly).

[shaobo.yan, 2016-11-08 11:33:44]

dcheng@ All the comments done (except wrapUnique -> makeUnique, it seems I need
to rebase to get the latest updated)

https://codereview.chromium.org/2420743003/diff/320001/device/vr/android/gvr/...
File device/vr/android/gvr/gvr_device.cc (right):

https://codereview.chromium.org/2420743003/diff/320001/device/vr/android/gvr/...
device/vr/android/gvr/gvr_device.cc:208: presenting_service_ = service;
On 2016/11/08 08:55:16, dcheng wrote:
> This isn't critical, but I personally found this a bit hard to follow: if we
get
> here, it's guaranteed that we're already the presenting service for this
device
> or there is no presenting service associated with this device.
> 
> However, the name "IsAccessAllowed(service)" doesn't really convey that
concept,
> but I'm not sure if I have a better idea. Maybe bajones@ will have some
> thoughts.

Yes, it means that if the presenting_service_ remains nullptr or it is the
service which request access that the access request would be approved.

https://codereview.chromium.org/2420743003/diff/320001/device/vr/android/gvr/...
device/vr/android/gvr/gvr_device.cc:235: if (IsAccessAllowed(service)) {
On 2016/11/08 08:55:16, dcheng wrote:
> FWIW, the usual error handling style in Chromium is to early return:
> 
> if (!IsAccessAllowed(service) || !delegate_)
>   return;
> 
> (GvrDevice could also use some comments describing lifetime; for example, why
> are we allowed to set a non-null delegate? It seems like this code would be
> simpler if |delegate_| were never null. Doing this in a followup is OK)

I could explain about delegate_ null check. Since the delegate_ is related to a
vr_shell, and until the vr_shell related class call setDelgate, that the
delegate_ would be a non-null object.

https://codereview.chromium.org/2420743003/diff/320001/device/vr/vr_device_ma...
File device/vr/vr_device_manager.h (right):

https://codereview.chromium.org/2420743003/diff/320001/device/vr/vr_device_ma...
device/vr/vr_device_manager.h:40: DEVICE_VR_EXPORT unsigned int
GetNumberOfConnectedDevices();
On 2016/11/08 08:55:16, dcheng wrote:
> Note that it might be easier to just export the entire class. (I don't feel
> strongly about this though)

Yes but I'd like to keep the original design :) for there may add some
NON-EXPORT function in future :)

https://codereview.chromium.org/2420743003/diff/320001/device/vr/vr_service_i...
File device/vr/vr_service_impl_unittest.cc (right):

https://codereview.chromium.org/2420743003/diff/320001/device/vr/vr_service_i...
device/vr/vr_service_impl_unittest.cc:33: client_binding_.reset(new
mojo::Binding<mojom::VRServiceClient>(
On 2016/11/08 08:55:16, dcheng wrote:
> Let's add a TODO to clean up this binding (this shouldn't need to be a
> unique_ptr either).
> 
> It's also a bit unusual to have an external binding like this. I'd suggest
just
> not using gmock in this particular case.

Yes, and I'm preparing a patch to update a whole unittest. So I added a TODO for
this file.

[shaobo.yan, 2016-11-08 11:34:23]

thx for reviewing dcheng@ and Please take a look at the latest update :)

[dcheng, 2016-11-08 18:15:44]

Thanks! Looks pretty good, my only remaining question is around |delegate_|.

https://codereview.chromium.org/2420743003/diff/320001/device/vr/android/gvr/...
File device/vr/android/gvr/gvr_device.cc (right):

https://codereview.chromium.org/2420743003/diff/320001/device/vr/android/gvr/...
device/vr/android/gvr/gvr_device.cc:208: presenting_service_ = service;
On 2016/11/08 11:33:44, shaobo.yan wrote:
> On 2016/11/08 08:55:16, dcheng wrote:
> > This isn't critical, but I personally found this a bit hard to follow: if we
> get
> > here, it's guaranteed that we're already the presenting service for this
> device
> > or there is no presenting service associated with this device.
> > 
> > However, the name "IsAccessAllowed(service)" doesn't really convey that
> concept,
> > but I'm not sure if I have a better idea. Maybe bajones@ will have some
> > thoughts.
> 
> Yes, it means that if the presenting_service_ remains nullptr or it is the
> service which request access that the access request would be approved.

Right, I'm hoping we can rename this function (in a followup) to make this
clearer: otherwise, a reader like me needs to examine two functions to
understand that the presenting_service_ == service invariant will hold after
208.

https://codereview.chromium.org/2420743003/diff/320001/device/vr/android/gvr/...
device/vr/android/gvr/gvr_device.cc:235: if (IsAccessAllowed(service)) {
On 2016/11/08 11:33:44, shaobo.yan wrote:
> On 2016/11/08 08:55:16, dcheng wrote:
> > FWIW, the usual error handling style in Chromium is to early return:
> > 
> > if (!IsAccessAllowed(service) || !delegate_)
> >   return;
> > 
> > (GvrDevice could also use some comments describing lifetime; for example,
why
> > are we allowed to set a non-null delegate? It seems like this code would be
> > simpler if |delegate_| were never null. Doing this in a followup is OK)
> 
> I could explain about delegate_ null check. Since the delegate_ is related to
a
> vr_shell, and until the vr_shell related class call setDelgate, that the
> delegate_ would be a non-null object.

Sorry, I think I still don't understand the class design.

The constructor takes a Delegate* argument. Shouldn't this  just always be set
to some non-null value? Why would we allow null delegates at all?

https://codereview.chromium.org/2420743003/diff/340001/device/vr/vr_display_i...
File device/vr/vr_display_impl.cc (right):

https://codereview.chromium.org/2420743003/diff/340001/device/vr/vr_display_i...
device/vr/vr_display_impl.cc:16: service_.reset(service);
Nit: please initialize |device_| and |service_| in the initializer list.

[shaobo.yan, 2016-11-09 01:11:20]

last comments done with others replys , thx for your review dcheng@

https://codereview.chromium.org/2420743003/diff/320001/device/vr/android/gvr/...
File device/vr/android/gvr/gvr_device.cc (right):

https://codereview.chromium.org/2420743003/diff/320001/device/vr/android/gvr/...
device/vr/android/gvr/gvr_device.cc:208: presenting_service_ = service;
On 2016/11/08 18:15:44, dcheng wrote:
> On 2016/11/08 11:33:44, shaobo.yan wrote:
> > On 2016/11/08 08:55:16, dcheng wrote:
> > > This isn't critical, but I personally found this a bit hard to follow: if
we
> > get
> > > here, it's guaranteed that we're already the presenting service for this
> > device
> > > or there is no presenting service associated with this device.
> > > 
> > > However, the name "IsAccessAllowed(service)" doesn't really convey that
> > concept,
> > > but I'm not sure if I have a better idea. Maybe bajones@ will have some
> > > thoughts.
> > 
> > Yes, it means that if the presenting_service_ remains nullptr or it is the
> > service which request access that the access request would be approved.
> 
> Right, I'm hoping we can rename this function (in a followup) to make this
> clearer: otherwise, a reader like me needs to examine two functions to
> understand that the presenting_service_ == service invariant will hold after
> 208.

Ok, it seems that give a property name is what I really need to improve :). I'll
update it in a followup and give it a TODO here for not block this issue.

https://codereview.chromium.org/2420743003/diff/320001/device/vr/android/gvr/...
device/vr/android/gvr/gvr_device.cc:235: if (IsAccessAllowed(service)) {
On 2016/11/08 18:15:43, dcheng wrote:
> On 2016/11/08 11:33:44, shaobo.yan wrote:
> > On 2016/11/08 08:55:16, dcheng wrote:
> > > FWIW, the usual error handling style in Chromium is to early return:
> > > 
> > > if (!IsAccessAllowed(service) || !delegate_)
> > >   return;
> > > 
> > > (GvrDevice could also use some comments describing lifetime; for example,
> why
> > > are we allowed to set a non-null delegate? It seems like this code would
be
> > > simpler if |delegate_| were never null. Doing this in a followup is OK)
> > 
> > I could explain about delegate_ null check. Since the delegate_ is related
to
> a
> > vr_shell, and until the vr_shell related class call setDelgate, that the
> > delegate_ would be a non-null object.
> 
> Sorry, I think I still don't understand the class design.
> 
> The constructor takes a Delegate* argument. Shouldn't this  just always be set
> to some non-null value? Why would we allow null delegates at all?

Because in gvr implementation vr_shell is the truly backend which GvrDevice
account on. But vr_shell and gvr_device have different life-cycle. I think
bajones@ could give you some answer.

[shaobo.yan, 2016-11-10 00:40:26]

dcheng@ ping :)
it seems this issue also blocks mthiesse@ work....

[dcheng, 2016-11-10 00:41:33]

On 2016/11/10 00:40:26, shaobo.yan wrote:
> dcheng@ ping :)
> it seems this issue also blocks mthiesse@ work....

I'm waiting for clarification from bajones@ on the delegate question.

[shaobo.yan, 2016-11-10 00:57:01]

bajones@ here is the context of dcheng@ question,  paste them here :
> > (GvrDevice could also use some comments describing lifetime; for example,
why
> > are we allowed to set a non-null delegate? It seems like this code would be
> > simpler if |delegate_| were never null. Doing this in a followup is OK)
> 
> I could explain about delegate_ null check. Since the delegate_ is related to
a
> vr_shell, and until the vr_shell related class call setDelgate, that the
> delegate_ would be a non-null object.

Sorry, I think I still don't understand the class design.

The constructor takes a Delegate* argument. Shouldn't this  just always be set
to some non-null value? Why would we allow null delegates at all?

[bajones, 2016-11-10 01:05:14]

On 2016/11/10 00:57:01, shaobo.yan wrote:
> bajones@ here is the context of dcheng@ question,  paste them here :
> > > (GvrDevice could also use some comments describing lifetime; for example,
> why
> > > are we allowed to set a non-null delegate? It seems like this code would
be
> > > simpler if |delegate_| were never null. Doing this in a followup is OK)
> > 
> > I could explain about delegate_ null check. Since the delegate_ is related
to
> a
> > vr_shell, and until the vr_shell related class call setDelgate, that the
> > delegate_ would be a non-null object.
> 
> Sorry, I think I still don't understand the class design.
> 
> The constructor takes a Delegate* argument. Shouldn't this  just always be set
> to some non-null value? Why would we allow null delegates at all?

Sorry for the slow reply.

The delegate is an object which lifetime is managed elsewhere and may in fact be
multiple different objects over the lifetime of the GVRDevice. There may be
occasions where the delegate is removed out from under us and we won't have a
new one right away to fill the void.

Agreed that this is generally confusing, and I definitely want to continue
working on the code clarity and simplicity of not only that aspect but several
other related areas. It'll be easiest to reason about those related refactorings
once this one is done, though.

[dcheng, 2016-11-10 06:21:58]

LGTM

Thanks for cleaning this up, it looks really nice overall, and will be a good
base for building upon!

https://codereview.chromium.org/2420743003/diff/380001/device/vr/vr_service.m...
File device/vr/vr_service.mojom (right):

https://codereview.chromium.org/2420743003/diff/380001/device/vr/vr_service.m...
device/vr/vr_service.mojom:76: interface VRDisplay {
Btw, I know this class didn't have existing comments before, but it would be
nice to add some (to document things like where interfaces live and what they're
used for). A followup is fine.

https://codereview.chromium.org/2420743003/diff/380001/device/vr/vr_service.m...
device/vr/vr_service.mojom:77: 
Nit: no newline

https://codereview.chromium.org/2420743003/diff/380001/device/vr/vr_service_i...
File device/vr/vr_service_impl_unittest.cc (right):

https://codereview.chromium.org/2420743003/diff/380001/device/vr/vr_service_i...
device/vr/vr_service_impl_unittest.cc:41: void SetNumberOfDevices(unsigned int
number_of_devices) {
Nit: usually, simple inlined setters are named with underscores, i.e.
set_number_of_devices (sorry, I might have incorrectly stated this in a previous
reply)

https://codereview.chromium.org/2420743003/diff/380001/device/vr/vr_service_i...
device/vr/vr_service_impl_unittest.cc:81:
std::unique_ptr<VRServiceTestBinding>(new VRServiceTestBinding());
Nit: auto test_binding = base::MakeUnique<...>(...);

[shaobo.yan, 2016-11-10 07:01:30]

The CQ bit was checked by shaobo.yan@intel.com

[shaobo.yan, 2016-11-10 07:01:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
bajones@chromium.org, haraken@chromium.org, clamy@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2420743003/#ps400001
(title: "rebase and address dcheng@ comments")

[commit-bot: I haz the power, 2016-11-10 07:01:55]

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, 2016-11-10 08:28:17]

Description was changed from

==========
mojo VR interface simplified

Since mojo interface prefer API without index transfer, current
implementation should be modified.

According to dcheng@ and bajones@ comments, WebVR implementation should
modified to let display/frame communicate directly so that index
parameters are no longer need.

This patch is based on the strategy above and will have three stages:
1(done). Modify event handler part;
2(current). Modify VRService Interface;
3. Modify unittest part;

BUG=652080
R=leon.han@intel.com,dcheng@chromium.org,bshe@chromium.org,bajones@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
mojo VR interface simplified

Since mojo interface prefer API without index transfer, current
implementation should be modified.

According to dcheng@ and bajones@ comments, WebVR implementation should
modified to let display/frame communicate directly so that index
parameters are no longer need.

This patch is based on the strategy above and will have three stages:
1(done). Modify event handler part;
2(current). Modify VRService Interface;
3. Modify unittest part;

BUG=652080
R=leon.han@intel.com,dcheng@chromium.org,bshe@chromium.org,bajones@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-11-10 08:28:20]

Committed patchset #21 (id:400001)

[commit-bot: I haz the power, 2016-11-10 08:30:13]

Description was changed from

==========
mojo VR interface simplified

Since mojo interface prefer API without index transfer, current
implementation should be modified.

According to dcheng@ and bajones@ comments, WebVR implementation should
modified to let display/frame communicate directly so that index
parameters are no longer need.

This patch is based on the strategy above and will have three stages:
1(done). Modify event handler part;
2(current). Modify VRService Interface;
3. Modify unittest part;

BUG=652080
R=leon.han@intel.com,dcheng@chromium.org,bshe@chromium.org,bajones@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
mojo VR interface simplified

Since mojo interface prefer API without index transfer, current
implementation should be modified.

According to dcheng@ and bajones@ comments, WebVR implementation should
modified to let display/frame communicate directly so that index
parameters are no longer need.

This patch is based on the strategy above and will have three stages:
1(done). Modify event handler part;
2(current). Modify VRService Interface;
3. Modify unittest part;

BUG=652080
R=leon.han@intel.com,dcheng@chromium.org,bshe@chromium.org,bajones@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/91d0e2e835b7e000e432f3df690ed37a36cde1ec
Cr-Commit-Position: refs/heads/master@{#431210}
==========

[commit-bot: I haz the power, 2016-11-10 08:30:15]

Patchset 21 (id:??) landed as
https://crrev.com/91d0e2e835b7e000e432f3df690ed37a36cde1ec
Cr-Commit-Position: refs/heads/master@{#431210}

[shaobo.yan, 2016-11-10 08:32:49]

thx for bajones@, haraken@, dcheng@ and clamy@ review this patch. And sorry for
this patch blocks for a long time.

[vabr (Chromium), 2016-11-10 11:21:18]

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

[vabr (Chromium), 2016-11-10 11:21:20]

This broke the Win x64 build:

https://build.chromium.org/p/chromium/builders/Win%20x64/builds/5867/steps/co...

I will revert now.

Today's sheriff

https://codereview.chromium.org/2420743003/diff/400001/device/vr/vr_device_ma...
File device/vr/vr_device_manager.cc (right):

https://codereview.chromium.org/2420743003/diff/400001/device/vr/vr_device_ma...
device/vr/vr_device_manager.cc:119: return devices_.size();
This is breaking the Win x64 build, with the following message:

FAILED: obj/device/vr/vr/vr_device_manager.obj 
ninja -t msvc -e environment.x64 -- C:\b\c\cipd\goma/gomacc.exe
"C:\b\depot_tools\win_toolchain\vs_files\d5dc33b15d1b2c086f2f6632e2fd15882f80dbd3\VC\bin\amd64/cl.exe"
/nologo /showIncludes /FC @obj/device/vr/vr/vr_device_manager.obj.rsp /c
../../device/vr/vr_device_manager.cc /Foobj/device/vr/vr/vr_device_manager.obj
/Fd"obj/device/vr/vr_cc.pdb"
c:\b\c\b\win_x64_archive\src\device\vr\vr_device_manager.cc(119): error C2220:
warning treated as error - no 'object' file generated
c:\b\c\b\win_x64_archive\src\device\vr\vr_device_manager.cc(119): warning C4267:
'return': conversion from 'size_t' to 'unsigned int', possible loss of data

[vabr (Chromium), 2016-11-10 11:21:47]

A revert of this CL (patchset #21 id:400001) has been created in
https://codereview.chromium.org/2488273002/ by vabr@chromium.org.

The reason for reverting is: This broke
https://build.chromium.org/p/chromium/builders/Win%20x64/builds/5867/steps/co....

[shaobo.yan, 2016-11-10 15:37:00]

On 2016/11/10 11:21:47, vabr (Chromium) wrote:
> A revert of this CL (patchset #21 id:400001) has been created in
> https://codereview.chromium.org/2488273002/ by mailto:vabr@chromium.org.
> 
> The reason for reverting is: This broke
>
https://build.chromium.org/p/chromium/builders/Win%20x64/builds/5867/steps/co....

Hi, vabr@
it seems that my build flag isn't strict as the bots(postcommit buildbots I
guess). Could you offer the flags which I could use to build on linux platform?

BR,
Yan,Shaobo

[vabr (Chromium), 2016-11-10 15:42:07]

On 2016/11/10 15:37:00, shaobo.yan wrote:
> On 2016/11/10 11:21:47, vabr (Chromium) wrote:
> > A revert of this CL (patchset #21 id:400001) has been created in
> > https://codereview.chromium.org/2488273002/ by mailto:vabr@chromium.org.
> > 
> > The reason for reverting is: This broke
> >
>
https://build.chromium.org/p/chromium/builders/Win%20x64/builds/5867/steps/co....
> 
> Hi, vabr@
> it seems that my build flag isn't strict as the bots(postcommit buildbots I
> guess). Could you offer the flags which I could use to build on linux
platform?
> 
> BR,
> Yan,Shaobo

Hi Yan,Shaobo,

I'm afraid I don know. My expectation would be that the trybots would have
caught this, but apparently they did not. Asking on chromium-dev might be a good
idea.

Cheers,
Vaclav