Report unreadable files as size zero when uploading.

Report unreadable files as size zero when uploading.
Upload zero bytes if the file size shrinks.

BUG=30850
TEST=uploading an unreadable file works

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

[vandebo (ex-Chrome), 2010-03-25 00:22:18]

Will add agl and darin if this looks reasonable.

[wtc, 2010-03-25 02:03:36]

vandebo: thanks a lot for taking this on.

eroman: do you have spare cycles to review this today or
tomorrow?  I need to write some code or review cbentzel's
CL for HTTP NTLM/Negotiate auth first.  Thanks.

[eroman, 2010-03-25 02:24:07]

wtc: I will review it now. Hopefully we can get it finished tonight, since I
will be gone thursday and friday.

[eroman, 2010-03-25 03:28:58]

Can you add a unit-test for this case?

http://codereview.chromium.org/1250002/diff/10001/11003
File net/base/upload_data.cc (right):

http://codereview.chromium.org/1250002/diff/10001/11003#newcode62
net/base/upload_data.cc:62: return file;
This reads really strangely to me, and I had to scratch my head a bit to make
sense of this.

From the name of the function, I expect the caller to take ownership of the
returned pointer, and not for there to be this caching behavior.

As I understand it, you are doing this so the "new file stream" you allocate in
GetContentLength() is returned by the subsequent NewFileStream() that
UploadDataStream does.

And you rely on NewFileStream() always being called after a call to
GetContentLength(), in order to free up the memory.

At a minimum, I think |file_| should be a scoped_ptr so memory won't leak if
NewFileStream() is not called, and this should be commented.

http://codereview.chromium.org/1250002/diff/10001/11003#newcode73
net/base/upload_data.cc:73: delete file;
please use scoped_ptr<> instead of manual deletes.

http://codereview.chromium.org/1250002/diff/10001/11004
File net/base/upload_data.h (right):

http://codereview.chromium.org/1250002/diff/10001/11004#newcode72
net/base/upload_data.h:72: // TYPE_BYTES or if the file is not openable.
nit: can you mention that the owner is responsible for deleting the stream?

http://codereview.chromium.org/1250002/diff/10001/11005
File net/base/upload_data_stream.cc (right):

http://codereview.chromium.org/1250002/diff/10001/11005#newcode97
net/base/upload_data_stream.cc:97: next_element_stream_.reset(NULL);
nit: NULL can be omitted.

[vandebo (ex-Chrome), 2010-03-25 06:26:51]

I'll add a unit test tomorrow.

http://codereview.chromium.org/1250002/diff/10001/11003
File net/base/upload_data.cc (right):

http://codereview.chromium.org/1250002/diff/10001/11003#newcode62
net/base/upload_data.cc:62: return file;
On 2010/03/25 03:28:58, eroman wrote:
> This reads really strangely to me, and I had to scratch my head a bit to make
> sense of this.

It is a bit odd.  I considered documenting it in the interface, but decided it
was an internal implementation detail.  I added a comment here and at the call
site in GetContentLength().
 
> From the name of the function, I expect the caller to take ownership of the
> returned pointer, and not for there to be this caching behavior.

The caller does take ownership.  It's just that when the caller is
GetContentLength, it's put back into the cache.

 
> As I understand it, you are doing this so the "new file stream" you allocate
in
> GetContentLength() is returned by the subsequent NewFileStream() that
> UploadDataStream does.

Yes.  GetContentLength() needs to open the file in order to figure out if it
should report the file size as zero, or the file size -- If we can't open the
file, then we'd rather report the size as 0 than send a bunch of Nulls.  But
once the Content-Length is set, we can't change our minds.  In practice,
GetContentSize() will be called once, and then NewFileStream() will be called
once after that.
 
> And you rely on NewFileStream() always being called after a call to
> GetContentLength(), in order to free up the memory.

I changed the destructor to clean it up if it wasn't used (instead of just
suggesting that it will always be used). 
 
> At a minimum, I think |file_| should be a scoped_ptr so memory won't leak if
> NewFileStream() is not called, and this should be commented.

I would prefer to do that as well, but scoped_ptr disallows a copy constructor
and UploadData::Elements are put into a std::vector, so that doesn't work.

http://codereview.chromium.org/1250002/diff/10001/11003#newcode73
net/base/upload_data.cc:73: delete file;
On 2010/03/25 03:28:58, eroman wrote:
> please use scoped_ptr<> instead of manual deletes.

Done.

