Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IDL array[]s no longer exist #28

Closed
domenic opened this issue Jul 27, 2016 · 9 comments
Closed

IDL array[]s no longer exist #28

domenic opened this issue Jul 27, 2016 · 9 comments

Comments

@domenic
Copy link

domenic commented Jul 27, 2016

As such, this spec has invalid Web IDL. Probably you want to use FrozenArray.

@luser
Copy link
Contributor

luser commented Aug 1, 2016

So the WebIDL that Gecko is actually using for our bindings is using sequence<T>:
https://dxr.mozilla.org/mozilla-central/rev/4a18b5cacb1b21a3e8b4b1dada6b2dd3dba51cb1/dom/webidl/Gamepad.webidl#46

I don't remember if that was just because T[] didn't work? I confess that every time I had a discussion about arrays vs. sequence types I got very confused on the difference. cc/ @bzbarsky

@bzbarsky
Copy link

bzbarsky commented Aug 1, 2016

Well, the array stuff was all up in the air for a while. T[] was never implemented by any browser and is slated to be nixed from the spec.

What Gecko actually uses here is:

[Pure, Cached, Frozen]
readonly attribute sequence<GamepadButton> buttons;

the [Frozen] bit gives this the same semantics as FrozenArray. See https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Frozen and compare to http://heycam.github.io/webidl/#dfn-create-frozen-array

In terms of how you would use a FrozenArray in the spec, I believe you would have the object store the FrozenArray and recreate it from a sequence as needed (e.g. whenever the list changes). The storing of the value is handled automatically in Gecko's bindings due to the Cached thing, but would need to be explicitly spelled out in prose in the spec....

@saschanaz
Copy link
Member

saschanaz commented May 18, 2017

Now ReSpec has removed array support and fails here:

Failed to parse WebIDL: 
```
        interface Gamepad {
          readonly attribute DOMString id;
          readonly attribute long index;
          readonly attribute boolean connected;
          readonly attribute DOMHighResTimeStamp timestamp;
          readonly attribute GamepadMappingType mapping;
          readonly attribute double[] axes;
          readonly attribute GamepadButton[] buttons;
        };
      
```
No name in attribute
respec-w3c-common (1,30553)

@rakuco
Copy link
Member

rakuco commented Jul 14, 2017

@luser, @bzbarsky, @toji I'd like to find a solution for this and make the spec compliant with modern WebIDL.

I think everyone agrees that those types should be converted to FrozenArray<>s, but I'm not entirely sure there needs to be prose saying the values (or at least Gamepad#buttons and Gamepad#hapticActuators) should also be cached. Gecko seems to be the only one caching the values, and populating those values in Gecko, WebKit and Blink looks fairly inexpensive.

Using FrozenArrays is a somewhat-breaking change though, as the arrays being returned so far are not frozen (however, I haven't seen code that requires changing those values around). The alternative would be returning an object and then specifying everything in prose, I guess.

@domenic
Copy link
Author

domenic commented Jul 14, 2017

It's quite bad if gamepad.buttons[0] !== gamepad.buttons[0], so please do cache the objects.

@rakuco
Copy link
Member

rakuco commented Jul 14, 2017

FWIW even with the change that'd still be true in Blink (and WebKit I think) due to how the bindings work, and Gecko already does the caching. I guess it wouldn't hurt to mention this explicitly in prose then.

@bzbarsky
Copy link

but I'm not entirely sure there needs to be prose saying the values (or at least Gamepad#buttons and Gamepad#hapticActuators) should also be cached.

I'm not sure what you mean here. Do you mean the value of the buttons property or the values in the buttons array?

That is, are you talking about whether gamepad.buttons === gamepad.buttons tests true, or about gamepad.buttons[0]?

Anyway, my take on how this should work is this:

  1. The object should have an internal slot storing a FrozenArray.
  2. The getter should return the value in that slot.
  3. When the set of buttons or whatnot changes, a new FrozenArray is created and stored in the slot.

This will all need to be specified in prose. Whether in step 3 the new FrozenArray uses the same GamepadButton objects as the old one or creates new ones, I don't have a strong view on. That's an API design decision, basically. It's not even clear to me whether the set of buttons can change.

Using FrozenArrays is a somewhat-breaking change though

Breaking from which point of view? Spec or implementation? Gecko has been shipping frozen arrays here since https://bugzilla.mozilla.org/show_bug.cgi?id=949682 was fixed back in Firefox 29. I can't tell from code inspection what Blink does.

@rakuco
Copy link
Member

rakuco commented Jul 17, 2017

I'm not sure what you mean here. Do you mean the value of the buttons property or the values in the buttons array?

That is, are you talking about whether gamepad.buttons === gamepad.buttons tests true, or about gamepad.buttons[0]?

I'd meant the former, but what I had in mind was basically what you described in your steps.

Using FrozenArrays is a somewhat-breaking change though

Breaking from which point of view? Spec or implementation? Gecko has been shipping frozen arrays here since https://bugzilla.mozilla.org/show_bug.cgi?id=949682 was fixed back in Firefox 29.

Spec. Gecko's been using frozen arrays, but so far there was nothing in the spec saying Object.isFrozen(gamepad.buttons) === true or Object.isFrozen(gamepad.axes) === true. I haven't seen any code assuming these were false (and thus making changes to the returned arrays) though, so my concern was only to ensure this kind of change is OK to make.

I can't tell from code inspection what Blink does.

At the moment, as far as I can see Blink will always return a new array (so gamepad.buttons === gamepad.buttons is always false), but the items in the array only change when the button count changes (so in most cases gamepad.buttons[0] === gamepad.buttons[0] is true). Moving to a fully caching behavior is pretty easy though.

rakuco added a commit to rakuco/gamepad that referenced this issue Jul 17, 2017
Arrays have not existed in the WebIDL spec since 2015, so the Gamepad
spec (as well as the accompanying Gamepad Extensions one) were providing
invalid IDL definitions.

Switch to FrozenArray<T> and clarify in prose that some attributes should be
cached by the user agent until new values have to be returned.

Fixes w3c#28.
rakuco added a commit to rakuco/gamepad that referenced this issue Jul 17, 2017
Arrays have not existed in the WebIDL spec since 2015, so the Gamepad
spec (as well as the accompanying Gamepad Extensions one) were providing
invalid IDL definitions.

Switch to FrozenArray<T> and clarify in prose that some attributes should be
cached by the user agent until new values have to be returned.

Fixes w3c#28.
@luser
Copy link
Contributor

luser commented Jul 26, 2017

Regardless of what the spec text has said, I would find it hard to believe that this would actually break any usage in practice, especially given that Gecko's implementation has been doing this since it first shipped. Thanks for cleaning this up!

@luser luser closed this as completed in #62 Jul 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants