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

net/http/http_stream_parser.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/http/http_stream_parser.h"
 
 #include "base/compiler_specific.h"
 #include "base/metrics/histogram.h"
 #include "base/stringprintf.h"
 #include "base/string_util.h"
@@ skipping 43 matching lines @@
     if (count_same_value || field_value != field_value2)
       return true;
   }
   return false;
 }
 
 }  // namespace
 
 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,
                                    const BoundNetLog& net_log)
     : io_state_(STATE_NONE),
       request_(request),
       request_headers_(NULL),
       request_body_(NULL),
       read_buf_(read_buffer),
       read_buf_unused_offset_(0),
@@ skipping 47 matching lines @@
     return result;
   response_->socket_address = HostPortPair::FromAddrInfo(address.head());
 
   std::string request = request_line + headers.ToString();
   scoped_refptr<StringIOBuffer> headers_io_buf(new StringIOBuffer(request));
   request_headers_ = new DrainableIOBuffer(headers_io_buf,
                                            headers_io_buf->size());
   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;
   result = DoLoop(OK);
   if (result == ERR_IO_PENDING)
     callback_ = callback;
 
   return result > 0 ? OK : result;
 }
 
@@ skipping 202 matching lines @@
       io_state_ = STATE_REQUEST_SENT;
       return OK;
     }
 
     // |chunk_length_without_encoding_| is 0 when DoSendBody() is first
     // called, hence the first call to MarkConsumedAndFillBuffer() is a noop.
     request_body_->MarkConsumedAndFillBuffer(chunk_length_without_encoding_);
     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?
       return ERR_IO_PENDING;
 
     return connection_->socket()->Write(chunk_buf_, chunk_length_,
                                         io_callback_);
   }
 
   // Non-chunked request body.
@@ skipping 387 matching lines @@
 
 void HttpStreamParser::GetSSLCertRequestInfo(
     SSLCertRequestInfo* cert_request_info) {
   if (request_->url.SchemeIs("https") && connection_->socket()) {
     SSLClientSocket* ssl_socket =
         static_cast<SSLClientSocket*>(connection_->socket());
     ssl_socket->GetSSLCertRequestInfo(cert_request_info);
   }
 }
 
+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