1215147 - Enable VR API's on FF for Android by default

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Casey Yee]

As per vlad in email thread:
"We should not need the WebVR enabler on mobile.  The only thing it does is flip a pref; the only reason webvr is not enabled by default on nightly/dev edition is because of the e10s compat issue.  We don't use e10s on mobile, so we can (and should) just enable the WebVR APIs on Nightly/Dev Edition there. "

Comment 1

[Daosheng Mu[:daoshengmu]]

Attachment: Enable VR API on Firefox for Android

If you agree that we should enable VR API for Android by default, we can use this patch.

Comment 2

[Casey Yee]

Fantastic!   Thanks Daosheng :D

Comment 3

[Dietrich Ayala (:dietrich)]

Who is the right reviewer for this patch?

Comment 4

[Casey Yee]

Sounds like we should get Vlad one this one. He's already marked for NI.   Will ping him on it.

Comment 5

[Daosheng Mu[:daoshengmu]]

Attachment: Enable VR API on Firefox for Android

V2: Add r=vlad on commit message.

Comment 6

[Josh Carpenter [:jcarpenter]]

Also flagging for James Wilcox

Comment 7

[James Willcox (:snorp) ([e-mail]) (he/him)]

I trust enabling WebVR doesn't break a bunch of stuff :)

Comment 8

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Nope, shouldn't change anything for fennec, just has some additional APIs available.

Comment 9

[Casey Yee]

It looks like we also need to flip the switch on dom.vr.cardboard.enabled=true at the same time for this to work.    Vlad, can you confirm that this is needed?   VR API's don't seem to pickup the device otherwise.

Comment 10

[Daosheng Mu[:daoshengmu]]

dom.vr.cardboard.enabled is default to be enabled at Fennec config.

Comment 11

[Daosheng Mu[:daoshengmu]]

Please land attachment 8677224 [details] [diff] [review]

Comment 12

[Carsten Book [:Tomcat]]

Hi, this does not apply cleanly:

applying 0001-Bug-1215147-Enable-VR-API-s-on-FF-for-Android-by-def.patch
patching file mobile/android/app/mobile.js
Hunk #1 FAILED at 928
1 out of 1 hunks FAILED -- saving rejects to file mobile/android/app/mobile.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and refresh 0001-Bug-1215147-Enable-VR-API-s-on-FF-for-Android-by-def.patch

could you take a look, thanks!

Comment 13

[Daosheng Mu[:daoshengmu]]

Attachment: Enable VR API on Firefox for Android (V2)

v2:
Rebase to the latest master branch and regenerate this patch. But, I find nothing special. Tomcat, could help me try to land this attachment again? Thanks a lot.

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/077054793949

Comment 15

[Carsten Book [:Tomcat]]

