Jason Smith [:jsmith] Reporter
	Description • 11 years ago

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.

	Randell Jesup [:jesup] (needinfo me)
	Comment 1 • 11 years ago

It appears readyState isn't going anywhere....

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 2 • 11 years ago

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.

	Jason Smith [:jsmith] Reporter
	Comment 3 • 11 years ago

Looks like it's been removed from the PC object, so closing as wfm.

	Nils Ohlmeier [:drno]
	Comment 4 • 10 years ago

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

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 5 • 10 years ago

Deleted lots of code.

Note: CheckApiState() now probably does something whereas before it didn't.
Passes all unittests and mochitests locally.

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 6 • 10 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.

	Kohei Yoshino
	Comment 7 • 10 years ago

So it won't affect site compat.

	Adam Roach [:abr]
	Comment 8 • 10 years ago

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.

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 9 • 10 years ago

Fixed nit. Carrying forward r=abr.

	Carsten Book [:Tomcat]
	Comment 10 • 10 years ago

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

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 11 • 10 years ago

Try - https://tbpl.mozilla.org/?tree=Try&rev=10c797553d85

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 12 • 10 years ago

Patch applies (with offset) for me. Is anything else needed for this to land?

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 13 • 10 years ago

The commit hook requiring a DOM peer to review WebIDL changes says this patch is lacking that DOM peer's review.

	Bobby Holley (:bholley)
	Comment 14 • 10 years ago

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.

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 15 • 10 years ago

Updated commit msg. Carrying forward r=bholley, abr

	Ryan VanderMeulen [:RyanVM]
	Comment 16 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/319ad9a67957

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 17 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/319ad9a67957