Closed Bug 1215147 Opened 6 years ago Closed 6 years ago

Enable VR API's on FF for Android by default

Categories

(Firefox for Android Graveyard :: General, defect)

44 Branch
defect
Not set
normal

Tracking

(firefox44+ fixed, firefox45 fixed)

RESOLVED FIXED
Firefox 45
Tracking Status
firefox44 + fixed
firefox45 --- fixed

People

(Reporter: caseyyee.ca, Assigned: daoshengmu, NeedInfo)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [webvr])

Attachments

(1 file, 5 obsolete files)

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. "
Flags: needinfo?(vladimir)
Whiteboard: [webvr]
Product: Firefox → Firefox for Android
Version: 44 Branch → Firefox 44
If you agree that we should enable VR API for Android by default, we can use this patch.
Fantastic!   Thanks Daosheng :D
Who is the right reviewer for this patch?
Sounds like we should get Vlad one this one. He's already marked for NI.   Will ping him on it.
V2: Add r=vlad on commit message.
Attachment #8676623 - Attachment is obsolete: true
Attachment #8677224 - Flags: review?(vladimir)
Also flagging for James Wilcox
Flags: needinfo?(snorp)
Attachment #8677224 - Flags: review?(vladimir) → review+
I trust enabling WebVR doesn't break a bunch of stuff :)
Flags: needinfo?(snorp)
Nope, shouldn't change anything for fennec, just has some additional APIs available.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(vladimir)
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.
Flags: needinfo?(vladimir)
dom.vr.cardboard.enabled is default to be enabled at Fennec config.
Assignee: nobody → dmu
Keywords: checkin-needed
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!
Flags: needinfo?(dmu)
Keywords: checkin-needed
See Also: → 1218482
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.
Attachment #8677224 - Attachment is obsolete: true
Flags: needinfo?(dmu) → needinfo?(cbook)
Attachment #8679270 - Flags: review+
Keywords: checkin-needed
(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 :)
Flags: needinfo?(cbook)
sorry had to back this out for test failures/warning like https://treeherder.mozilla.org/logviewer.html#?job_id=5365884&repo=fx-team
Flags: needinfo?(dmu)
Depends on: 1219124
Flags: needinfo?(dmu)
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.
Flags: needinfo?(vladimir)
Attachment #8679270 - Flags: review+ → review-
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.
Attachment #8679270 - Attachment is obsolete: true
Attachment #8681570 - Flags: review?(vladimir)
Ready for checkin-needed Daosheng?
Flags: needinfo?(dmu)
Dietrich, I am waiting for reviewing.
Flags: needinfo?(dmu)
Keywords: checkin-needed
Please help land attachment 8681570 [details] [diff] [review] to m-c.
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
Flags: needinfo?(dmu)
Carsten,
I think I got the problem. I have to modify test_interfaces.html and contact with a DOM peer.
Flags: needinfo?(dmu)
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.
Attachment #8681570 - Attachment is obsolete: true
Attachment #8682896 - Flags: review?(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?
Attachment #8682896 - Flags: review?(bzbarsky) → review-
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.
Attachment #8682896 - Attachment is obsolete: true
Attachment #8683006 - Flags: review?(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.
Attachment #8683006 - Flags: review?(bzbarsky) → review+
bz,

Yep. It is for nightly and aurora.
Keywords: checkin-needed
Please land Attachment #8683006 [details] [diff] to m-c.
(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
Daosheng, could you please make an aurora uplift request for this patch so it is also fixed in Aurora44?
Flags: needinfo?(dmu)
https://hg.mozilla.org/mozilla-central/rev/c951580b6f9b
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
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.
Flags: needinfo?(dmu)
Attachment #8683006 - Flags: approval-mozilla-aurora?
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.
Flags: needinfo?(vladimir)
Flags: needinfo?(elancaster)
No longer depends on: 1223082
pinged Vlad and Erin over email just now.
This is just for Firefox for Android, not Firefox desktop.  Enabling general webvr APIs in devedition was pushed out, yes.
Flags: needinfo?(vladimir)
(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 on attachment 8683006 [details] [diff] [review]
Enable VR API on Firefox for Android (V5)

This is planned for Fennec44, let's uplift to Aurora.
Attachment #8683006 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.