Added CompoundBuffer that will be used to store data in the encoding/decoding

Added CompoundBuffer that will be used to store data in the encoding/decoding
pipeline.

BUG=None
TEST=Unittests

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

[Sergey Ulanov, 2010-11-11 23:20:17]

Now I've merged CompoundBuffer with MultipleArrayInputStream as Alpha suggested.

[awong, 2010-11-12 02:40:06]

http://codereview.chromium.org/4779001/diff/3001/remoting/base/compound_buffe...
File remoting/base/compound_buffer.cc (right):

http://codereview.chromium.org/4779001/diff/3001/remoting/base/compound_buffe...
remoting/base/compound_buffer.cc:40: 
remove extra newline

http://codereview.chromium.org/4779001/diff/3001/remoting/base/compound_buffe...
remoting/base/compound_buffer.cc:47: DCHECK_EQ(position_, 0); // Haven't started
reading.
So the API contract is that once you start reading, you aren't supposed to
append anymore?

This should be in the class documentation.  Also, should any of these actually
be CHECKs?

http://codereview.chromium.org/4779001/diff/3001/remoting/base/compound_buffe...
remoting/base/compound_buffer.cc:60: DCHECK_GE(data, buffer->data());
Same comments as earlier.

Also, should there be a CanAddBuffers() method or something?

http://codereview.chromium.org/4779001/diff/3001/remoting/base/compound_buffe...
remoting/base/compound_buffer.cc:86: net::IOBufferWithSize* result = new
net::IOBufferWithSize(total_bytes_);
Are these things refcounted?

http://codereview.chromium.org/4779001/diff/3001/remoting/base/compound_buffer.h
File remoting/base/compound_buffer.h (right):

http://codereview.chromium.org/4779001/diff/3001/remoting/base/compound_buffe...
remoting/base/compound_buffer.h:9: // and assembling them into one buffer on the
receiving side.
Hmm...it feels like we just reimplemented rope. :P

http://www.sgi.com/tech/stl/Rope.html

