Closed Bug 859565 Opened 7 years ago Closed 5 years ago
State legacy attribute in future release
We're heading in the direction of removing the readyState parameter from the PeerConnection object. If the working group agrees, let's pull the parameter from the PeerConnection object. This bug tracks the work to remove the attribute if the working group agrees.
Whiteboard: [WebRTC][blocking-webrtc-] → [WebRTC][blocking-webrtc-][spec-issue]
It appears readyState isn't going anywhere....
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
There was some confusion. The editor's draft refers to three different "readyState"s: 1) In onsignalingstatechange - a missed s/readyState/signalingstate/ rename typo. 2) RTCDataChannel.readyState 3) MediaStreamTrack.readyState Only #1 is relevant to this bug, as readyState was the old name for signalingstate. That use of the name is indeed gone away (once this typo is fixed, I've mailed the list about it). We currently support the old name for compatibility with older Firefox releases, but we emit a warning to web console on its use. I'm re-purposing this bug as a reminder to remove it in the future. Marked as minor.
Severity: normal → minor
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Summary: Remove the readyState parameter if the W3C group agrees to remove it from the PeerConnection spec → Remove readyState legacy attribute in future release
Looks like it's been removed from the PC object, so closing as wfm.
Status: REOPENED → RESOLVED
Closed: 6 years ago → 6 years ago
Resolution: --- → WORKSFORME
Looks to me like we still have a PC.readyState: http://dxr.mozilla.org/mozilla-central/source/dom/webidl/PeerConnectionImpl.webidl#71 As jib pointed out we should get rid of it (also in the test code (pc.js)).
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
Deleted lots of code. Note: CheckApiState() now probably does something whereas before it didn't. Passes all unittests and mochitests locally.
Assignee: nobody → jib
Status: REOPENED → ASSIGNED
Attachment #8419571 - Flags: review?(adam)
5 years ago
Note that the public-facing RTCPeerConnection.readyState was removed last year in Bug 929530, and the code removed here is the internal PeerConnectionImpl.readyState, which should be unused.
Comment on attachment 8419571 [details] [diff] [review] removed legacy PeerConnectionImpl.readyState Review of attachment 8419571 [details] [diff] [review]: ----------------------------------------------------------------- LGTM. One small suggestion. ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp @@ +1794,5 @@ > case ADDICECANDIDATE: > mRemoteSDP = aInfo->getSDP(); > break; > > case CONNECTED: I think we can just take this case out now -- the logging adds basically no value beyond what we can see in the sipcc logs. My guess is that it was originally added to ensure that the "CONNECTED" event was flowing all the way up to this level; but, since we don't care about that any more, let's just remove it.
Attachment #8419571 - Flags: review?(adam) → review+
Fixed nit. Carrying forward r=abr.
Hi, could you provide a Try link. Suggestions for what to run if you haven't yet can be found here: https://wiki.mozilla.org/Sheriffing/How:To:Recommended_Try_Practices
Patch applies (with offset) for me. Is anything else needed for this to land?
The commit hook requiring a DOM peer to review WebIDL changes says this patch is lacking that DOM peer's review.
Attachment #8424363 - Flags: review?(bobbyholley)
Comment on attachment 8424363 [details] [diff] [review] removed legacy PeerConnectionImpl.readyState (2) r=abr Review of attachment 8424363 [details] [diff] [review]: ----------------------------------------------------------------- I don't understand the PeerConnection architecture enough to confirm comment 6, but I'll take jib's word for it.
Attachment #8424363 - Flags: review?(bobbyholley) → review+
Updated commit msg. Carrying forward r=bholley, abr
Status: ASSIGNED → RESOLVED
Closed: 6 years ago → 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.