1155923 - Unprefix WebRTC

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P3)

Description

[Martin Thomson [:mt:]]

This API isn't going to change in a way that will break compatibility.  Besides, we shouldn't have done this in the first place.

Identified:

mozRTCPeerConnection
mozRTCSessionDescription
mozRTCIceCandidate
HTMLMediaElement.mozSrcObject

For the conservative, we can add aliases for a couple of releases.

Comment 1

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

mozGetUserMedia

Comment 2

[Martin Thomson [:mt:]]

(In reply to Randell Jesup [:jesup] from comment #1)
> mozGetUserMedia

I considered it, but mediaDevices.getUserMedia isn't prefixed.  We can kill mozGetUserMedia when we remove the other prefixes (or maybe sooner).

Comment 3

[Martin Thomson [:mt:]]

Attachment: MozReview Request: Bug 1155923 - Removing moz prefix from RTC interfaces, r=jesup,smaug

Bug 1155923 - Removing moz prefix from RTC interfaces, r?jesup,smaug

Comment 4

[Martin Thomson [:mt:]]

Attachment: MozReview Request: Bug 1155923 - Add Deprecated attribute to interfaces, r=peterv

Bug 1155923 - Temporarily restoring moz-prefixed interface, r?jesup,smaug

Comment 5

[Martin Thomson [:mt:]]

It has become abundantly clear - even from just looking at this within the tree - that outright removing these interfaces will cause a huge number of problems.  For starters, all the adapter code that exists is in one of the following forms:

  tmp = webkitRTCPeerConnection || mozRTCPeerConnection; x = new tmp(); // Tokbox does this

  if (navigator.mozUserMedia) x = new mozRTCPeerConnection();

Few use the arguably correct form:

  RTCPeerConnection = RTCPeerConnection || webkitRTCPeerConnection || mozRTCPeerConnection;

So, my plan is to have two patches:

1. remove the prefix
2. create prefixed aliases for the interfaces, each with a deprecation warning that goes to the console

The second part is a little ugly, but I will provide a pointer to this bug in the code.  Hopefully we can back out the second patch when we are ready to remove the prefix properly.

Comment 6

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

(In reply to Martin Thomson [:mt:] from comment #5)
> So, my plan is to have two patches:
> 
> 1. remove the prefix
> 2. create prefixed aliases for the interfaces, each with a deprecation
> warning that goes to the console
> 
> The second part is a little ugly, but I will provide a pointer to this bug
> in the code.  Hopefully we can back out the second patch when we are ready
> to remove the prefix properly.

+1.  I was about to ask about this very thing, as no overlap would cause considerable problems (as you note).

Comment 7

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

>+++ b/dom/webidl/WebrtcDeprecated.webidl
>...
>+ * This file includes all the deprecated mozRTC prefixed interfaces.
>+ *
I don't see the file containing any deprecated interfaces, only deprecated properties. So, fix the comment.

Comment 8

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

Comment on attachment 8651282 [details]
MozReview Request: Bug 1155923 - Add Deprecated attribute to interfaces, r=peterv

I don't see how this approach could work, or at least the asserts might fire if the page overrides mozFoo properties in the global object.
The property might not be object anymore or so.

Couldn't we just have something like

[Same-stuff-here-as-for-SomeInterface]
interface moz<SomeInterface> : SomeInterface
{
}

Comment 9

[Martin Thomson [:mt:]]

Good catch on the page overrides, I was working on the (bad) assumption that WebIDL interfaces weren't writeable.

I tried the inheritance thing, and getting the [Deprecated] stuff working was tricky.  [Deprecated] only works on attributes and functions.  I'll try again.

Comment 10

[Martin Thomson [:mt:]]

Comment on attachment 8651281 [details]
MozReview Request: Bug 1155923 - Removing moz prefix from RTC interfaces, r=jesup,smaug

Bug 1155923 - Removing moz prefix from RTC interfaces, r?jesup=smaug

Comment 11

[Martin Thomson [:mt:]]

Comment on attachment 8651282 [details]
MozReview Request: Bug 1155923 - Add Deprecated attribute to interfaces, r=peterv

Bug 1155923 - Add Deprecated attribute to interfaces, r?peterv

Comment 12

[Martin Thomson [:mt:]]

Attachment: MozReview Request: Bug 1155923 - Temporarily restoring moz-prefixed interface, r=jesup,smaug

Bug 1155923 - Temporarily restoring moz-prefixed interface, r?jesup,smaug

Comment 13

[Martin Thomson [:mt:]]

OK, the final form of this required a little modification of the WebIDL bindings to support [Deprecated] attributes on interfaces.  But it works (my earlier attempt ran afoul of some strangeness in the codegen because I was using A implements B, which causes the codegen to #include "nsIB.h" and explode since B is JS-implemented).

The only catch that I found here is that the deprecated warning doesn't trigger for static methods.  RTCPeerConnection.generateCertificate is accessible as mozRTCPeerConnection.generateCertificate, but it doesn't trigger a deprecation warning.  I think that we can live with that.

Comment 14

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

Comment on attachment 8651281 [details]
MozReview Request: Bug 1155923 - Removing moz prefix from RTC interfaces, r=jesup,smaug

https://reviewboard.mozilla.org/r/16885/#review15021

::: browser/components/loop/test/shared/validate_test.js:21
(Diff revision 1)
> -    var mozRTC;
> +    var rtcsd;

Not loving the var name, but didn't love the one before either.  No big deal.

::: dom/webidl/WebrtcDeprecated.webidl:13
(Diff revision 2)
> +interface WebrtcDeprecated

webidl - needs DOM peer

Comment 15

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

Comment on attachment 8651937 [details]
MozReview Request: Bug 1155923 - Temporarily restoring moz-prefixed interface, r=jesup,smaug

https://reviewboard.mozilla.org/r/17033/#review15171

Ship It!

Comment 16

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

-    "RTCIceCandidate",
-// IMPORTANT: Do not change this list without review from a DOM peer!
-    "RTCPeerConnection",
-// IMPORTANT: Do not change this list without review from a DOM peer!
-    "RTCSessionDescription",
+    "mozRTCIceCandidate",
+// IMPORTANT: Do not change this list without review from a DOM peer!
+    "mozRTCPeerConnection",
+// IMPORTANT: Do not change this list without review from a DOM peer!
+    "mozRTCSessionDescription",
I don't understand this change. How can you remove support for non-prefixed interfaces?

Don't you just want to add those moz-prefixed ones?

Comment 17

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

Do we need to post an "Intent to implement RTCPeerConnection partially" on dev.platform? That's what we did for srcObject, which was arguably a smaller change than this.

There are still implementation pieces missing and API changes happening in RTCPeerConnection, like the switch from streams to tracks.

Comment 18

[Martin Thomson [:mt:]]

Sounds like it couldn't hurt.

Comment 19

[Martin Thomson [:mt:]]

(In reply to Olli Pettay [:smaug] (high review load) from comment #16)
> -    "RTCIceCandidate",
> -// IMPORTANT: Do not change this list without review from a DOM peer!
> -    "RTCPeerConnection",
> -// IMPORTANT: Do not change this list without review from a DOM peer!
> -    "RTCSessionDescription",
> +    "mozRTCIceCandidate",
> +// IMPORTANT: Do not change this list without review from a DOM peer!
> +    "mozRTCPeerConnection",
> +// IMPORTANT: Do not change this list without review from a DOM peer!
> +    "mozRTCSessionDescription",
> I don't understand this change. How can you remove support for non-prefixed
> interfaces?
> 
> Don't you just want to add those moz-prefixed ones?

That's a rebase error.  I'll fix it.  The intent is to have patch 1 remove mozRTC* and add RTC*.  Patch 2 should only restore mozRTC*, leaving RTC* in place.

Comment 20

[Martin Thomson [:mt:]]

Comment on attachment 8651281 [details]
MozReview Request: Bug 1155923 - Removing moz prefix from RTC interfaces, r=jesup,smaug

Bug 1155923 - Removing moz prefix from RTC interfaces, r=jesup,smaug

Comment 21

[Martin Thomson [:mt:]]

Comment on attachment 8651282 [details]
MozReview Request: Bug 1155923 - Add Deprecated attribute to interfaces, r=peterv

Bug 1155923 - Add Deprecated attribute to interfaces, r?peterv

Comment 22

[Martin Thomson [:mt:]]

Comment on attachment 8651937 [details]
MozReview Request: Bug 1155923 - Temporarily restoring moz-prefixed interface, r=jesup,smaug

Bug 1155923 - Temporarily restoring moz-prefixed interface, r=jesup,smaug

Comment 23

[Peter Van der Beken [:peterv]]

Comment on attachment 8651282 [details]
MozReview Request: Bug 1155923 - Add Deprecated attribute to interfaces, r=peterv

https://reviewboard.mozilla.org/r/16887/#review16341

Please add some tests to the .webidl files in dom/bindings/test (similar to the existing tests for Desprecated on attributes and methods).

::: dom/bindings/Codegen.py:1688
(Diff revision 3)
> +                """ % self.descriptor.interface.getExtendedAttribute("Deprecated")[0])

Please add a helper for this in BindingUtils.cpp that you can call from here and from the code that you copied this from.

::: dom/bindings/Codegen.py:12822
(Diff revision 3)
> +                    any(m.getExtendedAttribute("Deprecated") for m in iface.members + [iface]))

Why is the change needed? Doesn't the existing " + [iface]" cover this already?

Comment 25

[Martin Thomson [:mt:]]

https://reviewboard.mozilla.org/r/16887/#review16341

> Please add a helper for this in BindingUtils.cpp that you can call from here and from the code that you copied this from.

I did that, then discovered that I the second check was unnecessary - the new check was sufficient to cover all the cases.  Still, I think that it's cleaner to have the code in BindingUtils.

Comment 26

[Martin Thomson [:mt:]]

Comment on attachment 8651281 [details]
MozReview Request: Bug 1155923 - Removing moz prefix from RTC interfaces, r=jesup,smaug

Bug 1155923 - Removing moz prefix from RTC interfaces, r=jesup,smaug

Comment 27

[Martin Thomson [:mt:]]

Comment on attachment 8651282 [details]
MozReview Request: Bug 1155923 - Add Deprecated attribute to interfaces, r=peterv

Bug 1155923 - Add Deprecated attribute to interfaces, r?peterv

Comment 28

[Martin Thomson [:mt:]]

Comment on attachment 8651937 [details]
MozReview Request: Bug 1155923 - Temporarily restoring moz-prefixed interface, r=jesup,smaug

Bug 1155923 - Temporarily restoring moz-prefixed interface, r=jesup,smaug

Comment 29

[Peter Van der Beken [:peterv]]

Comment on attachment 8651282 [details]
MozReview Request: Bug 1155923 - Add Deprecated attribute to interfaces, r=peterv

https://reviewboard.mozilla.org/r/16887/#review17695

Nice!

Comment 30

[Martin Thomson [:mt:]]

Comment on attachment 8651281 [details]
MozReview Request: Bug 1155923 - Removing moz prefix from RTC interfaces, r=jesup,smaug

Bug 1155923 - Removing moz prefix from RTC interfaces, r=jesup,smaug

Comment 31

[Martin Thomson [:mt:]]

Comment on attachment 8651282 [details]
MozReview Request: Bug 1155923 - Add Deprecated attribute to interfaces, r=peterv

Bug 1155923 - Add Deprecated attribute to interfaces, r=peterv

Comment 32

[Martin Thomson [:mt:]]

Comment on attachment 8651937 [details]
MozReview Request: Bug 1155923 - Temporarily restoring moz-prefixed interface, r=jesup,smaug

Bug 1155923 - Temporarily restoring moz-prefixed interface, r=jesup,smaug

Comment 33

[Martin Thomson [:mt:]]

Comment on attachment 8651281 [details]
MozReview Request: Bug 1155923 - Removing moz prefix from RTC interfaces, r=jesup,smaug

Carrying r+, damned reviewboard

Comment 34

[Martin Thomson [:mt:]]

Comment on attachment 8651937 [details]
MozReview Request: Bug 1155923 - Temporarily restoring moz-prefixed interface, r=jesup,smaug

Carrying r=smaug, sorry about that, I blame RB.

Comment 35

[Kohei Yoshino]

Added the site compatibility doc: https://www.fxsitecompat.com/en-US/docs/2015/webrtc-interfaces-have-been-unprefixed/

Comment 36

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2981db92416a
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ac38acd6ab2
https://hg.mozilla.org/integration/mozilla-inbound/rev/321a932e01a7

Comment 37

[Carsten Book [:Tomcat]]

sorry backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=14797696&repo=mozilla-inbound

Comment 38

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a56e08c0443

Comment 39

[Martin Thomson [:mt:]]

Comment on attachment 8651281 [details]
MozReview Request: Bug 1155923 - Removing moz prefix from RTC interfaces, r=jesup,smaug

Bug 1155923 - Removing moz prefix from RTC interfaces, r=jesup,smaug

Comment 40

[Martin Thomson [:mt:]]

Comment on attachment 8651282 [details]
MozReview Request: Bug 1155923 - Add Deprecated attribute to interfaces, r=peterv

Bug 1155923 - Add Deprecated attribute to interfaces, r=peterv

Comment 41

[Martin Thomson [:mt:]]

Comment on attachment 8651937 [details]
MozReview Request: Bug 1155923 - Temporarily restoring moz-prefixed interface, r=jesup,smaug

Bug 1155923 - Temporarily restoring moz-prefixed interface, r=jesup,smaug

Comment 42

[Martin Thomson [:mt:]]

Attachment: MozReview Request: Bug 1155923 - Updating webplatform-tests, r?jib

Bug 1155923 - Updating webplatform-tests, r?jib

Comment 43

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

Comment on attachment 8666864 [details]
MozReview Request: Bug 1155923 - Updating webplatform-tests, r?jib

https://reviewboard.mozilla.org/r/20609/#review18489

lgtm.

Comment 44

[Martin Thomson [:mt:]]

Updated the webplatform-tests.

Comment 45

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/42657128cf56
https://hg.mozilla.org/integration/mozilla-inbound/rev/f8f71e377d67
https://hg.mozilla.org/integration/mozilla-inbound/rev/de9abaf6b170
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d20341337cc

Comment 46

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/143a6814b1d5

Comment 47

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/42657128cf56
https://hg.mozilla.org/mozilla-central/rev/f8f71e377d67
https://hg.mozilla.org/mozilla-central/rev/de9abaf6b170
https://hg.mozilla.org/mozilla-central/rev/6d20341337cc
https://hg.mozilla.org/mozilla-central/rev/143a6814b1d5

Comment 48

[Andreas Pehrson [:pehrsons]]

(In reply to Martin Thomson [:mt:] from comment #2)
> (In reply to Randell Jesup [:jesup] from comment #1)
> > mozGetUserMedia
> 
> I considered it, but mediaDevices.getUserMedia isn't prefixed.  We can kill
> mozGetUserMedia when we remove the other prefixes (or maybe sooner).

mozGetUserMedia is keeping a lot of web-platform tests from passing (like all of mediacapture) so we should push for updating them upstream then. Does this sound like something you want to do Martin? :-)

Comment 49

[Nils Ohlmeier [:drno]]

(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #48)
> mozGetUserMedia is keeping a lot of web-platform tests from passing (like
> all of mediacapture) so we should push for updating them upstream then. Does
> this sound like something you want to do Martin? :-)

Is this on treeherder or locally on your system? Last time I checked, quite some time ago, we were passing most of them if the tests got started with the option to use the prefixes.

Comment 50

[Andreas Pehrson [:pehrsons]]

(In reply to Nils Ohlmeier [:drno] from comment #49)
> (In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #48)
> Is this on treeherder or locally on your system?

Both.

I didn't try with prefixes, nor do I know how to :-)

Comment 51

[Martin Thomson [:mt:]]

I'd rather change the web-platform tests to point to navigator.mediaDevices.getUserMedia.  Fixing navigator.mozGetUserMedia isn't that much fun.

Comment 52

[Martin Thomson [:mt:]]

Oh, we could add [Deprecated] on navigator.mozGetUserMedia if that would make you happy.  That's a trivial change; the only problem being that it would require DOM peer review.

Comment 53

[Andreas Pehrson [:pehrsons]]

(In reply to Martin Thomson [:mt:] from comment #51)
> I'd rather change the web-platform tests to point to
> navigator.mediaDevices.getUserMedia.

Yeah, this is exactly what I am trying to push for.