Refactor ZLib and Verbatim encoders.

Refactor ZLib and Verbatim encoders.

Added EncoderRowBased that implements encoding for both ZLib and Verbatim.

BUG=None
TEST=None

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

[Alpha Left Google, 2010-12-02 19:39:54]

http://codereview.chromium.org/5382008/diff/1/remoting/base/compressor.h
File remoting/base/compressor.h (right):

http://codereview.chromium.org/5382008/diff/1/remoting/base/compressor.h#newc...
remoting/base/compressor.h:53: virtual void Reset() = 0;
Add comments for this method.

http://codereview.chromium.org/5382008/diff/1/remoting/base/encoder_row_based.cc
File remoting/base/encoder_row_based.cc (left):

http://codereview.chromium.org/5382008/diff/1/remoting/base/encoder_row_based...
remoting/base/encoder_row_based.cc:78: flush = Compressor::CompressorSyncFlush;
It is fine that we don't use sync flush but we need to add comments about how
use zlib on both encode and decode side to state clear why sync flush is not
used.

http://codereview.chromium.org/5382008/diff/1/remoting/base/encoder_row_based.h
File remoting/base/encoder_row_based.h (right):

http://codereview.chromium.org/5382008/diff/1/remoting/base/encoder_row_based...
remoting/base/encoder_row_based.h:20: class EncoderRowBased : public Encoder {
Add comments about the compressor used here. Also describe how we use the zlib
compressor since sync flush is not used.

http://codereview.chromium.org/5382008/diff/1/remoting/host/capturer_linux.cc
File remoting/host/capturer_linux.cc (right):

http://codereview.chromium.org/5382008/diff/1/remoting/host/capturer_linux.cc...
remoting/host/capturer_linux.cc:224: //    capture_fullscreen_ = false;
commented out code?

http://codereview.chromium.org/5382008/diff/1/remoting/remoting.gyp
File remoting/remoting.gyp (right):

http://codereview.chromium.org/5382008/diff/1/remoting/remoting.gyp#newcode461
remoting/remoting.gyp:461: # BUG57351       
'base/encoder_row_based_unittest.cc',
enable this test case?

[garykac, 2010-12-02 19:51:31]

2 nits + 1 question about commented-out code

http://codereview.chromium.org/5382008/diff/1/remoting/base/encoder_row_based.h
File remoting/base/encoder_row_based.h (right):

http://codereview.chromium.org/5382008/diff/1/remoting/base/encoder_row_based...
remoting/base/encoder_row_based.h:39: // Marks a packets as the first in a
series of rectangle updates.
Not part of your CL, but this should be "Marks a packet"

http://codereview.chromium.org/5382008/diff/1/remoting/host/capturer_linux.cc
File remoting/host/capturer_linux.cc (right):

http://codereview.chromium.org/5382008/diff/1/remoting/host/capturer_linux.cc...
remoting/host/capturer_linux.cc:213: LOG(WARNING) << "Damage receved for rect at
("
received

http://codereview.chromium.org/5382008/diff/1/remoting/host/capturer_linux.cc...
remoting/host/capturer_linux.cc:224: //    capture_fullscreen_ = false;
Why is this commented out?  Does it need a TODO?

[Sergey Ulanov, 2010-12-03 00:17:21]

http://codereview.chromium.org/5382008/diff/1/remoting/base/compressor.h
File remoting/base/compressor.h (right):

http://codereview.chromium.org/5382008/diff/1/remoting/base/compressor.h#newc...
remoting/base/compressor.h:53: virtual void Reset() = 0;
On 2010/12/02 19:39:55, Alpha wrote:
> Add comments for this method.

Done.

http://codereview.chromium.org/5382008/diff/1/remoting/base/encoder_row_based.cc
File remoting/base/encoder_row_based.cc (left):

http://codereview.chromium.org/5382008/diff/1/remoting/base/encoder_row_based...
remoting/base/encoder_row_based.cc:78: flush = Compressor::CompressorSyncFlush;
On 2010/12/02 19:39:55, Alpha wrote:
> It is fine that we don't use sync flush but we need to add comments about how
> use zlib on both encode and decode side to state clear why sync flush is not
> used.
> 
Added comments about it in the header.

http://codereview.chromium.org/5382008/diff/1/remoting/base/encoder_row_based.h
File remoting/base/encoder_row_based.h (right):

http://codereview.chromium.org/5382008/diff/1/remoting/base/encoder_row_based...
remoting/base/encoder_row_based.h:20: class EncoderRowBased : public Encoder {
On 2010/12/02 19:39:55, Alpha wrote:
> Add comments about the compressor used here. Also describe how we use the zlib
> compressor since sync flush is not used.

Done.

http://codereview.chromium.org/5382008/diff/1/remoting/base/encoder_row_based...
remoting/base/encoder_row_based.h:39: // Marks a packets as the first in a
series of rectangle updates.
On 2010/12/02 19:51:31, garykac wrote:
> Not part of your CL, but this should be "Marks a packet"

Done.

http://codereview.chromium.org/5382008/diff/1/remoting/host/capturer_linux.cc
File remoting/host/capturer_linux.cc (right):

http://codereview.chromium.org/5382008/diff/1/remoting/host/capturer_linux.cc...
remoting/host/capturer_linux.cc:224: //    capture_fullscreen_ = false;
On 2010/12/02 19:39:55, Alpha wrote:
> commented out code?
This wasn't meant to be part of this CL (Linux capturer is currently broken, so
I had to comment this line to workaround it). Reverted now

http://codereview.chromium.org/5382008/diff/1/remoting/remoting.gyp
File remoting/remoting.gyp (right):

http://codereview.chromium.org/5382008/diff/1/remoting/remoting.gyp#newcode461
remoting/remoting.gyp:461: # BUG57351       
'base/encoder_row_based_unittest.cc',
On 2010/12/02 19:39:55, Alpha wrote:
> enable this test case?
This test requires codec_test.cc, and the code in codec_test.cc is very stale. I
will fix it in a separate CL.

[Alpha Left Google, 2010-12-06 22:14:56]

LGTM.