Bug 163944

Summary: IceCandidate does not need to be refcounted
Product: WebKit Reporter: youenn fablet <youennf>
Component: WebCore Misc.Assignee: youenn fablet <youennf>
Status: RESOLVED FIXED    
Severity: Normal CC: adam.bergkvist, commit-queue, eric.carlson, pnormand
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Fixing debug build
none
Patch for landing none

Description youenn fablet 2016-10-25 08:15:09 PDT
IceCandidate does not need to be refcounted
Comment 1 youenn fablet 2016-10-25 08:38:49 PDT
Created attachment 292755 [details]
Patch
Comment 2 youenn fablet 2016-10-25 08:40:05 PDT
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?
Comment 3 youenn fablet 2016-10-25 08:40:50 PDT
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?
Comment 4 youenn fablet 2016-10-25 08:42:44 PDT
(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<>.
Comment 5 youenn fablet 2016-10-25 08:52:16 PDT
Created attachment 292758 [details]
Patch
Comment 6 Adam Bergkvist 2016-10-25 23:46:50 PDT
(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
Comment 7 youenn fablet 2016-10-26 03:31:21 PDT
Created attachment 292913 [details]
Patch
Comment 8 youenn fablet 2016-10-26 04:35:43 PDT
Created attachment 292915 [details]
Fixing debug build
Comment 9 Eric Carlson 2016-10-26 06:06:57 PDT
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)
Comment 10 youenn fablet 2016-10-26 07:17:19 PDT
Created attachment 292926 [details]
Patch for landing
Comment 11 WebKit Commit Bot 2016-10-26 07:52:38 PDT
Comment on attachment 292926 [details]
Patch for landing

Clearing flags on attachment: 292926

Committed r207897: <http://trac.webkit.org/changeset/207897>
Comment 12 WebKit Commit Bot 2016-10-26 07:52:41 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 youenn fablet 2016-10-26 09:14:09 PDT
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
Comment 14 Darin Adler 2016-10-26 17:23:52 PDT
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.
Comment 15 youenn fablet 2016-10-26 22:43:18 PDT
(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.