Refine VRDevice Class

Refine VRDevice Class

Since VRDisplayImpl knows its own VRService, VRDevice could only
contains VRDisplayImpl. This CL is a follow up for TODOs in VRDevice.

BUG=389343
R=bajones@chromium.org

Committed: https://crrev.com/1c602e5dd72a6acd31dd4f76eb606b54db549b11
Cr-Commit-Position: refs/heads/master@{#436531}

[shaobo.yan, 2016-11-29 06:50:49]

shaobo.yan@intel.com changed reviewers:
+ bajones; mthiesse@chromium.org
- bajones@chromium.org

[shaobo.yan, 2016-11-29 06:50:50]

PTAL.

[shaobo.yan, 2016-11-29 06:51:08]

Description was changed from

==========
Refine VRDevice Class

Since VRDisplayImpl knows its own VRService, VRDevice could only
contains VRDisplayImpl. This CL is a follow up for TODOs in VRDevice.

BUG=389343
R=bajones@chromium.org
==========

to

==========
Refine VRDevice Class

Since VRDisplayImpl knows its own VRService, VRDevice could only
contains VRDisplayImpl. This CL is a follow up for TODOs in VRDevice.

BUG=389343
R=bajones@chromium.org
==========

[shaobo.yan, 2016-11-29 06:51:08]

shaobo.yan@intel.com changed reviewers:
+ bajones; mthiesse@chromium.org;
- bajones; mthiesse@chromium.org

[shaobo.yan, 2016-11-29 06:51:41]

shaobo.yan@intel.com changed reviewers:
+ bajones@chromium.org, mthiesse@chromium.org
- bajones; mthiesse@chromium.org;

[mthiesse, 2016-11-29 16:35:13]

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

https://codereview.chromium.org/2537723002/diff/1/device/vr/vr_device.cc#newc...
device/vr/vr_device.cc:34: displays_.erase(std::remove(displays_.begin(),
displays_.end(), display),
Why is this necessary? Do we expect the same display to be added multiple times?

Perhaps we should be using a set rather than a vector?

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

https://codereview.chromium.org/2537723002/diff/1/device/vr/vr_device_manager...
device/vr/vr_device_manager.cc:82:
services_.erase(std::remove(services_.begin(), services_.end(), service),
Perhaps services_ should be a set too? We don't expect them to be added multiple
times, do we?

[shaobo.yan, 2016-11-30 00:32:54]

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

https://codereview.chromium.org/2537723002/diff/1/device/vr/vr_device_manager...
device/vr/vr_device_manager.cc:82:
services_.erase(std::remove(services_.begin(), services_.end(), service),
On 2016/11/29 16:35:13, mthiesse wrote:
> Perhaps services_ should be a set too? We don't expect them to be added
multiple
> times, do we?

Yes, I think set could be a better solution.

[shaobo.yan, 2016-11-30 02:54:43]

PTAL.

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

https://codereview.chromium.org/2537723002/diff/1/device/vr/vr_device.cc#newc...
device/vr/vr_device.cc:34: displays_.erase(std::remove(displays_.begin(),
displays_.end(), display),
On 2016/11/29 16:35:13, mthiesse wrote:
> Why is this necessary? Do we expect the same display to be added multiple
times?
> 
> Perhaps we should be using a set rather than a vector?

Done.

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

https://codereview.chromium.org/2537723002/diff/1/device/vr/vr_device_manager...
device/vr/vr_device_manager.cc:82:
services_.erase(std::remove(services_.begin(), services_.end(), service),
On 2016/11/29 16:35:13, mthiesse wrote:
> Perhaps services_ should be a set too? We don't expect them to be added
multiple
> times, do we?

Done.

[mthiesse, 2016-11-30 13:59:04]

lgtm % nits

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

https://codereview.chromium.org/2537723002/diff/20001/device/vr/vr_device.cc#...
device/vr/vr_device.cc:55: DisplaySet::iterator it =
displays_.find(presenting_display_);
nit: 'auto it = displays_.find(presenting_display_);' works if
std::set<VRDisplayImpl*>::iterator is too long.

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

https://codereview.chromium.org/2537723002/diff/20001/device/vr/vr_device.h#n...
device/vr/vr_device.h:65: using DisplaySet = std::set<VRDisplayImpl*>;
nit: Not sure if this using is useful or improves readability anymore, you might
consider just removing it.

https://codereview.chromium.org/2537723002/diff/20001/device/vr/vr_device_man...
File device/vr/vr_device_manager.h (right):

https://codereview.chromium.org/2537723002/diff/20001/device/vr/vr_device_man...
device/vr/vr_device_manager.h:75: using ServiceSet = std::set<VRServiceImpl*>;
nit: type alias is also pretty useless here, might be better to just remove it.

[bajones, 2016-11-30 17:41:22]

LGTM as well, with one additional nit.

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

https://codereview.chromium.org/2537723002/diff/20001/device/vr/vr_device.h#n...
device/vr/vr_device.h:64: // Each Service have one VRDisplay with one VRDevice.
This comment should be updated or removed. It's accurate, but no longer makes
sense in context.

[shaobo.yan, 2016-12-02 01:44:26]

Sorry for bothering under the issue. 
bajones@ and mthiesse@, I find that it need to enter a credit card info during
the DayDream setup steps. Since most of our credit card are UnionPays and it
seems that there is no location could be chosen in China, so as a developer, is
there any known method to skip this step?

[bajones, 2016-12-02 02:05:23]

On 2016/12/02 01:44:26, shaobo.yan wrote:
> Sorry for bothering under the issue. 
> bajones@ and mthiesse@, I find that it need to enter a credit card info during
> the DayDream setup steps. Since most of our credit card are UnionPays and it
> seems that there is no location could be chosen in China, so as a developer,
is
> there any known method to skip this step?

I don't know about a method for skipping that step. I believe that you could use
a Google Play gift card in place of a credit card.

[shaobo.yan, 2016-12-02 02:08:24]

On 2016/12/02 02:05:23, bajones wrote:
> On 2016/12/02 01:44:26, shaobo.yan wrote:
> > Sorry for bothering under the issue. 
> > bajones@ and mthiesse@, I find that it need to enter a credit card info
during
> > the DayDream setup steps. Since most of our credit card are UnionPays and it
> > seems that there is no location could be chosen in China, so as a developer,
> is
> > there any known method to skip this step?
> 
> I don't know about a method for skipping that step. I believe that you could
use
> a Google Play gift card in place of a credit card.

Oh, thx for the info! I'll try the google play gift card !

[mthiesse, 2016-12-02 15:25:45]

On 2016/12/02 02:08:24, shaobo.yan wrote:
> On 2016/12/02 02:05:23, bajones wrote:
> > On 2016/12/02 01:44:26, shaobo.yan wrote:
> > > Sorry for bothering under the issue. 
> > > bajones@ and mthiesse@, I find that it need to enter a credit card info
> during
> > > the DayDream setup steps. Since most of our credit card are UnionPays and
it
> > > seems that there is no location could be chosen in China, so as a
developer,
> > is
> > > there any known method to skip this step?
> > 
> > I don't know about a method for skipping that step. I believe that you could
> use
> > a Google Play gift card in place of a credit card.
> 
> Oh, thx for the info! I'll try the google play gift card !

Alternatively, if you have a rooted phone, you can override the daydream
settings and skip initial setup.
$adb shell
#cat /data/data/com.google.vr.vrcore/shared_prefs/VrCoreSettings.xml | grep
DaydreamSetupComplete

You'll want to set that line to <boolean name="DaydreamSetupComplete"
value="true" />

[shaobo.yan, 2016-12-06 00:12:17]

On 2016/12/02 15:25:45, mthiesse wrote:
> On 2016/12/02 02:08:24, shaobo.yan wrote:
> > On 2016/12/02 02:05:23, bajones wrote:
> > > On 2016/12/02 01:44:26, shaobo.yan wrote:
> > > > Sorry for bothering under the issue. 
> > > > bajones@ and mthiesse@, I find that it need to enter a credit card info
> > during
> > > > the DayDream setup steps. Since most of our credit card are UnionPays
and
> it
> > > > seems that there is no location could be chosen in China, so as a
> developer,
> > > is
> > > > there any known method to skip this step?
> > > 
> > > I don't know about a method for skipping that step. I believe that you
could
> > use
> > > a Google Play gift card in place of a credit card.
> > 
> > Oh, thx for the info! I'll try the google play gift card !
> 
> Alternatively, if you have a rooted phone, you can override the daydream
> settings and skip initial setup.
> $adb shell
> #cat /data/data/com.google.vr.vrcore/shared_prefs/VrCoreSettings.xml | grep
> DaydreamSetupComplete
> 
> You'll want to set that line to <boolean name="DaydreamSetupComplete"
> value="true" />

thx for this, too !

[shaobo.yan, 2016-12-06 00:55:57]

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

[shaobo.yan, 2016-12-06 00:55:57]

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

[commit-bot: I haz the power, 2016-12-06 00:56:26]

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-12-06 00:59:31]

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

[commit-bot: I haz the power, 2016-12-06 00:59:35]

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

[shaobo.yan, 2016-12-06 04:16:06]

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

[shaobo.yan, 2016-12-06 04:16:07]

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

[commit-bot: I haz the power, 2016-12-06 04:16:25]

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-12-06 05:06:17]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1480997766611950,
"parent_rev": "a2e74a6a07a3a50d79be1ff6b4900aa82184955d", "commit_rev":
"d0ed5b3cdf1de935d9bcae3b73cfc1294f7ab463"}

[commit-bot: I haz the power, 2016-12-06 05:07:17]

Description was changed from

==========
Refine VRDevice Class

Since VRDisplayImpl knows its own VRService, VRDevice could only
contains VRDisplayImpl. This CL is a follow up for TODOs in VRDevice.

BUG=389343
R=bajones@chromium.org
==========

to

==========
Refine VRDevice Class

Since VRDisplayImpl knows its own VRService, VRDevice could only
contains VRDisplayImpl. This CL is a follow up for TODOs in VRDevice.

BUG=389343
R=bajones@chromium.org
==========

[commit-bot: I haz the power, 2016-12-06 05:07:19]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-12-06 05:11:28]

Description was changed from

==========
Refine VRDevice Class

Since VRDisplayImpl knows its own VRService, VRDevice could only
contains VRDisplayImpl. This CL is a follow up for TODOs in VRDevice.

BUG=389343
R=bajones@chromium.org
==========

to

==========
Refine VRDevice Class

Since VRDisplayImpl knows its own VRService, VRDevice could only
contains VRDisplayImpl. This CL is a follow up for TODOs in VRDevice.

BUG=389343
R=bajones@chromium.org

Committed: https://crrev.com/1c602e5dd72a6acd31dd4f76eb606b54db549b11
Cr-Commit-Position: refs/heads/master@{#436531}
==========

[commit-bot: I haz the power, 2016-12-06 05:11:29]

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