(not currently active) Ted Mielczarek Assignee
	Description • 14 years ago

I think we should try prototyping some DOM Events for Joysticks/Gamepads. With all of the other HTML5 goodness going on, web gaming is starting to get much better. It would be nice to provide built-in support for joysticks as well. I think a good place to start would be to define a couple of events that could be used for joystick button presses/axis movement, and see how it works.

	David Humphrey (:humph)
	Comment 1 • 14 years ago

I think this is a great idea, at least to try as an experiment, and would be willing to help.  I know various people are thinking about this:

http://www.effectgames.com/effect/article.psp.html/joe/My_HTML5_CSS3_Browser_Wish_List#_section_1_13

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 2 • 14 years ago

Is there any cross platform native API for joysticks/gamepads?

	(not currently active) Ted Mielczarek Assignee
	Comment 3 • 14 years ago

We could probably use SDL, but I'm not sure how well that'd work (also it's LGPL, which makes it a non-starter).

We probably just want per-platform implementations:
* DirectX for Windows
* Joystick API for Linux: http://www.kernel.org/doc/Documentation/input/joystick-api.txt
* IOKit on OS X (I can't find any Apple docs on this, but Allegro has some code and it's liberally licensed: http://alleg.svn.sourceforge.net/viewvc/alleg/allegro/branches/4.9/src/macosx/hidjoy.m?revision=13760&view=markup )

	Mike Shaver (:shaver -- probably not reading bugmail closely)
	Comment 4 • 14 years ago

I don't think we're going to have satisfying experiences with abstraction layers here, since game developers don't.  It'll be like graphics, multi-touch and similar where we have to program to native APIs, perhaps by building our own abstractions.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 5 • 14 years ago

I was just hoping to have some generic subset API (via some library) which
all the platforms support. But if that is not the case, we need to
do that ourselves.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 6 • 13 years ago

See also http://www.pcmag.com/article2/0,2817,2383457,00.asp

I don't know if anyone from Mozilla is involved with that work.

	Katelyn Gadd (:kael)
	Comment 7 • 13 years ago

I've been working on a pure JS prototype for a simple, bare-minimum API to expose simple input devices to content. I'm starting by exposing mouse/keyboard data since that doesn't require any native code and can be used to prove out the API. After that my plan is to actually integrate other input devices, either using js-ctypes or a plugin like https://github.com/STRd6/Boomstick to figure stuff out there without having to write the platform-specific input device glue. Didn't know we had a bug for this already :)

I had a discussion about this with Felipe a while back. One of the things that came up was that while the DOM is traditionally event-based, using events for game input is a very bad idea, as you can end up with hundreds of events per second that bog down event processing and may end up getting discarded anyway.

The general approach I'm taking with my design is that content describes the kind of input device it needs to the browser (number of axes, number of buttons, etc.) so that an appropriate device can be selected. This has a few major advantages:

Devices can be plugged in and removed at any time, and the burden on content authors to handle this correctly is relatively large. Many retail video games for PC and consoles do not correctly respond in these cases. Knowing about the *input* required by the content means that the browser can do a lot of the heavy lifting to get this right.

Likewise, knowing the nature of the input required by the content means that the browser can expose solid, well-designed configuration UI for cases where it is not immediately obvious how to map the available input devices to the content. Being able to expose configuration UI has additional benefits, since it implies user customization of input settings (hotkey selection, etc) and makes it easier for the user to tell what buttons content can respond to.

Knowing the nature of the input required also means that when no suitable input device is available, one can be emulated. Right now, any web content built around mouse or keyboard events is unlikely to work on FF mobile, because traditional keystroke and click events aren't generated. Content that describes its input needs could have them met with the aid of the browser (through on-screen, touch-based virtual inputs) or with the aid of third-party javascript that provides an emulated input device when the browser has none available.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 8 • 13 years ago

Fennec does generate click and keydown/up/press events

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 9 • 13 years ago

Also, if you're adding support for some kind of joystick event stream, 
the stream could be DOMEventTarget. Handling events in that case would
be pretty much as fast as handling any other callback.

Something like
var joystickEventStream = new EventStream("joystick");
joystickEventStream.addEventListener("joystickdata",
  function(event) {
    // do something with event 
  });

	(not currently active) Ted Mielczarek Assignee
	Comment 10 • 13 years ago

Here's a first cut at a patch for implementing this. This is the platform-independent part, it adds DOM Events and the associated plumbing, as well as a JoystickService class to do some generic managing of listeners and firing of events. Each platform will subclass JoystickService as SystemJoystickService to talk to hardware and provide events.

The events generated are:
MozJoyButton{Up,Down} for each button press/release, with properties joystickID and button. joystickID is just an integer that's unique for each connected device, and button is an index for the button being pressed.

MozJoyAxisMove for each axis movement, with properties joystickID, axis, and value. axis is an index for the axis being moved, and value is the absolute position of the axis in the range -1.0 to 1.0, with 0.0 being centered.

Comments welcome, I'll attach the Linux backend in a minute.

	(not currently active) Ted Mielczarek Assignee
	Comment 11 • 13 years ago

Here's the Linux backend. It's using the Linux joystick API (/dev/input/jsN) to receive joystick input, spawning a background thread and pushing events to the foreground thread to fire off DOM events. Seems to work well on my Ubuntu system.

	(not currently active) Ted Mielczarek Assignee
	Comment 12 • 13 years ago

This is the test page I've been using, it adds listeners for all three event types and displays the data.

	Grant Galitz
	Comment 13 • 13 years ago

This is getting interesting...

	David Humphrey (:humph)
	Comment 14 • 13 years ago

Here's a game that's playable with this patch

https://mozillademos.org/demos/runfield/demo.html

then paste and execute this to add the joystick button event handling:

javascript:(function(){window.addEventListener("MozJoyButtonDown", function(){startJump({preventDefault:function(){}});}, false);}());

First joystick enabled web game!

	David Humphrey (:humph)
	Comment 15 • 13 years ago

Forgot the button-up logic:

(function () {
    var ev = { preventDefault: function(){} };
    window.addEventListener("MozJoyButtonDown", function () {
        startJump(ev);
    }, false);
    window.addEventListener("MozJoyButtonUp", function () {
        endJump(ev);
    }, false);
}());

This works, shortened is:

(function(){varev={preventDefault:function(){}};window.addEventListener("MozJoyButtonDown",function(){startJump(ev);},false);window.addEventListener("MozJoyButtonUp",function(){endJump(ev);},false);}());

	(not currently active) Ted Mielczarek Assignee
	Comment 16 • 13 years ago

This patch adds a Windows backend using DirectInput. It only sends button press events currently, axis movement is hooked up but not translated yet. Building this requires you to add the DirectX lib directory to your $LIB path, since it's not there by default, something like:
export LIB="$LIB:c:\\Program Files (x86)\\Microsoft DirectX SDK (June 2010)\\Lib\\x86"
from a MozillaBuild prompt.

	(not currently active) Ted Mielczarek Assignee
	Comment 17 • 13 years ago

I tweaked the interaction between JoystickService and the platform implementations, so that the per-platform class declarations can be private, which makes life a bit easier. I'll update the Linux patch to match shortly.

	(not currently active) Ted Mielczarek Assignee
	Comment 18 • 13 years ago

Updated the Linux patch to match the changes to the base patch.

	Bobby Richter [:secretrobotron]
	Comment 19 • 13 years ago

This is pretty amazing. Just tested with a PS3 controller via USB.

Careful though:(In reply to comment #15)
> This works, shortened is:
> 
> (function(){varev={preventDefault:function(){}};window.
> addEventListener("MozJoyButtonDown",function(){startJump(ev);},false);window.
> addEventListener("MozJoyButtonUp",function(){endJump(ev);},false);}());

Should be this (small white-space error):
(function(){var ev={preventDefault:function(){}};window.addEventListener("MozJoyButtonDown",function(){startJump(ev);},false);window.addEventListener("MozJoyButtonUp",function(){endJump(ev);},false);}());

	Ben Vanik
	Comment 20 • 13 years ago

Awesome stuff! Can't wait to use my xbox controller in my game!
A suggestion when playing with these APIs from somehow who has tried to build apps on various input systems...

More advanced uses prefer polling of input data to full on event callbacks. For frame-based applications (like most games, visualizations, etc) keeping the input logic in the flow of the frame loop is the only way to stay sane. Add in complicating factors such as requestAnimationFrame and it could become very unpredictable as to when all of the input events should be processed (and in what order, if they have all arrived, etc). Keeping animation, physics, etc consistent without knowing exactly when an event happened with relation to app time or taking the priority of events incorrectly can cause many subtle issues.

XInput, the latest MSFT input API, works really well at simplifying the interface to various input devices and providing high-performance polling (it's probably one of their cleanest APIs).

It would be possible to still have an event that would fire on the input frequency (1/60s, manually on event such as keypress, etc - depends on input type) to make it easier in non-polling situations, but modern games and apps could use polling without suffering from nasty state tracking or performance issues with hundreds of events being fired for simple input actions. The polling would allow the app to do everything right while running, and then in an idle state shut down the frame loop until the next event occurs (indicating a discrete input event).

	(not currently active) Ted Mielczarek Assignee
	Comment 21 • 13 years ago

We've had some discussion about this, and we're certainly still in the prototyping phases here, but I feel that using DOM events maps pretty well to the "web" way of doing things, and it also matches the underlying model of most platforms, as well as matching what some libraries do (like SDL). I think once we get this implemented for Linux/Mac/Windows, and we get some builds out, we can see how well this works for actual use cases.

I haven't looked at the XInput APIs (it didn't look useful to use for this purpose since it only supports the xbox controller), I'll have to take a look.

	Rain [:rain1] (inactive)
	Comment 22 • 13 years ago

Actually there are a lot of different Xbox controllers (the official controller, alternate controllers, fightpads, fightsticks), all of which can be used on a PC with Xinput. There are also some Logitech controllers which support Xinput [1].

[1] http://blog.logitech.com/2010/09/03/new-logitech-gamepads-bring-the-console-gaming-experience-to-pc-gamers/

	(not currently active) Ted Mielczarek Assignee
	Comment 23 • 13 years ago

I still think DirectInput will support a much wider range of existing devices, having been around for a long time. Unless there's a compelling reason to use XInput, I'll stick with DI.

	(not currently active) Ted Mielczarek Assignee
	Comment 24 • 13 years ago

This patch adds support for axis move events on Windows.

	:Felipe Gomes (needinfo for replies!)
	Comment 25 • 13 years ago

I don't think that DOM events can work well for game input because they don't fit the performance characteristics of game engines. You have no control or knowledge about when an event will be received, and can't preemptively switch between the physics/rendering engine to the input control. Analog axis can fire dozens of events per second which can starve your other processing (and you can't take input out of the main thread -- though this is not common). When more than one button/axis is being used at the same time, the app cannot see the full state of the controller because the events are not merged.
Basically you have very little decision power to prioritize each part of the engine (drop frames vs. drop input), and can't do "as fast as needed, but not faster"

IMO a sync pooling system or async similar to mozRequestAnimationFrame that can produce an idle wait and drive the event loop when new input is available would work better. (unless the EventStream idea from comment 9 is able to circumvent some of these issues)

	Gregg Tavares
	Comment 26 • 13 years ago

I agree with the others and polling is the only rational way to handle this data.

The API you have is missing the fact that PS3 controllers have 8 pressure sensitive buttons so just button up and button down events are not enough. Then you also need to add in the PS3's accelerometer data.

All of that data comes through very fast. Wiggle the controller or just have your hands near the buttons pretty much guarantees tons of events and slowing down your game.

	(not currently active) Ted Mielczarek Assignee
	Comment 27 • 13 years ago

I appreciate the feedback, but I don't think it's really useful unless we either have experience showing that DOM events won't be sufficient, or a counterproposal of a polling system that we can evaluate.

The point of this bug is to prototype something to find out what is useful. I chose DOM events as a starting point because they match existing web usage pretty well and were fairly straightforward to implement. I'd like to get them working on all 3 major platforms so we can test them out and see how well they work. We very well may find out that the API as implemented here is not useful, and we need to explore other options, and that's okay.

I also agree that this basic API doesn't cover complex use cases like the PS3 controller right now, but that's also okay. If we decide that DOM events are the right way to go, we can figure out what needs to change to support controllers like that. If we go in another direction, we can keep them in mind there as well.

For those proponents of a polling API, would anyone care to write up a simple API proposal on a wiki somewhere? It's hard to argue for or against a concept, it would be easier if someone could articulate in more detail how this would work in the context of the web.

	(not currently active) Ted Mielczarek Assignee
	Comment 29 • 13 years ago

I put together a wiki page to capture some thoughts about this work (in the URL field here), feel free to contribute there.

	(not currently active) Ted Mielczarek Assignee
	Comment 31 • 13 years ago

I rebased all the patches to a more recent mozilla-central changeset (101411c3ca1c), and I fiddled the configure bits a little.

	(not currently active) Ted Mielczarek Assignee
	Comment 32 • 13 years ago

Rebased, with mild configure changes.

	(not currently active) Ted Mielczarek Assignee
	Comment 33 • 13 years ago

Rebased, and I fixed the configury to properly check for the DirectX SDK and lik to it without any manual fiddling.

	(not currently active) Ted Mielczarek Assignee
	Comment 34 • 13 years ago

Rebased.

	Justin Dolske [:Dolske]
	Comment 35 • 13 years ago

(In reply to comment #27)
> I appreciate the feedback, but I don't think it's really useful unless we
> either have experience showing that DOM events won't be sufficient, or a
> counterproposal of a polling system that we can evaluate.

It might be useful to ask the NZ media team... Our HTML5 media backend was, until recently, firing |timeupdate| events for every _video frame_. The spec was changed to allow firing them less frequently, I think we do 'em every 250ms now. (See bug 571822, bug 605472).

On the upside, I don't believe this caused any noticeable problems with media playback (which can be highly jitter sensitive). OTOH, the spec did get changed. :)

IMO, this isn't a big enough issue to block initial work, and a followup could look at consolidating/averaging joystick events as an improvement.

P.S. please support electroshock events so I can implement Domination (as seen in Never Say Never Again).

	Jason Orendorff [:jorendorff]
	Comment 36 • 13 years ago

This demo page is stupid. It doesn't explain what's going on well, and it will only work for controllers where axis 0 and 1 are X and Y of an analog stick (or where buttons 12-15 happen to be a d-pad).

But it is kind of neat being able to play some actual games with the thing. :)

	(not currently active) Ted Mielczarek Assignee
	Comment 37 • 13 years ago

Try server builds are up:
ftp://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/tmielczarek@mozilla.com-8df56f741a66/

	Rick Waldron [:rwaldron]
	Comment 38 • 13 years ago

I put this together for studying event properties, other might find it useful as well


https://gist.github.com/58b0f9ed647f72da9173

	(not currently active) Ted Mielczarek Assignee
	Comment 39 • 13 years ago

Fixed configure so this actually links...

	Scott Graham
	Comment 40 • 13 years ago

I'm afraid I'm late to the party, and not even able offer a patch yet, but I agree with previous commenters that a polling-based API seems more suited to use by games. (With feedback from some Chromium people) here's a very rough sketch of a proposal for a polling-style API.

https://docs.google.com/document/d/1atsxnstVybfovkX_f6xf2P25i1NT0ilCihJuPDwYWEU/edit?hl=en_US

I'll add this link to the wiki page that Ted started also. Please let me know what you think.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 41 • 13 years ago

Could you perhaps share that document so that one doesn't need to have
Google account to read it.

	Scott Graham
	Comment 42 • 13 years ago

I'm sorry you're not able to access it. I thought it didn't require any account or login. I tried in a fresh Private Browsing session and I don't seem to need a Google account, could you perhaps try again?

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 43 • 13 years ago

It seems to Google requires me to login to my Google account when trying to access that doc. But if I clear all the Google related cookies, I can see
the doc without login. Looks like a bug in Google Docs.

	Scott Graham
	Comment 44 • 13 years ago

Hopefully less annoying link here https://sites.google.com/a/chromium.org/dev/developers/joystick-api

	Grant Galitz
	Comment 45 • 13 years ago

Are the joytsticks ID'd yet through something like a .controllerName ?
If browsers can map the controllers the same way (button 0, button 1, etc. are all the same), then there can be avoidance of user agent sniffing by developers. Having checks against each individual input pad on a controller seems less likely for a web dev to do.

	Rick Waldron [:rwaldron]
	Comment 46 • 13 years ago

@Scott With regard to https://sites.google.com/a/chromium.org/dev/developers/joystick-api - What sort of feedback are you looking for? At the moment, I have some scattered notes that touch on a variety of points and I'm trying to determine how to best relay them.

	Rick Waldron [:rwaldron]
	Comment 47 • 13 years ago

@Scott

I widdled away most of the nits and came up with this list as important notes.


-------------

RE: Proposal Overall 

Proposal looks like DOM0 (eg. reminiscent of document.forms ) _0, _1 ...numbered "constant" names are not idiomatic JavaScript

High resolution events aren't new to the web: scroll, mousemove, deviceorientation are all high resolution events, _good_ web devs know to throttle these events with a secondary setTimeout loop (easily replaced with requestAnimationFrame)

Example of events being throttled with a setTimeout loop: http://jsfiddle.net/rwaldron/wPuGB/


RE: Polling API

On the web, you want to minimize the amount of long running logic that exists - so naturally, events are the solution - only react when X occurs.

Long running polls are the first thing to go when you're refactoring poorly engineered web applications.


RE: navigator object

navigator may have recently become home to the geolocation api - but it doesn't have _hardware_. No mouse, no keyboard. A joystick should be on the same level as a mouse or keyboard.


-------------

	Katelyn Gadd (:kael)
	Comment 48 • 13 years ago

Games (and probably most other consumers of joysticks) are not 'long running polls'. Games almost exclusively run at a fixed interval (i.e. requestAnimationFrame) and will need to read input state each time they perform an update. An event-only interface simply is not a good fit for these applications.

I don't disagree that the 'DOM way' is to use events, and I'm a fan of an API that provides both events and polling. But polling is the way most game code is written now and it probably will remain so in the future. If you give people an events-only API, they'll just build a polling backend for it. I've written a few of those already.

Events-only also has nasty consequences in a world where you have multiple websites/apps open at once. If you attempt to maintain a set of state data like 'keys A and D are down' by consuming events, that state becomes hopelessly corrupted if you lose focus while a key is held. You will never get that keyup event, and the game will not be able to reliably recover from this. (This happens in Win32 for games too. The solution is to poll, even if you do consume events.)

In regards to naming buttons, constants, and remapping inputs, I think that's probably best left as a separate API. You can build something like that atop good low-level keyboard/mouse/joystick APIs, but trying to solve all those problems at once is super risky. I'm working on a proposal for *that* kind of API, but I don't think such features should be baked directly into the joystick API if we want things to work well across browsers and devices.


Re: The chromium API proposal:

Triggers and Accelerometers are weird. I've never seen that semantic identity exposed in a joystick API - neither linux nor directinput seem to reliably expose such an input type. How do you propose we identify triggers and accelerometers on input devices that expose them as axes? What is the advantage of making them distinct types that must be handled with divergent code paths?

Constants for buttons/axes/etc: Do not want. Not sure what problems those constants solve, but they certainly create new problems of their own. I would be in favor of adding an optional 'name' attribute for each button/axis that the user-agent is encouraged to expose, if the driver provides it - but most drivers do not even expose such information, and if it's there, it might be wrong.

You need events (or worst case, a 'isConnected' boolean you can poll) to notify the content of controller disconnection/connection. This is something that many commercial games get wrong, so we should make it easy to do the right thing here.

	Scott Graham
	Comment 49 • 13 years ago

@Rick

Thanks for your comments. You're right, high-resolution is a weak justification in the face of mousemove, etc. Really what's important in my mind is who owns the main loop. I think this is RAF or similar, and that means it's much easier to request the input state during that frame via polling, rather than trying to reverse engineer state via coalescing events.

Re: navigator object

I'm sorry, I don't entirely understand this comment. You're saying navigator is the wrong place, but what is the correct one? window? document? Something else?

	Scott Graham
	Comment 50 • 13 years ago

@Kevin

Thanks for thinking through the proposal and for your feedback. Agreed on your reasoning for avoiding events for game loops. I do also agree there's a place for a dual api the offers both events and polling.

Good point about perhaps over-separating the styles of buttons. What do we think about perhaps just buttons vs. axes? Triggers become buttons, and accelerometers become axes? It seems sensible to keep buttons and axes separate because of their differing ranges and differing typical uses in a game.

Constants for buttons (and storing the inputs in an array, rather than say as named value) was there explicitly to support remapping without implementing it. By just having a set of indices to control what goes where, it should be trivial to remap buttons however desired.

connect/disconnect: I think in an event api, there would certainly be a connected/disconnected events. It wasn't explicitly spelled out but the intention was that returning null from getState() would indicate not-connected so the game would be able to track connections and disconnections. Perhaps you think it would be better to return an Object with an isConnected=false though?

	Katelyn Gadd (:kael)
	Comment 51 • 13 years ago

Do we know why Bocoup is reporting performance issues with only a single joystick?

http://weblog.bocoup.com/javascript-firefox-nightly-introduces-dom-joystick-events

Even if I expected events to be a perf problem, I didn't expect that to be the case with a single controller.

	Rick Waldron [:rwaldron]
	Comment 52 • 13 years ago

@Kevin

The performance issues we mentioned are known - they center around the extraordinarily high resolution of joystick events. Perhaps the article wasn't worded properly, I'll go back and edit for clarity.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 53 • 13 years ago

But where is the time spent. Is it event dispatching/handling which
is taking the time, or is the OS-level API handling which is slow or what?
Or perhaps it is the flooding from OS to mainloop which causes other task to be
slow?

Someone should do some profiling. 
A Shark profile copy-pasted here as text/plain attachment would be great.

I think we could keep event based API, but the events itself could be different.
(1) maybe they should fire at most the same rate as animation callback,
    perhaps just before animation callbacks fire so that the state in them can be the
    most recent.
(2) Events could have a list of state changes since the previous event.
    event.states[event.states.length - 1] would be the most recent one, 
    but also other state changes could be there. 

This way some games could just use event listeners to drive them, no need for
animation callbacks. But also games using animation callbacks could work well.
And the game could capture all the state changes.

	Rick Waldron [:rwaldron]
	Comment 54 • 13 years ago

@Olli & @Kevin

I went back and re-read the post and there is nothing that indicates that we had "performance issues", just that we encountered our own issues with the high number of events being fired - similar to events like: scroll, mousemove and deviceorientation.

So, to clarify, there were no performance issues. I will gladly update the post to reflect that, if you'd like

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 55 • 13 years ago

Ok, thanks for clarifying!

	Antoine Turmel [:GeekShadow]
	Comment 56 • 13 years ago

Not sure if it's spam, but using try-builds on Windows 7 (32 bits) and XBox 360 Wireless Controler it's working fine with demos provided.

	David Humphrey (:humph)
	Comment 57 • 13 years ago

Builds on Ted's patches (http://hg.mozilla.org/users/tmielczarek_mozilla.com/mq/) and adds typed arrays to nsDOMJoystick for .buttons and .axes properties.  This builds, but is totally untested (no joystick here), so may well crash.  This should get things started, though.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 58 • 13 years ago

Why do we want to use typed arrays for getting buttons etc? As opposed to using "normal" JS objects and properties?

	(not currently active) Ted Mielczarek Assignee
	Comment 59 • 13 years ago

I don't think they need to be typed arrays, necessarily, we're just copying some code Dave had from audio events. We do need to do raw JSAPI stuff here because xpidl doesn't have support for array-type attributes. (I tried hacking that in, but the obvious stuff didn't work, it probably requires xpconnect changes.)

	David Humphrey (:humph)
	Comment 60 • 13 years ago

