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

Merge RTCConfiguration dict into RTCPeerConnection #12830

Merged
merged 6 commits into from Jun 8, 2022

Conversation

queengooborg
Copy link
Collaborator

This PR removes RTCConfiguration from BCD. This feature is a dictionary, enum, or WebIDL typedef and should not be included in BCD.

This PR removes `RTCConfiguration` from BCD.  This feature is a dictionary, enum, or WebIDL typedef and should not be included in BCD.
@queengooborg queengooborg added needs-release-note 📰 needs content update 📝 This PR needs a corresponding update to mdn/content to update the documentation labels Oct 13, 2021
@github-actions github-actions bot added the data:api 🐇 Compat data for Web APIs. https://developer.mozilla.org/docs/Web/API label Oct 13, 2021
@queengooborg queengooborg removed the needs content update 📝 This PR needs a corresponding update to mdn/content to update the documentation label Oct 13, 2021
@foolip
Copy link
Collaborator

foolip commented Oct 18, 2021

There's also a setConfiguration() method which takes this dictionary as an argument. I don't see any discussion about whether parameters should be added there too, or perhaps instead of in the constructor. Making a deliberate decision on this would be good.

@queengooborg queengooborg self-assigned this Nov 23, 2021
@queengooborg
Copy link
Collaborator Author

queengooborg commented Nov 30, 2021

I don't think that we have a set convention for this scenario, but in the MDN changes, I have made it so that the page for setConfiguration() has a link to the constructor to list the configuration options and their functions. Thus, users will head to the constructor's documentation to find more information about these parameters. As such, I think it makes sense to just keep the BCD data here!

@queengooborg queengooborg removed their assignment Nov 30, 2021
@foolip
Copy link
Collaborator

foolip commented Nov 30, 2021

Copy link
Collaborator

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

I don't trust the existing data here much at all. I double checked two, can you find the commits that added the rest in Chromium to double check?

api/RTCPeerConnection.json Outdated Show resolved Hide resolved
api/RTCPeerConnection.json Outdated Show resolved Hide resolved
api/RTCPeerConnection.json Outdated Show resolved Hide resolved
queengooborg and others added 2 commits December 2, 2021 04:46
Co-authored-by: Philip Jägenstedt <philip@foolip.org>
Copy link
Collaborator

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

I don't trust the original data at all here, so would like to see sources for at least all Chromium statements.

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

foolip commented Mar 4, 2022

@queengooborg can you turn this into a draft if it still needs research?

@queengooborg
Copy link
Collaborator Author

It's quite possible that our existing data is incorrect, but I'm not yet sure how to check it and find out what the accurate versions are. I think we can probably implement some form of test in the collector, but to do so, we'll need to fix the properties and where they're located. I'll open up an issue to remind myself!

In summary: I'm lazy and would like to defer fixing the version numbers to future me... 😛

@caugner
Copy link
Contributor

caugner commented May 12, 2022

This feature is a dictionary, enum, or WebIDL typedef and should not be included in BCD.

@queengooborg Maybe this is obvious, but is this mentioned anywhere in the contribution or data docs?

edit: If I interpret the changes correctly, this PR is essentially merging RTCConfiguration into RTCPeerConfiguration, right?

Copy link
Contributor

@caugner caugner left a comment

Choose a reason for hiding this comment

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

IMO it would be good to update the PR title, as RTCConfiguration is not just removed, but merged/integrated into RTCPeerConnection.

@queengooborg queengooborg changed the title Remove RTCConfiguration API from BCD Merge RTCConfiguration dict into RTCPeerConnection May 12, 2022
@queengooborg
Copy link
Collaborator Author

PR title has been updated and this is ready for review again!

@foolip foolip merged commit c425fb9 into mdn:main Jun 8, 2022
@foolip
Copy link
Collaborator

foolip commented Jun 8, 2022

I've filed #16595 about data that's probably wrong here.

@queengooborg queengooborg deleted the api/RTCConfiguration/removal branch June 8, 2022 12:49
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.

None yet

3 participants