947241 - Make Gamepad use the new attributes with cached sequence return values infrastructure

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

It fits like a glove.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Make Gamepad used cached sequences for its buttons and axes.

Comment 2

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

Comment on attachment 8343760 [details] [diff] [review]
Make Gamepad used cached sequences for its buttons and axes.

Review of attachment 8343760 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/gamepad/Gamepad.cpp
@@ +68,2 @@
>    mAxes[aAxis] = aValue;
> +  if (changed) {

I would do:

if (mAxes[aAxis] != aValue) {
  mAxes[aAxis] = aValue;
  GamepadBinding::ClearCachedAxesValue(this);
}

@@ +72,4 @@
>  }
>  
> +void
> +Gamepad::GetButtons(nsTArray<nsRefPtr<GamepadButton> >& aButtons)

Don't need the extra space anymore

Comment 3

[(not currently active) Ted Mielczarek]

Comment on attachment 8343760 [details] [diff] [review]
Make Gamepad used cached sequences for its buttons and axes.

Review of attachment 8343760 [details] [diff] [review]:
-----------------------------------------------------------------

This is so much nicer, I'm weeping with joy!

::: dom/gamepad/Gamepad.cpp
@@ +68,3 @@
>    mAxes[aAxis] = aValue;
> +  if (changed) {
> +    GamepadBinding::ClearCachedAxesValue(this);

So ClearCachedAxesValue is a bindings-generated method that comes from having [Cached] on the webidl method? Magic!

@@ +82,2 @@
>  {
> +  aAxes = mAxes;

Should we just inline these both in the class declaration? (You removed the previously-inlined overloads of these.)

::: dom/gamepad/Gamepad.h
@@ +89,5 @@
>    nsString mID;
>    uint32_t mIndex;
>  
>    // The mapping in use.
>    GamepadMappingType mMapping;

There's an mButtonsVariant field you can remove:
http://hg.mozilla.org/mozilla-central/annotate/1401e4b394ad/dom/gamepad/Gamepad.h#l116

Comment 4

[Boris Zbarsky [:bzbarsky]]

> I would do:

Yes, done.

> Don't need the extra space anymore

Mmm...ok.  ;)

> So ClearCachedAxesValue is a bindings-generated method 

Yup.  https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Cached

> Should we just inline these both in the class declaration?

Makes sense, done.  They ended up shorter than I first expected.

> There's an mButtonsVariant field you can remove:

Yup, done.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Make Gamepad used cached sequences for its buttons and axes.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Also, fixed a problem where we weren't actually CCing mButtons (!).

Comment 7

[(not currently active) Ted Mielczarek]

(In reply to Boris Zbarsky [:bz] from comment #4)
> > So ClearCachedAxesValue is a bindings-generated method 
> 
> Yup.  https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Cached

It would be nicer if that wiki mentioned the generated ClearCached${x}Value method explicitly, but awesome that this is documented!

Comment 8

[Boris Zbarsky [:bzbarsky]]

Hmm.  It's mentioned under [StoreInSlot], but you're right.  I'll make it clearer.

Comment 9

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/29725ca96a8d

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/29725ca96a8d