Change from getClientExtensionResults function to clientExtensionResults attribute

[selfissued]

Fixes part of #803

---

Preview | Diff

[emlun]

It looks like the [[clientExtensionsResults]] internal slot (added in 5c8dc49) is no longer needed, correct?

[jcjones]

Mozilla is okay with this.

[nadalin]

@selfissued Please review @emlun comment

[selfissued]

@emlun - you wrote "It looks like the [[clientExtensionsResults]] internal slot (added in 5c8dc49) is no longer needed, correct?". I suspect you're right but note that the new attribute is calledclientExtensionsResults and so that has to stay. Could you create a PR against my PR to show us what you think should change?

[emlun]

There's both the attribute clientExtensionResults and the internal slot [[clientExtensionsResults]] now. That seems a bit redundant to me, and I believe the latter was introduced only because the former was converted into an operation. I can write a PR but I'm not entirely sure how to merge the two descriptions.

[emlun]

PR submitted: selfissued#4

[selfissued]

Fixes #803

[ttaubert]

Sorry for the late comment, but I just noticed that per [1]:

"The type of the attribute, after resolving typedefs, must not be a nullable or non-nullable version of any of the following types: ... a dictionary type"

[1] https://heycam.github.io/webidl/#idl-attributes

[emlun]

Oh. I think that's exactly the same rule that made us change the previous record-type attribute to an operation.

So I guess we'll have to revert this, then? @equalsJeffH @selfissued

[selfissued]

@agl and @jcjones , do you believe that Tim's comment means that we have to revert this?

[agl]

I'm not immediately convinced that we need to revert this, although some more spec lawyering may be needed.

We have the response member of PublicKeyCredential already which is a structure with fields and such. That's all we want for clientExtensionResults too. So maybe we need to call AuthenticationExtensionsClientOutputs an interface rather than a dictionary, but it certainly looks like we can get the desired shape without a weird callback to do it.

I have to get to a meeting now, but I'll try to find someone who claims to be a WebIDL expert to weigh in.

[selfissued]

Thanks @agl

[ttaubert]

I'm certainly not a WebIDL expert, but IIUIC interfaces have only required members. And those can't have any default values either. If that's the case an interface probably wouldn't be a good fit for AuthenticationExtensionsClientOutputs.

[selfissued]

Adding @nadalin so he's aware of this discussion

[agl]

I've asked @jyasskin to weigh in. However, I'm not sure that the lack of optional members is a problem here. This is the browser's extension output, thus the structure only contains fields for extensions that the browser supports. It's typical for browsers that don't support something not to have certain fields (I think). For example, we suggest testing for window.PublicKeyCredential to tell whether webauthn is supported or not.

[agl]

To summarise what I've managed to get from emailing people who do know WebIDL:

• Indeed, we cannot have an interface attribute with a dictionary value for unknown reasons.
• We can define AuthenticationExtensionsClientOutputs as a series of partial interface chunks, one for each extension, and with the expectation that browsers will only implement a subset of the attributes. That, however, means that there'll probably be null-valued attributes for all the extensions that a client supports, even if those extensions weren't used in a given request.
• We can also have a function returning a dictionary (i.e. what we had).

I don't have a strong preference either way. I think the Javascript is a little cleaner if it's an interface, but not having null-valued attributes hanging around might swing it for some people.

[selfissued]

Having the extraneous null-valued attributes for extensions that were not requested by the RP sounds pretty sub-optimal. I hate to say it, but it seems like things will be simpler if we revert this change and go back to the function. I'll create a new PR to do so and submit it to the commenters on this thread to review.

[nadalin]

@selfissued Should this be closed now ?

[selfissued]

It's merged, which is the closed state for PRs.

[nadalin]

@selfissued It wasn't when I wrote the comment