Migrate WebVR Cardboard implementation to GVR

Migrate WebVR Cardboard implementation to GVR

BUG=389343
Committed: https://crrev.com/267d5f909f25543d2351cc506626b5d5c08461c3
Committed: https://crrev.com/7ab52ec35d497d6bb96c6a1fa36c9b123bc72330
Cr-Original-Commit-Position: refs/heads/master@{#412710}
Cr-Commit-Position: refs/heads/master@{#413217}

[bajones, 2016-08-08 18:43:50]

bajones@chromium.org changed reviewers:
+ dtrainor@chromium.org, girard@chromium.org

[bajones, 2016-08-08 18:43:50]

dtrainor@: Could you review the change to chrome/android/BUILD.gn?

[bajones, 2016-08-08 18:49:29]

bajones@chromium.org changed reviewers:
+ bshe@chromium.org, klausw@google.com, mthiesse@chromium.org

[bajones, 2016-08-08 18:49:30]

bshe@, mthiesse@, klausw@: Could you take a look at the changes to device/vr? I
know you're not owners, but you collectively have more experience with GVR now
than anyone else on the Chrome team. :)

A quick note: The creation of the GVRLayout in the java file here is intended to
be temporary, as it doesn't allow presentation as-is. This simply gets us
up-to-par with the current Cardboard implementation so that we can remove any
dependencies on that library.

[bshe, 2016-08-08 21:43:41]

lgtm with just a few nits

GvrLayout needs to be notified on Resume and Pause. But that's probably not
important right now as it is mainly for presentation view and scanline racing
view.

https://codereview.chromium.org/2219203002/diff/1/device/vr/android/google_vr...
File device/vr/android/google_vr/google_vr_device.h (right):

https://codereview.chromium.org/2219203002/diff/1/device/vr/android/google_vr...
device/vr/android/google_vr/google_vr_device.h:5: #ifndef
DEVICE_VR_GOOGLE_VR_DEVICE_H
missing ANDROID?

https://codereview.chromium.org/2219203002/diff/1/device/vr/android/google_vr...
File device/vr/android/google_vr/google_vr_device_provider.h (right):

https://codereview.chromium.org/2219203002/diff/1/device/vr/android/google_vr...
device/vr/android/google_vr/google_vr_device_provider.h:5: #ifndef
DEVICE_VR_GOOGLE_VR_DEVICE_PROVIDER_H
ditto

https://codereview.chromium.org/2219203002/diff/1/device/vr/android/java/src/...
File
device/vr/android/java/src/org/chromium/device/vr/GoogleVRDeviceProvider.java
(right):

https://codereview.chromium.org/2219203002/diff/1/device/vr/android/java/src/...
device/vr/android/java/src/org/chromium/device/vr/GoogleVRDeviceProvider.java:1:
// Copyright 2015 The Chromium Authors. All rights reserved.
2016 :)

[David Trainor- moved to gerrit, 2016-08-08 23:05:04]

chrome/android/BUILD.gn lgtm

[bajones, 2016-08-08 23:14:45]

Description was changed from

==========
Migrate WebVR Cardboard implementation to Google VR

BUG=389343
==========

to

==========
Migrate WebVR Cardboard implementation to GVR

BUG=389343
==========

[bajones, 2016-08-08 23:15:17]

The CQ bit was checked by bajones@chromium.org

[bajones, 2016-08-08 23:15:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org, bshe@chromium.org
Link to the patchset: https://codereview.chromium.org/2219203002/#ps20001
(title: "Name change to GVR across the board")

[commit-bot: I haz the power, 2016-08-08 23:15:59]

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-08-08 23:26:04]

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

[commit-bot: I haz the power, 2016-08-08 23:26:06]

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...)

[mthiesse, 2016-08-09 14:51:56]

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

https://codereview.chromium.org/2219203002/diff/20001/device/vr/android/gvr/g...
device/vr/android/gvr/gvr_device.cc:95: gvr::Mat4f head_mat =
gvr_->GetHeadPoseInStartSpace(gvr_get_time_point_now());
You're not doing any prediction? Most apps would choose a time point ~50ms in
the future.

[girard, 2016-08-09 14:57:58]

Thanks for renaming the class.

LGTM

[bajones, 2016-08-09 16:15:00]

