Closed Bug 1438290 Opened 6 years ago Closed 6 years ago

Implement RsdparsaSdpMediaSection::AddDataChannel

Categories

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

Product:
Core
Component:
WebRTC: Signaling
Version:
60 Branch
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: dminor, Assigned: johannes.willbold)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Bug 1438290: Part 1: Implemented RsdparsaSdpMediaSection::AddDataChannel,
59 bytes, text/x-review-board-request
dminor
: review+
Details
Bug 1438290: Part 2: Added testcases for AddDataChannel,
59 bytes, text/x-review-board-request
bwc
: review+
Details 
      No description provided.
Comment on attachment 8986945 [details]
Bug 1438290: Part 1: Implemented RsdparsaSdpMediaSection::AddDataChannel,

https://reviewboard.mozilla.org/r/252172/#review258870

lgtm

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp:1080
(Diff revision 1)
>  
>  void
> +RsdparsaSdpAttributeList::LoadSctpPort(RustAttributeList* attributeList)
> +{
> +  int64_t port = sdp_get_sctp_port(attributeList);
> +  if (port >= 0) {

Might as well check that port is <= 65535.
Attachment #8986945 - Flags: review?(dminor) → review+
Comment on attachment 8986945 [details]
Bug 1438290: Part 1: Implemented RsdparsaSdpMediaSection::AddDataChannel,

https://reviewboard.mozilla.org/r/252172/#review258870

> Might as well check that port is <= 65535.

Actually this is not a validity check for the port but a check whether the rust call was successfull. In case the rust lookup fails, it will return -1. 
In addition, the rust parser is checking that already.
Comment on attachment 8986945 [details]
Bug 1438290: Part 1: Implemented RsdparsaSdpMediaSection::AddDataChannel,

https://reviewboard.mozilla.org/r/252172/#review259274

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp:817
(Diff revision 1)
>  }
>  
>  void
> +RsdparsaSdpAttributeList::LoadMaxMessageSize(RustAttributeList* attributeList)
> +{
> +  int64_t max_msg_size = sdp_get_max_msg_sizse(attributeList);

s/sizse/size/
Comment on attachment 8986946 [details]
Bug 1438290: Part 2: Added testcases for AddDataChannel,

https://reviewboard.mozilla.org/r/252174/#review259304

::: media/webrtc/signaling/gtest/sdp_unittests.cpp:3988
(Diff revision 1)
> +  ParseSdp("v=0" CRLF
> +           "o=- 4294967296 2 IN IP4 127.0.0.1" CRLF
> +           "s=SIP Call" CRLF
> +           "c=IN IP4 198.51.100.7" CRLF
> +           "b=CT:5000" CRLF
> +           "t=0 0" CRLF
> +           "m=video 56436 RTP/SAVPF 120 110" CRLF
> +           "a=rtpmap:120 VP8/90000" CRLF
> +           "a=sendonly" CRLF
> +           "a=rtpmap:110 opus/48000/2" CRLF);
> +
> +  ASSERT_TRUE(!!mSdp) << "Parse failed: " << GetParseErrors();
> +  ASSERT_EQ(1U, mSdp->GetMediaSectionCount());

Hmm. I would have expected this test case to involve building the m-section with AddMediaSection instead of parsing it, but ok.

In this case, let's make this SDP identical to the one below, except substitute "DTLS/SCTP" for "UDP/DTLS/SCTP". Calling AddDataChannel on anything other than m=application probably should MOZ_CRASH, honestly.
Attachment #8986946 - Flags: review?(docfaraday) → review+
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 0dae344aee446d53b4866a035db72094fee6157e -d 02a46b307edd: rebasing 469965:0dae344aee44 "Bug 1438290: Part 1: Implemented RsdparsaSdpMediaSection::AddDataChannel, r=dminor"
merging media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp
merging media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.h
merging media/webrtc/signaling/src/sdp/RsdparsaSdpInc.h
merging media/webrtc/signaling/src/sdp/rsdparsa/src/media_type.rs
merging media/webrtc/signaling/src/sdp/rsdparsa_capi/src/attribute.rs
warning: conflicts while merging media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.h! (edit, then use 'hg resolve --mark')
warning: conflicts while merging media/webrtc/signaling/src/sdp/rsdparsa/src/media_type.rs! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by drno@ohlmeier.org:
https://hg.mozilla.org/integration/autoland/rev/d41ad8a48d45
Part 1: Implemented RsdparsaSdpMediaSection::AddDataChannel, r=dminor
https://hg.mozilla.org/integration/autoland/rev/afd967c185fe
Part 2: Added testcases for AddDataChannel, r=bwc
https://hg.mozilla.org/mozilla-central/rev/d41ad8a48d45
https://hg.mozilla.org/mozilla-central/rev/afd967c185fe
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Assignee: nobody → johannes.willbold
status-firefox60: affected → ---
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: