WebRTC: Fix and enable -Woverloaded-virtual warnings.

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}

[kjellander_webrtc, 2016-11-03 07:24:55]

Description was changed from

==========
WebRTC: Enable -Woverloaded-virtual everywhere

BUG=webrtc:6653
==========

to

==========
WebRTC: Fix -Woverloaded-virtual warnings.

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
==========

[kjellander_webrtc, 2016-11-03 07:25:23]

Description was changed from

==========
WebRTC: Fix -Woverloaded-virtual warnings.

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
==========

to

==========
WebRTC: Fix and enable -Woverloaded-virtual warnings.

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
==========

[kjellander_webrtc, 2016-11-03 07:25:44]

Description was changed from

==========
WebRTC: Fix and enable -Woverloaded-virtual warnings.

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
==========

to

==========
WebRTC: Fix and enable -Woverloaded-virtual warnings.

Essentially applying the same change as in
https://codereview.webrtc.org/2023413002 in more locations.

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
==========

[kjellander_webrtc, 2016-11-03 07:27:06]

The CQ bit was checked by kjellander@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-03 07:27:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[kjellander_webrtc, 2016-11-03 07:28:49]

Description was changed from

==========
WebRTC: Fix and enable -Woverloaded-virtual warnings.

Essentially applying the same change as in
https://codereview.webrtc.org/2023413002 in more locations.

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
==========

to

==========
WebRTC: Fix and enable -Woverloaded-virtual warnings.

Essentially applying the same change as in
https://codereview.webrtc.org/2023413002 in more locations.

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
==========

[kjellander_webrtc, 2016-11-03 07:30:16]

Description was changed from

==========
WebRTC: Fix and enable -Woverloaded-virtual warnings.

Essentially applying the same change as in
https://codereview.webrtc.org/2023413002 in more locations.

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
==========

to

==========
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
==========

[kjellander_webrtc, 2016-11-03 07:40:21]

kjellander@webrtc.org changed reviewers:
+ phoglund@webrtc.org

[kjellander_webrtc, 2016-11-03 07:40:22]

This doesn't change any of the production code though, but it enables the
warning for rtc_media. Do you know if this is sufficient for the recent internal
breakage?

[kjellander_webrtc, 2016-11-03 07:41:30]

The CQ bit was unchecked by kjellander@webrtc.org

[phoglund, 2016-11-03 10:24:37]

It's not going to fix anything, since the error was with peerconnection_jni.cc.
I guess there was a separate CL to land a fix?

[phoglund, 2016-11-03 10:28:42]

I don't see how this prevents the error. The error was with
peerconnection_jni.cc which is in the libjingle_peerconnection_jni target, and
you're not modifying the flag set for that target as far I can see (just unit
test targets). Or am I missing something?

[phoglund, 2016-11-03 10:36:05]

On 2016/11/03 10:28:42, phoglund wrote:
> I don't see how this prevents the error. The error was with
> peerconnection_jni.cc which is in the libjingle_peerconnection_jni target, and
> you're not modifying the flag set for that target as far I can see (just unit
> test targets). Or am I missing something?

Hm. I read this as "-Woverloaded-virtual is set for all targets if we're posix
but not android":
https://cs.chromium.org/chromium/src/third_party/webrtc/BUILD.gn?q=common_con...

[kjellander_webrtc, 2016-11-03 10:52:53]

On 2016/11/03 10:24:37, phoglund wrote:
> It's not going to fix anything, since the error was with
peerconnection_jni.cc.
> I guess there was a separate CL to land a fix?

I created this CL before I learned that the error was introduced by
https://codereview.webrtc.org/2449993003/ (and fixed in
https://codereview.webrtc.org/2468253002/).
Since this CL enabled the warning for rtc_media, it should have caught the error
in our own build.

I think the peerconnection_jni.cc error you're referring to is the one that was
fixed by deadbeef@ in https://codereview.webrtc.org/2023413002.

[kjellander_webrtc, 2016-11-03 10:58:15]

On 2016/11/03 10:36:05, phoglund wrote:
> On 2016/11/03 10:28:42, phoglund wrote:
> > I don't see how this prevents the error. The error was with
> > peerconnection_jni.cc which is in the libjingle_peerconnection_jni target,
and
> > you're not modifying the flag set for that target as far I can see (just
unit
> > test targets). Or am I missing something?
> 
> Hm. I read this as "-Woverloaded-virtual is set for all targets if we're posix
> but not android":
>
https://cs.chromium.org/chromium/src/third_party/webrtc/BUILD.gn?q=common_con...

For GYP, it's set for all Posix (which includes Android):
https://cs.chromium.org/chromium/src/third_party/webrtc/build/common.gypi?rcl...
For GN, it's similarly set, but is incorrectly excluding Android ARM64 builds.
That's a bug that I can fix in a separate CL. It is set for the other Android
configs though (notice the || ).

[phoglund, 2016-11-03 11:59:34]

On 2016/11/03 10:58:15, kjellander_webrtc wrote:
> On 2016/11/03 10:36:05, phoglund wrote:
> > On 2016/11/03 10:28:42, phoglund wrote:
> > > I don't see how this prevents the error. The error was with
> > > peerconnection_jni.cc which is in the libjingle_peerconnection_jni target,
> and
> > > you're not modifying the flag set for that target as far I can see (just
> unit
> > > test targets). Or am I missing something?
> > 
> > Hm. I read this as "-Woverloaded-virtual is set for all targets if we're
posix
> > but not android":
> >
>
https://cs.chromium.org/chromium/src/third_party/webrtc/BUILD.gn?q=common_con...
> 
> For GYP, it's set for all Posix (which includes Android):
>
https://cs.chromium.org/chromium/src/third_party/webrtc/build/common.gypi?rcl...
> For GN, it's similarly set, but is incorrectly excluding Android ARM64 builds.
> That's a bug that I can fix in a separate CL. It is set for the other Android
> configs though (notice the || ).