(In reply to Jonas Sicking (:sicking) from comment #58)
> Why do we want to use typed arrays for getting buttons etc? As opposed to
> using "normal" JS objects and properties?

Doesn't have to be typed arrays.  Ted just wanted .buttons -> [] vs. a function.  I did this so he could play with that api.

	Antoine Turmel [:GeekShadow]
	Comment 61 • 13 years ago

Be sure to handle dead zone for sticks.

	Mozilla RelEng Bot
	Comment 62 • 13 years ago

Try run for 39d8db37b5a3 is complete.
Detailed breakdown of the results available here:
    http://tbpl.mozilla.org/?tree=Try&rev=39d8db37b5a3
Results (out of 4 total builds):
    success: 4
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/tmielczarek@mozilla.com-39d8db37b5a3

	Mozilla RelEng Bot
	Comment 63 • 13 years ago

Try run for 58546deb15fa is complete.
Detailed breakdown of the results available here:
    http://tbpl.mozilla.org/?tree=Try&rev=58546deb15fa
Results (out of 4 total builds):
    success: 4
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/tmielczarek@mozilla.com-58546deb15fa

	(not currently active) Ted Mielczarek Assignee
	Comment 64 • 13 years ago

Try out these latest Try builds with this test page:
http://people.mozilla.com/~tmielczarek/joytest2.html

It shows some of the new things I implemented in this set of patches, namely:
1) Using MozJoyConnected and MozJoyDisconnected events
2) Holding a reference to the event.joystick property, and using its .buttons and .axes properties to get the state of the controller without using other events.
3) Referencing the event.joystick.id property, which gives some unique information about a device. (<USB vendor id-product id-device name> on Windows/Mac, just device name on Linux currently.)

	(not currently active) Ted Mielczarek Assignee
	Comment 65 • 13 years ago

For all those of you who requested a polling API: does the API shown above meet your needs? If you view source on joytest2.html, you can see that it's simply using events for device connected/disconnected, and using a requestAnimationFrame handler for actually updating the page state.

	Scott Graham
	Comment 66 • 13 years ago

@Ted, I like the connect/disconnect-and-hold API, much nicer than the index-based one I suggested. I haven't tried it out yet, but at first glance my only concern is that *when* the axes/buttons are updated feels a little fuzzy. I'm not clear on when the input data would be updated, and if it could change (say) half way through my animation code.

Admittedly most people probably won't be that precise about input sampling. But, perhaps a timestamp field on event.joystick that comes through from the device polling, and a snapshot() method to make an atomic copy of the joystick data?

	(not currently active) Ted Mielczarek Assignee
	Comment 67 • 13 years ago

I should have clarified, but in this implementation the joystick data will not update during the course of script execution. It will only update when your script finishes and returns to the event loop. That is, if you use requestAnimationFrame like in my example, in one pass through the callback function the data will not change. On the next pass, it will be updated with any changes that happened in the interim.

	(not currently active) Ted Mielczarek Assignee
	Comment 68 • 13 years ago

Here's an updated patch. Changes from last time:
* Joystick events are now sent only to the currently focused page.
* Added MozJoyConnected and MozJoyDisconnected events
** Pages will receive a MozJoyConnected event for each device
   before receiving data from that device. If you already have a
   device connected, a MozJoyConnected event will be sent to a page
   when you press a button or move an axis with that page focused.
   If you connect a new device with a page focused, and that page
   has previously received data from any device, a MozJoyConnected
   event will be sent immediately. When you disconnect a device,
   a MozJoyDisconnected event will be sent to every page that had
   previously received data from that device.
* Added a .joystick property to all events
** The joystick property contains the full state of the device, comprising:
*** .id : A unique id for the device type, currently contains the USB
    vendor and product ID as well as a name.
*** .connected : true if the device is still connected to the system.
*** .buttons[] an array of the buttons present on the device. Each
    entry in the array is 0 if the button is not pressed, and nonzero
    if the button is pressed.
*** .axes[] an array of the axes present on the device. Each entry in the
    array is a float value in the range -1.0..1.0 representing the axis
    position from the lowest value (-1.0) to the highest value (1.0).
** Web content may save a reference to the joystick property and refer
   to it at any time to determine the current state of the device. The
   state will not be updated while content script is executing, only
   between script executions. (For example, the state will not be
   updated during the course of a setTimeout callback, but it may be
   updated the next time the callback is called.)

	(not currently active) Ted Mielczarek Assignee
	Comment 69 • 13 years ago

Updated Mac patch. I tweaked a couple of things in this patch, notably:
* Button indices were 1-indexed, I changed that to 0-indexed
* Axis values weren't correct with one of my controllers, I fixed that

	(not currently active) Ted Mielczarek Assignee
	Comment 70 • 13 years ago

Updated Windows patch. I don't think there are any major changes. Note that the Windows patch doesn't currently support hotplug of controllers, so you won't get MozJoyDisconnected events there yet.

	(not currently active) Ted Mielczarek Assignee
	Comment 71 • 13 years ago

Updated Linux patch. No major changes. The Linux backend doesn't currently support hotplug either. I have some test code to use libudev, it shouldn't be too hard to integrate.

	d
	Comment 72 • 13 years ago

This is very interesting work, and if it gets into a stable version it could make a large difference for web gaming. However, I wonder, with the try-build on my harddrive... how would I test this? I have a Mac, and access to an XBOX 360 controller. What do I do to try this with the previously mentioned examples?

	(not currently active) Ted Mielczarek Assignee
	Comment 73 • 13 years ago

I think Dave is going to put some documentation up on the wiki page. We'll make sure to get some details on how to test. For most USB gamepads you just plug them in and they work. For XBOX 360 controllers you might need to install a driver.

	d
	Comment 74 • 13 years ago

I installed the driver from http://tattiebogle.net/index.php/ProjectRoot/Xbox360Controller/OsxDriver (although it may not have been necessary for this), but realized that it will only work with wired controllers. I don't think I've ever had cords to my controllers, so either I'll have to buy one or try with my PS3 controllers later on. Thanks for help still, I won't take up more space in this bug for now.

	David Humphrey (:humph)
	Comment 75 • 13 years ago

I've done a first pass at documenting the API, including a tutorial:

https://wiki.mozilla.org/JoystickAPI

If you spot things in it, please let me know, and/or make corrections.

Also, if you're building examples or demos, please add links to:

https://wiki.mozilla.org/JoystickAPI#Demos.2C_Libraries.2C_Other_Code

I'll try to keep this document up to date as the API evolves.

	David Humphrey (:humph)
	Comment 76 • 13 years ago

As per discussions on irc today, this patch adds an index property to the joystick object, which allows one to differentiate between two or more of the same joystick type connected at the same time (e.g., two players both using Joystick Type X).  The index is just a way to say, "Joystick 1" vs. "Joystick 2" and doesn't contain or give away any new information about the joystick.

This builds on Ted's latest patch queue.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 77 • 13 years ago

Vibrator support was brought up over in bug 679966.

I think it would be ok to give any webpage control of the vibrator if the user has interacted with the page using joystick. So if we add API on the Joystick object which indicates if vibrator is available and for turning it on/off, then the page should only get access to that if the user has used the joystick on the page, right?

	(not currently active) Ted Mielczarek Assignee
	Comment 78 • 13 years ago

I think it would be fairly straightforward to build on top of what I have here, but I'd rather postpone that to a followup bug. I think getting the input side of things working is enough to bite off here. (For reference, I'm also explicitly not going to try to handle accelerometer data in this bug.)

	Tab Atkins Jr.
	Comment 79 • 13 years ago

I've done a quick review of the proposed spec along with Alex Russell from Chrome.  We have some minor points of feedback, some of which may already have been provided in the comments above (I didn't read all of them).

1. Buttons are usually analog on modern controllers.  The button object should have a .value, like the axis object, and the .buttons array should explicitly range from [0...1], with 0 meaning not pressed and >0 meaning pressed with the given pressure.  Digital buttons would always return a 1 when pressed.

2. I agree that the current division into buttons and axises is appropriate.  Triggers are a type of button, and accelerometers are a type of axis.

3. Grabbing and stashing the joystick object from an event as a way of enabling polling is pretty whack.  Instead, the API should explicitly expose a global object which can be directly polled for the current values of all buttons/axises.  The event-based interface should still remain, as that's useful as well.  (You can maintain the anti-fingerprinting goal by making the global object report a single disconnected joystick until the user has used the joystick, similar to what you currently describe.)

4. There needs to be a way to distinguish between multiple controllers.  David Humphrey provides a patch to expose this through the joystick.index property.  If you have a global object for polling the joysticks, it should be enumerable to get at the individual joysticks in it.

5. I agree with Jonas that access to the vibrator is useful, and that it's probably sufficient to allow it freely after the user has used the controller on the page in this session.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 80 • 13 years ago

