Undisable half of new ftp unit tests in url_request_unittest.cc.

Re-enable half of new ftp unit tests in url_request_unittest.cc.

This change might introduce flakiness and test hangs, but we have
infrastructure in place to quickly detect it.

TEST=none
http://crbug.com/18036

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

[Paweł Hajdan Jr., 2009-07-29 23:55:44]

The tests that are left disabled do not pass EXPECT_FALSE(r.is_pending())
assertion, and they make URLRequestJob leak.

[wtc, 2009-07-30 00:28:20]

LGTM!

I changed "undisable" to "re-enable".

http://codereview.chromium.org/159614/diff/1/2
File net/url_request/url_request_unittest.cc (right):

http://codereview.chromium.org/159614/diff/1/2#newcode1861
Line 1861: // Needs more work, tracked at http://crbug.com/18036.
Nit: at => in

[Paweł Hajdan Jr., 2009-07-30 16:42:51]

I had to make one change in ftp_network_transaction.cc. I couldn't repro it
locally, but on the trybots (I inspected the logs out of curiosity) there was
some garbage at the end of the log message.

I guess that the comment there was wrong, and the buffer was *not*
NUL-terminated. It could be a reason of intermittent hangs - if the NUL byte
wasn't found too early... I don't know.

Please take a look.

[wtc, 2009-07-30 18:02:32]

LGTM.  As I noted below, please remove
ftp_network_transaction.cc from your CL.

http://codereview.chromium.org/159614/diff/1002/6
File net/ftp/ftp_network_transaction.cc (right):

http://codereview.chromium.org/159614/diff/1002/6#newcode916
Line 916: DLOG(INFO) << std::string(read_data_buf_->data(), result);
This should be protected with if (result > 0).

But in Ibrar's CL http://codereview.chromium.org/146137,
he removed this DLOG(INFO) statement, so I think you
should remove this file from your CL, and let Ibrar's CL
take care of this.

By the way, I'm also not clear on why the data in the
read_data_buf_ is null terminated.  Ibrar says it's
only null terminated if the data is a directory listing.
But I still don't fully understand it.

[wtc, 2009-07-30 22:16:32]

http://codereview.chromium.org/159614/diff/1002/7
File net/url_request/url_request_unittest.cc (right):

http://codereview.chromium.org/159614/diff/1002/7#newcode47
Line 47: class URLRequestTestContext : public URLRequestContext {
I found that there is a TestURLRequestContext class defined
in url_request_unittest.h.  The two class names are very
similar.  It would be nice to rename URLRequestTestContext
to be more different, or change it back to
URLRequestHttpCacheContext.

[ibrar, 2009-07-31 09:19:42]

On 2009/07/30 18:02:32, wtc wrote:
> LGTM.  As I noted below, please remove
> ftp_network_transaction.cc from your CL.
> 
> http://codereview.chromium.org/159614/diff/1002/6
> File net/ftp/ftp_network_transaction.cc (right):
> 
> http://codereview.chromium.org/159614/diff/1002/6#newcode916
> Line 916: DLOG(INFO) << std::string(read_data_buf_->data(), result);
> This should be protected with if (result > 0).
> 
> But in Ibrar's CL http://codereview.chromium.org/146137,
> he removed this DLOG(INFO) statement, so I think you
> should remove this file from your CL, and let Ibrar's CL
> take care of this.
> 
> By the way, I'm also not clear on why the data in the
> read_data_buf_ is null terminated.  Ibrar says it's
> only null terminated if the data is a directory listing.
> But I still don't fully understand it.

Let me clear this again. Actually data is not null terminated string in either
case a file or directory listing "BUT" we need a null terminated string just in
case of directory listing. Because I am treating it as string.

See

"std::istringstream iss(buf->data());" 
in function 

"URLRequestNewFtpJob::ProcessFtpDir"


So I read a byte less than the total buffer and put a "null" in case of
directory listing only.

SEE

"buf->data()[bytes_read] = 0;"

in function

URLRequestNewFtpJob::ProcessFtpDir