Sync Navigator.getGamepads() with the spec

Sync Navigator.getGamepads() with the spec

https://dvcs.w3.org/hg/gamepad/raw-file/default/gamepad.html#navigator-interface-extension
https://www.w3.org/Bugs/Public/show_bug.cgi?id=26181

The spec allows the returned array to be longer than necessary. The
minimum possible length is used instead, and a spec bug was filed:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=27986

BUG=344556

[philipj_slow, 2014-12-17 21:29:39]

philipj@opera.com changed reviewers:
+ b.kelemen@samsung.com, bajones@chromium.org

[philipj_slow, 2014-12-17 21:29:39]

PTAL. As the FIXMEs in gamepad-polling-access.html suggest, this still doesn't
do what the spec does.

I've peeked at Gecko's WebIDL and suspect they'd have the same problem:
sequence<Gamepad?> getGamepads();

IMHO the spec is a bit odd on this point, and I suspect that custom bindings may
be required to make it match the spec.

Maybe we should ask the Mozilla folks if they think this API makes sense as is,
and maybe change it to Gamepad?[] or something, which would match this CL.

[kbalazs, 2014-12-17 21:53:41]

Looks good modulo nuances.

Exposing an array as an array sound good :)

https://codereview.chromium.org/808643005/diff/1/Source/modules/gamepad/Navig...
File Source/modules/gamepad/NavigatorGamepad.cpp (right):

https://codereview.chromium.org/808643005/diff/1/Source/modules/gamepad/Navig...
Source/modules/gamepad/NavigatorGamepad.cpp:107: gamepads[i] = gamepad;
This should be gamepads[actualLength], no?

It seems to me that if there is a hole in GamepadList - which is legit - you
might lose pads when you shrink.

https://codereview.chromium.org/808643005/diff/1/Source/modules/gamepad/Navig...
Source/modules/gamepad/NavigatorGamepad.cpp:111: gamepads.shrink(actualLength);
In the common case this would allocate, deallocate and allocate again, right? We
can do it with just one allocation. Actually, with inlineCapacity we can avoid
using any heap at all.

[kbalazs, 2014-12-17 21:56:33]

On 2014/12/17 21:53:41, kbalazs wrote:
> Looks good modulo nuances.
> 
> Exposing an array as an array sound good :)
> 
>
https://codereview.chromium.org/808643005/diff/1/Source/modules/gamepad/Navig...
> File Source/modules/gamepad/NavigatorGamepad.cpp (right):
> 
>
https://codereview.chromium.org/808643005/diff/1/Source/modules/gamepad/Navig...
> Source/modules/gamepad/NavigatorGamepad.cpp:107: gamepads[i] = gamepad;
> This should be gamepads[actualLength], no?
> 
> It seems to me that if there is a hole in GamepadList - which is legit - you
> might lose pads when you shrink.
> 
>
https://codereview.chromium.org/808643005/diff/1/Source/modules/gamepad/Navig...
> Source/modules/gamepad/NavigatorGamepad.cpp:111:
gamepads.shrink(actualLength);
> In the common case this would allocate, deallocate and allocate again, right?
We
> can do it with just one allocation. Actually, with inlineCapacity we can avoid
> using any heap at all.

Ah, and as Brandon mentioned we should make sure we agree about holes.

[philipj_slow, 2014-12-17 22:45:38]

So, about holes in the array. The simplest thing is to just return the existing
gamepads as a hole-free array, but what is the spec trying to accomplish by
these sparse arrays and the Gamepad.index attribute? It's a design I haven't
seen elsewhere in the Web platform.

https://codereview.chromium.org/808643005/diff/1/Source/modules/gamepad/Navig...
File Source/modules/gamepad/NavigatorGamepad.cpp (right):

https://codereview.chromium.org/808643005/diff/1/Source/modules/gamepad/Navig...
Source/modules/gamepad/NavigatorGamepad.cpp:107: gamepads[i] = gamepad;
On 2014/12/17 21:53:41, kbalazs wrote:
> This should be gamepads[actualLength], no?
> 
> It seems to me that if there is a hole in GamepadList - which is legit - you
> might lose pads when you shrink.

This started out as a loop simply copying gamepads[i] = gamepadList->item(i).
The actualLength is set for any non-null item to shrink the array afterwards to
the minimum length. The modified test shows how a hole in GamepadList is
handled.

https://codereview.chromium.org/808643005/diff/1/Source/modules/gamepad/Navig...
Source/modules/gamepad/NavigatorGamepad.cpp:111: gamepads.shrink(actualLength);
On 2014/12/17 21:53:41, kbalazs wrote:
> In the common case this would allocate, deallocate and allocate again, right?
We
> can do it with just one allocation. Actually, with inlineCapacity we can avoid
> using any heap at all.

shrink() only modifies the size, it doesn't reallocate. Because
GamepadList::length() is always 4, this function will allocate 4 slots and then
shrink the size.

