Closed Bug 1413362 Opened 2 years ago Closed 2 years ago

Create GVR implementation of WebVR for Android GeckoView

Categories

(GeckoView :: General, enhancement)

enhancement
Not set

Tracking

(firefox58 fixed)

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: rbarker, Assigned: rbarker)

Details

Attachments

(3 files, 2 obsolete files)

Create GVR implementation of WebVR for Android GeckoView.

Currently WebVR is only support in Gecko on Android as a shim. Implementing WebVR with GVR would WebVR to work natively on Android.
Assignee: nobody → rbarker
Comment on attachment 8924257 [details]
Bug 1413362 - part 1: Add GeckoVRManager to support GVR WebVR implementation on Android

https://reviewboard.mozilla.org/r/195468/#review200612

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoView.java:328
(Diff revision 1)
> +      */
> +     public interface GVRDelegate {
> +         /**
> +          * Creates non-presenting context. Will be invoked in the compositor thread.
> +          */
> +         long createGVRNonPresentingContext();

I wonder if there is anything useful the app can do here that's different from what you have in the demo app. We could just do that directly in GV and remove this API, right?

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoView.java:332
(Diff revision 1)
> +          */
> +         long createGVRNonPresentingContext();
> +         /**
> +          * Destroys non-presenting context. Will be invoked in the compositor thread.
> +          */
> +         void destroyGVRNonPresentingContext();

Same as above

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoView.java:349
(Diff revision 1)
> +
> +    /**
> +     * Set the GVR Delegate for GeckoView.
> +     * @param delegate GVRDelegate instance or null to unset.
> +     */
> +    public static void setGVRDelegate(GVRDelegate delegate) {

The static delegate is weird, even if we only have one GV that can use VR at a time. Also, the delegate methods don't include the GV instance that is requesting VR.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoView.java:358
(Diff revision 1)
> +    /**
> +     * Set the GVR paused state.
> +     * @param aPaused True if the application is being paused, False if the
> +     * application is resuming.
> +     */
> +    public static void setGVRPaused(final boolean aPaused) {

I think you should just do this in setActive(), which the app should call in onResume/onPause anyway.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoView.java:368
(Diff revision 1)
> +     * Set the GVR presenting context.
> +     * @param aContext GVR context to use when WebVR starts to present. Pass in
> +     * zero to stop presenting.
> +     */
> +    public static void setGVRPresentingContext(final long aContext) {
> +        Window.setGVRPresentingContext(aContext);

Should this be a return value (maybe async) from enableVRMode()?

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoView.java:374
(Diff revision 1)
> +    }
> +
> +    /**
> +     * Inform WebVR that the non-presenting context needs to be destroyed.
> +     */
> +    public static void cleanupGVRNonPresentingContext() {

When does that need to happen? Can GV infer that itself?
Comment on attachment 8924258 [details]
Bug 1413362 - part 2: Gecko gfx/vr GVR WebVR implementation

https://reviewboard.mozilla.org/r/195470/#review200620

::: gfx/vr/gfxVRGVR.h:26
(Diff revision 1)
> +#include "nsIScreen.h"
> +#include "nsCOMPtr.h"
> +
> +#include "VRDisplayHost.h"
> +
> +#pragma GCC system_header

We use clang now, do these pragmas stil work?
Attachment #8924257 - Attachment is obsolete: true
Attachment #8924259 - Attachment is obsolete: true
Comment on attachment 8924364 [details]
Bug 1413362 - part 3: Add support to build system for GoogleVR based WebVR on Android

https://reviewboard.mozilla.org/r/195624/#review201108

::: build/autoconf/android.m4:184
(Diff revision 2)
> +AC_DEFUN([MOZ_GVR_ANDROID_SDK],
> +[
> +
> +MOZ_ARG_WITH_STRING(gvr-android-sdk,
> +[  --with-gvr-android-sdk=DIR
> +                          location where the Android GVR SDK can be found (like ~/.mozbuild/gvr-android-sdk)],
> +    gvr_android_sdk=$withval)
> +
> +case "$target" in
> +*-android*|*-linuxandroid*)
> +    AC_MSG_CHECKING([for GVR Android SDK])
> +    if test -d "$gvr_android_sdk" ; then
> +        AC_MSG_RESULT([$gvr_android_sdk])
> +        AC_DEFINE(MOZ_USE_GVR_ANDROID)
> +        MOZ_GVR_INCLUDE=$gvr_android_sdk/libraries/headers/
> +        MOZ_GVR_LIBS=$gvr_android_sdk/libraries/jni/armeabi-v7a/
> +        if test -d "$MOZ_GVR_INCLUDE" -a -d "$MOZ_GVR_LIBS" ; then
> +          MOZ_USE_GVR_ANDROID=1
> +          AC_SUBST(MOZ_USE_GVR_ANDROID)
> +          AC_SUBST(MOZ_GVR_INCLUDE)
> +          AC_SUBST(MOZ_GVR_LIBS)
> +        else
> +          AC_MSG_ERROR([Could not find GVR NDK. Did you run ./gradlew :extractNdk in $gvr_android_sdk])
> +        fi
> +    elif test -n "$gvr_android_sdk" ; then
> +        AC_MSG_ERROR([Could not find GVR SDK $gvr_android_sdk])
> +    else
> +        AC_MSG_RESULT([not specified])
> +    fi
> +    ;;
> +esac
> +
> +])
> +

OK, so there is a right way to do this.

1. Add something like:

```
option('--with-gvr-android-sdk', nargs=1, help='...')
```

to build/moz.configure.

2. You then need to validate the value that got passed in.  So you write something like:

```
@depends('--with-gvr-android-sdk')
@imports('os')
def gvr_android_sdk(sdk_location):
  if not sdk_location:
    return
    
  # All the logic to construct paths and verify
  # they exist, in Python.
```

See something like `android_clang_compiler` in the same file for how this works out.  `die()` if you can't find things.  You'll probably need to return a `namespace` from this function, since you're trying to return two values: the include path and the library path.  See e.g. rustc_info in  build/moz.configure/rust.configure for how to do that.

3. Then you'll need to use the results:

```
set_config('MOZ_USE_GVR_ANDROID', ...)
set_config('MOZ_GVR_INCLUDE', gvr_android_sdk.includes)
set_config('MOZ_GVR_LIBS', gvr_android_sdk.libs)
```

Then you won't need to add the option name to old.configure or modifying old-configure.in or anything like that.
Comment on attachment 8924364 [details]
Bug 1413362 - part 3: Add support to build system for GoogleVR based WebVR on Android

https://reviewboard.mozilla.org/r/195624/#review201108

> OK, so there is a right way to do this.
> 
> 1. Add something like:
> 
> ```
> option('--with-gvr-android-sdk', nargs=1, help='...')
> ```
> 
> to build/moz.configure.
> 
> 2. You then need to validate the value that got passed in.  So you write something like:
> 
> ```
> @depends('--with-gvr-android-sdk')
> @imports('os')
> def gvr_android_sdk(sdk_location):
>   if not sdk_location:
>     return
>     
>   # All the logic to construct paths and verify
>   # they exist, in Python.
> ```
> 
> See something like `android_clang_compiler` in the same file for how this works out.  `die()` if you can't find things.  You'll probably need to return a `namespace` from this function, since you're trying to return two values: the include path and the library path.  See e.g. rustc_info in  build/moz.configure/rust.configure for how to do that.
> 
> 3. Then you'll need to use the results:
> 
> ```
> set_config('MOZ_USE_GVR_ANDROID', ...)
> set_config('MOZ_GVR_INCLUDE', gvr_android_sdk.includes)
> set_config('MOZ_GVR_LIBS', gvr_android_sdk.libs)
> ```
> 
> Then you won't need to add the option name to old.configure or modifying old-configure.in or anything like that.

As a note, can we please call anything to do with Google VR "googlevr" and not "gvr"?  "gvr" is way too close to GeckoView / GeckoVR.

froydnj has set out the way to define the integration with the build system -- that's definitely the way forward.  Let the Proguard work be your guide, it does almost exactly this.

The missing piece is how to get this in automation.  Again, Proguard shows the way.  Following http://searchfox.org/mozilla-central/source/taskcluster/scripts/misc/repack-proguard-jar.sh, I would "just" fetch a tarball (like https://github.com/googlevr/gvr-android-sdk/archive/v1.100.0.tar.gz), unpack it, remove some bits that you don't need,  unzip the AARs you need, repack it as an xz with the correct structure, and then make the Android builds depend on your android-googlevr-dependencies toolchain task.  The toolchain mechanism will unpack your tar.xz at $topsrcdir in automation -- make sure it's rooted at googlevr/ -- and then you can refer to the paths in your mozconfig files.
Comment on attachment 8924258 [details]
Bug 1413362 - part 2: Gecko gfx/vr GVR WebVR implementation

https://reviewboard.mozilla.org/r/195470/#review201188

::: gfx/vr/gfxVRGVR.cpp:384
(Diff revision 4)
> +  }
> +
> +  gvr_clock_time_point when = GVR_CHECK(gvr_get_time_point_now());
> +  if (mIsPresenting) {
> +    // The future
> +    when.monotonic_system_time_nanos += 50000000;

It may be useful to have a comment explaining why the value 50000000 was chosen.
Attachment #8924258 - Flags: review?(kgilbert) → review+
Comment on attachment 8924258 [details]
Bug 1413362 - part 2: Gecko gfx/vr GVR WebVR implementation

https://reviewboard.mozilla.org/r/195470/#review201194

This LGTM, thanks!

Please just add a comment about the constant value then r=me
Comment on attachment 8924363 [details]
Bug 1413362 - part 1: Add GeckoVRManager to support GVR WebVR implementation on Android

https://reviewboard.mozilla.org/r/195622/#review201198

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoVRManager.java:20
(Diff revision 2)
> +      */
> +     public interface GVRDelegate {
> +         /**
> +          * Creates non-presenting context. Will be invoked in the compositor thread.
> +          */
> +         long createGVRNonPresentingContext();

We already know this is for GVR (it's in GVRDelegate), so createNonPresentingContext() seems a better name
Attachment #8924363 - Flags: review?(snorp) → review+
Comment on attachment 8924258 [details]
Bug 1413362 - part 2: Gecko gfx/vr GVR WebVR implementation

https://reviewboard.mozilla.org/r/195470/#review201292

r=me. after fixing some nits. Great!

::: gfx/vr/gfxVRGVR.cpp:601
(Diff revision 4)
> +VRControllerGVR::VRControllerGVR(dom::GamepadHand aHand, uint32_t aDisplayID)
> +  : VRControllerHost(VRDeviceType::GVR, aHand, aDisplayID)
> +{
> +  MOZ_COUNT_CTOR_INHERITED(VRControllerGVR, VRControllerHost);
> +  mControllerInfo.mControllerName.AssignLiteral("Daydream Controller");
> +  mControllerInfo.mNumButtons = GVR_CONTROLLER_BUTTON_COUNT - 1; // Skip dummy none button

why the num of buttons is GVR_CONTROLLER_BUTTON_COUNT - 1 not GVR_CONTROLLER_BUTTON_COUNT?

::: gfx/vr/gfxVRGVR.cpp:709
(Diff revision 4)
> +}
> +
> +bool
> +VRSystemManagerGVR::GetHMDs(nsTArray<RefPtr<VRDisplayHost> >& aHMDResult)
> +{
> +

a redundant newline

::: gfx/vr/gfxVROSVR.cpp:365
(Diff revision 4)
> +#elif defined(MOZ_USE_GVR_ANDROID)
> +
> +bool
> +VRDisplayOSVR::SubmitFrame(const mozilla::layers::EGLImageDescriptor*,
> +                           const gfx::Rect& aLeftEyeRect,
> +                           const gfx::Rect& aRightEyeRect) {

Please move "{" to the next newline
Attachment #8924258 - Flags: review?(dmu) → review+
(In reply to Daosheng Mu[:daoshengmu] from comment #20)
> Comment on attachment 8924258 [details]
> Bug 1413362 - part 2: Gecko gfx/vr GVR WebVR implementation
> 
> https://reviewboard.mozilla.org/r/195470/#review201292
> 
> r=me. after fixing some nits. Great!
> 
> ::: gfx/vr/gfxVRGVR.cpp:601
> (Diff revision 4)
> > +VRControllerGVR::VRControllerGVR(dom::GamepadHand aHand, uint32_t aDisplayID)
> > +  : VRControllerHost(VRDeviceType::GVR, aHand, aDisplayID)
> > +{
> > +  MOZ_COUNT_CTOR_INHERITED(VRControllerGVR, VRControllerHost);
> > +  mControllerInfo.mControllerName.AssignLiteral("Daydream Controller");
> > +  mControllerInfo.mNumButtons = GVR_CONTROLLER_BUTTON_COUNT - 1; // Skip dummy none button
> 
> why the num of buttons is GVR_CONTROLLER_BUTTON_COUNT - 1 not
> GVR_CONTROLLER_BUTTON_COUNT?
> 

In GVR the button at index zero is a dummy button that does not exist on the device. So I skip it. See: https://developers.google.com/vr/android/ndk/reference/group/types#group__types_1ga25a81f172e4de5c794b8f3ddb6194f41
Comment on attachment 8924363 [details]
Bug 1413362 - part 1: Add GeckoVRManager to support GVR WebVR implementation on Android

https://reviewboard.mozilla.org/r/195622/#review201536

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoVRManager.java:16
(Diff revision 2)
> +     public interface GVRDelegate {
> +         /**
> +          * Creates non-presenting context. Will be invoked in the compositor thread.
> +          */
> +         long createGVRNonPresentingContext();
> +         /**
> +          * Destroys non-presenting context. Will be invoked in the compositor thread.
> +          */
> +         void destroyGVRNonPresentingContext();
> +         /**
> +          * Called when WebVR needs a presenting context. Will be invoked in the UI thread.
> +          */
> +         boolean enableVRMode();
> +         /**
> +          * Called when WebVR has finished presenting. Will be invoked in the UI thread.
> +          */
> +         void disableVRMode();
> +     }

nit: indentation

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoVRManager.java:50
(Diff revision 2)
> +    public static void setGVRPaused(final boolean aPaused) {
> +        nativeSetGVRPaused(aPaused);
> +    }
> +
> +    /**
> +     * Set the GVR presenting context.
> +     * @param aContext GVR context to use when WebVR starts to present. Pass in
> +     * zero to stop presenting.
> +     */
> +    public static void setGVRPresentingContext(final long aContext) {
> +        nativeSetGVRPresentingContext(aContext);
> +    }
> +
> +    /**
> +     * Inform WebVR that the non-presenting context needs to be destroyed.
> +     */
> +    public static void cleanupGVRNonPresentingContext() {
> +        nativeCleanupGVRNonPresentingContext();
> +    }

Use `@WrapForJNI` on these methods directly instead of extra `native***` methods.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoVRManager.java:92
(Diff revision 2)
> +        }
> +        mGVRDelegate.destroyGVRNonPresentingContext();
> +    }
> +
> +    @WrapForJNI
> +    private static void enableVRMode() {

/* package */

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoVRManager.java:111
(Diff revision 2)
> +
> +        mGVRDelegate.enableVRMode();
> +    }
> +
> +    @WrapForJNI
> +    private static void disableVRMode() {

/* package */

::: widget/android/GeckoVRManager.h:1
(Diff revision 2)
> +#ifndef GeckoVRManager_h_

`mozilla_GeckoVRManager_h_`

::: widget/android/GeckoVRManager.h:13
(Diff revision 2)
> +class GeckoVRManager
> +    : public mozilla::java::GeckoVRManager::Natives<mozilla::GeckoVRManager>
> +{
> +
> +public:
> +  static void InitJNI();

Call it `Init` to override the same method from base class.

::: widget/android/GeckoVRManager.cpp:1
(Diff revision 2)
> +#include "GeckoVRManager.h"

IMO GeckoVRManager.cpp is simple enough that everything can go in GeckoVRManager.h itself.
Attachment #8924363 - Flags: review?(nchen) → review+
Comment on attachment 8924258 [details]
Bug 1413362 - part 2: Gecko gfx/vr GVR WebVR implementation

https://reviewboard.mozilla.org/r/195470/#review200620

> We use clang now, do these pragmas stil work?

Yes, these are still needed. If there are llvm version I can switch but these still work with clang.
Comment on attachment 8924258 [details]
Bug 1413362 - part 2: Gecko gfx/vr GVR WebVR implementation

https://reviewboard.mozilla.org/r/195470/#review201292

> why the num of buttons is GVR_CONTROLLER_BUTTON_COUNT - 1 not GVR_CONTROLLER_BUTTON_COUNT?

Added better comment explaining
Comment on attachment 8924364 [details]
Bug 1413362 - part 3: Add support to build system for GoogleVR based WebVR on Android

https://reviewboard.mozilla.org/r/195624/#review201590

Lots of nits, but almost no substance.  r- just to make sure I loop back on the leading / required.

::: build/moz.configure/android-ndk.configure:312
(Diff revision 3)
> +@depends('--with-googlevr-android-sdk', target)
> +@checking('for GoogleVR SDK', lambda x: x.result)
> +@imports(_from='os.path', _import='exists')
> +def googlevr_sdk(value, target):
> +    if not value:
> +        return namespace (

nit: no space between `namespace` and `(`.

::: build/moz.configure/android-ndk.configure:315
(Diff revision 3)
> +def googlevr_sdk(value, target):
> +    if not value:
> +        return namespace (
> +            result='Not specified'
> +        )
> +    path = value[0]

I think you should make this an absolute path.  `os.path.abspath`.

::: build/moz.configure/android-ndk.configure:317
(Diff revision 3)
> +        return namespace (
> +            result='Not specified'
> +        )
> +    path = value[0]
> +    if not exists(path):
> +        die('Could not find GoogleVR SDK %s', path)

Here and above you have SDK; below, you have GoogleVR NDK.  Patch up the bottom one?

::: build/moz.configure/android-ndk.configure:332
(Diff revision 3)
> +        die('Unsupported GoogleVR cpu architecture %s' % target.cpu)
> +
> +    libs = '{0}/libraries/jni/{1}/'.format(path, arch)
> +
> +    if not exists(libs):
> +        die('Could not find GoogleVR NDK at %s. Did you run ./gradlew :extractNdk in %s', libs, path)

nit: full sentence, so "Try running ..." or "Did you run ... ?" with a question mark.

::: build/moz.configure/android-ndk.configure:333
(Diff revision 3)
> +
> +    libs = '{0}/libraries/jni/{1}/'.format(path, arch)
> +
> +    if not exists(libs):
> +        die('Could not find GoogleVR NDK at %s. Did you run ./gradlew :extractNdk in %s', libs, path)
> +    return namespace (

nit: no space between, like above.

::: build/moz.configure/android-ndk.configure:340
(Diff revision 3)
> +        include=include,
> +        libs=libs,
> +        enabled=True,
> +    )
> +
> +set_define('MOZ_USE_GOOGLE_VR_ANDROID', googlevr_sdk.enabled)

Can we go with `MOZ_ANDROID_GOOGLE_VR`?  Most of the Android specific things are `MOZ_ANDROID_*`.

::: build/moz.configure/android-ndk.configure:342
(Diff revision 3)
> +        enabled=True,
> +    )
> +
> +set_define('MOZ_USE_GOOGLE_VR_ANDROID', googlevr_sdk.enabled)
> +set_config('MOZ_USE_GOOGLE_VR_ANDROID', googlevr_sdk.enabled)
> +set_config('MOZ_GOOGLE_VR_INCLUDE', googlevr_sdk.include)

And maybe `MOZ_ANDROID_GOOGLE_VR*`.

::: mobile/android/app/moz.build:61
(Diff revision 3)
>    'ua-update.json.in',
>  ]
> +
> +if CONFIG['MOZ_USE_GOOGLE_VR_ANDROID']:
> +    FINAL_TARGET_FILES += [
> +        '/' + CONFIG['MOZ_GOOGLE_VR_LIBS'] + 'libgvr.so',

froydnj: this I can't quite understand -- even when the config is absolute (so starts with `/`), I needed this leading '/'.  Do you know what's happening iwth the `Path` instances?
Attachment #8924364 - Flags: review?(nalexander) → review-
(In reply to Nick Alexander :nalexander from comment #28)
> Comment on attachment 8924364 [details]
> Bug 1413362 - part 3: Add support to build system for GoogleVR based WebVR
> on Android
> ::: build/moz.configure/android-ndk.configure:317
> (Diff revision 3)
> > +        return namespace (
> > +            result='Not specified'
> > +        )
> > +    path = value[0]
> > +    if not exists(path):
> > +        die('Could not find GoogleVR SDK %s', path)
> 
> Here and above you have SDK; below, you have GoogleVR NDK.  Patch up the
> bottom one?
> 

No, this is correct. The SDK contains the NDK but it must be extracted. If you just checkout the SDK from github, libgvr.so is not part of the repo. It must be extracted so the later check ensures the NDK has been extracted inside of the SDK.
Comment on attachment 8924364 [details]
Bug 1413362 - part 3: Add support to build system for GoogleVR based WebVR on Android

https://reviewboard.mozilla.org/r/195624/#review201620

I'm happy if you're happy!

::: build/moz.configure/android-ndk.configure:333
(Diff revisions 3 - 4)
>          die('Unsupported GoogleVR cpu architecture %s' % target.cpu)
>  
>      libs = '{0}/libraries/jni/{1}/'.format(path, arch)
>  
>      if not exists(libs):
> -        die('Could not find GoogleVR NDK at %s. Did you run ./gradlew :extractNdk in %s', libs, path)
> +        die('Could not find GoogleVR NDK at %s. Did you try running \'./gradlew :extractNdk\' in %s?', libs, path)

nit: make sure this lints; it's a long line.  Something like `./mach lint -l py2 build/moz.configure/android-ndk.configure` might do it.
Attachment #8924364 - Flags: review?(nalexander) → review+
(In reply to Nick Alexander :nalexander from comment #33)
> Comment on attachment 8924364 [details]
> Bug 1413362 - part 3: Add support to build system for GoogleVR based WebVR
> on Android
> 
> https://reviewboard.mozilla.org/r/195624/#review201620
> 
> I'm happy if you're happy!
> 
> ::: build/moz.configure/android-ndk.configure:333
> (Diff revisions 3 - 4)
> >          die('Unsupported GoogleVR cpu architecture %s' % target.cpu)
> >  
> >      libs = '{0}/libraries/jni/{1}/'.format(path, arch)
> >  
> >      if not exists(libs):
> > -        die('Could not find GoogleVR NDK at %s. Did you run ./gradlew :extractNdk in %s', libs, path)
> > +        die('Could not find GoogleVR NDK at %s. Did you try running \'./gradlew :extractNdk\' in %s?', libs, path)
> 
> nit: make sure this lints; it's a long line.  Something like `./mach lint -l
> py2 build/moz.configure/android-ndk.configure` might do it.

Linter on try actually found three issues.
Comment on attachment 8924364 [details]
Bug 1413362 - part 3: Add support to build system for GoogleVR based WebVR on Android

https://reviewboard.mozilla.org/r/195624/#review201908

::: build/moz.configure/android-ndk.configure:319
(Diff revision 4)
> +            result='Not specified'
> +        )
> +    path = abspath(value[0])
> +    if not exists(path):
> +        die('Could not find GoogleVR SDK %s', path)
> +    include = '%s/libraries/headers/' % path

We aren't checking for the existence of the includes, but we check for the existence of the libraries, below?  Why the inconsistency?

::: build/moz.configure/android-ndk.configure:320
(Diff revision 4)
> +        )
> +    path = abspath(value[0])
> +    if not exists(path):
> +        die('Could not find GoogleVR SDK %s', path)
> +    include = '%s/libraries/headers/' % path
> +    arch = 'unknown'

We don't need this assignment.

::: build/moz.configure/android-ndk.configure:321
(Diff revision 4)
> +    if 'arm' in target.cpu:
> +        arch = 'armeabi-v7a'
> +    elif 'aarch64' in target.cpu:
> +        arch = 'arm64-v8a'
> +    elif 'x86' in target.cpu:
> +        arch = 'x86'

Can you use `'string' == target.cpu` instead, please?  Your x86 check, at least, will match x86-64, which I don't think is what you intended.

::: mobile/android/app/moz.build:61
(Diff revision 4)
>    'ua-update.json.in',
>  ]
> +
> +if CONFIG['MOZ_ANDROID_GOOGLE_VR']:
> +    FINAL_TARGET_FILES += [
> +        '/' + CONFIG['MOZ_ANDROID_GOOGLE_VR_LIBS'] + 'libgvr.so',

To answer nalexander's earlier question: `CONFIG[...]` might be an absolute path, but the build system interprets "absolute" paths as being relative to the root of the source directory.  To make it consider them *absolute* paths, we need this extra `'/'`, as you have done here.
Attachment #8924364 - Flags: review?(nfroyd) → review+
(In reply to Nathan Froyd [:froydnj] from comment #35)
> Comment on attachment 8924364 [details]
> Bug 1413362 - part 3: Add support to build system for GoogleVR based WebVR
> on Android
> 
> https://reviewboard.mozilla.org/r/195624/#review201908
> 
> ::: build/moz.configure/android-ndk.configure:319
> (Diff revision 4)
> > +            result='Not specified'
> > +        )
> > +    path = abspath(value[0])
> > +    if not exists(path):
> > +        die('Could not find GoogleVR SDK %s', path)
> > +    include = '%s/libraries/headers/' % path
> 
> We aren't checking for the existence of the includes, but we check for the
> existence of the libraries, below?  Why the inconsistency?

The build will fail if the includes are missing, but succeed in the rare situation that the includes are present but the libraries are missing.  We'd only see missing libraries at runtime, which is annoying.  This prevents that.

> 
> ::: build/moz.configure/android-ndk.configure:320
> (Diff revision 4)
> > +        )
> > +    path = abspath(value[0])
> > +    if not exists(path):
> > +        die('Could not find GoogleVR SDK %s', path)
> > +    include = '%s/libraries/headers/' % path
> > +    arch = 'unknown'
> 
> We don't need this assignment.
> 
> ::: build/moz.configure/android-ndk.configure:321
> (Diff revision 4)
> > +    if 'arm' in target.cpu:
> > +        arch = 'armeabi-v7a'
> > +    elif 'aarch64' in target.cpu:
> > +        arch = 'arm64-v8a'
> > +    elif 'x86' in target.cpu:
> > +        arch = 'x86'
> 
> Can you use `'string' == target.cpu` instead, please?  Your x86 check, at
> least, will match x86-64, which I don't think is what you intended.
> 
> ::: mobile/android/app/moz.build:61
> (Diff revision 4)
> >    'ua-update.json.in',
> >  ]
> > +
> > +if CONFIG['MOZ_ANDROID_GOOGLE_VR']:
> > +    FINAL_TARGET_FILES += [
> > +        '/' + CONFIG['MOZ_ANDROID_GOOGLE_VR_LIBS'] + 'libgvr.so',
> 
> To answer nalexander's earlier question: `CONFIG[...]` might be an absolute
> path, but the build system interprets "absolute" paths as being relative to
> the root of the source directory.  To make it consider them *absolute*
> paths, we need this extra `'/'`, as you have done here.

Thanks!  I learned something.  I'd forgotten about this wrinkle.
(In reply to Nathan Froyd [:froydnj] from comment #35)
> Comment on attachment 8924364 [details]
> Bug 1413362 - part 3: Add support to build system for GoogleVR based WebVR
> on Android
> 
> https://reviewboard.mozilla.org/r/195624/#review201908
> 
> ::: build/moz.configure/android-ndk.configure:319
> (Diff revision 4)
> > +            result='Not specified'
> > +        )
> > +    path = abspath(value[0])
> > +    if not exists(path):
> > +        die('Could not find GoogleVR SDK %s', path)
> > +    include = '%s/libraries/headers/' % path
> 
> We aren't checking for the existence of the includes, but we check for the
> existence of the libraries, below?  Why the inconsistency?
> 

The headers are part of the SDK github checkout. So if you've checked out the SDK you have the headers. The libs have to be extracted with a gradle command so I check for them explicitly.
Pushed by rbarker@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/653c66220a5f
part 1: Add GeckoVRManager to support GVR WebVR implementation on Android r=jchen,snorp
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d9da0d19d04
part 2: Gecko gfx/vr GVR WebVR implementation r=kip,daoshengmu
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b15dfaeecaa
part 3: Add support to build system for GoogleVR based WebVR on Android r=froydnj,nalexander
Backout by archaeopteryx@coole-files.de:
https://hg.mozilla.org/mozilla-central/rev/0b32504fcef2
Backed out 3 changesets for causing merge conflicts. r=merge a=merge
Sorry, we had to back this out for merge conflicts.
Flags: needinfo?(rbarker)
Pushed by rbarker@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a2400c11832
part 1: Add GeckoVRManager to support GVR WebVR implementation on Android r=jchen,snorp
https://hg.mozilla.org/integration/mozilla-inbound/rev/c398e89c47d5
part 2: Gecko gfx/vr GVR WebVR implementation r=kip,daoshengmu
https://hg.mozilla.org/integration/mozilla-inbound/rev/1413e5b916a3
part 3: Add support to build system for GoogleVR based WebVR on Android r=froydnj,nalexander
Rebased and repushed.
Flags: needinfo?(rbarker)
Product: Firefox for Android → GeckoView
Target Milestone: Firefox 58 → mozilla58
You need to log in before you can comment on or make changes to this bug.