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

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.

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

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

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

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

Scott: do you remember why?

	Scott Graham
	Comment 3 • 13 years ago

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?

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

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?

	Mounir Lamouri (:mounir)
	Comment 5 • 13 years ago

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.

	Scott Graham
	Comment 6 • 13 years ago

(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?

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

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.

	Paul Irish
	Comment 8 • 12 years ago

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

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

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

	Mounir Lamouri (:mounir)
	Comment 10 • 11 years ago

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

	Scott Graham
	Comment 11 • 11 years ago

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

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

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.

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

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

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

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.

	Rahly
	Comment 15 • 11 years ago

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.

	Mounir Lamouri (:mounir)
	Comment 16 • 11 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 17 • 11 years ago

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.

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

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.

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

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

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 20 • 11 years ago

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

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

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.

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

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

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

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

	Ed Morley [:emorley]
	Comment 24 • 11 years ago

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

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

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

	Lawrence Mandel [:lmandel] (use needinfo)
	Comment 26 • 11 years ago

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

	Jean-Yves Perrier [:teoli]
	Comment 27 • 10 years ago

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