Moving VR service from /content/browser to /device

Moving VR service from /content/browser to /device

BUG=612342
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/98f9652e82de64155e08f422017fede4e051af51
Cr-Commit-Position: refs/heads/master@{#397828}

[bajones, 2016-06-01 18:59:22]

Description was changed from

==========
Moving VR service from /content/browser to /device

BUG=612342
==========

to

==========
Moving VR service from /content/browser to /device

BUG=612342
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[bajones, 2016-06-01 19:09:13]

bajones@chromium.org changed reviewers:
+ jam@chromium.org, jochen@chromium.org, reillyg@chromium.org

[bajones, 2016-06-01 19:09:14]

jochen@: Just need a rubber stamp for the removals from content/

reillyg@: Please take a look at a the additions to device/ migrated from
content/

[Reilly Grant (use Gerrit), 2016-06-01 21:42:24]

https://codereview.chromium.org/2029703002/diff/1/device/vr/BUILD.gn
File device/vr/BUILD.gn (right):

https://codereview.chromium.org/2029703002/diff/1/device/vr/BUILD.gn#newcode49
device/vr/BUILD.gn:49: sources = common_sources
This should just depend on :vr. Otherwise you are linking tests against two
copies of common_sources.

[bajones, 2016-06-02 21:17:57]

On 2016/06/01 21:42:24, Reilly Grant wrote:
> https://codereview.chromium.org/2029703002/diff/1/device/vr/BUILD.gn#newcode49
> device/vr/BUILD.gn:49: sources = common_sources
> This should just depend on :vr. Otherwise you are linking tests against two
> copies of common_sources.

Ah, I had tried that previously and it failed. Turns out I wasn't being liberal
enough with my EXPORT macros. Fixed.

[Reilly Grant (use Gerrit), 2016-06-02 21:30:09]

lgtm

[jochen (gone - plz use gerrit), 2016-06-03 12:46:12]

lgtm

[bajones, 2016-06-03 15:36:50]

The CQ bit was checked by bajones@chromium.org

[commit-bot: I haz the power, 2016-06-03 15:37:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2029703002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2029703002/20001

[commit-bot: I haz the power, 2016-06-03 15:40:59]

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

[commit-bot: I haz the power, 2016-06-03 15:41:00]

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

[bajones, 2016-06-03 17:18:48]

Hm... not sure what the right pattern is to fix the bot issues. It's flagging me
for an include (device/vr/vr_device_manager.h) that doesn't have an associated
gn dependency. Sounds like a good thing to catch! Unfortunately in this case
both the dependency and the file include are gated on webvr being enabled for
the build configuration. The include is only included if "ENABLE_WEBVR" is
defined, and the dependency is only included if "enable_webvr" is set, which
happens to be what defines ENABLE_WEBVR in build/config/BUILD.gn. So it appears
that the dependency check in this case is ignoring the "#if
defined(ENABLE_WEBVR)", which may be WAI.

Is there a better pattern to use here for includes for a component that are
should only be included conditionally?

[Reilly Grant (use Gerrit), 2016-06-03 17:44:47]

On 2016/06/03 at 17:18:48, bajones wrote:
> Hm... not sure what the right pattern is to fix the bot issues. It's flagging
me for an include (device/vr/vr_device_manager.h) that doesn't have an
associated gn dependency. Sounds like a good thing to catch! Unfortunately in
this case both the dependency and the file include are gated on webvr being
enabled for the build configuration. The include is only included if
"ENABLE_WEBVR" is defined, and the dependency is only included if "enable_webvr"
is set, which happens to be what defines ENABLE_WEBVR in build/config/BUILD.gn.
So it appears that the dependency check in this case is ignoring the "#if
defined(ENABLE_WEBVR)", which may be WAI.
> 
> Is there a better pattern to use here for includes for a component that are
should only be included conditionally?

Just add // nogncheck to the conditional include.

[bajones, 2016-06-03 18:08:28]

On 2016/06/03 17:44:47, Reilly Grant wrote:
> Just add // nogncheck to the conditional include.

That's pretty simple. Thank you!

[bajones, 2016-06-03 18:08:32]

The CQ bit was checked by bajones@chromium.org

[bajones, 2016-06-03 18:08:33]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
reillyg@chromium.org
Link to the patchset: https://codereview.chromium.org/2029703002/#ps40001
(title: "Added nogncheck to failing include")

[commit-bot: I haz the power, 2016-06-03 18:09:05]

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

[commit-bot: I haz the power, 2016-06-03 18:18:44]

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

[commit-bot: I haz the power, 2016-06-03 18:18:45]

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

[bajones, 2016-06-03 18:23:25]

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

[bajones, 2016-06-03 18:23:26]

+danakj@ to OK inclusion of dependency on /ui/gfx from /device/vr (Used
primarily for gfx::DecomposeTransform to derive a quaternion+position from a
matrix.)

[danakj, 2016-06-03 21:35:09]

LGTM

[bajones, 2016-06-03 22:37:44]

The CQ bit was checked by bajones@chromium.org

[commit-bot: I haz the power, 2016-06-03 22:38:37]

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

[commit-bot: I haz the power, 2016-06-03 22:44:14]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-06-03 22:45:56]

Description was changed from

==========
Moving VR service from /content/browser to /device

BUG=612342
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Moving VR service from /content/browser to /device

BUG=612342
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/98f9652e82de64155e08f422017fede4e051af51
Cr-Commit-Position: refs/heads/master@{#397828}
==========

[commit-bot: I haz the power, 2016-06-03 22:45:58]

Patchset 3 (id:??) landed as
https://crrev.com/98f9652e82de64155e08f422017fede4e051af51
Cr-Commit-Position: refs/heads/master@{#397828}