Introduced ChunkedByteBuffer class which will be needed by next speech recognition CLs.

Introduced ChunkedByteBuffer class which will be required by next speech recognition CLs (2.x).


BUG=116954
TEST=content_unittest (ChunkedByteBufferTest.BasicTest)

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

[Satish, 2012-04-23 11:01:44]

http://codereview.chromium.org/10123008/diff/1/content/browser/speech/chunked...
File content/browser/speech/chunked_byte_buffer.cc (right):

http://codereview.chromium.org/10123008/diff/1/content/browser/speech/chunked...
content/browser/speech/chunked_byte_buffer.cc:21: buffer_.reserve(buffer_.size()
+ length);
I haven't verified it but can we use
  buffer_.insert(buffer_.end(), start, start + length);
?

http://codereview.chromium.org/10123008/diff/1/content/browser/speech/chunked...
content/browser/speech/chunked_byte_buffer.cc:34: COMPILE_ASSERT(sizeof(size_t)
>= kLengthFieldSize,
move this outside to line 13

http://codereview.chromium.org/10123008/diff/1/content/browser/speech/chunked...
File content/browser/speech/chunked_byte_buffer.h (right):

http://codereview.chromium.org/10123008/diff/1/content/browser/speech/chunked...
content/browser/speech/chunked_byte_buffer.h:25: class ChunkedByteBuffer {
A general comment about this class - you are inserting all data into a single
byte vector, readers read the first chunk & erase (which shifts remaining data
to the start) and so on. I think another way to do this would be to keep a
vector of chunks, i.e. vector<vector<byte>> and an 'in-progress' chunk
separately. Append will add content to the 'in-progress' chunk and once it
receives all data move it into the chunk vector. Get/Read methods will always
read the chunk vector so the in-progress chunk would be left out of it. I think
this way you have less memory copies and the GetNextChunk() method can return
the vector to the caller and remove it from this list instead of the current
ReadChunk & RemoveChunk methods

http://codereview.chromium.org/10123008/diff/1/content/browser/speech/chunked...
content/browser/speech/chunked_byte_buffer.h:33: // Appends bytes contained into
the |string| to the buffer.
into -> in

http://codereview.chromium.org/10123008/diff/1/content/browser/speech/chunked...
content/browser/speech/chunked_byte_buffer.h:38: bool CanReadChunk(size_t*
chunk_length) const;
perhaps simpler to combine these two methods into
  size_t GetNextChunkSize() const;
which returns 0 if there are no chunks and a non-zero value if there is data?

http://codereview.chromium.org/10123008/diff/1/content/browser/speech/chunked...
content/browser/speech/chunked_byte_buffer.h:48: bool ReadChunk(const uint8**
start_ptr, size_t* length) const;
If you like the above idea, could rewrite this as
  size_t GetNextChunk(const uint8** start_ptr);

http://codereview.chromium.org/10123008/diff/1/content/browser/speech/chunked...
content/browser/speech/chunked_byte_buffer.h:51: // Fails (NOTREACHED) if there
is not enough data for an entire chunk.
this is an implementation detail and shouldn't be mentioned here

http://codereview.chromium.org/10123008/diff/1/content/browser/speech/chunked...
content/browser/speech/chunked_byte_buffer.h:58: size_t Size() const { return
buffer_.size(); }
rename to GetSize() so the method name is the action it does than the value it
returns

http://codereview.chromium.org/10123008/diff/1/content/browser/speech/chunked...
content/browser/speech/chunked_byte_buffer.h:62: static const int
kLengthFieldSize;
I'd prefer these two to be moved to the anonymous namespace of the .cc file as
they aren't accessing the methods/members  of this class at all

[Primiano Tucci (use gerrit), 2012-04-23 17:13:57]

http://codereview.chromium.org/10123008/diff/1/content/browser/speech/chunked...
File content/browser/speech/chunked_byte_buffer.cc (right):

http://codereview.chromium.org/10123008/diff/1/content/browser/speech/chunked...
content/browser/speech/chunked_byte_buffer.cc:21: buffer_.reserve(buffer_.size()
+ length);
On 2012/04/23 11:01:44, Satish wrote:
> I haven't verified it but can we use
>   buffer_.insert(buffer_.end(), start, start + length);
> ?

Done.

http://codereview.chromium.org/10123008/diff/1/content/browser/speech/chunked...
content/browser/speech/chunked_byte_buffer.cc:34: COMPILE_ASSERT(sizeof(size_t)
>= kLengthFieldSize,
On 2012/04/23 11:01:44, Satish wrote:
> move this outside to line 13

Done.

http://codereview.chromium.org/10123008/diff/1/content/browser/speech/chunked...
File content/browser/speech/chunked_byte_buffer.h (right):

http://codereview.chromium.org/10123008/diff/1/content/browser/speech/chunked...
content/browser/speech/chunked_byte_buffer.h:25: class ChunkedByteBuffer {
On 2012/04/23 11:01:44, Satish wrote:
> A general comment about this class - you are inserting all data into a single
> byte vector, readers read the first chunk & erase (which shifts remaining data
> to the start) and so on. I think another way to do this would be to keep a
> vector of chunks, i.e. vector<vector<byte>> and an 'in-progress' chunk
> separately. Append will add content to the 'in-progress' chunk and once it
> receives all data move it into the chunk vector. Get/Read methods will always
> read the chunk vector so the in-progress chunk would be left out of it. I
think
> this way you have less memory copies and the GetNextChunk() method can return
> the vector to the caller and remove it from this list instead of the current
> ReadChunk & RemoveChunk methods

Hmm I like it. it will increase a bit the complexity of this class (since no
assumption can be made on the granularity and boundaries of Append-ed data), but
I think it will be worth doing it.

http://codereview.chromium.org/10123008/diff/1/content/browser/speech/chunked...
content/browser/speech/chunked_byte_buffer.h:33: // Appends bytes contained into
the |string| to the buffer.
On 2012/04/23 11:01:44, Satish wrote:
> into -> in

Done.

http://codereview.chromium.org/10123008/diff/1/content/browser/speech/chunked...
content/browser/speech/chunked_byte_buffer.h:38: bool CanReadChunk(size_t*
chunk_length) const;
On 2012/04/23 11:01:44, Satish wrote:
> perhaps simpler to combine these two methods into
>   size_t GetNextChunkSize() const;
> which returns 0 if there are no chunks and a non-zero value if there is data?

I originally adopted this pattern in order to contemplate the sequence 00 00 00
00 which leads into a 0 length chunk. However, I agree that it does not make a
lot of sense, so I "banned" the zero-length chunks and adopted the pattern you
suggested.

http://codereview.chromium.org/10123008/diff/1/content/browser/speech/chunked...
content/browser/speech/chunked_byte_buffer.h:48: bool ReadChunk(const uint8**
start_ptr, size_t* length) const;
On 2012/04/23 11:01:44, Satish wrote:
> If you like the above idea, could rewrite this as
>   size_t GetNextChunk(const uint8** start_ptr);

Done.

http://codereview.chromium.org/10123008/diff/1/content/browser/speech/chunked...
content/browser/speech/chunked_byte_buffer.h:51: // Fails (NOTREACHED) if there
is not enough data for an entire chunk.
On 2012/04/23 11:01:44, Satish wrote:
> this is an implementation detail and shouldn't be mentioned here

Done.

http://codereview.chromium.org/10123008/diff/1/content/browser/speech/chunked...
content/browser/speech/chunked_byte_buffer.h:58: size_t Size() const { return
buffer_.size(); }
On 2012/04/23 11:01:44, Satish wrote:
> rename to GetSize() so the method name is the action it does than the value it
> returns
Renamed to GetTotalLength(), since it is more clear.

http://codereview.chromium.org/10123008/diff/1/content/browser/speech/chunked...
content/browser/speech/chunked_byte_buffer.h:62: static const int
kLengthFieldSize;
On 2012/04/23 11:01:44, Satish wrote:
> I'd prefer these two to be moved to the anonymous namespace of the .cc file as
> they aren't accessing the methods/members  of this class at all

Done.

[Satish, 2012-04-24 09:10:13]

Thanks, looks better. Some additional comments..

http://codereview.chromium.org/10123008/diff/5001/content/browser/speech/chun...
File content/browser/speech/chunked_byte_buffer.cc (right):

http://codereview.chromium.org/10123008/diff/5001/content/browser/speech/chun...
content/browser/speech/chunked_byte_buffer.cc:119: uint32
ChunkedByteBuffer::ReadBigEndian32(const uint8* buffer) {
Move this out to the anonymous namespace at the top of this file as it doesn't
depend on this class and is used only by 'struct Chunk'

http://codereview.chromium.org/10123008/diff/5001/content/browser/speech/chun...
File content/browser/speech/chunked_byte_buffer.h (right):

http://codereview.chromium.org/10123008/diff/5001/content/browser/speech/chun...
content/browser/speech/chunked_byte_buffer.h:47: size_t GetNextChunk(const
uint8** start_ptr) const;
since this class doesn't allow iterating through all available chunks and the
caller has to always pair a GetNextChunk with a RemoveNextChunk, I think we
could use a simpler interface by providing a 'Chunk* PopChunk()' method which
removes and returns the first entry, transferring ownership to the caller. Chunk
can be changed to a class, its data members hidden if required and will provide
2 methods, 'uint8* GetData()' and 'size_t GetDataLength()' for this to work.

http://codereview.chromium.org/10123008/diff/5001/content/browser/speech/chun...
content/browser/speech/chunked_byte_buffer.h:62: Chunk() {}
ctor/dtor shouldn't be inlined, move these to the .cc file

http://codereview.chromium.org/10123008/diff/5001/content/browser/speech/chun...
content/browser/speech/chunked_byte_buffer.h:67: inline size_t
ExpectedContentLength() const;
remove 'inline'

[Primiano Tucci (use gerrit), 2012-04-24 13:46:04]

Refactored and added the ByteBufferChunk class for returning chunks in a more
elegant way, transferring the ownership upon pop.

[Primiano Tucci (use gerrit), 2012-04-24 14:29:42]

Re-refactored, in order to return a std::vector<uint8>.

[hans, 2012-04-24 14:48:08]

http://codereview.chromium.org/10123008/diff/16001/content/browser/speech/chu...
File content/browser/speech/chunked_byte_buffer.h (right):

http://codereview.chromium.org/10123008/diff/16001/content/browser/speech/chu...
content/browser/speech/chunked_byte_buffer.h:38: // Appends bytes contained into
the |string| to the buffer.
s/into/in/ :)

[Satish, 2012-04-24 15:05:33]

lgtm

http://codereview.chromium.org/10123008/diff/15002/content/browser/speech/chu...
File content/browser/speech/chunked_byte_buffer.cc (right):

http://codereview.chromium.org/10123008/diff/15002/content/browser/speech/chu...
content/browser/speech/chunked_byte_buffer.cc:32: : partial_chunk_(new Chunk()),
indent by 4 spaces

http://codereview.chromium.org/10123008/diff/15002/content/browser/speech/chu...
content/browser/speech/chunked_byte_buffer.cc:101: const std::vector<uint8>&
ChunkedByteBuffer::GetFirstChunk() const {
since we have PopChunk now, perhaps this can be removed.

[Primiano Tucci (use gerrit), 2012-04-24 15:24:41]

http://codereview.chromium.org/10123008/diff/15002/content/browser/speech/chu...
File content/browser/speech/chunked_byte_buffer.cc (right):

http://codereview.chromium.org/10123008/diff/15002/content/browser/speech/chu...
content/browser/speech/chunked_byte_buffer.cc:32: : partial_chunk_(new Chunk()),
On 2012/04/24 15:05:33, Satish wrote:
> indent by 4 spaces

Done.

http://codereview.chromium.org/10123008/diff/15002/content/browser/speech/chu...
content/browser/speech/chunked_byte_buffer.cc:101: const std::vector<uint8>&
ChunkedByteBuffer::GetFirstChunk() const {
On 2012/04/24 15:05:33, Satish wrote:
> since we have PopChunk now, perhaps this can be removed.

Done.

[jam, 2012-04-24 15:58:18]

gypi files lgtm

[commit-bot: I haz the power, 2012-04-24 16:59:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/primiano@chromium.org/10123008/25003

[commit-bot: I haz the power, 2012-04-24 18:00:50]

Try job failure for 10123008-25003 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-04-25 07:51:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/primiano@chromium.org/10123008/37001

[commit-bot: I haz the power, 2012-04-25 09:47:16]

Change committed as 133885