Implement RsdparsaSdp::AddMediaSection method

RESOLVED FIXED in Firefox 62

Status

()

P3
normal
RESOLVED FIXED
a year ago
9 months ago

People

(Reporter: dminor, Assigned: johannes.willbold)

Tracking

(Blocks: 1 bug)

58 Branch
mozilla62
Points:
---

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(1 attachment)

Comment hidden (empty)
Assignee: nobody → johannes.willbold
Comment hidden (mozreview-request)
(Reporter)

Comment 2

9 months ago
mozreview-review
Comment on attachment 8986517 [details]
Bug 1436080: Implemented RsdparsaSdp::AddMediaSection,

https://reviewboard.mozilla.org/r/251842/#review258296

lgtm!

::: media/webrtc/signaling/gtest/sdp_unittests.cpp:3995
(Diff revision 1)
> +  ASSERT_EQ(3U, mSdp->GetMediaSectionCount())
> +    << "Wrong number of media sections";
> +
> +  mSdp->AddMediaSection(SdpMediaSection::kVideo,
> +                        SdpDirectionAttribute::Direction::kSendrecv,
> +                        58000,SdpMediaSection::kUdpDtlsSctp,sdp::kIPv4,

nit: here and below please make sure you have a space after each comma in an argument list.

::: media/webrtc/signaling/gtest/sdp_unittests.cpp:4029
(Diff revision 1)
> +            nextNewMediaSection.GetDirectionAttribute().mValue);
> +  ASSERT_EQ(14006U,nextNewMediaSection.GetPort());
> +  ASSERT_EQ(SdpMediaSection::kTcpTlsRtpSavpf,nextNewMediaSection.GetProtocol());
> +  ASSERT_EQ(sdp::kIPv6,nextNewMediaSection.GetConnection().GetAddrType());
> +  ASSERT_EQ("2607:f8b0:4004:801::2013",
> +            nextNewMediaSection.GetConnection().GetAddress());

Please add some tests for invalid values to make sure the error is handled properly.

::: media/webrtc/signaling/src/sdp/rsdparsa/src/lib.rs:21
(Diff revision 1)
>  pub mod network;
>  pub mod unsupported_types;
>  
>  use attribute_type::{SdpAttribute, SdpAttributeType, parse_attribute};
>  use error::{SdpParserInternalError, SdpParserError};
> -use media_type::{SdpMedia, SdpMediaLine, parse_media, parse_media_vector};
> +use media_type::*;

Please list the types you use rather than using *.

::: media/webrtc/signaling/src/sdp/rsdparsa/src/lib.rs:188
(Diff revision 1)
>      pub fn has_media(&self) -> bool {
>          !self.media.is_empty()
>      }
> +
> +    pub fn add_media(&mut self,media_type: SdpMediaValue, direction: SdpAttribute, port: u32,
> +                              protocol: SdpProtocolValue, addr: String)

nits: add whitespace after "self,", fix up indent

::: media/webrtc/signaling/src/sdp/rsdparsa_capi/src/lib.rs:150
(Diff revision 1)
> +pub unsafe extern "C" fn sdp_add_media_section(session: *mut SdpSession,
> +                                               media_type: u32, direction: u32,
> +                                               port: u16, protocol: u32,
> +                                               addr_type: u32, addr: StringView) -> nsresult {
> +
> +    let addr_str:String = match addr.into(){

nit: space before {

::: media/webrtc/signaling/src/sdp/rsdparsa_capi/src/lib.rs:158
(Diff revision 1)
> +           println!("Error while pasing string, description: {:?}", (*boxed_error).description());
> +           return NS_ERROR_INVALID_ARG;
> +       }
> +    };
> +
> +    let media_type = match media_type{

nit: space before {

::: media/webrtc/signaling/src/sdp/rsdparsa_capi/src/lib.rs:190
(Diff revision 1)
> +
> +    // Check that the provided address type is valid, the rust parser will find out
> +    // on his own which address type was provided
> +    match addr_type {
> +      // enum AddrType { kAddrTypeNone, kIPv4, kIPv6 };
> +      1...2 => (),

Please add a comment explaining why you exclude kAddrTypeNone here. My first thought is that if it is a valid enumerated value, we should accept it.

::: media/webrtc/signaling/src/sdp/rsdparsa_capi/src/lib.rs:196
(Diff revision 1)
> +      _ => {
> +          return NS_ERROR_INVALID_ARG;
> +      }
> +    }
> +
> +    match (*session).add_media(media_type,direction,port as u32,protocol,addr_str) {

nit: please add spaces after commas
Attachment #8986517 - Flags: review?(dminor) → review+
(Assignee)

Comment 3

9 months ago
mozreview-review-reply
Comment on attachment 8986517 [details]
Bug 1436080: Implemented RsdparsaSdp::AddMediaSection,

https://reviewboard.mozilla.org/r/251842/#review258296

> Please list the types you use rather than using *.

Fixed it. Although I think that this is very time consuming, as these lines have high merge conflict potential.

> Please add a comment explaining why you exclude kAddrTypeNone here. My first thought is that if it is a valid enumerated value, we should accept it.

I assumed that kAddrTypeNone is basically an 'invalid' flag. My asumption is based on this line: https://searchfox.org/mozilla-central/source/media/webrtc/signaling/src/sdp/SdpEnum.h#39
Comment hidden (mozreview-request)

Comment 5

9 months ago
Pushed by drno@ohlmeier.org:
https://hg.mozilla.org/integration/autoland/rev/301f7d8a5ba2
Implemented RsdparsaSdp::AddMediaSection, r=dminor

Comment 6

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/301f7d8a5ba2
Status: NEW → RESOLVED
Last Resolved: 9 months ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.