Be able to build cast_unittest and related targets in Chrome tree

Be able to build cast_unittest and related targets in Chrome tree

Adding cast.gyp to build/all.gyp. Also a bunch of chromium style errors.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=226264

[pwestin, 2013-09-25 21:24:46]

question

https://codereview.chromium.org/24586003/diff/1/media/cast/audio_receiver/aud...
File media/cast/audio_receiver/audio_decoder.cc (left):

https://codereview.chromium.org/24586003/diff/1/media/cast/audio_receiver/aud...
media/cast/audio_receiver/audio_decoder.cc:52:
webrtc::AudioCodingModule::Destroy(audio_decoder_);
This is needed to avoid a memory leak; why did you need to remove it?

https://codereview.chromium.org/24586003/diff/1/media/cast/audio_sender/audio...
File media/cast/audio_sender/audio_encoder.cc (left):

https://codereview.chromium.org/24586003/diff/1/media/cast/audio_sender/audio...
media/cast/audio_sender/audio_encoder.cc:114:
webrtc::AudioCodingModule::Destroy(audio_encoder_);
This is needed to avoid a memory leak; why did you need to remove it?

[Alpha Left Google, 2013-09-25 21:26:20]

https://codereview.chromium.org/24586003/diff/1/media/cast/audio_receiver/aud...
File media/cast/audio_receiver/audio_decoder.cc (left):

https://codereview.chromium.org/24586003/diff/1/media/cast/audio_receiver/aud...
media/cast/audio_receiver/audio_decoder.cc:52:
webrtc::AudioCodingModule::Destroy(audio_decoder_);
On 2013/09/25 21:24:46, pwestin wrote:
> This is needed to avoid a memory leak; why did you need to remove it?

I can't find reference to ACM::Destroy() in the header file:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...

And compiler complains about the same thing as well.

[Alpha Left Google, 2013-09-25 21:31:24]

Updated code to use scoped_ptr to own ACM.

[pwestin, 2013-09-25 21:41:24]

lgtm

[hubbe, 2013-09-26 23:03:19]

https://codereview.chromium.org/24586003/diff/57001/media/cast/audio_receiver...
File media/cast/audio_receiver/audio_decoder.h (right):

https://codereview.chromium.org/24586003/diff/57001/media/cast/audio_receiver...
media/cast/audio_receiver/audio_decoder.h:41: virtual ~AudioDecoder();
What's the point of making AudioDecoder virtual?
There are no virtual methods in this class, is there?

https://codereview.chromium.org/24586003/diff/57001/media/cast/audio_receiver...
media/cast/audio_receiver/audio_decoder.h:44: friend class
base::RefCountedThreadSafe<AudioDecoder>;
Why is this needed?

https://codereview.chromium.org/24586003/diff/57001/media/cast/audio_receiver...
File media/cast/audio_receiver/audio_receiver.cc (right):

