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

Add compat data for RTCPeerConnection #1070

Merged
merged 14 commits into from
Mar 6, 2018
Merged

Conversation

bunnybooboo
Copy link
Contributor

}
],
"chrome": {
"version_added": true
Copy link
Collaborator

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.

"version_added": null
},
"opera_android": {
"version_added": null
Copy link
Collaborator

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

"version_added": "56"
},
"chrome": {
"version_added": true
Copy link
Collaborator

Choose a reason for hiding this comment

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

56

"version_added": null
},
"opera_android": {
"version_added": null
Copy link
Collaborator

Choose a reason for hiding this comment

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

43 for both Opera

"version_added": "56"
},
"chrome": {
"version_added": true
Copy link
Collaborator

Choose a reason for hiding this comment

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

56

"opera": {
"version_added": null
},
"opera_android": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

43

"version_added": "56"
},
"chrome": {
"version_added": true
Copy link
Collaborator

Choose a reason for hiding this comment

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

56

"version_added": null
},
"opera_android": {
"version_added": null
Copy link
Collaborator

Choose a reason for hiding this comment

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

43

"version_added": "56"
},
"chrome": {
"version_added": true
Copy link
Collaborator

Choose a reason for hiding this comment

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

56

"version_added": null
},
"opera_android": {
"version_added": null
Copy link
Collaborator

Choose a reason for hiding this comment

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

43

@Elchi3 Elchi3 added the data:api 🐇 Compat data for Web APIs. https://developer.mozilla.org/docs/Web/API label Feb 14, 2018
"notes": "Promise based version."
},
{
"version_added": "56"
Copy link
Collaborator

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.

},
{
"version_added": "43"
}
Copy link
Collaborator

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.

},
{
"version_added": "56"
}
Copy link
Collaborator

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.

Copy link
Contributor Author

@bunnybooboo bunnybooboo Feb 15, 2018

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When looking at your review vs what is in the file, they match up.
screenshot from 2018-02-15 08-06-11

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

Copy link
Contributor Author

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.

Copy link
Contributor Author

@bunnybooboo bunnybooboo Feb 15, 2018

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

"version_added": "43"
},
"opera_android": {
"version_added": "43"
Copy link
Collaborator

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actioning this right now.

{
"version_added": "56"
}
],
Copy link
Collaborator

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.

"version_added": "43"
},
"opera_android": {
"version_added": "43"
Copy link
Collaborator

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.

@bunnybooboo
Copy link
Contributor Author

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.

@jpmedley
Copy link
Collaborator

No. Thank you. That's one less that I have to correct. (And I have a bunch.)

Copy link
Member

@Elchi3 Elchi3 left a 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

"version_removed": "56"
},
{
"notes": "unprefixed",
Copy link
Member

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",
Copy link
Member

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"
              }

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'm on it..

},
"firefox_android": [
{
"notes": "unprefixed",
Copy link
Contributor

@ExE-Boss ExE-Boss Mar 4, 2018

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.

Copy link
Contributor Author

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

Copy link
Member

@Elchi3 Elchi3 left a 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!

@Elchi3 Elchi3 merged commit 6c81a4b into mdn:master Mar 6, 2018
@bunnybooboo bunnybooboo deleted the RTCPeerConnection branch March 7, 2018 18:52
dontcallmedom pushed a commit to dontcallmedom/browser-compat-data that referenced this pull request Mar 17, 2018
mlbrgl pushed a commit to mlbrgl/browser-compat-data that referenced this pull request Mar 29, 2018
foolip pushed a commit that referenced this pull request Sep 23, 2021
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.
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