net: Don't merge HTTP headers and body if the body is not in memory.

net: Don't merge HTTP headers and body if the body is not in memory.

UploadDataStream::MarkConsumedAndFillBuffer() may touch the file
system if the body data comes from a file.

BUG=72001, 107966
TEST=added tests.


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

[agl, 2012-01-23 15:01:31]

LGTM. The previous change was merged to M17 as a compat fix. Do we need to merge
this too? (Note that it's getting very close to the cut-off so we should only
merge if it's going to fix something concrete. I don't think that's the case,
but I may have missed something.)

[satorux1, 2012-01-23 17:25:15]

We don't need to merge this patch.

Practically, UploadDataStream has contents of a small file entirely in memory,
as it fills the 16KB buffer when the object is created. However, this behavior
is implicit and may change in the future (besides, buf() +
MarkConsumedAndFillBuffer() API is awkward), hence checking IsInMemory()
explicitly is safer.

[commit-bot: I haz the power, 2012-01-23 22:45:41]

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

[commit-bot: I haz the power, 2012-01-24 01:07:53]

Change committed as 118769

[wtc, 2012-01-24 19:08:03]

Patch Set 2 LGTM.  I have a question and some suggested
changes below.

http://codereview.chromium.org/9270030/diff/9001/net/base/upload_data.cc
File net/base/upload_data.cc (right):

http://codereview.chromium.org/9270030/diff/9001/net/base/upload_data.cc#newc...
net/base/upload_data.cc:181: // Chunks are provided as a stream, hence it's not
in memory.

Nit: this comment is not 100% accurate.  Chunks are in memory,
but UploadData does not have all the chunks at once.

http://codereview.chromium.org/9270030/diff/9001/net/base/upload_data.cc#newc...
net/base/upload_data.cc:182: if (is_chunked_)

Question: Is this test necessary?  If is_chunked_ is true,
then the elements have the type TYPE_CHUNK.  (See
UploadData::Element::SetToChunk().)  So it seems that it is
sufficient to just check for TYPE_BYTES below.

http://codereview.chromium.org/9270030/diff/9001/net/http/http_stream_parser.cc
File net/http/http_stream_parser.cc (right):

http://codereview.chromium.org/9270030/diff/9001/net/http/http_stream_parser....
net/http/http_stream_parser.cc:817: // IsInMemory() ensures that the request
body is not chunked.

This comment doesn't seem useful -- or it should also mention
that the request body is not in a file (so we can avoid file
I/O).  This motivation should be documented somewhere, and this
function seems like a good place to do that.

http://codereview.chromium.org/9270030/diff/9001/net/http/http_stream_parser.h
File net/http/http_stream_parser.h (right):

http://codereview.chromium.org/9270030/diff/9001/net/http/http_stream_parser....
net/http/http_stream_parser.h:94: // Returns true if request and body should be
merged (i.e. the sum is

Note: 'request' consists of 'request headers' and 'request body',
so this comment needs to be more verbose to be accurate:
  Returns true if request headers and body should be merged...

http://codereview.chromium.org/9270030/diff/9001/net/http/http_stream_parser....
net/http/http_stream_parser.h:96: static bool ShouldMerge(const std::string&
request,

The 'request' parameter should be renamed 'request_headers'.

Nit: this method name should say what should be merged.
For example, ShouldMergeRequestBody or the more verbose
ShouldMergeRequestHeadersAndBody.

http://codereview.chromium.org/9270030/diff/9001/net/net.gyp
File net/net.gyp (right):

http://codereview.chromium.org/9270030/diff/9001/net/net.gyp#newcode1053
net/net.gyp:1053: 'base/upload_data_unittest.cc',

Nit: this should be listed after upload_data_stream_unittest.cc.

[satorux1, 2012-01-24 19:25:42]

wtc, thank you for the comments. created a follow-up patch based on your
comments:

https://chromiumcodereview.appspot.com/9284033

http://codereview.chromium.org/9270030/diff/9001/net/base/upload_data.cc
File net/base/upload_data.cc (right):

http://codereview.chromium.org/9270030/diff/9001/net/base/upload_data.cc#newc...
net/base/upload_data.cc:181: // Chunks are provided as a stream, hence it's not
in memory.
On 2012/01/24 19:08:04, wtc wrote:
> 
> Nit: this comment is not 100% accurate.  Chunks are in memory,
> but UploadData does not have all the chunks at once.

Done.

http://codereview.chromium.org/9270030/diff/9001/net/base/upload_data.cc#newc...
net/base/upload_data.cc:182: if (is_chunked_)
On 2012/01/24 19:08:04, wtc wrote:
> 
> Question: Is this test necessary?  If is_chunked_ is true,
> then the elements have the type TYPE_CHUNK.  (See
> UploadData::Element::SetToChunk().)  So it seems that it is
> sufficient to just check for TYPE_BYTES below.

I originally thought about it, but there is a case when is_chunk_ is set to
true, but the first chunk is not yet delivered, so checking this here is needed.
Added a comment about it.

http://codereview.chromium.org/9270030/diff/9001/net/http/http_stream_parser.h
File net/http/http_stream_parser.h (right):

http://codereview.chromium.org/9270030/diff/9001/net/http/http_stream_parser....
net/http/http_stream_parser.h:96: static bool ShouldMerge(const std::string&
request,
On 2012/01/24 19:08:04, wtc wrote:
> 
> The 'request' parameter should be renamed 'request_headers'.
> 
> Nit: this method name should say what should be merged.
> For example, ShouldMergeRequestBody or the more verbose
> ShouldMergeRequestHeadersAndBody.

I like the idea and chose the latter.