[rtp_rtcp] rtcp::SenderReport moved into own file and got Parse function

[rtp_rtcp] rtcp::SenderReport moved into own file and got Parse function

BUG=webrtc:5260
R=åsapersson

Committed: https://crrev.com/34ed2b95a59a3935d0a013cbc6d389b1608bf7eb
Cr-Commit-Position: refs/heads/master@{#11288}

[åsapersson, 2016-01-13 14:11:33]

https://codereview.webrtc.org/1544983002/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtcp_packet/remb_unittest.cc (right):

https://codereview.webrtc.org/1544983002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet/remb_unittest.cc:12: 
Should this file be part of this CL?

https://codereview.webrtc.org/1544983002/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtcp_packet/sender_report.cc (right):

https://codereview.webrtc.org/1544983002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet/sender_report.cc:49:
RTC_DCHECK_EQ(header.packet_type, kPacketType);
RTC_DCHECK_EQ(kPacketType, header.packet_type);

https://codereview.webrtc.org/1544983002/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtcp_packet/sender_report.h (right):

https://codereview.webrtc.org/1544983002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet/sender_report.h:66: static const int
kMaxNumberOfReportBlocks = 0x1f;
size_t?

https://codereview.webrtc.org/1544983002/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtcp_packet/sender_report_unittest.cc
(right):

https://codereview.webrtc.org/1544983002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet/sender_report_unittest.cc:29: using
RTCPUtility::RtcpParseCommonHeader;
move above?

https://codereview.webrtc.org/1544983002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet/sender_report_unittest.cc:34:
EXPECT_TRUE(SenderReport::kPacketType == header.packet_type);
remove, already checked in Parse?

https://codereview.webrtc.org/1544983002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet/sender_report_unittest.cc:39:
SenderReport sr;
end with underscore?

https://codereview.webrtc.org/1544983002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet/sender_report_unittest.cc:41: // only
ParsePacket can change parsed, tests should use it in ro mode
s/only/Only (end w/period).

https://codereview.webrtc.org/1544983002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet/sender_report_unittest.cc:48:
TEST_F(RtcpPacketSenderReportTest, SenderReport) {
maybe call WithNoReportBlock

https://codereview.webrtc.org/1544983002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet/sender_report_unittest.cc:50:
sr.WithNtp(NtpTime(0x11121418, 0x22242628));
use constants for these and below?

[danilchap, 2016-01-13 15:54:44]

https://codereview.webrtc.org/1544983002/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtcp_packet/remb_unittest.cc (right):

https://codereview.webrtc.org/1544983002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet/remb_unittest.cc:12: 
On 2016/01/13 14:11:32, åsapersson wrote:
> Should this file be part of this CL?

oops, removed.

https://codereview.webrtc.org/1544983002/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtcp_packet/sender_report.cc (right):

https://codereview.webrtc.org/1544983002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet/sender_report.cc:49:
RTC_DCHECK_EQ(header.packet_type, kPacketType);
On 2016/01/13 14:11:32, åsapersson wrote:
> RTC_DCHECK_EQ(kPacketType, header.packet_type);

Actually can't use _EQ in this case. While it works great with clang, it creates
linking error in some windows toolchain.
have to reverse to less nice RTC_DCHECK(value == constant) syntax.

https://codereview.webrtc.org/1544983002/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtcp_packet/sender_report.h (right):

https://codereview.webrtc.org/1544983002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet/sender_report.h:66: static const int
kMaxNumberOfReportBlocks = 0x1f;
On 2016/01/13 14:11:32, åsapersson wrote:
> size_t?

Done.

https://codereview.webrtc.org/1544983002/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtcp_packet/sender_report_unittest.cc
(right):

https://codereview.webrtc.org/1544983002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet/sender_report_unittest.cc:29: using
RTCPUtility::RtcpParseCommonHeader;
On 2016/01/13 14:11:32, åsapersson wrote:
> move above?

Done.

https://codereview.webrtc.org/1544983002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet/sender_report_unittest.cc:34:
EXPECT_TRUE(SenderReport::kPacketType == header.packet_type);
On 2016/01/13 14:11:32, åsapersson wrote:
> remove, already checked in Parse?

Done.

https://codereview.webrtc.org/1544983002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet/sender_report_unittest.cc:39:
SenderReport sr;
On 2016/01/13 14:11:32, åsapersson wrote:
> end with underscore?

tests access this variable directly, as if it is a local variable.
So I guess would be better to make it local.

https://codereview.webrtc.org/1544983002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet/sender_report_unittest.cc:41: // only
ParsePacket can change parsed, tests should use it in ro mode
On 2016/01/13 14:11:32, åsapersson wrote:
> s/only/Only (end w/period).

Done.

https://codereview.webrtc.org/1544983002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet/sender_report_unittest.cc:48:
TEST_F(RtcpPacketSenderReportTest, SenderReport) {
On 2016/01/13 14:11:32, åsapersson wrote:
> maybe call WithNoReportBlock

Done, WithoutReportBlocks feels even better.

https://codereview.webrtc.org/1544983002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet/sender_report_unittest.cc:50:
sr.WithNtp(NtpTime(0x11121418, 0x22242628));
On 2016/01/13 14:11:32, åsapersson wrote:
> use constants for these and below?

Done.

[åsapersson, 2016-01-13 16:20:39]

lgtm

[danilchap, 2016-01-13 20:09:17]

Description was changed from

==========
[rtp_rtcp] rtcp::SenderReport moved into own file and got Parse function

BUG=webrtc:5260
R=åsapersson
==========

to

==========
[rtp_rtcp] rtcp::SenderReport moved into own file and got Parse function

BUG=webrtc:5260
R=åsapersson
==========

[danilchap, 2016-01-13 20:09:17]

danilchap@webrtc.org changed reviewers:
+ mflodman@webrtc.org

[mflodman, 2016-01-18 08:55:12]

LGTM

[danilchap, 2016-01-18 09:00:06]

The CQ bit was checked by danilchap@webrtc.org

[danilchap, 2016-01-18 09:00:07]

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

[commit-bot: I haz the power, 2016-01-18 09:00:10]

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

[commit-bot: I haz the power, 2016-01-18 10:43:36]

Description was changed from

==========
[rtp_rtcp] rtcp::SenderReport moved into own file and got Parse function

BUG=webrtc:5260
R=åsapersson
==========

to

==========
[rtp_rtcp] rtcp::SenderReport moved into own file and got Parse function

BUG=webrtc:5260
R=åsapersson
==========

[commit-bot: I haz the power, 2016-01-18 10:43:37]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-01-18 10:43:41]

Description was changed from

==========
[rtp_rtcp] rtcp::SenderReport moved into own file and got Parse function

BUG=webrtc:5260
R=åsapersson
==========

to

==========
[rtp_rtcp] rtcp::SenderReport moved into own file and got Parse function

BUG=webrtc:5260
R=åsapersson

Committed: https://crrev.com/34ed2b95a59a3935d0a013cbc6d389b1608bf7eb
Cr-Commit-Position: refs/heads/master@{#11288}
==========

[commit-bot: I haz the power, 2016-01-18 10:43:42]

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