1476085 - Add C++/Rust glue code for the SDP attribute candidate

Status: RESOLVED FIXED

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

Description

[Johannes Willbold]

The rust parser already parses the candidate attribute, but the C++/Rust glue code only uses the a string list to store these attributes and passes this list to the ice-stack. So we probably want the rust code to serialize his parsed candidate attribute and pass it to the glue code.

Comment 1

[Johannes Willbold]

Attachment: Bug 1476085: Added SDP candidate serialization,

Added SDP candidate serializaton in rust
Extended Rust unit test for candidates
Added C++/Rust glue code for candidates
Added free_boxed_string_vec

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

Comment 2

[Dan Minor [:dminor]]

Comment on attachment 8992719 [details]
Bug 1476085: Added SDP candidate serialization,

https://reviewboard.mozilla.org/r/257576/#review264736

lgtm. As usual, bwc is the better person to check the correctness of the sdp parsing itself.

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:150
(Diff revision 1)
>  }
>  
> +impl ToString for SdpAttributeCandidate {
> +    fn to_string(&self) -> String {
> +        macro_rules! option_to_string {
> +            ($opt:expr, $fmt_str:expr) => {

Up to you, but I think it would make sense to switch the order here so that the format string comes first, just like with format! itself.

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:1753
(Diff revision 1)
>      assert!(parse_attribute("candidate:0 1 UDP 2122252543 172.16.156 49760 typ host").is_err());
>      assert!(parse_attribute("candidate:0 1 UDP 2122252543 172.16.156.106 70000 typ host").is_err());
>      assert!(parse_attribute("candidate:0 1 UDP 2122252543 172.16.156.106 49760 type host")
>                  .is_err());
>      assert!(parse_attribute("candidate:0 1 UDP 2122252543 172.16.156.106 49760 typ fost").is_err());
>      // FIXME this should fail without the extra 'foobar' at the end

Is this FIXME still valid?

Comment 3

[Byron Campen [:bwc]]

Comment on attachment 8992719 [details]
Bug 1476085: Added SDP candidate serialization,

https://reviewboard.mozilla.org/r/257576/#review265066

The SDP stuff looks fine to me.

Comment 4

[Johannes Willbold]

Comment on attachment 8992719 [details]
Bug 1476085: Added SDP candidate serialization,

https://reviewboard.mozilla.org/r/257576/#review264736

> Up to you, but I think it would make sense to switch the order here so that the format string comes first, just like with format! itself.

Makes sense

> Is this FIXME still valid?

Yes it is, I checked it. I haven't done any changes to the parsing.

Comment 5

[Johannes Willbold]

Comment on attachment 8992719 [details]
Bug 1476085: Added SDP candidate serialization,

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

Comment 6

[Mozilla Autoland]

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 4b64a1afe13da8c7b91646d9734c37322d0b49a6 -d df0874ae21c8: rebasing 473586:4b64a1afe13d "Bug 1476085: Added SDP candidate serialization, r=bwc,dminor" (tip)
merging media/webrtc/signaling/gtest/sdp_unittests.cpp
merging media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp
merging media/webrtc/signaling/src/sdp/RsdparsaSdpInc.h
merging media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs
merging media/webrtc/signaling/src/sdp/rsdparsa_capi/src/attribute.rs
merging media/webrtc/signaling/src/sdp/rsdparsa_capi/src/types.rs
warning: conflicts while merging media/webrtc/signaling/src/sdp/rsdparsa_capi/src/types.rs! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 7

[Johannes Willbold]

Comment on attachment 8992719 [details]
Bug 1476085: Added SDP candidate serialization,

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

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e1e741df87f2
Added SDP candidate serialization, r=bwc,dminor

Comment 9

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/e1e741df87f2