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

net/http/http_stream_parser.cc

Index: net/http/http_stream_parser.cc
diff --git a/net/http/http_stream_parser.cc b/net/http/http_stream_parser.cc
index 37f089a332f9237cd881bce79a0c91b93b19bcfc..11df737ef1cc0e3b2e7c1d625ef52cadc303dff6 100644
--- a/net/http/http_stream_parser.cc
+++ b/net/http/http_stream_parser.cc
@@ -61,6 +61,15 @@ bool HeadersContainMultipleCopiesOfField(
 
 namespace net {
 
+// 2 CRLFs + max of 8 hex chars.
+const int HttpStreamParser::kChunkHeaderFooterSize = 12;
+
+// The chunk buffer is guaranteed to be large enough to hold the encoded
+// chunk.

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

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

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

Done.

+static const size_t kChunkBufferSize =
+    net::UploadDataStream::kBufferSize +

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

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

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

Done.

+    HttpStreamParser::kChunkHeaderFooterSize;
+
 HttpStreamParser::HttpStreamParser(ClientSocketHandle* connection,
                                    const HttpRequestInfo* request,
                                    GrowableIOBuffer* read_buffer,
@@ -128,9 +137,7 @@ int HttpStreamParser::SendRequest(const std::string& request_line,
   request_body_.reset(request_body);
   if (request_body_ != NULL && request_body_->is_chunked()) {
     request_body_->set_chunk_callback(this);
-    const int kChunkHeaderFooterSize = 12;  // 2 CRLFs + max of 8 hex chars.
-    chunk_buf_ = new IOBuffer(request_body_->GetMaxBufferSize() +

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

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.

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

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.

-                              kChunkHeaderFooterSize);
+    chunk_buf_ = new IOBuffer(kChunkBufferSize);
   }
 
   io_state_ = STATE_SENDING_HEADERS;
@@ -353,23 +360,16 @@ int HttpStreamParser::DoSendBody(int result) {
     chunk_length_without_encoding_ = 0;
     chunk_length_ = 0;
 
-    int buf_len = static_cast<int>(request_body_->buf_len());
     if (request_body_->eof()) {
-      static const char kLastChunk[] = "0\r\n\r\n";
-      chunk_length_ = strlen(kLastChunk);
-      memcpy(chunk_buf_->data(), kLastChunk, chunk_length_);
+      EncodeChunk("", chunk_buf_->data(), kChunkBufferSize);

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

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

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

Done.

       sent_last_chunk_ = true;
-    } else if (buf_len) {
+    } else if (request_body_->buf_len() > 0) {
       // Encode and send the buffer as 1 chunk.
-      std::string chunk_header = StringPrintf("%X\r\n", buf_len);
-      char* chunk_ptr = chunk_buf_->data();
-      memcpy(chunk_ptr, chunk_header.data(), chunk_header.length());
-      chunk_ptr += chunk_header.length();
-      memcpy(chunk_ptr, request_body_->buf()->data(), buf_len);
-      chunk_ptr += buf_len;
-      memcpy(chunk_ptr, "\r\n", 2);
-      chunk_length_without_encoding_ = buf_len;
-      chunk_length_ = chunk_header.length() + buf_len + 2;
+      const base::StringPiece payload(request_body_->buf()->data(),
+                                      request_body_->buf_len());
+      chunk_length_ = EncodeChunk(payload, chunk_buf_->data(),
+                                  kChunkBufferSize);
+      chunk_length_without_encoding_ = payload.size();
     }
 
     if (!chunk_length_)  // chunk_buf_ is empty. More POST data is yet to come?
@@ -777,4 +777,26 @@ void HttpStreamParser::GetSSLCertRequestInfo(
   }
 }
 
+int HttpStreamParser::EncodeChunk(const base::StringPiece& payload,
+                                  char* output,
+                                  size_t output_size) {
+  if (output_size < payload.size() + kChunkHeaderFooterSize)
+    return -1;
+
+  char* cursor = output;
+  // Add the header.
+  std::string chunk_header =
+      StringPrintf("%X\r\n", static_cast<int>(payload.size()));
+  memcpy(cursor, chunk_header.data(), chunk_header.length());
+  cursor += chunk_header.length();

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

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.

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

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. :)

+  // Add the payload.
+  memcpy(cursor, payload.data(), payload.size());
+  cursor += payload.size();

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

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]

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.

+  // Add the trailing CRLF.
+  memcpy(cursor, "\r\n", 2);
+  cursor += 2;
+
+  return cursor - output;
+}
+
 }  // namespace net