In CompositeHTTPBodyStream, coalesce small GetBytesBuffer()s to better fill the buffer.

In CompositeHTTPBodyStream, coalesce small GetBytesBuffer()s to better fill the buffer.

R=mark@chromium.org

Committed: https://chromium.googlesource.com/crashpad/crashpad/+/e5048b3a8084d4ec8e1db76cde03d108a316003b

[Mark Mentovai, 2014-11-07 17:00:11]

https://codereview.chromium.org/707223002/diff/1/util/net/http_body.cc
File util/net/http_body.cc (right):

https://codereview.chromium.org/707223002/diff/1/util/net/http_body.cc#newcod...
util/net/http_body.cc:102: std::min(max_len,
Maybe more readable if std::min( went on the first line and the two arguments
went on the next line? It’d look less like a staircase that way. Up to you.

https://codereview.chromium.org/707223002/diff/1/util/net/http_body.cc#newcod...
util/net/http_body.cc:104: size_t bytes_copied = 0;
I think this should be ssize_t because that’s what you’re sticking in it and
that’s what you’re returning it as.

https://codereview.chromium.org/707223002/diff/1/util/net/http_body_test.cc
File util/net/http_body_test.cc (right):

https://codereview.chromium.org/707223002/diff/1/util/net/http_body_test.cc#n...
util/net/http_body_test.cc:221: }  // namespace crashpad
Do you want to have a simple test that has two small StringHTTPBodyStreams and a
buffer big enough to hold both, where you check that you read the entire thing
in a single GetBytesBuffer()?

[Robert Sesek, 2014-11-07 17:04:33]

Patchset #2 (id:20001) has been deleted

[Robert Sesek, 2014-11-07 17:05:33]

https://codereview.chromium.org/707223002/diff/1/util/net/http_body.cc
File util/net/http_body.cc (right):

https://codereview.chromium.org/707223002/diff/1/util/net/http_body.cc#newcod...
util/net/http_body.cc:102: std::min(max_len,
On 2014/11/07 17:00:11, Mark Mentovai wrote:
> Maybe more readable if std::min( went on the first line and the two arguments
> went on the next line? It’d look less like a staircase that way. Up to you.

Done.

https://codereview.chromium.org/707223002/diff/1/util/net/http_body.cc#newcod...
util/net/http_body.cc:104: size_t bytes_copied = 0;
On 2014/11/07 17:00:11, Mark Mentovai wrote:
> I think this should be ssize_t because that’s what you’re sticking in it and
> that’s what you’re returning it as.

Done.

https://codereview.chromium.org/707223002/diff/1/util/net/http_body_test.cc
File util/net/http_body_test.cc (right):

https://codereview.chromium.org/707223002/diff/1/util/net/http_body_test.cc#n...
util/net/http_body_test.cc:221: }  // namespace crashpad
On 2014/11/07 17:00:11, Mark Mentovai wrote:
> Do you want to have a simple test that has two small StringHTTPBodyStreams and
a
> buffer big enough to hold both, where you check that you read the entire thing
> in a single GetBytesBuffer()?

No, because the interface isn't making any guarantees about that. The fact of
efficiency is an implementation detail. This code path is exercised implicitly,
though.

[Mark Mentovai, 2014-11-07 17:07:02]

Cool, LGTM. I’m glad you decided to go for this.

[Robert Sesek, 2014-11-07 17:08:17]

Committed patchset #2 (id:40001) manually as
e5048b3a8084d4ec8e1db76cde03d108a316003b (presubmit successful).