Closed Bug 1173851 Opened 9 years ago Closed 6 years ago

Rename DataChannel to RTCDataChannel per specification

Categories

(Core :: WebRTC: Networking, defect, P5)

defect

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: sheppy, Assigned: drno)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, site-compat)

Attachments

(3 files)

Currently, our WebRTC implementation uses the name "DataChannel"; however, the specification says it should be "RTCDataChannel".

While we have a binding in place to map uses of "RTCDataChannel" to work, the object type is reported in console (and presumably elsewhere) as "DataChannel". This may be a problem for developers, either while debugging or in code that looks at object types for some reason.

Perhaps the core object should be RTCDataChannel with DataChannel as a binding?
Since these objects are created through a function rather than a named constructor, this isn't a super-urgent compatibility issue, but we should still fix it.

The issue is observable in JS using:

    var pc = new mozRTCPeerConnection();
    var dc = pc.createDataChannel("test");
    console.log(Object.prototype.toString.call(dc));

which emits: [object DataChannel]
Blocks: webrtc_spec
backlog: --- → webRTC+
Rank: 45
Priority: -- → P4
FWIW, I'd be happy to see this fixed, because I keep looking for dom/webidl/RTCDataChannel.webidl and not finding it, before remembering it's called DataChannel.
A side benefit is that tools we're working on to find interop issues would then understand that it's a problem that Chrome has RTCDataChannel.prototype.maxRetransmits, but Firefox does not.
Mass change P4->P5 to align with new Mozilla triage process.
Priority: P4 → P5
This is an important compatibility issue now that Edge supports WebRTC but not DataChannels. To detect if DataChannels is supported, the obvious thing to do is check if RTCDataChannel is defined. It is in Chrome and Safari, but not Edge, so at a glance it looks like it'll do the job. However it's also undefined in Firefox because it uses a different name. This just caught us out and we accidentally disabled DataChannel support in Firefox due to this. It's an easy fix but a bit of a gotcha.

So currently to detect if DataChannels are supported you have to check if either RTCDataChannel or DataChannel are defined, which is a bit unintuitive. If you get it wrong you either enable DataChannels in Edge where they won't work, or needlessly disable DataChannels in Firefox.
You can also test if createDataChannel() returns anything, per comment 1:

    var pc = new RTCPeerConnection();
    var dc = pc.createDataChannel("test");

with appropriate tests of pc/dc/try's/etc
Comment on attachment 8948841 [details]
Bug 1173851: rename DataChannel to RTCDataChannel

https://reviewboard.mozilla.org/r/218214/#review224032


Code analysis found 1 defect in this patch:
 - 1 defect found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1405
