Relanding https://codereview.webrtc.org/1715883002/ in pieces.

Relanding https://codereview.webrtc.org/1715883002/ in pieces.

- Change argument type to int for SetSendTelephoneEventPayloadType()

BUG=webrtc:4690
Committed: https://crrev.com/31642aa8f924926b742fedefb0566bda46484e6c
Cr-Commit-Position: refs/heads/master@{#11980}

[the sun, 2016-03-14 10:57:15]

Description was changed from

==========
Relanding https://codereview.webrtc.org/1715883002/ in pieces.

- A few more cleanups.

BUG=webrtc:4690
==========

to

==========
Relanding https://codereview.webrtc.org/1715883002/ in pieces.

- Change argument type to int for SetSendTelephoneEventPayloadType()

BUG=webrtc:4690
==========

[the sun, 2016-03-14 10:57:15]

solenberg@webrtc.org changed reviewers:
+ tina.legrand@webrtc.org

[the sun, 2016-03-14 12:01:53]

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

[commit-bot: I haz the power, 2016-03-14 12:01:59]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1798903002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1798903002/40001

[tlegrand-webrtc, 2016-03-14 12:11:13]

The behavior if the payload type is wrong changes with this CL, but since I
believe it is handled correctly later in the function, I say LGTM.

I also commented on some code that looks weird. I might have missed something,
though.

https://codereview.webrtc.org/1798903002/diff/40001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/1798903002/diff/40001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.cc:2235: CodecInst codec = {0};
By removing the return -1 here, you have changed behavior in case of faulty
input. Is that handled later in  RegisterSendPayload()?

https://codereview.webrtc.org/1798903002/diff/40001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.cc:2241: if
(_rtpRtcpModule->RegisterSendPayload(codec) != 0) {
Isn't this code a bit odd? If _rtpRtcpModule->RegisterSendPayload(codec)returns
error the first time, we will call it again. Why?

[the sun, 2016-03-14 13:21:01]

Right, I've added validation of the payload type and a corresponding unit test.

https://codereview.webrtc.org/1798903002/diff/40001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/1798903002/diff/40001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.cc:2235: CodecInst codec = {0};
On 2016/03/14 12:11:13, tlegrand-webrtc wrote:
> By removing the return -1 here, you have changed behavior in case of faulty
> input. Is that handled later in  RegisterSendPayload()?

Very good point. I thought it was already capped, when I look through the SDP
handling code it becomes apparent that the payload type range is never
validated. I've added the necessary validation in WVoMC, where it should be, and
keeping the DCHECKs here.

https://codereview.webrtc.org/1798903002/diff/40001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.cc:2241: if
(_rtpRtcpModule->RegisterSendPayload(codec) != 0) {
On 2016/03/14 12:11:13, tlegrand-webrtc wrote:
> Isn't this code a bit odd? If
_rtpRtcpModule->RegisterSendPayload(codec)returns
> error the first time, we will call it again. Why?

Notice the call to DeRegisterSendPayload() - if the code fails, deregister and
register again.

[the sun, 2016-03-14 13:21:22]

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

[commit-bot: I haz the power, 2016-03-14 13:21:28]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1798903002/20002
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1798903002/20002

[commit-bot: I haz the power, 2016-03-14 14:43:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-14 14:43:42]

Dry run: This issue passed the CQ dry run.

[the sun, 2016-03-14 14:59:14]

The CQ bit was checked by solenberg@webrtc.org

[the sun, 2016-03-14 14:59:15]

The patchset sent to the CQ was uploaded after l-g-t-m from
tina.legrand@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1798903002/#ps20002 (title:
"Added test case")

[commit-bot: I haz the power, 2016-03-14 14:59:20]

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

[commit-bot: I haz the power, 2016-03-14 15:00:40]

Description was changed from

==========
Relanding https://codereview.webrtc.org/1715883002/ in pieces.

- Change argument type to int for SetSendTelephoneEventPayloadType()

BUG=webrtc:4690
==========

to

==========
Relanding https://codereview.webrtc.org/1715883002/ in pieces.

- Change argument type to int for SetSendTelephoneEventPayloadType()

BUG=webrtc:4690
==========

[commit-bot: I haz the power, 2016-03-14 15:00:41]

Committed patchset #5 (id:20002)

[commit-bot: I haz the power, 2016-03-14 15:00:44]

Description was changed from

==========
Relanding https://codereview.webrtc.org/1715883002/ in pieces.

- Change argument type to int for SetSendTelephoneEventPayloadType()

BUG=webrtc:4690
==========

to

==========
Relanding https://codereview.webrtc.org/1715883002/ in pieces.

- Change argument type to int for SetSendTelephoneEventPayloadType()

BUG=webrtc:4690

Committed: https://crrev.com/31642aa8f924926b742fedefb0566bda46484e6c
Cr-Commit-Position: refs/heads/master@{#11980}
==========

[commit-bot: I haz the power, 2016-03-14 15:00:45]

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

[tlegrand-webrtc, 2016-03-16 13:54:50]

I took a look at your final changes after the fact, and it looks great.