http://codereview.chromium.org/4779001/diff/3001/remoting/base/compound_buffe...
remoting/base/compound_buffer.h:31: public
google::protobuf::io::ZeroCopyInputStream {
Isn't this just supposed to go in 4-spaces?

http://codereview.chromium.org/4779001/diff/3001/remoting/base/compound_buffe...
remoting/base/compound_buffer.h:38: // Adds new chunk to the buffer. |data| must
point to start of the data
How about rename |data| to |start|?

Also, specify if data_size is the size of the whole buffer, or the size of the
start.

Lastly, why pass in a pointer, and not an int that is the start position?

http://codereview.chromium.org/4779001/diff/3001/remoting/base/compound_buffe...
remoting/base/compound_buffer.h:41: void Append(net::IOBuffer* buffer, const
char* data, int data_size);
overloading is frowned upon by the style guide.  Rename to

Append() and AppendWithStart()?

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Overl...

http://codereview.chromium.org/4779001/diff/3001/remoting/base/compound_buffe...
remoting/base/compound_buffer.h:46: void CopyAndAppend(const char* data, int
data_size);
CopyFrom() below implies a clear.  These do not.

How about:

  AppendCopyOf()
  PrependCopyOf()

?

http://codereview.chromium.org/4779001/diff/3001/remoting/base/compound_buffe...
remoting/base/compound_buffer.h:52: // Assemble all chunks in one buffer.
The word "buffer" has too many meanings in this file.  There are IOBuffers,
CompoundBuffers, the "current buffer."  I'm not sure what this comment means. 
Clarify?

Also, you should state the memory ownership semantics.

http://codereview.chromium.org/4779001/diff/3001/remoting/base/compound_buffe...
remoting/base/compound_buffer.h:54: void Assemble(char* data, int data_size)
const;
How about the names:

OutputAsIOBufferWithSize()
ToByteArray()

?

http://codereview.chromium.org/4779001/diff/3001/remoting/base/compound_buffe...
remoting/base/compound_buffer.h:56: // Drops current content of the buffer, and
initializes it with the interval
How about "Calls Clear(), and then initializes..."

http://codereview.chromium.org/4779001/diff/3001/remoting/base/compound_buffe...
remoting/base/compound_buffer.h:61: // google::protobuf::io::ZeroCopyInputStream
interface.
There seems to be some funky API interactions. Namely, after you start reading
via Next(), etc., some of the functions are no longer allowed to be called (eg.,
Append().

I wonder if it'd be better to create a thin wrapper that adapted the interfaces?

Maybe CompoundBufferInputStream?

http://codereview.chromium.org/4779001/diff/3001/remoting/base/compound_buffe...
remoting/base/compound_buffer.h:80: // Current position for ZeroCopyInputStream.
What happens to all this stuff if you start using things like Assemble, etc.?

This should be documented in the file header.

Maybe also give some sample usage?

http://codereview.chromium.org/4779001/diff/3001/remoting/base/compound_buffe...
File remoting/base/compound_buffer_unittest.cc (right):

http://codereview.chromium.org/4779001/diff/3001/remoting/base/compound_buffe...
remoting/base/compound_buffer_unittest.cc:228: }
We should also test the interactions between the ZeroCopyInputStream methods and
the other methods.

[Sergey Ulanov, 2010-11-13 04:43:39]

http://codereview.chromium.org/4779001/diff/3001/remoting/base/compound_buffe...
File remoting/base/compound_buffer.cc (right):

http://codereview.chromium.org/4779001/diff/3001/remoting/base/compound_buffe...
remoting/base/compound_buffer.cc:40: 
On 2010/11/12 02:40:06, awong wrote:
> remove extra newline

Done.

http://codereview.chromium.org/4779001/diff/3001/remoting/base/compound_buffe...
remoting/base/compound_buffer.cc:47: DCHECK_EQ(position_, 0); // Haven't started
reading.
On 2010/11/12 02:40:06, awong wrote:
> So the API contract is that once you start reading, you aren't supposed to
> append anymore?
> 
> This should be in the class documentation.  Also, should any of these actually
> be CHECKs?

Done.

http://codereview.chromium.org/4779001/diff/3001/remoting/base/compound_buffe...
remoting/base/compound_buffer.cc:60: DCHECK_GE(data, buffer->data());
On 2010/11/12 02:40:06, awong wrote:
> Same comments as earlier.
> 
> Also, should there be a CanAddBuffers() method or something?
Added locked() returns true if content is locked.

http://codereview.chromium.org/4779001/diff/3001/remoting/base/compound_buffe...
remoting/base/compound_buffer.cc:86: net::IOBufferWithSize* result = new
net::IOBufferWithSize(total_bytes_);
On 2010/11/12 02:40:06, awong wrote:
> Are these things refcounted?
Yes, but we just create it, and then the caller will add references for it.
Should be safe, no?

http://codereview.chromium.org/4779001/diff/3001/remoting/base/compound_buffer.h
File remoting/base/compound_buffer.h (right):

http://codereview.chromium.org/4779001/diff/3001/remoting/base/compound_buffe...
remoting/base/compound_buffer.h:9: // and assembling them into one buffer on the
receiving side.
On 2010/11/12 02:40:06, awong wrote:
> Hmm...it feels like we just reimplemented rope. :P
> 
> http://www.sgi.com/tech/stl/Rope.html

hmm, yeah. Except rope doesn't use IOBuffer, so you need memcopy if you need to
put IOBuffer into it.

http://codereview.chromium.org/4779001/diff/3001/remoting/base/compound_buffe...
remoting/base/compound_buffer.h:31: public
google::protobuf::io::ZeroCopyInputStream {
On 2010/11/12 02:40:06, awong wrote:
> Isn't this just supposed to go in 4-spaces?
":" must be on this line. Done.

http://codereview.chromium.org/4779001/diff/3001/remoting/base/compound_buffe...
remoting/base/compound_buffer.h:38: // Adds new chunk to the buffer. |data| must
point to start of the data
On 2010/11/12 02:40:06, awong wrote:
> How about rename |data| to |start|?
Done.

> 
> Also, specify if data_size is the size of the whole buffer, or the size of the
> start.
> 
Done

> Lastly, why pass in a pointer, and not an int that is the start position?
The reason is that IOBuffer()->data() doesn't always point to the same location.
For example DrainableIOBuffer changes data() when you call DidConsume(). This
causes very nasty issues when this method is called with DrainableIOBuffer.

http://codereview.chromium.org/4779001/diff/3001/remoting/base/compound_buffe...
remoting/base/compound_buffer.h:41: void Append(net::IOBuffer* buffer, const
char* data, int data_size);
On 2010/11/12 02:40:06, awong wrote:
> overloading is frowned upon by the style guide.  Rename to
> 
> Append() and AppendWithStart()?
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Overl...

I think this is a legal use of overloading, as both Append methods do the same
thing. No?
CopyAndAppend() does a different, hence a different name.

http://codereview.chromium.org/4779001/diff/3001/remoting/base/compound_buffe...
remoting/base/compound_buffer.h:46: void CopyAndAppend(const char* data, int
data_size);
On 2010/11/12 02:40:06, awong wrote:
> CopyFrom() below implies a clear.  These do not.
> 
> How about:
> 
>   AppendCopyOf()
>   PrependCopyOf()
> 
> ?
> 

Done.

http://codereview.chromium.org/4779001/diff/3001/remoting/base/compound_buffe...
remoting/base/compound_buffer.h:52: // Assemble all chunks in one buffer.
On 2010/11/12 02:40:06, awong wrote:
> The word "buffer" has too many meanings in this file.  There are IOBuffers,
> CompoundBuffers, the "current buffer."  I'm not sure what this comment means. 
> Clarify?
> 
> Also, you should state the memory ownership semantics.

Done.

http://codereview.chromium.org/4779001/diff/3001/remoting/base/compound_buffe...
remoting/base/compound_buffer.h:54: void Assemble(char* data, int data_size)
const;
On 2010/11/12 02:40:06, awong wrote:
> How about the names:
> 
> OutputAsIOBufferWithSize()
Renamed to ToIOBufferWithSize().

> ToByteArray()
Renamed to CopyTo().

> 
> ?

http://codereview.chromium.org/4779001/diff/3001/remoting/base/compound_buffe...
remoting/base/compound_buffer.h:56: // Drops current content of the buffer, and
initializes it with the interval
On 2010/11/12 02:40:06, awong wrote:
> How about "Calls Clear(), and then initializes..."
The fact that it calls Clear() is an implementation detail. Changed this to
"Clears the buffer ..."

http://codereview.chromium.org/4779001/diff/3001/remoting/base/compound_buffe...
remoting/base/compound_buffer.h:61: // google::protobuf::io::ZeroCopyInputStream
interface.
On 2010/11/12 02:40:06, awong wrote:
> There seems to be some funky API interactions. Namely, after you start reading
> via Next(), etc., some of the functions are no longer allowed to be called
(eg.,
> Append().
> 
> I wonder if it'd be better to create a thin wrapper that adapted the
interfaces?
> 
> Maybe CompoundBufferInputStream?
Done. Also added Lock() method to ensure that the content doesn't change after
an CompoundBufferInputStream has been created.

http://codereview.chromium.org/4779001/diff/3001/remoting/base/compound_buffe...
remoting/base/compound_buffer.h:80: // Current position for ZeroCopyInputStream.
On 2010/11/12 02:40:06, awong wrote:
> What happens to all this stuff if you start using things like Assemble, etc.?
> 
> This should be documented in the file header.
> 
> Maybe also give some sample usage?
Moved these to CompoundBufferInputStream

http://codereview.chromium.org/4779001/diff/3001/remoting/base/compound_buffe...
File remoting/base/compound_buffer_unittest.cc (right):

http://codereview.chromium.org/4779001/diff/3001/remoting/base/compound_buffe...
remoting/base/compound_buffer_unittest.cc:228: }
On 2010/11/12 02:40:06, awong wrote:
> We should also test the interactions between the ZeroCopyInputStream methods
and
> the other methods.
Not needed anymore, as I separated CompoundBuffer from
CompoundBufferInputStream.

[awong, 2010-11-16 00:04:57]

LGTM / nits

http://codereview.chromium.org/4779001/diff/3001/remoting/base/compound_buffer.h
File remoting/base/compound_buffer.h (right):

http://codereview.chromium.org/4779001/diff/3001/remoting/base/compound_buffe...
remoting/base/compound_buffer.h:9: // and assembling them into one buffer on the
receiving side.
On 2010/11/13 04:43:39, sergeyu wrote:
> On 2010/11/12 02:40:06, awong wrote:
> > Hmm...it feels like we just reimplemented rope. :P
> > 
> > http://www.sgi.com/tech/stl/Rope.html
> 
> hmm, yeah. Except rope doesn't use IOBuffer, so you need memcopy if you need
to
> put IOBuffer into it.

Yeah...that sucks. :-/

http://codereview.chromium.org/4779001/diff/3001/remoting/base/compound_buffe...
remoting/base/compound_buffer.h:41: void Append(net::IOBuffer* buffer, const
char* data, int data_size);
On 2010/11/13 04:43:39, sergeyu wrote:
> On 2010/11/12 02:40:06, awong wrote:
> > overloading is frowned upon by the style guide.  Rename to
> > 
> > Append() and AppendWithStart()?
> > 
> >
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Overl...
> 
> I think this is a legal use of overloading, as both Append methods do the same
> thing. No?
> CopyAndAppend() does a different, hence a different name.

Yeah...I don't feel fully comfortable with it, but you're right. It's not a
style guide issue.  Let's leave as is for now.

http://codereview.chromium.org/4779001/diff/17001/remoting/base/compound_buff...
File remoting/base/compound_buffer.cc (right):

http://codereview.chromium.org/4779001/diff/17001/remoting/base/compound_buff...
remoting/base/compound_buffer.cc:16: start(start),
ick, that's confusing.  Can we call the parameters something that doesn't shadow
the member variables?

http://codereview.chromium.org/4779001/diff/17001/remoting/base/compound_buff...
File remoting/base/compound_buffer.h (right):

http://codereview.chromium.org/4779001/diff/17001/remoting/base/compound_buff...
remoting/base/compound_buffer.h:13: // a protocol buffer message.
Add comment on threadsafety of this class.

http://codereview.chromium.org/4779001/diff/17001/remoting/base/compound_buff...
remoting/base/compound_buffer.h:57: void Lock();
You mean nothing can be appended to prepend right? You can still change the data
inside it if you pass in an IOBuffer and manipulate it externally...

Change to be more specific that you can't append or prepend?

http://codereview.chromium.org/4779001/diff/17001/remoting/base/compound_buff...
File remoting/base/compound_buffer_unittest.cc (right):

http://codereview.chromium.org/4779001/diff/17001/remoting/base/compound_buff...
remoting/base/compound_buffer_unittest.cc:146: CompoundBuffer* mstream = new
CompoundBuffer();
Why is this called "mstream"?  Is there a more sensible name?

http://codereview.chromium.org/4779001/diff/17001/remoting/protocol/message_d...
File remoting/protocol/message_decoder.h (right):

http://codereview.chromium.org/4779001/diff/17001/remoting/protocol/message_d...
remoting/protocol/message_decoder.h:13: #include "net/base/io_buffer.h"
ordering

http://codereview.chromium.org/4779001/diff/17001/remoting/protocol/message_d...
remoting/protocol/message_decoder.h:14: #include
"google/protobuf/message_lite.h"
Should this be from third_party/protobuf/src?

[Sergey Ulanov, 2010-11-16 01:25:09]

http://codereview.chromium.org/4779001/diff/17001/remoting/base/compound_buff...
File remoting/base/compound_buffer.cc (right):

http://codereview.chromium.org/4779001/diff/17001/remoting/base/compound_buff...
remoting/base/compound_buffer.cc:16: start(start),
On 2010/11/16 00:04:58, awong wrote:
> ick, that's confusing.  Can we call the parameters something that doesn't
shadow
> the member variables?

Done.

http://codereview.chromium.org/4779001/diff/17001/remoting/base/compound_buff...
File remoting/base/compound_buffer.h (right):

http://codereview.chromium.org/4779001/diff/17001/remoting/base/compound_buff...
remoting/base/compound_buffer.h:13: // a protocol buffer message.
On 2010/11/16 00:04:58, awong wrote:
> Add comment on threadsafety of this class.

Done.

http://codereview.chromium.org/4779001/diff/17001/remoting/base/compound_buff...
remoting/base/compound_buffer.h:57: void Lock();
On 2010/11/16 00:04:58, awong wrote:
> You mean nothing can be appended to prepend right? You can still change the
data
> inside it if you pass in an IOBuffer and manipulate it externally...
> 
> Change to be more specific that you can't append or prepend?

Done.

http://codereview.chromium.org/4779001/diff/17001/remoting/base/compound_buff...
File remoting/base/compound_buffer_unittest.cc (right):

http://codereview.chromium.org/4779001/diff/17001/remoting/base/compound_buff...
remoting/base/compound_buffer_unittest.cc:146: CompoundBuffer* mstream = new
CompoundBuffer();
On 2010/11/16 00:04:58, awong wrote:
> Why is this called "mstream"?  Is there a more sensible name?

Done.

http://codereview.chromium.org/4779001/diff/17001/remoting/protocol/message_d...
File remoting/protocol/message_decoder.h (right):

http://codereview.chromium.org/4779001/diff/17001/remoting/protocol/message_d...
remoting/protocol/message_decoder.h:13: #include "net/base/io_buffer.h"
On 2010/11/16 00:04:58, awong wrote:
> ordering

Done.

http://codereview.chromium.org/4779001/diff/17001/remoting/protocol/message_d...
remoting/protocol/message_decoder.h:14: #include
"google/protobuf/message_lite.h"
On 2010/11/16 00:04:58, awong wrote:
> Should this be from third_party/protobuf/src?

Done. Also changed util.h