(In reply to Daosheng Mu[:daoshengmu] from comment #13)
> Created attachment 8679270 [details] [diff] [review]
> Enable VR API on Firefox for Android (V2)
> 
> v2:
> Rebase to the latest master branch and regenerate this patch. But, I find
> nothing special. Tomcat, could help me try to land this attachment again?
> Thanks a lot.

worked now and checkin done in comment #14 :)

Comment 16

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures/warning like https://treeherder.mozilla.org/logviewer.html#?job_id=5365884&repo=fx-team

Comment 17

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/fx-team/rev/a86899c6955a

Comment 18

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 8679270 [details] [diff] [review]
Enable VR API on Firefox for Android (V2)

I missed this the first time around -- this API should only be enabled in nightly/aurora builds, not in release.  I think you can use #ifndef RELEASE_BUILD for this purpose.

Comment 19

[Daosheng Mu[:daoshengmu]]

Attachment: Enable VR API on Firefox for Android (V3)

Vlad, thanks for your suggestion. After add #ifndef RELEASE_BUILD, that can solve the test failures.

V3 - 
Refer to Comment 18, add #ifndef RELEASE_BUILD for only be enabled in nightly/aurora builds.

Comment 20

[Daosheng Mu[:daoshengmu]]

It looks good.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=29864ca52ffa

Comment 21

[Dietrich Ayala (:dietrich)]

Ready for checkin-needed Daosheng?

Comment 22

[Daosheng Mu[:daoshengmu]]

Dietrich, I am waiting for reviewing.

Comment 23

[Daosheng Mu[:daoshengmu]]

Please help land attachment 8681570 [details] [diff] [review] to m-c.

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/f94a6394b5d1

Comment 25

[Carsten Book [:Tomcat]]

backed this out for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=5540074&repo=fx-team - seems its the same like in comment #16

Comment 26

[Daosheng Mu[:daoshengmu]]

Carsten,
I think I got the problem. I have to modify test_interfaces.html and contact with a DOM peer.

Comment 27

[Daosheng Mu[:daoshengmu]]

Attachment: Enable VR API on Firefox for Android (V4)

BZ, Please help me take a review on test_interfaces.html. Because we decide to enable VR API on Android by default, I have to make some modifications on it. Let's review it here, and I will close Bug 1219124.

V4 - 
Modify test_interfaces.html to allow VR API can be test pass.

Comment 28

[Daosheng Mu[:daoshengmu]]

Try link, it looks good.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=89925586d347

Comment 29

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8682896 [details] [diff] [review]
Enable VR API on Firefox for Android (V4)

>+#ifndef RELEASE_BUILD

So you're enabling on non-release... but changing the test to say it's enabled on android always.  That means the test will fail as soon as you merge to beta.  Might be worth asking one of the sheriffs how to do a "pretend I merged to beta" try build.

I assume you want the test to say "android: true, release: false" or something along those lines, no?

Comment 30

[Daosheng Mu[:daoshengmu]]

Attachment: Enable VR API on Firefox for Android (V5)

Hi bz,
Please help me review it again. I have confirmed that I just need to enable it on Firefox Android Nightly. Therefore, I should use "android: true, release: false".

V4 - 
Modifying test_interfaces.html to make VR API "release: false". Because we just want to enable it on Nightly.

Comment 31

[Daosheng Mu[:daoshengmu]]

Try result is good as well.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b7011f459e7f

Comment 32

[Pulsebot]

Backout:
https://hg.mozilla.org/mozilla-central/rev/9630c50d0966

Comment 33

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8683006 [details] [diff] [review]
Enable VR API on Firefox for Android (V5)

> I have confirmed that I just need to enable it on Firefox Android Nightly

Just nightly, or nightly and devedition?

r=me if it's "nightly and devedition".  If it's really meant to be nightly, both your test changes and the ifdef in mobile.js are wrong.

Comment 34

[Daosheng Mu[:daoshengmu]]

bz,

Yep. It is for nightly and aurora.

Comment 35

[Daosheng Mu[:daoshengmu]]

Please land Attachment #8683006 [details] [diff] to m-c.

Comment 36

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/c951580b6f9b

Comment 37

[Carsten Book [:Tomcat]]

(In reply to Daosheng Mu[:daoshengmu] from comment #34)
> bz,
> 
> Yep. It is for nightly and aurora.

landed now on fx-team and given this patch sticks on the tree it will get merged to m-c then.

For aurora this would need approval requests for aurora

Comment 38

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Daosheng, could you please make an aurora uplift request for this patch so it is also fixed in Aurora44?

Comment 39

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

This seems worth tracking for FF44.

Comment 40

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c951580b6f9b

Comment 41

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8683006 [details] [diff] [review]
Enable VR API on Firefox for Android (V5)

Approval Request Comment
[Feature/regressing bug #]: Enable VR API on Android Firefox by default.
[User impact if declined]: Nope. It would help users enable dom.vr.enabled and needn't do it manually. 
[Describe test coverage new/current, TreeHerder]:I have run try tests for Android devices, https://treeherder.mozilla.org/#/jobs?repo=try&revision=b7011f459e7f. And all of them are good.
[Risks and why]: Nope. I just enable additional APIs for users.
[String/UUID change made/needed]: none.

Comment 42

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Vlad, Erin: I might be mistaken but I thought that during the WebVR Go/No Go meeting there was a general consensus that webvr was pushed out to 45. Has that changed? I came upon this when I looked at the Aurora uplift request and would like to confirm if this is planned for Firefox44.

Comment 43

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

pinged Vlad and Erin over email just now.

Comment 44

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

This is just for Firefox for Android, not Firefox desktop.  Enabling general webvr APIs in devedition was pushed out, yes.

Comment 45

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #44)
> This is just for Firefox for Android, not Firefox desktop.  Enabling general
> webvr APIs in devedition was pushed out, yes.

Thanks. In that case I'll approve the aurora44 uplift.

Comment 46

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8683006 [details] [diff] [review]
Enable VR API on Firefox for Android (V5)

This is planned for Fennec44, let's uplift to Aurora.

Comment 47

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c96b70c245aa

Comment 48

[Chris Mills [:cmills]]

Browser compat info updated; see 

https://developer.mozilla.org/en-US/docs/Web/API/WebVR_API

(and child pages)