-
Notifications
You must be signed in to change notification settings - Fork 2k
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
API/Credential: deprecate name + add Firefox compat data #2275
Conversation
Sources: 1. Credential Management Level 1 specification: https://w3c.github.io/webappsec-credential-management/#credential 2. Chrome repository: https://chromium.googlesource.com/chromium/src/+/2388feb43e89660223fe86f63a298ce07870c2cd/third_party/blink/renderer/modules/credentialmanager/credential.idl (status quo) 3. Firefox repository: https://github.com/mozilla/gecko-dev/blob/852a0c88903d1204f99dfc3821b282780547fd2e/dom/webidl/CredentialManagement.webidl#L11-L14 (status quo)
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.
Thanks for this PR, @caugner. Some questions, particularly on the status of name
before we merge this. Thank you!
api/Credential.json
Outdated
"deprecated": false | ||
"experimental": false, | ||
"standard_track": false, | ||
"deprecated": true |
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 realize representing mixins is a bit odd (#472), but this status seems off to me: https://w3c.github.io/webappsec-credential-management/#credentialuserdata Maybe this data needs to be moved? Or returned to experimental and standard track?
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.
To be honest, I wasn't really aware of the mixin situation.
Strictly speaking, CredentialUserData
is not included in Credential
itself, but in the only two interfaces that extend it (i.e. FederatedCredential
and PasswordCredential
).
Would it be a bad idea to extract name
to a dedicated CredentialUserData
feature (alongside with the – anyway – missing iconURL
sub-feature)?
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 it be a bad idea to extract name to a dedicated CredentialUserData feature (alongside with the – anyway – missing iconURL sub-feature)?
Well, the discussion on #472 makes it sound like the right way to go is to document mixin interfaces as if they were part of the interface that inherits from the mixin. I get the impression that mixins are a sort of shorthand for spec authors to say that two or more interfaces will have some semantics in common, not that browsers will necessarily implement them as named mixins.
So in this case, I'd expect to see features for Credential.name
, FederatedCredential.name
, and PasswordCredential.name
, assuming that there's actually implementations out there.
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.
Alright. Should we maybe describe the name
sub-feature with <code>CredentialUserData.name</code>
to make it easier to understand that this is not directly a sub-feature of FederatedCredential
and PasswortCredential
?
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.
PS: To be honest, I personally tend to prefer having mixins separated and handle them like parent interfaces. Recently I edited the HashChangeEvent article which had listed target
, type
, bubbles
and cancelable
as sub-features (not compat data though), even though they originated from Event
– and I found that mixin' (pun intended) a bit confusing.
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.
Yeah, I'm with you that mixins are confusing. I like your includes <code>FooMixin</code>
idea on #472 — that seems like a promising approach. But I don't think I'm the right person to make a decision about it (Florian is the right person, though he's away on vacation right now). But I'd rather not delay merging this PR. What do you think of this:
- Set the standards track data like I suggested above (true, true, false)
- Set the description to
"<code>name</code> (from <code>CredentialUserData</code> mixin)"
(building on your idea, with a more explicit description of the relationship betweenCredentialUserData
andCredential
) - Revisit the discussion on How to represent 'mixins'? #472 when Florian gets back (in a couple weeks)
Does that sound OK to you?
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.
Thank you very much, let's do this (see ccdd0da)! :-)
@@ -139,19 +139,25 @@ | |||
"mdn_url": "https://developer.mozilla.org/docs/Web/API/Credential/name", | |||
"support": { | |||
"webview_android": { | |||
"version_added": "51" | |||
"version_added": "51", | |||
"version_removed": "52", |
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.
Out of curiosity, where did the 52 number come from? I couldn't figure it out from the links.
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 remember that it was a bit tricky, because removed lines don't show up in the blame view, but here's the Chromium diff that removes name
and iconURL
from Credential
(and extracts it into a new super-interface SiteBoundCredential
):
https://chromium.googlesource.com/chromium/src/+/be6cc81fc74f63c8e134525bbf15ea966c8403d5%5E%21/#F5
And I (hopefully) verified the version_added
with this blame:
https://chromium.googlesource.com/chromium/src/+blame/3df6ff1f5b8a0732469c57e56c805aefde17ebfe/third_party/WebKit/Source/modules/credentialmanager/Credential.idl
Thank you again, @caugner! |
Sources:
https://w3c.github.io/webappsec-credential-management/#credential
https://chromium.googlesource.com/chromium/src/+/2388feb43e89660223fe86f63a298ce07870c2cd/third_party/blink/renderer/modules/credentialmanager/credential.idl (status quo)
https://github.com/mozilla/gecko-dev/blob/852a0c88903d1204f99dfc3821b282780547fd2e/dom/webidl/CredentialManagement.webidl#L11-L14 (status quo)