Make Gamepad API preffable

RESOLVED FIXED in mozilla24

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: ted, Assigned: ted)

Tracking

unspecified
mozilla24
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

We should have a pref that controls enabling the Gamepad API so that we can make it off-by-default in releases until the API stabilizes. Right now we can disable it in configure if we need to, so it's not the end of the world, but it would be nicer to have a pref so users can flip it on for testing (it's also less invasive and less likely to break the build).
Posted patch Make Gamepad API preffable (obsolete) — Splinter Review
This seems to work in my testing. When I get around to exposing navigator.getGamepads() that will have to be pref-controlled as well, but currently we only have the events.

I had to refactor the tests a bit so they'd still work when the pref was disabled by default. While I was there I removed test_gamepad_basic.html since it wasn't testing anything useful (it was a leftover from very early testing).

Also I realized that GamepadService wasn't implementing QI for nsIObserver, oops. (I hit some assertions while testing this patch.)

I have the pref configured in all.js so that the API will be disabled for release builds now, since the spec and implementation aren't stable yet.
Attachment #730199 - Flags: review?(bugs)
Assignee: nobody → ted
Comment on attachment 730199 [details] [diff] [review]
Make Gamepad API preffable

This doesn't hide the events from the global scope.
Attachment #730199 - Flags: review?(bugs) → review-
Perhaps we should disable the API for FF23 release using the configure, and once we have
webidl events, update the patch here to hide events too.
Ok. Would you rather I waited on WebIDL events to land this then, so we can do it all properly?
Well, this pref'ing should probably wait for Webidl events, but
could we just disable the configure option once the gamepad API is about to merge to beta?
Yes, that was my intent if I couldn't get an acceptable way to pref it off. If WebIDL events aren't going to make it in time then we'll just do that.
I'm tracking this assuming this is where we'll also perform the disable for Beta.
Since this isn't going to make it in time for FF22 (per comment 3), should we just file a new bug to disable it at build time for beta?
bz pointed out that I could just set dom.gamepad.enabled to true in the Mochitest prefs file and skip all this pushPrefEnv stuff in the tests.
do you want to just morph this bug to be where we track disabling for 22 then?
I already have a patch here, I filed bug 857797 for that.
Posted patch Make Gamepad API preffable (obsolete) — Splinter Review
I tweaked this patch a little bit to make it simpler. It no longer supports dynamically changing the pref, you have to restart to make it take effect. I also got rid of the test changes and simply force the pref on in the test harness.
Attachment #730199 - Attachment is obsolete: true
Depends on: 847611
Now that smaug added webidl interfaces for the Gamepad events in bug 847611, I can pref-disable them here. Manual testing shows this works fine--all the Gamepad* interfaces are not visible to content when the pref is disabled.
Attachment #764739 - Flags: review?(bugs)
Attachment #763708 - Attachment is obsolete: true
Comment on attachment 764739 [details] [diff] [review]
Make Gamepad API preffable

The nsIObserver stuff aren't really related, but ok.

I don't understand why you remove test_gamepad_basic.html
It is perhaps rather useless test though
Attachment #764739 - Flags: review?(bugs) → review+
The nsIObserver change is not critical, it's leftover from when I had a pref observer in the first revision of the patch. It is just a correctness fix.

The test removal is also incidental, sorry, that is also leftover. The test isn't particularly useful though, so removing it is harmless.

Thanks for the review!
Blocks: 690935
https://hg.mozilla.org/mozilla-central/rev/3af6f05984ae
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.