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.

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

Update WebAuthentication API data for Safari. #6328

Merged
merged 1 commit into from
Jun 26, 2020

Conversation

@ghost ghost added the data:api 🐇 Compat data for Web APIs. https://developer.mozilla.org/docs/Web/API label Jun 25, 2020
@alanwaketan
Copy link
Contributor Author

Sorry, I'm new to the repo. Can anyone walk me through the checks? Like, why are they failing?

@sideshowbarker
Copy link
Collaborator

sideshowbarker commented Jun 25, 2020

Hi @alanwaketan — great to see you here

The failures are for the npm test check — and specifically for the lint part of that, which includes checking for know browser versions.

The problem is that this repo doesn’t yet know about the 13.3 version of iOS Safari; instead it only knows about versions 13 and 13.4:

"13.4": {
"release_date": "2020-03-24",
"release_notes": "https://developer.apple.com/documentation/safari-release-notes/safari-13_1-release_notes",
"status": "current",
"engine": "WebKit",
"engine_version": "609.1.20"
},

So, to make the repo recognize 13.3 as a known iOS Safari version, there would need to be a separate PR that updates the https://github.com/mdn/browser-compat-data/blob/master/browsers/safari_ios.json file with details for the 13.3 release similar to those shown above for the 13.4 release.

@alanwaketan
Copy link
Contributor Author

@sideshowbarker Nice to see you in this repo! I will upload another pull request then.

@othermaciej
Copy link

Why is Safari 13.1 labeled as Safari 13.4 in here? That seems like another PR-worthy bug (if there is no deep reason). The actually existing 13.x versions are 13.0.1, 13.0.2, 13.0.3, 13.0.4, 13.0.5, 13.1, 13.1.1

@alanwaketan
Copy link
Contributor Author

Why is Safari 13.1 labeled as Safari 13.4 in here? That seems like another PR-worthy bug (if there is no deep reason). The actually existing 13.x versions are 13.0.1, 13.0.2, 13.0.3, 13.0.4, 13.0.5, 13.1, 13.1.1

I will try to correct all these faults in my next PR, and will post it to our webkit channel to let folks do proof reading.

@sideshowbarker
Copy link
Collaborator

Heya @othermaciej. This repo maintains browser-version data both for macOS Safari and, separately, for iOS Safari. So the error the linter is reporting here is "13.3" is NOT a valid version number for safari_ios (not for macOS).

The version data for both macOS Safari and iOS Safari here has so far been maintained by non-Apple volunteer contributors, so it’s somewhat unsurprising that it may not be as accurate as it should be. If somebody from the WebKit project would be willing to take some time to review it and correct/update it where necessary, that would be super great.

@Elchi3
Copy link
Member

Elchi3 commented Jun 25, 2020

Maintainer here. I was about to write what @sideshowbarker said :) Let me know if I can help to sort this out in any way and welcome to mdn-browser-compat-data 🎉

@sideshowbarker
Copy link
Collaborator

@alanwaketan
Copy link
Contributor Author

Just out of curiosity, what's the benefit to have the engine version in the db?

@sideshowbarker
Copy link
Collaborator

sideshowbarker commented Jun 25, 2020

Just out of curiosity, what's the benefit to have the engine version in the db?

When somebody is adding new data, tt allows the related tooling to catch (1) typos, (2) version-formatting inconsistencies, and (3) copypasta wrong-version numbers.

What I mean by version-formatting inconsistencies is that it ensures there’s not one contributor adding data with a 13.0 version number while another contributor is adding it with 13. We always use one or the other.

As far copypasta wrong-version numbers, what I mean is, for example: when adding data for a new feature recently, I accidentally pasted a safari_ios version number (13.4, I think) into the value of the samsunginternet_android key — because they’re right next to each other in the JSON source, so it’s an easy mistake to make.

And since samsunginternet_android doesn’t yet have a version 13.4 yet, the tooling caught that and let me know.

So overall it’s a linting thing, and a consistency thing.

@alanwaketan
Copy link
Contributor Author

@sideshowbarker Thanks for your throughout answer. My question probably lacks too many details to have someone understand it correctly. Specifically, I was trying to ask what the engine_version here is used for?

"13.4": { "release_date": "2020-03-24", "release_notes": "https://developer.apple.com/documentation/safari-release-notes/safari-13_1-release_notes", "status": "current", "engine": "WebKit", "engine_version": "609.1.20" },

@sideshowbarker
Copy link
Collaborator

Specifically, I was trying to ask what the engine_version here is used for?

