Implement navigator.getGamepads() method

RESOLVED FIXED in mozilla24

Status

()

Core
DOM
RESOLVED FIXED
6 years ago
4 months ago

People

(Reporter: ted, Assigned: ted)

Tracking

(Depends on: 1 bug, {dev-doc-complete})

Trunk
mozilla24
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

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

Updated

6 years ago
Keywords: dev-doc-needed

Comment 1

6 years ago
document.gamepads?
Wouldn't window.gamepads or navigator.gamepads make more sense.
(Assignee)

Comment 2

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

Scott: do you remember why?

Comment 3

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

Comment 4

6 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?
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.
(Assignee)

Updated

6 years ago
Summary: Implement document.gamepads property. → Implement navigator.gamepads property.

Comment 6

6 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?
(Assignee)

Updated

5 years ago
Assignee: nobody → ted.mielczarek
Status: NEW → ASSIGNED
(Assignee)

Comment 7

5 years ago
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

5 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
(Assignee)

Comment 9

4 years ago
The spec changed to make this a method:
https://dvcs.w3.org/hg/gamepad/raw-file/default/gamepad.html#methods
Summary: Implement navigator.gamepads property. → Implement navigator.getGamepads() method
Ted, do you know what is the rationale behind that change?
Flags: needinfo?(ted)

Comment 11

4 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.
Flags: needinfo?(ted)
(Assignee)

Comment 12

4 years ago
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.
(Assignee)

Updated

4 years ago
Attachment #595600 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #732863 - Attachment description: implement navigator.mozGamepads → implement navigator.gamepads
(Assignee)

Comment 13

4 years ago
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.
(Assignee)

Updated

4 years ago
Attachment #732863 - Attachment is obsolete: true
(Assignee)

Comment 14

4 years ago
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.
(Assignee)

Updated

4 years ago
Attachment #732990 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Blocks: 860409
(Assignee)

Updated

4 years ago
Blocks: 860413

Comment 15

4 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.
(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.
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.
(Assignee)

Comment 18

4 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.
(Assignee)

Comment 19

4 years ago
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/.
Attachment #764491 - Flags: review?(bugs)
(Assignee)

Updated

4 years ago
Attachment #733486 - Attachment is obsolete: true
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
Attachment #764491 - Flags: review?(bugs) → review+
(Assignee)

Comment 21

4 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.
(Assignee)

Updated

4 years ago
Depends on: 851547
(Assignee)

Comment 22

4 years ago
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.
(Assignee)

Updated

4 years ago
Attachment #764491 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
(Assignee)

Comment 23

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a159e3dad7b
https://hg.mozilla.org/mozilla-central/rev/7a159e3dad7b
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
relnote-firefox: --- → ?
(Assignee)

Comment 25

4 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).
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. :)
relnote-firefox: ? → ---
Doc updated:
https://developer.mozilla.org/en-US/Firefox/Releases/24
and
https://developer.mozilla.org/en-US/docs/Web/API/Navigator.getGamepads
Keywords: dev-doc-needed → dev-doc-complete
Depends on: 1337161
You need to log in before you can comment on or make changes to this bug.