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

Added support for RTCPeerConnection.getStats() #163

Closed
wants to merge 1 commit into from
Closed

Added support for RTCPeerConnection.getStats() #163

wants to merge 1 commit into from

Conversation

oNaiPs
Copy link
Contributor

@oNaiPs oNaiPs commented Apr 14, 2016

This PR implements the RTCPeerConnection.getStats() as per the RFC.

@oNaiPs
Copy link
Contributor Author

oNaiPs commented Apr 14, 2016

@ibc what kind of documentation to you need?

@justindujardin
Copy link

Please consider this pull request. We use getStats to determine connection health for users, and would like to do the same for iOS users.

@@ -584,6 +587,84 @@ RTCPeerConnection.prototype.createDataChannel = function (label, options) {
return new RTCDataChannel(this, label, options);
};

RTCPeerConnection.prototype.getStats = function () {
var self = this,
isPromise,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is inconsistent with the rest of the plugin. IMHO the switch to promises should happen all at once in a major release, say 4.0. So IMHO I'd just have the callback API here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most of the APIs in RTCPeerConnection.js are using promises already (look at setLocalDescription for example), what do you mean by being inconsistent?

Copy link
Collaborator

Choose a reason for hiding this comment

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

You're right, I had a brain meltdown there. Disregard that comment.

@saghul
Copy link
Collaborator

saghul commented Jun 16, 2016

I was looking into getting statistics myself and found this open PR. Thanks @oNaiPs! I made some comments. One notable thing missing is documentation: some text here would be necessary, including a link to a reference which mentions what data is to be expected in the reports.

@oNaiPs
Copy link
Contributor Author

oNaiPs commented Jun 17, 2016

@saghul I commented your inlines. In relation to the expected data, this comes directly from the WebRTC library. I didn't put what are the results, same way i don't know the results in chrome until i try the API call.

@saghul
Copy link
Collaborator

saghul commented Jun 18, 2016

I didn't put what are the results, same way i don't know the results in chrome until i try the API call.

You don't need to document every single option there, just showing an example and a link pointing to some reference should be enough).

Is it really that hard to call the API once and show and example of its output? Following your reasoning, why document anything at all? Users can just try the API calls and see what happens.

@oNaiPs
Copy link
Contributor Author

oNaiPs commented Jun 25, 2016

@saghul the documentation is actually already there. Check this on the spec API link.

@ibc
Copy link
Collaborator

ibc commented Jul 1, 2016

Sorry, time for holidays. I will handle this PR on my return.

@saghul
Copy link
Collaborator

saghul commented Nov 21, 2016

Landed with some slight changes in d40e7da. Thanks for the perseverance @oNaiPs :-)

@saghul saghul closed this Nov 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants