Closed Bug 690935 Opened 8 years ago Closed 7 years ago

Implement navigator.getGamepads() method

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: ted, Assigned: ted)

References

()

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 5 obsolete files)

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.
Keywords: dev-doc-needed
document.gamepads?
Wouldn't window.gamepads or navigator.gamepads make more sense.
This was suggested somewhere, in the bug or in WG discussion, I can't recall why exactly.

Scott: do you remember why?
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?
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.
Summary: Implement document.gamepads property. → Implement navigator.gamepads property.
(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: nobody → ted.mielczarek
Status: NEW → ASSIGNED
Attached patch implement navigator.mozGamepads (obsolete) — Splinter Review
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.
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
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)
(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)
Attached patch implement navigator.gamepads (obsolete) — Splinter Review
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.
Attachment #595600 - Attachment is obsolete: true
Attachment #732863 - Attachment description: implement navigator.mozGamepads → implement navigator.gamepads
...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.
Attachment #732863 - Attachment is obsolete: true
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.
Attachment #732990 - Attachment is obsolete: true
Blocks: 860409
Blocks: 860413
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.
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.
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)
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+
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.
Depends on: 851547
Just attaching this for reference, this fixes all the review comments and also a fix from rebasing on top of bug 847611.
Attachment #764491 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/7a159e3dad7b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
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: ? → ---
Depends on: 1337161
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.