Use rtcp class instead of RTCPUtility parser for rtcp_sender_unittest

webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc

Index: webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc
diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc b/webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc
index 235c84b09ee4c12b79319d197d4703fea6e2320b..2758f6053523378dc2416c51b4c924954f920b2f 100644
--- a/webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc
+++ b/webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc
@@ -12,8 +12,9 @@
 
 #include "webrtc/base/rate_limiter.h"
 #include "webrtc/common_types.h"
+#include "webrtc/modules/rtp_rtcp/source/rtcp_packet/bye.h"
+#include "webrtc/modules/rtp_rtcp/source/rtcp_packet/common_header.h"
 #include "webrtc/modules/rtp_rtcp/source/rtcp_sender.h"
-#include "webrtc/modules/rtp_rtcp/source/rtcp_utility.h"
 #include "webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.h"
 #include "webrtc/test/gmock.h"
 #include "webrtc/test/gtest.h"
@@ -23,7 +24,6 @@
 using ::testing::_;
 using ::testing::ElementsAre;
 using ::testing::Invoke;
-using webrtc::RTCPUtility::RtcpCommonHeader;
 
 namespace webrtc {
 
@@ -790,17 +790,14 @@ TEST_F(RtcpSenderTest, ByeMustBeLast) {
   EXPECT_CALL(mock_transport, SendRtcp(_, _))
     .WillOnce(Invoke([](const uint8_t* data, size_t len) {
     const uint8_t* next_packet = data;
-    while (next_packet < data + len) {
-      RtcpCommonHeader header;
-      RtcpParseCommonHeader(next_packet, len - (next_packet - data), &header);
-      next_packet = next_packet +
-        header.payload_size_bytes +
-        RtcpCommonHeader::kHeaderSizeBytes;
-      if (header.packet_type == RTCPUtility::PT_BYE) {
-        bool is_last_packet = (data + len == next_packet);
-        EXPECT_TRUE(is_last_packet) <<
-          "Bye packet should be last in a compound RTCP packet.";
-      }
+    const uint8_t* const packet_end = data + len;
+    rtcp::CommonHeader packet;
+    while (next_packet < packet_end) {
+      EXPECT_TRUE(packet.Parse(next_packet, packet_end - next_packet));
+      next_packet = packet.NextPacket();
+      if (packet.type() == rtcp::Bye::kPacketType)
+        EXPECT_EQ(0, packet_end - next_packet)
+            << "Bye packet should be last in a compound RTCP packet.";

[sprang_webrtc, 2016/11/01 11:44:05]

Not a fault of this cl, but isn't this a bit backwards? If there is no bye
packet this will succeed. Shouldn't it be

if (packet_end - next_packet == 0) {
  EXPECT_EQ(rtcp::Bye::kPacketType, packet.type());
}

[sprang_webrtc, 2016/11/01 11:46:18]

...or even

if (packet_end - next_packet == 0) {
  EXPECT_EQ(rtcp::Bye::kPacketType, packet.type());
} else { 
  EXPECT_NE(rtcp::Bye::kPacketType, packet.type());
} 

[danilchap, 2016/11/01 12:12:09]

This test is technically correct: it validates that if Bye packet is present, it
is the very last of a Compound packet.
Absent of Bye packet is valid in general.

Though for this particular test absent of Bye packet mean test setup is
incorrect.
Added an additional validation that last packet is Bye packet.
Added comments to note two neighbor similar expectations are actually very
different.

     }
 
     return true;