878945 - Update DataChannel WebIDL to match spec changes (renaming dictionary values)

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

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

The spec for the RTCDataChannelInit dictionary changed once Microsoft and Google actually paid attention and made their preferences known (and people actually talked about the values).  The result was renaming of 4 entries, and one of them swapped meaning in the rename (outOfOrderAllowed -> ordered=true).

We need to change our usage to match.  It would be good to support the existing names for a short time (fx22 basically) to avoid immediately breaking everyone.  We'd throw warnings on use of deprecated names.

Comment 1

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

If we *need* to, we can release without naming changes, but that will force developers to sniff UA versions since dictionaries aren't easily sniffed directly.

Comment 2

[Jason Smith [:jsmith]]

Sounds like we should push to get this implemented asap. Tracking for 23 and 24 at least. Might be too late for 22, but we'll see.

Comment 3

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

Attachment: rename RTCDataChannelInit dictionary items to match updated spec

tested this with webrtc-landing/data_test.html; warnings appear on the web console

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

It might be useful to mention the replacements in the warnings.

Comment 5

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

Comment on attachment 757553 [details] [diff] [review]
rename RTCDataChannelInit dictionary items to match updated spec

Looking for a fast review to try to get into Beta 4 which cuts tomorrow EOD in california

Comment 6

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

Comment on attachment 757553 [details] [diff] [review]
rename RTCDataChannelInit dictionary items to match updated spec

But I don't think we have this .webidl in beta

Comment 7

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

Attachment: rename DataChannel dict items (beta patch, no warnings)

beta version of the patch (no webidl, warning function isn't available

Comment 8

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

Attachment: rename DataChannel dict items (beta patch, no warnings)

Noticed we expose 'stream' as an attribute; now should be named 'id'.  Tested (on m-c patch and beta patch)

Comment 9

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

Attachment: rename RTCDataChannelInit dictionary items to match updated spec

Comment 10

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

Comment on attachment 757601 [details] [diff] [review]
rename DataChannel dict items (beta patch, no warnings)

We can't change interfaces in beta. You could add a new interface which has 
.id and implement it and expose in classinfo.

Comment 11

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

Comment on attachment 757603 [details] [diff] [review]
rename RTCDataChannelInit dictionary items to match updated spec

update uuid

Comment 12

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

Attachment: rename RTCDataChannelInit dictionary items to match updated spec

updated UUID

Comment 13

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

Attachment: rename DataChannel dict items (beta patch, no warnings)

Comment 14

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

Comment on attachment 757612 [details] [diff] [review]
rename RTCDataChannelInit dictionary items to match updated spec

[Approval Request Comment] (for Aurora and Beta patches) 

Bug caused by (feature/regressing bug #): N/A (spec change)

User impact if declined: Apps are likely to be forced to UA/version sniff, which is bad.  We really don't want to cause them to do that

Testing completed (on m-c, etc.): Waiting to land on inbound (closure).  Patch tested locally on inbound, aurora and beta with custom test to verify the backwards-compatibility.

Risk to taking this patch (and alternatives if risky): No risk other than changing the UUID

String or IDL/UUID changes made by this patch: DataChannel UUID changes.  We believe strongly no-one is using DataChannels from a binary extension, especially with the spec still in a flux - and if they are, they should track the release process as it's not in release yet.

smaug is r+ other than the UUID issue

Comment 15

[Lukas Blakk [:lsblakk] use ?needinfo]

Tracking, waiting for landing to central to approve for Aurora -  Alex is running FF23 and so will make the call on uplifting the UUID change in beta but is there a reason to rush this to beta? If no one is using DataChannel yet couldn't this ride from 23?

Comment 16

[Alex Keybl [:akeybl]]

Spoke with rjesup - we'll take this change (ba=akeybl) once it's on m-c given no known clients. We want this in FF22 since people will start targeting this interface in that version.

Comment 17

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/d784336f86e9

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d784336f86e9

Comment 19

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

https://hg.mozilla.org/releases/mozilla-beta/rev/0ed8a89f8484
Aurora is waiting on tree closure

Comment 20

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/e67923f89166

Comment 21

[Mihaela Velimiroviciu (:mihaelav)]

Can you please provide some guidelines about how QA can manually verify this?

Comment 22

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

Verify that the new names work, and that the old names work (and on 24 throw a warning in the web console).

Modifying the tests at mozilla.github.com/webrtc-landing/data_test.html is the easiest way.