Cast: Re-factor rtcp_sender.cc into rtcp_builder.cc and do some cleanup

Cast: Re-factor rtcp_sender.cc into rtcp_builder.cc and do some cleanup

Committed: https://crrev.com/9f7acae47abef17f27479ee91053058184e2baa4
Cr-Commit-Position: refs/heads/master@{#293787}

[hubbe, 2014-08-28 22:02:49]

hubbe@chromium.org changed reviewers:
+ miu@chromium.org

[miu, 2014-09-03 22:28:07]

https://codereview.chromium.org/513313004/diff/1/media/cast/net/rtcp/rtcp.h
File media/cast/net/rtcp/rtcp.h (right):

https://codereview.chromium.org/513313004/diff/1/media/cast/net/rtcp/rtcp.h#n...
media/cast/net/rtcp/rtcp.h:141: const scoped_ptr<RtcpBuilder> rtcp_builder_;
nit: Can we use composition instead of pointer-to-separate-object now?

https://codereview.chromium.org/513313004/diff/1/media/cast/net/rtcp/rtcp_bui...
File media/cast/net/rtcp/rtcp_builder.h (right):

https://codereview.chromium.org/513313004/diff/1/media/cast/net/rtcp/rtcp_bui...
media/cast/net/rtcp/rtcp_builder.h:45: RtcpBuilder(uint32 sending_ssrc);
Need explicit keyword.

https://codereview.chromium.org/513313004/diff/1/media/cast/net/rtcp/rtcp_bui...
media/cast/net/rtcp/rtcp_builder.h:57: protected:
Is there a subclass that needs access to these?

https://codereview.chromium.org/513313004/diff/1/media/cast/net/rtcp/rtcp_bui...
media/cast/net/rtcp/rtcp_builder.h:64: void AddReportBlocks(const
RtcpReportBlock& report_block);
nit: Some of these methods are called BuildBLAH() and some are called AddBLAH().
 Can you make it consistently one or the other since the methods all output
bytes via |writer_|?  IMO, "add" is the more-accurate verb.

https://codereview.chromium.org/513313004/diff/1/media/cast/net/rtcp/rtcp_bui...
File media/cast/net/rtcp/rtcp_builder_unittest.cc (right):

https://codereview.chromium.org/513313004/diff/1/media/cast/net/rtcp/rtcp_bui...
media/cast/net/rtcp/rtcp_builder_unittest.cc:64:
scoped_refptr<test::FakeSingleThreadTaskRunner> task_runner_;
Seems like you can get rid of task_runner_ (and its #include).

[hubbe, 2014-09-04 22:21:19]

https://codereview.chromium.org/513313004/diff/1/media/cast/net/rtcp/rtcp.h
File media/cast/net/rtcp/rtcp.h (right):

https://codereview.chromium.org/513313004/diff/1/media/cast/net/rtcp/rtcp.h#n...
media/cast/net/rtcp/rtcp.h:141: const scoped_ptr<RtcpBuilder> rtcp_builder_;
On 2014/09/03 22:28:07, miu wrote:
> nit: Can we use composition instead of pointer-to-separate-object now?

Oklidokli.

https://codereview.chromium.org/513313004/diff/1/media/cast/net/rtcp/rtcp_bui...
File media/cast/net/rtcp/rtcp_builder.h (right):

https://codereview.chromium.org/513313004/diff/1/media/cast/net/rtcp/rtcp_bui...
media/cast/net/rtcp/rtcp_builder.h:45: RtcpBuilder(uint32 sending_ssrc);
On 2014/09/03 22:28:07, miu wrote:
> Need explicit keyword.

Done.

https://codereview.chromium.org/513313004/diff/1/media/cast/net/rtcp/rtcp_bui...
media/cast/net/rtcp/rtcp_builder.h:57: protected:
On 2014/09/03 22:28:07, miu wrote:
> Is there a subclass that needs access to these?

The idea was for the rtcp_test_builder to use it, but I think that's actually a
bad thing, so I'm going to make these private.

https://codereview.chromium.org/513313004/diff/1/media/cast/net/rtcp/rtcp_bui...
media/cast/net/rtcp/rtcp_builder.h:64: void AddReportBlocks(const
RtcpReportBlock& report_block);
On 2014/09/03 22:28:07, miu wrote:
> nit: Some of these methods are called BuildBLAH() and some are called
AddBLAH().
>  Can you make it consistently one or the other since the methods all output
> bytes via |writer_|?  IMO, "add" is the more-accurate verb.

Build->Add (plus some other minor renamings)

https://codereview.chromium.org/513313004/diff/1/media/cast/net/rtcp/rtcp_bui...
File media/cast/net/rtcp/rtcp_builder_unittest.cc (right):

https://codereview.chromium.org/513313004/diff/1/media/cast/net/rtcp/rtcp_bui...
media/cast/net/rtcp/rtcp_builder_unittest.cc:64:
scoped_refptr<test::FakeSingleThreadTaskRunner> task_runner_;
On 2014/09/03 22:28:07, miu wrote:
> Seems like you can get rid of task_runner_ (and its #include).

Done.

[miu, 2014-09-05 18:22:37]

lgtm

https://codereview.chromium.org/513313004/diff/1/media/cast/net/rtcp/rtcp_bui...
File media/cast/net/rtcp/rtcp_builder.h (right):

https://codereview.chromium.org/513313004/diff/1/media/cast/net/rtcp/rtcp_bui...
media/cast/net/rtcp/rtcp_builder.h:57: protected:
On 2014/09/04 22:21:19, hubbe wrote:
> On 2014/09/03 22:28:07, miu wrote:
> > Is there a subclass that needs access to these?
> 
> The idea was for the rtcp_test_builder to use it, but I think that's actually
a
> bad thing, so I'm going to make these private.

I think you meant to s/protected:/private:/ here.

[hubbe, 2014-09-05 20:39:47]

The CQ bit was checked by hubbe@chromium.org

[commit-bot: I haz the power, 2014-09-05 20:47:08]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hubbe@chromium.org/513313004/40001

[hubbe, 2014-09-05 20:53:39]

https://codereview.chromium.org/513313004/diff/1/media/cast/net/rtcp/rtcp_bui...
File media/cast/net/rtcp/rtcp_builder.h (right):

https://codereview.chromium.org/513313004/diff/1/media/cast/net/rtcp/rtcp_bui...
media/cast/net/rtcp/rtcp_builder.h:57: protected:
On 2014/09/05 18:22:37, miu wrote:
> On 2014/09/04 22:21:19, hubbe wrote:
> > On 2014/09/03 22:28:07, miu wrote:
> > > Is there a subclass that needs access to these?
> > 
> > The idea was for the rtcp_test_builder to use it, but I think that's
actually
> a
> > bad thing, so I'm going to make these private.
> 
> I think you meant to s/protected:/private:/ here.

Done.

[commit-bot: I haz the power, 2014-09-06 08:42:11]

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

[commit-bot: I haz the power, 2014-09-06 08:42:12]

Exceeded time limit waiting for builds to trigger.

[hubbe, 2014-09-07 06:44:08]

The CQ bit was checked by hubbe@chromium.org

[commit-bot: I haz the power, 2014-09-07 06:44:18]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hubbe@chromium.org/513313004/40001

[commit-bot: I haz the power, 2014-09-07 06:45:10]

Committed patchset #3 (id:40001) as 6217e450d9e75037c62fff773c94aebe7f3ed2e6

[Noel Gordon, 2014-09-07 20:39:02]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/541293004/ by noel@chromium.org.

The reason for reverting is: Caused cast_unittest rtcp test failures on Linux
Asan Lsan builders (memory leaks).

http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Te...
.

[hubbe, 2014-09-08 20:18:06]

The CQ bit was checked by hubbe@chromium.org

[commit-bot: I haz the power, 2014-09-08 20:19:02]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hubbe@chromium.org/513313004/60001

[commit-bot: I haz the power, 2014-09-08 21:23:56]

Committed patchset #4 (id:60001) as cb2cbc5caa7cc9cc73c1b3a255038881a049aa36

[commit-bot: I haz the power, 2014-09-10 03:44:12]

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

[commit-bot: I haz the power, 2014-09-10 03:48:13]

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