IceCandidate does not need to be refcounted
Created attachment 292755 [details] Patch
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?
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?
(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<>.
Created attachment 292758 [details] Patch
(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
Created attachment 292913 [details] Patch
Created attachment 292915 [details] Fixing debug build
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)
Created attachment 292926 [details] Patch for landing
Comment on attachment 292926 [details] Patch for landing Clearing flags on attachment: 292926 Committed r207897: <http://trac.webkit.org/changeset/207897>
All reviewed patches have been landed. Closing bug.
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 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.
(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.