[philipj_slow, 2014-12-17 22:55:57]

Oh, there's already a spec bug:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=26181

Let's try to move that along, then.

[philipj_slow, 2014-12-17 22:55:58]

Oh, there's already a spec bug:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=26181

Let's try to move that along, then.

[kbalazs, 2014-12-18 00:34:39]

https://codereview.chromium.org/808643005/diff/1/Source/modules/gamepad/Navig...
File Source/modules/gamepad/NavigatorGamepad.cpp (right):

https://codereview.chromium.org/808643005/diff/1/Source/modules/gamepad/Navig...
Source/modules/gamepad/NavigatorGamepad.cpp:107: gamepads[i] = gamepad;
I see this in Vector::shrink:

TypeOperations::destruct(begin() + size, end());

So if you have a hole and than you have a non-null item, you will lose that
item.

https://codereview.chromium.org/808643005/diff/1/Source/modules/gamepad/Navig...
Source/modules/gamepad/NavigatorGamepad.cpp:111: gamepads.shrink(actualLength);
On 2014/12/17 22:45:38, philipj wrote:
> On 2014/12/17 21:53:41, kbalazs wrote:
> > In the common case this would allocate, deallocate and allocate again,
right?
> We
> > can do it with just one allocation. Actually, with inlineCapacity we can
avoid
> > using any heap at all.
> 
> shrink() only modifies the size, it doesn't reallocate. Because
> GamepadList::length() is always 4, this function will allocate 4 slots and
then
> shrink the size.

Correct, it doesn't deallocate but it destroys elements in the shrinked part of
the array.

[philipj_slow, 2014-12-18 08:29:03]

Clearly this code is non-obvious, I think it would be clearer without the
intermediate GamepadList.

https://codereview.chromium.org/808643005/diff/1/Source/modules/gamepad/Navig...
File Source/modules/gamepad/NavigatorGamepad.cpp (right):

https://codereview.chromium.org/808643005/diff/1/Source/modules/gamepad/Navig...
Source/modules/gamepad/NavigatorGamepad.cpp:107: gamepads[i] = gamepad;
On 2014/12/18 00:34:39, kbalazs wrote:
> I see this in Vector::shrink:
> 
> TypeOperations::destruct(begin() + size, end());
> 
> So if you have a hole and than you have a non-null item, you will lose that
> item.

gamepad-polling-access.html tries exactly this, and the second item isn't lost.

https://codereview.chromium.org/808643005/diff/1/Source/modules/gamepad/Navig...
Source/modules/gamepad/NavigatorGamepad.cpp:111: gamepads.shrink(actualLength);
On 2014/12/18 00:34:39, kbalazs wrote:
> On 2014/12/17 22:45:38, philipj wrote:
> > On 2014/12/17 21:53:41, kbalazs wrote:
> > > In the common case this would allocate, deallocate and allocate again,
> right?
> > We
> > > can do it with just one allocation. Actually, with inlineCapacity we can
> avoid
> > > using any heap at all.
> > 
> > shrink() only modifies the size, it doesn't reallocate. Because
> > GamepadList::length() is always 4, this function will allocate 4 slots and
> then
> > shrink the size.
> 
> Correct, it doesn't deallocate but it destroys elements in the shrinked part
of
> the array.

It's Member<Gamepad> being destroyed, so that seems OK.

[bajones, 2014-12-18 17:37:47]

On 2014/12/18 at 08:29:03, philipj wrote:
> Clearly this code is non-obvious, I think it would be clearer without the
intermediate GamepadList.

Agreed. It seems like we shouldn't have any trouble storing it as a HeapVector
internally.

[philipj_slow, 2015-02-07 04:50:38]

rebase

[philipj_slow, 2015-02-07 04:53:06]

Please ignore the rebase, this still isn't ready for review. Time allowing I'll
tend to it next week.

[philipj_slow, 2015-02-09 13:53:26]

Hi guys,

I started working on a refresh of this CL today in order to implement the change
from https://www.w3.org/Bugs/Public/show_bug.cgi?id=26181

However, it quickly became clear that there are further problem in the spec:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=21434#c6

I see two choices:

1. Create a new Gamepad object whenever needed, including for the GamepadEvent.
This seems a bit crazy to me.

2. Ensure that scripts see a single Gamepad object for its entire lifetime. This
necessarily means that these objects will be live, however, and that scripts
don't need to call navigator.getGamepad() to get new state, they could just keep
a reference to the gamepad they got in the gamepadconnected event.

Neither seems particularly appealing to me, honestly. This CL is just a guest
appearance in the gamepad module on my part, so I would appreciate your feedback
on this.

There's also a related problem: It looks like Gamepad.axes and Gamepad.buttons
will return a new array every time, such that gamepad.axes!=gamepad.axes. I
don't have an actual gamepad for testing, could you confirm if this is the case
and intentional?