Eric Shepherd [:sheppy] Reporter
	Description • 9 years ago

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?

	Jan-Ivar Bruaroey [:jib] (needinfo? me)
	Comment 1 • 9 years ago

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]

	Philip Jägenstedt [:foolip]
	Comment 2 • 7 years ago

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.

	Philip Jägenstedt [:foolip]
	Comment 3 • 7 years ago

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.

	Bulk Bug Changes for mreavy's org
	Comment 4 • 7 years ago

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

	ashley
	Comment 5 • 6 years ago

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.

	Randell Jesup [:jesup] (needinfo me)
	Comment 6 • 6 years ago

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

	Code Review Bot [:reviewbot]
	Comment 8 • 6 years ago
mozreview-review

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                       &

	Nils Ohlmeier [:drno] Assignee
	Comment 10 • 6 years ago

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?

	Code Review Bot [:reviewbot]
	Comment 11 • 6 years ago
mozreview-review

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                       &

	Jan-Ivar Bruaroey [:jib] (needinfo? me)
	Comment 12 • 6 years ago

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

	Jan-Ivar Bruaroey [:jib] (needinfo? me)
	Comment 14 • 6 years ago
mozreview-review

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.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 15 • 6 years ago
mozreview-review

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)

	Nico Grunbaum [:ng, @chew:mozilla.org]
	Comment 17 • 6 years ago
mozreview-review

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

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

LGTM

	Mozilla Autoland
	Comment 18 • 6 years ago

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)

	Pulsebot
	Comment 21 • 6 years ago

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

	Dorel Luca [:dluca]
	Comment 22 • 6 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 27 • 6 years ago
mozreview-review

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

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

	Pulsebot
	Comment 28 • 6 years ago

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

	Nils Ohlmeier [:drno] Assignee
	Comment 29 • 6 years ago

Clearing NI

	Narcis Beleuzu [:NarcisB]
	Comment 30 • 6 years ago
bugherder

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

	Eric Rescorla (:ekr)
	Comment 32 • 6 years ago

That link appears to be broken

	Kohei Yoshino [:kohei]
	Comment 33 • 6 years ago

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

	Eric Shepherd [:sheppy] Reporter
	Comment 34 • 6 years ago

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.