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

Use of FrozenArray<>s in dictionaries is unnecessary and bad #1368

Closed
Tracked by #1399
domenic opened this issue Apr 3, 2024 · 7 comments · Fixed by #1369
Closed
Tracked by #1399

Use of FrozenArray<>s in dictionaries is unnecessary and bad #1368

domenic opened this issue Apr 3, 2024 · 7 comments · Fixed by #1369

Comments

@domenic
Copy link

domenic commented Apr 3, 2024

The spec has one "init" dictionary which accepts FrozenArray<XRInputSource>.

It's not clear what was intended here. But per the Web IDL spec, almost the exact same set of values are accepted compared to accepting the more conventional sequence<XRInputSource>.

(The difference is that certain proxies for arrays, which define throwing getOwnPropertyDescriptor() traps, would be rejected, because in theory the implementation is supposed to freeze the incoming array.)

We'd like to prohibit the usage of FrozenArray<T> as a dictionary member in Web IDL, per whatwg/webidl#1399. It would be lovely if you could move to using sequence<>s.

@Maksims
Copy link

Maksims commented Apr 3, 2024

Is it frozen to avoid any modifications of it by the application? Which allows implementers to provide the same array and know that it only gets modified by them. Which eliminates a need to return a new array each time, which is beneficial for GC as many of these arrays can be referenced many-many times each frame.

@petervanderbeken
Copy link

The attributes can still return a FrozenArray<XRInputSource>.

But the array passed in the dictionary is not frozen. First a sequence<XRInputSource> is created, and then a new array is created from that but is then frozen. Using a sequence<XRInputSource> would work mostly the same. The constructor can then still create a frozen array from that if that's needed.

Note that as currently written the spec doesn't really work, it also still needs to create a frozen array, since the attributes are defined as being a list, which does not magically convert to a FrozenArray<XRInputSource>.

@AdaRoseCannon
Copy link
Member

/agenda Bringing it up at the next teleconference call.

@probot-label probot-label bot added the agenda Request discussion in the next telecon/FTF label Apr 3, 2024
@toji
Copy link
Member

toji commented Apr 9, 2024

This appears to be a simple copy-paste error, where the attributes from the event were brought over to the associated init dictionary too literally. There's no reason I'm aware of why this needs to be a FrozenArray on the dictionary, though as @petervanderbeken points out it can and should continue to be a FrozenArray on the event itself.

@toji
Copy link
Member

toji commented Apr 9, 2024

Put up a pull request to fix this. We can bring it up at the next call, but I think it'll be more of an advisory "Hey, this is a change that's gonna land" rather than something anyone will actually be concerned about.

@domenic
Copy link
Author

domenic commented Jun 5, 2024

I'm not sure who here is involved in the WebXR hit testing spec, but I have two similar issues over there which could use some attention, if there is any overlap: immersive-web/hit-test#116 immersive-web/hit-test#117

@toji
Copy link
Member

toji commented Jun 5, 2024

It still falls broadly under this group. I can fix them.

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

Successfully merging a pull request may close this issue.

6 participants