Last Comment Bug 604039 - Prototype DOM Gamepad (Joystick) API
: Prototype DOM Gamepad (Joystick) API
Status: RESOLVED FIXED
[games:p1]
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM: Events (show other bugs)
: Trunk
: All All
: -- normal with 5 votes (vote)
: ---
Assigned To: Ted Mielczarek [:ted.mielczarek]
: Ada [:adalucinet]
:
Mentors:
https://wiki.mozilla.org/JoystickAPI
Depends on: 680326 706554 783376 1248794
Blocks: webapi gamepad-rumble 691444 gecko-games 852258 855364 690935 690937 690938 790775 ouya 851542 851547 852257 852935 852944 854152 878828
  Show dependency treegraph
 
Reported: 2010-10-13 07:40 PDT by Ted Mielczarek [:ted.mielczarek]
Modified: 2016-02-23 14:59 PST (History)
77 users (show)
ted: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
disabled
-


Attachments
Add DOM Joystick events (47.84 KB, patch)
2011-07-28 13:25 PDT, Ted Mielczarek [:ted.mielczarek]
no flags Details | Diff | Splinter Review
Linux joystick backend (8.24 KB, patch)
2011-07-28 13:28 PDT, Ted Mielczarek [:ted.mielczarek]
no flags Details | Diff | Splinter Review
Demo page (723 bytes, text/html)
2011-07-28 13:41 PDT, Ted Mielczarek [:ted.mielczarek]
no flags Details
Windows joystick backend using DirectInput (11.05 KB, patch)
2011-07-30 05:23 PDT, Ted Mielczarek [:ted.mielczarek]
no flags Details | Diff | Splinter Review
Add DOM Joystick events (47.94 KB, patch)
2011-07-30 05:24 PDT, Ted Mielczarek [:ted.mielczarek]
no flags Details | Diff | Splinter Review
Linux joystick backend (7.71 KB, patch)
2011-07-30 05:36 PDT, Ted Mielczarek [:ted.mielczarek]
no flags Details | Diff | Splinter Review
Windows joystick backend using DirectInput (14.78 KB, patch)
2011-08-01 10:14 PDT, Ted Mielczarek [:ted.mielczarek]
no flags Details | Diff | Splinter Review
Mac joystick backend (9.80 KB, patch)
2011-08-02 12:23 PDT, Jason Orendorff [:jorendorff]
no flags Details | Diff | Splinter Review
Mac joystick backend (9.80 KB, patch)
2011-08-03 08:06 PDT, Jason Orendorff [:jorendorff]
no flags Details | Diff | Splinter Review
Add DOM Joystick events (47.61 KB, patch)
2011-08-03 13:01 PDT, Ted Mielczarek [:ted.mielczarek]
no flags Details | Diff | Splinter Review
Mac joystick backend (10.61 KB, patch)
2011-08-03 13:02 PDT, Ted Mielczarek [:ted.mielczarek]
no flags Details | Diff | Splinter Review
Windows joystick backend using DirectInput (18.62 KB, patch)
2011-08-03 13:04 PDT, Ted Mielczarek [:ted.mielczarek]
no flags Details | Diff | Splinter Review
Linux joystick backend (9.27 KB, patch)
2011-08-03 13:07 PDT, Ted Mielczarek [:ted.mielczarek]
no flags Details | Diff | Splinter Review
Another demo page (5.74 KB, text/html)
2011-08-04 03:26 PDT, Jason Orendorff [:jorendorff]
no flags Details
Linux joystick backend (9.27 KB, patch)
2011-08-04 15:03 PDT, Ted Mielczarek [:ted.mielczarek]
no flags Details | Diff | Splinter Review
[WIP] Typed Arrays for nsDOMJoystick (3.93 KB, patch)
2011-08-10 11:18 PDT, David Humphrey (:humph)
no flags Details | Diff | Splinter Review
Add DOM Joystick events (68.03 KB, patch)
2011-08-16 05:22 PDT, Ted Mielczarek [:ted.mielczarek]
no flags Details | Diff | Splinter Review
Mac joystick backend (11.76 KB, patch)
2011-08-16 05:25 PDT, Ted Mielczarek [:ted.mielczarek]
no flags Details | Diff | Splinter Review
Windows joystick backend using DirectInput (19.57 KB, patch)
2011-08-16 05:27 PDT, Ted Mielczarek [:ted.mielczarek]
no flags Details | Diff | Splinter Review
Linux joystick backend (10.00 KB, patch)
2011-08-16 05:31 PDT, Ted Mielczarek [:ted.mielczarek]
no flags Details | Diff | Splinter Review
joystick.index property (8.57 KB, patch)
2011-08-16 11:24 PDT, David Humphrey (:humph)
no flags Details | Diff | Splinter Review
Add DOM Joystick events (91.60 KB, patch)
2011-09-30 14:51 PDT, Ted Mielczarek [:ted.mielczarek]
no flags Details | Diff | Splinter Review
Mac joystick backend (11.76 KB, patch)
2011-09-30 14:53 PDT, Ted Mielczarek [:ted.mielczarek]
no flags Details | Diff | Splinter Review
Windows joystick backend using DirectInput (19.61 KB, patch)
2011-09-30 14:54 PDT, Ted Mielczarek [:ted.mielczarek]
no flags Details | Diff | Splinter Review
Linux joystick backend (20.20 KB, patch)
2011-09-30 14:56 PDT, Ted Mielczarek [:ted.mielczarek]
no flags Details | Diff | Splinter Review
Demo page (2.29 KB, text/html)
2011-09-30 14:58 PDT, Ted Mielczarek [:ted.mielczarek]
no flags Details
Add DOM Gamepad APIs (90.72 KB, patch)
2011-10-07 12:37 PDT, Ted Mielczarek [:ted.mielczarek]
no flags Details | Diff | Splinter Review
Mac gamepad backend (11.86 KB, patch)
2011-10-07 12:43 PDT, Ted Mielczarek [:ted.mielczarek]
no flags Details | Diff | Splinter Review
Windows gamepad backend using DirectInput (23.74 KB, patch)
2011-10-07 12:47 PDT, Ted Mielczarek [:ted.mielczarek]
no flags Details | Diff | Splinter Review
Linux gamepad backend (19.83 KB, patch)
2011-10-07 12:49 PDT, Ted Mielczarek [:ted.mielczarek]
no flags Details | Diff | Splinter Review
Demo page (2.42 KB, text/html)
2011-10-07 12:53 PDT, Ted Mielczarek [:ted.mielczarek]
no flags Details
Add DOM Gamepad APIs (116.83 KB, patch)
2012-01-26 13:06 PST, Ted Mielczarek [:ted.mielczarek]
no flags Details | Diff | Splinter Review
Mac gamepad backend (11.90 KB, patch)
2012-01-26 13:07 PST, Ted Mielczarek [:ted.mielczarek]
no flags Details | Diff | Splinter Review
Windows gamepad backend using DirectInput (23.68 KB, patch)
2012-01-26 13:13 PST, Ted Mielczarek [:ted.mielczarek]
no flags Details | Diff | Splinter Review
Linux gamepad backend (19.89 KB, patch)
2012-01-26 13:13 PST, Ted Mielczarek [:ted.mielczarek]
no flags Details | Diff | Splinter Review
Add DOM Gamepad APIs (125.59 KB, patch)
2012-01-30 10:33 PST, Ted Mielczarek [:ted.mielczarek]
no flags Details | Diff | Splinter Review
Add DOM Gamepad APIs (128.52 KB, patch)
2012-02-02 11:58 PST, Ted Mielczarek [:ted.mielczarek]
bugs: review-
cjones.bugs: review+
Details | Diff | Splinter Review
Mac gamepad backend (16.39 KB, patch)
2012-02-02 11:59 PST, Ted Mielczarek [:ted.mielczarek]
no flags Details | Diff | Splinter Review
Windows gamepad backend using DirectInput (24.65 KB, patch)
2012-02-02 12:03 PST, Ted Mielczarek [:ted.mielczarek]
bas: review+
Details | Diff | Splinter Review
Linux gamepad backend (19.89 KB, patch)
2012-02-02 12:05 PST, Ted Mielczarek [:ted.mielczarek]
karlt: review-
Details | Diff | Splinter Review
Add DOM Gamepad APIs (103.40 KB, patch)
2012-05-30 12:44 PDT, Ted Mielczarek [:ted.mielczarek]
bugs: review-
Details | Diff | Splinter Review
Linux gamepad backend (16.72 KB, patch)
2012-06-01 13:09 PDT, Ted Mielczarek [:ted.mielczarek]
karlt: review-
Details | Diff | Splinter Review
Linux gamepad backend (15.94 KB, patch)
2012-08-08 12:24 PDT, Ted Mielczarek [:ted.mielczarek]
karlt: review+
Details | Diff | Splinter Review
Add DOM Gamepad APIs (85.95 KB, patch)
2012-08-17 11:45 PDT, Ted Mielczarek [:ted.mielczarek]
bugs: review-
Details | Diff | Splinter Review
Add DOM Gamepad APIs (83.43 KB, patch)
2013-03-08 12:24 PST, Ted Mielczarek [:ted.mielczarek]
no flags Details | Diff | Splinter Review
Linux gamepad backend (14.95 KB, patch)
2013-03-08 12:26 PST, Ted Mielczarek [:ted.mielczarek]
no flags Details | Diff | Splinter Review
Mac gamepad backend (13.28 KB, patch)
2013-03-08 12:29 PST, Ted Mielczarek [:ted.mielczarek]
smichaud: review+
Details | Diff | Splinter Review
Windows gamepad backend using DirectInput (23.22 KB, patch)
2013-03-08 17:04 PST, Ted Mielczarek [:ted.mielczarek]
no flags Details | Diff | Splinter Review
Add DOM Gamepad APIs (on eccf45749400) (79.08 KB, patch)
2013-03-11 13:10 PDT, :Ms2ger (⌚ UTC+1/+2)
no flags Details | Diff | Splinter Review
Add DOM Gamepad APIs (82.42 KB, patch)
2013-03-15 10:25 PDT, Ted Mielczarek [:ted.mielczarek]
bugs: review-
Details | Diff | Splinter Review
Demo page (2.43 KB, text/html)
2013-03-15 10:28 PDT, Ted Mielczarek [:ted.mielczarek]
no flags Details
Add DOM Gamepad APIs (82.19 KB, patch)
2013-03-18 13:15 PDT, Ted Mielczarek [:ted.mielczarek]
no flags Details | Diff | Splinter Review
Add DOM Gamepad APIs (83.06 KB, patch)
2013-03-18 13:19 PDT, Ted Mielczarek [:ted.mielczarek]
bugs: review+
Details | Diff | Splinter Review

