Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(820)

Issue 3006343002: Add explicit copy constructors to RTPHeader and RTPHeaderExtension (Closed)

Created:
3 years, 3 months ago by eladalon
Modified:
3 years, 3 months ago
CC:
webrtc-reviews_webrtc.org, interface-changes_webrtc.org, yujie_mao (webrtc), tterriberry_mozilla.com, qiang.lu, niklas.enbom, peah-webrtc
Target Ref:
refs/heads/master
Project:
webrtc
Visibility:
Public.

Description

Add explicit copy constructors to RTPHeader and RTPHeaderExtension Explicit copy-constructors are required by chromium. (No copy constructors were used until now, but a different CL requires them.) BUG=None TBR=stefan@webrtc.org Review-Url: https://codereview.webrtc.org/3006343002 Cr-Commit-Position: refs/heads/master@{#19777} Committed: https://chromium.googlesource.com/external/webrtc/+/98b1b7d59e5b9eab61211d3b13dab87cb3bb18ce

Patch Set 1 #

Total comments: 2

Patch Set 2 : Assignment constructors added. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+14 lines, -0 lines) Patch
M webrtc/common_types.h View 1 2 chunks +4 lines, -0 lines 0 comments Download
M webrtc/common_types.cc View 1 2 chunks +10 lines, -0 lines 0 comments Download

Messages

Total messages: 11 (5 generated)
eladalon
PTAL
3 years, 3 months ago (2017-09-11 13:22:20 UTC) #2
danilchap
RTPHeader already copied sometimes using memcpy explicit copy constructor is better. https://codereview.webrtc.org/3006343002/diff/1/webrtc/common_types.h File webrtc/common_types.h (right): ...
3 years, 3 months ago (2017-09-11 13:34:21 UTC) #3
eladalon
https://codereview.webrtc.org/3006343002/diff/1/webrtc/common_types.h File webrtc/common_types.h (right): https://codereview.webrtc.org/3006343002/diff/1/webrtc/common_types.h#newcode795 webrtc/common_types.h:795: RTPHeaderExtension(const RTPHeaderExtension& other); On 2017/09/11 13:34:21, danilchap wrote: > ...
3 years, 3 months ago (2017-09-11 14:15:04 UTC) #4
danilchap
lgtm
3 years, 3 months ago (2017-09-11 14:20:19 UTC) #5
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/3006343002/20001
3 years, 3 months ago (2017-09-11 15:17:35 UTC) #8
commit-bot: I haz the power
3 years, 3 months ago (2017-09-11 15:48:33 UTC) #11
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/external/webrtc/+/98b1b7d59e5b9eab61211d3b1...

Powered by Google App Engine
This is Rietveld 408576698