(Diff revision 1)
>    return tmp3.forget();
>  }
>  
>  
>  // Not a member function so that we don't need to keep the PC live.
> -static void NotifyDataChannel_m(RefPtr<nsIDOMDataChannel> aChannel,
> +static void NotifyDataChannel_m(RefPtr<nsIDOMRTCDataChannel> aChannel,

Warning: The parameter 'achannel' is copied for each invocation but only used as a const reference; consider making it a const reference [clang-tidy: performance-unnecessary-value-param]

static void NotifyDataChannel_m(RefPtr<nsIDOMRTCDataChannel> aChannel,
                                                             ^
                                const                       &
jib: as you can see in the attached patch I not only renamed the webidl part, but also our internal type. Do you think that is a problem?

Second question: do we need to keep DataChannel around for some time (e.g. with deprecation warnings)? If so, how can I get that added?
Flags: needinfo?(jib)
Comment on attachment 8948841 [details]
Bug 1173851: rename DataChannel to RTCDataChannel

https://reviewboard.mozilla.org/r/218214/#review224044


Code analysis found 1 defect in this patch:
 - 1 defect found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1405
(Diff revision 2)
>    return tmp3.forget();
>  }
>  
>  
>  // Not a member function so that we don't need to keep the PC live.
> -static void NotifyDataChannel_m(RefPtr<nsIDOMDataChannel> aChannel,
> +static void NotifyDataChannel_m(RefPtr<nsIDOMRTCDataChannel> aChannel,

Warning: The parameter 'achannel' is copied for each invocation but only used as a const reference; consider making it a const reference [clang-tidy: performance-unnecessary-value-param]

static void NotifyDataChannel_m(RefPtr<nsIDOMRTCDataChannel> aChannel,
                                                             ^
                                const                       &
(In reply to Nils Ohlmeier [:drno] from comment #10)
> jib: as you can see in the attached patch I not only renamed the webidl
> part, but also our internal type. Do you think that is a problem?

I don't see any reason to change the internal nsIDOMDataChannel name. nsIDOMRTCDataChannel is a bit of a mouthful.

That said, I don't know know of any uses of it that would break.

> Second question: do we need to keep DataChannel around for some time (e.g.
> with deprecation warnings)? If so, how can I get that added?

No, see comment 1.
Flags: needinfo?(jib)
Assignee: nobody → drno
Attachment #8948841 - Flags: review?(jib)
Comment on attachment 8948841 [details]
Bug 1173851: rename DataChannel to RTCDataChannel

https://reviewboard.mozilla.org/r/218214/#review225466

Lgtm. You'll need DOM review though.
Attachment #8948841 - Flags: review?(jib) → review+
Attachment #8948841 - Flags: review?(bugs)
Comment on attachment 8948841 [details]
Bug 1173851: rename DataChannel to RTCDataChannel

https://reviewboard.mozilla.org/r/218214/#review225526

Hopefully not too many pages break because we fix the interface name so late.
(comment 3 is a tad worrisome from that point of view)
Attachment #8948841 - Flags: review?(bugs) → review+
Comment on attachment 8950658 [details]
Bug 1173851: re-enable passing WPT tests.

https://reviewboard.mozilla.org/r/219902/#review225716

LGTM
Attachment #8950658 - Flags: review?(na-g) → review+
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 9c57775224792ac25406c6ec70be96ad87e1da64 -d 24721e406e20: rebasing 447243:9c5777522479 "Bug 1173851: rename DataChannel to RTCDataChannel r=jib,smaug"
merging xpcom/reflect/xptinfo/ShimInterfaceInfo.cpp
rebasing 447244:6f59f5607336 "Bug 1173851: re-enable passing WPT tests. r=ng" (tip)
merging testing/web-platform/meta/webrtc/interfaces.https.html.ini
warning: conflicts while merging testing/web-platform/meta/webrtc/interfaces.https.html.ini! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by drno@ohlmeier.org:
https://hg.mozilla.org/integration/autoland/rev/36e6f0595461
rename DataChannel to RTCDataChannel r=jib,smaug
https://hg.mozilla.org/integration/autoland/rev/aba40941f027
re-enable passing WPT tests. r=ng
Backed out 2 changesets (bug 1173851) for Mochitest failures on dom/tests/mochitest/general/test_interfaces.html. CLOSED TREE

Log of the failure:
https://treeherder.mozilla.org/logviewer.html#?job_id=162270913&repo=autoland&lineNumber=10703

Backout changeset:
https://hg.mozilla.org/integration/autoland/rev/18d26380ad3d1fa89c4b428170a458bc97b06bfc

Push that got backed out:
https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=aba40941f0274fb7b47a896fc6519875174e1f1f
Flags: needinfo?(drno)
Comment on attachment 8951120 [details]
Bug 1173851: renamed DataChannel -> RTCDataChannel in mochitest.

https://reviewboard.mozilla.org/r/220370/#review226448
Attachment #8951120 - Flags: review?(bugs) → review+
Pushed by drno@ohlmeier.org:
https://hg.mozilla.org/integration/autoland/rev/2762e7c17577
rename DataChannel to RTCDataChannel r=jib,smaug
https://hg.mozilla.org/integration/autoland/rev/74f4974ee014
re-enable passing WPT tests. r=ng
https://hg.mozilla.org/integration/autoland/rev/66c3ef09ef93
renamed DataChannel -> RTCDataChannel in mochitest. r=smaug
Clearing NI
Flags: needinfo?(drno)
Keywords: dev-doc-needed
That link appears to be broken
A PR is in to update the compatibility database: https://github.com/mdn/browser-compat-data/pull/1831. Once this is approved and merged, it will then get pulled onto the production site. Hopefully within a week or so.

Also updated Firefox 60 for developers.
You need to log in before you can comment on or make changes to this bug.