Bug 163944 – IceCandidate does not need to be refcounted

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[youenn fablet]

IceCandidate does not need to be refcounted

[youenn fablet]

Created attachment 292755 [details]
Patch

[youenn fablet]

Comment on attachment 292755 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=292755&action=review

> Source/WebCore/Modules/mediastream/MediaEndpointPeerConnection.cpp:689
> +    targetMediaDescription->addIceCandidate(IceCandidate(result.candidate()));

Can we do that now?

> Source/WebCore/platform/mediastream/IceCandidate.h:44
> +    int priority { 0 };

Should it be an unsigned/unsigned long instead?

[youenn fablet]

Adam, do you know the answers for these?

(In reply to comment #2)
> Comment on attachment 292755 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=292755&action=review
> 
> > Source/WebCore/Modules/mediastream/MediaEndpointPeerConnection.cpp:689
> > +    targetMediaDescription->addIceCandidate(IceCandidate(result.candidate()));
> 
> Can we do that now?
> 
> > Source/WebCore/platform/mediastream/IceCandidate.h:44
> > +    int priority { 0 };
> 
> Should it be an unsigned/unsigned long instead?

[youenn fablet]

(In reply to comment #0)
> IceCandidate does not need to be refcounted

In general, I think all struct-like classes in platform/mediastream probably do not need to be refcounted and should be converted to struct.
This might also allow removing some uncertainty about null pointers triggered by using RefPtr<>.

[youenn fablet]

Created attachment 292758 [details]
Patch

[Adam Bergkvist]

(In reply to comment #3)
> Adam, do you know the answers for these?
> 
> (In reply to comment #2)
> > Comment on attachment 292755 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=292755&action=review
> > 
> > > Source/WebCore/Modules/mediastream/MediaEndpointPeerConnection.cpp:689
> > > +    targetMediaDescription->addIceCandidate(IceCandidate(result.candidate()));
> > 
> > Can we do that now?

That should be safe.

> > > Source/WebCore/platform/mediastream/IceCandidate.h:44
> > > +    int priority { 0 };
> > 
> > Should it be an unsigned/unsigned long instead?

Yes, it should be a positive integer. See [1].

[1] https://tools.ietf.org/html/rfc5245#section-4.1.2

[youenn fablet]

Created attachment 292913 [details]
Patch

[youenn fablet]

Created attachment 292915 [details]
Fixing debug build

[Eric Carlson]

Comment on attachment 292915 [details]
Fixing debug build

View in context: https://bugs.webkit.org/attachment.cgi?id=292915&action=review

> Source/WebCore/Modules/mediastream/MediaEndpointPeerConnection.cpp:852
> +    SDPProcessor::Result result = m_sdpProcessor->generateCandidateLine(candidate, candidateLine);

Nit: can you use "auto" here?

> Source/WebCore/Modules/mediastream/MediaEndpointPeerConnection.cpp:862
>      RefPtr<RTCIceCandidate> iceCandidate = RTCIceCandidate::create(candidateLine, mid, mediaDescriptionIndex);

Ditto (as long as you are editing this method)

[youenn fablet]

Created attachment 292926 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 292926 [details]
Patch for landing

Clearing flags on attachment: 292926

Committed r207897: <http://trac.webkit.org/changeset/207897>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[youenn fablet]

Thanks for the review.

> > Source/WebCore/Modules/mediastream/MediaEndpointPeerConnection.cpp:852
> > +    SDPProcessor::Result result = m_sdpProcessor->generateCandidateLine(candidate, candidateLine);
> 
> Nit: can you use "auto" here?

Done

> > Source/WebCore/Modules/mediastream/MediaEndpointPeerConnection.cpp:862
> >      RefPtr<RTCIceCandidate> iceCandidate = RTCIceCandidate::create(candidateLine, mid, mediaDescriptionIndex);
> 
> Ditto (as long as you are editing this method)

Done

[Darin Adler]

Comment on attachment 292926 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=292926&action=review

> Source/WebCore/platform/mediastream/IceCandidate.h:63
> +    IceCandidate() = default;
> +    IceCandidate(String&& type, String&& foundation, unsigned componentId, String&& transport, unsigned long priority, String&& address, unsigned port, String&& tcpType, String&& relatedAddress, unsigned relatedPort)
> +        : type(WTFMove(type))
> +        , foundation(WTFMove(foundation))
> +        , componentId(componentId)
> +        , transport(WTFMove(transport))
> +        , priority(priority)
> +        , address(WTFMove(address))
> +        , port(port)
> +        , tcpType(WTFMove(tcpType))
> +        , relatedAddress(WTFMove(relatedAddress))
> +        , relatedPort(relatedPort)
> +    { }

Can we leave out both of these? Aggregate initialization syntax with { } should work without either of these constructors.

[youenn fablet]

(In reply to comment #14)
> Comment on attachment 292926 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=292926&action=review
> 
> > Source/WebCore/platform/mediastream/IceCandidate.h:63
> > +    IceCandidate() = default;
> > +    IceCandidate(String&& type, String&& foundation, unsigned componentId, String&& transport, unsigned long priority, String&& address, unsigned port, String&& tcpType, String&& relatedAddress, unsigned relatedPort)
> > +        : type(WTFMove(type))
> > +        , foundation(WTFMove(foundation))
> > +        , componentId(componentId)
> > +        , transport(WTFMove(transport))
> > +        , priority(priority)
> > +        , address(WTFMove(address))
> > +        , port(port)
> > +        , tcpType(WTFMove(tcpType))
> > +        , relatedAddress(WTFMove(relatedAddress))
> > +        , relatedPort(relatedPort)
> > +    { }
> 
> Can we leave out both of these? Aggregate initialization syntax with { }
> should work without either of these constructors.

I had to add them to make GTK bot happy.
Probably I should add a FIXME.