http://codereview.chromium.org/1250002/diff/10001/11004
File net/base/upload_data.h (right):

http://codereview.chromium.org/1250002/diff/10001/11004#newcode72
net/base/upload_data.h:72: // TYPE_BYTES or if the file is not openable.
On 2010/03/25 03:28:58, eroman wrote:
> nit: can you mention that the owner is responsible for deleting the stream?

Done.

http://codereview.chromium.org/1250002/diff/10001/11005
File net/base/upload_data_stream.cc (right):

http://codereview.chromium.org/1250002/diff/10001/11005#newcode97
net/base/upload_data_stream.cc:97: next_element_stream_.reset(NULL);
On 2010/03/25 03:28:58, eroman wrote:
> nit: NULL can be omitted.

Done.

[vandebo (ex-Chrome), 2010-03-26 01:12:33]

agl, do you have time to review this?

[vandebo (ex-Chrome), 2010-03-26 02:19:54]

James, do you have time to look at the windows code in file_util.h ?

[wtc, 2010-03-26 02:24:35]

vandebo: I can review this CL first thing tomorrow morning.

[James Hawkins, 2010-03-26 02:38:39]

http://codereview.chromium.org/1250002/diff/24001/25001
File base/file_util.h (right):

http://codereview.chromium.org/1250002/diff/24001/25001#newcode504
base/file_util.h:504: #if defined(OS_POSIX)
Does this need to be inline?  If so the inline keyword should be before the
type.  It would be ideal if we could split the implementation into the
respective file_util_win.cc and file_util_posix.cc.

http://codereview.chromium.org/1250002/diff/24001/25001#newcode513
base/file_util.h:513: scoped_ptr<wchar_t> file(_wcsdup(path.value().c_str()));
I think it's safe to use const_cast in this situation on path.value().c_str(),
as passing strings from std::sting to windows APIs is one of the common use
cases for const_cast.

http://codereview.chromium.org/1250002/diff/24001/25001#newcode518
base/file_util.h:518: NULL, &security_descriptor) != 0)
You should explicitly compare against ERROR_SUCCESS instead of 0 here and the
two cases below.

[wtc, 2010-03-26 18:51:59]

LGTM!

vandebo, I like your CL very much.  Thank you for tracking
down the problem with AGL's patch.

I note the more important comments below with "IMPORTANT".

You can commit this CL without getting a new review from me.
Just upload a new patch set before you check in.  Thanks!

http://codereview.chromium.org/1250002/diff/24001/25001
File base/file_util.h (right):

http://codereview.chromium.org/1250002/diff/24001/25001#newcode17
base/file_util.h:17: #endif  // UNIT_TEST
Nit: for a short #ifdef block (such as the one liner here),
I recommend not adding a comment after #endif to reduce the
clutter.

http://codereview.chromium.org/1250002/diff/24001/25001#newcode508
base/file_util.h:508: stat_buf.st_mode = stat_buf.st_mode & ~(S_IRUSR | S_IRGRP
| S_IROTH);
Nit: use the &= operator.

