RESOLVED FIXED 164059
PeerMediaDescription does not need to be refcounted 
https://bugs.webkit.org/show_bug.cgi?id=164059
Summary PeerMediaDescription does not need to be refcounted
youenn fablet
Reported 2016-10-27 05:08:46 PDT
And it should be a struct probably
Attachments
Patch (46.86 KB, patch)
2016-10-27 05:16 PDT, youenn fablet 		no flags
DetailsFormatted DiffDiff
Patch (46.80 KB, patch)
2016-10-27 05:27 PDT, youenn fablet 		no flags
DetailsFormatted DiffDiff
Patch (51.44 KB, patch)
2016-10-27 08:16 PDT, youenn fablet 		no flags
DetailsFormatted DiffDiff
Patch for landing (51.71 KB, patch)
2016-10-28 03:41 PDT, youenn fablet 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
youenn fablet
Comment 1 2016-10-27 05:16:58 PDT
Created attachment 293013 [details] Patch
youenn fablet
Comment 2 2016-10-27 05:27:53 PDT
Created attachment 293014 [details] Patch
youenn fablet
Comment 3 2016-10-27 08:16:22 PDT
Created attachment 293019 [details] Patch
Darin Adler
Comment 4 2016-10-27 14:31:13 PDT
Comment on attachment 293019 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=293019&action=review > Source/WebCore/Modules/mediastream/MediaEndpointPeerConnection.h:46 > -class PeerMediaDescription; > +struct PeerMediaDescription; We normally put struct forward declarations in a separate paragraph, rather than sorting them in with the classes. > Source/WebCore/Modules/mediastream/MediaEndpointPeerConnection.h:49 > -typedef Vector<RefPtr<PeerMediaDescription>> MediaDescriptionVector; > +typedef Vector<PeerMediaDescription> MediaDescriptionVector; In code we are touching we want to move from typedef to using. > Source/WebCore/platform/mediastream/PeerMediaDescription.h:43 > public: No need for "public:" in a struct. > Source/WebCore/platform/mediastream/PeerMediaDescription.h:52 > + String mode { "sendrecv" }; Slightly nicer to use { ASCIILiteral { "sendrecv" } }. > Source/WebCore/platform/mediastream/openwebrtc/MediaEndpointOwr.h:46 > -class PeerMediaDescription; > +struct PeerMediaDescription; Same comment about putting a struct in a separate paragraph.
youenn fablet
Comment 5 2016-10-28 03:41:49 PDT
Created attachment 293137 [details] Patch for landing
WebKit Commit Bot
Comment 6 2016-10-28 04:17:43 PDT
Comment on attachment 293137 [details] Patch for landing Clearing flags on attachment: 293137 Committed r208043: <http://trac.webkit.org/changeset/208043>
WebKit Commit Bot
Comment 7 2016-10-28 04:17:47 PDT
All reviewed patches have been landed. Closing bug.
youenn fablet
Comment 8 2016-10-28 06:39:38 PDT
Thanks for the review. (In reply to comment #4) > Comment on attachment 293019 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=293019&action=review > > > Source/WebCore/Modules/mediastream/MediaEndpointPeerConnection.h:46 > > -class PeerMediaDescription; > > +struct PeerMediaDescription; > > We normally put struct forward declarations in a separate paragraph, rather > than sorting them in with the classes. OK > > Source/WebCore/Modules/mediastream/MediaEndpointPeerConnection.h:49 > > -typedef Vector<RefPtr<PeerMediaDescription>> MediaDescriptionVector; > > +typedef Vector<PeerMediaDescription> MediaDescriptionVector; > > In code we are touching we want to move from typedef to using. OK > > Source/WebCore/platform/mediastream/PeerMediaDescription.h:43 > > public: > > No need for "public:" in a struct. Done > > Source/WebCore/platform/mediastream/PeerMediaDescription.h:52 > > + String mode { "sendrecv" }; > > Slightly nicer to use { ASCIILiteral { "sendrecv" } }. Done > > Source/WebCore/platform/mediastream/openwebrtc/MediaEndpointOwr.h:46 > > -class PeerMediaDescription; > > +struct PeerMediaDescription; > > Same comment about putting a struct in a separate paragraph. OK
Note You need to log in before you can comment on or make changes to this bug.