Implement RTCDataChannel.negotiated

RESOLVED FIXED in Firefox 68

Status

()

enhancement
P2
normal
RESOLVED FIXED
5 months ago
Last month

People

(Reporter: bwc, Assigned: fabio.alessandrelli)

Tracking

(Blocks 1 bug, {dev-doc-complete})

unspecified
mozilla68
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox68 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

No description provided.

Please adjust priority as needed, as I'm unfamiliar with this work.

Priority: -- → P2
Blocks: 1533020

It seems pretty trivial, I hope I didn't neglect anything, it's my first contribution.

Comment on attachment 9061804 [details] [diff] [review]
Expose "RTCDataChannel.negotiated" to Javascript.

Review of attachment 9061804 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch!

::: netwerk/sctp/datachannel/DataChannel.h
@@ +423,5 @@
>  
>    dom::Nullable<uint16_t> GetMaxRetransmits() const;
>  
> +  bool GetNegotiated() {
> +    return mFlags & DATA_CHANNEL_FLAGS_EXTERNAL_NEGOTIATED;

This does not look threadsafe, but then again neither does GetOrdered! I think the threadsafety problems with mFlags need to be fixed first. I have filed bug 1548272 for this cleanup. Stand by...
Attachment #9061804 - Flags: review-
Depends on: 1548272

Once bug 1548272 is fixed, you can take another try at this using GetOrdered as an example. However, bear in mind that there are two test-cases that we currently expect to fail because of this bug:

https://searchfox.org/mozilla-central/rev/f0ef51bfafc8ad0c3a2f523bf076edc57dc4891a/testing/web-platform/meta/webrtc/RTCPeerConnection-createDataChannel.html.ini#2-4

and

https://searchfox.org/mozilla-central/rev/f0ef51bfafc8ad0c3a2f523bf076edc57dc4891a/testing/web-platform/meta/webrtc/RTCPeerConnection-createDataChannel.html.ini#10-12

Once your patch is applied, we expect that these test-cases should start working; if so, you can simply remove the bits I highlighted above in testing/web-platform/meta/webrtc/RTCPeerConnection-createDataChannel.html.ini. You can run this test like this:

./mach wpt testing/web-platform/tests/webrtc/RTCPeerConnection-createDataChannel.html

Grep the output for "Unexpected", and that should tell you if everything is still ok.

If one of these test-cases is still failing, then it becomes more complex; I will probably need to figure out whether there is a flaw in the test-case, or if there's some other bug that is causing the test-case to fail, and update the test or .ini file (and maybe file new bugs) as needed.

Assignee: nobody → fabio.alessandrelli
Flags: needinfo?(fabio.alessandrelli)

(In reply to Byron Campen [:bwc] from comment #4)

./mach wpt testing/web-platform/tests/webrtc/RTCPeerConnection-createDataChannel.html

Grep the output for "Unexpected", and that should tell you if everything is still ok.

If one of these test-cases is still failing, then it becomes more complex; I will probably need to figure out whether there is a flaw in the test-case, or if there's some other bug that is causing the test-case to fail, and update the test or .ini file (and maybe file new bugs) as needed.

[createDataChannel with negotiated true should succeed] succedes.

[createDataChannel attribute default values] still fails, but with another error:

assert_equals: expected (string) "low" but got (undefined) undefined
Due to another missing property (priority): assert_equals(channel.priority, 'low');

Flags: needinfo?(fabio.alessandrelli)

(In reply to Fabio Alessandrelli from comment #5)

(In reply to Byron Campen [:bwc] from comment #4)

./mach wpt testing/web-platform/tests/webrtc/RTCPeerConnection-createDataChannel.html

Grep the output for "Unexpected", and that should tell you if everything is still ok.

If one of these test-cases is still failing, then it becomes more complex; I will probably need to figure out whether there is a flaw in the test-case, or if there's some other bug that is causing the test-case to fail, and update the test or .ini file (and maybe file new bugs) as needed.

[createDataChannel with negotiated true should succeed] succedes.

[createDataChannel attribute default values] still fails, but with another error:

assert_equals: expected (string) "low" but got (undefined) undefined
Due to another missing property (priority): assert_equals(channel.priority, 'low');

Ok, so that's bug 1531100, so for that chunk in the .ini file you would change the bug link to https://bugzilla.mozilla.org/show_bug.cgi?id=1531100

Updated patch, reads values from ctor, update tests ini file.

Comment on attachment 9063972 [details] [diff] [review]
Bug 1529695: Implement RTCDataChannel.negotiated

Review of attachment 9063972 [details] [diff] [review]:
-----------------------------------------------------------------

Ok, this all looks good. Since this work involves updating the webidl file, you will also need to ask for review from a DOM peer. I usually ask smaug. In the meantime, I will push this to the try servers for you.
Attachment #9063972 - Flags: review+
Attachment #9063972 - Attachment description: negotiated_updated.diff → Bug 1529695: Implement RTCDataChannel.negotiated
Attachment #9061804 - Attachment is obsolete: true

It looks like some additional web-platform-tests stuff needs to be marked as passing now.

Flags: needinfo?(docfaraday)
Flags: needinfo?(docfaraday) → needinfo?(fabio.alessandrelli)
Attachment #9063972 - Flags: feedback?(bugs) → review+

Tested on whole testing/web-platform/tests/webrtc/ folder.
Removed 2 expected failures which now succeeds.

Attachment #9063972 - Attachment is obsolete: true
Flags: needinfo?(fabio.alessandrelli)

Build seems okay, do I need to take any further action?

Flags: needinfo?(docfaraday)

I think you should be good to go. Marking checkin-needed.

Flags: needinfo?(docfaraday)
Keywords: checkin-needed
Keywords: checkin-needed
Status: NEW → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68

The following changes have been made to documentation:

  • Added text and code snippets to the Creating a data channel section in the "Using data channels" guide; this describes both automatic and manual negotiation
  • Submitted BCD PR 4324 adding negotiated to RTCDataChannel in Firefox 68
  • Updated negotiated page with new information and a code sample
You need to log in before you can comment on or make changes to this bug.