-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Add compat data for RTCPeerConnection #1070
Conversation
api/RTCPeerConnection.json
Outdated
} | ||
], | ||
"chrome": { | ||
"version_added": 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.
This should be the same as chrome_android
and webview_android
.
api/RTCPeerConnection.json
Outdated
"version_added": null | ||
}, | ||
"opera_android": { | ||
"version_added": null |
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.
Both versions of Opera are 43 for unprefixed. https://www.chromestatus.com/features/5312661013135360
api/RTCPeerConnection.json
Outdated
"version_added": "56" | ||
}, | ||
"chrome": { | ||
"version_added": 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.
56
api/RTCPeerConnection.json
Outdated
"version_added": null | ||
}, | ||
"opera_android": { | ||
"version_added": null |
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.
43 for both Opera
api/RTCPeerConnection.json
Outdated
"version_added": "56" | ||
}, | ||
"chrome": { | ||
"version_added": 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.
56
api/RTCPeerConnection.json
Outdated
"opera": { | ||
"version_added": null | ||
}, | ||
"opera_android": { |
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.
43
api/RTCPeerConnection.json
Outdated
"version_added": "56" | ||
}, | ||
"chrome": { | ||
"version_added": 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.
56
api/RTCPeerConnection.json
Outdated
"version_added": null | ||
}, | ||
"opera_android": { | ||
"version_added": null |
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.
43
api/RTCPeerConnection.json
Outdated
"version_added": "56" | ||
}, | ||
"chrome": { | ||
"version_added": 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.
56
api/RTCPeerConnection.json
Outdated
"version_added": null | ||
}, | ||
"opera_android": { | ||
"version_added": null |
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.
43
f3ce8e6
to
d4bc206
Compare
d4bc206
to
34da639
Compare
api/RTCPeerConnection.json
Outdated
"notes": "Promise based version." | ||
}, | ||
{ | ||
"version_added": "56" |
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 numbers are backwards. Also, this information applies to Webview and Android.
api/RTCPeerConnection.json
Outdated
}, | ||
{ | ||
"version_added": "43" | ||
} |
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 version numbers for both Operas is backwards.
api/RTCPeerConnection.json
Outdated
}, | ||
{ | ||
"version_added": "56" | ||
} |
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.
Am I giving you bad instructions? Added in 50. Promise-based version in 56. All 3 versions of Chrome.
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 was up until 2am fixing this file. The final commit 34cae58 is only 2537 lines long. The review you did was unfortunately on one of the 2 previously failing commits in Travis. It was late, errors were being made, and I believe I finally fixed them. The line numbers you quote there are out of scope of the final commit.
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.
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 link you shared discussing M50..
Add promise-based versions of RTCPeerConnection methods: setLocalDescription, setRemoteDescription, addIceCandidate, createOffer and createAnswer. To be done in 2 steps. First, setLocalDescription, setRemoteDescription and addIceCandidate (anticipated in M50).
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.
Under the light of a new day this now does not look quite right, as it suggests the removal of promises in 56. Seeking out another potential code example inside BCD to replicate, to avoid unecessary repetitious notes in a fix.
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.
Adding the line "notes": "Promise based version and unprefixed."
to an immediate commit. This will communicate promises did not STOP in 56, in all instances of earlier rolled out subfeatures. Reflected in f0f62a8
api/RTCPeerConnection.json
Outdated
"version_added": "43" | ||
}, | ||
"opera_android": { | ||
"version_added": "43" |
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.
Added in 37. Promise-based in 43. Both versions of Opera.
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.
Actioning this right now.
api/RTCPeerConnection.json
Outdated
{ | ||
"version_added": "56" | ||
} | ||
], |
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 numbers are backwards, all three versions of Chrome. Please check this throughout.
api/RTCPeerConnection.json
Outdated
"version_added": "43" | ||
}, | ||
"opera_android": { | ||
"version_added": "43" |
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.
Add in 37. Promise-based in 43. Both versions of Opera. Please check this throughout.
Wahoo! Thanks for your patience @jpmedley. I had a steep learning curve in the last week. This file was a massive test on me and you truly helped me get to grips with it. Apologies if it was unusually labor intensive. |
No. Thank you. That's one less that I have to correct. (And I have a bunch.) |
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 @bunnybooboo! This is a massive amount of work. Well done! Sorry for being slow with reviews, this repo is quite busy.
I have two comments that apply to the whole file. See below
There are 5 sub features that I couldn't find entries for, but I would be OK with adding this as a separate PR as this is gigantic already.
https://developer.mozilla.org/en-US/docs/Web/API/RTCPeerConnection/close
https://developer.mozilla.org/en-US/docs/Web/API/RTCPeerConnection/addTransceiver
https://developer.mozilla.org/en-US/docs/Web/API/RTCPeerConnection/currentRemoteDescription
https://developer.mozilla.org/en-US/docs/Web/API/RTCPeerConnection/iceGatheringState
https://developer.mozilla.org/en-US/docs/Web/API/RTCPeerConnection/onicecandidateerror
api/RTCPeerConnection.json
Outdated
"version_removed": "56" | ||
}, | ||
{ | ||
"notes": "unprefixed", |
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.
We don't mark unprefixed separately. So this needs to be a block like this, with the unprefixed / "normal" support first:
"webview_android": [
{
"version_added": "56"
},
{
"prefix": "webkit",
"version_added": "51",
"version_removed": "56"
}
],
This is an issue throughout the file. I'm not marking it everywhere. Please double-check this.
"version_removed": "43" | ||
}, | ||
{ | ||
"version_added": "43", |
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.
Here as well. The most relevant version needs to come first. Swap this around please:
{
"version_added": "43",
"notes": "Promise based version."
},
{
"version_added": "37",
"version_removed": "43"
}
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 on it..
api/RTCPeerConnection.json
Outdated
}, | ||
"firefox_android": [ | ||
{ | ||
"notes": "unprefixed", |
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.
This line is unnecessary, because there is no prefix
value defined.
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.
ahh that one slipped through the net.. I'm on it
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 your fixes! Lets get this in. I will file a follow-up issue for the missing sub features and we can do correction PRs if we want to fix more stuff here. This way the massiveness about this should be gone going forward hopefully. Thanks again for this PR, epic amount of work!
This PR corrects the data throughout the RTCPeerConnection API, which was a **big** mess, mostly due to copy-paste error. The fixes include the following: - Converts the "Promise-based version" notes into subfeatures. It is more standard to use a subfeature rather than the notes. Additionally, they were copied and pasted all across the Opera data, so it caused some major data discrepancies. (Particularly, claiming support for features in Opera that weren't supported, wrong version numbers...) Fixes #11158. - Corrects the data regarding promise-based versions for Chrome. After further digging in, it turns out that most of these features had the wrong version number. New data comes from the commit history ([1](https://source.chromium.org/chromium/chromium/src/+/e16c96f5a5b517a64fdbc5be75f05e359e04b461), [2](https://source.chromium.org/chromium/chromium/src/+/70302a3d99ed149d59acf592edf9d32d4fd0dbda), [3](https://source.chromium.org/chromium/chromium/src/+/68f8e256dc7c7369127bd345100bcf776fa8acc0)). - Corrects the Samsung Internet and Opera/Opera Android data. In the case of Samsung Internet, it appears to have been set to 6.0 most everywhere after a mirroring from the old, incorrect wiki data (#1606), and Opera's data initially came from the wiki tables (#1070). When the Chrome data was corrected, however (see #3287), only the Chrome, Chrome Android, and WebView data was adjusted, leaving these other browsers untouched. - Corrects the version number for `createDataChannel` based upon results from the mdn-bcd-collector project (v3.3.0). - Finally, mirrors the Chrome data to both Chrome Android and WebView. Both had been left as `true` for many of the features, however since we have ranged values, we're able to replace the `true` with `≤37` for WebView, and mirror Chrome straight to Chrome Android since we know Chrome Android has supported this API back then.
Via #987
https://developer.mozilla.org/en-US/docs/Web/API/RTCPeerConnection