Add SetReportBlocks to rtcp Sender/Receive Report classes.

webrtc/modules/rtp_rtcp/source/rtcp_packet/receiver_report_unittest.cc

Index: webrtc/modules/rtp_rtcp/source/rtcp_packet/receiver_report_unittest.cc
diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_packet/receiver_report_unittest.cc b/webrtc/modules/rtp_rtcp/source/rtcp_packet/receiver_report_unittest.cc
index 4061db889f2b39a0a2681f680722bc2ea2c08c43..d18d4737764d40d634e522883dcb493b623ccfc8 100644
--- a/webrtc/modules/rtp_rtcp/source/rtcp_packet/receiver_report_unittest.cc
+++ b/webrtc/modules/rtp_rtcp/source/rtcp_packet/receiver_report_unittest.cc
@@ -127,4 +127,35 @@ TEST(RtcpPacketReceiverReportTest, CreateWithTooManyReportBlocks) {
   EXPECT_FALSE(rr.AddReportBlock(rb));
 }
 
+TEST(RtcpPacketReceiverReportTest, SetReportBlocksOverwritesOldBlocks) {
+  ReceiverReport rr;
+  ReportBlock report_block;
+  // Use jitter field of the report blocks to distinguish them.
+  report_block.SetJitter(1001);
+  rr.AddReportBlock(report_block);
+  ASSERT_EQ(rr.report_blocks().size(), 1u);
+  ASSERT_EQ(rr.report_blocks()[0].jitter(), 1001u);
+
+  std::vector<ReportBlock> blocks(3);
+  blocks[0].SetJitter(2001);

[eladalon, 2017/07/25 11:46:01]

nit: Maybe use 2001u, here and elsewhere, to prevent the reader from a momentary
confusion over the value here appearing different than in the EXPECT_EQ part
(where I know the u suffix is necessary for the compiler).

[danilchap, 2017/07/25 12:45:20]

Done. Less confusion is better.

+  blocks[1].SetJitter(3001);
+  blocks[2].SetJitter(4001);
+  EXPECT_TRUE(rr.SetReportBlocks(blocks));
+  ASSERT_EQ(rr.report_blocks().size(), 3u);
+  EXPECT_EQ(rr.report_blocks()[0].jitter(), 2001u);
+  EXPECT_EQ(rr.report_blocks()[1].jitter(), 3001u);
+  EXPECT_EQ(rr.report_blocks()[2].jitter(), 4001u);
+}
+
+TEST(RtcpPacketReceiverReportTest, CanSetMax31ReportBlock) {

[eladalon, 2017/07/25 11:46:01]

nit: I would have used something like MaxReportBlockLimitObserved, or anything
else that wouldn't use the value as part of the name.

[danilchap, 2017/07/25 12:45:20]

Since exact value moved away from test, moved it away from test name too.

+  ReceiverReport rr;
+  const size_t kMaxReportBlocks = (1 << 5) - 1;

[eladalon, 2017/07/25 11:48:47]

Oh, by the way - I would understand if it were:
(1 << some_constant) - 1
But I'm not sure we get extra clarity from
(1 << 5) - 1
Maybe reconsider this? Perhaps we can give the 5 a name? (I see it's also used
in other tests in this file, btw.)

[danilchap, 2017/07/25 12:45:20]

Not sure why I hide the constant in the private sections of the Sender/Receiver
Report classes in the first place: it is fixed by the spec and shouldn't change.
Made it public and use that one in tests.

+
+  std::vector<ReportBlock> blocks(kMaxReportBlocks);
+  EXPECT_TRUE(rr.SetReportBlocks(blocks));
+
+  blocks.push_back(ReportBlock());
+  EXPECT_FALSE(rr.SetReportBlocks(blocks));

[eladalon, 2017/07/25 11:46:01]

nit: This is not important enough to block (no pun intended), and might even be
subjective, but I'd just like to mention that I think the testing guidelines
would discourage this, with the rationale being that there could be two
different bugs that would still trigger the same test failing:
1. Unable to set 31 blocks (the limit somehow became lower).
2. Able to set 32 blocks (the limit somehow became higher).
By breaking this into two tests, we know immediately what happened as soon as
the failure occurs.

[danilchap, 2017/07/25 12:45:20]

test guide suggest to test distinct behavior in different tests.
I consider this single behavior even if it can break in different ways.

Changed expectations to make them look different.

[eladalon, 2017/07/25 13:26:09]

The guide discourages performing actions on the object under test, after a test
has been performed. But I agree that this case is very subjective, and it can be
very strongly argued that having two tests for the observation of a limit might
actually be less clear, rather than more. I think I'll check this like you in
future tests I'll write.

+}
+
 }  // namespace webrtc