1433529 - Fix TODOs in rsdparsa_capi parse_sdp

Status: RESOLVED FIXED

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

Description

[Dan Minor [:dminor]]

There are a few TODOs in this function that need to be addressed.

Comment 2

[Johannes Willbold]

Attachment: Bug 1433529: Fixed TODOs in parse_sdp,

Changed parse_sdp to use StringView instad of raw pointers
Fixed all TODOs by using the exsting StringView::into trait instead of doing a manual string conversion.

Review commit: https://reviewboard.mozilla.org/r/254050/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/254050/

Comment 3

[Dan Minor [:dminor]]

Comment on attachment 8988927 [details]
Bug 1433529: Fixed TODOs in parse_sdp,

https://reviewboard.mozilla.org/r/254050/#review262510

::: commit-message-a009b:4
(Diff revision 1)
> +Bug 1433529: Fixed TODOs in parse_sdp, r=dminor
> +
> +Changed parse_sdp to use StringView instad of raw pointers
> +Fixed all TODOs by using the exsting StringView::into trait instead of doing a manual string conversion.

typo: s/exsting/existing

::: media/webrtc/signaling/src/sdp/RsdparsaSdpParser.cpp:30
(Diff revision 1)
> -  nsresult rv = parse_sdp(rawString, sdpText.length() + 1, false,
> -                          &result, &err);
> +  StringView sdpTextView{sdpText.c_str(), sdpText.length()};
> +  nsresult rv = parse_sdp(sdpTextView, false, &result, &err);
>    if (rv != NS_OK) {
> -    // TODO: err should eventually never be null if rv != NS_OK
> -    // see Bug 1433529
>      if (err != nullptr) {

With your changes, err should never be null. Please remove the if statement and the else clause.

::: media/webrtc/signaling/src/sdp/rsdparsa_capi/src/lib.rs:42
(Diff revision 1)
>              *session = ptr::null();
> -            *error = ptr::null(); // TODO: Give more useful return value here
> -            debug!("Error converting string to utf8");
> +            *error = Box::into_raw(Box::new(SdpParserError::Sequence {
> +                message: (*boxed_error).description().to_string(),
> +                line_number: 0,
> +            }));
> +            debug!("Error while reading sdp streaing: {}", (*boxed_error).description());

Please remove the debug! statement, I don't think it is necessary if we're returning a proper error message.

Comment 4

[Johannes Willbold]

Comment on attachment 8988927 [details]
Bug 1433529: Fixed TODOs in parse_sdp,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254050/diff/1-2/

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/607e9d3256b0
Fixed TODOs in parse_sdp, r=dminor

Comment 6

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/607e9d3256b0