Added files for audio writers.

Added files for audio writers.

The structure of AudioWriter and ProtobufAudioWriter are similar
to their video counterparts.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=142297

[kxing, 2012-06-14 17:25:40]

Ping

[garykac, 2012-06-14 17:50:02]

lgtm

http://codereview.chromium.org/10535153/diff/3008/remoting/protocol/protobuf_...
File remoting/protocol/protobuf_audio_writer.h (right):

http://codereview.chromium.org/10535153/diff/3008/remoting/protocol/protobuf_...
remoting/protocol/protobuf_audio_writer.h:47: // TODO(kxing): Remove |channel_|
and let |buffered_writer_| own it.
Can you create a bug for this and then do it (for audio/video writers) in a
follow-up cl?

[Sergey Ulanov, 2012-06-14 17:53:16]

http://codereview.chromium.org/10535153/diff/3008/remoting/protocol/audio_wri...
File remoting/protocol/audio_writer.cc (right):

http://codereview.chromium.org/10535153/diff/3008/remoting/protocol/audio_wri...
remoting/protocol/audio_writer.cc:17: // TODO(kxing) make this correct
nit: should be formatted like this:
 TODO(kxing): Make this correct.

http://codereview.chromium.org/10535153/diff/3008/remoting/protocol/audio_wri...
remoting/protocol/audio_writer.cc:17: // TODO(kxing) make this correct
nit: comment is not clear. Should it be "Implement this."?
Also please add NOTIMPLEMENTED() here.

http://codereview.chromium.org/10535153/diff/3008/remoting/protocol/audio_wri...
File remoting/protocol/audio_writer.h (right):

http://codereview.chromium.org/10535153/diff/3008/remoting/protocol/audio_wri...
remoting/protocol/audio_writer.h:22: class AudioWriter : public AudioStub {
Don't really need this to be an abstract interface. We needed it for VideoWriter
in the past (when we had and RTP video writer), not potentially
ProtobufVideoWriter can be merged with VideoWriter. I suggest you just move all
code you have in ProtobufAudioWriter to AudioWriter.

http://codereview.chromium.org/10535153/diff/3008/remoting/protocol/protobuf_...
File remoting/protocol/protobuf_audio_writer.h (right):

http://codereview.chromium.org/10535153/diff/3008/remoting/protocol/protobuf_...
remoting/protocol/protobuf_audio_writer.h:47: // TODO(kxing): Remove |channel_|
and let |buffered_writer_| own it.
Here and in all other places - don't change the name when you copy TODO. After
all its still the same TODO.

[kxing, 2012-06-14 22:30:42]

http://codereview.chromium.org/10535153/diff/3008/remoting/protocol/audio_wri...
File remoting/protocol/audio_writer.cc (right):

http://codereview.chromium.org/10535153/diff/3008/remoting/protocol/audio_wri...
remoting/protocol/audio_writer.cc:17: // TODO(kxing) make this correct
Sorry, I meant for the comment to read: "Support different configurations."

On 2012/06/14 17:53:17, sergeyu wrote:
> nit: comment is not clear. Should it be "Implement this."?
> Also please add NOTIMPLEMENTED() here.

http://codereview.chromium.org/10535153/diff/3008/remoting/protocol/audio_wri...
remoting/protocol/audio_writer.cc:17: // TODO(kxing) make this correct
On 2012/06/14 17:53:17, sergeyu wrote:
> nit: should be formatted like this:
>  TODO(kxing): Make this correct.

Done.

http://codereview.chromium.org/10535153/diff/3008/remoting/protocol/audio_wri...
File remoting/protocol/audio_writer.h (right):

http://codereview.chromium.org/10535153/diff/3008/remoting/protocol/audio_wri...
remoting/protocol/audio_writer.h:22: class AudioWriter : public AudioStub {
On 2012/06/14 17:53:17, sergeyu wrote:
> Don't really need this to be an abstract interface. We needed it for
VideoWriter
> in the past (when we had and RTP video writer), not potentially
> ProtobufVideoWriter can be merged with VideoWriter. I suggest you just move
all
> code you have in ProtobufAudioWriter to AudioWriter.

Done.

http://codereview.chromium.org/10535153/diff/3008/remoting/protocol/protobuf_...
File remoting/protocol/protobuf_audio_writer.h (right):

http://codereview.chromium.org/10535153/diff/3008/remoting/protocol/protobuf_...
remoting/protocol/protobuf_audio_writer.h:47: // TODO(kxing): Remove |channel_|
and let |buffered_writer_| own it.
On 2012/06/14 17:50:02, garykac wrote:
> Can you create a bug for this and then do it (for audio/video writers) in a
> follow-up cl?

Done.

http://codereview.chromium.org/10535153/diff/3008/remoting/protocol/protobuf_...
remoting/protocol/protobuf_audio_writer.h:47: // TODO(kxing): Remove |channel_|
and let |buffered_writer_| own it.
On 2012/06/14 17:53:17, sergeyu wrote:
> Here and in all other places - don't change the name when you copy TODO. After
> all its still the same TODO.

Done.

[Sergey Ulanov, 2012-06-14 23:30:16]

LGTM after my nits are addressed

http://codereview.chromium.org/10535153/diff/8002/remoting/protocol/audio_wri...
File remoting/protocol/audio_writer.h (right):

http://codereview.chromium.org/10535153/diff/8002/remoting/protocol/audio_wri...
remoting/protocol/audio_writer.h:5: // AudioWriter is a generic interface used
by ConnectionToClient to
Update the comment please. It's not a generic interface anymore

http://codereview.chromium.org/10535153/diff/8002/remoting/protocol/audio_wri...
remoting/protocol/audio_writer.h:58: DISALLOW_COPY_AND_ASSIGN(AudioWriter);
This should be at the end of the class definition.

[kxing, 2012-06-14 23:45:15]

http://codereview.chromium.org/10535153/diff/8002/remoting/protocol/audio_wri...
File remoting/protocol/audio_writer.h (right):

http://codereview.chromium.org/10535153/diff/8002/remoting/protocol/audio_wri...
remoting/protocol/audio_writer.h:5: // AudioWriter is a generic interface used
by ConnectionToClient to
On 2012/06/14 23:30:17, sergeyu wrote:
> Update the comment please. It's not a generic interface anymore

Done.

http://codereview.chromium.org/10535153/diff/8002/remoting/protocol/audio_wri...
remoting/protocol/audio_writer.h:58: DISALLOW_COPY_AND_ASSIGN(AudioWriter);
On 2012/06/14 23:30:17, sergeyu wrote:
> This should be at the end of the class definition.

Done.

[commit-bot: I haz the power, 2012-06-14 23:46:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kxing@chromium.org/10535153/7003

[commit-bot: I haz the power, 2012-06-15 00:58:39]

Change committed as 142297