Values of readyState in nsDOMDataChannel don't match the spec

VERIFIED FIXED in Firefox 22

Status

()

Core
WebRTC: Networking
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: whimboo, Assigned: jesup)

Tracking

unspecified
mozilla23
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox21 disabled, firefox22 fixed, firefox23 fixed)

Details

(Whiteboard: [webrtc][blocking-webrtc-][spec-issue])

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
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 +  };
Valid bug, but not blocking.
Whiteboard: [WebRTC] → [WebRTC][blocking-webrtc-][spec-issue]
(Assignee)

Comment 2

5 years ago
Created attachment 735224 [details] [diff] [review]
change DataChannel readyState names to match spec (capitalization)
(Assignee)

Comment 3

5 years ago
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
Attachment #735224 - Flags: review?(bugs)
(Assignee)

Comment 4

5 years ago
Given the low risk, I'd uplift this patch to Aurora 22
Whiteboard: [WebRTC][blocking-webrtc-][spec-issue] → [webrtc][blocking-webrtc-][spec-issue][webrtc-uplift]
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
Attachment #735224 - Flags: review?(bugs) → review+
(Reporter)

Comment 6

5 years ago
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'?
Status: NEW → ASSIGNED
(Assignee)

Comment 7

5 years ago
Some of these may be hard to test, since states can be transitory.  However, I think it's testable now.
(Assignee)

Comment 8

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd9e94fd401f
status-firefox21: --- → disabled
status-firefox22: --- → affected
status-firefox23: --- → affected
Target Milestone: --- → mozilla23
(Reporter)

Comment 9

5 years ago
(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.
Flags: in-testsuite?
(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.
https://hg.mozilla.org/mozilla-central/rev/dd9e94fd401f
Assignee: nobody → rjesup
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Keywords: verifyme
(Assignee)

Comment 12

5 years ago
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).
Attachment #735224 - Flags: approval-mozilla-aurora?

Updated

5 years ago
status-firefox23: affected → fixed
Verified through a sanity check that I'm seeing two of the states in caps form.
Status: RESOLVED → VERIFIED
Keywords: verifyme
Attachment #735224 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 14

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/913019e93240
status-firefox22: affected → fixed
Whiteboard: [webrtc][blocking-webrtc-][spec-issue][webrtc-uplift] → [webrtc][blocking-webrtc-][spec-issue]
You need to log in before you can comment on or make changes to this bug.