Closed Bug 926091 Opened 6 years ago Closed 6 years ago

Make Gamepad.buttons into an array of GamepadButton objects

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: ted, Assigned: ted)

References

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 4 obsolete files)

I made a spec change to change Gamepad.buttons into an array of GamepadButton objects instead of just doubles:
https://dvcs.w3.org/hg/gamepad/rev/a37c10a031b4
https://dvcs.w3.org/hg/gamepad/raw-file/default/gamepad.html#widl-Gamepad-buttons

I wanted to do this now before we ship anything because it's a breaking change. This patch implements the new spec behavior.

Unfortunately it's not perfect, it's still failing one Mochitest, and also leaking during Mochitests.
Blocks: 878828
Is this what NS_FREE_XPCOM_ISUPPORTS_POINTER_ARRAY is for?
Ok, I fixed the leak and the test failure. The leak was due to me unnecessarily AddRef'ing the nsISupports* I was passing to the variant. The test failure was because Gamepad::SyncState was no longer doing the right thing now that mButtons was an array of objects and not just doubles. I fixed both of those.
Attachment #817922 - Flags: review?(bugs)
Attachment #816254 - Attachment is obsolete: true
Comment on attachment 817922 [details] [diff] [review]
Make Gamepad.buttons into an array of GamepadButton objects

I think we should actually cache the variant for buttons.
(and cycle collect it then)

Also, GamepadButton really should have some sane parent, the same what Gamepad has. That way if chrome/addon touches GamepadButton first, content can still
touch the button without getting some security exception.
Attachment #817922 - Flags: review?(bugs) → review-
I added caching of both variants on Gamepad. I also made GamepadButton use
the parent of the Gamepad that creates it.
Attachment #818019 - Flags: review?(bugs)
Attachment #817922 - Attachment is obsolete: true
Comment on attachment 818019 [details] [diff] [review]
Make Gamepad.buttons into an array of GamepadButton objects

> Gamepad::GetButtons(nsIVariant** aButtons)
> {
>+  if (mButtonsVariant) {
>+    nsCOMPtr<nsIVariant> buttons = mButtonsVariant;
>+    *aButtons = buttons.forget().get();
I'd write this
NS_ADDREF(*aButtons = mButtonsVariant);

> Gamepad::GetAxes(nsIVariant** aAxes)
> {
>+  if (mAxesVariant) {
>+    nsCOMPtr<nsIVariant> axes = mAxesVariant;
>+    *aAxes = axes.forget().get();
>+    return NS_OK;
>+  }
Unfortunately we can't quite do this.
the values of axes may change, unlike buttons in the current impl
Attachment #818019 - Flags: review?(bugs) → review-
Thanks. I don't know why I didn't realize that.
Attachment #818413 - Flags: review?(bugs)
Attachment #818019 - Attachment is obsolete: true
Comment on attachment 818413 [details] [diff] [review]
Make Gamepad.buttons into an array of GamepadButton objects

nsTArray<nsRefPtr<GamepadButton>>
space between > and > (unless all our compilers supports >> already)

>+interface GamepadButton {
>+    readonly    attribute boolean pressed;
>+    readonly    attribute double  value;
>+};
2 space indentation please.
Attachment #818413 - Flags: review?(bugs) → review+
(In reply to comment #8)
> Comment on attachment 818413 [details] [diff] [review]
>   --> https://bugzilla.mozilla.org/attachment.cgi?id=818413
> Make Gamepad.buttons into an array of GamepadButton objects
> 
> nsTArray<nsRefPtr<GamepadButton>>
> space between > and > (unless all our compilers supports >> already)

They do now!!!  Go crazy with >>'s.  :-)
My try push showed that I forgot to fix test_interfaces.html to include GamepadButton. smaug gave r+ over IRC to make that change.
Attachment #818413 - Attachment is obsolete: true
Comment on attachment 818605 [details] [diff] [review]
Make Gamepad.buttons into an array of GamepadButton objects.

r=smaug from the previous patch and r=smaug over IRC for the test change.
Attachment #818605 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/2c7feaa3d1be
https://hg.mozilla.org/mozilla-central/rev/1cf18e533f44
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #9)

> > nsTArray<nsRefPtr<GamepadButton>>
> > space between > and > (unless all our compilers supports >> already)
> 
> They do now!!!  Go crazy with >>'s.  :-)

Preparing to file the s/> >/>>/ bug.... ;-)
Note that this required a clobber for Windows dep build bustage.
The page https://developer.mozilla.org/en-US/docs/Web/API/Gamepad.buttons and the previous version has not been released (on by default) in a final version. So I think this is ok like this.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.