Closed Bug 1064223 Opened 7 years ago Closed 6 years ago

Retire backwards compatible RTCOfferOptions in about 6 weeks

Categories

(Core :: WebRTC, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed
Blocking Flags:

People

(Reporter: jib, Assigned: jib)

References

Details

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

Attachments

(1 file)

Bug 1063808 added support for constraint-like backwards compatible RTCOfferOptions format with warnings. This bug is to remember to remove that again eventually, preferably in 6 weeks if Bug 1063808 is uplifted.
(In reply to Jan-Ivar Bruaroey [:jib] from comment #0)
> preferably in 6 weeks if Bug 1063808 is uplifted.

Let's wait to see when Chrome gets this in release. We really want to start the clock ticking only when both implementations have been updates.
Jib - does chrome have this in release now?
backlog: --- → webRTC+
Rank: 35
Flags: needinfo?(jib)
Priority: -- → P3
Yes, since August it seems [1]. Works too: http://jsfiddle.net/v7dcwvkg

A bit surprising perhaps with so many still using the old format (which is still accepted).

[1] https://code.google.com/p/webrtc/issues/detail?id=3282
Flags: needinfo?(jib)
(In reply to Jan-Ivar Bruaroey [:jib] from comment #3)
> Yes, since August it seems [1].

Not accounting for release trains. Some people on stackoverflow had problems as recently as February, but it works now.
Ping.
Flags: needinfo?(jib)
Bug 1064223 - remove support for constraint-like RTCOfferOptions predecessor
Attachment #8650801 - Flags: review?(martin.thomson)
pong.
Assignee: nobody → jib
Flags: needinfo?(jib)
Comment on attachment 8650801 [details]
MozReview Request: Bug 1064223 - remove support for constraint-like RTCOfferOptions predecessor

https://reviewboard.mozilla.org/r/16697/#review14971

Needs more deleting, but it's OK with me.

::: dom/media/PeerConnection.js:718
(Diff revision 1)
>        if (options && convertLegacyOptions(options)) {
> -        this.logWarning(
> -          "Mandatory/optional in createOffer options is deprecated! Use " +
> +        this.logError(
> +          "Mandatory/optional in createOffer options no longer works! Use " +
>              JSON.stringify(options) + " instead (note the case difference)!",
>            null, 0);
> +        options = {};
>        }

I think that you could just check for options.mandatory || options.optional here and delete the conversion function.  That's a lot of code that is (now) both untested and effectively unused.  I know that we get less of a good signal that the deprecated stuff is there without it, but I'd rather just eliminate it.

::: dom/media/tests/mochitest/test_peerConnection_bug1064223.html:11
(Diff revision 1)
> +    title: "CreateOffer fails without streams"

Fix title
Attachment #8650801 - Flags: review?(martin.thomson) → review+
Comment on attachment 8650801 [details]
MozReview Request: Bug 1064223 - remove support for constraint-like RTCOfferOptions predecessor

Bug 1064223 - remove support for constraint-like RTCOfferOptions predecessor
Attachment #8650801 - Flags: review+ → review?(martin.thomson)
https://reviewboard.mozilla.org/r/16697/#review14971

> I think that you could just check for options.mandatory || options.optional here and delete the conversion function.  That's a lot of code that is (now) both untested and effectively unused.  I know that we get less of a good signal that the deprecated stuff is there without it, but I'd rather just eliminate it.

The conversion code is used in the error message, hence is not untested. It is also unchanged, so there's little chance of regression here in the few weeks this code has left.

You'd be surprised how few web-devs actually know the right thing to put in here, so I highly recommend keeping this in.
Comment on attachment 8650801 [details]
MozReview Request: Bug 1064223 - remove support for constraint-like RTCOfferOptions predecessor

I guess I can live with that, though I don't think that we should be - in general - responsible for providing that sort of assistance, particularly since the l10n story is so poor for these warnings.
Attachment #8650801 - Flags: review?(martin.thomson) → review+
https://hg.mozilla.org/mozilla-central/rev/2a64861d0625
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.