Description Ted Mielczarek [:ted.mielczarek] 2010-10-13 07:40:17 PDT
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.
Comment 1 David Humphrey (:humph) 2010-10-13 07:43:34 PDT
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
Comment 2 Olli Pettay [:smaug] 2010-10-13 08:01:56 PDT
Is there any cross platform native API for joysticks/gamepads?
Comment 3 Ted Mielczarek [:ted.mielczarek] 2010-10-13 08:23:01 PDT
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 )
Comment 4 Mike Shaver (:shaver -- probably not reading bugmail closely) 2010-10-13 08:43:09 PDT
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.
Comment 5 Olli Pettay [:smaug] 2010-10-13 09:34:15 PDT
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.
Comment 6 Olli Pettay [:smaug] 2011-07-25 08:53:28 PDT
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.
Comment 7 K. Gadd (:kael) 2011-07-26 08:00:31 PDT
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.
Comment 8 Olli Pettay [:smaug] 2011-07-26 08:18:22 PDT
Fennec does generate click and keydown/up/press events
Comment 9 Olli Pettay [:smaug] 2011-07-26 08:20:53 PDT
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 
  });
Comment 10 Ted Mielczarek [:ted.mielczarek] 2011-07-28 13:25:58 PDT
Created attachment 549208 [details] [diff] [review]
Add DOM Joystick events

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.
Comment 11 Ted Mielczarek [:ted.mielczarek] 2011-07-28 13:28:10 PDT
Created attachment 549209 [details] [diff] [review]
Linux joystick backend

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.
Comment 12 Ted Mielczarek [:ted.mielczarek] 2011-07-28 13:41:13 PDT
Created attachment 549214 [details]
Demo page

This is the test page I've been using, it adds listeners for all three event types and displays the data.
Comment 13 Grant Galitz 2011-07-28 13:43:03 PDT
This is getting interesting...
Comment 14 David Humphrey (:humph) 2011-07-30 04:52:17 PDT
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!
Comment 15 David Humphrey (:humph) 2011-07-30 05:05:55 PDT
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);}());
Comment 16 Ted Mielczarek [:ted.mielczarek] 2011-07-30 05:23:34 PDT
Created attachment 549569 [details] [diff] [review]
Windows joystick backend using DirectInput

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.
Comment 17 Ted Mielczarek [:ted.mielczarek] 2011-07-30 05:24:32 PDT
Created attachment 549570 [details] [diff] [review]
Add DOM Joystick events

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.
Comment 18 Ted Mielczarek [:ted.mielczarek] 2011-07-30 05:36:04 PDT
Created attachment 549571 [details] [diff] [review]
Linux joystick backend

Updated the Linux patch to match the changes to the base patch.
Comment 19 Bobby Richter [:secretrobotron] 2011-07-30 11:08:34 PDT
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);}());
Comment 20 Ben Vanik 2011-07-30 22:46:50 PDT
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).
Comment 21 Ted Mielczarek [:ted.mielczarek] 2011-07-31 06:01:51 PDT
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.
Comment 22 Siddharth Agarwal [:sid0] (inactive) 2011-07-31 06:05:19 PDT
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/
Comment 23 Ted Mielczarek [:ted.mielczarek] 2011-07-31 13:19:59 PDT
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.
Comment 24 Ted Mielczarek [:ted.mielczarek] 2011-08-01 10:14:53 PDT
Created attachment 549838 [details] [diff] [review]
Windows joystick backend using DirectInput

This patch adds support for axis move events on Windows.
Comment 25 :Felipe Gomes (needinfo me!) 2011-08-01 15:07:45 PDT
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)
Comment 26 Gregg Tavares 2011-08-01 20:56:48 PDT
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.
Comment 27 Ted Mielczarek [:ted.mielczarek] 2011-08-02 04:17:26 PDT
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.
Comment 28 Jason Orendorff [:jorendorff] 2011-08-02 12:23:43 PDT
Created attachment 550159 [details] [diff] [review]
Mac joystick backend
Comment 29 Ted Mielczarek [:ted.mielczarek] 2011-08-03 05:26:06 PDT
I put together a wiki page to capture some thoughts about this work (in the URL field here), feel free to contribute there.
Comment 30 Jason Orendorff [:jorendorff] 2011-08-03 08:06:06 PDT
Created attachment 550391 [details] [diff] [review]
Mac joystick backend
Comment 31 Ted Mielczarek [:ted.mielczarek] 2011-08-03 13:01:11 PDT
Created attachment 550468 [details] [diff] [review]
Add DOM Joystick events

I rebased all the patches to a more recent mozilla-central changeset (101411c3ca1c), and I fiddled the configure bits a little.
Comment 32 Ted Mielczarek [:ted.mielczarek] 2011-08-03 13:02:20 PDT
Created attachment 550469 [details] [diff] [review]
Mac joystick backend

Rebased, with mild configure changes.
Comment 33 Ted Mielczarek [:ted.mielczarek] 2011-08-03 13:04:13 PDT
Created attachment 550470 [details] [diff] [review]
Windows joystick backend using DirectInput

Rebased, and I fixed the configury to properly check for the DirectX SDK and lik to it without any manual fiddling.
Comment 34 Ted Mielczarek [:ted.mielczarek] 2011-08-03 13:07:38 PDT
Created attachment 550471 [details] [diff] [review]
Linux joystick backend

Rebased.
Comment 35 Justin Dolske [:Dolske] 2011-08-03 21:05:42 PDT
(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).
Comment 36 Jason Orendorff [:jorendorff] 2011-08-04 03:26:54 PDT
Created attachment 550638 [details]
Another demo page

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. :)
Comment 37 Ted Mielczarek [:ted.mielczarek] 2011-08-04 04:53:31 PDT
Try server builds are up:
ftp://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/tmielczarek@mozilla.com-8df56f741a66/
Comment 38 Rick Waldron [:rwaldron] 2011-08-04 14:24:50 PDT
I put this together for studying event properties, other might find it useful as well


https://gist.github.com/58b0f9ed647f72da9173
Comment 39 Ted Mielczarek [:ted.mielczarek] 2011-08-04 15:03:33 PDT
Created attachment 550843 [details] [diff] [review]
Linux joystick backend

Fixed configure so this actually links...
Comment 40 Scott Graham 2011-08-05 16:57:51 PDT
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.
Comment 41 Olli Pettay [:smaug] 2011-08-06 02:33:49 PDT
Could you perhaps share that document so that one doesn't need to have
Google account to read it.
Comment 42 Scott Graham 2011-08-06 07:04:12 PDT
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?
Comment 43 Olli Pettay [:smaug] 2011-08-06 07:10:45 PDT
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.
Comment 44 Scott Graham 2011-08-08 09:01:58 PDT
Hopefully less annoying link here https://sites.google.com/a/chromium.org/dev/developers/joystick-api
Comment 45 Grant Galitz 2011-08-08 09:19:39 PDT
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.
Comment 46 Rick Waldron [:rwaldron] 2011-08-08 10:28:18 PDT
@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.
Comment 47 Rick Waldron [:rwaldron] 2011-08-08 11:11:37 PDT
@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.


-------------
Comment 48 K. Gadd (:kael) 2011-08-08 17:07:28 PDT
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.
Comment 49 Scott Graham 2011-08-08 20:04:52 PDT
@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?
Comment 50 Scott Graham 2011-08-08 20:14:51 PDT
@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?
Comment 51 K. Gadd (:kael) 2011-08-09 01:12:01 PDT
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.
Comment 52 Rick Waldron [:rwaldron] 2011-08-09 05:44:11 PDT
@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.
Comment 53 Olli Pettay [:smaug] 2011-08-09 06:03:43 PDT
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.
Comment 54 Rick Waldron [:rwaldron] 2011-08-09 06:40:00 PDT
@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
Comment 55 Olli Pettay [:smaug] 2011-08-09 06:49:23 PDT
Ok, thanks for clarifying!
Comment 56 Antoine Turmel [:GeekShadow] 2011-08-09 17:15:13 PDT
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.
Comment 57 David Humphrey (:humph) 2011-08-10 11:18:51 PDT
Created attachment 552132 [details] [diff] [review]
[WIP] Typed Arrays for nsDOMJoystick

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.
Comment 58 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-08-10 11:25:31 PDT
Why do we want to use typed arrays for getting buttons etc? As opposed to using "normal" JS objects and properties?
Comment 59 Ted Mielczarek [:ted.mielczarek] 2011-08-10 11:33:30 PDT
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.)
Comment 60 David Humphrey (:humph) 2011-08-10 11:40:06 PDT
(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.
Comment 61 Antoine Turmel [:GeekShadow] 2011-08-10 14:57:57 PDT
Be sure to handle dead zone for sticks.
Comment 62 Mozilla RelEng Bot 2011-08-11 21:01:05 PDT
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
Comment 63 Mozilla RelEng Bot 2011-08-14 22:20:40 PDT
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
Comment 64 Ted Mielczarek [:ted.mielczarek] 2011-08-15 05:47:59 PDT
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.)
Comment 65 Ted Mielczarek [:ted.mielczarek] 2011-08-15 05:58:27 PDT
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.
Comment 66 Scott Graham 2011-08-15 08:29:55 PDT
@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?
Comment 67 Ted Mielczarek [:ted.mielczarek] 2011-08-15 08:48:47 PDT
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.
Comment 68 Ted Mielczarek [:ted.mielczarek] 2011-08-16 05:22:44 PDT
Created attachment 553442 [details] [diff] [review]
Add DOM Joystick events

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.)
Comment 69 Ted Mielczarek [:ted.mielczarek] 2011-08-16 05:25:46 PDT
Created attachment 553443 [details] [diff] [review]
Mac joystick backend

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
Comment 70 Ted Mielczarek [:ted.mielczarek] 2011-08-16 05:27:25 PDT
Created attachment 553444 [details] [diff] [review]
Windows joystick backend using DirectInput

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.
Comment 71 Ted Mielczarek [:ted.mielczarek] 2011-08-16 05:31:36 PDT
Created attachment 553445 [details] [diff] [review]
Linux joystick backend

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.
Comment 72 d 2011-08-16 05:51:58 PDT
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?
Comment 73 Ted Mielczarek [:ted.mielczarek] 2011-08-16 05:54:07 PDT
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.
Comment 74 d 2011-08-16 06:15:27 PDT
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.
Comment 75 David Humphrey (:humph) 2011-08-16 08:38:56 PDT
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.
Comment 76 David Humphrey (:humph) 2011-08-16 11:24:33 PDT
Created attachment 553531 [details] [diff] [review]
joystick.index property

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.
Comment 77 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-08-18 14:56:37 PDT
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?
Comment 78 Ted Mielczarek [:ted.mielczarek] 2011-08-18 16:11:40 PDT
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.)
Comment 79 Tab Atkins Jr. 2011-08-24 18:36:22 PDT
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.
Comment 80 Olli Pettay [:smaug] 2011-08-25 04:09:55 PDT
(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.
Comment 81 Tab Atkins Jr. 2011-08-25 09:09:40 PDT
(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.)
Comment 82 Tab Atkins Jr. 2011-08-25 09:20:23 PDT
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.
Comment 83 Ted Mielczarek [:ted.mielczarek] 2011-08-25 09:50:06 PDT
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.
Comment 84 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-08-25 09:55:47 PDT
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.
Comment 85 Tab Atkins Jr. 2011-08-25 10:04:33 PDT
(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.
Comment 86 Tab Atkins Jr. 2011-08-25 10:09:06 PDT
(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).
Comment 87 Tab Atkins Jr. 2011-08-25 10:16:44 PDT
(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.
Comment 88 Scott Graham 2011-08-25 10:25:06 PDT
(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.
Comment 89 Scott Graham 2011-08-25 10:28:53 PDT
(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.
Comment 90 Ted Mielczarek [:ted.mielczarek] 2011-08-25 10:36:48 PDT
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.
Comment 91 David Humphrey (:humph) 2011-08-25 11:51:58 PDT
 > > 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.
Comment 92 Scott Graham 2011-08-25 11:58:07 PDT
(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.
Comment 93 Tab Atkins Jr. 2011-08-25 12:17:34 PDT
(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.
Comment 94 Ted Mielczarek [:ted.mielczarek] 2011-08-25 12:33:08 PDT
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
Comment 95 David Humphrey (:humph) 2011-08-25 13:06:20 PDT
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
Comment 96 Ted Mielczarek [:ted.mielczarek] 2011-09-27 08:32:50 PDT
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.
Comment 97 David Woldrich 2011-09-27 09:42:35 PDT
(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.
Comment 98 Ted Mielczarek [:ted.mielczarek] 2011-09-27 09:45:44 PDT
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.
Comment 99 David Woldrich 2011-09-27 12:13:48 PDT
(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.
Comment 100 David Humphrey (:humph) 2011-09-27 12:25:38 PDT
> 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.
Comment 101 Ted Mielczarek [:ted.mielczarek] 2011-09-27 12:34:56 PDT
Just for clarification, as currently implemented, events are fired at |window| and do not bubble.
Comment 102 Ted Mielczarek [:ted.mielczarek] 2011-09-30 14:51:41 PDT
Created attachment 563853 [details] [diff] [review]
Add DOM Joystick events

Here's an updated patch!
Comment 103 Ted Mielczarek [:ted.mielczarek] 2011-09-30 14:53:08 PDT
Created attachment 563854 [details] [diff] [review]
Mac joystick backend

Updated mac patch.
Comment 104 Ted Mielczarek [:ted.mielczarek] 2011-09-30 14:54:01 PDT
Created attachment 563855 [details] [diff] [review]
Windows joystick backend using DirectInput

Updated Windows patch.
Comment 105 Ted Mielczarek [:ted.mielczarek] 2011-09-30 14:56:03 PDT
Created attachment 563857 [details] [diff] [review]
Linux joystick backend

This is a much-updated Linux patch. I've switched to using libudev,
which means that it can now get device connected/disconnected notifications.
Comment 106 Ted Mielczarek [:ted.mielczarek] 2011-09-30 14:57:28 PDT
Comment on attachment 553531 [details] [diff] [review]
joystick.index property

This got folded into the DOM patch. Thanks Dave!
Comment 107 Ted Mielczarek [:ted.mielczarek] 2011-09-30 14:58:31 PDT
Created attachment 563859 [details]
Demo page

This is an updated demo page that handles device connected/disconnected events, and then polls the joystick object for changes.
Comment 108 Ted Mielczarek [:ted.mielczarek] 2011-09-30 15:01:44 PDT
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.
Comment 109 Mozilla RelEng Bot 2011-09-30 19:31:22 PDT
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
Comment 110 :Ms2ger (⌚ UTC+1/+2) 2011-10-01 01:36:49 PDT
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
Comment 111 Ted Mielczarek [:ted.mielczarek] 2011-10-01 06:37:20 PDT
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.
Comment 112 Ted Mielczarek [:ted.mielczarek] 2011-10-05 09:51:46 PDT
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.)
Comment 113 Ted Mielczarek [:ted.mielczarek] 2011-10-05 19:14:56 PDT
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.
Comment 114 Ted Mielczarek [:ted.mielczarek] 2011-10-07 12:37:42 PDT
Created attachment 565609 [details] [diff] [review]
Add DOM Gamepad APIs

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.
Comment 115 Ted Mielczarek [:ted.mielczarek] 2011-10-07 12:43:32 PDT
Created attachment 565611 [details] [diff] [review]
Mac gamepad backend

Mostly just cleanup + renaming here.
Comment 116 Ted Mielczarek [:ted.mielczarek] 2011-10-07 12:47:19 PDT
Created attachment 565613 [details] [diff] [review]
Windows gamepad backend using DirectInput

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!)
Comment 117 Ted Mielczarek [:ted.mielczarek] 2011-10-07 12:49:38 PDT
Created attachment 565614 [details] [diff] [review]
Linux gamepad backend

Mostly just cleanup + renaming in the Linux patch, although I did fix a possible crash bug as well.
Comment 118 Ted Mielczarek [:ted.mielczarek] 2011-10-07 12:53:55 PDT
Created attachment 565617 [details]
Demo page

This is the same demo page, updated to use the new event/property names.
Comment 119 Mozilla RelEng Bot 2011-10-07 16:08:23 PDT
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
Comment 120 David Woldrich 2011-10-08 01:50:56 PDT
(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!
Comment 121 Ted Mielczarek [:ted.mielczarek] 2011-10-08 05:59:11 PDT
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.
Comment 122 David Woldrich 2011-10-08 16:40:31 PDT
(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.
Comment 123 David Woldrich 2011-10-08 19:44:31 PDT
(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!
Comment 124 Ted Mielczarek [:ted.mielczarek] 2011-10-09 06:12:19 PDT
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.
Comment 125 David Woldrich 2011-10-09 14:09:19 PDT
(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.  ;)
Comment 126 Romain Pouclet 2011-11-29 00:38:08 PST
How exactly would that work?  I can't tell you how much I dream about a webapp using my wiimote
Comment 127 Yannick INIZAN 2011-12-01 18:38:49 PST
I have tested my controller under linux with firefox (Thrustmaster dual analog 3) Unfortunately this doesn't work.
Comment 128 Scott Graham 2012-01-09 14:37:22 PST
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.
Comment 129 Ted Mielczarek [:ted.mielczarek] 2012-01-26 13:06:33 PST
Created attachment 591904 [details] [diff] [review]
Add DOM Gamepad APIs

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.
Comment 130 Ted Mielczarek [:ted.mielczarek] 2012-01-26 13:07:12 PST
Created attachment 591906 [details] [diff] [review]
Mac gamepad backend

Rebased.
Comment 131 Ted Mielczarek [:ted.mielczarek] 2012-01-26 13:13:04 PST
Created attachment 591908 [details] [diff] [review]
Windows gamepad backend using DirectInput

Rebased.
Comment 132 Ted Mielczarek [:ted.mielczarek] 2012-01-26 13:13:44 PST
Created attachment 591910 [details] [diff] [review]
Linux gamepad backend

Rebased.
Comment 133 Ted Mielczarek [:ted.mielczarek] 2012-01-30 10:33:58 PST
Created attachment 592772 [details] [diff] [review]
Add DOM Gamepad APIs

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.
Comment 134 Ted Mielczarek [:ted.mielczarek] 2012-01-30 17:19:14 PST
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.
Comment 135 Ted Mielczarek [:ted.mielczarek] 2012-01-30 17:19:49 PST
I need to find reviewers for the platform backends, if anyone has suggestions I'd appreciate it.
Comment 136 Scott Graham 2012-02-02 11:38:12 PST
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.
Comment 137 Ted Mielczarek [:ted.mielczarek] 2012-02-02 11:58:09 PST
Created attachment 593928 [details] [diff] [review]
Add DOM Gamepad APIs

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.
Comment 138 Ted Mielczarek [:ted.mielczarek] 2012-02-02 11:59:20 PST
Created attachment 593929 [details] [diff] [review]
Mac gamepad backend

Mac backend, implemented as part of hal.

smichaud graciously agreed to review the mac backend.
Comment 139 Ted Mielczarek [:ted.mielczarek] 2012-02-02 12:03:57 PST
Created attachment 593931 [details] [diff] [review]
Windows gamepad backend using DirectInput

The Windows backend, similarly updated and moved to hal.

I cajoled Bas into reviewing this.
Comment 140 Ted Mielczarek [:ted.mielczarek] 2012-02-02 12:05:27 PST
Created attachment 593932 [details] [diff] [review]
Linux gamepad backend

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.
Comment 141 Ted Mielczarek [:ted.mielczarek] 2012-02-02 12:05:58 PST
(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. :-/
Comment 142 Mozilla RelEng Bot 2012-02-02 14:45:24 PST
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
Comment 143 Scott Graham 2012-02-02 17:19:31 PST
(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.
Comment 144 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-02-03 18:25:27 PST
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
Comment 145 Ted Mielczarek [:ted.mielczarek] 2012-02-03 18:32:20 PST
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.
Comment 146 Rob Hawkes [:rob] 2012-02-11 10:26:13 PST
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/
Comment 147 Olli Pettay [:smaug] 2012-02-12 18:52:04 PST
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..
Comment 148 Olli Pettay [:smaug] 2012-02-12 19:21:15 PST
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.
Comment 149 :Ms2ger (⌚ UTC+1/+2) 2012-02-13 01:24:44 PST
(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.
Comment 150 Karl Tomlinson (:karlt) 2012-02-16 21:22:28 PST
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.
Comment 151 David Humphrey (:humph) 2012-03-09 08:24:11 PST
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
Comment 152 xavier deschuyteneer 2012-03-09 09:59:58 PST
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
Comment 153 Rob Hawkes [:rob] 2012-04-11 06:28:15 PDT
What is the status on the review process? I'd love to see this land in Nightly.
Comment 154 Steven Michaud [:smichaud] (Retired) 2012-04-11 06:53:25 PDT
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.
Comment 155 Ted Mielczarek [:ted.mielczarek] 2012-04-11 08:45:40 PDT
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.
Comment 156 Steven Michaud [:smichaud] (Retired) 2012-04-11 08:52:43 PDT
I forgot to mention it in comment #154, but I do now have joysticks I can test with.
Comment 157 Bas Schouten (:bas.schouten) 2012-04-11 17:04:52 PDT
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.
Comment 158 Rob Hawkes [:rob] 2012-05-11 11:45:30 PDT
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.
Comment 159 Ted Mielczarek [:ted.mielczarek] 2012-05-11 11:51:27 PDT
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.
Comment 160 :Ms2ger (⌚ UTC+1/+2) 2012-05-29 12:15:45 PDT
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
Comment 161 Ted Mielczarek [:ted.mielczarek] 2012-05-30 09:36:23 PDT
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.
Comment 162 Ted Mielczarek [:ted.mielczarek] 2012-05-30 12:44:16 PDT
Created attachment 628426 [details] [diff] [review]
Add DOM Gamepad APIs

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.
Comment 163 Ted Mielczarek [:ted.mielczarek] 2012-05-30 12:48:41 PDT
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)
Comment 164 Ted Mielczarek [:ted.mielczarek] 2012-05-31 12:46:57 PDT
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.
Comment 165 Karl Tomlinson (:karlt) 2012-05-31 19:19:14 PDT
(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.
Comment 166 Karl Tomlinson (:karlt) 2012-05-31 19:26:46 PDT
(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.
Comment 167 Scott Graham 2012-05-31 19:44:17 PDT
(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)
Comment 168 Ted Mielczarek [:ted.mielczarek] 2012-06-01 13:09:59 PDT
Created attachment 629309 [details] [diff] [review]
Linux gamepad backend

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.
Comment 169 Olli Pettay [:smaug] 2012-06-04 08:24:07 PDT
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.
Comment 170 Olli Pettay [:smaug] 2012-06-04 08:29:15 PDT
The patch misses navigator.gamepads[]
Comment 171 Olli Pettay [:smaug] 2012-06-04 09:24:18 PDT
Ah, there is Bug 690935
Comment 172 Ted Mielczarek [:ted.mielczarek] 2012-06-04 10:34:10 PDT
(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.
Comment 173 Karl Tomlinson (:karlt) 2012-06-29 03:11:06 PDT
Comment on attachment 629309 [details] [diff] [review]
Linux gamepad backend

This wasn't quite a complete review sorry, but I won't be able to get back to
this next week, so I'll write down where I got to.

> case "$target_os" in
>-darwin*|mingw*)
>+darwin*|mingw*|linux*)
>     MOZ_GAMEPAD=1
>     ;;

I guess this needs to depend on GTK now.

(Using something like poll or select in the main loop is quite typical of a
 Unix program, so I expect there are ways to do this with each toolkit main
 loop, but we have only a GLib implementation at this stage.

 We already have code that uses GIOChannel to hook up with the event loop while
 still using plain POSIX read on the file descriptor, and that is also
 essentially what is happening with the udev_monitor here, so that can be done
 also when reading the joystick data when we want to share more code with other
 toolkits.)

>diff --git a/dom/system/Makefile.in b/dom/system/Makefile.in

>-EXPORTS_NAMESPACES = mozilla/dom
>+EXPORTS_NAMESPACES += mozilla/dom

I don't know the effect of this, so check this is intentional and in the right
patch.

>+} Gamepad;
>+
>+
>+
>+class LinuxGamepadService {

Vertical space.

>+  mMonitorChannel = g_io_channel_unix_new(monitor_fd);
>+  if (!mMonitorChannel) {

g_io_channel_unix_new never returns NULL.  It is quite typical of GLib object
creation functions to abort the program on memory allocation failure, which
simplifies error handling somewhat.  The documentation should make it clear if
any function can return NULL.

The g_io_channel_shutdown calls are unnecessary as the last unref will look
after anything this does.  By releasing our reference immediately after
g_io_add_watch, g_source_remove will release the final reference and close the
file if necessary.  For mMonitorChannel the fd is owned by the udev_monitor,
so the close from g_io_channel_shutdown could be bad.  (The last unref will not
call close for channels from g_io_channel_unix_new.)  That makes
Gamepad::channel and mMonitorChannel unnecessary.

I expect we probably want G_IO_HUP in the g_io_add_watch flags.
I'm not sure we need this, but from GLib source code, it looks like the
callbacks will never have this bit in |condition| unless we pass the flag.

g_io_channel_set_flags is essentially a wrapper around fcntl.  For
mMonitorChannel, the fd really belongs to udev and I'm not sure we should
change it.  The docs for udev_monitor_receive_device say "The caller needs to
make sure that there is data to read from the socket. The call will block
until the socket becomes readable."  For the joystick files the while (true)
loop requires NONBLOCK, so the g_io_channel_set_flags call is appropriate
there.  In both situations there seems to be an assumption that a whole
"event" will become available at once - that's probably fairly reasonable.

>+    mUdev.udev_device_get_property_value(dev,"ID_VENDOR_ID");

Missed a space between args.

There may be some unnecessary includes with the changes that have been made.
Comment 174 Daniel Hendrycks 2012-07-19 09:09:21 PDT
Are Nightly builds to include this feature in the non-distant future? (I've had my interest sparked by Chrome beta's inclusion.)
Comment 175 Ted Mielczarek [:ted.mielczarek] 2012-08-08 12:24:30 PDT
Created attachment 650250 [details] [diff] [review]
Linux gamepad backend

This should fix all the comments from your partial review.
Comment 176 Ted Mielczarek [:ted.mielczarek] 2012-08-08 12:27:34 PDT
(In reply to Karl Tomlinson (:karlt) from comment #173)
> I guess this needs to depend on GTK now.

Fixed.

> I don't know the effect of this, so check this is intentional and in the
> right
> patch.

Yeah, that was supposed to be in the base DOM patch.

> The g_io_channel_shutdown calls are unnecessary as the last unref will look
> after anything this does.  By releasing our reference immediately after
> g_io_add_watch, g_source_remove will release the final reference and close

I think I fixed all the issues related to this.
Comment 177 Ted Mielczarek [:ted.mielczarek] 2012-08-08 12:29:03 PDT
(In reply to Daniel Hendrycks from comment #174)
> Are Nightly builds to include this feature in the non-distant future? (I've
> had my interest sparked by Chrome beta's inclusion.)

I'm working to get these patches finished and landed. It's been hard because it's a side project for me, and more important tasks keep taking up all my time.
Comment 178 Karl Tomlinson (:karlt) 2012-08-09 19:50:25 PDT
Comment on attachment 650250 [details] [diff] [review]
Linux gamepad backend

>+  mUdev.udev_monitor_filter_add_match_subsystem_devtype(mMonitor,
>+                                                        "input",
>+                                                      nullptr);

Alignment.

>+  mMonitor =
>+    mUdev.udev_monitor_new_from_netlink(mUdev.udev, "udev");

Should handle NULL return from udev_monitor_new_from_netlink, I guess, unless
you are certain there will be no error.

>+  char name[128];
>+  ioctl(fd, JSIOCGNAME(sizeof(name)), &name);

Should check for failure and set name (to the empty string even), or
initialize name to ensure there will be a null terminator.

>+  strncpy(gamepad.devpath, devpath, sizeof(gamepad.devpath));

strncpy does not ensure null termination, so snprintf (which does null
terminate) may be the simplest solution.

>+  char numAxes, numButtons;
>+  ioctl(fd, JSIOCGAXES, &numAxes);

Perhaps initialize these to 0 for a sane value in case of failure.

>+    struct udev_device* dev = mUdev.udev_device_new_from_syspath(mUdev.udev,
>+                                                                 path);
>+    if (!is_gamepad(dev))
>+      continue;
>+
>+    AddDevice(dev);
>+    mUdev.udev_device_unref(dev);

Need to redo this to unref even when !is_gamepad.

>+  ScanForDevices();
>+  AddMonitor();

Should really AddMonitor before ScanForDevices to detect devices added or
removed after/during ScanForDevices.  AddDevice would need to check the device
doesn't already exist.

>+void
>+LinuxGamepadService::Shutdown()
>+{
>+  Cleanup();
>+}
>+
>+void
>+LinuxGamepadService::Cleanup()
>+{
>+  for (unsigned int i = 0; i < mGamepads.Length(); i++) {
>+    g_source_remove(mGamepads[i].source_id);
>+  }
>+  mGamepads.Clear();
>+  RemoveMonitor();
>+}

Might as well merge Cleanup into Shutdown.
Comment 179 Ted Mielczarek [:ted.mielczarek] 2012-08-17 11:45:49 PDT
Created attachment 652855 [details] [diff] [review]
Add DOM Gamepad APIs

This fixes all the review comments (AFAIK), and adds event constructor support. I switched to using the event code generator, so I actually was able to remove the source files containing the implementation of the events.
Comment 180 Olli Pettay [:smaug] 2012-08-20 10:55:58 PDT
Comment on attachment 652855 [details] [diff] [review]
Add DOM Gamepad APIs

When generating patches, please use -P


>+/* readonly attribute DOMString id; */
>+NS_IMETHODIMP
>+nsDOMGamepad::GetId(nsAString & aID)
>+{
nsAString&



>+  } else {
>+    // Note: The resulting nsIVariant dupes both the array and its elements.
>+    double *array = reinterpret_cast<double*>
>+                              (NS_Alloc(mButtons.Length() * sizeof(double)));
double* array


>+    // Note: The resulting nsIVariant dupes both the array and its elements.
>+    double *array = reinterpret_cast<double*>
>+                              (NS_Alloc(mAxes.Length() * sizeof(double)));
ditto.


>+nsDOMGamepad::SyncState(nsDOMGamepad* other)
aOther

>+  // Make the state of this gamepad equivalent to other.
>+  void SyncState(nsDOMGamepad* other);
aOther

>     mShowFocusRingForContent(false),
>     mFocusByKeyOccurred(false),
>+    mHasGamepad(false),
>     mNotifiedIDDestroyed(false),
>     mTimeoutInsertionPoint(nullptr),
>     mTimeoutPublicIdCounter(1),
>     mTimeoutFiringDepth(0),
>     mJSObject(nullptr),
>     mTimeoutsSuspendDepth(0),
>     mFocusMethod(0),
>     mSerial(0),
> #ifdef DEBUG
>     mSetOpenerWindowCalled(false),
> #endif
>     mCleanedUp(false),
>     mCallCleanUpAfterModalDialogCloses(false),
>+#ifdef MOZ_GAMEPAD
>+    mHasSeenGamepadInput(false),
>+#endif
Would be nice to have these new member variables close each other


> void nsGlobalWindow::SetIsBackground(bool aIsBackground)
> {
>   bool resetTimers = (!aIsBackground && IsBackground());
>   nsPIDOMWindow::SetIsBackground(aIsBackground);
>   if (resetTimers) {
>     ResetTimersForNonBackgroundWindow();
>   }
>+#ifdef MOZ_GAMEPAD
>+  if (!aIsBackground) {
>+    GetCurrentInnerWindowInternal()->SyncGamepadState();
>+  }
>+#endif
Should we do something when a page goes to background?


>+nsGlobalWindow::AddGamepad(PRUint32 aIndex, nsDOMGamepad *aGamepad)
nsDOMGamepad* aGamepad

>+#ifdef MOZ_GAMEPAD
>+  void AddGamepad(PRUint32 aIndex, nsDOMGamepad *aGamepad);
ditto
>+++ b/dom/base/nsPIDOMWindow.h
Update IID


>+[builtinclass, scriptable, uuid(bd09eef8-8e07-4baf-8d39-4f92003dbca8)]
>+interface nsIDOMMozGamepadAxisMoveEvent : nsIDOMMozGamepadEvent
>+{
>+  /**
>+   * Index in gamepad.axes of the axis that was moved.
>+   */
>+  readonly attribute uint32_t axis;
I'd really prefer unsigned long. That is what WebIDL has anyway, and we'll convert this to WebIDL at some point.

>+dictionary MozGamepadAxisMoveEventInit : MozGamepadEventInit
>+{
>+  uint32_t axis;
ditto

>+[builtinclass, scriptable, uuid(d75d4d2b-e7b4-4b93-ac35-2e70b57d9b28)]
>+interface nsIDOMMozGamepadButtonEvent : nsIDOMMozGamepadEvent
>+{
>+  /**
>+   * Index in gamepad.buttons of the button that was pressed or released.
>+   */
>+  readonly attribute uint32_t button;
Same here

>+dictionary MozGamepadButtonEventInit : MozGamepadEventInit
>+{
>+  uint32_t button;
And here


>+[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 uint32_t index;
And here

>+++ b/dom/interfaces/gamepad/nsIDOMGamepadServiceTest.idl
>@@ -0,0 +1,22 @@
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>+ * You can obtain one at http://mozilla.org/MPL/2.0/. */
>+
>+#include "nsISupports.idl"
>+
>+interface nsIVariant;
>+
>+/*
>+ * This interface is intended only for use in tests.
>+ */
>+[scriptable, uuid(7edf77a2-6b3e-4bbb-9100-4452d425feaa)]
>+interface nsIDOMGamepadServiceTest : nsISupports
Please drop 'DOM' from the interface name.
nsIDOM* interfaces and up automatically to global scope.


>+{
>+  uint32_t addGamepad(in string id, in uint32_t numButtons,
>+		      in uint32_t numAxes);
>+  void removeGamepad(in uint32_t index);
>+  void newButtonEvent(in uint32_t index, in uint32_t button,
>+		      in boolean pressed);
>+  void newAxisMoveEvent(in uint32_t index, in uint32_t axis,
>+			in double value);
>+};
unsigned longs, please


>+GamepadService::AddGamepad(const char* aId,
>+                           uint32_t aNumButtons,
>+                           uint32_t aNumAxes)
>+{
>+  //TODO: get initial button/axis state
>+  nsRefPtr<nsDOMGamepad> gamepad =
>+    new nsDOMGamepad(NS_ConvertUTF8toUTF16(nsDependentCString(aId)),
>+                     0,
>+                     aNumButtons,
>+                     aNumAxes);
>+  int index = -1;
>+  for (uint32_t i = 0; i < mGamepads.Length(); i++) {
>+    if (!mGamepads[i]) {
>+      mGamepads[i] = gamepad;
>+      index = i;
>+      break;
>+    }
>+  }
>+  if (index == -1) {
>+    mGamepads.AppendElement(gamepad);
>+    index = mGamepads.Length() - 1;
>+  }
>+
>+  gamepad->SetIndex(index);
>+  NewConnectionEvent(index, true);
>+
>+  return index;
>+}
I wonder... would it be better to use a hashtable for gamepads: index->pad and
pad could know its own index.
That way we wouldn't reuse indexes and the for-loop here wouldn't be needed.



>+
>+void
>+GamepadService::RemoveGamepad(uint32_t aIndex)
>+{
>+  if (aIndex < mGamepads.Length()) {
>+    mGamepads[aIndex]->SetConnected(false);
>+    NewConnectionEvent(aIndex, false);
>+    // If this is the last entry in the list, just remove it.
>+    if (aIndex == mGamepads.Length() - 1) {
>+      mGamepads.RemoveElementAt(aIndex);
>+    } else {
>+      // Otherwise just null it out and leave it, so the
>+      // indices of the following entries remain valid.
>+      mGamepads[aIndex] = NULL;
nullptr


>+GamepadService::FireConnectionEvent(nsIDOMDocument* aDomdoc,
>+                                    nsIDOMEventTarget* aTarget,
>+                                    nsDOMGamepad* aGamepad,
>+                                    bool aConnected)
>+{
>+  nsCOMPtr<nsIDOMEvent> event;
>+  bool defaultActionEnabled = true;
>+  NS_NewDOMMozGamepadEvent(getter_AddRefs(event), NULL, NULL);
>+
>+  if (!event) {
>+    return;
>+  }
NS_New*Event really can't fail


>+
>+  nsCOMPtr<nsIDOMMozGamepadEvent> je = do_QueryInterface(event);
>+
>+  if (!je) {
>+    return;
>+  }
And this kind of case could be just MOZ_ASSERT

>+/* uint32_t addGamepad (in string id, in uint32_t numButtons, in uint32_t numAxes); */
>+NS_IMETHODIMP GamepadServiceTest::AddGamepad(const char* aID,
>+                                             uint32_t aNumButtons,
>+                                             uint32_t aNumAxes,
>+                                             uint32_t* _retval)
aRetVal
Comment 181 Louis-Rémi BABE 2012-08-30 05:07:03 PDT
Can we please make all event names *lowercase* (vendor prefix included), just like what we did for "mozfullscreenchange" and "mozpointerlockchanged".
The convention in event names is lowercase (with a few annoying exceptions).
Comment 182 Ted Mielczarek [:ted.mielczarek] 2012-08-30 05:12:16 PDT
I was not aware of that, we can certainly make that change.
Comment 183 Louis-Rémi BABE 2012-08-30 07:12:32 PDT
That would be great :-)
Comment 184 Olli Pettay [:smaug] 2012-10-30 08:29:29 PDT
Any chance to get review for the OSX part?
Comment 185 Martin Best (:mbest) 2013-01-22 13:31:46 PST
Hey guys, we are starting to see more and more devices shipping that use controllers on Android.  I know Kats is working towards getting Gamepad API working on Android, it's likely time to drive this API to release else where.  

What is the current status, hard to tell based on the comments?
Comment 186 Dietrich Ayala (:dietrich) 2013-02-05 14:26:28 PST
Bump. What'll it take to get this landed pref'd off for developer feedback?
Comment 187 SteveBaker 2013-02-13 08:57:45 PST
What is the current state of this?  I'm running 'nightly' and have an Xbox360 controller installed under Win7 and nothing seems to be working with the example code.  I tried with a Nostromo gamepad under Ubuntu Linux 12.xx - same deal.
Comment 188 Ted Mielczarek [:ted.mielczarek] 2013-02-13 10:21:50 PST
None of this code has landed yet.

Martin/Dietrich: this is mostly gated on my time. It's hard to get a large DOM patch landed, especially when you are doing it in your spare time. Large parts of infrastructure keep getting rewritten which means every time I'd find time to update the patch I'd have to rewrite large parts of it. I think I just ran out of steam to keep it updated. Hopefully I'll be able to find some time in the near future to get things updated to latest trunk and up for another round of review.
Comment 189 Martin Best (:mbest) 2013-02-15 13:55:04 PST
What do you think would be required to land this by the 3rd week in March assuming we could get you free of other requirements on your time?
Comment 190 Vladimir Vukicevic [:vlad] [:vladv] 2013-02-15 16:26:38 PST
Ted, Ehsan may be able to help you get this landed once he gets back from vacation; I'll talk to him.
Comment 191 Ted Mielczarek [:ted.mielczarek] 2013-02-15 17:53:22 PST
(In reply to Martin Best (:mbest) from comment #189)
> What do you think would be required to land this by the 3rd week in March
> assuming we could get you free of other requirements on your time?

I can't imagine a world in which this could happen, sorry.
Comment 192 Ted Mielczarek [:ted.mielczarek] 2013-03-08 12:24:55 PST
Created attachment 722903 [details] [diff] [review]
Add DOM Gamepad APIs

It lives! I rebased this patch to a fairly recent changeset, and made sure it builds and passes the included mochitests. I made some changes from the previous revision, mostly around handling shutdown to avoid hitting assertions/shutdown leaks.

I changed the code to always build the DOM bits unless --disable-gamepad is specified.

Ms2ger has promised to help me WebIDLify this patch, which ought to make it a bit more palatable. Once that's done and we get review, we should be able to land this independent of the backend implementations.
Comment 193 Ted Mielczarek [:ted.mielczarek] 2013-03-08 12:26:43 PST
Created attachment 722904 [details] [diff] [review]
Linux gamepad backend

Linux patch, rebased and tested.
Comment 194 Ted Mielczarek [:ted.mielczarek] 2013-03-08 12:29:00 PST
Created attachment 722905 [details] [diff] [review]
Mac gamepad backend

Mac patch, rebased and tested.
Comment 195 Ted Mielczarek [:ted.mielczarek] 2013-03-08 12:33:39 PST
Comment on attachment 722905 [details] [diff] [review]
Mac gamepad backend

This patch still needs review from someone with Mac knowledge. :-/
Comment 196 Steven Michaud [:smichaud] (Retired) 2013-03-08 13:02:19 PST
I'll try to shove other stuff aside and review this next week.  If others complain about this I'll ask them to do the review themselves :-)

I've had a couple of game controllers for months, now, sitting in their boxes unused.
Comment 197 Ted Mielczarek [:ted.mielczarek] 2013-03-08 17:04:47 PST
Created attachment 723012 [details] [diff] [review]
Windows gamepad backend using DirectInput

Windows patch, rebased and tested.
Comment 198 :Ms2ger (⌚ UTC+1/+2) 2013-03-11 13:10:05 PDT
Created attachment 723611 [details] [diff] [review]
Add DOM Gamepad APIs (on eccf45749400)
Comment 199 Doug Turner (:dougt) 2013-03-13 15:45:00 PDT
ted, can we just land this as xpcom and followup with the webidl.

also -- if smichaud can't review asap, assign the review to me.
Comment 200 Steven Michaud [:smichaud] (Retired) 2013-03-13 16:55:20 PDT
I hope to do the review (plus testing) tomorrow or the day after.
Comment 201 Steven Michaud [:smichaud] (Retired) 2013-03-14 10:07:22 PDT
Comment on attachment 722905 [details] [diff] [review]
Mac gamepad backend

This looks fine to me.  I also got what I think are reasonable test results on the "demo page" (attachment 565617 [details]) with two different game controllers (a Logitech Gamepad F310 and a Shuang long Shocks Joystick):  All the buttons triggered some kind of action in at least one of the controllers' "modes".  (This is the first time I've ever used a game controller, so I can't really say more.)

I do have one question:

> +  void clear() { mDevice = NULL; axes.clear(); mSuperIndex = -1; }

Why doesn't this also call buttons.clear()?

Sorry to have taken so long with this.  Given that I was completely unfamiliar with this functionality and these APIs, I was convinced that I needed to schedule up to two days to finish the review and testing -- and finding two days in my schedule is *very* difficult.  Turns out it took only a few hours.  Sigh.
Comment 202 Ted Mielczarek [:ted.mielczarek] 2013-03-14 10:28:06 PDT
Thanks for the review!

(In reply to Steven Michaud from comment #201)
> > +  void clear() { mDevice = NULL; axes.clear(); mSuperIndex = -1; }
> 
> Why doesn't this also call buttons.clear()?

This looks like an oversight, I'll fix it.
Comment 203 Doug Turner (:dougt) 2013-03-14 10:43:43 PDT
is this ready to go now?
Comment 204 Ted Mielczarek [:ted.mielczarek] 2013-03-15 09:36:17 PDT
(In reply to Doug Turner (:dougt) from comment #203)
> is this ready to go now?

The DOM patch still needs an r+ from smaug. I'm cleaning up a few last things and will attach a patch for his review today.
Comment 205 Ted Mielczarek [:ted.mielczarek] 2013-03-15 10:05:52 PDT
If I didn't reply to a comment assume I fixed it.

(In reply to Olli Pettay [:smaug] from comment #180)
> Should we do something when a page goes to background?

I don't think we need to. The GamepadService checks that windows are foreground before it fires events/updates their gamepad objects. We just need to sync the state of existing gamepad objects to the current state when a window comes back from being in the background.

> I wonder... would it be better to use a hashtable for gamepads: index->pad
> and
> pad could know its own index.
> That way we wouldn't reuse indexes and the for-loop here wouldn't be needed.

Reusing indexes is kind of a feature. It works out well if you disconnect and then reconnect the same gamepad. I agree that having a global gamepad index that just gets incremented for each device would simplify this somewhat, but I don't know that I want to change the reuse behavior.

New patch up in a few minutes.
Comment 206 Ted Mielczarek [:ted.mielczarek] 2013-03-15 10:25:20 PDT
Created attachment 725469 [details] [diff] [review]
Add DOM Gamepad APIs

Updated to address smaug's last review comments. I also went through and unprefixed all the event names here, since that's our new WebAPI policy. I filed bug 851547 on adding a pref to disable the Gamepad API so that we can not ship it on beta. Worst case we can just disable it in configure if we have to for now.
Comment 207 Ted Mielczarek [:ted.mielczarek] 2013-03-15 10:28:12 PDT
Created attachment 725472 [details]
Demo page
Comment 208 Ted Mielczarek [:ted.mielczarek] 2013-03-15 10:29:36 PDT
Pushed the latest patches to try:
https://tbpl.mozilla.org/?tree=Try&rev=a4f23a503e90
Comment 209 Olli Pettay [:smaug] 2013-03-18 07:27:19 PDT
Comment on attachment 725469 [details] [diff] [review]
Add DOM Gamepad APIs

>+  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.
>+    double* array = reinterpret_cast<double*>
>+                              (NS_Alloc(mButtons.Length() * sizeof(double)));
Tiny bit odd indentation. ( should be under i

>+nsGlobalWindow::SetHasGamepadEventListener(bool aHasGamepad/* = true*/)
>+{
>+  mHasGamepad = aHasGamepad;
>+  if (aHasGamepad)
>+    EnableGamepadUpdates();
Missing {}

>+already_AddRefed<nsDOMGamepad>
>+nsGlobalWindow::GetGamepad(PRUint32 aIndex)
>+{
This could probably have the macro to forward to inner window
FORWARD_TO_INNER

>+  nsRefPtr<nsDOMGamepad> gamepad;
>+  if (mGamepads.Get(aIndex, getter_AddRefs(gamepad))) {
>+    return gamepad.forget();
>+  }
>+
>+  return NULL;
nullptr


>+}
>+
>+// static
>+PLDHashOperator
>+nsGlobalWindow::EnumGamepadsForSync(const PRUint32& aKey, nsDOMGamepad* aData, void* userArg)
>+{
>+  nsRefPtr<GamepadService> gamepadsvc(GamepadService::GetService());
>+  gamepadsvc->SyncGamepadState(aKey, aData);
>+  return PL_DHASH_NEXT;
>+}
>+
>+void
>+nsGlobalWindow::SyncGamepadState()
>+{
>+  if (mHasSeenGamepadInput) {
>+    mGamepads.EnumerateRead(EnumGamepadsForSync, NULL);
>+  }
>+}
>+#endif


> NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(nsGlobalChromeWindow,
>+#ifdef MOZ_GAMEPAD
>+  void AddGamepad(PRUint32 aIndex, nsDOMGamepad* aGamepad);
>+  void RemoveGamepad(PRUint32 aIndex);
>+  already_AddRefed<nsDOMGamepad> GetGamepad(PRUint32 aIndex);
>+  void SetHasSeenGamepadInput(bool aHasSeen) { mHasSeenGamepadInput = aHasSeen; }
>+  bool HasSeenGamepadInput() { return mHasSeenGamepadInput; }
>+  void SyncGamepadState();
>+  static PLDHashOperator EnumGamepadsForSync(const PRUint32& aKey,
>+                                             nsDOMGamepad* aData,
>+                                             void* userArg);
>+#endif
>+
>+  // Enable/disable updates for gamepad input.
>+  void EnableGamepadUpdates();
>+  void DisableGamepadUpdates();
>+
>+


>+  // Indicates whether this window wants gamepad input events
>+  bool                   mHasGamepad : 1;
>+#ifdef MOZ_GAMEPAD
>+  nsRefPtrHashtable<nsUint32HashKey, nsDOMGamepad> mGamepads;
>+  bool mHasSeenGamepadInput;
>+#endif
Hmm, so it is a bit unclear in which window mGamepads actually live.
It is initialized both in outer and inner, but I assume it should be used only in inner.
Could you fix that, and perhaps add some MOZ_ASSERTs to make sure it is used only in inner window


>   /**
>+   * Tell this window that there is an observer for gamepad input
>+   */
>+  virtual void SetHasGamepadEventListener(bool aHasGamepad=true) = 0;
space before and after =



>+
>+  /**
>    * Set a arguments for this window. This will be set on the window
>    * right away (if there's an existing document) and it will also be
>    * installed on the window when the next document is loaded. Each
>    * language impl is responsible for converting to an array of args
>    * as appropriate for that language.
>    */
>   virtual nsresult SetArguments(nsIArray *aArguments, nsIPrincipal *aOrigin) = 0;
> 
>   /**
>    * NOTE! This function *will* be called on multiple threads so the
>+interface nsIDOMGamepadAxisMoveEvent : nsIDOMGamepadEvent
This is not in the spec. I'm assuming it will be added.

>+interface nsIDOMGamepadButtonEvent : nsIDOMGamepadEvent
ditto

>+[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;
Please file a bug to add this to the spec.

I filed a bug (https://www.w3.org/Bugs/Public/show_bug.cgi?id=21322) to make
.timestamp implementable. The current spec is just totally unclear.
But please file a followup bug to implement .timestamp.



>+namespace {
>+// Amount of time to wait before cleaning up gamepad resources
>+// when no pages are listening for events.
>+const int kCleanupDelayMS = 2000;
>+const nsTArray<nsRefPtr<nsGlobalWindow> >::index_type NoIndex =
>+    nsTArray<nsRefPtr<nsGlobalWindow> >::NoIndex;
>+
>+StaticRefPtr<GamepadService> sGamepadServiceSingleton;
Hmm, isn't this technically global. So g...

>+  return NS_OK;
>+}
>+
>+
>+void
Extra newline before void


>+GamepadService::AddGamepad(const char* aId,
>+                           uint32_t aNumButtons,
>+                           uint32_t aNumAxes)
>+{
>+  //TODO: get initial button/axis state
File a followup bug about this TODO and add a

>+
>+  //TODO: the spec uses double values for buttons, to allow for analog
>+  // buttons.
Any reason we can't use double here?

>     # Audio
>     'nsIDOMNotifyAudioAvailableEvent.frameBuffer',
>     'nsIDOMNotifyAudioAvailableEvent.time',
>     'nsIDOMHTMLAudioElement.mozWriteAudio',
> 
>+    # Gamepad
>+    'nsIDOMGamepad.*',
>+    'nsIDOMGamepadEvent.*',
>+    'nsIDOMGamepadButtonEvent.*',
>+    'nsIDOMGamepadAxisMoveEvent.*',
>+
Hmm, can we #ifdef this, or do we need to leave these out for now?


>+++ b/js/xpconnect/src/event_impl_gen.conf.in
>@@ -36,21 +36,24 @@
>     'MozWifiStatusChangeEvent',
>     'MozWifiConnectionInfoEvent',
>     'MozCellBroadcastEvent',
>     'MozVoicemailEvent',
>     'USSDReceivedEvent',
> #endif
>     'ElementReplaceEvent',
>     'MozSmsEvent',
>     'DeviceStorageChangeEvent',
>     'PopupBlockedEvent',
>-    'BlobEvent'
>+    'BlobEvent',
>+    'GamepadEvent',
>+    'GamepadButtonEvent',
>+    'GamepadAxisMoveEvent',
These should be inside #ifdef gamepad?

Would like to see a new patch especially because of the inner/outer window thing.
Comment 210 Ted Mielczarek [:ted.mielczarek] 2013-03-18 12:53:35 PDT
Anything I didn't reply to here I'm fixing in a new patch. Will upload it in a little bit once I make sure it all compiles.

(In reply to Olli Pettay [:smaug] from comment #209)
> Hmm, so it is a bit unclear in which window mGamepads actually live.
> It is initialized both in outer and inner, but I assume it should be used
> only in inner.
> Could you fix that, and perhaps add some MOZ_ASSERTs to make sure it is used
> only in inner window

It should be only inner windows, I think. I made all the methods that access gamepad-related fields to FORWARD_TO_INNER. I don't know where else I'd put assertions, did you have something specific in mind?

> >+interface nsIDOMGamepadAxisMoveEvent : nsIDOMGamepadEvent
> This is not in the spec. I'm assuming it will be added.
> 
> >+interface nsIDOMGamepadButtonEvent : nsIDOMGamepadEvent
> ditto

I'm not totally sure. Scott and I need to discuss this. Scott drafted most of the spec based on some mashup of what we'd both implemented, but he left these out, and I hadn't added them. I'm not sure if we actually want them or not. I'll file a spec bug or start a discussion on the mailing list.

> >+  readonly attribute boolean connected;
> Please file a bug to add this to the spec.

Filed: https://www.w3.org/Bugs/Public/show_bug.cgi?id=21323

> 
> I filed a bug (https://www.w3.org/Bugs/Public/show_bug.cgi?id=21322) to make
> .timestamp implementable. The current spec is just totally unclear.
> But please file a followup bug to implement .timestamp.

Filed bug 852257.

> >+  //TODO: get initial button/axis state
> File a followup bug about this TODO and add a

Filed bug 852258.

> >+
> >+  //TODO: the spec uses double values for buttons, to allow for analog
> >+  // buttons.
> Any reason we can't use double here?

So from the perspective of content we're spec-compliant here. We report "1.0" for pressed buttons and "0.0" for unpressed buttons. The internal implementation is a little more difficult, because none of the platform APIs I used actually support analog buttons. This will likely get fixed when I figure out how to do smart mapping of platform-specific controller layouts.

> >+    # Gamepad
> >+    'nsIDOMGamepad.*',
> >+    'nsIDOMGamepadEvent.*',
> >+    'nsIDOMGamepadButtonEvent.*',
> >+    'nsIDOMGamepadAxisMoveEvent.*',
> >+
> Hmm, can we #ifdef this, or do we need to leave these out for now?

I just removed them, since we're going to WebIDLify this all anyway.
Comment 211 Ted Mielczarek [:ted.mielczarek] 2013-03-18 13:15:05 PDT
Created attachment 726307 [details] [diff] [review]
Add DOM Gamepad APIs

With review comments addressed.
Comment 212 Ted Mielczarek [:ted.mielczarek] 2013-03-18 13:19:03 PDT
Created attachment 726309 [details] [diff] [review]
Add DOM Gamepad APIs

Fixed the test_interfaces.html breakage I saw on try as well.
Comment 213 Olli Pettay [:smaug] 2013-03-20 02:54:53 PDT
Comment on attachment 726309 [details] [diff] [review]
Add DOM Gamepad APIs


>+#ifdef MOZ_GAMEPAD
>+  if (!aIsBackground) {
>+    nsGlobalWindow *inner = GetCurrentInnerWindowInternal();
nsGlobalWindow* inner



>+GamepadService::TimeoutHandler(nsITimer *aTimer, void *aClosure)
nsITimer* ... void*
Comment 216 :aceman 2013-03-21 14:43:20 PDT
This seems to cause the following error on Linux with gcc:

In file included from mozilla/hal/linux/LinuxGamepad.cpp:13:0:
../dist/system_wrappers/glib.h:3:23: fatal error: glib.h: File does not exist
compilation terminated.
gmake[5]: *** [LinuxGamepad.o] Error 1
gmake[5]: *** Waiting for unfinished jobs....
gmake[4]: *** [libs_tier_platform] Error 2
gmake[3]: *** [tier_platform] Error 2
gmake[2]: *** [default] Error 2
gmake[1]: *** [default] Error 2
gmake: *** [build] Error 2

Reverting to the parent revision of the linux check-in (2e774935eaec) helped me.
Comment 217 K. Gadd (:kael) 2013-03-22 08:53:15 PDT
On Win32 if my gamepad is connected it shows up in the page within a few seconds without me sending any input. Not sure why; maybe drift in the analog stick values is showing up as input?

What happened to the Moz prefix? Is this API standardized? If so, neat. Otherwise, is the prefix going to reappear when it heads to Beta/Release or is the current API safe to use in apps?

What's the recommended way to feature-detect the presence of this API? I don't see 'ongamepadxxx' event properties on document or window, so should we check for the presence of GamepadEvent?
Comment 218 Ted Mielczarek [:ted.mielczarek] 2013-03-22 09:03:57 PDT
(In reply to Kevin Gadd (:kael) from comment #217)
> On Win32 if my gamepad is connected it shows up in the page within a few
> seconds without me sending any input. Not sure why; maybe drift in the
> analog stick values is showing up as input?

This is possible, I don't do anything with deadzones yet. We should probably do that. File a bug?

> What happened to the Moz prefix? Is this API standardized? If so, neat.
> Otherwise, is the prefix going to reappear when it heads to Beta/Release or
> is the current API safe to use in apps?

Our current Web API strategy is to allow experimental features on nightly/aurora, as long as they're standards-track, AIUI. Since there is a spec here and we're attempting to conform to it, I'm not using a prefix. If the spec/impl aren't in a good place before this would hit beta I'll just disable it for that release (hopefully behind a pref).

> What's the recommended way to feature-detect the presence of this API? I
> don't see 'ongamepadxxx' event properties on document or window, so should
> we check for the presence of GamepadEvent?

This sucks at the moment, I need to fix bug 690935. :-/
Comment 219 Alex Keybl [:akeybl] 2013-03-29 13:13:12 PDT
We'll use the new tag of "experimental" in bug 777929 to release note this for FF22, since bug 851547 will likely be used to disable this on Beta.
Comment 220 Ted Mielczarek [:ted.mielczarek] 2013-03-29 15:34:58 PDT
Per discussion in bug 851547, the pref support isn't likely to land in time for 22, so we'll be turning of the feature entirely for 22.
Comment 221 Alex Keybl [:akeybl] 2013-05-08 13:13:38 PDT
Please set relnote-firefox back to ? once we decide this can stick and we stop disabling/backing out in bug 857797.
Comment 222 Mihaela Velimiroviciu (:mihaelav) 2014-02-25 04:28:54 PST
Are there any other automated tests needed for this feature?
Comment 223 Ted Mielczarek [:ted.mielczarek] 2014-02-26 05:25:29 PST
I landed some Mochitests with this feature. As with all things it's not exhaustive, but I think it's sufficient for now. Thanks!

Note You need to log in before you can comment on or make changes to this bug.