Ah, I see.

Ok, so there are two errors we've seen, one in media/.../webrtcvideoengine2.cc
and one in peerconnection_jni.cc. This CL will enable warnings for the former
but not the latter. That sound right?

[kjellander_webrtc, 2016-11-03 12:26:15]

On 2016/11/03 11:59:34, phoglund wrote:
> On 2016/11/03 10:58:15, kjellander_webrtc wrote:
> > On 2016/11/03 10:36:05, phoglund wrote:
> > > On 2016/11/03 10:28:42, phoglund wrote:
> > > > I don't see how this prevents the error. The error was with
> > > > peerconnection_jni.cc which is in the libjingle_peerconnection_jni
target,
> > and
> > > > you're not modifying the flag set for that target as far I can see (just
> > unit
> > > > test targets). Or am I missing something?
> > > 
> > > Hm. I read this as "-Woverloaded-virtual is set for all targets if we're
> posix
> > > but not android":
> > >
> >
>
https://cs.chromium.org/chromium/src/third_party/webrtc/BUILD.gn?q=common_con...
> > 
> > For GYP, it's set for all Posix (which includes Android):
> >
>
https://cs.chromium.org/chromium/src/third_party/webrtc/build/common.gypi?rcl...
> > For GN, it's similarly set, but is incorrectly excluding Android ARM64
builds.
> > That's a bug that I can fix in a separate CL. It is set for the other
Android
> > configs though (notice the || ).
> 
> Ah, I see.
> 
> Ok, so there are two errors we've seen, one in media/.../webrtcvideoengine2.cc
> and one in peerconnection_jni.cc. This CL will enable warnings for the former
> but not the latter. That sound right?

Yes, the latter already got warnings enabled shortly after the fix was
submitted: https://codereview.webrtc.org/2035593003/ (peerconnection_jni.cc is
in that GYP target)
So, do I get approval for this, since it's at least a small improvement?

[phoglund, 2016-11-03 12:58:32]

On 2016/11/03 12:26:15, kjellander_webrtc wrote:
> On 2016/11/03 11:59:34, phoglund wrote:
> > On 2016/11/03 10:58:15, kjellander_webrtc wrote:
> > > On 2016/11/03 10:36:05, phoglund wrote:
> > > > On 2016/11/03 10:28:42, phoglund wrote:
> > > > > I don't see how this prevents the error. The error was with
> > > > > peerconnection_jni.cc which is in the libjingle_peerconnection_jni
> target,
> > > and
> > > > > you're not modifying the flag set for that target as far I can see
(just
> > > unit
> > > > > test targets). Or am I missing something?
> > > > 
> > > > Hm. I read this as "-Woverloaded-virtual is set for all targets if we're
> > posix
> > > > but not android":
> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/third_party/webrtc/BUILD.gn?q=common_con...
> > > 
> > > For GYP, it's set for all Posix (which includes Android):
> > >
> >
>
https://cs.chromium.org/chromium/src/third_party/webrtc/build/common.gypi?rcl...
> > > For GN, it's similarly set, but is incorrectly excluding Android ARM64
> builds.
> > > That's a bug that I can fix in a separate CL. It is set for the other
> Android
> > > configs though (notice the || ).
> > 
> > Ah, I see.
> > 
> > Ok, so there are two errors we've seen, one in
media/.../webrtcvideoengine2.cc
> > and one in peerconnection_jni.cc. This CL will enable warnings for the
former
> > but not the latter. That sound right?
> 
> Yes, the latter already got warnings enabled shortly after the fix was
> submitted: https://codereview.webrtc.org/2035593003/ (peerconnection_jni.cc is
> in that GYP target)
> So, do I get approval for this, since it's at least a small improvement?

Ohh, so the peerconnection error isn't one of the current ones, but the old
error?
Then I understand.

lgtm

[kjellander_webrtc, 2016-11-03 13:02:00]

kjellander@webrtc.org changed reviewers:
+ deadbeef@webrtc.org

[kjellander_webrtc, 2016-11-03 13:02:01]

+deadbeef@ for webrtc/api (and since added TODO's are copies of his).

[Taylor Brandstetter, 2016-11-03 23:50:45]

lgtm. Now I really want to get rid of those duplicate methods though.

[kjellander_webrtc, 2016-11-07 06:15:37]

Description was changed from

==========
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
==========

to

==========
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
==========

[kjellander_webrtc, 2016-11-07 06:15:37]

Description was changed from

==========
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
==========

to

==========
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
==========

[kjellander_webrtc, 2016-11-07 09:16:22]

The CQ bit was checked by kjellander@webrtc.org

[commit-bot: I haz the power, 2016-11-07 09:16:33]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-11-07 09:18:12]

Description was changed from

==========
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
==========

to

==========
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
==========

[commit-bot: I haz the power, 2016-11-07 09:18:13]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-11-07 09:18:20]

Description was changed from

==========
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
==========

to

==========
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}
==========

[commit-bot: I haz the power, 2016-11-07 09:18:21]

Patchset 2 (id:??) landed as
https://crrev.com/71a1b61c4f1b9afe769e4b0d448008aa3fa322e4
Cr-Commit-Position: refs/heads/master@{#14938}