We sometimes use the engine version when trying to figure out which release a particular feature landed in.

As I alluded to earlier, because this project hasn’t yet really had anybody from the WebKit team directly contributing to this repo and doing reviews in the same way that there are people from the Chrome and Firefox teams doing those things, we often have to do a lot of research ourselves to try to identify which Safari release a particular feature ended up shipping in.

So what we sometimes do is, we look in WebKit Trac to find the revision where the feature landed, then based on that, we look at what tag that first went into, and then find the Safari release with the closest engine_version number to that tag.

For example, see #6157 and #6151 (just two examples among many).

https://developer.mozilla.org/en-US/docs/MDN/Contribute/Processes/Matching_features_to_browser_version#Safari has some related details.

@alanwaketan
Copy link
Contributor Author

Specifically, I was trying to ask what the engine_version here is used for?

We sometimes use the engine version when trying to figure out which release a particular feature landed in.

As I alluded to earlier, because this project hasn’t yet really had anybody from the WebKit team directly contributing to this repo and doing reviews in the same way that there are people from the Chrome and Firefox teams doing those things, we often have to do a lot of research ourselves to try to identify which Safari release a particular feature ended up shipping in.

So what we sometimes do is, we look in WebKit Trac to find the revision where the feature landed, then based on that, we look at what tag that first went into, and then find the Safari release with the closest engine_version number to that tag.

For example, see #6157 and #6151 (just two examples among many).

https://developer.mozilla.org/en-US/docs/MDN/Contribute/Processes/Matching_features_to_browser_version#Safari has some related details.

Now, you have me. Understood. I was confused when I was trying to edit this list because we don't have that internally. And the WebKit version is forever locked to "605.1.15" in our user agent string to prevent finger printing. I will keep the field there in case people outside of WebKit wish to continue updating the data.

@alanwaketan
Copy link
Contributor Author

alanwaketan commented Jun 25, 2020

The PR to add an entry for iOS 13.3 is here. #6336

@sideshowbarker
Copy link
Collaborator

Thanks much!

@alanwaketan
Copy link
Contributor Author

Thanks, Michael!

@alanwaketan alanwaketan deleted the safari_webauthn branch June 26, 2020 01:44
@othermaciej
Copy link

Heya @othermaciej. This repo maintains browser-version data both for macOS Safari and, separately, for iOS Safari. So the error the linter is reporting here is "13.3" is NOT a valid version number for safari_ios (not for macOS).

iOS Safari uses the same version numbers as Mac Safari. 13.3 and 13.4 are both valid iOS versions, but are not Safari versions. For example, the User-Agent header for iOS Safari on iOS 13.4.1 is Mozilla/5.0 (iPhone; CPU iPhone OS 13_4_1 like Mac OS X) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/13.1 Mobile/15E148 Safari/604.1. Notice the Version/13.1, that is the Safari version.

If MDN is choosing to identify iOS Safari by the OS version number instead of the browser version number, then I guess this is consistent, but it's not at all clear from the tables that this one case is an OS version number, while all other browsers, including Chrome for Android, are reported by browser version number instead.

This practice has the potential to confuse, because iOS 13.1 does not come with Safari 13.1, so 13.1 can mean two very different things.

Fixing this is probably beyond the scope of this PR. So sorry for the drive-by comment, but I think this is a real issue that should be fixed.

The version data for both macOS Safari and iOS Safari here has so far been maintained by non-Apple volunteer contributors, so it’s somewhat unsurprising that it may not be as accurate as it should be. If somebody from the WebKit project would be willing to take some time to review it and correct/update it where necessary, that would be super great.

Noted! This is not a complaint, just something I happened to notice that looked wrong.

@othermaciej
Copy link

Filed the thing I noticed as #6338

@sideshowbarker
Copy link
Collaborator

Filed the thing I noticed as #6338

Thanks much for raising that

If MDN is choosing to identify iOS Safari by the OS version number instead of the browser version number, then I guess this is consistent

Yeah, as #6338 (comment) indicates, that’s in fact the policy that has been in place — and that explains why there was previously no 13.3 version in the https://github.com/mdn/browser-compat-data/blob/master/browsers/safari_ios.json file.

But I had actually forgotten that was the policy in place. So I’m glad you took time to comment here and raise #6338, and I look forward to it getting resolved — because (for the reasons you’ve pointed), the current state of things seems to have a lot of potential to confuse.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
data:api 🐇 Compat data for Web APIs. https://developer.mozilla.org/docs/Web/API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants