Factor out chunk encoding logic into HttpStreamParser::EncodeChunk().

Factor out chunk encoding logic into HttpStreamParser::EncodeChunk().

The logic is meaty enough to be factored out.
Add unit tests along the way.

BUG=72001
TEST=add unit tests.


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

[satorux1, 2012-01-17 22:54:48]

adding wtc@

[wtc, 2012-01-18 22:49:24]

Patch Set 1 LGTM.  I suggest some changes below.

http://codereview.chromium.org/9242018/diff/1/net/base/upload_data_stream.h
File net/base/upload_data_stream.h (right):

http://codereview.chromium.org/9242018/diff/1/net/base/upload_data_stream.h#n...
net/base/upload_data_stream.h:33: // with this method and make the interface
cleaner as well with less memmove

method => constant

http://codereview.chromium.org/9242018/diff/1/net/base/upload_data_stream.h#n...
net/base/upload_data_stream.h:35: static const size_t kBufferSize;

Please document this constant.  (I know this is a pre-existing
problem.)

http://codereview.chromium.org/9242018/diff/1/net/http/http_stream_parser.cc
File net/http/http_stream_parser.cc (left):

http://codereview.chromium.org/9242018/diff/1/net/http/http_stream_parser.cc#...
net/http/http_stream_parser.cc:132: chunk_buf_ = new
IOBuffer(request_body_->GetMaxBufferSize() +

I understand why we need kChunkBufferSize.  The alternative
is to store the chunk_buf_ size as a class member
chunk_buf_size_.

It seems more robust to use request_body_->GetMaxBufferSize()
than the UploadDataStream::kBufferSize constant here.

http://codereview.chromium.org/9242018/diff/1/net/http/http_stream_parser.cc
File net/http/http_stream_parser.cc (right):

http://codereview.chromium.org/9242018/diff/1/net/http/http_stream_parser.cc#...
net/http/http_stream_parser.cc:68: // chunk.

Add "The size of the chunk buffer (chunk_buf_)." at the
beginning of this comment block.

http://codereview.chromium.org/9242018/diff/1/net/http/http_stream_parser.cc#...
net/http/http_stream_parser.cc:70: net::UploadDataStream::kBufferSize +

The net:: prefix is not necessary because this is inside the
net namespace.

http://codereview.chromium.org/9242018/diff/1/net/http/http_stream_parser.cc#...
net/http/http_stream_parser.cc:791: cursor += chunk_header.length();
If the standard C library function snprintf() is not banned
in our code base, we can use it to add the header:

  int num_chars = snprintf(output, output_size, "%X\r\n",
                           static_cast<int>(payload.size()));
  cursor += num_chars;

But I am worried that snprintf is considered a problematic
function.

http://codereview.chromium.org/9242018/diff/1/net/http/http_stream_parser.h
File net/http/http_stream_parser.h (right):

http://codereview.chromium.org/9242018/diff/1/net/http/http_stream_parser.h#n...
net/http/http_stream_parser.h:84: // kChunkHeaderFooterSize. Returns -1 if
|output_size| is not large

Instead of -1, you can return a network error code such as
ERR_FAILED or some other more specific error code.  (We can
add an error code for an output buffer that's not large enough.)

http://codereview.chromium.org/9242018/diff/1/net/http/http_stream_parser.h#n...
net/http/http_stream_parser.h:87: // The output will look like:
"HEX\r\n[payload]\r\n".

Nit: should explain what HEX is.  For example,
  where HEX is a length in hexdecimal (without the "0x" prefix)

http://codereview.chromium.org/9242018/diff/1/net/http/http_stream_parser.h#n...
net/http/http_stream_parser.h:93: static const int kChunkHeaderFooterSize;

This constant should have the type size_t.

http://codereview.chromium.org/9242018/diff/1/net/http/http_stream_parser_uni...
File net/http/http_stream_parser_unittest.cc (right):

http://codereview.chromium.org/9242018/diff/1/net/http/http_stream_parser_uni...
net/http/http_stream_parser_unittest.cc:15: const size_t kNumMaxPayloadBytes =

Nit: kNumMaxPayloadBytes => kMaxPayloadSize ?

http://codereview.chromium.org/9242018/diff/1/net/http/http_stream_parser_uni...
net/http/http_stream_parser_unittest.cc:18: TEST(HttpStreamParser,
EncodeChunk_EmptyPayload) {

Please add a comment to point out this is also how "last-chunk"
is encoded.

Or just rename this test EncodeChunk_LastChunk.

[wtc, 2012-01-18 22:55:00]

Two more nits.

http://codereview.chromium.org/9242018/diff/1/net/http/http_stream_parser.cc
File net/http/http_stream_parser.cc (right):

http://codereview.chromium.org/9242018/diff/1/net/http/http_stream_parser.cc#...
net/http/http_stream_parser.cc:364: EncodeChunk("", chunk_buf_->data(),
kChunkBufferSize);

Nit: it may be cheaper to construct an empty StringPiece
directly:
  EncodeChunk(base::StringPiece, chunk_buf_->data(), kChunkBufferSize);

http://codereview.chromium.org/9242018/diff/1/net/http/http_stream_parser.cc#...
net/http/http_stream_parser.cc:794: cursor += payload.size();

I wonder if we should check for the last-chunk case:
  if (payload.size()) {
    memcpy(cursor, payload.data(), payload.size());
    cursor += payload.size();
  }

[satorux1, 2012-01-18 23:42:30]

Thank you for the great feedback!

https://chromiumcodereview.appspot.com/9242018/diff/1/net/base/upload_data_st...
File net/base/upload_data_stream.h (right):

https://chromiumcodereview.appspot.com/9242018/diff/1/net/base/upload_data_st...
net/base/upload_data_stream.h:33: // with this method and make the interface
cleaner as well with less memmove
On 2012/01/18 22:49:24, wtc wrote:
> 
> method => constant

Good catch. Done.

https://chromiumcodereview.appspot.com/9242018/diff/1/net/base/upload_data_st...
net/base/upload_data_stream.h:35: static const size_t kBufferSize;
On 2012/01/18 22:49:24, wtc wrote:
> 
> Please document this constant.  (I know this is a pre-existing
> problem.)

Done.

https://chromiumcodereview.appspot.com/9242018/diff/1/net/http/http_stream_pa...
File net/http/http_stream_parser.cc (left):

https://chromiumcodereview.appspot.com/9242018/diff/1/net/http/http_stream_pa...
net/http/http_stream_parser.cc:132: chunk_buf_ = new
IOBuffer(request_body_->GetMaxBufferSize() +
On 2012/01/18 22:49:24, wtc wrote:
> 
> I understand why we need kChunkBufferSize.  The alternative
> is to store the chunk_buf_ size as a class member
> chunk_buf_size_.
> 
> It seems more robust to use request_body_->GetMaxBufferSize()
> than the UploadDataStream::kBufferSize constant here.

I thought about keeping GetMaxBufferSize() but changed it to a constant, as this
made it easier to write tests fro UploadDataStream in a pending patch that I had
locally (pre-allocate some output buffer based on the constant, before creating
an UploadDataStream object). For now, I'd keep them as constants.

https://chromiumcodereview.appspot.com/9242018/diff/1/net/http/http_stream_pa...
File net/http/http_stream_parser.cc (right):

https://chromiumcodereview.appspot.com/9242018/diff/1/net/http/http_stream_pa...
net/http/http_stream_parser.cc:68: // chunk.
On 2012/01/18 22:49:24, wtc wrote:
> 
> Add "The size of the chunk buffer (chunk_buf_)." at the
> beginning of this comment block.

Done.

https://chromiumcodereview.appspot.com/9242018/diff/1/net/http/http_stream_pa...
net/http/http_stream_parser.cc:70: net::UploadDataStream::kBufferSize +
On 2012/01/18 22:49:24, wtc wrote:
> 
> The net:: prefix is not necessary because this is inside the
> net namespace.

Done.

https://chromiumcodereview.appspot.com/9242018/diff/1/net/http/http_stream_pa...
net/http/http_stream_parser.cc:364: EncodeChunk("", chunk_buf_->data(),
kChunkBufferSize);
On 2012/01/18 22:55:00, wtc wrote:
> 
> Nit: it may be cheaper to construct an empty StringPiece
> directly:
>   EncodeChunk(base::StringPiece, chunk_buf_->data(), kChunkBufferSize);

Done.

https://chromiumcodereview.appspot.com/9242018/diff/1/net/http/http_stream_pa...
net/http/http_stream_parser.cc:791: cursor += chunk_header.length();
On 2012/01/18 22:49:24, wtc wrote:
> If the standard C library function snprintf() is not banned
> in our code base, we can use it to add the header:
> 
>   int num_chars = snprintf(output, output_size, "%X\r\n",
>                            static_cast<int>(payload.size()));
>   cursor += num_chars;
> 
> But I am worried that snprintf is considered a problematic
> function.

My understanding is that use of snprintf here should be safe. The format string
is a string literal, and the parameter is nothing but an int. Besides, I just
noticed we had base::snprintf() in base/string_util.h. Let's use this. :)

https://chromiumcodereview.appspot.com/9242018/diff/1/net/http/http_stream_pa...
net/http/http_stream_parser.cc:794: cursor += payload.size();
On 2012/01/18 22:55:00, wtc wrote:
> 
> I wonder if we should check for the last-chunk case:
>   if (payload.size()) {
>     memcpy(cursor, payload.data(), payload.size());
>     cursor += payload.size();
>   }

Done. if payload is empty, the last chunk is essentially a noop and we have a
test for it, but I agree with you that checking the size would make it easier to
read.

https://chromiumcodereview.appspot.com/9242018/diff/1/net/http/http_stream_pa...
File net/http/http_stream_parser.h (right):

https://chromiumcodereview.appspot.com/9242018/diff/1/net/http/http_stream_pa...
net/http/http_stream_parser.h:84: // kChunkHeaderFooterSize. Returns -1 if
|output_size| is not large
On 2012/01/18 22:49:24, wtc wrote:
> 
> Instead of -1, you can return a network error code such as
> ERR_FAILED or some other more specific error code.  (We can
> add an error code for an output buffer that's not large enough.)

Good idea. I chose to use ERR_FAILED.

https://chromiumcodereview.appspot.com/9242018/diff/1/net/http/http_stream_pa...
net/http/http_stream_parser.h:87: // The output will look like:
"HEX\r\n[payload]\r\n".
On 2012/01/18 22:49:24, wtc wrote:
> 
> Nit: should explain what HEX is.  For example,
>   where HEX is a length in hexdecimal (without the "0x" prefix)

Good idea. Done.

https://chromiumcodereview.appspot.com/9242018/diff/1/net/http/http_stream_pa...
net/http/http_stream_parser.h:93: static const int kChunkHeaderFooterSize;
On 2012/01/18 22:49:24, wtc wrote:
> 
> This constant should have the type size_t.

Done.

[satorux1, 2012-01-18 23:44:02]

https://chromiumcodereview.appspot.com/9242018/diff/1/net/http/http_stream_pa...
File net/http/http_stream_parser.h (right):

https://chromiumcodereview.appspot.com/9242018/diff/1/net/http/http_stream_pa...
net/http/http_stream_parser.h:84: // kChunkHeaderFooterSize. Returns -1 if
|output_size| is not large
On 2012/01/18 23:42:30, satorux1 wrote:
> On 2012/01/18 22:49:24, wtc wrote:
> > 
> > Instead of -1, you can return a network error code such as
> > ERR_FAILED or some other more specific error code.  (We can
> > add an error code for an output buffer that's not large enough.)
> 
> Good idea. I chose to use ERR_FAILED.

Changed it to ERR_INVALID_ARGUMENT as it sounded a bit more appropriate here.

[satorux1, 2012-01-19 00:07:41]

oops, forgot to check the comments in unittest.cc. Now these comments are also
addressed.

https://chromiumcodereview.appspot.com/9242018/diff/1/net/http/http_stream_pa...
File net/http/http_stream_parser_unittest.cc (right):

https://chromiumcodereview.appspot.com/9242018/diff/1/net/http/http_stream_pa...
net/http/http_stream_parser_unittest.cc:15: const size_t kNumMaxPayloadBytes =
On 2012/01/18 22:49:24, wtc wrote:
> 
> Nit: kNumMaxPayloadBytes => kMaxPayloadSize ?

Done.

https://chromiumcodereview.appspot.com/9242018/diff/1/net/http/http_stream_pa...
net/http/http_stream_parser_unittest.cc:18: TEST(HttpStreamParser,
EncodeChunk_EmptyPayload) {
On 2012/01/18 22:49:24, wtc wrote:
> 
> Please add a comment to point out this is also how "last-chunk"
> is encoded.
> 
> Or just rename this test EncodeChunk_LastChunk.

Done.

[commit-bot: I haz the power, 2012-01-19 05:52:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/satorux@chromium.org/9242018/8004

[commit-bot: I haz the power, 2012-01-19 07:09:31]

Change committed as 118265

[Nico, 2012-01-19 15:09:06]

http://codereview.chromium.org/9242018/diff/8004/net/base/upload_data_stream.cc
File net/base/upload_data_stream.cc (right):

http://codereview.chromium.org/9242018/diff/8004/net/base/upload_data_stream....
net/base/upload_data_stream.cc:16: const size_t UploadDataStream::kBufferSize =
16384;
Since this is now in the cc file, it can't be inlined, which causes other files
that allocate static arrays with this size to have static initializers. See
http://build.chromium.org/f/chromium/perf/linux-release/sizes/report.html?his...

I'll revert this change for now.