1173851 - Rename DataChannel to RTCDataChannel per specification

Status: RESOLVED FIXED

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

Description

[Eric Shepherd [:sheppy]]

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?

Comment 1

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

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]

Comment 2

[Philip Jägenstedt [:foolip]]

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.

Comment 3

[Philip Jägenstedt [:foolip]]

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.

Comment 4

[Bulk Bug Changes for mreavy's org]

Mass change P4->P5 to align with new Mozilla triage process.

Comment 5

[ashley]

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.

Comment 6

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

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 7

[Nils Ohlmeier [:drno]]

Attachment: Bug 1173851: rename DataChannel to RTCDataChannel

Review commit: https://reviewboard.mozilla.org/r/218214/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/218214/

Comment 8

[Code Review Bot [:reviewbot]]

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                       &

Comment 9

[Nils Ohlmeier [:drno]]

Comment on attachment 8948841 [details]
Bug 1173851: rename DataChannel to RTCDataChannel

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/218214/diff/1-2/

Comment 10

[Nils Ohlmeier [:drno]]

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?

Comment 11

[Code Review Bot [:reviewbot]]

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                       &

Comment 12

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

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

Comment 13

[Nils Ohlmeier [:drno]]

Comment on attachment 8948841 [details]
Bug 1173851: rename DataChannel to RTCDataChannel

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/218214/diff/2-3/

Comment 14

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

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.

Comment 15

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

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)

Comment 16

[Nils Ohlmeier [:drno]]

Attachment: Bug 1173851: re-enable passing WPT tests.

Review commit: https://reviewboard.mozilla.org/r/219902/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/219902/

Comment 17

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Comment on attachment 8950658 [details]
Bug 1173851: re-enable passing WPT tests.

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

LGTM

Comment 18

[Mozilla Autoland]

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)

Comment 19

[Nils Ohlmeier [:drno]]

Comment on attachment 8948841 [details]
Bug 1173851: rename DataChannel to RTCDataChannel

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/218214/diff/3-4/

Comment 20

[Nils Ohlmeier [:drno]]

Comment on attachment 8950658 [details]
Bug 1173851: re-enable passing WPT tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/219902/diff/1-2/

Comment 21

[Pulsebot]

Pushed by [e-mail]:
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

Comment 22

[Dorel Luca [:dluca]]

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

Comment 23

[Nils Ohlmeier [:drno]]

Attachment: Bug 1173851: renamed DataChannel -> RTCDataChannel in mochitest.

Review commit: https://reviewboard.mozilla.org/r/220370/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/220370/

Comment 24

[Nils Ohlmeier [:drno]]

Comment on attachment 8948841 [details]
Bug 1173851: rename DataChannel to RTCDataChannel

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/218214/diff/4-5/

Comment 25

[Nils Ohlmeier [:drno]]

Comment on attachment 8950658 [details]
Bug 1173851: re-enable passing WPT tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/219902/diff/2-3/

Comment 26

[Nils Ohlmeier [:drno]]

Comment on attachment 8951120 [details]
Bug 1173851: renamed DataChannel -> RTCDataChannel in mochitest.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220370/diff/1-2/

Comment 27

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

Comment on attachment 8951120 [details]
Bug 1173851: renamed DataChannel -> RTCDataChannel in mochitest.

https://reviewboard.mozilla.org/r/220370/#review226448

Comment 28

[Pulsebot]

Pushed by [e-mail]:
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

Comment 29

[Nils Ohlmeier [:drno]]

Clearing NI

Comment 30

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/2762e7c17577
https://hg.mozilla.org/mozilla-central/rev/74f4974ee014
https://hg.mozilla.org/mozilla-central/rev/66c3ef09ef93

Comment 31

[Kohei Yoshino]

Posted the site compatibility note: https://www.fxsitecompat.com/en-CA/docs/2012/datachannel-has-been-renamed-to-rtcdatachannel/

Comment 32

[Eric Rescorla (:ekr)]

That link appears to be broken

Comment 33

[Kohei Yoshino]

Correct link: https://www.fxsitecompat.com/en-CA/docs/2018/datachannel-has-been-renamed-to-rtcdatachannel/

Comment 34

[Eric Shepherd [:sheppy]]

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.