http://codereview.chromium.org/1250002/diff/24001/25001#newcode538
base/file_util.h:538: int rc = SetNamedSecurityInfo(file.get(), SE_FILE_OBJECT,
Nit: SetNamedSecurityInfo returns a DWORD, not int.

http://codereview.chromium.org/1250002/diff/24001/25001#newcode545
base/file_util.h:545: #endif  // OS_WIN
Add a #else case that has a NOTIMPLEMENTED() macro and
returns false.

http://codereview.chromium.org/1250002/diff/24001/25002
File chrome/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/1250002/diff/24001/25002#newcode135
chrome/browser/renderer_host/resource_dispatcher_host.cc:135:
std::vector<net::UploadData::Element>* uploads =
|uploads| can probably be a const pointer.

http://codereview.chromium.org/1250002/diff/24001/25002#newcode137
chrome/browser/renderer_host/resource_dispatcher_host.cc:137:
std::vector<net::UploadData::Element>::iterator iter;
Can you still declare |iter| as a const_iterator?  |iter| is
only used to call the type() and file_path() methods, both of
which are const.

http://codereview.chromium.org/1250002/diff/24001/25004
File net/base/upload_data.cc (right):

http://codereview.chromium.org/1250002/diff/24001/25004#newcode9
net/base/upload_data.cc:9: #include "net/base/file_stream.h"
"net/base/file_stream.h" is already included by
"net/base/upload_data.h".

You can get by with just a forward declaration of
class FileStream in "net/base/upload_data.h", but that
requires moving the Upload::Element constructor and
destructor to this file.

http://codereview.chromium.org/1250002/diff/24001/25004#newcode29
net/base/upload_data.cc:29: if (content_length_computed_)
You can merge this into the if statement at the begining of
this function.

http://codereview.chromium.org/1250002/diff/24001/25004#newcode77
net/base/upload_data.cc:77: << "\" for reading";
Nit: should we also log the error code |rv|?  Same for the
other log message below.

http://codereview.chromium.org/1250002/diff/24001/25004#newcode79
net/base/upload_data.cc:79: } else if (file_range_offset_) {
Nit: don't use "else" after a return statement.

http://codereview.chromium.org/1250002/diff/24001/25005
File net/base/upload_data.h (right):

http://codereview.chromium.org/1250002/diff/24001/25005#newcode39
net/base/upload_data.h:39: delete file_;
Perhaps define |file_| as a scoped_ptr so that it's
automatically initialized and deleted?

Consider renaming |file_| to |file_stream_| to reflect its
type.

http://codereview.chromium.org/1250002/diff/24001/25005#newcode70
net/base/upload_data.h:70: // Returns a FileStream for this element, positioned
at
Please note that this FileStream is opened for reading.
Alternatively, rename this method NewFileStreamForReading.

http://codereview.chromium.org/1250002/diff/24001/25005#newcode72
net/base/upload_data.h:72: // for cleaning up the FileStream. Returns NULL if
this element is of type
Nit: say "if this element is not of type TYPE_FILE", just in
case we add other non-file types in the future.

http://codereview.chromium.org/1250002/diff/24001/25006
File net/base/upload_data_stream.cc (left):

http://codereview.chromium.org/1250002/diff/24001/25006#oldcode86
net/base/upload_data_stream.cc:86: next_element_remaining_ =
element.file_range_length();
IMPORTANT:

Hmm... the original code uses element.file_range_length()
here, which is not clipped to EOF.  Do you think that's a
bug?

http://codereview.chromium.org/1250002/diff/24001/25006
File net/base/upload_data_stream.cc (right):

http://codereview.chromium.org/1250002/diff/24001/25006#newcode71
net/base/upload_data_stream.cc:71:
next_element_stream_.reset(element.NewFileStream());
Note that if next_element_remaining_ is nonzero,
next_element_stream_ must be non-NULL.  This is guaranteed
by your new code, even though we make two separate function
calls here (GetContentLength and NewFileStream).

You can actually depend on this property and delete the
null check for next_element_stream_.get() at line 79 below.
Probably not a good idea.

http://codereview.chromium.org/1250002/diff/24001/25008
File net/base/upload_data_stream_unittest.cc (right):

http://codereview.chromium.org/1250002/diff/24001/25008#newcode68
net/base/upload_data_stream_unittest.cc:68: EXPECT_EQ(read_counter,
stream.size());
Add a comment to explain the expected result (we pad with 0
bytes to the advertised content length).

http://codereview.chromium.org/1250002/diff/24001/25009
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/1250002/diff/24001/25009#newcode13
net/http/http_network_transaction_unittest.cc:13: #include
"net/base/file_stream.h"
Nit: "net/base/file_stream.h" is now included by
"net/base/upload_data.h".

http://codereview.chromium.org/1250002/diff/24001/25009#newcode4133
net/http/http_network_transaction_unittest.cc:4133:
TEST_F(HttpNetworkTransactionTest, UploadUnreadableFile) {
Nit: Add a comment to note that the key expected result of
this unit test is the "Content-Length: 0" request header
when the file to upload is not readable.

http://codereview.chromium.org/1250002/diff/24001/25009#newcode4143
net/http/http_network_transaction_unittest.cc:4143:
EXPECT_TRUE(file_util::MakeFileUnreadable(temp_file));
Should this also be ASSERT_TRUE?  If the file is still
readable, there is no point in continuing this unit test.

http://codereview.chromium.org/1250002/diff/24001/25009#newcode4210
net/http/http_network_transaction_unittest.cc:4210: // Get the length and
consume the opened file to force uploading 0's.
IMPORTANT:

It's not clear what you meant by "consume the opened file".
I think you meant "taking the FileStream away from the
Upload::Element", right?

The undocumented side effects of GetContentLength() and
NewFileStream() you depend on to accomplish this are not
obvious at all, and could be easily broken by a future
change to UploadData::Element.  I hope you can come up with
a better way to create the error condition for this unit
test.

http://codereview.chromium.org/1250002/diff/24001/25009#newcode4212
net/http/http_network_transaction_unittest.cc:4212: scoped_ptr<FileStream>
file_ref(
Nit: file_ref => file_stream

http://codereview.chromium.org/1250002/diff/29001/30006
File net/base/upload_data_stream.cc (right):

http://codereview.chromium.org/1250002/diff/29001/30006#newcode80
net/base/upload_data_stream.cc:80: // If the underlying file has been changed,
treat it as error.
IMPORTANT:

Do we need to do this expected file modification time check
every time we fill up the read buffer?  (If the upload file
is larger than the read buffer's size, we will go through
this loop multiple times for that file.)

Is it sufficient to just check this once, when we open the
new file stream (at line 94 below)?

[vandebo (ex-Chrome), 2010-03-26 22:22:13]

http://codereview.chromium.org/1250002/diff/24001/25001
File base/file_util.h (right):

http://codereview.chromium.org/1250002/diff/24001/25001#newcode17
base/file_util.h:17: #endif  // UNIT_TEST
On 2010/03/26 18:51:59, wtc wrote:
> Nit: for a short #ifdef block (such as the one liner here),
> I recommend not adding a comment after #endif to reduce the
> clutter.

Done.

http://codereview.chromium.org/1250002/diff/24001/25001#newcode504
base/file_util.h:504: #if defined(OS_POSIX)
On 2010/03/26 02:38:40, James Hawkins wrote:
> Does this need to be inline?

As far as I know, that's the only way of putting it in base and including it
only in unit tests.

> If so the inline keyword should be before the type.

Done.

http://codereview.chromium.org/1250002/diff/24001/25001#newcode508
base/file_util.h:508: stat_buf.st_mode = stat_buf.st_mode & ~(S_IRUSR | S_IRGRP
| S_IROTH);
On 2010/03/26 18:51:59, wtc wrote:
> Nit: use the &= operator.

Done.

http://codereview.chromium.org/1250002/diff/24001/25001#newcode513
base/file_util.h:513: scoped_ptr<wchar_t> file(_wcsdup(path.value().c_str()));
On 2010/03/26 02:38:40, James Hawkins wrote:
> I think it's safe to use const_cast in this situation on path.value().c_str(),
> as passing strings from std::sting to windows APIs is one of the common use
> cases for const_cast.

Done.

http://codereview.chromium.org/1250002/diff/24001/25001#newcode518
base/file_util.h:518: NULL, &security_descriptor) != 0)
On 2010/03/26 02:38:40, James Hawkins wrote:
> You should explicitly compare against ERROR_SUCCESS instead of 0 here and the
> two cases below.

Done.

http://codereview.chromium.org/1250002/diff/24001/25001#newcode538
base/file_util.h:538: int rc = SetNamedSecurityInfo(file.get(), SE_FILE_OBJECT,
On 2010/03/26 18:51:59, wtc wrote:
> Nit: SetNamedSecurityInfo returns a DWORD, not int.

Done.

http://codereview.chromium.org/1250002/diff/24001/25001#newcode545
base/file_util.h:545: #endif  // OS_WIN
On 2010/03/26 18:51:59, wtc wrote:
> Add a #else case that has a NOTIMPLEMENTED() macro and
> returns false.

Done.

http://codereview.chromium.org/1250002/diff/24001/25002
File chrome/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/1250002/diff/24001/25002#newcode135
chrome/browser/renderer_host/resource_dispatcher_host.cc:135:
std::vector<net::UploadData::Element>* uploads =
On 2010/03/26 18:51:59, wtc wrote:
> |uploads| can probably be a const pointer.

Done.

http://codereview.chromium.org/1250002/diff/24001/25002#newcode137
chrome/browser/renderer_host/resource_dispatcher_host.cc:137:
std::vector<net::UploadData::Element>::iterator iter;
On 2010/03/26 18:51:59, wtc wrote:
> Can you still declare |iter| as a const_iterator?  |iter| is
> only used to call the type() and file_path() methods, both of
> which are const.

Done.

http://codereview.chromium.org/1250002/diff/24001/25004
File net/base/upload_data.cc (right):

http://codereview.chromium.org/1250002/diff/24001/25004#newcode9
net/base/upload_data.cc:9: #include "net/base/file_stream.h"
On 2010/03/26 18:51:59, wtc wrote:
> "net/base/file_stream.h" is already included by
> "net/base/upload_data.h".

Done.

http://codereview.chromium.org/1250002/diff/24001/25004#newcode29
net/base/upload_data.cc:29: if (content_length_computed_)
On 2010/03/26 18:51:59, wtc wrote:
> You can merge this into the if statement at the begining of
> this function.

Done.

http://codereview.chromium.org/1250002/diff/24001/25004#newcode77
net/base/upload_data.cc:77: << "\" for reading";
On 2010/03/26 18:51:59, wtc wrote:
> Nit: should we also log the error code |rv|?  Same for the
> other log message below.

Done.

http://codereview.chromium.org/1250002/diff/24001/25004#newcode79
net/base/upload_data.cc:79: } else if (file_range_offset_) {
On 2010/03/26 18:51:59, wtc wrote:
> Nit: don't use "else" after a return statement.

Done.

http://codereview.chromium.org/1250002/diff/24001/25005
File net/base/upload_data.h (right):

http://codereview.chromium.org/1250002/diff/24001/25005#newcode39
net/base/upload_data.h:39: delete file_;
On 2010/03/26 18:51:59, wtc wrote:
> Perhaps define |file_| as a scoped_ptr so that it's
> automatically initialized and deleted?

I would like to, but scoped_ptr doesn't have a copy constructor and Element's
are copied when inserted into std::vector in UploadData.
 
> Consider renaming |file_| to |file_stream_| to reflect its
> type.

Done.

http://codereview.chromium.org/1250002/diff/24001/25005#newcode70
net/base/upload_data.h:70: // Returns a FileStream for this element, positioned
at
On 2010/03/26 18:51:59, wtc wrote:
> Please note that this FileStream is opened for reading.
> Alternatively, rename this method NewFileStreamForReading.

Done.

http://codereview.chromium.org/1250002/diff/24001/25005#newcode72
net/base/upload_data.h:72: // for cleaning up the FileStream. Returns NULL if
this element is of type
On 2010/03/26 18:51:59, wtc wrote:
> Nit: say "if this element is not of type TYPE_FILE", just in
> case we add other non-file types in the future.

Done.

http://codereview.chromium.org/1250002/diff/24001/25006
File net/base/upload_data_stream.cc (left):

http://codereview.chromium.org/1250002/diff/24001/25006#oldcode86
net/base/upload_data_stream.cc:86: next_element_remaining_ =
element.file_range_length();
On 2010/03/26 18:51:59, wtc wrote:
> IMPORTANT:
> 
> Hmm... the original code uses element.file_range_length()
> here, which is not clipped to EOF.  Do you think that's a
> bug?

Yes, I think this was a bug in the original code that was never visible because
of some simple/defensive programming. (When read returns 0, the old code just
goes to the next element.)  I was initially using file_range_length() but had to
switch to GetContentLength() in order to get the right behavior

http://codereview.chromium.org/1250002/diff/24001/25006
File net/base/upload_data_stream.cc (right):

http://codereview.chromium.org/1250002/diff/24001/25006#newcode71
net/base/upload_data_stream.cc:71:
next_element_stream_.reset(element.NewFileStream());
On 2010/03/26 18:51:59, wtc wrote:
> Note that if next_element_remaining_ is nonzero,
> next_element_stream_ must be non-NULL.  This is guaranteed
> by your new code, even though we make two separate function
> calls here (GetContentLength and NewFileStream).

You can only make that assumption if you depend on the internals of UploadData
and some other assumptions.  From UpdateDataStream's perspective, if the file
became unreadable after we observed its size, next_element_remaining will be > 0
and NewFileStream() could fail.

http://codereview.chromium.org/1250002/diff/24001/25008
File net/base/upload_data_stream_unittest.cc (right):

http://codereview.chromium.org/1250002/diff/24001/25008#newcode68
net/base/upload_data_stream_unittest.cc:68: EXPECT_EQ(read_counter,
stream.size());
On 2010/03/26 18:51:59, wtc wrote:
> Add a comment to explain the expected result (we pad with 0
> bytes to the advertised content length).

Done.

http://codereview.chromium.org/1250002/diff/24001/25009
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/1250002/diff/24001/25009#newcode13
net/http/http_network_transaction_unittest.cc:13: #include
"net/base/file_stream.h"
On 2010/03/26 18:51:59, wtc wrote:
> Nit: "net/base/file_stream.h" is now included by
> "net/base/upload_data.h".

Done.

http://codereview.chromium.org/1250002/diff/24001/25009#newcode4133
net/http/http_network_transaction_unittest.cc:4133:
TEST_F(HttpNetworkTransactionTest, UploadUnreadableFile) {
On 2010/03/26 18:51:59, wtc wrote:
> Nit: Add a comment to note that the key expected result of
> this unit test is the "Content-Length: 0" request header
> when the file to upload is not readable.

Done.

http://codereview.chromium.org/1250002/diff/24001/25009#newcode4143
net/http/http_network_transaction_unittest.cc:4143:
EXPECT_TRUE(file_util::MakeFileUnreadable(temp_file));
On 2010/03/26 18:51:59, wtc wrote:
> Should this also be ASSERT_TRUE?  If the file is still
> readable, there is no point in continuing this unit test.

Done.

http://codereview.chromium.org/1250002/diff/24001/25009#newcode4210
net/http/http_network_transaction_unittest.cc:4210: // Get the length and
consume the opened file to force uploading 0's.
On 2010/03/26 18:51:59, wtc wrote:
> I hope you can come up with a better way to create the 
> error condition for this unit test.

Done.

http://codereview.chromium.org/1250002/diff/24001/25009#newcode4212
net/http/http_network_transaction_unittest.cc:4212: scoped_ptr<FileStream>
file_ref(
On 2010/03/26 18:51:59, wtc wrote:
> Nit: file_ref => file_stream

Done.

[James Hawkins, 2010-03-26 22:25:24]

My part is LGTM.

[wtc, 2010-03-27 01:55:11]

vandebo: thanks for the changes!

The only problem I have is the "incorrect" use of
RestartIgnoringLastError in the UnreadableUploadFileAfterRestart
unit test.  I'd rather you revert to the previous
UnreadableUploadFileAfterStart unit test, check this CL
in, and work on that problem later.

http://codereview.chromium.org/1250002/diff/53001/54009
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/1250002/diff/53001/54009#newcode4197
net/http/http_network_transaction_unittest.cc:4197: scoped_array<char>
unreadable_contents(new char[temp_file_contents.length()]);
Nit: use std::vector<char> next time.

http://codereview.chromium.org/1250002/diff/53001/54009#newcode4215
net/http/http_network_transaction_unittest.cc:4215: MockRead("HTTP/1.0 408
OK\r\n\r\n"),
Nit: 408 means "Request Timeout", not "OK".

http://codereview.chromium.org/1250002/diff/53001/54009#newcode4252
net/http/http_network_transaction_unittest.cc:4252: rv =
trans->RestartIgnoringLastError(&callback);
RestartIgnoringLastError() should only be called after
Start() failed.  If we get a HTTP 408 response, Start()
succeeded.

Right now we only call RestartIgnoringLastError() after
Start() failed with an SSL certificate error.  So calling
RestartIgnoringLastError here does not reflect how Chrome
actually uses this method.

[vandebo (ex-Chrome), 2010-03-29 18:59:21]

http://codereview.chromium.org/1250002/diff/53001/54009
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/1250002/diff/53001/54009#newcode4197
net/http/http_network_transaction_unittest.cc:4197: scoped_array<char>
unreadable_contents(new char[temp_file_contents.length()]);
On 2010/03/27 01:55:11, wtc wrote:
> Nit: use std::vector<char> next time.

Found the right std::string constructor and used that.

http://codereview.chromium.org/1250002/diff/53001/54009#newcode4215
net/http/http_network_transaction_unittest.cc:4215: MockRead("HTTP/1.0 408
OK\r\n\r\n"),
On 2010/03/27 01:55:11, wtc wrote:
> Nit: 408 means "Request Timeout", not "OK".

Done.

http://codereview.chromium.org/1250002/diff/53001/54009#newcode4252
net/http/http_network_transaction_unittest.cc:4252: rv =
trans->RestartIgnoringLastError(&callback);
On 2010/03/27 01:55:11, wtc wrote:
> RestartIgnoringLastError() should only be called after
> Start() failed.  If we get a HTTP 408 response, Start()
> succeeded.
> 
> Right now we only call RestartIgnoringLastError() after
> Start() failed with an SSL certificate error.  So calling
> RestartIgnoringLastError here does not reflect how Chrome
> actually uses this method.

Changed to a 401 error and using RestartWithAuth.  Done.

[wtc, 2010-03-29 19:15:34]

LGTM.  Thanks!