https://codereview.chromium.org/24586003/diff/57001/media/cast/audio_receiver...
media/cast/audio_receiver/audio_receiver.cc:74: virtual void RequestKeyFrame() {
I guess I don't understand OVERRIDE, why is it not appropriate here?

https://codereview.chromium.org/24586003/diff/57001/media/cast/audio_receiver...
File media/cast/audio_receiver/audio_receiver_unittest.cc (right):

https://codereview.chromium.org/24586003/diff/57001/media/cast/audio_receiver...
media/cast/audio_receiver/audio_receiver_unittest.cc:58:
payload_.assign(kPacketSize, 0);
Can you fix the indentation here too?

https://codereview.chromium.org/24586003/diff/57001/media/cast/audio_sender/a...
File media/cast/audio_sender/audio_sender.cc (right):

https://codereview.chromium.org/24586003/diff/57001/media/cast/audio_sender/a...
media/cast/audio_sender/audio_sender.cc:25: virtual void
OnReceivedSendReportRequest() {
Why only this one?

https://codereview.chromium.org/24586003/diff/57001/media/cast/rtcp/test_rtcp...
File media/cast/rtcp/test_rtcp_packet_builder.h (right):

https://codereview.chromium.org/24586003/diff/57001/media/cast/rtcp/test_rtcp...
media/cast/rtcp/test_rtcp_packet_builder.h:35: static const int kRembBitrate =
52428;
?

[pwestin, 2013-09-26 23:20:23]

Added comments

https://codereview.chromium.org/24586003/diff/57001/media/cast/cast_thread.cc
File media/cast/cast_thread.cc (right):

https://codereview.chromium.org/24586003/diff/57001/media/cast/cast_thread.cc...
media/cast/cast_thread.cc:28: CastThread::~CastThread() {
nit: {} on same line

https://codereview.chromium.org/24586003/diff/57001/media/cast/cast_thread.cc...
media/cast/cast_thread.cc:65: return NULL;
Why add this? it's much better to get a compilation error than a run-time error

[Alpha Left Google, 2013-09-27 00:45:12]

https://codereview.chromium.org/24586003/diff/57001/media/cast/audio_receiver...
File media/cast/audio_receiver/audio_decoder.h (right):

https://codereview.chromium.org/24586003/diff/57001/media/cast/audio_receiver...
media/cast/audio_receiver/audio_decoder.h:41: virtual ~AudioDecoder();
On 2013/09/26 23:03:20, hubbe wrote:
> What's the point of making AudioDecoder virtual?
> There are no virtual methods in this class, is there?

This is necessary. This is a common problem with RefCountedThreadSafe.
RefCountedThreadSafe deletes an object when a ref goes to 0 using the type of
the base class, and in this case AudioDecoder. So the destructor has to be
virtual otherwise classes inherit from AudioDecoder will not have their dtor
executed properly.

https://codereview.chromium.org/24586003/diff/57001/media/cast/audio_receiver...
media/cast/audio_receiver/audio_decoder.h:44: friend class
base::RefCountedThreadSafe<AudioDecoder>;
On 2013/09/26 23:03:20, hubbe wrote:
> Why is this needed?

This is chromium style. Chromium style wants a refcounted class to have
protected or private dtor, this is needed such that RefCountedThreadSafe can
delete this object.

https://codereview.chromium.org/24586003/diff/57001/media/cast/audio_receiver...
File media/cast/audio_receiver/audio_receiver.cc (right):

https://codereview.chromium.org/24586003/diff/57001/media/cast/audio_receiver...
media/cast/audio_receiver/audio_receiver.cc:74: virtual void RequestKeyFrame() {
On 2013/09/26 23:03:20, hubbe wrote:
> I guess I don't understand OVERRIDE, why is it not appropriate here?

Because base class doesn't have this method any more. I think this method should
be deleted now.

https://codereview.chromium.org/24586003/diff/57001/media/cast/audio_receiver...
File media/cast/audio_receiver/audio_receiver_unittest.cc (right):

https://codereview.chromium.org/24586003/diff/57001/media/cast/audio_receiver...
media/cast/audio_receiver/audio_receiver_unittest.cc:58:
payload_.assign(kPacketSize, 0);
On 2013/09/26 23:03:20, hubbe wrote:
> Can you fix the indentation here too?

Done.

https://codereview.chromium.org/24586003/diff/57001/media/cast/audio_sender/a...
File media/cast/audio_sender/audio_sender.cc (right):

https://codereview.chromium.org/24586003/diff/57001/media/cast/audio_sender/a...
media/cast/audio_sender/audio_sender.cc:25: virtual void
OnReceivedSendReportRequest() {
On 2013/09/26 23:03:20, hubbe wrote:
> Why only this one?

This method is no more in the base class. I should remove this entirely.

https://codereview.chromium.org/24586003/diff/57001/media/cast/cast_thread.cc
File media/cast/cast_thread.cc (right):

https://codereview.chromium.org/24586003/diff/57001/media/cast/cast_thread.cc...
media/cast/cast_thread.cc:28: CastThread::~CastThread() {
On 2013/09/26 23:20:23, pwestin wrote:
> nit: {} on same line

Done.

https://codereview.chromium.org/24586003/diff/57001/media/cast/cast_thread.cc...
media/cast/cast_thread.cc:65: return NULL;
On 2013/09/26 23:20:23, pwestin wrote:
> Why add this? it's much better to get a compilation error than a run-time
error 

VC++ complains without this case.

https://codereview.chromium.org/24586003/diff/57001/media/cast/rtcp/test_rtcp...
File media/cast/rtcp/test_rtcp_packet_builder.h (right):

https://codereview.chromium.org/24586003/diff/57001/media/cast/rtcp/test_rtcp...
media/cast/rtcp/test_rtcp_packet_builder.h:35: static const int kRembBitrate =
52428;
On 2013/09/26 23:03:20, hubbe wrote:
> ?

This value is chosen randomly for testing. It can be of any value. Compiler
complains because this const value get assigned to a uint16.

[hubbe, 2013-09-27 19:22:52]

https://codereview.chromium.org/24586003/diff/57001/media/cast/audio_receiver...
File media/cast/audio_receiver/audio_receiver.cc (right):

https://codereview.chromium.org/24586003/diff/57001/media/cast/audio_receiver...
media/cast/audio_receiver/audio_receiver.cc:74: virtual void RequestKeyFrame() {
On 2013/09/27 00:45:13, Alpha wrote:
> On 2013/09/26 23:03:20, hubbe wrote:
> > I guess I don't understand OVERRIDE, why is it not appropriate here?
> 
> Because base class doesn't have this method any more. I think this method
should
> be deleted now.

Then delete it?
Or at least add a TODO

https://codereview.chromium.org/24586003/diff/57001/media/cast/audio_sender/a...
File media/cast/audio_sender/audio_sender.cc (right):

https://codereview.chromium.org/24586003/diff/57001/media/cast/audio_sender/a...
media/cast/audio_sender/audio_sender.cc:25: virtual void
OnReceivedSendReportRequest() {
On 2013/09/27 00:45:13, Alpha wrote:
> On 2013/09/26 23:03:20, hubbe wrote:
> > Why only this one?
> 
> This method is no more in the base class. I should remove this entirely.

Please do. (or add TODO)

https://codereview.chromium.org/24586003/diff/57001/media/cast/rtcp/test_rtcp...
File media/cast/rtcp/test_rtcp_packet_builder.h (right):

https://codereview.chromium.org/24586003/diff/57001/media/cast/rtcp/test_rtcp...
media/cast/rtcp/test_rtcp_packet_builder.h:35: static const int kRembBitrate =
52428;
On 2013/09/27 00:45:13, Alpha wrote:
> On 2013/09/26 23:03:20, hubbe wrote:
> > ?
> 
> This value is chosen randomly for testing. It can be of any value. Compiler
> complains because this const value get assigned to a uint16.

Add a comment that the value is arbitrary perhaps?

[Alpha Left Google, 2013-09-27 21:13:53]

https://codereview.chromium.org/24586003/diff/57001/media/cast/audio_receiver...
File media/cast/audio_receiver/audio_receiver.cc (right):

https://codereview.chromium.org/24586003/diff/57001/media/cast/audio_receiver...
media/cast/audio_receiver/audio_receiver.cc:74: virtual void RequestKeyFrame() {
On 2013/09/27 19:22:53, hubbe wrote:
> On 2013/09/27 00:45:13, Alpha wrote:
> > On 2013/09/26 23:03:20, hubbe wrote:
> > > I guess I don't understand OVERRIDE, why is it not appropriate here?
> > 
> > Because base class doesn't have this method any more. I think this method
> should
> > be deleted now.
> 
> Then delete it?
> Or at least add a TODO

Already removed.

https://codereview.chromium.org/24586003/diff/57001/media/cast/audio_sender/a...
File media/cast/audio_sender/audio_sender.cc (right):

https://codereview.chromium.org/24586003/diff/57001/media/cast/audio_sender/a...
media/cast/audio_sender/audio_sender.cc:25: virtual void
OnReceivedSendReportRequest() {
On 2013/09/27 19:22:53, hubbe wrote:
> On 2013/09/27 00:45:13, Alpha wrote:
> > On 2013/09/26 23:03:20, hubbe wrote:
> > > Why only this one?
> > 
> > This method is no more in the base class. I should remove this entirely.
> 
> Please do. (or add TODO)

Already removed.

https://codereview.chromium.org/24586003/diff/57001/media/cast/rtcp/test_rtcp...
File media/cast/rtcp/test_rtcp_packet_builder.h (right):

https://codereview.chromium.org/24586003/diff/57001/media/cast/rtcp/test_rtcp...
media/cast/rtcp/test_rtcp_packet_builder.h:35: static const int kRembBitrate =
52428;
On 2013/09/27 19:22:53, hubbe wrote:
> On 2013/09/27 00:45:13, Alpha wrote:
> > On 2013/09/26 23:03:20, hubbe wrote:
> > > ?
> > 
> > This value is chosen randomly for testing. It can be of any value. Compiler
> > complains because this const value get assigned to a uint16.
> 
> Add a comment that the value is arbitrary perhaps?

Done.

[hubbe, 2013-09-27 21:19:19]

lgtm

[commit-bot: I haz the power, 2013-09-27 21:23:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hclam@chromium.org/24586003/77001

[commit-bot: I haz the power, 2013-09-27 23:14:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hclam@chromium.org/24586003/99001

[commit-bot: I haz the power, 2013-09-28 02:52:20]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_x64_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-09-28 07:07:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hclam@chromium.org/24586003/99001

[commit-bot: I haz the power, 2013-09-28 07:41:43]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_x64_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-09-30 19:31:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hclam@chromium.org/24586003/136001

[commit-bot: I haz the power, 2013-10-01 00:43:10]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_x64_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-10-01 04:31:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hclam@chromium.org/24586003/146001

[commit-bot: I haz the power, 2013-10-01 16:41:50]

The commit queue went berserk retrying too often for a
seemingly flaky test on builder android_dbg:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[commit-bot: I haz the power, 2013-10-01 17:06:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hclam@chromium.org/24586003/146001

[commit-bot: I haz the power, 2013-10-01 18:11:15]

Change committed as 226264