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

Issue 2468093004: WebRTC: Fix and enable -Woverloaded-virtual warnings. (Closed)

Created:
4 years, 1 month ago by kjellander_webrtc
Modified:
4 years, 1 month ago
CC:
webrtc-reviews_webrtc.org, zhengzhonghou_agora.io, tterriberry_mozilla.com, sdk-team_agora.io, the sun, perkj_webrtc, mflodman
Target Ref:
refs/pending/heads/master
Project:
webrtc
Visibility:
Public.

Description

WebRTC: Fix and enable -Woverloaded-virtual warnings. Essentially applying the same change as in https://codereview.webrtc.org/2023413002 in more locations. There's only one change affecting production code: enabling the warning for webrtc/media:rtc_media. The rest are test changes. With these changes, the only place the warning is disabled is in the Windows implementation of webrtc/modules/video_capture:video_capture_internal_impl, which is harder to fix, since it relies on sample code from the Windows SDK. BUG=webrtc:6653 NOTRY=True Committed: https://crrev.com/71a1b61c4f1b9afe769e4b0d448008aa3fa322e4 Cr-Commit-Position: refs/heads/master@{#14938}

Patch Set 1 #

Patch Set 2 : Fixed errors and reverted video capture changes #

Unified diffs Side-by-side diffs Delta from patch set Stats (+28 lines, -6 lines) Patch
M webrtc/api/BUILD.gn View 1 chunk +0 lines, -1 line 0 comments Download
M webrtc/api/peerconnection_unittest.cc View 1 1 chunk +7 lines, -0 lines 0 comments Download
M webrtc/api/peerconnectionfactory_unittest.cc View 1 1 chunk +7 lines, -0 lines 0 comments Download
M webrtc/api/peerconnectioninterface_unittest.cc View 1 1 chunk +7 lines, -0 lines 0 comments Download
M webrtc/api/test/peerconnectiontestwrapper.h View 1 1 chunk +7 lines, -0 lines 0 comments Download
M webrtc/media/BUILD.gn View 2 chunks +0 lines, -2 lines 0 comments Download
M webrtc/media/media.gyp View 1 chunk +0 lines, -3 lines 0 comments Download

Messages

Total messages: 29 (15 generated)
kjellander_webrtc
This doesn't change any of the production code though, but it enables the warning for ...
4 years, 1 month ago (2016-11-03 07:40:22 UTC) #9
phoglund
It's not going to fix anything, since the error was with peerconnection_jni.cc. I guess there ...
4 years, 1 month ago (2016-11-03 10:24:37 UTC) #11
phoglund
I don't see how this prevents the error. The error was with peerconnection_jni.cc which is ...
4 years, 1 month ago (2016-11-03 10:28:42 UTC) #12
phoglund
On 2016/11/03 10:28:42, phoglund wrote: > I don't see how this prevents the error. The ...
4 years, 1 month ago (2016-11-03 10:36:05 UTC) #13
kjellander_webrtc
On 2016/11/03 10:24:37, phoglund wrote: > It's not going to fix anything, since the error ...
4 years, 1 month ago (2016-11-03 10:52:53 UTC) #14
kjellander_webrtc
On 2016/11/03 10:36:05, phoglund wrote: > On 2016/11/03 10:28:42, phoglund wrote: > > I don't ...
4 years, 1 month ago (2016-11-03 10:58:15 UTC) #15
phoglund
On 2016/11/03 10:58:15, kjellander_webrtc wrote: > On 2016/11/03 10:36:05, phoglund wrote: > > On 2016/11/03 ...
4 years, 1 month ago (2016-11-03 11:59:34 UTC) #16
kjellander_webrtc
On 2016/11/03 11:59:34, phoglund wrote: > On 2016/11/03 10:58:15, kjellander_webrtc wrote: > > On 2016/11/03 ...
4 years, 1 month ago (2016-11-03 12:26:15 UTC) #17
phoglund
On 2016/11/03 12:26:15, kjellander_webrtc wrote: > On 2016/11/03 11:59:34, phoglund wrote: > > On 2016/11/03 ...
4 years, 1 month ago (2016-11-03 12:58:32 UTC) #18
kjellander_webrtc
+deadbeef@ for webrtc/api (and since added TODO's are copies of his).
4 years, 1 month ago (2016-11-03 13:02:01 UTC) #20
Taylor Brandstetter
lgtm. Now I really want to get rid of those duplicate methods though.
4 years, 1 month ago (2016-11-03 23:50:45 UTC) #21
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/2468093004/20001
4 years, 1 month ago (2016-11-07 09:16:33 UTC) #25
commit-bot: I haz the power
Committed patchset #2 (id:20001)
4 years, 1 month ago (2016-11-07 09:18:13 UTC) #27
commit-bot: I haz the power
4 years, 1 month ago (2016-11-07 09:18:21 UTC) #29
Message was sent while issue was closed.
Patchset 2 (id:??) landed as
https://crrev.com/71a1b61c4f1b9afe769e4b0d448008aa3fa322e4
Cr-Commit-Position: refs/heads/master@{#14938}

Powered by Google App Engine
This is Rietveld 408576698