Closed Bug 859565 Opened 7 years ago Closed 5 years ago

Remove readyState legacy attribute in future release

Categories

(Core :: WebRTC, defect, minor)

defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: jsmith, Assigned: jib)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-needed, Whiteboard: [WebRTC][blocking-webrtc-][spec-issue])

Attachments

(1 file, 2 obsolete files)

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-]
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 ago6 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 → ---
QA Contact: jsmith → drno
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)
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.
So it won't affect site compat.
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.
Attachment #8419571 - Attachment is obsolete: true
Attachment #8424363 - Flags: review+
Keywords: checkin-needed
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
Keywords: checkin-needed
Keywords: checkin-needed
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.
Flags: needinfo?(jib)
Flags: needinfo?(adam)
Keywords: checkin-needed
Attachment #8424363 - Flags: review?(bobbyholley)
Flags: needinfo?(jib)
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
Attachment #8424363 - Attachment is obsolete: true
Attachment #8430320 - Flags: review+
Flags: needinfo?(adam)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/319ad9a67957
Status: ASSIGNED → RESOLVED
Closed: 6 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Blocks: 1050930
You need to log in before you can comment on or make changes to this bug.