Last Comment Bug 690935 - Implement navigator.getGamepads() method
: Implement navigator.getGamepads() method
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla24
Assigned To: Ted Mielczarek [:ted.mielczarek]
:
Mentors:
https://dvcs.w3.org/hg/gamepad/raw-fi...
Depends on: 604039 851547
Blocks: 860409 860413
  Show dependency treegraph
 
Reported: 2011-09-30 15:03 PDT by Ted Mielczarek [:ted.mielczarek]
Modified: 2014-09-12 08:19 PDT (History)
18 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
implement navigator.mozGamepads (19.47 KB, patch)
2012-02-08 18:08 PST, Ted Mielczarek [:ted.mielczarek]
no flags Details | Diff | Splinter Review
implement navigator.gamepads (44.82 KB, patch)
2013-04-03 08:47 PDT, Ted Mielczarek [:ted.mielczarek]
no flags Details | Diff | Splinter Review
implement navigator.gamepads.get() (46.20 KB, patch)
2013-04-03 13:02 PDT, Ted Mielczarek [:ted.mielczarek]
no flags Details | Diff | Splinter Review
implement navigator.gamepads.get() (47.51 KB, patch)
2013-04-04 12:16 PDT, Ted Mielczarek [:ted.mielczarek]
no flags Details | Diff | Splinter Review
implement navigator.getGamepads() (42.95 KB, patch)
2013-06-18 18:09 PDT, Ted Mielczarek [:ted.mielczarek]
bugs: review+
Details | Diff | Splinter Review
implement navigator.getGamepads() (43.83 KB, patch)
2013-06-19 11:35 PDT, Ted Mielczarek [:ted.mielczarek]
no flags Details | Diff | Splinter Review

Description Ted Mielczarek [:ted.mielczarek] 2011-09-30 15:03:08 PDT
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] (TPAC) 2011-10-01 01:29:35 PDT
document.gamepads?
Wouldn't window.gamepads or navigator.gamepads make more sense.
Comment 2 Ted Mielczarek [:ted.mielczarek] 2011-10-01 06:40:25 PDT
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 2011-10-01 15:53:34 PDT
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] (TPAC) 2011-10-02 01:57:12 PDT
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) 2011-10-03 08:55:04 PDT
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 2011-10-03 10:06:49 PDT
(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 Ted Mielczarek [:ted.mielczarek] 2012-02-08 18:08:06 PST
Created attachment 595600 [details] [diff] [review]
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 2012-08-06 11:31:48 PDT
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 Ted Mielczarek [:ted.mielczarek] 2013-03-22 09:12:00 PDT
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) 2013-03-22 09:47:03 PDT
Ted, do you know what is the rationale behind that change?
Comment 11 Scott Graham 2013-03-22 09:49:09 PDT
(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 Ted Mielczarek [:ted.mielczarek] 2013-04-03 08:47:33 PDT
Created attachment 732863 [details] [diff] [review]
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 Ted Mielczarek [:ted.mielczarek] 2013-04-03 13:02:24 PDT
Created attachment 732990 [details] [diff] [review]
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 Ted Mielczarek [:ted.mielczarek] 2013-04-04 12:16:54 PDT
Created attachment 733486 [details] [diff] [review]
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 2013-04-28 06:20:44 PDT
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) 2013-04-28 09:58:55 PDT
(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] (TPAC) 2013-04-28 14:10:09 PDT
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 Ted Mielczarek [:ted.mielczarek] 2013-04-29 04:57:55 PDT
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 Ted Mielczarek [:ted.mielczarek] 2013-06-18 18:09:07 PDT
Created attachment 764491 [details] [diff] [review]
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] (TPAC) 2013-06-19 06:09:09 PDT
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 Ted Mielczarek [:ted.mielczarek] 2013-06-19 06:16:53 PDT
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 Ted Mielczarek [:ted.mielczarek] 2013-06-19 11:35:55 PDT
Created attachment 764898 [details] [diff] [review]
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 Ted Mielczarek [:ted.mielczarek] 2013-06-19 12:46:52 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a159e3dad7b
Comment 24 Ed Morley [:emorley] 2013-06-20 01:28:04 PDT
https://hg.mozilla.org/mozilla-central/rev/7a159e3dad7b
Comment 25 Ted Mielczarek [:ted.mielczarek] 2013-06-21 11:19:11 PDT
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) 2013-06-21 12:17:32 PDT
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. :)

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