net: Introduce SeekableIOBuffer and clean up HttpStreamParser.

net/base/io_buffer.h

Index: net/base/io_buffer.h
diff --git a/net/base/io_buffer.h b/net/base/io_buffer.h
index 3a04b6a969cfe3f5ee6da2aad1f15fff0514c4e1..1c2f236ccf9fe9630194d80d084ed4d81471abbc 100644
--- a/net/base/io_buffer.h
+++ b/net/base/io_buffer.h
@@ -126,6 +126,21 @@ class NET_EXPORT StringIOBuffer : public IOBuffer {
 
 // This version wraps an existing IOBuffer and provides convenient functions
 // to progressively read all the data.
+//
+// DrainableIOBuffer is useful when you have an IOBuffer that contains data
+// to be written progressively, and Write() function takes an IOBuffer rather
+// than const char*. DrainableIOBuffer can be used as follows:
+//
+// // payload is the IOBuffer containing the data to be written.
+// buf = new DrainableIOBuffer(payload, payload_size);
+//
+// while (buf->BytesRemaining() > 0) {
+//   // Write() takes an IOBuffer. If it takes const char*, we could
+//   // simply use the regular IOBuffer like payload->data() + offset.
+//   int bytes_written = Write(buf, buf->BytesRemaining());
+//   buf->DidConsume(bytes_written);
+// }
+//
 class NET_EXPORT DrainableIOBuffer : public IOBuffer {
  public:
   DrainableIOBuffer(IOBuffer* base, int size);
@@ -154,7 +169,123 @@ class NET_EXPORT DrainableIOBuffer : public IOBuffer {
   int used_;
 };
 
+// Similar to DrainableIOBuffer(), but this version comes with its own
+// storage, so you don't need to create a separate IOBuffer. DidAppend(),

[rvargas (doing something else), 2012/02/03 23:13:34]

I would prefer not having references to bad patterns here (or at least not that
much emphasis). This makes me feel like if allocating a separate buffer was OK
in the first place, instead of forcing a square peg into a round hole, and the
new class was just making things a little more convenient.

I think it would be better to say that DrainableIOBuffer is meant for code that
lives in the middle layer of a stack that uses IOBuffers (so it receives an
IOBuffer and it has to provide an IOBuffer to the lower layer) (and you added a
comment saying that), while this class is meant for code that lives at the top
of a particular stack, so it does not receive an IOBuffer from anybody else, and
it can be the ultimate owner of the buffer provided to a lower layer.

This means I would remove the loop showing the bad pattern.

[satorux1, 2012/02/03 23:46:11]

Good point. Reworked and simplified the comment per your suggestion.

+// capacity(), and Clear() are also provided, so that the SeekableIOBuffer is
+// reusable.
+//
+// SeekableIOBuffer is useful when you want to avoid repeated allocations of
+// DrainableIOBuffer. Consider this case:
+//
+// scoped_refptr<IOBuffer> buf = new IOBufferWithSize(1024);
+// while (!some_reader->IsEOF()) {
+//   int bytes_read = some_reader->Read(buf, buf->size());
+//   scoped_refptr<DrainableIOBuffer> drainable(buf, num_bytes);  // HERE
+//   while (!drainable->BytesRemaining()) {
+//      int bytes_written = Write(drainable, drainable->BytesRemaining());
+//      drainable->DidConsume(bytes_written);
+//   }
+// }
+//
+// As shown, DrainableIOBuffer is repeatedly allocated. With
+// SeekableIOBuffer, the extra allocations can be eliminated:
+//
+// scoped_refptr<SeekableIOBuffer> seekable = new SeekableIOBuffer(1024);
+// while (!some_reader->IsEOF()) {
+//   seekable->Clear();  // Clear before reuse.
+//   int bytes_read = some_reader->Read(seekable, seekable->capacity());
+//   seekable->DidAppend(bytes_read);
+//   while (!seekable->BytesRemaining()) {
+//      int bytes_written = Write(seekable, seekable->BytesRemaining());
+//      seekable->DidConsume(bytes_written);
+//   }
+// }
+//
+// General example:
+//
+// scoped_refptr<SeekableIOBuffer> buf = new SeekableIOBuffer(1024);
+// // capacity() == 1024. size() == BytesRemaining == BytesConsumed() == 0.
+// // data() points to the beginning of the buffer.
+//
+// // Read() takes an IOBuffer.
+// int bytes_read = some_reader->Read(buf, buf->capacity());
+// buf->DidAppend(bytes_read);
+// // size() == BytesRemaining() == bytes_read. data() is unaffected.
+//
+// while (buf->BytesRemaining() > 0) {
+//   // Write() takes an IOBuffer. If it takes const char*, we could
+///  // simply use the regular IOBuffer like buf->data() + offset.
+//   int bytes_written = Write(buf, buf->BytesRemaining());
+//   buf->DidConsume(bytes_written);
+// }
+// // BytesRemaining() == 0. BytesConsumed() == size().
+// // data() points to the end of the comsumed bytes (exclusive).
+//
+// // If you want to reuse the buffer, be sure to clear the buffer.
+// buf->Clear();
+// // size() == BytesRemaining() == BytesConsumed() == 0.
+// // data() points to the beginning of the buffer.
+//
+class NET_EXPORT SeekableIOBuffer : public IOBuffer {
+ public:
+  explicit SeekableIOBuffer(int capacity);
+
+  // DidConsume() changes the |data_| pointer so that |data_| always points
+  // to the first unconsumed byte.
+  void DidConsume(int bytes);
+
+  // Returns the number of unconsumed bytes.
+  // GUARANTEES: 0 <= BytesRemaining() <= size().

[rvargas (doing something else), 2012/02/03 23:13:34]

nit: we have always used comments that are in regular prose instead of a list of
any kind (arguments, returns, preconditions etc). I suggest following that style
here: if you think something is not obvious and should be spelled out, just
mention it.

(and that is to say that I would consider this line obvious, and just cognitive
burden when I'm reading it).

[satorux1, 2012/02/03 23:46:11]

Removed.

+  int BytesRemaining() const;
+
+  // Seeks to an arbitrary point in the buffer. The notion of bytes consumed
+  // and remaining are updated appropriately.
+  // REQUIRES: 0 <= bytes <= size().
+  void SetOffset(int bytes);
+
+  // Marks that |bytes| have been appended. |bytes| is added to |size_|, but

[rvargas (doing something else), 2012/02/03 23:13:34]

nit: "marks" is not that clear. Go either with "called after blah", or what it
actually does: "Modifies the number of bytes..."

[satorux1, 2012/02/03 23:46:11]

Done.

+  // data() is unaffected.
+  // REQUIRES: 0 <= |bytes| + size() <= capacity().
+  void DidAppend(int bytes);
+
+  // Changes the logical size to 0, and the offset to 0.
+  void Clear();
+
+  // Returns the logical size of the buffer (i.e the number of bytes of data
+  // in the buffer).
+  // GUARANTEES: 0 <= size() <= capacity().
+  int size() const { return size_; }
+
+  // Returns the capacity of the buffer. The capacity is the size used when
+  // the object is created.
+  int capacity() const { return capacity_; };
+
+ private:
+  virtual ~SeekableIOBuffer();
+
+  char* real_data_;

[rvargas (doing something else), 2012/02/03 23:13:34]

buffer_start_ ? data_start_ ?

[satorux1, 2012/02/03 23:46:11]

it used to be begin_, but changed to real_data_ per wtc's suggestion. real_data_
makes sense as it always points to the beginning of the allocated memory chunk.

+  int capacity_;
+  int size_;
+  int used_;
+};
+
 // This version provides a resizable buffer and a changeable offset.
+//
+// GrowableIOBuffer is useful when you read data progressively without
+// knowing the total size in advance. GrowableIOBuffer can be used as
+// follows:
+//
+// buf = new GrowableIOBuffer;
+// buf->SetCapacity(1024);  // Initial capacity.
+//
+// while (!some_stream->IsEOF()) {
+//   // Double the capacity if the remaining capacity is empty.
+//   if (buf->RemainingCapacity() == 0)
+//     buf->SetCapacity(buf->capacity() * 2);
+//   int bytes_read = some_stream->Read(buf, buf->RemainingCapacity());
+//   some_stream->set_offset(buf->offset() + bytes_read);

[rvargas (doing something else), 2012/02/03 23:13:34]

This should be buf->set_offset(

[satorux1, 2012/02/03 23:46:11]

Good catch. Done.

+// }
+//
 class NET_EXPORT GrowableIOBuffer : public IOBuffer {
  public:
   GrowableIOBuffer();