859774 - Values of readyState in nsDOMDataChannel don't match the spec

Status: VERIFIED FIXED

(Core :: WebRTC: Networking, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

As landed on bug 733002 the values of the nsDOMDataChannel readyState don't match the spec.

http://dev.w3.org/2011/webrtc/editor/webrtc.html#idl-def-RTCDataChannelState

enum RTCDataChannelState {
    "connecting",
    "open",
    "closing",
    "closed"
};

http://hg.mozilla.org/mozilla-central/diff/ed6491f2335e/content/base/src/nsDOMDataChannel.cpp#l1.201

   1.201 +  const char * stateName[] = {
   1.202 +    "Connecting",
   1.203 +    "Open",
   1.204 +    "Closing",
   1.205 +    "Closed"
   1.206 +  };

Comment 1

[Jason Smith [:jsmith]]

Valid bug, but not blocking.

Comment 2

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

Attachment: change DataChannel readyState names to match spec (capitalization)

Comment 3

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

Comment on attachment 735224 [details] [diff] [review]
change DataChannel readyState names to match spec (capitalization)

Trivial patch to fix capitalization of readyState values to match spec

Comment 4

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

Given the low risk, I'd uplift this patch to Aurora 22

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 735224 [details] [diff] [review]
change DataChannel readyState names to match spec (capitalization)

http://dev.w3.org/2011/webrtc/editor/webrtc.html#idl-def-RTCDataChannelState

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Randell, don't we have any unit test for data channel yet which would cover certain states of the data channel state, e.g. after calling createDataChannel state has to be 'connecting'?

Comment 7

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

Some of these may be hard to test, since states can be transitory.  However, I think it's testable now.

Comment 8

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/dd9e94fd401f

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Randell Jesup [:jesup] from comment #7)
> Some of these may be hard to test, since states can be transitory.  However,
> I think it's testable now.

Do you wanna add tests on an unit test level? If that's not doable I could fold this into our datachannel mochitests.

Comment 10

[Jason Smith [:jsmith]]

(In reply to Henrik Skupin (:whimboo) from comment #9)
> (In reply to Randell Jesup [:jesup] from comment #7)
> > Some of these may be hard to test, since states can be transitory.  However,
> > I think it's testable now.
> 
> Do you wanna add tests on an unit test level? If that's not doable I could
> fold this into our datachannel mochitests.

FWIW, we probably should add the tests through mochitests, anyways. That falls in line with one of pieces of automation we need - attribute state checking.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/dd9e94fd401f

Comment 12

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

Comment on attachment 735224 [details] [diff] [review]
change DataChannel readyState names to match spec (capitalization)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a

User impact if declined: some applications will fail to see the states they expect per the spec, or may write code that uses our incorrect values.

Testing completed (on m-c, etc.): on m-c.

Risk to taking this patch (and alternatives if risky): nil - simple string changes for states (capitalization) to match the spec.

String or IDL/UUID changes made by this patch: no user-facing changes, just strings used as states in JS (no l10n issues).

Comment 13

[Jason Smith [:jsmith]]

Verified through a sanity check that I'm seeing two of the states in caps form.

Comment 14

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/913019e93240