Status

()

Core
WebRTC
P3
normal
Rank:
35
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: mt, Assigned: mt)

Tracking

(Blocks: 3 bugs, {dev-doc-needed, site-compat})

Trunk
mozilla44
dev-doc-needed, site-compat
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 affected, firefox44 fixed)

Details

(Whiteboard: [spec compliance])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments)

(Assignee)

Description

2 years ago
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.
mozGetUserMedia
(Assignee)

Comment 2

2 years ago
(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).

Updated

2 years ago
Blocks: 1165687
backlog: --- → webRTC+
Rank: 35
Priority: -- → P3
Whiteboard: [spec compliance]
(Assignee)

Comment 3

2 years ago
Created 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
Attachment #8651281 - Flags: review?(rjesup)
Attachment #8651281 - Flags: review?(bugs)
(Assignee)

Comment 4

2 years ago
Created attachment 8651282 [details]
MozReview Request: Bug 1155923 - Add Deprecated attribute to interfaces, r=peterv

Bug 1155923 - Temporarily restoring moz-prefixed interface, r?jesup,smaug
Attachment #8651282 - Flags: review?(rjesup)
Attachment #8651282 - Flags: review?(bugs)
(Assignee)

Comment 5

2 years ago
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.
(Assignee)

Updated

2 years ago
Keywords: leave-open
(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

2 years ago
>+++ 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.

Updated

2 years ago
Attachment #8651281 - Flags: review?(bugs) → review+

Comment 8

2 years ago
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
{
}
Attachment #8651282 - Flags: review?(bugs) → review-
(Assignee)

Comment 9

2 years ago
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.
(Assignee)

Updated

2 years ago
Attachment #8651281 - Attachment description: MozReview Request: Bug 1155923 - Removing moz prefix from RTC interfaces, r?jesup,smaug → MozReview Request: Bug 1155923 - Removing moz prefix from RTC interfaces, r?jesup=smaug
Attachment #8651281 - Flags: review+
(Assignee)

Comment 10

2 years ago
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
(Assignee)

Comment 11

2 years ago
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
Attachment #8651282 - Attachment description: MozReview Request: Bug 1155923 - Temporarily restoring moz-prefixed interface, r?jesup,smaug → MozReview Request: Bug 1155923 - Add Deprecated attribute to interfaces, r?peterv
Attachment #8651282 - Flags: review?(rjesup)
Attachment #8651282 - Flags: review?(peterv)
Attachment #8651282 - Flags: review-
(Assignee)

Comment 12

2 years ago
Created 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
Attachment #8651937 - Flags: review?(rjesup)
Attachment #8651937 - Flags: review?(bugs)
(Assignee)

Comment 13

2 years ago
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.
Assignee: nobody → martin.thomson

Updated

2 years ago
Attachment #8651281 - Flags: review?(rjesup) → review+
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 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!
Attachment #8651937 - Flags: review?(rjesup) → review+
-    "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?

Updated

2 years ago
Attachment #8651937 - Flags: review?(bugs) → review+
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.
(Assignee)

Comment 18

2 years ago
Sounds like it couldn't hurt.
(Assignee)

Comment 19

2 years ago
(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.
(Assignee)

Comment 20

2 years ago
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
Attachment #8651281 - Attachment description: MozReview Request: Bug 1155923 - Removing moz prefix from RTC interfaces, r?jesup=smaug → MozReview Request: Bug 1155923 - Removing moz prefix from RTC interfaces, r=jesup,smaug
(Assignee)

Comment 21

2 years ago
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
(Assignee)

Comment 22

2 years ago
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
Attachment #8651937 - Attachment description: MozReview Request: Bug 1155923 - Temporarily restoring moz-prefixed interface, r?jesup,smaug → MozReview Request: Bug 1155923 - Temporarily restoring moz-prefixed interface, r=jesup,smaug
Attachment #8651937 - Flags: review+
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?
Attachment #8651282 - Flags: review?(peterv)
Keywords: dev-doc-needed

Updated

2 years ago
Keywords: site-compat
Duplicate of this bug: 917092

Updated

2 years ago
Blocks: 775235
(Assignee)

Comment 25

2 years ago
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.
(Assignee)

Comment 26

2 years ago
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
(Assignee)

Comment 27

2 years ago
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
Attachment #8651282 - Flags: review?(peterv)
(Assignee)

Comment 28

2 years ago
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 on attachment 8651282 [details]
MozReview Request: Bug 1155923 - Add Deprecated attribute to interfaces, r=peterv

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

Nice!
Attachment #8651282 - Flags: review?(peterv) → review+
(Assignee)

Comment 30

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

Comment 31

2 years ago
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
Attachment #8651282 - Attachment description: MozReview Request: Bug 1155923 - Add Deprecated attribute to interfaces, r?peterv → MozReview Request: Bug 1155923 - Add Deprecated attribute to interfaces, r=peterv
(Assignee)

Updated

2 years ago
Attachment #8651937 - Flags: review?(bugs)
(Assignee)

Comment 32

2 years ago
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
(Assignee)

Comment 33

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

Carrying r+, damned reviewboard
Attachment #8651281 - Flags: review?(bugs) → review+
(Assignee)

Comment 34

2 years ago
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.
Attachment #8651937 - Flags: review?(bugs) → review+
Added the site compatibility doc: https://www.fxsitecompat.com/en-US/docs/2015/webrtc-interfaces-have-been-unprefixed/
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 36

2 years ago
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
Keywords: checkin-needed
sorry backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=14797696&repo=mozilla-inbound
Flags: needinfo?(martin.thomson)

Comment 38

2 years ago
Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a56e08c0443
(Assignee)

Comment 39

2 years ago
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
Attachment #8651281 - Flags: review+
(Assignee)

Comment 40

2 years ago
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
(Assignee)

Comment 41

2 years ago
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
Attachment #8651937 - Flags: review+
(Assignee)

Comment 42

2 years ago
Created attachment 8666864 [details]
MozReview Request: Bug 1155923 - Updating webplatform-tests, r?jib

Bug 1155923 - Updating webplatform-tests, r?jib
Attachment #8666864 - Flags: review?(jib)
Comment on attachment 8666864 [details]
MozReview Request: Bug 1155923 - Updating webplatform-tests, r?jib

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

lgtm.
Attachment #8666864 - Flags: review?(jib) → review+
(Assignee)

Comment 44

2 years ago
Updated the webplatform-tests.
Flags: needinfo?(martin.thomson)
(Assignee)

Updated

2 years ago
Keywords: leave-open → checkin-needed

Comment 45

2 years ago
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
Keywords: checkin-needed

Comment 46

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/143a6814b1d5
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
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
(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? :-)
Flags: needinfo?(martin.thomson)
(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.
(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 :-)
(Assignee)

Comment 51

2 years ago
I'd rather change the web-platform tests to point to navigator.mediaDevices.getUserMedia.  Fixing navigator.mozGetUserMedia isn't that much fun.
Flags: needinfo?(martin.thomson)
(Assignee)

Comment 52

2 years ago
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.
(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.
Blocks: 1050930
You need to log in before you can comment on or make changes to this bug.