Closed Bug 1433529 Opened 2 years ago Closed Last year

Fix TODOs in rsdparsa_capi parse_sdp

Categories

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

58 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: dminor, Assigned: johannes.willbold)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

There are a few TODOs in this function that need to be addressed.
Duplicate of this bug: 1438324
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.
Attachment #8988927 - Flags: review?(dminor) → review+
https://hg.mozilla.org/mozilla-central/rev/607e9d3256b0
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Assignee: nobody → johannes.willbold
You need to log in before you can comment on or make changes to this bug.