On 2016/08/09 14:51:56, mthiesse wrote:
>
https://codereview.chromium.org/2219203002/diff/20001/device/vr/android/gvr/g...
> File device/vr/android/gvr/gvr_device.cc (right):
> 
>
https://codereview.chromium.org/2219203002/diff/20001/device/vr/android/gvr/g...
> device/vr/android/gvr/gvr_device.cc:95: gvr::Mat4f head_mat =
> gvr_->GetHeadPoseInStartSpace(gvr_get_time_point_now());
> You're not doing any prediction? Most apps would choose a time point ~50ms in
> the future.

Ah, thanks for pointing this out! I had mentally put it on a "figure out what
should go here later" list. :)

Looks like the NDK treasure hunt sample actually does just use a static 50ms for
it's look ahead.
(https://github.com/googlevr/gvr-android-sdk/blob/master/ndk-beta/demos/treasu...)
I'd like to think we can do better, but I'll update to use that same constant
for the time being.

[bajones, 2016-08-11 22:38:51]

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

[commit-bot: I haz the power, 2016-08-11 22:40:03]

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-08-11 23:19:01]

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

[commit-bot: I haz the power, 2016-08-11 23:19:02]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[bajones, 2016-08-12 16:55:55]

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

[commit-bot: I haz the power, 2016-08-12 16:56:17]

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-08-12 16:58:15]

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

[commit-bot: I haz the power, 2016-08-12 16:58:16]

Dry run: 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...)

[bajones, 2016-08-12 17:08:11]

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

[commit-bot: I haz the power, 2016-08-12 17:08:31]

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-08-12 18:03:35]

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

[commit-bot: I haz the power, 2016-08-12 18:03:37]

Dry run: Exceeded global retry quota

[bajones, 2016-08-12 20:12:48]

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

[bajones, 2016-08-12 20:12:49]

sievers@: Please review dependency removal from /content?

[no sievers, 2016-08-15 17:13:20]

On 2016/08/12 20:12:49, bajones wrote:
> sievers@: Please review dependency removal from /content?

lgtm

[bajones, 2016-08-17 22:13:20]

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

[commit-bot: I haz the power, 2016-08-17 22:14:00]

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

[bajones, 2016-08-17 22:54:18]

The CQ bit was checked by bajones@chromium.org

[bajones, 2016-08-17 22:54:19]

The patchset sent to the CQ was uploaded after l-g-t-m from girard@chromium.org,
dtrainor@chromium.org, bshe@chromium.org, sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/2219203002/#ps380001
(title: "dpranke@ suggested a MUCH better way to handle the so dependency!
THANKS!")

[commit-bot: I haz the power, 2016-08-17 22:54:42]

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-08-18 00:52:58]

Description was changed from

==========
Migrate WebVR Cardboard implementation to GVR

BUG=389343
==========

to

==========
Migrate WebVR Cardboard implementation to GVR

BUG=389343
==========

[commit-bot: I haz the power, 2016-08-18 00:52:59]

Committed patchset #20 (id:380001)

[commit-bot: I haz the power, 2016-08-18 00:55:40]

Description was changed from

==========
Migrate WebVR Cardboard implementation to GVR

BUG=389343
==========

to

==========
Migrate WebVR Cardboard implementation to GVR

BUG=389343

Committed: https://crrev.com/267d5f909f25543d2351cc506626b5d5c08461c3
Cr-Commit-Position: refs/heads/master@{#412710}
==========

[commit-bot: I haz the power, 2016-08-18 00:55:41]

Patchset 20 (id:??) landed as
https://crrev.com/267d5f909f25543d2351cc506626b5d5c08461c3
Cr-Commit-Position: refs/heads/master@{#412710}

[boliu, 2016-08-18 02:20:02]

This caused cronet bot gn gen failing (and I suspect though haven't confirmed a
few other android compile failures)

Reverting

[boliu, 2016-08-18 02:21:55]

A revert of this CL (patchset #20 id:380001) has been created in
https://codereview.chromium.org/2258513003/ by boliu@chromium.org.

The reason for reverting is: Android build failure on cronet:
https://build.chromium.org/p/chromium.android/builders/Android%20Cronet%20MIP...

/b/build/slave/Android_Cronet_MIPS_Builder/build/src/buildtools/linux64/gn gen
//out/Release --check
  -> returned 1
ERROR at //third_party/gvr-android-sdk/BUILD.gn:27:1: Empty sources for copy
command.
copy("libgvr_shared_library") {
^------------------------------
You have to specify at least one file to copy in the "sources".
See //device/vr/BUILD.gn:47:7: which caused the file to be included.
      "//third_party/gvr-android-sdk:libgvr",
      ^-------------------------------------
GN gen failed: 1
step returned non-zero exit code: 1


Also suspect but have not confirmed causing these two build failures:
https://build.chromium.org/p/chromium.android/builders/Android%20arm64%20Buil...
https://build.chromium.org/p/chromium.android/builders/Android%20x64%20Builde....

[bajones, 2016-08-18 16:29:47]

Description was changed from

==========
Migrate WebVR Cardboard implementation to GVR

BUG=389343

Committed: https://crrev.com/267d5f909f25543d2351cc506626b5d5c08461c3
Cr-Commit-Position: refs/heads/master@{#412710}
==========

to

==========
Migrate WebVR Cardboard implementation to GVR

BUG=389343

Committed: https://crrev.com/267d5f909f25543d2351cc506626b5d5c08461c3
Cr-Commit-Position: refs/heads/master@{#412710}
==========

[bajones, 2016-08-19 16:08:50]

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

[bajones, 2016-08-19 16:08:51]

scottmg@: Please review build/config/features.gni, which should avoid the
problems we saw last time this patch landed with MIPS and non-component builds.
(Also, the header of that file suggests that it's not the right place for these
sorts of flags going forward. Happy to hear suggestions for a better home!)

Excluding MIPS is probably permanent (and no real loss?) but the restriction to
component builds only is a temporary measure to allow this CL to land and
unblock dependent patches. There's conversations underway to figure out how to
work around the single-library restriction of the Crazy Linker, and once we
figure out the right path forward for that I'll re-enable this feature on
statically linked builds.

[scottmg, 2016-08-19 16:34:23]

lgtm

[bajones, 2016-08-19 17:30:09]

The CQ bit was checked by bajones@chromium.org

[bajones, 2016-08-19 17:30:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org, girard@chromium.org, sievers@chromium.org,
bshe@chromium.org
Link to the patchset: https://codereview.chromium.org/2219203002/#ps440001
(title: "Disabling WebVR on non-component builds")

[commit-bot: I haz the power, 2016-08-19 17:30:41]

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-08-19 19:54:43]

Description was changed from

==========
Migrate WebVR Cardboard implementation to GVR

BUG=389343

Committed: https://crrev.com/267d5f909f25543d2351cc506626b5d5c08461c3
Cr-Commit-Position: refs/heads/master@{#412710}
==========

to

==========
Migrate WebVR Cardboard implementation to GVR

BUG=389343

Committed: https://crrev.com/267d5f909f25543d2351cc506626b5d5c08461c3
Cr-Commit-Position: refs/heads/master@{#412710}
==========

[commit-bot: I haz the power, 2016-08-19 19:54:44]

Committed patchset #23 (id:440001)

[commit-bot: I haz the power, 2016-08-19 19:56:48]

Description was changed from

==========
Migrate WebVR Cardboard implementation to GVR

BUG=389343

Committed: https://crrev.com/267d5f909f25543d2351cc506626b5d5c08461c3
Cr-Commit-Position: refs/heads/master@{#412710}
==========

to

==========
Migrate WebVR Cardboard implementation to GVR

BUG=389343

Committed: https://crrev.com/267d5f909f25543d2351cc506626b5d5c08461c3
Committed: https://crrev.com/7ab52ec35d497d6bb96c6a1fa36c9b123bc72330
Cr-Original-Commit-Position: refs/heads/master@{#412710}
Cr-Commit-Position: refs/heads/master@{#413217}
==========

[commit-bot: I haz the power, 2016-08-19 19:56:49]

Patchset 23 (id:??) landed as
https://crrev.com/7ab52ec35d497d6bb96c6a1fa36c9b123bc72330
Cr-Commit-Position: refs/heads/master@{#413217}

[michaelbai, 2016-09-29 19:14:49]

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

[michaelbai, 2016-09-29 19:14:51]

https://codereview.chromium.org/2219203002/diff/440001/build/config/features.gni
File build/config/features.gni (right):

https://codereview.chromium.org/2219203002/diff/440001/build/config/features....
build/config/features.gni:127: enable_webvr = is_android && is_component_build
&&
It seems not correct to use is_component_build here, is_component_build  is for
native code, webvr has Java code,

If someone changes is_component_build, Java code are not expected to be changed.

Do you really mean 'is_debug' here?

[klausw, 2016-09-29 19:28:53]

On 2016/09/29 19:14:51, michaelbai wrote:
>
https://codereview.chromium.org/2219203002/diff/440001/build/config/features.gni
> File build/config/features.gni (right):
> 
>
https://codereview.chromium.org/2219203002/diff/440001/build/config/features....
> build/config/features.gni:127: enable_webvr = is_android && is_component_build
> &&
> It seems not correct to use is_component_build here, is_component_build  is
for
> native code, webvr has Java code,
> 
> If someone changes is_component_build, Java code are not expected to be
changed.
> 
> Do you really mean 'is_debug' here?

I don't know the full history, AFAIK this is just a temporary restriction to
work around a linker issue that made it impossible to build correctly in
non-component builds. The GVR library is going to provide a .a library for
linking in addition to the current .so one soon, this should make it possible to
remove the is_component_build restriction.

[bshe, 2016-09-29 19:32:25]

https://codereview.chromium.org/2219203002/diff/440001/build/config/features.gni
File build/config/features.gni (right):

https://codereview.chromium.org/2219203002/diff/440001/build/config/features....
build/config/features.gni:127: enable_webvr = is_android && is_component_build
&&
On 2016/09/29 19:14:51, michaelbai wrote:
> It seems not correct to use is_component_build here, is_component_build  is
for
> native code, webvr has Java code,
> 
> If someone changes is_component_build, Java code are not expected to be
changed.
> 
> Do you really mean 'is_debug' here?

bajones@ is OOO. I can try to answer your question.
The reason we use is_component_build here is because libgvr.so has linking issue
in
non component builds. Chrome's linker expecting only one single .so and doesn't
handle
two .so well. The solution is to get a libgvr.a and have it linked into
libchrome.so. We are
working on it and once it landed, is_component_build will be removed.

[michaelbai, 2016-09-29 19:42:15]

https://codereview.chromium.org/2219203002/diff/440001/build/config/features.gni
File build/config/features.gni (right):

https://codereview.chromium.org/2219203002/diff/440001/build/config/features....
build/config/features.gni:127: enable_webvr = is_android && is_component_build
&&
On 2016/09/29 19:32:24, bshe wrote:
> On 2016/09/29 19:14:51, michaelbai wrote:
> > It seems not correct to use is_component_build here, is_component_build  is
> for
> > native code, webvr has Java code,
> > 
> > If someone changes is_component_build, Java code are not expected to be
> changed.
> > 
> > Do you really mean 'is_debug' here?
> 
> bajones@ is OOO. I can try to answer your question.
> The reason we use is_component_build here is because libgvr.so has linking
issue
> in
> non component builds. Chrome's linker expecting only one single .so and
doesn't
> handle
> two .so well. The solution is to get a libgvr.a and have it linked into
> libchrome.so. We are
> working on it and once it landed, is_component_build will be removed.

In this case, WebVR shouldn't be enabled by default, if you want the code be
covered by build bot, you can have a dedicated bot.

[bshe, 2016-09-29 19:50:22]

https://codereview.chromium.org/2219203002/diff/440001/build/config/features.gni
File build/config/features.gni (right):

https://codereview.chromium.org/2219203002/diff/440001/build/config/features....
build/config/features.gni:127: enable_webvr = is_android && is_component_build
&&
On 2016/09/29 19:42:14, michaelbai wrote:
> On 2016/09/29 19:32:24, bshe wrote:
> > On 2016/09/29 19:14:51, michaelbai wrote:
> > > It seems not correct to use is_component_build here, is_component_build 
is
> > for
> > > native code, webvr has Java code,
> > > 
> > > If someone changes is_component_build, Java code are not expected to be
> > changed.
> > > 
> > > Do you really mean 'is_debug' here?
> > 
> > bajones@ is OOO. I can try to answer your question.
> > The reason we use is_component_build here is because libgvr.so has linking
> issue
> > in
> > non component builds. Chrome's linker expecting only one single .so and
> doesn't
> > handle
> > two .so well. The solution is to get a libgvr.a and have it linked into
> > libchrome.so. We are
> > working on it and once it landed, is_component_build will be removed.
> 
> In this case, WebVR shouldn't be enabled by default, if you want the code be
> covered by build bot, you can have a dedicated bot.

We are targetting to ship it for M55. So more coverage sounds better at this
stage. There were
some issues recently after we upgrade gvr library. We wouldn't be able to catch
them if we disable webvr. Could you elaborate the reason that you prefer us to
disable it by default?

[michaelbai, 2016-09-29 20:01:46]

https://codereview.chromium.org/2219203002/diff/440001/build/config/features.gni
File build/config/features.gni (right):

https://codereview.chromium.org/2219203002/diff/440001/build/config/features....
build/config/features.gni:127: enable_webvr = is_android && is_component_build
&&
On 2016/09/29 19:50:22, bshe wrote:
> On 2016/09/29 19:42:14, michaelbai wrote:
> > On 2016/09/29 19:32:24, bshe wrote:
> > > On 2016/09/29 19:14:51, michaelbai wrote:
> > > > It seems not correct to use is_component_build here, is_component_build 
> is
> > > for
> > > > native code, webvr has Java code,
> > > > 
> > > > If someone changes is_component_build, Java code are not expected to be
> > > changed.
> > > > 
> > > > Do you really mean 'is_debug' here?
> > > 
> > > bajones@ is OOO. I can try to answer your question.
> > > The reason we use is_component_build here is because libgvr.so has linking
> > issue
> > > in
> > > non component builds. Chrome's linker expecting only one single .so and
> > doesn't
> > > handle
> > > two .so well. The solution is to get a libgvr.a and have it linked into
> > > libchrome.so. We are
> > > working on it and once it landed, is_component_build will be removed.
> > 
> > In this case, WebVR shouldn't be enabled by default, if you want the code be
> > covered by build bot, you can have a dedicated bot.
> 
> We are targetting to ship it for M55. So more coverage sounds better at this
> stage. There were
> some issues recently after we upgrade gvr library. We wouldn't be able to
catch
> them if we disable webvr. Could you elaborate the reason that you prefer us to
> disable it by default?

First of all, the issue caused by enable_webvr depending on is_component_build
is confusing and not easy to be discovered, basically it is wrong.

Second, as you can see, if webvr only worked for component_build, it is not
ready for launch at all, and the current solution is very different than the one
you want to launch, it shouldn't be enabled by default.

Last, you can have separated bot which enables webvr to cover your code if you
really want to.

[no sievers, 2016-09-29 20:52:22]

Description was changed from

==========
Migrate WebVR Cardboard implementation to GVR

BUG=389343

Committed: https://crrev.com/267d5f909f25543d2351cc506626b5d5c08461c3
Committed: https://crrev.com/7ab52ec35d497d6bb96c6a1fa36c9b123bc72330
Cr-Original-Commit-Position: refs/heads/master@{#412710}
Cr-Commit-Position: refs/heads/master@{#413217}
==========

to

==========
Migrate WebVR Cardboard implementation to GVR

BUG=389343

Committed: https://crrev.com/267d5f909f25543d2351cc506626b5d5c08461c3
Committed: https://crrev.com/7ab52ec35d497d6bb96c6a1fa36c9b123bc72330
Cr-Original-Commit-Position: refs/heads/master@{#412710}
Cr-Commit-Position: refs/heads/master@{#413217}
==========

[no sievers, 2016-09-29 20:52:22]

sievers@chromium.org changed reviewers:
- sievers@chromium.org

[bshe, 2016-09-29 21:03:30]

https://codereview.chromium.org/2219203002/diff/440001/build/config/features.gni
File build/config/features.gni (right):

https://codereview.chromium.org/2219203002/diff/440001/build/config/features....
build/config/features.gni:127: enable_webvr = is_android && is_component_build
&&
On 2016/09/29 20:01:46, michaelbai wrote:
> On 2016/09/29 19:50:22, bshe wrote:
> > On 2016/09/29 19:42:14, michaelbai wrote:
> > > On 2016/09/29 19:32:24, bshe wrote:
> > > > On 2016/09/29 19:14:51, michaelbai wrote:
> > > > > It seems not correct to use is_component_build here,
is_component_build 
> > is
> > > > for
> > > > > native code, webvr has Java code,
> > > > > 
> > > > > If someone changes is_component_build, Java code are not expected to
be
> > > > changed.
> > > > > 
> > > > > Do you really mean 'is_debug' here?
> > > > 
> > > > bajones@ is OOO. I can try to answer your question.
> > > > The reason we use is_component_build here is because libgvr.so has
linking
> > > issue
> > > > in
> > > > non component builds. Chrome's linker expecting only one single .so and
> > > doesn't
> > > > handle
> > > > two .so well. The solution is to get a libgvr.a and have it linked into
> > > > libchrome.so. We are
> > > > working on it and once it landed, is_component_build will be removed.
> > > 
> > > In this case, WebVR shouldn't be enabled by default, if you want the code
be
> > > covered by build bot, you can have a dedicated bot.
> > 
> > We are targetting to ship it for M55. So more coverage sounds better at this
> > stage. There were
> > some issues recently after we upgrade gvr library. We wouldn't be able to
> catch
> > them if we disable webvr. Could you elaborate the reason that you prefer us
to
> > disable it by default?
> 
> First of all, the issue caused by enable_webvr depending on is_component_build
> is confusing and not easy to be discovered, basically it is wrong.
> 
> Second, as you can see, if webvr only worked for component_build, it is not
> ready for launch at all, and the current solution is very different than the
one
> you want to launch, it shouldn't be enabled by default.
> 
> Last, you can have separated bot which enables webvr to cover your code if you
> really want to.
webvr actually gets a thumbs up to release in M55 if we can put everything
together before
M55 cut. The main issue for the specific code is a linking issue. It has little
to do with webvr
feature itself(And we are working on support non component build). We were able
to play with
webvr in component build.
We could have separate bots to test webvr. But there are a lot of different
combinations (cpu, release/debug, component/non component) that we want to test
for linking. It is not trivial to setup all the
combination to make sure that we could simply flip the webvr flag without
surprises near the end
of M55.
Given we only have a few days for M55, unless you still strongly disagree, it
would really simplify our work to leave it enable by default. And if we are not
able to ship webvr in M55, I will make sure that I turn it off before M55 cut.

[michaelbai, 2016-09-30 00:11:22]

On 2016/09/29 21:03:30, bshe wrote:
>
https://codereview.chromium.org/2219203002/diff/440001/build/config/features.gni
> File build/config/features.gni (right):
> 
>
https://codereview.chromium.org/2219203002/diff/440001/build/config/features....
> build/config/features.gni:127: enable_webvr = is_android && is_component_build
> &&
> On 2016/09/29 20:01:46, michaelbai wrote:
> > On 2016/09/29 19:50:22, bshe wrote:
> > > On 2016/09/29 19:42:14, michaelbai wrote:
> > > > On 2016/09/29 19:32:24, bshe wrote:
> > > > > On 2016/09/29 19:14:51, michaelbai wrote:
> > > > > > It seems not correct to use is_component_build here,
> is_component_build 
> > > is
> > > > > for
> > > > > > native code, webvr has Java code,
> > > > > > 
> > > > > > If someone changes is_component_build, Java code are not expected to
> be
> > > > > changed.
> > > > > > 
> > > > > > Do you really mean 'is_debug' here?
> > > > > 
> > > > > bajones@ is OOO. I can try to answer your question.
> > > > > The reason we use is_component_build here is because libgvr.so has
> linking
> > > > issue
> > > > > in
> > > > > non component builds. Chrome's linker expecting only one single .so
and
> > > > doesn't
> > > > > handle
> > > > > two .so well. The solution is to get a libgvr.a and have it linked
into
> > > > > libchrome.so. We are
> > > > > working on it and once it landed, is_component_build will be removed.
> > > > 
> > > > In this case, WebVR shouldn't be enabled by default, if you want the
code
> be
> > > > covered by build bot, you can have a dedicated bot.
> > > 
> > > We are targetting to ship it for M55. So more coverage sounds better at
this
> > > stage. There were
> > > some issues recently after we upgrade gvr library. We wouldn't be able to
> > catch
> > > them if we disable webvr. Could you elaborate the reason that you prefer
us
> to
> > > disable it by default?
> > 
> > First of all, the issue caused by enable_webvr depending on
is_component_build
> > is confusing and not easy to be discovered, basically it is wrong.
> > 
> > Second, as you can see, if webvr only worked for component_build, it is not
> > ready for launch at all, and the current solution is very different than the
> one
> > you want to launch, it shouldn't be enabled by default.
> > 
> > Last, you can have separated bot which enables webvr to cover your code if
you
> > really want to.
> webvr actually gets a thumbs up to release in M55 if we can put everything
> together before
> M55 cut. The main issue for the specific code is a linking issue. It has
little
> to do with webvr
> feature itself(And we are working on support non component build). We were
able
> to play with
> webvr in component build.
> We could have separate bots to test webvr. But there are a lot of different
> combinations (cpu, release/debug, component/non component) that we want to
test
> for linking. It is not trivial to setup all the
> combination to make sure that we could simply flip the webvr flag without
> surprises near the end
> of M55.
> Given we only have a few days for M55, unless you still strongly disagree, it
> would really simplify our work to leave it enable by default. And if we are
not
> able to ship webvr in M55, I will make sure that I turn it off before M55 cut.

sgtm, good luck