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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jul 24, 2018
Merged

API/Credential: deprecate name + add Firefox compat data #2275

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
801b8e3
API/Credential: deprecate name + add Firefox compat data
caugner Jun 9, 2018
ccdd0da
Credential/name: fix status + add description
caugner Jul 23, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 17 additions & 11 deletions api/Credential.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
"version_added": "51"
},
"firefox": {
"version_added": null
"version_added": "60"
},
"firefox_android": {
"version_added": null
Expand Down Expand Up @@ -58,7 +58,7 @@
"version_added": "51"
},
"firefox": {
"version_added": null
"version_added": "60"
},
"firefox_android": {
"version_added": null
Expand Down Expand Up @@ -103,7 +103,7 @@
"version_added": "51"
},
"firefox": {
"version_added": null
"version_added": "60"
},
"firefox_android": {
"version_added": null
Expand Down Expand Up @@ -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",
Copy link
Collaborator

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.

Copy link
Contributor Author

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

"notes": "See <a href='https://crbug.com/602980'>Bug 602980</a>."
},
"chrome": {
"version_added": "51"
"version_added": "51",
"version_removed": "52",
"notes": "See <a href='https://crbug.com/602980'>Bug 602980</a>."
},
"chrome_android": {
"version_added": "51"
"version_added": "51",
"version_removed": "52",
"notes": "See <a href='https://crbug.com/602980'>Bug 602980</a>."
},
"firefox": {
"version_added": null
"version_added": false
},
"firefox_android": {
"version_added": null
"version_added": false
},
"ie": {
"version_added": null
Expand All @@ -173,9 +179,9 @@
}
},
"status": {
"experimental": true,
"standard_track": true,
"deprecated": false
"experimental": false,
"standard_track": false,
"deprecated": true
Copy link
Collaborator

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?

Copy link
Contributor Author

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)?

Copy link
Collaborator

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.

Copy link
Contributor Author

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?

Copy link
Contributor Author

@caugner caugner Jul 20, 2018 •
edited
Loading

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.

Copy link
Collaborator

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 between CredentialUserData and Credential)
  • Revisit the discussion on How to represent 'mixins'? #472 when Florian gets back (in a couple weeks)

Does that sound OK to you?

Copy link
Contributor Author

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)! :-)

}
}
}
Expand Down