Bug 164059

Summary:

PeerMediaDescription 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:

commit-queue, eric.carlson, pnormand

Priority:

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch for landing	none

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	Details Formatted Diff Diff
Patch (46.80 KB, patch) 2016-10-27 05:27 PDT, youenn fablet	no flags	Details Formatted Diff Diff
Patch (51.44 KB, patch) 2016-10-27 08:16 PDT, youenn fablet	no flags	Details Formatted Diff Diff
Patch for landing (51.71 KB, patch) 2016-10-28 03:41 PDT, youenn fablet	no flags	Details Formatted Diff Diff
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.