CodecManager::RegisterEncoder: Call SetFec on new encoder, not old

CodecManager::RegisterEncoder: Call SetFec on new encoder, not old

BUG=webrtc:5028
Committed: https://crrev.com/4dc941128f89d8997264d1b0d22262dc283de468
Cr-Commit-Position: refs/heads/master@{#10604}

[kwiberg-webrtc, 2015-11-09 19:37:52]

Description was changed from

==========
CodecManager::RegisterEncoder: Call SetFec on new encoder, not old

BUG=webrtc:5028
==========

to

==========
CodecManager::RegisterEncoder: Call SetFec on new encoder, not old

BUG=webrtc:5028
==========

[kwiberg-webrtc, 2015-11-09 19:37:52]

kwiberg@webrtc.org changed reviewers:
+ henrik.lundin@webrtc.org

[kwiberg-webrtc, 2015-11-09 19:39:06]

⚘

[hlundin-webrtc, 2015-11-10 10:39:19]

Oh, was this a bug?
LGTM.

[kwiberg-webrtc, 2015-11-10 10:41:09]

On 2015/11/10 10:39:19, hlundin-webrtc wrote:
> Oh, was this a bug?
> LGTM.

Yes. Found by inspection, not by triggering it.

[The Sun (google.com), 2015-11-10 12:01:54]

On 2015/11/10 10:41:09, kwiberg-webrtc wrote:
> On 2015/11/10 10:39:19, hlundin-webrtc wrote:
> > Oh, was this a bug?
> > LGTM.
> 
> Yes. Found by inspection, not by triggering it.

Add test case to trigger, to guard from regressions?

[kwiberg-webrtc, 2015-11-10 15:23:52]

On 2015/11/10 12:01:54, solenberg wrote:
> On 2015/11/10 10:41:09, kwiberg-webrtc wrote:
> > On 2015/11/10 10:39:19, hlundin-webrtc wrote:
> > > Oh, was this a bug?
> > > LGTM.
> > 
> > Yes. Found by inspection, not by triggering it.
> 
> Add test case to trigger, to guard from regressions?

Done.

[kwiberg-webrtc, 2015-11-11 11:41:44]

Improved test posted. (There are rebase artifacts in the other files; sorry
about that. Just look at the changes to the test.)

[the sun, 2015-11-11 11:47:20]

On 2015/11/11 11:41:44, kwiberg-webrtc wrote:
> Improved test posted. (There are rebase artifacts in the other files; sorry
> about that. Just look at the changes to the test.)

lgtm

Is the scoped_ptr necessary? Do mocks support assignment?

[hlundin-webrtc, 2015-11-11 11:56:02]

LG, but I'd like you to test the opposite direction too.

https://codereview.webrtc.org/1416633011/diff/60001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/main/acm2/codec_manager_unittest.cc (right):

https://codereview.webrtc.org/1416633011/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/main/acm2/codec_manager_unittest.cc:52:
EXPECT_TRUE(cm.codec_fec_enabled());
Also try the opposite: disable FEC and verify that it sticks across codec
change.

[kwiberg-webrtc, 2015-11-11 13:12:02]

On 2015/11/11 11:47:20, the sun wrote:
> On 2015/11/11 11:41:44, kwiberg-webrtc wrote:
> > Improved test posted. (There are rebase artifacts in the other files; sorry
> > about that. Just look at the changes to the test.)
> 
> lgtm
> 
> Is the scoped_ptr necessary? Do mocks support assignment?

If I return it directly, it'll be either move or copy constructed, and the
original destroyed. I figured I didn't want to complicate things just for the
convenience of a utility function like this. (But no, I didn't actually check
what happens.)

[kwiberg-webrtc, 2015-11-11 14:09:23]

https://codereview.webrtc.org/1416633011/diff/60001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/main/acm2/codec_manager_unittest.cc (right):

https://codereview.webrtc.org/1416633011/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/main/acm2/codec_manager_unittest.cc:52:
EXPECT_TRUE(cm.codec_fec_enabled());
On 2015/11/11 11:56:02, hlundin-webrtc wrote:
> Also try the opposite: disable FEC and verify that it sticks across codec
> change.

Done.

[hlundin-webrtc, 2015-11-11 14:24:41]

lgtm

[kwiberg-webrtc, 2015-11-11 16:30:03]

The CQ bit was checked by kwiberg@webrtc.org

[kwiberg-webrtc, 2015-11-11 16:30:04]

The patchset sent to the CQ was uploaded after l-g-t-m from solenberg@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1416633011/#ps80001 (title:
"better better test")

[commit-bot: I haz the power, 2015-11-11 16:30:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1416633011/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1416633011/80001

[commit-bot: I haz the power, 2015-11-11 16:34:26]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-11-11 16:34:39]

Patchset 5 (id:??) landed as
https://crrev.com/4dc941128f89d8997264d1b0d22262dc283de468
Cr-Commit-Position: refs/heads/master@{#10604}