859565 - Remove readyState legacy attribute in future release

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[Jason Smith [:jsmith]]

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.

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 2

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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.

Comment 3

[Jason Smith [:jsmith]]

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

Comment 4

[Nils Ohlmeier [:drno]]

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

Comment 5

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: removed legacy PeerConnectionImpl.readyState

Deleted lots of code.

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

Comment 6

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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 7

[Kohei Yoshino]

So it won't affect site compat.

Comment 8

[Adam Roach [:abr]]

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.

Comment 9

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: removed legacy PeerConnectionImpl.readyState (2) r=abr

Fixed nit. Carrying forward r=abr.

Comment 10

[Carsten Book [:Tomcat]]

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

Comment 11

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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

Comment 12

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 14

[Bobby Holley (:bholley)]

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.

Comment 15

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: removed legacy PeerConnectionImpl.readyState (3) r=bholley, abr

Updated commit msg. Carrying forward r=bholley, abr

Comment 16

[Ryan VanderMeulen [:RyanVM]]

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

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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