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

Fix RTCPeerConnection #11150

Merged
merged 11 commits into from
Jul 14, 2021
Merged

Fix RTCPeerConnection #11150

merged 11 commits into from
Jul 14, 2021

Conversation

teoli2003
Copy link
Contributor

Added two missing events: onicegatheringstatechange_event and icecandidateerror_event(I took their compat info form onicegatheringstatechange and onicecadidateerror that were both existing.
Added mdn and spec urls for RTCPeerConnection.getTranceivers.

@github-actions github-actions bot added the data:api Compat data for Web APIs. https://developer.mozilla.org/docs/Web/API label Jun 18, 2021
@foolip foolip changed the title Fix RTCPeerConnections Fix RTCPeerConnection Jun 19, 2021
@foolip
Copy link
Contributor

foolip commented Jun 19, 2021

I took their compat info form onicegatheringstatechange and onicecadidateerror that were both existing

@teoli2003 this is often correct, but depressingly often it's not. See #7545 for more complexities than you're probably interested in regarding events.

My advice when adding onfoo and foo_event entries would be:

  • First work out when the event can be fired and write a test for that, then use BrowserStack to find out when it was first supported
  • Then check if the onfoo property was added in the same version
  • If it wasn't, add notes about how only one of the methods of listening for the event work for some range of versions, and cry a little for web developers

Digging through implementation source is often faster than the testing approach, but has many pitfalls of its own. In particular, it's hard to be confident whether some code path firing an event is behind a flag or not.

api/RTCPeerConnection.json Outdated Show resolved Hide resolved
api/RTCPeerConnection.json Outdated Show resolved Hide resolved
api/RTCPeerConnection.json Show resolved Hide resolved
api/RTCPeerConnection.json Show resolved Hide resolved
"version_added": "22"
},
"firefox_android": {
"version_added": "44"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Firefox + Firefox for Android data in https://developer.mozilla.org/en-US/docs/Web/API/RTCPeerConnection#browser_compatibility looks pretty implausible, suggesting that the unprefixed RTCPeerConnection was available on desktop since 22, but that on Android it was prefixed as mozRTCPeerConnection from 22 to 43, and unprefixed in 44.

That's probably wrong, and the likely error is being spread around more here.

https://mdn-bcd-collector.appspot.com/tests/api/mozRTCPeerConnection can be used to figure out when the prefixed support was added and removed, if you have the appetite for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The unprefixing happened in Firefox 44 in the code: https://bugzilla.mozilla.org/show_bug.cgi?id=1155923. So RTCPeerConnection on Fx 22 is impossible.

Note that the removal of the unprefixed version has not happened yet: https://bugzilla.mozilla.org/show_bug.cgi?id=1531812.

I'm adding a commit reflecting this.

api/RTCPeerConnection.json Outdated Show resolved Hide resolved
api/RTCPeerConnection.json Outdated Show resolved Hide resolved
api/RTCPeerConnection.json Show resolved Hide resolved
@foolip
Copy link
Contributor

foolip commented Jun 19, 2021

I've also sent foolip/mdn-bcd-collector#1246 to improve the collector's ability to detect early support of RTCPeerConnection members.

@foolip
Copy link
Contributor

foolip commented Jul 13, 2021

@teoli2003 would you like to give this a final push? I'm happy to review again :)

@teoli2003
Copy link
Contributor Author

(Thanks for reminding me of this PR, lost in the list of those waiting review)

@teoli2003 teoli2003 requested a review from foolip July 14, 2021 08:55
Copy link
Contributor

@foolip foolip left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's still lots of Firefox Android data claiming 44 where the Firefox data says something else. This adds one such error, but I'll send a follow-up to fix them all.

@foolip foolip merged commit 7662996 into mdn:main Jul 14, 2021
@teoli2003
Copy link
Contributor Author

Thanks!

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.

3 participants