690935 - Implement navigator.getGamepads() method

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[(not currently active) Ted Mielczarek]

This was discussed in bug 604039, and is almost certainly going to wind up in the spec, but I don't want to block landing my initial patches on getting this working, so I'll leave it as a followup.

Comment 1

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

document.gamepads?
Wouldn't window.gamepads or navigator.gamepads make more sense.

Comment 2

[(not currently active) Ted Mielczarek]

This was suggested somewhere, in the bug or in WG discussion, I can't recall why exactly.

Scott: do you remember why?

Comment 3

[Scott Graham]

I'm not sure if it was publically discussed. document, navigator, and window seemed like the reasonable places. Rationale was:

a) navigator is a "dead end" and new things aren't being standardized there (?).

b) window is a bad because it implicates interception making it global.

which leaves document. Is there good arguments for a different location?

Comment 4

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

How is navigator "dead end"? New stuff are being added there. See for example
http://www.whatwg.org/specs/web-apps/current-work/multipage/video-conferencing-and-peer-to-peer-communication.html#obtaining-local-multimedia-content

The problem with document is that it has nothing to do with gamepads. Would xhr.responseXML have gamepads property?

Comment 5

[Mounir Lamouri (:mounir)]

The WebAPI specifications tends to add stuff to window.navigator instead of window. Adding this to document seems a very bad idea for the reasons mentioned.

Comment 6

[Scott Graham]

(In reply to Olli Pettay [:smaug] from comment #4)
> How is navigator "dead end"? New stuff are being added there. See for example
> http://www.whatwg.org/specs/web-apps/current-work/multipage/video-
> conferencing-and-peer-to-peer-communication.html#obtaining-local-multimedia-
> content
> 
> The problem with document is that it has nothing to do with gamepads. Would
> xhr.responseXML have gamepads property?

Fair enough, makes sense and seems like a better location. Does it seem a problem that the contents of navigator might change between documents?

Comment 7

[(not currently active) Ted Mielczarek]

Attachment: implement navigator.mozGamepads

This isn't finished but it mostly works. There are some stupid hunks in this patch because I shuffled things around a few times and didn't put them back where I originally had them. I'll clean that up later. This needs more tests. I added a simple test, and it passes, but the Mochitests leak, so I'm doing something wrong.

Comment 8

[Paul Irish]

Looking forward to this. As of now, there isn't a way to feature detect Firefox's gamepad implementation: http://www.html5rocks.com/en/tutorials/doodles/gamepad/#toc-featuredetect

Comment 9

[(not currently active) Ted Mielczarek]

The spec changed to make this a method:
https://dvcs.w3.org/hg/gamepad/raw-file/default/gamepad.html#methods

Comment 10

[Mounir Lamouri (:mounir)]

Ted, do you know what is the rationale behind that change?

Comment 11

[Scott Graham]

(In reply to Mounir Lamouri (:mounir) from comment #10)
> Ted, do you know what is the rationale behind that change?

It was because content (that is otherwise uninterested in gamepads) was enumerating navigator, which, when gamepads was a simple array caused background stuff to spin up unnecessarily.

Comment 12

[(not currently active) Ted Mielczarek]

Attachment: implement navigator.gamepads

I'd like to change the spec to be "navigator.gamepads.get()" instead, since that has a lot of other nice properties (gives us a place to hang future features off of, can use navigator.gamepads as an EventTarget). Here's the start of an implementation that does that. Currently .get() just returns an empty list.

I moved all the gamepad code under dom/gamepad, and s/nsDOMGamepad/Gamepad/ while I was at it.

Comment 13

[(not currently active) Ted Mielczarek]

Attachment: implement navigator.gamepads.get()

...and this hooks up navigator.gamepads.get() to do the right thing. I'll hold off on getting review until I see if I can get the spec changed to match.

Comment 14

[(not currently active) Ted Mielczarek]

Attachment: implement navigator.gamepads.get()

This patch actually works when you have >1 gamepad. Also I made the test test that we can have holes in the list of gamepads.

Comment 15

[Rahly]

If we are going to have a method for doing this, I think navigator.gamepads.enumerate(function(pad) {/* or this = pad*/}); would be a better solution.  This could allow a system thread to query the gamepads without potentially locking up the user interface/javascript, pads would be received asynchronously.  navigator.gamepads.get() would have to be a blocking call and cannot return until all gamepads are queried.

Comment 16

[Mounir Lamouri (:mounir)]

(In reply to Rahly from comment #15)
> navigator.gamepads.get() would have to be a
> blocking call and cannot return until all gamepads are queried.

navigator.gamepads.get() could definitely return a promise-like object like DOM Future.

Comment 17

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

Well, aren't all the gamepads notified with connect event? If so, I'd expect the array to return
only those gamepads which have been notified about already. So browser wouldn't need to
query OS about them.

Comment 18

[(not currently active) Ted Mielczarek]

Right, the API is intended to only return the current state of all known gamepads, not to enumerate over all gamepads in the system. We might need to tighten up the spec language around that.

Comment 19

[(not currently active) Ted Mielczarek]

Attachment: implement navigator.getGamepads()

I changed this to match the existing spec, it implements navigator.getGamepads(). Slightly more hassle because I can't use WebIDL bindings for that method, but not terrible, and actually a net reduction of code vs. the previous patch.

Note that as previously this patch includes some cleanup where I've moved all the gamepad code to dom/gamepad, and renamed s/nsDOMGamepad/mozilla::dom::Gamepad/.

Comment 20

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

Comment on attachment 764491 [details] [diff] [review]
implement navigator.getGamepads()


>+NS_IMETHODIMP
>+Navigator::GetGamepads(nsIVariant** _retval)
nsIVariant** aRetVal

>+  if (gamepads.Length() == 0) {
>+    nsresult rv = out->SetAsEmptyArray();
>+    NS_ENSURE_SUCCESS(rv, rv);
Variant API is so user-hostile. Luckily we don't have to use it too often


>+  } else {
>+    out->SetAsArray(nsIDataType::VTYPE_INTERFACE,
>+                    &NS_GET_IID(nsISupports),
>+                    gamepads.Length(),
>+                    const_cast<void*>(static_cast<const void*>(gamepads.Elements())));
>+  }
>+  out.forget(_retval);
>+
>+  return NS_OK;
>+}
So it is expected to return a new array each time the method is called?
Per spec, it looks like so, so this impl is ok, but just wondering if sequence<Gamepad> would make sense.


>+nsGlobalWindow::EnumGamepadsForGet(const uint32_t& aKey, Gamepad* aData,
>+                                   void* userArg)
>+{
>+  nsTArray<nsRefPtr<Gamepad> >* array =
>+    reinterpret_cast<nsTArray<nsRefPtr<Gamepad> >*>(userArg);
why you need reinterpret_cast? static_cast should work, I think.
s/userArg/aUserArg/

>+  array->EnsureLengthAtLeast(aKey + 1);
set the capacity of the array before you call EnumerateRead.
Capacity should be the same as the Count() of mGamepads.

>-nsGlobalWindow::EnumGamepadsForSync(const uint32_t& aKey, nsDOMGamepad* aData, void* userArg)
>+nsGlobalWindow::EnumGamepadsForSync(const uint32_t& aKey, Gamepad* aData,
>+                                    void* userArg)
aUserArg

>   static PLDHashOperator EnumGamepadsForSync(const uint32_t& aKey,
>-                                             nsDOMGamepad* aData,
>+                                             mozilla::dom::Gamepad* aData,
>                                              void* userArg);
>+  static PLDHashOperator EnumGamepadsForGet(const PRUint32& aKey,
>+                                            mozilla::dom::Gamepad* aData,
>+                                            void* userArg);
ditto

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

Comment 21

[(not currently active) Ted Mielczarek]

I cargo-culted a lot of GetGamepads from GetDeviceStorages, so any weirdness is probably a result of that.

The spec does have "Gamepad[] getGamepads()", that probably wants to be "sequence<Gamepad> getGamepads()". Maybe when we get WebIDL for Navigator I'll deal with that.

Comment 22

[(not currently active) Ted Mielczarek]

Attachment: implement navigator.getGamepads()

Just attaching this for reference, this fixes all the review comments and also a fix from rebasing on top of bug 847611.

Comment 23

[(not currently active) Ted Mielczarek]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7a159e3dad7b

Comment 24

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7a159e3dad7b

Comment 25

[(not currently active) Ted Mielczarek]

Why do you think this deserves a relnote? (just curious) Note that the Gamepad API is still slated to be shipped preffed off in 24 (as opposed to disabled before beta in previous releases).

Comment 26

[Lawrence Mandel [:lmandel] (use needinfo)]

I did a drive by addition of the relnote? flag to a bunch of bugs. I knew some would be incorrectly marked. Thanks for identifying this one. :)

Comment 27

[Jean-Yves Perrier [:teoli]]

Doc updated:
https://developer.mozilla.org/en-US/Firefox/Releases/24
and
https://developer.mozilla.org/en-US/docs/Web/API/Navigator.getGamepads