Prefer getReceivers over getRemoteStreams

[jlaine]

In most places, we prefer to use the modern getReceivers/getSenders
instead of the legacy getRemoteStreams/getLocalStreams. However there
were two places where this logic was reversed.

[jlaine]

A question : do we really need the getDescription/setDescription events? This seems like an internal implementation detail which should not leak into the API. In any case these two events don't even seem to match the documentation, which states these events are fired with "sdpWrapper" as their argument.

https://sipjs.com/api/0.8.0/sessionDescriptionHandler/#getdescription

[egreenmachine]

I have to run this through some manual testing to make sure it doesn't break anything. I originally had it this way, but then reversed it during some browser testing.

In terms of the events, I think they should be left. The events across the library generally are inconsistent and there are lots that do not make sense. A complete rework of the events is on my wishlist, but until then I am inclined to just leave them in place. As for the sdpWrapper argument, it should match what is there. It is really a RTCSessionDescription per WebRTC spec. I can update the documentation as such.

[jlaine]

Sorry, ignore my last comment, I meant : do we need the setRemoteDescription event? It isn't documented nor is it used.

[jlaine]

As for tests, I tested "Simple" on Linux using:

• Chrome 62
• Firefox 56

[egreenmachine]

It looks like setRemoteDescription event is safe to remove. It is tough to see how these applications would be useful for applications. If that event is removed then the peerConnection-setRemoteDescriptionFailed event should also be removed.

[jlaine]

I agree concerning peerConnection-setRemoteDescriptionFailed. Do you want a separate PR for the removal of these two events or should I just include it in this PR?

[jlaine]

We also have a peerConnection-SetLocalDescriptionFailed (note the inconsistent capital "S"), which would probably be better replaced self.logger.error(e) to be consistent with how setRemoteDescription fails

[egreenmachine]

You can add it to this PR or submit a new one. No preference on our end.

[jlaine]

I'll submit a separate pull request, so this one is ready as far as I'm concerned.

[egreenmachine]

I ran this through all the browsers, and it worked well. Due to how potentially disruptive this small change may be, I am going to let it sit for a few days incase anyone else has comments on it or anything else comes up on our end.

[jlaine]

Sure, take your time. By the way, do you think we can cut a release soon, I'm quite eager to get the other fixes out?

[egreenmachine]

Planning to cut a release of 0.8.4 once this is merged in (assuming nothing else comes up before then). Then planning on 0.9.0 shortly after that for #459 as it is API breaking.

[jlaine]

Ok. 0.9 sounds like the target for weeding out the undocumented events then

[jlaine]

Fantastic, thanks Eric, and thanks for pushing out the release! I have a feeling that a fast release schedule is highly desirable as most users (myself included) only discover features once they hit a release.