Unit test for Encoder / Decoder for remoting

Unit test for Encoder / Decoder for remoting

Unit test utils for Encoder and Decoder. 

The tests will cover the state and messages sent out of the Encoder and
Decoder are in correct order.

It also make sure Decoded output matches input to the Encoder.

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

[dmac, 2010-07-19 17:55:44]

http://codereview.chromium.org/3011010/diff/1/2
File remoting/base/codec_test.cc (right):

http://codereview.chromium.org/3011010/diff/1/2#newcode42
remoting/base/codec_test.cc:42: void ReceivedState(int state) {
instead of int, use  EncodingState?

http://codereview.chromium.org/3011010/diff/1/2#newcode61
remoting/base/codec_test.cc:61: int next_state_;
use EncodingState?

http://codereview.chromium.org/3011010/diff/1/2#newcode69
remoting/base/codec_test.cc:69: EncoderMessageTester() :
colon should be on next line. See C++ style guide.

http://codereview.chromium.org/3011010/diff/1/2#newcode120
remoting/base/codec_test.cc:120: void SetStrict(bool strict) {
should be set_strict

http://codereview.chromium.org/3011010/diff/1/2#newcode147
remoting/base/codec_test.cc:147: DecoderTester(Decoder* decoder) :
strict_(false),
match style guidelines on c'tor list.

http://codereview.chromium.org/3011010/diff/1/2#newcode157
remoting/base/codec_test.cc:157: if (message->has_begin_update_stream()) {
weird owndership for HostMessage. Can we clean this up?

http://codereview.chromium.org/3011010/diff/1/2#newcode176
remoting/base/codec_test.cc:176: strict_ = strict;
set_strict

http://codereview.chromium.org/3011010/diff/1/2#newcode194
remoting/base/codec_test.cc:194: for (size_t i = 0; i < update_rects_.size();
++i) {
use c++ iterator?

http://codereview.chromium.org/3011010/diff/1/2#newcode224
remoting/base/codec_test.cc:224: bool decode_done_;
DISALLOW_COPY_AND_ASSIGN();

http://codereview.chromium.org/3011010/diff/1/2#newcode271
remoting/base/codec_test.cc:271: void SetDecoderTester(DecoderTester*
decoder_tester) {
set_decoder_tester

http://codereview.chromium.org/3011010/diff/1/3
File remoting/base/codec_test.h (right):

http://codereview.chromium.org/3011010/diff/1/3#newcode18
remoting/base/codec_test.h:18: // Returns the encoding data prepared.
what does prepared mean? can you expand the comment?

http://codereview.chromium.org/3011010/diff/1/3#newcode29
remoting/base/codec_test.h:29: // Generate test dat and test the encoder and
decoder pair.
s/dat/data/

http://codereview.chromium.org/3011010/diff/1/3#newcode31
remoting/base/codec_test.h:31: // If |strict| is set to true, this routine will
make the update rects
fix up this comment. It doesn't make sense.

http://codereview.chromium.org/3011010/diff/1/4
File remoting/base/decoder_verbatim.h (right):

http://codereview.chromium.org/3011010/diff/1/4#newcode24
remoting/base/decoder_verbatim.h:24: void SetReverseRows(bool reverse) {
reverse_rows_ = reverse; }
set_reverse_rows

[Alpha Left Google, 2010-07-20 22:40:00]

http://codereview.chromium.org/3011010/diff/1/2
File remoting/base/codec_test.cc (right):

http://codereview.chromium.org/3011010/diff/1/2#newcode42
remoting/base/codec_test.cc:42: void ReceivedState(int state) {
On 2010/07/19 17:55:44, dmac wrote:
> instead of int, use  EncodingState?

Done.

http://codereview.chromium.org/3011010/diff/1/2#newcode61
remoting/base/codec_test.cc:61: int next_state_;
On 2010/07/19 17:55:44, dmac wrote:
> use EncodingState?

Done.

http://codereview.chromium.org/3011010/diff/1/2#newcode69
remoting/base/codec_test.cc:69: EncoderMessageTester() :
On 2010/07/19 17:55:44, dmac wrote:
> colon should be on next line. See C++ style guide.

Done.

http://codereview.chromium.org/3011010/diff/1/2#newcode120
remoting/base/codec_test.cc:120: void SetStrict(bool strict) {
On 2010/07/19 17:55:44, dmac wrote:
> should be set_strict

Done.

http://codereview.chromium.org/3011010/diff/1/2#newcode147
remoting/base/codec_test.cc:147: DecoderTester(Decoder* decoder) :
strict_(false),
On 2010/07/19 17:55:44, dmac wrote:
> match style guidelines on c'tor list.

Done.

http://codereview.chromium.org/3011010/diff/1/2#newcode157
remoting/base/codec_test.cc:157: if (message->has_begin_update_stream()) {
On 2010/07/19 17:55:44, dmac wrote:
> weird owndership for HostMessage. Can we clean this up?

The semantics is that who uses it deletes it. So that's why the decoder deletes
the message. I think the easiest way is to enclose the message in a refcount ptr
but it seems unnecessary in this case.. I think we should clean this up in a
separate patch.

http://codereview.chromium.org/3011010/diff/1/2#newcode176
remoting/base/codec_test.cc:176: strict_ = strict;
On 2010/07/19 17:55:44, dmac wrote:
> set_strict

Done.

http://codereview.chromium.org/3011010/diff/1/2#newcode194
remoting/base/codec_test.cc:194: for (size_t i = 0; i < update_rects_.size();
++i) {
On 2010/07/19 17:55:44, dmac wrote:
> use c++ iterator?

I prefer using index as using iterator for vector is a bit clumsy.

http://codereview.chromium.org/3011010/diff/1/2#newcode224
remoting/base/codec_test.cc:224: bool decode_done_;
On 2010/07/19 17:55:44, dmac wrote:
> DISALLOW_COPY_AND_ASSIGN();

Done.

http://codereview.chromium.org/3011010/diff/1/2#newcode271
remoting/base/codec_test.cc:271: void SetDecoderTester(DecoderTester*
decoder_tester) {
On 2010/07/19 17:55:44, dmac wrote:
> set_decoder_tester

Done.

http://codereview.chromium.org/3011010/diff/1/3
File remoting/base/codec_test.h (right):

http://codereview.chromium.org/3011010/diff/1/3#newcode18
remoting/base/codec_test.h:18: // Returns the encoding data prepared.
On 2010/07/19 17:55:44, dmac wrote:
> what does prepared mean? can you expand the comment?

Done.

http://codereview.chromium.org/3011010/diff/1/3#newcode29
remoting/base/codec_test.h:29: // Generate test dat and test the encoder and
decoder pair.
On 2010/07/19 17:55:44, dmac wrote:
> s/dat/data/

Done.

http://codereview.chromium.org/3011010/diff/1/3#newcode31
remoting/base/codec_test.h:31: // If |strict| is set to true, this routine will
make the update rects
On 2010/07/19 17:55:44, dmac wrote:
> fix up this comment. It doesn't make sense.

Done.

http://codereview.chromium.org/3011010/diff/1/4
File remoting/base/decoder_verbatim.h (right):

http://codereview.chromium.org/3011010/diff/1/4#newcode24
remoting/base/decoder_verbatim.h:24: void SetReverseRows(bool reverse) {
reverse_rows_ = reverse; }
On 2010/07/19 17:55:44, dmac wrote:
> set_reverse_rows

Done.

[Alpha Left Google, 2010-07-23 00:39:44]

ping.

[dmac, 2010-07-23 05:32:34]

On 2010/07/23 00:39:44, Alpha wrote:
> ping.

LGTM

[Timur Iskhodzhanov, 2010-07-24 12:11:17]

This CL introduced mismatched new[]/delete
http://code.google.com/p/chromium/issues/detail?id=50126
which I've fixed by http://codereview.chromium.org/3051012
On 2010/07/23 05:32:34, dmac wrote:
> On 2010/07/23 00:39:44, Alpha wrote:
> > ping.
> 
> LGTM