Fwd: Code Review: windows http_upload should allow response without Content-Length header (#144)

[Ted Mielczarek]

I always send from the wrong email address...

---------- Forwarded message ----------
From: Ted Mielczarek <t...@mielczarek.org>
Date: Mar 29, 2007 2:46 PM
Subject: Code Review: windows http_upload should allow response
without Content-Length header (#144)
To: Mark Mentovai <mmen...@gmail.com>
Cc: google-br...@googlegroups.com

Hi Mark,

I've submitted a patch to the Windows HTTP upload code to allow it to
handle responses without a Content-Length header. This code does the
job for me. Can you take a look at it?

http://code.google.com/p/google-breakpad/issues/detail?id=144
http://code.google.com/p/google-breakpad/issues/attachment?aid=4235507991214105273&name=http_content_length.patch

Thanks,
-Ted

[Mark Mentovai]

+ bool has_content_length_header;
[...]
+ if (...) {
+ has_content_length_header = true;
[...]
+ }
+ else {
+ has_content_length_header = false;

For this, I'd prefer to see you initialize has_content_length_header
to false where it's declared, and eliminate the else clause. Also,
our style is to use snuggly |} else {| braces (even though I don't
care for 'em, personally).

+ string response_body;

Quick optimization: if has_content_length_header is true, you can do
|response_body.reserve(content_length_size);| to avoid a bunch of
potential reallocs later on when appending.

+ while (InternetQueryDataAvailable(request, &bytes_available, 0, 0)) {
+ if (bytes_available == 0)
+ break;

Why not |while(InternetQueryDataAvailable(...) && bytes_available != 0) {|?

+ read_result = InternetReadFile(request, response_buffer, bytes_available,
+ &size_read);
+ if (size_read == 0)
+ break;

Same, sorta: |if (InternetReadFile(...) && size_read > 0) {...} else
{break;}|. You can get rid of read_result entirely this way, but if
you don't get rid of it for some reason, you can at least scope it
more narrowly now. You're only breaking if size_read is 0, but don't
we also want to call ourselves unsuccessful if InternetReadFile
failed? Also note that if you break here, you leak response_buffer[].
Rather than needing multiple points where you delete[] it, I'd
suggest |vector<char> response_buffer(bytes_available)| and taking
&response_buffer[0] as your pointer, provided that you guard against
bytes_available being 0, which you do.

+ delete response_buffer;

Ought to be |delete[] response_buffer;|, but see above about the vector<char>.

+ bool succeeded = (!has_content_length_header || (total_read ==
claimed_size));

The outer parentheses are unnecessary.

We also want succeeded to be false if either of the WinINet calls
returned false, right?

[Ted Mielczarek]

Mark,

I addressed all your comments, and put up a new patch:

http://code.google.com/p/google-breakpad/issues/detail?id=144
http://code.google.com/p/google-breakpad/issues/attachment?aid=7809309544107051367&name=breakpad.144.patch

Thanks,
-Ted

[Mark Mentovai]

+ if (HttpQueryInfo(request, HTTP_QUERY_CONTENT_LENGTH,
+ static_cast<LPVOID>(&content_length),
+ &content_length_size, 0)) {

Fix indentation.

Looks good otherwise, I'll check in with that change in a few.

Mark