-
Notifications
You must be signed in to change notification settings - Fork 30
Change from Registry to Supported Protocols #401
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
base: main
Are you sure you want to change the base?
Conversation
|
Thank you Tim, this is looking like a great start to me. I've filed a Chromium issue to track our impl work and referenced it in your PR description. |
|
PR made to Specref to add these references: tobie/specref#898 |
marcoscaceres
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like where this is going ... some initial feedback.
| // MARK: Supported Protocols | ||
| --> | ||
| <h2 id="supported-protocols"> | ||
| Supported protocols |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should avoid the word "Supported" as passive voice begs the question "supported by who?". Also, it's not clear what "supported by this specification" means.
This should really be about, "protocols that are known to be allowed by user agents" as verified by userAgentAllowsProtocol() for instance.
We could even have a column(s) for user agent or engine (WebKit | Gecko | Chromium).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The text says: "supported by this specification."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps...?
| Supported protocols | |
| Protocols |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would "recognized" work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Recognized" works for me, but yes also to Ted just "Protocols". The specification defines an API surface including possible values for this string, I don't think it's really any different than say the "CredentialMediationRequirement" enum, except for the detail of how exactly unrecognized values are handled.
| </dl> | ||
| </section> | ||
| <h2 id="protocol-registry"> | ||
| Registry of protocols |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree we should delete this, but I'm wondering if any of this is salvageable for how new protocols are added to the spec. Should we have a non-normative note about "file a bug if you think your protocol belongs here?" or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it really different than any other spec feature though? We don't say in the spec "If you want to add a new feature, create an issue".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's pretty unlikely that we're going to see people wanting to add more protocols, I'd hate to complicate our spec with such text. I'd be OK pulling this out to a separate md file in the repo though Marcos if you want to keep it visible beyond an old spec checkpoint?
|
@marcoscaceres I've opened a new pull request, #408, to work on those changes. Once the pull request is ready, I'll request review from you. |
marcoscaceres
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good... some suggestions to make this more future proof and better integration with the rest of the API.
| </h2> | ||
| <p> | ||
| The following [=digital credential/exchange protocols=] are supported | ||
| by this specification. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My original comment stands, as I think "supported" will lead people to think that user agents support the listed protocols.
It be good to just go with "Protocols", as @TallTed suggests for the heading.
Instead of "supports", could we use either the word "permitted" or "recognized"?
Permitted implies the specification allows for these protocols to be used within the API's framework, but does not mandate their implementation in all user agents.
"The following [=digital credential/exchange protocols=] are permitted by this specification."
Recognized suggests the specification formally acknowledges these protocols as valid types for the API's parameters, without promising implementation in every user agents.
"The following [=digital credential/exchange protocols=] are recognized by this specification."
The "permitted" aspect seems more closely match out intent, specially with respect to the inclusion of the enum.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to "permitted"
| <p> | ||
| The following [=digital credential/exchange protocols=] are supported | ||
| by this specification. | ||
| </p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we assure developers that at least one of the things listed will be supported, and how to check for support. We also reflect the reality of what is actually implemented and we mandate that at least one must be implemented.
| </p> | |
| </p> | |
| <p> | |
| A [=user agent=] MUST implement at least one of the [=digital credential/exchange protocol=] listed in the [=table of exchange protocols=]. However, it is OPTIONAL for a user agent to implement all the [=digital credential/exchange protocols=] listed in the [=table of exchange protocols=]. | |
| </p> | |
| <aside class="note" title="Checking which exchange protocols a user agent allows"> | |
| <p>Developers can check which [=digital credential/exchange protocols=] | |
| a user agent allows by calling the `DigitalCredential.`{{DigitalCredential.userAgentAllowsProtocol()}} static method. | |
| See [[[#checking-if-protocol-is-allowed]]] for a usage example. | |
| </p> | |
| </aside> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works for me, with shorter lines that make life easier in many ways. I also added a missing s. I think that's all... but I encourage review with local DIFF of your choice, since GitHub fails to properly highlight this suggestion.
| </p> | |
| </p> | |
| <p> | |
| A [=user agent=] MUST implement at least one of the | |
| [=digital credential/exchange protocols=] listed in the | |
| [=table of exchange protocols=]. However, it is OPTIONAL for a user agent | |
| to implement all the [=digital credential/exchange protocols=] listed in | |
| the [=table of exchange protocols=]. | |
| </p> | |
| <aside class="note" title="Checking which exchange protocols a user agent allows"> | |
| <p>Developers can check which [=digital credential/exchange protocols=] | |
| a user agent allows by calling the | |
| `DigitalCredential.`{{DigitalCredential.userAgentAllowsProtocol()}} static | |
| method. See [[[#checking-if-protocol-is-allowed]]] for a usage example. | |
| </p> | |
| </aside> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion, this should be added in a separate PR, as there was not alignment on what this text should say during the 2026-01-12 call.
marcoscaceres
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of additional link suggestions for OpenID...
|
The suggestions is what I've implemented in WebKit WebKit/WebKit#55603 ... and this very closely matches what is in WebKit already, which is great. |
|
Coincidently, @mohamedamir and I recently updated the web platform tests to include the registered types 😄 |
| Enumerations as DOMString types | ||
| </h2> | ||
| <p> | ||
| Enumeration types are not referenced by other parts of the Web IDL because that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can add this to the appropriate algorithm as a note. Or better, if we do use the enums in the algorithms (even if not seen by JavaScript), then we don't need this text at all (i.e., we do a "IDL conversion" to an enum value, which is a perfectly fine thing to do).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Describing in the algorithm that the value is converted to the enum (with failures silently dropped without throwing) sounds right to me.
RByers
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With a few tweaks this seems worth landing to me, and then we can further iterate on the details.
| // MARK: Supported Protocols | ||
| --> | ||
| <h2 id="supported-protocols"> | ||
| Supported protocols |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Recognized" works for me, but yes also to Ted just "Protocols". The specification defines an API surface including possible values for this string, I don't think it's really any different than say the "CredentialMediationRequirement" enum, except for the detail of how exactly unrecognized values are handled.
| </dl> | ||
| </section> | ||
| <h2 id="protocol-registry"> | ||
| Registry of protocols |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's pretty unlikely that we're going to see people wanting to add more protocols, I'd hate to complicate our spec with such text. I'd be OK pulling this out to a separate md file in the repo though Marcos if you want to keep it visible beyond an old spec checkpoint?
| Enumerations as DOMString types | ||
| </h2> | ||
| <p> | ||
| Enumeration types are not referenced by other parts of the Web IDL because that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Describing in the algorithm that the value is converted to the enum (with failures silently dropped without throwing) sounds right to me.
This PR makes the following changes:
Closes #396
Closes #213
Closes #211
Closes #345
The following tasks have been completed:
Implementation commitment:
Documentation and checks
Preview | Diff