Closed Bug 1476085 Opened 2 years ago Closed 2 years ago

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

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: johannes.willbold, Assigned: johannes.willbold)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.
Assignee: nobody → johannes.willbold
Status: UNCONFIRMED → NEW
Rank: 15
Ever confirmed: true
Priority: -- → P2
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?
Attachment #8992719 - Flags: review?(dminor) → review+
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.
Attachment #8992719 - Flags: review?(docfaraday) → review+
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.
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)
Pushed by drno@ohlmeier.org:
https://hg.mozilla.org/integration/autoland/rev/e1e741df87f2
Added SDP candidate serialization, r=bwc,dminor
https://hg.mozilla.org/mozilla-central/rev/e1e741df87f2
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Duplicate of this bug: 1474997
You need to log in before you can comment on or make changes to this bug.