926091 - Make Gamepad.buttons into an array of GamepadButton objects

Status: RESOLVED FIXED

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

Description

[(not currently active) Ted Mielczarek]

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.

Comment 1

[(not currently active) Ted Mielczarek]

Attachment: Make Gamepad.buttons into an array of GamepadButton objects

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Is this what NS_FREE_XPCOM_ISUPPORTS_POINTER_ARRAY is for?

Comment 3

[(not currently active) Ted Mielczarek]

Attachment: Make Gamepad.buttons into an array of GamepadButton objects

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.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 5

[(not currently active) Ted Mielczarek]

Attachment: Make Gamepad.buttons into an array of GamepadButton objects

I added caching of both variants on Gamepad. I also made GamepadButton use
the parent of the Gamepad that creates it.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 7

[(not currently active) Ted Mielczarek]

Attachment: Make Gamepad.buttons into an array of GamepadButton objects

Thanks. I don't know why I didn't realize that.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 9

[(no longer active)]

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

Comment 10

[(not currently active) Ted Mielczarek]

Attachment: Make Gamepad.buttons into an array of GamepadButton objects.

My try push showed that I forgot to fix test_interfaces.html to include GamepadButton. smaug gave r+ over IRC to make that change.

Comment 11

[(not currently active) Ted Mielczarek]

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.

Comment 12

[(not currently active) Ted Mielczarek]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2c7feaa3d1be

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/2c7feaa3d1be
https://hg.mozilla.org/mozilla-central/rev/1cf18e533f44

Comment 14

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 15

[(no longer active)]

Note that this required a clobber for Windows dep build bustage.

Comment 16

[Jean-Yves Perrier [:teoli]]

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.