-
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
Mark MediaRecorder as shipping in Safari 14 #8838
Conversation
browsers/safari_ios.json
Outdated
"engine": "WebKit", | ||
"engine_version": "610.1.28" | ||
}, | ||
"14.3": { |
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.
@ddbeck @Elchi3 I'd appreciate review and suggestions here. I don't know how to figure out which WebKit version is used here, since I can't find it anywhere in iOS settings. Have we just been assuming that it's the same as Safari macOS since the Safari UA string was frozen?
MediaRecorder is enabled on Safari macOS now as well, so I suspect the same thing might have happened in both. Safari 14.0.2 on macOS seems to be WebKit 610.3.7.
I'm just not sure what the bar for including a new version is...
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 release lines guideline partly defines the bar. Safari and iOS don't make it easy to define though. I'd say that "turns on a sizeable API" probably clears it. I'm fine with introducing 14.3 (though, oddly, I'd probably let macOS Safari 14 cover 14.0.[0-2] anyway—seems like less of an intervention to upgrade on the mac and more likely to be a passive, non-event for those users).
All that said, I'd appreciate @Elchi3's take too.
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.
If a release "turns on a sizeable API" then that would be a good call to add that release to BCD, yes. I can't really say more about macOS/iOS - the version numbers aren't straight forward and we've been struggling to get this correct since the begging of the BCD project unfortunately. :(
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 that handling this differently between macOS and iOS probably isn't right. In both cases, installing the update is something one has to approve (by default at least) and it restarts the device.
But it looks like adding the release for iOS but not for macOS is exactly what was done for iOS 13.3. The handful of APIs that were marked as introduced in iOS 13.3 in #6328 are also marked as Safari 13. I haven't tested, but a good guess is that it's a similar situation.
I can't decide what I'd actually prefer here. If we introduce minor versions, then we can more accurately capture what happened in the few cases where we can test carefully enough to be sure. But we'll also accumulate errors from cases where it's only possible/convenient to test the latest minor version, or contributors send PRs that we don't want to hold up to get this pedantically correct.
Note that https://developer.apple.com/documentation/ios-ipados-release-notes/ios-ipados-14_3-release-notes doesn't even mention this change, so I wonder if it was even intentional? I haven't tried using the API to see if it really works.
Help me decide? :)
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.
From experience, adding more versions to be pedantically correct adds more problems than it helps imo. So, I'm leaning towards being less precise (unless there are a lot of APIs introduced and it isn't a case of edge case pedantry anymore).
In say 3 years from now, this precision usually isn't worth it anymore anyway.
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'd be OK with that. Should we also remove the Safari iOS 13.3 release and collapse it into 13?
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.
Seems like there are only 34 entries and it's mostly the WebAuth API, so I'd say collapsing is fine. (I was reading #6338 also, but I'm not sure if that effects anything).
node scripts/traverse "safari_ios" "all" "13.3"
api.AuthenticatorAssertionResponse
api.AuthenticatorAssertionResponse.authenticatorData
api.AuthenticatorAssertionResponse.signature
api.AuthenticatorAssertionResponse.userHandle
api.AuthenticatorAttestationResponse
api.AuthenticatorAttestationResponse.attestationObject
api.AuthenticatorResponse
api.AuthenticatorResponse.clientDataJSON
api.Credential
api.Credential.id
api.Credential.type
api.HTMLMediaElement.disableRemotePlayback
api.PublicKeyCredential
api.PublicKeyCredential.getClientExtensionResults
api.PublicKeyCredential.isUserVerifyingPlatformAuthenticatorAvailable
api.PublicKeyCredential.rawId
api.PublicKeyCredential.response
api.PublicKeyCredentialCreationOptions
api.PublicKeyCredentialCreationOptions.attestation
api.PublicKeyCredentialCreationOptions.authenticatorSelection
api.PublicKeyCredentialCreationOptions.challenge
api.PublicKeyCredentialCreationOptions.excludeCredentials
api.PublicKeyCredentialCreationOptions.extensions
api.PublicKeyCredentialCreationOptions.pubKeyCredParams
api.PublicKeyCredentialCreationOptions.rp
api.PublicKeyCredentialCreationOptions.timeout
api.PublicKeyCredentialCreationOptions.user
api.PublicKeyCredentialRequestOptions
api.PublicKeyCredentialRequestOptions.allowCredentials
api.PublicKeyCredentialRequestOptions.challenge
api.PublicKeyCredentialRequestOptions.extensions
api.PublicKeyCredentialRequestOptions.rpId
api.PublicKeyCredentialRequestOptions.timeout
api.PublicKeyCredentialRequestOptions.userVerification
34
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'm also fine with collapsing the Safari versions. 👍
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.
[junk elided]
This was not in the initial releases of Safari 14, but was in Safari 14.0.3 on macOS and in Safari iOS 14.3. These releases are not recorded in BCD, and this appears to be the only change introduced. Using results collected for iOS 14.2 and 14.3 using https://mdn-bcd-collector.appspot.com/. Two entries were update manually: - the options object for the constructor, based on WebKit source - the error_event entry, based on the onerror entry
4b85924
to
8a78492
Compare
I've now updated this to mark this has shipped in both Safaris. |
As discussed when considering adding 14.3: mdn#8838 (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.
Great, thank you! 👍
As discussed when considering adding 14.3: #8838 (comment)
This was not in the initial releases of Safari 14, but was in Safari
14.0.3 on macOS and in Safari iOS 14.3. These releases are not recorded
in BCD, and this appears to be the only change introduced.
Using results collected for iOS 14.2 and 14.3 using
https://mdn-bcd-collector.appspot.com/.
Two entries were update manually: