What is a "valid feature descriptor"

[mounirlamouri]

Given that the type is "any", some more details may be required in order to describe what a "valid" feature descriptor is. I'm not sure how this can be written so I can't give more guidance. In the case of Permissions, we had to specify the operations to go from Object to the base dictionary and there is a step that converts the dictionary to the specialised one depending on the name. It's possible that the spec will have to iterate through all the known types.

[rcabanier]

We touched on this in #791
We will need to be careful in the extension specs so this parsing is clearly defined.

[mounirlamouri]

Issue is that at the moment, the parsing isn't defined as far as I can tell. I'm not sure how one goes from any to the enum type.

[rcabanier]

Isn't https://immersive-web.github.io/webxr/#feature-descriptor enough?

Are you looking prose like the Permissions spec uses?

[mounirlamouri]

Not literally like the permissions spec because this is very different but a prose that converts the input to something usable because it's not done by WebIDL.

[rcabanier]

That is what I meant: have very precise prose like in the permissions spec to convert from any to DOMString and future extensions

[mounirlamouri]

It will need to check the DOMString against the enum values. Making this extensible will require some additional non-trivial spec work.

[toji]

We can and should make this check more algorithmic because it's easier to follow that way. The text linked by Rik in #860 (comment) does imply the intended behavior, though, so making it more explicit should not be a compatibility breaking change.

[bzbarsky]

The text linked by Rik in #860 (comment) does imply the intended behavior, though

@toji, I don't think it does. Which of the following literal JS values would be considered "valid feature descriptors" based on that text and why?

1. "viewer"
2. new String("viewer")
3. { toString: () => "viewer" }

All of those would convert to the XRReferenceSpaceType type without throwing an exception, if you passed them as an argument to a function that takes XRReferenceSpaceType as an argument type, say. I looked briefly, and I think Chrome's implementation at https://cs.chromium.org/chromium/src/third_party/blink/renderer/modules/xr/xr.cc?l=715&rcl=b225359b50c4af3f691822e82f517ddbad5d48d5 only accepts the first of those three, but that is not at all obvious from the linked text. In fact, if it had gotten implemented that way in Firefox and I were reviewing the IDL integration, I would likely have asked it to accept all the values that enums accept during review, so we would have ended up with incompatible implementations.

This really does need to be defined and ideally before we end up with incompatible implementations shipping.

[bzbarsky]

Probably the right way to approach this is to define this using a Web IDL union, not any. That way the "what is it?" processing will be handled automatically. And that way if it turns out some of the values are not distinguishable from each other (in the IDL sense), we can have a serious conversation about how they should be distinguished (and maybe change to using any at that point if it's needed). But it's worth thinking up front about the sort of feature descriptors that may need to be supported in the future and how best to express them in this API in a reasonable way. My understanding is that @Manishearth has suggested just that in the past...

[rcabanier]

My understanding is that @Manishearth has suggested just that in the past...

Was it here: #791 (comment)

[bzbarsky]

I was just told this by a third party; I don't know the history here.

[mounirlamouri]

It was indeed suggested during a call: https://www.w3.org/2019/08/13-immersive-web-minutes.html#item01

Maybe a good case where what was said during the call should have been summarised in the issue.

[kearwood]

My understanding is that @Manishearth has suggested just that in the past...

Was it here: #791 (comment)

I was the third party.. This is the one :-)

[Manishearth]

Does #1003 fix this as well?

[Manishearth]

I think #1003 fixes this, please reopen if there's something missing.