(In reply to Tab Atkins Jr. from comment #79)
> 3. Grabbing and stashing the joystick object from an event as a way of
> enabling polling is pretty whack.
How so? The current connected/disconnected approach looks good to me.

	Tab Atkins Jr.
	Comment 81 • 13 years ago

(In reply to Olli Pettay [:smaug] from comment #80)
> (In reply to Tab Atkins Jr. from comment #79)
> > 3. Grabbing and stashing the joystick object from an event as a way of
> > enabling polling is pretty whack.
> How so? The current connected/disconnected approach looks good to me.

I don't think any other event sends over live objects, that you can only access by stashing it then and there.  DOM events send DOM nodes, but they're accessible elsewhere.

The security concern (don't want to allow fingerprinting via the connected devices) isn't a reasonable objection here, as it's trivial to address with the global polling object, as I showed in my example above.  (I later realized that my example was stupid - if the joysticks global is an enumerated list of joysticks, just make it an empty list before any joysticks have been connected.)

	Tab Atkins Jr.
	Comment 82 • 13 years ago

Two more comments: one an API bug, one a feature request.

The API bug is event oversubscription.  Right now when you register an axis or button listener, you get events for *every* axis or button change.  This is problematic in the same way that keyboard events or mutation events are problematic today: if you're only interested in a subset of stuff you still have to wade through all the other, useless events.  This is annoying for the author, and expensive for the browser, as it has to generate a bunch of events that will just be checked and ignored.

To fix this, there needs to be a way to listen to changes to particular buttons/axises (or particular sets of them).

The feature request regards button/axis naming.  The current approach of simply numbering them in order is of course necessary, since there are tons of different controllers that could potentially be used.  However, it's not easy to remember what button is what, even if you know the user is using a particular controller.  I'd like some way to assign a name map to a joystick, and have the joystick report things through that name as well as the numeric index.

Something like joystick.registerKeyMap(axises, buttons), where each argument is an array of strings that are mapped to the axis/button with the correspond index.  Then in, for example, a button event, the event object can have a .name property as well as a .index.  The joystick.buttons object can have the names and the numeric indexes hanging off of it.

With this, it would be really simple for a game library to then have, say, an array of Xbox button names and axis names, and another one for PS3 controllers, etc.

	(not currently active) Ted Mielczarek Assignee
	Comment 83 • 13 years ago

Thanks for the feedback! Let me reply to your points individually:

(In reply to Tab Atkins Jr. from comment #79)
> 1. Buttons are usually analog on modern controllers.  The button object
> should have a .value, like the axis object, and the .buttons array should
> explicitly range from [0...1], with 0 meaning not pressed and >0 meaning
> pressed with the given pressure.  Digital buttons would always return a 1
> when pressed.

I don't know that I have a controller handy that has this feature, so I haven't had a chance to work it into the design. I hear the PS3 controller has analog buttons, so I may pick one up to try it out. I think exposing the value as you proposed is a good fit. (We may need to figure out the semantics of "button down" and "button up" with analog buttons.)

> 2. I agree that the current division into buttons and axises is appropriate.
> Triggers are a type of button, and accelerometers are a type of axis.

It's currently a bit messy, given the sad state of affairs in cross-platform device APIs. Right now triggers are generally reported as an axis and I've punted on supporting accelerometers (since I don't have a controller with them to test, and they're not directly supported by the input APIs I'm using on most platforms).

> 3. Grabbing and stashing the joystick object from an event as a way of
> enabling polling is pretty whack.  Instead, the API should explicitly expose

It seems to work fine for me, but your alternate proposal is also fairly sensible. We can expose a .joysticks array somewhere that contains all the joystick objects that the page has access to (joysticks that have already sent events to the page).

> 4. There needs to be a way to distinguish between multiple controllers. 
> David Humphrey provides a patch to expose this through the joystick.index
> property.  If you have a global object for polling the joysticks, it should
> be enumerable to get at the individual joysticks in it.

Yeah, I'm going to fold his patch in. I think I can also make it such that the .index property will index into the .joysticks array.

> 5. I agree with Jonas that access to the vibrator is useful, and that it's
> probably sufficient to allow it freely after the user has used the
> controller on the page in this session.

I'm punting this to a followup, Jonas has filed bug 680289 on it. I think we have enough to chew on here just getting buttons and axes correct!

(In reply to Tab Atkins Jr. from comment #82)
> The API bug is event oversubscription.  Right now when you register an axis
> or button listener, you get events for *every* axis or button change.  This
> is problematic in the same way that keyboard events or mutation events are
> problematic today: if you're only interested in a subset of stuff you still
> have to wade through all the other, useless events.  This is annoying for
> the author, and expensive for the browser, as it has to generate a bunch of
> events that will just be checked and ignored.

I don't think "expensive for the browser" really matters all that much. We have to handle all the input regardless, posting some extra DOM events doesn't matter much. It might be nice to have a way to filter events to what you're actually interested in, but I don't have any ideas for how to do that in a sane web API. If you've got suggestions I'm all ears.

> The feature request regards button/axis naming.

This is probably the biggest open issue we have right now. I think we'd like to do some experimentation with the current API to gather data from users and find out what various devices look like on various platforms so we can assess the scope of the problem. My current proposal is to see if we can expose some identifying information per-device, and just write a content JS library to do the pretty name mapping, but we'll see how that works out in practice.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 84 • 13 years ago

IndexedDB delivers objects using events. Specifically the database object is delivered using a "success" event fired at the request object created when .open is called. And canvas.toBlob gives access to the Blob object though a callback which is similar to an event.

	Tab Atkins Jr.
	Comment 85 • 13 years ago

(In reply to Ted Mielczarek [:ted, :luser] from comment #83)
> (In reply to Tab Atkins Jr. from comment #79)
> > 1. Buttons are usually analog on modern controllers.  The button object
> > should have a .value, like the axis object, and the .buttons array should
> > explicitly range from [0...1], with 0 meaning not pressed and >0 meaning
> > pressed with the given pressure.  Digital buttons would always return a 1
> > when pressed.
> 
> I don't know that I have a controller handy that has this feature, so I
> haven't had a chance to work it into the design. I hear the PS3 controller
> has analog buttons, so I may pick one up to try it out. I think exposing the
> value as you proposed is a good fit. (We may need to figure out the
> semantics of "button down" and "button up" with analog buttons.)

IIRC, both the PS2 and original XBox controllers had analog face buttons and dpads.  PS3 and Xbox360 have even more analog - I know for a fact that the 360's triggers are analog.


> > 2. I agree that the current division into buttons and axises is appropriate.
> > Triggers are a type of button, and accelerometers are a type of axis.
> 
> It's currently a bit messy, given the sad state of affairs in cross-platform
> device APIs. Right now triggers are generally reported as an axis and I've
> punted on supporting accelerometers (since I don't have a controller with
> them to test, and they're not directly supported by the input APIs I'm using
> on most platforms).

Triggers are reported as axises?  Wut?  That's just crazy talk.

If we can possibly massage that into sanity that would be great.  If not, then shrug, it's just a wart on current controller interfaces.


> > 5. I agree with Jonas that access to the vibrator is useful, and that it's
> > probably sufficient to allow it freely after the user has used the
> > controller on the page in this session.
> 
> I'm punting this to a followup, Jonas has filed bug 680289 on it. I think we
> have enough to chew on here just getting buttons and axes correct!

Yeah, that's fine.

 
> (In reply to Tab Atkins Jr. from comment #82)
> > The API bug is event oversubscription.  Right now when you register an axis
> > or button listener, you get events for *every* axis or button change.  This
> > is problematic in the same way that keyboard events or mutation events are
> > problematic today: if you're only interested in a subset of stuff you still
> > have to wade through all the other, useless events.  This is annoying for
> > the author, and expensive for the browser, as it has to generate a bunch of
> > events that will just be checked and ignored.
> 
> I don't think "expensive for the browser" really matters all that much. We
> have to handle all the input regardless, posting some extra DOM events
> doesn't matter much. It might be nice to have a way to filter events to what
> you're actually interested in, but I don't have any ideas for how to do that
> in a sane web API. If you've got suggestions I'm all ears.

Well, they're expensive enough that we don't fire events at all if nobody's listening to them.  I figure that for events like the accelerometer (once it's handled as an axis or whatever) that would fire *all the time* while the controller's in use, it would actually make a measurable difference to omit firing them.

It's also annoying for the author.  If I'm building a game with NES-style controls (two buttons and a single stick) I *absolutely* do not want to have to deal with constant accelerometer events just because I'm listening to the stick's axises.

As for suggestions, I'll report back.  I know some of us Google engineers have talked about "better key events" (plenty of other people have too, I'll wager), and targeted subscription was one of the features we'd wanted.  I'll collect some of our thoughts and show them around.

This can certainly be a v2 feature, though.


> > The feature request regards button/axis naming.
> 
> This is probably the biggest open issue we have right now. I think we'd like
> to do some experimentation with the current API to gather data from users
> and find out what various devices look like on various platforms so we can
> assess the scope of the problem. My current proposal is to see if we can
> expose some identifying information per-device, and just write a content JS
> library to do the pretty name mapping, but we'll see how that works out in
> practice.

Sounds good.

	Tab Atkins Jr.
	Comment 86 • 13 years ago

(In reply to Jonas Sicking (:sicking) from comment #84)
> IndexedDB delivers objects using events. Specifically the database object is
> delivered using a "success" event fired at the request object created when
> .open is called. And canvas.toBlob gives access to the Blob object though a
> callback which is similar to an event.

Hmm.  IndexedDB's example is definitely relevant.  Not so sure about toBlob - semantically, it's just an async getter.

This might not be a big deal, it just seems more appropriate to poll based on a persistent global object.  But then, by delivering the object in an event, you can tell if any are still alive and simply not update if they've all been collected.  With the global you have to write to it forever just in case, unless you get funky with things (like not writing until script asks for it the first time).

	Tab Atkins Jr.
	Comment 87 • 13 years ago

(In reply to Ted Mielczarek [:ted, :luser] from comment #83)
> (In reply to Tab Atkins Jr. from comment #79)
> > 1. Buttons are usually analog on modern controllers.  The button object
> > should have a .value, like the axis object, and the .buttons array should
> > explicitly range from [0...1], with 0 meaning not pressed and >0 meaning
> > pressed with the given pressure.  Digital buttons would always return a 1
> > when pressed.
> 
> I don't know that I have a controller handy that has this feature, so I
> haven't had a chance to work it into the design. I hear the PS3 controller
> has analog buttons, so I may pick one up to try it out. I think exposing the
> value as you proposed is a good fit. (We may need to figure out the
> semantics of "button down" and "button up" with analog buttons.)

Forgot to respond to the parenthetical at the end!

There are two semantics that I've seen games exhibit.

The first and simplest is to simply say that the button is "up" when pressure is 0 and "down" when pressure is >0.  Of course, I'm not sure of the exact pressure values as I play, so the threshold may actually be slightly higher than zero.  I think we could start with just using zero, though, and add a control for adjusting the threshold later if necessary.

The second is to say that a button is "pressed" when the pressure jumps upward with sufficient velocity.  A very slow press doesn't trigger whatever action the button has.  Usually, though, a slow press does *something*, just something different from a normal "press".  I don't think this needs to be addressed directly - there's too much variety here, and games can implement this themselves by tracking velocity manually.  Potentially we could offer the current velocity as a piece of data alongside the pressure, but that's definitely a solution that should wait for an actual problem.

	Scott Graham
	Comment 88 • 13 years ago

(In reply to Ted Mielczarek [:ted, :luser] from comment #83)

> haven't had a chance to work it into the design. I hear the PS3 controller
> has analog buttons, so I may pick one up to try it out. I think exposing the
> value as you proposed is a good fit. (We may need to figure out the
> semantics of "button down" and "button up" with analog buttons.)

Do you mean the threshold for where it becomes up and down? This is my general concern with the edge-triggered events, as it's not clear how much filtering should be done on the data. For example, all authors should have dead-zones on axes, but there's not one correct way to do that.

When using the input to control a thing on a 2D plane, the dead-zone should be circular (X/Y dead-zoned together), whereas other uses might want axis-independent deadzones. Thresholds apply to the analog buttons also, so the pressed/unpressed isn't clear at the browser level. However, in order to have events trigger only when something "changes" those thresholds have to be done in the browser, else the inherent noise in sampling analog devices will cause constant event triggers.

For that reason, I was inclined to punt on events in v1 and focus on polling access, though I understand that seems non-browsery and might suck for some use cases.

I prefer enumerable .joysticks (on document?) as Tab proposed for what that's worth, though the connect/disconnect events are still useful notifications to have.

	Scott Graham
	Comment 89 • 13 years ago

(In reply to Tab Atkins Jr. from comment #85)
> (In reply to Ted Mielczarek [:ted, :luser] from comment #83)

> > It's currently a bit messy, given the sad state of affairs in cross-platform
> > device APIs. Right now triggers are generally reported as an axis and I've
> > punted on supporting accelerometers (since I don't have a controller with
> > them to test, and they're not directly supported by the input APIs I'm using
> > on most platforms).
> 
> Triggers are reported as axises?  Wut?  That's just crazy talk.
> 
> If we can possibly massage that into sanity that would be great.  If not,
> then shrug, it's just a wart on current controller interfaces.

It's different across devices, platforms, and even APIs (XInput reports triggers differently than DirectInput on Windows). I'd probably try to avoid massaging just because of the number of permutations involved. I guess that's just deferring the problem onto the magical content JS library, but that should be an easier place to iterate/update e.g. for new devices.

	(not currently active) Ted Mielczarek Assignee
	Comment 90 • 13 years ago

That pretty much precisely sums up my thoughts on the matter. I'd like to land a prototype, even if it's not really great, and see how it works on the web. If we can smooth out the rough edges with content JS libraries, then we might be okay. If we need to push some of that back up into the browser, then we can do that.

	David Humphrey (:humph)
	Comment 91 • 13 years ago

 > > 4. There needs to be a way to distinguish between multiple controllers. 
> > David Humphrey provides a patch to expose this through the joystick.index
> > property.  If you have a global object for polling the joysticks, it should
> > be enumerable to get at the individual joysticks in it.
> 
> Yeah, I'm going to fold his patch in. I think I can also make it such that
> the .index property will index into the .joysticks array.

Ted, the only reason I did an incremental value vs. an actual index into the array is so that add-joystick-1, remove-joystick-1, add-joystick-2 would not report the same device via index reuse.  Might not matter, but that was my thinking fwiw.

	Scott Graham
	Comment 92 • 13 years ago

(In reply to David Humphrey (:humph) from comment #91)
>  > > 4. There needs to be a way to distinguish between multiple controllers. 
> > > David Humphrey provides a patch to expose this through the joystick.index
> > > property.  If you have a global object for polling the joysticks, it should
> > > be enumerable to get at the individual joysticks in it.
> > 
> > Yeah, I'm going to fold his patch in. I think I can also make it such that
> > the .index property will index into the .joysticks array.
> 
> Ted, the only reason I did an incremental value vs. an actual index into the
> array is so that add-joystick-1, remove-joystick-1, add-joystick-2 would not
> report the same device via index reuse.  Might not matter, but that was my
> thinking fwiw.

Yes, I think that's important. .index should be stable across connect/disconnect. It's intended to identify players, correct? We don't want player2 changing into player1 if player1 disconnects.

It does mean that the enumeration of .joysticks is non-contiguous if we want to maintain .index being an indexer. I think I prefer that, though it will be unexpected to get a null back when iterating over them.

On the other side, if player1 disconnects and reconnects because he tripped over the cable, he should really stay player1, and not become a new player3.

	Tab Atkins Jr.
	Comment 93 • 13 years ago

(In reply to Scott Graham from comment #92)
> (In reply to David Humphrey (:humph) from comment #91)
> >  > > 4. There needs to be a way to distinguish between multiple controllers. 
> > > > David Humphrey provides a patch to expose this through the joystick.index
> > > > property.  If you have a global object for polling the joysticks, it should
> > > > be enumerable to get at the individual joysticks in it.
> > > 
> > > Yeah, I'm going to fold his patch in. I think I can also make it such that
> > > the .index property will index into the .joysticks array.
> > 
> > Ted, the only reason I did an incremental value vs. an actual index into the
> > array is so that add-joystick-1, remove-joystick-1, add-joystick-2 would not
> > report the same device via index reuse.  Might not matter, but that was my
> > thinking fwiw.
> 
> Yes, I think that's important. .index should be stable across
> connect/disconnect. It's intended to identify players, correct? We don't
> want player2 changing into player1 if player1 disconnects.

Yes, index should be stable (it shouldn't change because some other joystick was disconnected).  Consoles tend to slot controllers into the first unused index, like:

1. Turn on controller A.  It gets index 1.
2. Turn on controller B.  It gets index 2.
3. Turn off controller A.  Now there's nothing at index 1, and B is still at 2.
4. Turn on controller C.  It gets index 1.
5. Turn on controller A.  It gets index 3.

This is reasonable and well-known behavior.

> It does mean that the enumeration of .joysticks is non-contiguous if we want
> to maintain .index being an indexer. I think I prefer that, though it will
> be unexpected to get a null back when iterating over them.
> 
> On the other side, if player1 disconnects and reconnects because he tripped
> over the cable, he should really stay player1, and not become a new player3.

Yes.

(In reply to David Humphrey (:humph) from comment #91)
>  > > 4. There needs to be a way to distinguish between multiple controllers. 
> > > David Humphrey provides a patch to expose this through the joystick.index
> > > property.  If you have a global object for polling the joysticks, it should
> > > be enumerable to get at the individual joysticks in it.
> > 
> > Yeah, I'm going to fold his patch in. I think I can also make it such that
> > the .index property will index into the .joysticks array.
> 
> Ted, the only reason I did an incremental value vs. an actual index into the
> array is so that add-joystick-1, remove-joystick-1, add-joystick-2 would not
> report the same device via index reuse.  Might not matter, but that was my
> thinking fwiw.

See my above comments.  That scenario occurs currently on consoles, and the behavior *is* to have joystick B sit in index 1, since it was the first empty index.  It's useful when, for example, you accidentally turn on the controller that's almost battery-drained, and you want to switch to another one.

	(not currently active) Ted Mielczarek Assignee
	Comment 94 • 13 years ago

The "reuse of empty slots" is exactly the behavior implemented in my patch. You'd get:
connect A -> A.index == 0
connect B -> B.index == 1
disconnect A
connect A -> A.index == 0
disconnect A
connect C -> C.index == 0

	David Humphrey (:humph)
	Comment 95 • 13 years ago

FYI, Scott's got the discussion going on webkit and w3c lists, too (thanks):

https://lists.webkit.org/pipermail/webkit-dev/2011-August/017757.html

http://lists.w3.org/Archives/Public/public-webapps/2011JulSep/1019.html

	(not currently active) Ted Mielczarek Assignee
	Comment 96 • 13 years ago

Sorry for letting this bug go silent for so long, I've been tied up in other things. A few things have happened in the interim.

First, the W3C WebEvents WG is modifying their charter to add a Joystick API spec as a deliverable. Scott Graham (Google) and I will be co-authoring the spec.

Second, we (Mozilla) had a security review of this feature last week (September 19th). Some takeaways from the security team:
* Joystick objects should not update when a page doesn't have focus.
* Only iframes with same origin should be able to get the joystick events/data.

I'm going to try to update these patches this week and get them into a reviewable state so we can get them landed.

	David Woldrich
	Comment 97 • 13 years ago

(In reply to Ted Mielczarek [:ted, :luser] from comment #96)
[snip]
> Second, we (Mozilla) had a security review of this feature last week (September 19th).
> Some takeaways from the security team:
> * Joystick objects should not update when a page doesn't have focus.
> * Only iframes with same origin should be able to get the joystick events/data.

I'm assume the system already works like this, but just in case, I'll ask it anyways.  

Would it be possible to NOT enumerate or otherwise touch the host OS'es joystick drivers unless the page shows interest in the joystick API?  I've seen poorly written joystick drivers years ago in another life that made Windows unstable.   For general stability, I'd like to avoid that (albeit unlikely these days) scenario unless/until Firefox actually encounters a page that wants to know about Joystick.  Reading the spec, I think this is possible as the messages for joystick enumeration are only broadcast to the window onLoad if that window has previously registered handlers for those messages.

	(not currently active) Ted Mielczarek Assignee
	Comment 98 • 13 years ago

Yes, that's how my patches currently implement things. We don't touch anything joystick-related unless a page registers an event listener for some type of joystick event.

	David Woldrich
	Comment 99 • 13 years ago

(In reply to Ted Mielczarek [:ted, :luser] from comment #98)

> Yes, that's how my patches currently implement things.

Ok great, those semantics probably mean quicker time to page onLoad and other memory footprint goodness as well.

And am I reading these bullets right ...

> * Joystick objects should not update when a page doesn't have focus.
> * Only iframes with same origin should be able to get the joystick events/data.

... that iframes who register for joystick events independent of his parent window, but whose origin differs from the parent, shall *never* receive joystick messages?  Restated, joystick events flow from topmost window down only and the flowing stops when not same origin?  

If so, I vaguely understand the security motivations behind such a design decision.  But, to take account for iframe focus and to allow for shared iframey games reasons, could these bullets be rewritten to read something like ... 

* Joystick objects should not update when a page doesn't have focus.
* Iframes who have Joystick objects should not see their Joystick objects update when that iframe doesn't have focus.
* Topmost windows receive updates when Joystick objects are registered to that window, and iframes of same origin in the page can inherit those objects and also receive the same updates when the iframe is focused.
* Iframes not of same origin as their parent window that have Joystick objects shall not receive updates unless the parent windows (all the way to topmost) have not also registered for joystick updates.
  * An iframe who previously registered for and received Joystick object updates shall be silently cutoff from updates if their their not-same origin parent windows (all the way to topmost) ever registers for Joystick object updates.

In other words, joystick updates are shared as long as there is same origin and focus.  Typical scenario: force user to first mouse click on iframe to give it joystick focus, just like keyboard.  A lot of flash games work this way, explicitly asking you to click once to get past the splash screen - just a trick to bait the user into focusing the flash object.  

Otherwise, if there is not same origin, focused iframes may receive joystick updates only until a not-same origin parent window claims dibs.

	David Humphrey (:humph)
	Comment 100 • 13 years ago

> And am I reading these bullets right ...
> 
> > * Joystick objects should not update when a page doesn't have focus.
> > * Only iframes with same origin should be able to get the joystick events/data.
> 
> ... that iframes who register for joystick events independent of his parent
> window, but whose origin differs from the parent, shall *never* receive
> joystick messages?  Restated, joystick events flow from topmost window down
> only and the flowing stops when not same origin?  
> 
> If so, I vaguely understand the security motivations behind such a design
> decision.  But, to take account for iframe focus and to allow for shared
> iframey games reasons, could these bullets be rewritten to read something
> like ... 
> 
> * Joystick objects should not update when a page doesn't have focus.
> * Iframes who have Joystick objects should not see their Joystick objects
> update when that iframe doesn't have focus.
> * Topmost windows receive updates when Joystick objects are registered to
> that window, and iframes of same origin in the page can inherit those
> objects and also receive the same updates when the iframe is focused.
> * Iframes not of same origin as their parent window that have Joystick
> objects shall not receive updates unless the parent windows (all the way to
> topmost) have not also registered for joystick updates.
>   * An iframe who previously registered for and received Joystick object
> updates shall be silently cutoff from updates if their their not-same origin
> parent windows (all the way to topmost) ever registers for Joystick object
> updates.
> 
> In other words, joystick updates are shared as long as there is same origin
> and focus.  Typical scenario: force user to first mouse click on iframe to
> give it joystick focus, just like keyboard.  A lot of flash games work this
> way, explicitly asking you to click once to get past the splash screen -
> just a trick to bait the user into focusing the flash object.  
> 
> Otherwise, if there is not same origin, focused iframes may receive joystick
> updates only until a not-same origin parent window claims dibs.

CC'ing Dan so he can clarify what was meant re: joystick events and iframe origin checks.

	(not currently active) Ted Mielczarek Assignee
	Comment 101 • 13 years ago

Just for clarification, as currently implemented, events are fired at |window| and do not bubble.

	(not currently active) Ted Mielczarek Assignee
	Comment 102 • 13 years ago

Here's an updated patch!

	(not currently active) Ted Mielczarek Assignee
	Comment 103 • 13 years ago

Updated mac patch.

	(not currently active) Ted Mielczarek Assignee
	Comment 104 • 13 years ago

Updated Windows patch.

	(not currently active) Ted Mielczarek Assignee
	Comment 105 • 13 years ago

This is a much-updated Linux patch. I've switched to using libudev,
which means that it can now get device connected/disconnected notifications.

	(not currently active) Ted Mielczarek Assignee
	Comment 106 • 13 years ago

Comment on attachment 553531 [details] [diff] [review]
joystick.index property

This got folded into the DOM patch. Thanks Dave!

	(not currently active) Ted Mielczarek Assignee
	Comment 107 • 13 years ago

This is an updated demo page that handles device connected/disconnected events, and then polls the joystick object for changes.

	(not currently active) Ted Mielczarek Assignee
	Comment 108 • 13 years ago

Before I forget, here are the things I'd like to do before I try to get these reviewed and landed:
1) Get the patch in bug 680326 working and implement device connection/disconnection on Windows.
2) Fix the concern from the security group about joystick objects updating when a window does not have focus.
3) Rename everything in the patch from Joystick to Gamepad, since that's what we've settled on in the Web Events WG.

Anything outside of that list is likely going to be punted to a followup.

	Mozilla RelEng Bot
	Comment 109 • 13 years ago

Try run for a19029817763 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=a19029817763
Results (out of 5 total builds):
    success: 5
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/tmielczarek@mozilla.com-a19029817763

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 110 • 13 years ago

Comment on attachment 563853 [details] [diff] [review]
Add DOM Joystick events

>--- /dev/null
>+++ b/content/events/src/nsDOMJoystick.cpp
>@@ -0,0 +1,187 @@
>+NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(nsDOMJoystick)
>+  (void)tmp;

unused << tmp?

>+void nsDOMJoystick::SetConnected(bool connected)

aConnected

>+void nsDOMJoystick::SetButton(int button, PRUint8 value)

aButton, aValue. Also, size_t for aButton?

>+void nsDOMJoystick::SetAxis(int axis, float value)

Same here.

>+/* readonly attribute nsIVariant buttons; */
>+NS_IMETHODIMP nsDOMJoystick::GetButtons(nsIVariant **aButtons)
>+{
>+  nsresult rv;
>+  nsCOMPtr<nsIWritableVariant> out = do_CreateInstance(NS_VARIANT_CONTRACTID,
>+                                                       &rv);

You don't check rv here.

>+  if (mButtons.Length() == 0) {
>+    rv = out->SetAsEmptyArray();
>+  }
>+  else {

} else {

>+  NS_ADDREF(*aButtons = out);

out.swap(*aButtons) would save an addref-release.

>+NS_IMETHODIMP nsDOMJoystick::GetAxes(nsIVariant **aAxes)
>+{

Same comments. Can you factor all this duplicated code into a helper?

>--- /dev/null
>+++ b/content/events/src/nsDOMJoystick.h
>+#ifndef NSDOMJOYSTICK_H_
>+#define NSDOMJOYSTICK_H_

nsDOMJoystick_h

>--- /dev/null
>+++ b/content/events/src/nsDOMJoystickAxisMoveEvent.cpp
>+nsDOMJoystickAxisMoveEvent::nsDOMJoystickAxisMoveEvent(nsPresContext* aPresContext,
>+                                                       nsEvent* aEvent,
>+                                                       nsIDOMJoystick *aJoystick,

nsIDOMJoystick* aJoystick

>+NS_IMETHODIMP nsDOMJoystickAxisMoveEvent::GetJoystick(nsIDOMJoystick * *aJoystick)
>+{
>+  *aJoystick = mJoystick;
>+  NS_IF_ADDREF(*aJoystick);

How about

nsCOMPtr<nsIDOMJoystick> joystick = mJoystick;
joystick.forget(aJoystick);

>+NS_IMETHODIMP nsDOMJoystickAxisMoveEvent::InitJoystickAxisMoveEvent(const nsAString & typeArg, PRBool canBubbleArg, PRBool cancelableArg, nsIDOMJoystick *aJoystick, PRUint32 aAxis, float aValue)

I'm sad to see these being added now we're moving to constructors.

>+nsresult
>+NS_NewDOMJoystickAxisMoveEvent(nsIDOMEvent** aInstancePtrResult,
>+                               nsPresContext* aPresContext,
>+                               class nsEvent *aEvent,
>+                               nsIDOMJoystick *aJoystick,
>+                               PRUint32 aAxis,
>+                               float aValue)
>+{
>+  printf("NS_NewDOMJoystickAxisMoveEvent\n");

Ahem.

>+  nsDOMJoystickAxisMoveEvent* event = new nsDOMJoystickAxisMoveEvent(aPresContext,
>+                                                                     aEvent,
>+                                                                     aJoystick,
>+                                                                     aAxis,
>+                                                                     aValue);
>+  return CallQueryInterface(event, aInstancePtrResult);

Shouldn't QI here.

>--- /dev/null
>+++ b/content/events/src/nsDOMJoystickButtonEvent.cpp

Same.

>--- /dev/null
>+++ b/content/events/src/nsDOMJoystickConnectionEvent.cpp

Same.

>--- a/content/events/src/nsEventListenerManager.cpp
>+++ b/content/events/src/nsEventListenerManager.cpp
>+#ifdef MOZ_JOYSTICK
>+  } else if (aType >= NS_MOZJOYSTICK_START &&
>+             aType <= NS_MOZJOYSTICK_END) {
>+    nsPIDOMWindow* window = GetInnerWindowForTarget();
>+    if (window)
>+      window->SetHasJoystickEventListener();

{}

>--- a/dom/base/nsGlobalWindow.cpp
>+++ b/dom/base/nsGlobalWindow.cpp
>+nsGlobalWindow::EnableJoystickUpdates()
>+{
>+    printf("nsGlobalWindow::EnableJoystickUpdates\n");

Ahem.

>+nsGlobalWindow::DisableJoystickUpdates()
>+{
>+    printf("nsGlobalWindow::DisableJoystickUpdates\n");

.

>--- a/dom/base/nsGlobalWindow.h
>+++ b/dom/base/nsGlobalWindow.h
>+  // Indicates whether this window is getting joystick input events
>+  PRPackedBool           mHasJoystick : 1;

bool, of course.

>--- /dev/null
>+++ b/dom/interfaces/events/nsIDOMJoystick.idl
>+  // An identifier, unique per device.

Please use javadoc.

>--- /dev/null
>+++ b/dom/interfaces/events/nsIDOMJoystickAxisMoveEvent.idl

>\ No newline at end of file

>--- /dev/null
>+++ b/dom/interfaces/events/nsIDOMJoystickButtonEvent.idl

>\ No newline at end of file

>--- /dev/null
>+++ b/dom/system/JoystickService.cpp
>+#include "nsAutoPtr.h"
>+#include "nsFocusManager.h"
>+#include "nsIDOMEvent.h"
>+#include "nsIDOMDocument.h"
>+#include "nsIDOMEventTarget.h"
>+#include "nsDOMJoystick.h"
>+#include "nsIDOMJoystickButtonEvent.h"
>+#include "nsIDOMJoystickAxisMoveEvent.h"
>+#include "nsIDOMJoystickConnectionEvent.h"
>+#include "nsIDOMWindow.h"
>+#include "nsIPrivateDOMEvent.h"
>+#include "nsIServiceManager.h"
>+#include "nsITimer.h"
>+#include "JoystickService.h"

This should go first.

>+JoystickService::AddListener(nsIDOMWindow *aWindow)
>+{
>+  PRUint32 data;
>+  if (mListeners.Get(aWindow, &data))
>+    return; // already exists

{}

>+JoystickService::RemoveListener(nsIDOMWindow *aWindow)
>+{
>+  PRUint32 data;
>+  if (!mListeners.Get(aWindow, &data))
>+    return; // doesn't exist

{}

>+JoystickService::AddJoystick(const char* id, int numButtons, int numAxes) {
>+  nsDOMJoystick* joy = new nsDOMJoystick(NS_ConvertUTF8toUTF16(nsDependentCString(id)),

So why does id need to be a const char*?

>+  for (int i = 0; i < mJoysticks.Length(); i++) {

PRUint32, this would warn.

>+    if (mJoysticks[i] == NULL) {

if (!mJoysticks[i]) {

>+JoystickService::RemoveJoystick(int index) {
>+    }
>+    else {

} else {

>+JoystickService::FireButtonEvent(nsIDOMDocument *domdoc,
>+JoystickService::NewAxisMoveEvent(int joyIndex, int axis, float value) {
>+JoystickService::FireAxisMoveEvent(nsIDOMDocument *domdoc,
>+JoystickService::NewConnectionEvent(int joyIndex, bool connected)
>+JoystickService::FireConnectionEvent(nsIDOMDocument *domdoc,

These look pretty similar... How about a helper?

>+JoystickService*  JoystickService::GetService() {

Double space.

>+JoystickService::WindowHasSeenJoystick(nsIDOMWindow* aWindow, int joyIndex)
>+{
>+  PRUint32 data;
>+  if (!mListeners.Get(aWindow, &data))
>+    // This window isn't even listening for joystick events.
>+    return false;

{}

>+JoystickService::SetWindowHasSeenJoystick(nsIDOMWindow* aWindow, int joyIndex, bool hasSeen)
>+{
>+  PRUint32 data;
>+  if (!mListeners.Get(aWindow, &data))
>+    // This window isn't even listening for joystick events.
>+    return;

{}

>+  if (hasSeen) {
>+    data |= 1 << joyIndex;
>+  }
>+  else {

} else {

>+JoystickService::GetFocusedWindow(nsIDOMWindow** aWindow)
>+{
>+  nsCOMPtr<nsIDOMWindow> focusedWindow;
>+  if (mFocusManager->GetFocusedWindow(getter_AddRefs(focusedWindow)) != NS_OK)

NS_FAILED.

>+    return false;

{}

>+  if (!outerWindow)
>+    return false;

{}

>+  nsCOMPtr<nsPIDOMWindow> innerWindow = outerWindow->GetCurrentInnerWindow();
>+  *aWindow = innerWindow;
>+  NS_IF_ADDREF(*aWindow);

  nsCOMPtr<nsIDOMWindow> innerWindow = outerWindow->GetCurrentInnerWindow();
  innerWindow.forget(aWindow);

>+  return *aWindow != NULL;

Don't compare to NULL.

>+JoystickService::TimeoutHandler(nsITimer *aTimer, void *aClosure)
>+{
>+  if (self->mListeners.Count() == 0) {
>+    self->Shutdown();
>+    if (self->mJoysticks.Length() != 0) {

!self->mJoysticks.IsEmpty()

>+JoystickService::StartCleanupTimer()
>+{
>+  if (mTimer)
>+    mTimer->Cancel();

{}

>+  if (mTimer)
>+    mTimer->InitWithFuncCallback(TimeoutHandler,
>+                                 this,
>+                                 kCleanupDelayMS,
>+                                 nsITimer::TYPE_ONE_SHOT);

{}

>--- /dev/null
>+++ b/dom/system/JoystickService.h
>+#ifndef MOZILLA_DOM_JOYSTICKSERVICE_H_
>+#define MOZILLA_DOM_JOYSTICKSERVICE_H_

mozilla_dom_JoystickService_h

>--- a/dom/system/Makefile.in
>+++ b/dom/system/Makefile.in
>+EXPORTS_NAMESPACES = mozilla/dom
>+EXPORTS_mozilla/dom = \
>+  JoystickService.h \
>+  $(NULL)

ifdefs

	(not currently active) Ted Mielczarek Assignee
	Comment 111 • 13 years ago

Thanks for the comments!

(In reply to Ms2ger from comment #110)
> >--- /dev/null
> >+++ b/content/events/src/nsDOMJoystick.cpp
> >@@ -0,0 +1,187 @@
> >+NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(nsDOMJoystick)
> >+  (void)tmp;
> 
> unused << tmp?

I think this is actually just leftover code from when we were trying to use a typed array for buttons/axes.

> >+NS_IMETHODIMP nsDOMJoystickAxisMoveEvent::InitJoystickAxisMoveEvent(const nsAString & typeArg, PRBool canBubbleArg, PRBool cancelableArg, nsIDOMJoystick *aJoystick, PRUint32 aAxis, float aValue)
> 
> I'm sad to see these being added now we're moving to constructors.

I admit that I've just been cargo-culting here. I heard about constructors vs. init methods in the Web Events call, but I don't really know anything about it. Do you have any examples you can point me at?

> >--- a/dom/base/nsGlobalWindow.cpp
> >+++ b/dom/base/nsGlobalWindow.cpp
> >+nsGlobalWindow::EnableJoystickUpdates()
> >+{
> >+    printf("nsGlobalWindow::EnableJoystickUpdates\n");
> 
> Ahem.

Yeah, my patches are littered with debug printfs currently. I'll strip those all out.

> >+JoystickService::AddJoystick(const char* id, int numButtons, int numAxes) {
> >+  nsDOMJoystick* joy = new nsDOMJoystick(NS_ConvertUTF8toUTF16(nsDependentCString(id)),
> 
> So why does id need to be a const char*?

I guess it could be an nsACString, it didn't particularly matter. All the callers are going to be dealing with char*, so it seemed silly to push UTF16 conversion down to them.

	(not currently active) Ted Mielczarek Assignee
	Comment 112 • 13 years ago

w.r.t. the init*Event methods, we discussed this on IRC a bit. There aren't any examples of event constructors in the tree, so I'm going to leave the init methods in the patch, but make them [noscript], and punt on script-accessible constructors. (Which matches what the Web Events WG is doing for touch events.)

	(not currently active) Ted Mielczarek Assignee
	Comment 113 • 13 years ago

I wrote down some notes about my experiences with hooking up various game controllers to various operating systems:
https://wiki.mozilla.org/Gamepads:Notes

If anyone has anything to add there, feel free.

	(not currently active) Ted Mielczarek Assignee
	Comment 114 • 13 years ago

This patch renames everything from joystick -> gamepad. This means the events and event properties have been renamed as well, so existing demos will break. Bummer!

I also cleaned up all the things Ms2ger commented on, except for the code duplication comments (I haven't gotten to that level of refactoring yet), and one or two things that he suggested that I couldn't get to actually compile.

	(not currently active) Ted Mielczarek Assignee
	Comment 115 • 13 years ago

Mostly just cleanup + renaming here.

	(not currently active) Ted Mielczarek Assignee
	Comment 116 • 13 years ago

I've done some cleanup and renaming in this patch, and also used the notification in the patch in bug 680326 to support device connection/disconnection events on Windows. (finally!)

	(not currently active) Ted Mielczarek Assignee
	Comment 117 • 13 years ago

Mostly just cleanup + renaming in the Linux patch, although I did fix a possible crash bug as well.

	(not currently active) Ted Mielczarek Assignee
	Comment 118 • 13 years ago

This is the same demo page, updated to use the new event/property names.

	Mozilla RelEng Bot
	Comment 119 • 13 years ago

Try run for 87dbd17cad7c is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=87dbd17cad7c
Results (out of 5 total builds):
    success: 5
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/tmielczarek@mozilla.com-87dbd17cad7c

	David Woldrich
	Comment 120 • 13 years ago

(In reply to Ted Mielczarek [:ted, :luser] from comment #118)

Ted, I'm testing with your latest trybuild on my local (Windows 7, 64-bit).  The connected/disconnected events appear to be mostly working in windows, yay!

I am noticing two things:

1) I am only seeing the MozGamepadConnected event sent to the window after a button on the gamepad is pressed or an axis changes.  If there was a corresponding MozGamepadButtonDown event generated, I think that that event is getting eaten under the hood, because I am never seeing that show up in my test.  I assume this button-press-to-complete-activation is temporary semantics, just mentioning it in case you hadn't seen that.  

I am registered for MozGamepadConnected in onReady, so I was hoping to see the connection events generated onLoad.  But as I read in the JoystickAPI page, you will not send those events unless a button is pressed.  "If a joystick was already connected when the page loaded, the MozJoyConnected event will be dispatched to the focused page when the user presses a button or moves an axis."

Is that part of the design for security reasons?  Not a dealbreaker in any way, but why can't I have connectivity information onLoad if my javascript has actively solicited for it?  Seeing as how my game controller cannot steal focus or do anything meaningful and controller input event acquisition happens without any notice or approval by the user, I don't see what the harm would be with sending the MozGamepadConnected event when there are eventlisteners registered.

I know it's pretty silly, but some games have creatively used plugged/unplugged events to control the game, so it'd be interesting to have those events when they actually happen.  For example, in Metal Gear Solid, you have to move your controller to different ports on the PS2 mid-battle in order to beat a boss!  :)  http://www.ehow.com/how_5121096_beat-psycho-mantis.html

2) My gamepad is getting enumerated with an id that has a trailing space.  If that's how it comes from the driver, then fine, otherwise I think it would be good if the string were trimmed before being set in the gamepad object.  My gamepad's id currently looks like this:  "428-4001-GamePad Pro USB ".

Thanks!

	(not currently active) Ted Mielczarek Assignee
	Comment 121 • 13 years ago

The connection events are pretty carefully crafted. To wit:
1) If you have controllers connected when a page first registers for connection events, no connection events will be sent until the user interacts with a particular controller.
2) The first time the user interacts with a controller while your page has focus, you should receive a connection event before any input events.
3) Once a page has received input from any controller, connecting a new controller while that page has focus will send a connection event to the page.
4) Pages will receive disconnection events for any controller that had previously sent events to the page.

The security issue here is fingerprinting. Any information we expose to pages about the number and type of connected controllers is more bits of data to uniquely identify a user. If a user has interacted with a gamepad while viewing a page, we treat that as intent, the same as we would if you typed on the keyboard or moved the mouse.

I'll look into the button event being lost, and the extra space in the gamepad ID. Neither of those are intentional.

	David Woldrich
	Comment 122 • 13 years ago

(In reply to Ted Mielczarek [:ted, :luser] from comment #121)

That fingerprinting subject is so interesting.  What you're saying makes sense, but I would go so far as to say connected joysticks are a boring datapoint compared to my list of installed fonts.  I went to panopticlick, and my font list is unique to all the systems ever seen by them.  Any additional data point is a data point, though, so the concern is valid.

I'm hoping you've discussed this point to death, but I can come up with three potential options to widen what we disclose by default so that our programs can have plugged information prior to user registering intent.  

1) Make controller fingerprinting-by-default an opt-out setting with a default of allowing it.  Meaning, tracking-conscious users can uncheck auto-enumeration of gamepads in the security section.  

2) After a user has registered intent on any gamepad, all known plugged/unplugged gamepads are enumerated.  Furthermore, any subsequent plugged and unplugged events are broadcast to listeners for that window until unload.  Most games have a "press button to continue" from their attract mode/title screen, so this one would be easy to reckon with.

3) Enumerate controller ID separately from plugged/unplugged state.  So, there'd have to be a separate event for communicating the user has registered intent after the first axis or button down event fires, and the gamepad.id field would only then be populated.  As one who is trying to use the browser as a platform for building an operating environment, this is the most fair because at least I get the device presence, so I can build up my joysticks array with the correct count of devices even if it isn't fully initialized.

Though option 2 is probably the only passable one I've named, I also like option 3 because it enables the web developer to distinguish between a controller being plugged in (and needing to ask the user to "connect a gamepad to continue"), and the user saying "I want to play using controller #2."  

There could still be some fingerprinting that could go on with option 3 in terms disclosing the number of devices connected.  But the juicy bit about device name wouldn't be made available until the user registered intent.

	David Woldrich
	Comment 123 • 13 years ago

(In reply to Ted Mielczarek [:ted, :luser] from comment #121)

I was incorrect in my report earlier about the first button event getting eaten after re-connecting a gamepad, sorry for misreporting that as an issue.  I had lost that first button event after reconnect due to a bug in my code.

What you stated:

> 3) Once a page has received input from any controller, connecting a new
> controller while that page has focus will send a connection event to the page.

doesn't appear to be true currently for Win64, at least for existing controllers that I unplug and re-plug in.  Here's what events I see when I interact with a gamepad that's connected when firefox starts up:

I press button 1
   - MozGamepadConnected
   - MozGamepadButtonDown
   - MozGamepadButtonUp
I unplug the controller
   - MozGamepadDisconnected
I wait 10 seconds, plugin the same controller
   - (no events fire)
I wait 10 seconds, I press button 1
   - MozGamepadConnected
   - MozGamepadButtonDown
   - MozGamepadButtonUp

So, based on your #3, I should see MozGamepadConnected immediately after I re-plugin the controller without requiring a button press to fire?  If so, that'd be great and addresses most of my concerns from my last comment.  Thanks!

	(not currently active) Ted Mielczarek Assignee
	Comment 124 • 13 years ago

Yeah, I noticed this as well. I think it's a bug in my current implementation. If you have one controller connected and you connect a second you should see the connection event. If you disconnect all controllers and then connect one it doesn't work. I think that's just a bug.

I realize that the security aspects of this are annoying, but you would have to make a much more persuasive argument to change my mind on it. We're certainly not going to make it a preference, with the fingerprinting on by default. Your #2, where we enumerate all other controllers once one is connected is interesting, and a possibility.

	David Woldrich
	Comment 125 • 13 years ago

(In reply to Ted Mielczarek [:ted, :luser] from comment #124)
> I realize that the security aspects of this are annoying, but you would have
> to make a much more persuasive argument to change my mind on it. We're
> certainly not going to make it a preference, with the fingerprinting on by
> default. 

Yes, the era of peace and love in internet design was long ago.  I knew that opt-out idea of was a long shot.  ;)

> Your #2, where we enumerate all other controllers once one is
> connected is interesting, and a possibility.

I know of very few use cases (I think mine being a rare exception) where a gamepad-enabled program wouldn't ask a user to "press start" or "press a joystick button" to get past their attract mode or title screen.  If you were to enumerate all known devices at that time, then that enables designs where the game knows how many potential controllers/players there are and that information can be reflected as lit-but-empty player slots in a subsequent matchmaking or game session enrollment screen.

So, yes, if any gamepad intent would cause all connected gamepads to enumerate (my #2), I think you've got the 90% solution there.  Please do that?(!!)

Ok, so let me try to sell you on my #3 idea...

The sweetener for my requirements would be to get at least a known controller count with connected states, but with NO information given on axis count, button count, or ID when no user intent has yet been registered on a gamepad.  

In my #2, I'm suggesting that we separate the connect/disconnect event from the communication of metadata change event.  Just revealing the known gamepad count and connected states is an additional data point for fingerprinting, but a pretty weak one, probably just a number between 0 and 2 for most computers.  

I don't know if there are any polymorphic devices out there (surfaces on the gamepad can change dynamically after the connected - perhaps programmable macros and such?) but if there are, that's another reason to separate device connected/disconnected from the device metadata change messages.

Finally, the product I'm developing is supposed to be a learning tool for computer programming; having those additional device states of "connected but unenrolled" and "unconnected and unenrolled" would be very thought-provoking and could help young knuckleheads gain insights into esoteric system states and spur discussions about designs that incorporate user security/privacy and whatnot.  My system is all about working with numeric codes (we're bringing the coding back into ... erm, coding.)  Extra status codes are extra juicy nuggets for me.  ;)

	Romain Pouclet
	Comment 126 • 13 years ago

How exactly would that work?  I can't tell you how much I dream about a webapp using my wiimote

	Yannick INIZAN
	Comment 127 • 13 years ago

I have tested my controller under linux with firefox (Thrustmaster dual analog 3) Unfortunately this doesn't work.

	Scott Graham
	Comment 128 • 13 years ago

fyi: to attempt to mitigate mappings mess, I did some default maps here: http://goo.gl/dO69n. If there's a "nice map" then the .id field is updated to include "STANDARD GAMEPAD" per http://goo.gl/f4RPU. Otherwise it's left as usb prod/vend id. It's definitely not a 100% solution, but I think it's a relatively low cost improved default.

	(not currently active) Ted Mielczarek Assignee
	Comment 129 • 12 years ago

Rebased! and cleaned up! and the start of Mochitests!

This patch also fixes one large behavior issue, which is that all visible
windows now receive gamepad input. This means that events in frames work,
as well as if you have multiple browser windows simultaneously visible.

There's still one more behavior issue to fix there, which is that gamepad
state isn't synced if you make a non-visible window visible. You can see
this by opening the demo page, pressing a gamepad button, then switching tabs
and releasing the button. The page will still show the button as being
pressed. This shouldn't be super hard to fix, I'll get that soon.

	(not currently active) Ted Mielczarek Assignee
	Comment 130 • 12 years ago

Rebased.

	(not currently active) Ted Mielczarek Assignee
	Comment 131 • 12 years ago

Rebased.

	(not currently active) Ted Mielczarek Assignee
	Comment 132 • 12 years ago

Rebased.

	(not currently active) Ted Mielczarek Assignee
	Comment 133 • 12 years ago

I think this is now in a reviewable state. I fixed the last remaining issue that I was aware of, which was that background tabs didn't have their gamepad objects synced to the current state when they became visible. I added a test for that behavior. I also found that I was hitting a shutdown crash in the mochitests, so I fixed that.

This could definitely use some more tests, but this is a start.

	(not currently active) Ted Mielczarek Assignee
	Comment 134 • 12 years ago

Comment on attachment 592772 [details] [diff] [review]
Add DOM Gamepad APIs

Since this is somewhat event-heavy, Olli, can you review it? If you want someone else to take a look, let me know.

	(not currently active) Ted Mielczarek Assignee
	Comment 135 • 12 years ago

I need to find reviewers for the platform backends, if anyone has suggestions I'd appreciate it.

	Scott Graham
	Comment 136 • 12 years ago

Excellent, looking forward to seeing this land. The only comment I have is that it'd be nice to have the navigator.gamepads (navigator.mozGamepads) access point available.

	(not currently active) Ted Mielczarek Assignee
	Comment 137 • 12 years ago

After some IRC discussion, I've reworked things slightly to put the platform-specific backends under hal/ instead of dom/system. The DOM glue code still lives in system, and the DOM and content parts of this patch are virtually unchanged.

Adding cjones for review on the hal bits.

	(not currently active) Ted Mielczarek Assignee
	Comment 138 • 12 years ago

Mac backend, implemented as part of hal.

smichaud graciously agreed to review the mac backend.

	(not currently active) Ted Mielczarek Assignee
	Comment 139 • 12 years ago

The Windows backend, similarly updated and moved to hal.

I cajoled Bas into reviewing this.

	(not currently active) Ted Mielczarek Assignee
	Comment 140 • 12 years ago

And last but not least, the Linux backend.

Karl let me know that if this was the last patch on earth he would review it. Close enough.

	(not currently active) Ted Mielczarek Assignee
	Comment 141 • 12 years ago

(In reply to Scott Graham from comment #136)
> Excellent, looking forward to seeing this land. The only comment I have is
> that it'd be nice to have the navigator.gamepads (navigator.mozGamepads)
> access point available.

I punted navigator.gamepads to bug 690935, so as not to block my work here. I might be able to get that working and get it landed after this (but in the same release cycle, hopefully). A friend of mine complained recently about the non-overlap of our implementations: mine lacking navigator.gamepads and yours lacking connected/disconnected events. That is unfortunate. :-/

	Mozilla RelEng Bot
	Comment 142 • 12 years ago

Try run for 25207b2975b2 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=25207b2975b2
Results (out of 29 total builds):
    success: 25
    warnings: 2
    failure: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/tmielczarek@mozilla.com-25207b2975b2

	Scott Graham
	Comment 143 • 12 years ago

(In reply to Ted Mielczarek [:ted, :luser] from comment #141)
> (In reply to Scott Graham from comment #136)
> > Excellent, looking forward to seeing this land. The only comment I have is
> > that it'd be nice to have the navigator.gamepads (navigator.mozGamepads)
> > access point available.
> 
> I punted navigator.gamepads to bug 690935, so as not to block my work here.
> I might be able to get that working and get it landed after this (but in the
> same release cycle, hopefully). A friend of mine complained recently about
> the non-overlap of our implementations: mine lacking navigator.gamepads and
> yours lacking connected/disconnected events. That is unfortunate. :-/

Yeah, that sucks. How come you did the more difficult one first? ;) Seriously though, I will try to get the event version sorted out relatively soon for webkit.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 144 • 12 years ago

Comment on attachment 593928 [details] [diff] [review]
Add DOM Gamepad APIs

You should be using the new license header in new files.

The hal stuff looks pretty trivial ... I guess implementations are coming in later patches?

r=me for hal stuff

	(not currently active) Ted Mielczarek Assignee
	Comment 145 • 12 years ago

Yeah, you can see the other patches on this bug for the actual hal implementations. I tagged platform experts for review there, you're welcome to take a look if you'd like.

	Rob Hawkes [:rob]
	Comment 146 • 12 years ago

I've added my gamepad visualisation demo to GitHub for others to play with and get a sense of what's happening with their gamepad. I've also added it to the wiki.

http://robhawkes.github.com/gamepad-demo/

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 147 • 12 years ago

Comment on attachment 593928 [details] [diff] [review]
Add DOM Gamepad APIs

> CPPSRCS		= \
> 		nsEventListenerManager.cpp \
> 		nsEventStateManager.cpp \
> 		nsDOMEvent.cpp \
> 		nsDOMDataContainerEvent.cpp \
> 		nsDOMUIEvent.cpp \
> 		nsDOMKeyboardEvent.cpp \
> 		nsDOMTextEvent.cpp \
> 		nsDOMMouseEvent.cpp \
> 		nsDOMMouseScrollEvent.cpp \
>@@ -92,20 +93,29 @@
> 		nsDOMTransitionEvent.cpp \
> 		nsDOMAnimationEvent.cpp \
> 		nsDOMPopStateEvent.cpp \
> 		nsDOMHashChangeEvent.cpp \
> 		nsDOMCloseEvent.cpp \
> 		nsDOMTouchEvent.cpp \
> 		nsDOMCustomEvent.cpp \
> 		nsDOMCompositionEvent.cpp \
> 		$(NULL)
> 
>+ifdef MOZ_GAMEPAD
>+CPPSRCS += \
>+  nsDOMGamepad.cpp \
>+  nsDOMGamepadButtonEvent.cpp \
>+  nsDOMGamepadAxisMoveEvent.cpp \
>+  nsDOMGamepadConnectionEvent.cpp \
>+  $(NULL) 
>+endif
This would be easier to read if same indentation was used as above.


>+#ifdef MOZ_GAMEPAD
>+  "MozGamepadButtonDown",
>+  "MozGamepadButtonUp",
>+  "MozGamepadAxisMove",
>+  "MozGamepadConnected",
>+  "MozGamepadDisconnected",
>+#endif
>+/* readonly attribute DOMString id; */
>+NS_IMETHODIMP nsDOMGamepad::GetId(nsAString & aID)
NS_IMETHODIMP
nsDOMGamepad::GetId(nsAString & aID)
Same also elsewhere.

>+NS_IMETHODIMP nsDOMGamepad::GetButtons(nsIVariant** aButtons)
>+{
>+  nsresult rv;
>+  nsCOMPtr<nsIWritableVariant> out = do_CreateInstance(NS_VARIANT_CONTRACTID,
>+                                                       &rv);
Do you really need rv?
nsCOMPtr<nsIWritableVariant> out = do_CreateInstance(NS_VARIANT_CONTRACTID);
NS_ENSURE_STATE(out);
should be ok


>+    NS_Free(array);
>+  }
>+  if (NS_FAILED(rv))
>+    return rv;
NS_ENSURE_SUCCESS(rv, rv);

>+  
>+  return CallQueryInterface(out, aButtons);
Maybe 
*aButtons = out.forget().get();
to reduce extra addref/release/QI

The following method has same problems. Skipping it here.



>+nsDOMGamepad::SyncState(nsDOMGamepad* other)
>+{
>+  if (mButtons.Length() != other->mButtons.Length() ||
>+      mAxes.Length() != other->mAxes.Length())
>+    return;
if (expr) {
  stmt;
}


>+
>+already_AddRefed<nsDOMGamepad>
>+nsDOMGamepad::Clone() {
{ should be in the next line


>+++ b/content/events/src/nsDOMGamepad.h
>@@ -0,0 +1,80 @@
Use the new license 



>+++ b/content/events/src/nsDOMGamepadAxisMoveEvent.cpp
>@@ -0,0 +1,126 @@
>+/* ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+ *
ditto


>+/* void initGamepadAxisMoveEvent (in DOMString typeArg, in boolean canBubbleArg, in boolean cancelableArg, in nsIDOMGamepad gamepad, in unsigned long axis, in float value); */
over long line.


>+NS_IMETHODIMP
>+nsDOMGamepadAxisMoveEvent::InitGamepadAxisMoveEvent(const nsAString& typeArg,
>+                                                    bool canBubbleArg,
>+                                                    bool cancelableArg,
>+                                                    nsIDOMGamepad* aGamepad,
>+                                                    PRUint32 aAxis,
>+                                                    float aValue)
>+{
Could you perhaps add support for EventCtor too:
See for example how nsIUIEventInit is used as a nsDOMUIEvent ctor dictionary.
And there is also nsDOMUIEvent::InitFromCtor and NS_DEFINE_EVENT_CTOR(UIEvent) in nsDOMClasssInfo.cpp
Since you're implementing Init*Event, adding support for EventCtor should be very easy.


>+++ b/content/events/src/nsDOMGamepadAxisMoveEvent.h
>@@ -0,0 +1,67 @@
>+/* ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
Use the new license

>+++ b/content/events/src/nsDOMGamepadButtonEvent.cpp
>@@ -0,0 +1,114 @@
>+/* ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
Ditto
>+/* void initGamepadButtonEvent (in DOMString typeArg, in boolean canBubbleArg, in boolean cancelableArg, in nsIDOMGamepad gamepad, in unsigned long button); */
>+NS_IMETHODIMP
>+nsDOMGamepadButtonEvent::InitGamepadButtonEvent(const nsAString& typeArg,
>+                                                bool canBubbleArg,
>+                                                bool cancelableArg,
>+                                                nsIDOMGamepad *aGamepad,
>+                                                PRUint32 aButton)
>+{
Again, support for EventCtor would be good. That is the new way script should initialize events.
init*Event is the old way.


>+class nsDOMGamepadButtonEvent : public nsDOMEvent,
>+                                public nsIDOMGamepadButtonEvent
>+{
>+public:
>+  nsDOMGamepadButtonEvent(nsPresContext* aPresContext, nsEvent* aEvent,
>+                          nsIDOMGamepad* aGamepad, PRUint32 aButton);
>+
>+  NS_DECL_ISUPPORTS_INHERITED
>+
>+  // Forward to base class
>+  NS_FORWARD_NSIDOMEVENT(nsDOMEvent::)
>+
>+  NS_DECL_NSIDOMGAMEPADBUTTONEVENT
>+
>+private:
>+  ~nsDOMGamepadButtonEvent();
>+  nsCOMPtr<nsIDOMGamepad> mGamepad;
Does anything guarantee that mGamepad doesn't keep the event alive?
Currently, I think, nsIDOMGamepad can be implemented by JS.
Making it builtinclass would be safer. Other option is to make this Event class cycle collectable.

>   // FIXME: Should get spec to say what the right string is here!  This
>   // is probably wrong!
>   if (aEventType.LowerCaseEqualsLiteral("transitionevent"))
>     return NS_NewDOMTransitionEvent(aDOMEvent, aPresContext, nsnull);
>   if (aEventType.LowerCaseEqualsLiteral("animationevent"))
>     return NS_NewDOMAnimationEvent(aDOMEvent, aPresContext, nsnull);
>   if (aEventType.LowerCaseEqualsLiteral("popstateevent"))
>     return NS_NewDOMPopStateEvent(aDOMEvent, aPresContext, nsnull);
>   if (aEventType.LowerCaseEqualsLiteral("mozaudioavailableevent"))
>     return NS_NewDOMAudioAvailableEvent(aDOMEvent, aPresContext, nsnull);
>+#ifdef MOZ_GAMEPAD
>+  if (aEventType.LowerCaseEqualsLiteral("mozgamepadbuttonevent"))
>+    return NS_NewDOMGamepadButtonEvent(aDOMEvent, aPresContext, nsnull);
>+  if (aEventType.LowerCaseEqualsLiteral("mozgamepadaxismoveevent"))
>+    return NS_NewDOMGamepadAxisMoveEvent(aDOMEvent, aPresContext, nsnull);
>+  if (aEventType.LowerCaseEqualsLiteral("mozgamepadconnectionevent"))
>+    return NS_NewDOMGamepadConnectionEvent(aDOMEvent, aPresContext, nsnull);
>+#endif
Please add these to the end of the list

>+already_AddRefed<nsDOMGamepad>
>+nsGlobalWindow::GetGamepad(PRUint32 aIndex)
>+{
>+  nsRefPtr<nsDOMGamepad> gamepad;
>+  if (mGamepads.Get(aIndex, getter_AddRefs(gamepad)))
>+    return gamepad.forget();
if (expr) {
  stmt;
}

>+#ifdef MOZ_GAMEPAD
>+  nsRefPtrHashtable<nsUint32HashKey, nsDOMGamepad> mGamepads;
So, should these be added to cycle collection.
I'm not sure whether nsDOMGamepads can keep other stuff alive, and cause a cycle.


>+[scriptable, uuid(bd09eef8-8e07-4baf-8d39-4f92003dbca8)]
>+interface nsIDOMGamepadAxisMoveEvent : nsIDOMEvent
nsIDOMMoz*

>+[scriptable, uuid(d75d4d2b-e7b4-4b93-ac35-2e70b57d9b28)]
>+interface nsIDOMGamepadButtonEvent : nsIDOMEvent
ditto


>+[scriptable, uuid(ff13acd9-11da-4817-8f2a-4a5700dfd13e)]
>+interface nsIDOMGamepad : nsISupports {
{ should be in the next line.
And I think nsIDOMGamepad should be builtinclass, as I mentioned earlier.


>+[scriptable, uuid(7edf77a2-6b3e-4bbb-9100-4452d425feaa)]
>+interface nsIDOMGamepadServiceTest : nsISupports {
{ next line

>+class DestroyGamepadServiceEvent : public nsRunnable {
{ next line

>+public:
>+  DestroyGamepadServiceEvent() {}
>+
>+  NS_IMETHOD Run() {
>+    GamepadService::DestroyService();
>+    return NS_OK;
>+  }
>+};
>+
>+class ShutdownObserver : public nsIObserver {
ditto, and also elsewhere

>+GamepadService::GamepadService()
>+  : mStarted(false),
>+    mShuttingDown(false),
>+    mFocusManager(do_GetService(FOCUSMANAGER_CONTRACTID)),
>+    mObserver(new ShutdownObserver())
>+{
>+}
Hmm, is this not linked to libxul?
You should access focusmanager using
nsFocusManager::GetFocusManager();

I need to continue here..

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 148 • 12 years ago

Comment on attachment 593928 [details] [diff] [review]
Add DOM Gamepad APIs

>+GamepadService::NewButtonEvent(uint32_t index, uint32_t button, bool pressed) {
>+  if (mShuttingDown || index >= mGamepads.Length()) {
>+    return;
>+  }
>+
>+  mGamepads[index]->SetButton(button, pressed ? 1 : 0);
>+
>+  for (uint32_t i = mListeners.Length(); i > 0 ; ) {
>+    --i;
>+
>+    // Only send events to non-background windows
>+    if (!mListeners[i]->GetOuterWindow() ||
>+        mListeners[i]->GetOuterWindow()->IsBackground())
>+      continue;
>+
>+    if (!WindowHasSeenGamepad(mListeners[i], index)) {
>+      SetWindowHasSeenGamepad(mListeners[i], index);
>+      // This window hasn't seen this gamepad before, so
>+      // send a connection event first.
>+      NewConnectionEvent(index, true);
>+    }
>+
>+    nsRefPtr<nsDOMGamepad> gamepad = mListeners[i]->GetGamepad(index);
>+    nsCOMPtr<nsIDOMDocument> domdoc;
>+    mListeners[i]->GetDocument(getter_AddRefs(domdoc));
>+
>+    if (domdoc && gamepad) {
>+      gamepad->SetButton(button, pressed ? 1 : 0);
>+      // Fire event
>+      FireButtonEvent(domdoc, mListeners[i], gamepad, button, pressed);
>+    }
So what if some event listeners ends up closing several windows.
Next iteration might index out-of-bounds of mListeners 
Same also elsewhere.


>+void
>+GamepadService::FireAxisMoveEvent(nsIDOMDocument* domdoc,
>+                                  nsIDOMEventTarget* target,
>+                                  nsDOMGamepad* gamepad,
>+                                  uint32_t axis,
>+                                  float value)
Parameter names should be
aDomdoc, aTarget etc.


I'd like to see a new patch, and I could review that one.

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 149 • 12 years ago

(In reply to Olli Pettay [:smaug] from comment #147)
> >+ifdef MOZ_GAMEPAD
> >+CPPSRCS += \
> >+  nsDOMGamepad.cpp \
> >+  nsDOMGamepadButtonEvent.cpp \
> >+  nsDOMGamepadAxisMoveEvent.cpp \
> >+  nsDOMGamepadConnectionEvent.cpp \
> >+  $(NULL) 
> >+endif
> This would be easier to read if same indentation was used as above.

I approve of using two spaces :)

> >+NS_IMETHODIMP nsDOMGamepad::GetButtons(nsIVariant** aButtons)
> >+{
> >+  nsresult rv;
> >+  nsCOMPtr<nsIWritableVariant> out = do_CreateInstance(NS_VARIANT_CONTRACTID,
> >+                                                       &rv);
> Do you really need rv?
> nsCOMPtr<nsIWritableVariant> out = do_CreateInstance(NS_VARIANT_CONTRACTID);
> NS_ENSURE_STATE(out);
> should be ok

How about "new nsVariant();"?

> >+  
> >+  return CallQueryInterface(out, aButtons);
> Maybe 
> *aButtons = out.forget().get();
> to reduce extra addref/release/QI

"out.forget(aButtons);" should work.

	Karl Tomlinson (:karlt)
	Comment 150 • 12 years ago

Comment on attachment 593932 [details] [diff] [review]
Linux gamepad backend

Is the separate thread necessary to empty the joystick event queue fast
enough?

There are some race conditions here and monitoring the file descriptors on the
main thread would help.  A separate thread may also be more heavy handed than
necessary.

>+  GamepadChangeEvent(Gamepad& gamepad,
>+                     Type type) : mGamepad(gamepad),
>+                                  mIndex(gamepad.index),
>+                                  mType(type) {
>+  }
>+
>+  NS_IMETHOD Run() {
>+    GamepadService* gamepadsvc = GamepadService::GetService();
>+    if (mType == Added) {
>+      mGamepad.index = gamepadsvc->AddGamepad(mGamepad.idstring,
>+                                              (int)mGamepad.numButtons,
>+                                              (int)mGamepad.numAxes);

Gamepad::index is required (for remove events) when the change event is
created on the udev thread, but it is not set until the change event for the
add event has run on the main thread.

>+      mGamepads.RemoveElementAt(i);

This invalidates references in the event queue to this Gamepad and any after
this in the array (at least).

I don't know whether Gecko APIs provide support for monitoring file
descriptors on the event loop, but these GLib functions would provide what is
necessary.  (This code would be better not used for the Qt port.)

http://developer.gnome.org/glib/2.30/glib-IO-Channels.html#g-io-channel-unix-new
http://developer.gnome.org/glib/2.30/glib-IO-Channels.html#g-io-add-watch

r- mainly for that, but some minor points below.

>+  virtual ~LinuxGamepadService() {

No need for this to be virtual.

>+    snprintf(gamepad.idstring, sizeof(gamepad.idstring),
>+             "%s-%s-%s",
>+             mUdev.udev_device_get_property_value(dev,"ID_VENDOR_ID"),
>+             mUdev.udev_device_get_property_value(dev, "ID_MODEL_ID"),

Can we be sure these properties won't be null?

>+    mGamepads.AppendElement(gamepad);
>+    // Inform the GamepadService
>+    nsRefPtr<GamepadChangeEvent> event =
>+      new GamepadChangeEvent(mGamepads[mGamepads.Length() - 1],
>+                             GamepadChangeEvent::Added);

Passing the result of AppendElement() to the GamepadChangeEvent constructor
would save having to do the mGamepads[mGamepads.Length() - 1] math.

>+  self->mUdev.udev_monitor_filter_add_match_subsystem_devtype(monitor,
>+                                                              "input", NULL);

Do joystick devices have a specific devtype?

>+  if (!mUdev.udev_device_get_property_value(dev, "ID_INPUT_JOYSTICK"))
>+    return false;
>+  
>+  const char* devpath = mUdev.udev_device_get_devnode(dev);
>+  if (!devpath) {
>+    return false;
>+  }
>+  if (strncmp(kJoystickPath, devpath, sizeof(kJoystickPath) - 1) != 0) {
>+    return false;
>+  }

Is the last test to check that this is not a generic eventN device for the
joystick?

KERNEL=="js[0-9]*",             NAME="input/%k"
is under the heading "naming rules for kernels <2.6.31" in
30-kernel-compat.rules but I don't know exactly what that means.

>+namespace mozilla {
>+namespace hal_impl {
>+
>+LinuxGamepadService* gService = NULL;

Please use internal linkage for gService.

>+void StartMonitoringGamepadStatus()
>+{
>+  if (gService)
>+    return;

>+void StopMonitoringGamepadStatus()
>+{
>+  if (!gService)
>+    return;

I suspect these should just be assertions.

> namespace udev {

I wonder what this provides, given every identifier already begins with "udev"?

>  operator bool() {
>    return lib && udev;
>  }

It is sufficient to test only udev.

>+    s = (typeof (s))dlsym(lib, #s); \

I like typeof here.  Gecko convention is no space before the parameter(s).

I guess we can make udev_lib a singleton and un-inline methods if/when this is
used by more than one client.

LoadSymbols() and lib should be private (or protected) to udev_lib.

	David Humphrey (:humph)
	Comment 151 • 12 years ago

Ted,

From irc, Steven is crashing in mochitests with this:

[Thread 0x7fffe35ff700 (LWP 12417) exited]
Assertion failure: thred != NULL, at /home/cwtseng/hgRepo/src/nsprpub/pr/src/pthreads/ptthread.c:541

Program received signal SIGABRT, Aborted.
0x0000003a8a8352d5 in raise () from /lib64/libc.so.6
Missing separate debuginfos, use: debuginfo-install GConf2-2.32.3-1.fc15.x86_64 ORBit2-2.14.19-2.fc15.x86_64 alsa-lib-1.0.24-2.fc15.x86_64 atk-2.0.0-1.fc15.x86_64 avahi-glib-0.6.30-3.fc15.x86_64 avahi-libs-0.6.30-3.fc15.x86_64 cairo-1.10.2-3.fc15.x86_64 dbus-glib-0.92-2.fc15.x86_64 dbus-libs-1.4.6-5.fc15.x86_64 expat-2.0.1-11.fc15.x86_64 fontconfig-2.8.0-3.fc15.x86_64 freetype-2.4.4-7.fc15.x86_64 gamin-0.1.10-9.fc15.x86_64 gdk-pixbuf2-2.23.3-2.fc15.x86_64 glib2-2.28.8-1.fc15.x86_64 glibc-2.14-5.x86_64 gnome-vfs2-2.24.4-5.fc15.x86_64 gtk2-2.24.7-3.fc15.x86_64 gtk2-engines-2.20.2-2.fc15.x86_64 gvfs-1.8.2-1.fc15.x86_64 ibus-gtk2-1.4.0-11.fc15.x86_64 ibus-libs-1.4.0-11.fc15.x86_64 keyutils-libs-1.2-7.fc15.x86_64 krb5-libs-1.9.1-14.fc15.x86_64 libICE-1.0.6-3.fc15.x86_64 libSM-1.2.0-2.fc15.x86_64 libX11-1.4.3-1.fc15.x86_64 libXScrnSaver-1.2.1-2.fc15.x86_64 libXau-1.0.6-2.fc15.x86_64 libXcomposite-0.4.3-2.fc15.x86_64 libXcursor-1.1.11-3.fc15.x86_64 libXdamage-1.1.3-2.fc15.x86_64 libXext-1.2.0-2.fc15.x86_64 libXfixes-5.0-1.fc15.x86_64 libXi-1.4.3-3.fc15.x86_64 libXinerama-1.1.1-2.fc15.x86_64 libXrandr-1.3.1-2.fc15.x86_64 libXrender-0.9.6-2.fc15.x86_64 libXt-1.1.0-1.fc15.x86_64 libacl-2.2.49-11.fc15.x86_64 libart_lgpl-2.3.21-2.fc15.x86_64 libattr-2.4.44-7.fc15.x86_64 libbonobo-2.32.1-1.fc15.x86_64 libbonoboui-2.24.5-1.fc15.x86_64 libcanberra-0.28-3.fc15.x86_64 libcom_err-1.41.14-2.fc15.x86_64 libgcc-4.6.1-9.fc15.x86_64 libgcrypt-1.4.6-1.fc15.x86_64 libgnome-2.32.1-2.fc15.x86_64 libgnome-keyring-3.0.3-1.fc15.x86_64 libgnomecanvas-2.30.3-2.fc15.x86_64 libgnomeui-2.24.5-2.fc15.x86_64 libgpg-error-1.9-2.fc15.x86_64 libnotify-0.7.3-1.fc15.x86_64 libogg-1.2.2-3.fc15.x86_64 libpng-1.2.46-1.fc15.x86_64 libselinux-2.0.99-4.fc15.x86_64 libstdc++-4.6.1-9.fc15.x86_64 libtdb-1.2.9-9.fc15.x86_64 libtool-ltdl-2.4-6.fc15.x86_64 libudev-167-6.fc15.x86_64 libuuid-2.19.1-1.4.fc15.x86_64 libvorbis-1.3.2-1.fc15.x86_64 libxcb-1.7-2.fc15.x86_64 libxml2-2.7.8-6.fc15.x86_64 nss-mdns-0.10-9.fc15.x86_64 openssl-1.0.0e-1.fc15.x86_64 pango-1.28.4-1.fc15.x86_64 pixman-0.20.2-2.fc15.x86_64 popt-1.13-8.fc15.x86_64 zlib-1.2.5-5.fc15.x86_64
(gdb) bt
#0  0x0000003a8a8352d5 in raise () from /lib64/libc.so.6
#1  0x0000003a8a836beb in abort () from /lib64/libc.so.6
#2  0x00007ffff7ac3d0f in PR_Assert (s=0x7ffff7af23f9 "thred != NULL", file=0x7ffff7af2380 "/home/cwtseng/hgRepo/src/nsprpub/pr/src/pthreads/ptthread.c", ln=541)
    at /home/cwtseng/hgRepo/src/nsprpub/pr/src/io/prlog.c:587
#3  0x00007ffff7ae78b9 in PR_JoinThread (thred=0x0) at /home/cwtseng/hgRepo/src/nsprpub/pr/src/pthreads/ptthread.c:541
#4  0x00007ffff47d7ffd in (anonymous namespace)::LinuxGamepadService::Shutdown (this=0x7fffd9bc5000) at /home/cwtseng/hgRepo/src/hal/linux/LinuxGamepad.cpp:356
#5  0x00007ffff47d817f in mozilla::hal_impl::StopMonitoringGamepadStatus () at /home/cwtseng/hgRepo/src/hal/linux/LinuxGamepad.cpp:406
#6  0x00007ffff47d3440 in mozilla::hal::StopMonitoringGamepadStatus () at /home/cwtseng/hgRepo/src/hal/Hal.cpp:395
#7  0x00007ffff3c0a071 in mozilla::dom::GamepadService::BeginShutdown (this=0x7fffd7427080) at /home/cwtseng/hgRepo/src/dom/system/GamepadService.cpp:133
#8  0x00007ffff3c09f00 in mozilla::dom::(anonymous namespace)::ShutdownObserver::Observe (this=0x7fffd9b706a0, aSubject=0x0, aTopic=
    0x7ffff5583488 "xpcom-will-shutdown", aData=0x0) at /home/cwtseng/hgRepo/src/dom/system/GamepadService.cpp:103
#9  0x00007ffff480556d in nsObserverList::NotifyObservers (this=0x7fffd97ea360, aSubject=0x0, aTopic=0x7ffff5583488 "xpcom-will-shutdown", someData=0x0)
    at /home/cwtseng/hgRepo/src/xpcom/ds/nsObserverList.cpp:130
#10 0x00007ffff4806f95 in nsObserverService::NotifyObservers (this=0x7fffeb65a420, aSubject=0x0, aTopic=0x7ffff5583488 "xpcom-will-shutdown", someData=0x0)
    at /home/cwtseng/hgRepo/src/xpcom/ds/nsObserverService.cpp:182
#11 0x00007ffff47ed271 in mozilla::ShutdownXPCOM (servMgr=0x7ffff7d70338) at /home/cwtseng/hgRepo/src/xpcom/build/nsXPComInit.cpp:595
#12 0x00007ffff47ed104 in NS_ShutdownXPCOM_P (servMgr=0x7ffff7d70338) at /home/cwtseng/hgRepo/src/xpcom/build/nsXPComInit.cpp:563
#13 0x00007ffff3086a86 in ScopedXPCOMStartup::~ScopedXPCOMStartup (this=0x7fffffffb7b0, __in_chrg=<optimized out>)
    at /home/cwtseng/hgRepo/src/toolkit/xre/nsAppRunner.cpp:1112
#14 0x00007ffff308edb6 in XRE_main (argc=5, argv=0x7fffffffe088, aAppData=0x6202c0) at /home/cwtseng/hgRepo/src/toolkit/xre/nsAppRunner.cpp:3300
#15 0x00000000004022c8 in do_main (exePath=0x7fffffffce50 "/home/cwtseng/hgRepo/src/obj-gamepad-dbg/dist/bin/", argc=5, argv=0x7fffffffe088)
    at /home/cwtseng/hgRepo/src/browser/app/nsBrowserApp.cpp:205
#16 0x00000000004024d3 in main (argc=5, argv=0x7fffffffe088) at /home/cwtseng/hgRepo/src/browser/app/nsBrowserApp.cpp:295

	xavier deschuyteneer
	Comment 152 • 12 years ago

hi,

i'm running debian testing, no pad detection at all (with sixaxis).

i tried this code:
http://people.mozilla.com/~tmielczarek/udevtest.cpp

and it result with: infinite loop when trying with bluetooth and seg fault when usb.

my version of libudev: libudev0 175-3.1

lsusb: Bus 001 Device 009: ID 054c:0268 Sony Corp. Batoh Device / PlayStation 3 Controller

pad work recognized by xorg-input-joystick
kernel version: 3.2.0-1-amd64

	Rob Hawkes [:rob]
	Comment 153 • 12 years ago

What is the status on the review process? I'd love to see this land in Nightly.

	Steven Michaud [:smichaud] (Retired)
	Comment 154 • 12 years ago

I'm currently swamped, and have been since shortly after I promised to review the Mac back end.  Conceivably I could get to it next week, though ... unless something else intervenes.

	(not currently active) Ted Mielczarek Assignee
	Comment 155 • 12 years ago

I've dug out of my backlog of real-life and higher-priority tasks, and have started updating the two patches here that got r-. I should hopefully have them up for re-review within the next week or so. I'm still blocked on Steven and Bas' review of the other platform backends.

	Steven Michaud [:smichaud] (Retired)
	Comment 156 • 12 years ago

I forgot to mention it in comment #154, but I do now have joysticks I can test with.

	Bas Schouten (:bas.schouten)
	Comment 157 • 12 years ago

Comment on attachment 593931 [details] [diff] [review]
Windows gamepad backend using DirectInput

Review of attachment 593931 [details] [diff] [review]:
-----------------------------------------------------------------

I didn't review this in as much detail as I'd like to, but it looks good to me and a large part of this is just testing with different devices I believe.

::: hal/windows/WindowsGamepad.cpp
@@ +51,5 @@
> +  int numButtons;
> +  // The human-readable device name.
> +  char name[128];
> +  // The DirectInput device.
> +  LPDIRECTINPUTDEVICE8 device;

We could use nsRefPtr<IDirectInputDevice8>

@@ +69,5 @@
> +//XXX: ostensibly the values could be arbitrary degrees for a hat with
> +// full rotation, but we'll punt on that for now. This should handle
> +// 8-way D-pads exposed as POV hats.
> +static void
> +HatPosToAxes(DWORD hatPos, HatState& axes) {

Any reason we wouldn't just use sin/cos here to deal with full rotation hats?

@@ +282,5 @@
> +  nsTArray<Gamepad> mGamepads;
> +  // List of event handles used for signaling.
> +  nsTArray<HANDLE> mEvents;
> +
> +  LPDIRECTINPUT8 dinput;

Could make this nsRefPtr<IDirectInput8>

@@ +465,5 @@
> +      continue;
> +    }
> +
> +    if (i >= self->mGamepads.Length()) {
> +      // Something would be terribly wrong here.

We could probably add a comment that this means we got a WAIT_ABANDONED_x I believe.

	Rob Hawkes [:rob]
	Comment 158 • 12 years ago

Are we still in need of reviews here? I'm happy to help find people if it will speed up the process of getting this landed.

	(not currently active) Ted Mielczarek Assignee
	Comment 159 • 12 years ago

I still need a review on the mac backend, but I have review on the Windows/Linux backends and the core DOM bits, and I just need to provide updated patches.

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 160 • 12 years ago

Comment on attachment 593929 [details] [diff] [review]
Mac gamepad backend

Review of attachment 593929 [details] [diff] [review]:
-----------------------------------------------------------------

::: hal/darwin/DarwinHal.cpp
@@ +46,5 @@
> +namespace mozilla {
> +namespace hal_impl {
> +
> +void
> +Vibrate(const nsTArray<uint32>& pattern, const hal::WindowIdentifier &)

uint32_t

	(not currently active) Ted Mielczarek Assignee
	Comment 161 • 12 years ago

A few replies to review comments. Anything I haven't replied to I have fixed in my upcoming patch:

(In reply to Olli Pettay [:smaug] from comment #147)
> This would be easier to read if same indentation was used as above.

I'm trying to standardize on 2-space indent for Makefiles. It doesn't seem worth changing every Makefile in existence right now, but adding new entries should use two spaces.

> Does anything guarantee that mGamepad doesn't keep the event alive?
> Currently, I think, nsIDOMGamepad can be implemented by JS.
> Making it builtinclass would be safer. Other option is to make this Event
> class cycle collectable.

I've marked nsIDOMGamepad builtinclass, that seems sane. I talked to mccr8 and he didn't think anything needed to be cycle-collected, since nothing is holding onto a cycle-collected class.

> Hmm, is this not linked to libxul?
> You should access focusmanager using
> nsFocusManager::GetFocusManager();

It is, but I copied this from elsewhere. Looking at the code, it appears that I no longer use the focus manager anyway, so I've simply removed this.

	(not currently active) Ted Mielczarek Assignee
	Comment 162 • 12 years ago

Okay, this fixes all the review comments smaug mentioned (except for the few things I noted in my previous comment). It's also been rebased to a more recent mozilla-central revison. I fixed all the license headers to be MPL2.

	(not currently active) Ted Mielczarek Assignee
	Comment 163 • 12 years ago

Incidentally, the Gamepad spec had its first published working draft:
http://www.w3.org/TR/2012/WD-gamepad-20120529/
(doesn't really mean anything, except that the W3C intends to publish this as a spec)

	(not currently active) Ted Mielczarek Assignee
	Comment 164 • 12 years ago

Karl, thanks for the review! Just now getting around to addressing it. A few replies here:

(In reply to Karl Tomlinson (:karlt) from comment #150)
> Is the separate thread necessary to empty the joystick event queue fast
> enough?

It may not be, it just seemed like the simplest approach at the time. I'll try out the glib IO channel stuff and see how it works. Not being glib/gtk dependent would be nice, but it's probably not terribly important (until I want to make this work on Android or B2G...) 

> >+             mUdev.udev_device_get_property_value(dev,"ID_VENDOR_ID"),
> >+             mUdev.udev_device_get_property_value(dev, "ID_MODEL_ID"),
> 
> Can we be sure these properties won't be null?

I'm fairly certain these are required by the USB spec, but I can add NULL checking if it'd make you feel better.


> >+  self->mUdev.udev_monitor_filter_add_match_subsystem_devtype(monitor,
> >+                                                              "input", NULL);
> 
> Do joystick devices have a specific devtype?

Not that I can find (although I have no idea what documentation I'd consult to find out). By manual testing, udev_device_get_devtype returns NULL.

> >+  if (strncmp(kJoystickPath, devpath, sizeof(kJoystickPath) - 1) != 0) {
> >+    return false;
> >+  }
> 
> Is the last test to check that this is not a generic eventN device for the
> joystick?
> 
> KERNEL=="js[0-9]*",             NAME="input/%k"
> is under the heading "naming rules for kernels <2.6.31" in
> 30-kernel-compat.rules but I don't know exactly what that means.

Yes. The only other way I can see to distinguish the event interface and the js interface is by the minor device numbers (per http://www.kernel.org/doc/Documentation/devices.txt, the js devices have minor 0-31 and the event devices have minor 64+), but that seems less intuitive.

> >+namespace mozilla {
> >+namespace hal_impl {
> >+
> >+LinuxGamepadService* gService = NULL;
> 
> Please use internal linkage for gService.

Would you be okay with me putting it into the anonymous namespace in this file?

> > namespace udev {
> 
> I wonder what this provides, given every identifier already begins with
> "udev"?

I was trying to avoid conflicts with the actual udev.h header symbols. Obviously I don't want to use that directly because I don't want to link to libudev.

	Karl Tomlinson (:karlt)
	Comment 165 • 12 years ago

(In reply to Ted Mielczarek [:ted] from comment #164)
> By manual testing, udev_device_get_devtype returns NULL.

Thanks; that sounds like a good test.

> > Please use internal linkage for gService.
> 
> Would you be okay with me putting it into the anonymous namespace in this
> file?

Yes, I get the impression that C++ is moving away from "static" for internal linkage.  And I expect any sane compiler would implement something similar enough for anonymous namespaces.

> > > namespace udev {
> > 
> > I wonder what this provides, given every identifier already begins with
> > "udev"?
> 
> I was trying to avoid conflicts with the actual udev.h header symbols.
> Obviously I don't want to use that directly because I don't want to link to
> libudev.

I don't have a strong opinion here, but I wonder whether something like namespace mozilla or hal would be more appropriate to avoid such conflicts, and istr a m.d.platform thread about not adding more namespaces.

	Karl Tomlinson (:karlt)
	Comment 166 • 12 years ago

(In reply to Ted Mielczarek [:ted] from comment #164)
> > >+             mUdev.udev_device_get_property_value(dev,"ID_VENDOR_ID"),
> > >+             mUdev.udev_device_get_property_value(dev, "ID_MODEL_ID"),
> > 
> > Can we be sure these properties won't be null?
> 
> I'm fairly certain these are required by the USB spec, but I can add NULL
> checking if it'd make you feel better.

I assume the dev is not necessarily a USB device, and so these properties may not be set.

	Scott Graham
	Comment 167 • 12 years ago

(In reply to Karl Tomlinson (:karlt) from comment #166)
> (In reply to Ted Mielczarek [:ted] from comment #164)
> > > >+             mUdev.udev_device_get_property_value(dev,"ID_VENDOR_ID"),
> > > >+             mUdev.udev_device_get_property_value(dev, "ID_MODEL_ID"),
> > > 
> > > Can we be sure these properties won't be null?
> > 
> > I'm fairly certain these are required by the USB spec, but I can add NULL
> > checking if it'd make you feel better.
> 
> I assume the dev is not necessarily a USB device, and so these properties
> may not be set.

In case it's helpful, for reference my implementation is here: http://code.google.com/searchframe#OAMlx_jo-ck/src/content/browser/gamepad/platform_data_fetcher_linux.cc&exact_package=chromium&q=udev%20gamepad&type=cs&l=151

It's similar but got a bit more complicated in the case of a bluetooth device (so that the device is reported, not the hub)

	(not currently active) Ted Mielczarek Assignee
	Comment 168 • 12 years ago

Okay, I reworked the Linux code to use glib for monitoring all the file descriptors, so everything runs on the main thread. It's a bit simpler than before, modulo all the extra glib stuff. I believe I've addressed all your other review comments as well.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 169 • 12 years ago

Comment on attachment 628426 [details] [diff] [review]
Add DOM Gamepad APIs

>+nsDOMGamepad::SetAxis(uint32_t axis, float value)
>+{
>+  mAxes[axis] = value;
>+}
So, callers ensure that this is *never* out-of-bounds


>+/* readonly attribute nsIVariant buttons; */
>+NS_IMETHODIMP
>+nsDOMGamepad::GetButtons(nsIVariant** aButtons)
>+{
>+  nsRefPtr<nsVariant> out = new nsVariant();
>+  NS_ENSURE_STATE(out);
>+
>+  if (mButtons.Length() == 0) {
>+    nsresult rv = out->SetAsEmptyArray();
>+    NS_ENSURE_SUCCESS(rv, rv);
>+  } else {
>+    // Note: The resulting nsIVariant dupes both the array and its elements.
>+    uint8_t *array = reinterpret_cast<uint8_t*>
>+                              (NS_Alloc(mButtons.Length() * sizeof(uint8_t)));
uint8_t ? Per spec buttons[] is float[], though I think double[] would be more natural

>+NS_IMETHODIMP
>+nsDOMGamepad::GetAxes(nsIVariant** aAxes)
>+{
>+  nsRefPtr<nsVariant> out = new nsVariant();
>+  NS_ENSURE_STATE(out);
>+
>+  if (mAxes.Length() == 0) {
>+    nsresult rv = out->SetAsEmptyArray();
>+    NS_ENSURE_SUCCESS(rv, rv);
>+  } else {
>+    // Note: The resulting nsIVariant dupes both the array and its elements.
>+    float *array = reinterpret_cast<float*>
>+                              (NS_Alloc(mAxes.Length() * sizeof(float)));
This is correct per spec, but I think spec should use double, not float.

>+nsDOMGamepadButtonEvent::GetGamepad(nsIDOMGamepad** aGamepad)
>+{
>+  nsCOMPtr<nsIDOMGamepad> gamepad = mGamepad;
>+  gamepad.forget(aGamepad);
>+  return NS_OK;
>+}
Why not
NS_IF_ADDREF(*aGamepad = mGamepad);
return NS_OK;
Same also elsewhere, though, if you change event interface inheritance, there would be only
one *Event::GetGamepad method

>+#ifdef MOZ_GAMEPAD
>+  if (aEventType.LowerCaseEqualsLiteral("mozgamepadbuttonevent"))
>+    return NS_NewDOMGamepadButtonEvent(aDOMEvent, aPresContext, nsnull);
>+  if (aEventType.LowerCaseEqualsLiteral("mozgamepadaxismoveevent"))
>+    return NS_NewDOMGamepadAxisMoveEvent(aDOMEvent, aPresContext, nsnull);
>+  if (aEventType.LowerCaseEqualsLiteral("mozgamepadconnectionevent"))
>+    return NS_NewDOMGamepadConnectionEvent(aDOMEvent, aPresContext, nsnull);
>+#endif
Per latest DOM4 spec, new kinds of events should be created only using event ctors, not document.createEvent.
So, remove these

>@@ -525,28 +530,45 @@
>   static WindowByIdTable* GetWindowsTable() {
>     return sWindowsById;
>   }
> 
>   void SizeOfIncludingThis(nsWindowSizes* aWindowSizes) const;
> 
>   void UnmarkGrayTimers();
> 
>   void AddEventTargetObject(nsDOMEventTargetHelper* aObject);
>   void RemoveEventTargetObject(nsDOMEventTargetHelper* aObject);
>-
>+  
Unneeded change

>+[scriptable, uuid(93b048d6-2aef-46a9-b0f4-06d7f00d8ef2)]
>+interface nsIDOMMozGamepadConnectionEvent : nsIDOMEvent
>+{
>+  /**
>+   * The device that generated this event.
>+   */
>+  readonly attribute nsIDOMGamepad gamepad;
>+
>+  [noscript]
>+  void initGamepadConnectionEvent(in DOMString typeArg,
>+                                  in boolean canBubbleArg,
>+                                  in boolean cancelableArg,
>+                                  in nsIDOMGamepad gamepad);
>+};
In the spec this event interface is called GamepadEvent, not GamepadConnectionEvent.

If the event interface name was GamepadEvent, it would be natural for 
GamepadButtonEvent and GamepadAxisMoveEvent to extend it
GamepadButtonEvent would just add .button and GamepadAxisMoveEvent would add
.value and .axis

I wish the events had dictionary for the constructor, and constructors implemented.
See for example nsDOMCustomEvent::InitFromCtor

>+[builtinclass, scriptable, uuid(ff13acd9-11da-4817-8f2a-4a5700dfd13e)]
>+interface nsIDOMGamepad : nsISupports
>+{
>+  /**
>+   * An identifier, unique per type of device.
>+   */
>+  readonly attribute DOMString id;
>+
>+  /**
>+   * The game port index for the device. Unique per device
>+   * attached to this system.
>+   */
>+  readonly attribute unsigned long index;
>+
>+  /**
>+   * true if this gamepad is currently connected to the system.
>+   */
>+  readonly attribute boolean connected;
>+
>+  /**
>+   * The current state of all buttons on the device.
>+   */
>+  readonly attribute nsIVariant buttons;
>+
>+  /**
>+   * The current position of all axes on the device.
>+   */
>+  readonly attribute nsIVariant axes;
>+};
Document that both buttons and variants are actually arrays.



>+[scriptable, uuid(7edf77a2-6b3e-4bbb-9100-4452d425feaa)]
>+interface nsIDOMGamepadServiceTest : nsISupports
>+{
>+  long AddGamepad(in string id, in long numButtons, in long numAxes);
>+  void RemoveGamepad(in long index);
>+  void NewButtonEvent(in long index, in long button, in boolean pressed);
>+  void NewAxisMoveEvent(in long index, in long axis, in float value);
methods in idl interfaces start with lowercase letter


>+
>+  nsString name = aPressed ? NS_LITERAL_STRING("MozGamepadButtonDown") :
>+                            NS_LITERAL_STRING("MozGamepadButtonUp");
Missing one space before NS_LITERAL_STRING("MozGamepadButtonUp")
>+
>+      if (domdoc && gamepad) {
>+        // Fire event
>+        FireConnectionEvent(domdoc, listeners[i], gamepad, aConnected);
So, same gamepad object is used for different windows? that is odd, unless the spec
requires that. But gamepad objects in events and in navigator.gamepads should be the same

>+class GamepadService {
{ should be in the next line


>+    # Gamepad
>+    'nsIDOMMozGamepadButtonEvent.button',
>+    'nsIDOMMozGamepadButtonEvent.gamepad',
>+    'nsIDOMMozGamepadAxisMoveEvent.axis',
>+    'nsIDOMMozGamepadAxisMoveEvent.value',
>+    'nsIDOMMozGamepadAxisMoveEvent.gamepad',
nsIDOMMozGamepadAxisMoveEvent.* should work
Same for other interfaces too.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 170 • 12 years ago

The patch misses navigator.gamepads[]

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 171 • 12 years ago

Ah, there is Bug 690935

	(not currently active) Ted Mielczarek Assignee
	Comment 172 • 12 years ago

(In reply to Olli Pettay [:smaug] from comment #169)
> >+nsDOMGamepad::SetAxis(uint32_t axis, float value)
> >+{
> >+  mAxes[axis] = value;
> >+}
> So, callers ensure that this is *never* out-of-bounds

The only callers are the native backends, so yes. I can put an assertion here if you'd like.

> uint8_t ? Per spec buttons[] is float[], though I think double[] would be
> more natural

That got changed after I implemented this. I can change it, it's not a big deal. (We won't ever actually report real float values for any buttons currently due to the underlying APIs we're using.)

> >+nsDOMGamepadButtonEvent::GetGamepad(nsIDOMGamepad** aGamepad)
> >+{
> >+  nsCOMPtr<nsIDOMGamepad> gamepad = mGamepad;
> >+  gamepad.forget(aGamepad);
> >+  return NS_OK;
> >+}
> Why not
> NS_IF_ADDREF(*aGamepad = mGamepad);

I keep getting conflicting review comments about these sorts of things. Should I just always go with whatever is fewer lines of code?

> If the event interface name was GamepadEvent, it would be natural for 
> GamepadButtonEvent and GamepadAxisMoveEvent to extend it
> GamepadButtonEvent would just add .button and GamepadAxisMoveEvent would add
> .value and .axis

Yeah, that probably makes sense.

> I wish the events had dictionary for the constructor, and constructors
> implemented.
> See for example nsDOMCustomEvent::InitFromCtor

Sorry, I forgot to implement the event constructor bits you mentioned in your previous review.

> >+      if (domdoc && gamepad) {
> >+        // Fire event
> >+        FireConnectionEvent(domdoc, listeners[i], gamepad, aConnected);
> So, same gamepad object is used for different windows? that is odd, unless
> the spec
> requires that. But gamepad objects in events and in navigator.gamepads
> should be the same

If you look up just a few lines above this, you'll see:
nsRefPtr<nsDOMGamepad> gamepad = listeners[i]->GetGamepad(aIndex);

Each window maintains a list of gamepad objects. I originally tried sharing them between windows, but we decided that non-visible windows shouldn't be able to get updated gamepad state, so they needed to be separate.

(In reply to Olli Pettay [:smaug] from comment #171)
> Ah, there is Bug 690935

Yes, navigator.gamepads got added to the spec after I implemented all this, and I didn't want to make my initial implementation take even longer. I'll implement that as a follow-up.