851547 - Make Gamepad API preffable

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[(not currently active) Ted Mielczarek]

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

Comment 1

[(not currently active) Ted Mielczarek]

Attachment: Make Gamepad API preffable

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.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 730199 [details] [diff] [review]
Make Gamepad API preffable

This doesn't hide the events from the global scope.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 4

[(not currently active) Ted Mielczarek]

Ok. Would you rather I waited on WebIDL events to land this then, so we can do it all properly?

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

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?

Comment 6

[(not currently active) Ted Mielczarek]

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.

Comment 7

[Alex Keybl [:akeybl]]

I'm tracking this assuming this is where we'll also perform the disable for Beta.

Comment 8

[(not currently active) Ted Mielczarek]

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?

Comment 9

[(not currently active) Ted Mielczarek]

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.

Comment 10

[Lukas Blakk [:lsblakk] use ?needinfo]

do you want to just morph this bug to be where we track disabling for 22 then?

Comment 11

[(not currently active) Ted Mielczarek]

I already have a patch here, I filed bug 857797 for that.

Comment 12

[(not currently active) Ted Mielczarek]

Attachment: Make Gamepad API preffable

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.

Comment 13

[(not currently active) Ted Mielczarek]

Attachment: Make Gamepad API preffable

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.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 15

[(not currently active) Ted Mielczarek]

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!

Comment 16

[(not currently active) Ted Mielczarek]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3af6f05984ae

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3af6f05984ae