1529695 - Implement RTCDataChannel.negotiated

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, enhancement, P2)

Comment 1

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

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

Comment 2

[Fabio Alessandrelli]

Attachment: Expose "RTCDataChannel.negotiated" to Javascript.

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

Comment 3

[Byron Campen [:bwc]]

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...

Comment 4

[Byron Campen [:bwc]]

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.

Comment 5

[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');

Comment 6

[Byron Campen [:bwc]]

(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

Comment 7

[Fabio Alessandrelli]

Attachment: Bug 1529695: Implement RTCDataChannel.negotiated

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

Comment 8

[Byron Campen [:bwc]]

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.

Comment 9

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4a9e43cbc96960066ad3ea1ba2547796ce2613a4

Comment 10

[Byron Campen [:bwc]]

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

Comment 11

[Fabio Alessandrelli]

Attachment: Bug 1529695: Implement RTCDataChannel.negotiated

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

Comment 12

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dcd968bc2db47e973c6f2a85fcc9533d77d9e8be

Comment 13

[Fabio Alessandrelli]

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

Comment 14

[Byron Campen [:bwc]]

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

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3235df251814
Implement RTCDataChannel.negotiated. r=bwc,smaug

Comment 16

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/3235df251814

Comment 17

[Eric Shepherd [:sheppy]]

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