New dictionary attributes for RTCDataChannelInit added in bug 878945 have no effect

RESOLVED FIXED in Firefox 23
(NeedInfo fromanyone)

Status

()

Core
WebRTC
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: standard8, Assigned: jesup, NeedInfo)

Tracking

unspecified
mozilla25
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox22 wontfix, firefox23+ fixed, firefox24+ fixed)

Details

(Whiteboard: [WebRTC],[blocking-webrtc-])

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
Attempting to join a non-negotiated data channel on both sides of a connection with:

this.dc = this.pc.createDataChannel('dc', {id:5, negotiated:false});

has no effect. If I change the code to:

this.dc = this.pc.createDataChannel('dc', {stream:5, negotiated:false});

Then this works.

I also verified this with a similar change on http://mozilla.github.io/webrtc-landing/data_test.html

The issue is that the code was changed to warn about the obsolete parameters, but it then wasn't updated to use those new parameters:

https://hg.mozilla.org/mozilla-central/rev/d784336f86e9#l4.7 shows the change

http://hg.mozilla.org/mozilla-central/annotate/82fafd8ee40f/dom/media/PeerConnection.js#l876 shows the code is still using the old stream option for dict, rather than the id option.

The other renamed parameters are also not used, but use the old options: ordered, maxRetransmits, negotiated.
(Assignee)

Comment 1

5 years ago
Created attachment 774682 [details] [diff] [review]
Actually use the new names for createDataChannel

Ooops - had the wrong patch at the top of me queue.  tested with a modified version of data_test.html
(Assignee)

Comment 2

5 years ago
Created attachment 774686 [details]
Modified data_test.html
(Assignee)

Comment 3

5 years ago
This highlights the need for tests to cover DataChannels better; when this patch was done the DataChannel test framework had not landed yet.
(Assignee)

Updated

5 years ago
Blocks: 893021
(Assignee)

Updated

5 years ago
Whiteboard: [WebRTC],[blocking-webrtc-][webrtc-uplift]
(Assignee)

Updated

5 years ago
Attachment #774682 - Flags: review?(bugs)
(Assignee)

Updated

5 years ago
status-firefox22: --- → wontfix
status-firefox23: --- → affected
status-firefox24: --- → affected
tracking-firefox23: --- → ?
tracking-firefox24: --- → ?

Comment 4

5 years ago
Comment on attachment 774682 [details] [diff] [review]
Actually use the new names for createDataChannel

And we really need mochitests for this stuff.
Attachment #774682 - Flags: review?(bugs) → review+
(Assignee)

Comment 5

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d0bb07cb10e

Now we need to get tests in place!
Target Milestone: --- → mozilla25
https://hg.mozilla.org/mozilla-central/rev/0d0bb07cb10e
Assignee: nobody → rjesup
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Can you please explain the user benefit/impact of tracking this for release ?
(Assignee)

Comment 8

5 years ago
This strongly affects spec interoperability and will affect if an app someone develops is backwards-compatible with 23.  I hope to land tests for this shortly, and then will nominate for uplift.
tracking-firefox23: ? → +
tracking-firefox24: ? → +
(Assignee)

Comment 9

5 years ago
Comment on attachment 774682 [details] [diff] [review]
Actually use the new names for createDataChannel

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 878945 - fix was incomplete

User impact if declined: Interoperabilty issues with Chrome, developer confusion and polyfills (and browser/version sniffing) needed.  developers can't directly test if a dictionary item is implemented; they have to test indirectly at best.

Testing completed (on m-c, etc.): On m-c.  Tests have landed on inbound (bug 893021).  They depend on a testing framework that isn't on Beta.  Also tested via http://mozilla.github.com/webrtc-landing/data_test.html

Risk to taking this patch (and alternatives if risky): extremely low

String or IDL/UUID changes made by this patch: none
Attachment #774682 - Flags: approval-mozilla-beta?
Attachment #774682 - Flags: approval-mozilla-aurora?
Attachment #774682 - Flags: approval-mozilla-beta?
Attachment #774682 - Flags: approval-mozilla-beta+
Attachment #774682 - Flags: approval-mozilla-aurora?
Attachment #774682 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 10

5 years ago
https://hg.mozilla.org/releases/mozilla-beta/rev/41ef4f85333c

https://hg.mozilla.org/releases/mozilla-aurora/rev/fdcc8e1ea638
status-firefox23: affected → fixed
status-firefox24: affected → fixed
Whiteboard: [WebRTC],[blocking-webrtc-][webrtc-uplift] → [WebRTC],[blocking-webrtc-]
Can you please provide some guidelines about how QA can manually verify this?

I loaded the data test from http://mozilla.github.com/webrtc-landing/data_test.html and I introduced values on each field pc1 says, pc1 sends a blob etc. and hit Start!
and I get: 
"pc2 got remote stream from pc1 video

pc1 got remote stream from pc2 video

HIP HIP HOORAY

pc1 onConnection

pc1 onopen fired for [object DataChannel]

pc2 onConnection

pc1 onopen fired for [object DataChannel]

pc2 onDataChannel [2] = [object DataChannel], label='This is pc1', protocol=''

pc1 onDataChannel [3] = [object DataChannel], label='This is pc2', protocol=''

*** pc2 onopen fired, sending to [object DataChannel]

pc1 onopen fired for [object DataChannel]

pc1 state: undefined

pc2 said: pc2 says Hello...

pc1 said: pc2 says Hi there!

pc1 said: pc1 says Hello out there..."
Flags: needinfo?
You need to log in before you can comment on or make changes to this bug.