FTP: fix navigation to systems with broken EPSV support.

FTP: fix navigation to systems with broken EPSV support.

Added error code check in DoDataConnectComplete, which falls back
to PASV if the connection with EPSV has timed out, and stops the transaction
in case of any other error.

BUG=48285
TEST=navigate to ftp.apc.com, it should succeed and display a directory listing

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

[Paweł Hajdan Jr., 2010-07-26 19:59:47]

This still makes it wait for the timeout (a few seconds). I think that's the
price the broken servers have to pay. Maybe I missed something though.

I tried to write a test for this, but it turned out to be very tricky due to the
way FtpNetworkTransaction sends completion callbacks. It's very hard to test
scenarios in which something different that a successful connection happens to
the data socket. I plan to fix this in the future.

[wtc, 2010-07-26 21:52:45]

Pawel: I asked gavinp, who added EPSV support, to review
this CL.

I have one question about this CL below.

http://codereview.chromium.org/3033028/diff/1/2
File net/ftp/ftp_network_transaction.cc (right):

http://codereview.chromium.org/3033028/diff/1/2#newcode1197
net/ftp/ftp_network_transaction.cc:1197: if (result != OK)
I don't understand these two lines (1197-1198).  They seem
to contradict your new comment on lines 1200-1201.  Now
you will only call RecordDataConnectionError with result=OK,
so "record the connection error" is no longer true.

[gavinp, 2010-07-27 15:59:09]

I echo WTC's question; should lines 1197-1998 maybe be moved below 1200-1201?

Also, I apologise for nitting on style; I'm still learning our coding style, and
so I'm paying close attention to it.

During implementation of EPSV, I wondered if we should memoize
FtpNetworkTransaction::use_epsv_ for a server across an entire session.  At the
time, I decided that having an extra round-trip for FTP with non-EPSV servers
was acceptable, and didn't proceed in adding this feature; instead we just
discover the correct value on each and every FTP transaction.

With this CL, I'm leaning more towards thinking we should revisit that thinking.
 It's OK to wait a few seconds for a broken server, but I think it's not all
that acceptable the second, third or tenth time a user FTPs from that server in
a particular session.

On 2010/07/26 21:52:45, wtc wrote:
> Pawel: I asked gavinp, who added EPSV support, to review
> this CL.
> 
> I have one question about this CL below.
> 
> http://codereview.chromium.org/3033028/diff/1/2
> File net/ftp/ftp_network_transaction.cc (right):
> 
> http://codereview.chromium.org/3033028/diff/1/2#newcode1197
> net/ftp/ftp_network_transaction.cc:1197: if (result != OK)
> I don't understand these two lines (1197-1198).  They seem
> to contradict your new comment on lines 1200-1201.  Now
> you will only call RecordDataConnectionError with result=OK,
> so "record the connection error" is no longer true.

[gavinp, 2010-07-27 15:59:27]

http://codereview.chromium.org/3033028/diff/1/2
File net/ftp/ftp_network_transaction.cc (right):

http://codereview.chromium.org/3033028/diff/1/2#newcode1205
net/ftp/ftp_network_transaction.cc:1205: 
nit: remove blank lines 1203 and 1205

[Paweł Hajdan Jr., 2010-07-27 18:12:19]

Gavin, style comments are a good thing, thanks for that.

Yeah, the histogram code was in wrong place. A sort of editor thinko.

I added a TODO for memoization. I'd prefer to make the navigation work at all
first, and then improve the performance.

[gavinp, 2010-07-27 18:16:23]

LGTM. 

Go ahead and add me to the TODO for the memorization, I might do that too.

http://codereview.chromium.org/3033028/diff/7001/8001
File net/ftp/ftp_network_transaction.cc (right):

http://codereview.chromium.org/3033028/diff/7001/8001#newcode1192
net/ftp/ftp_network_transaction.cc:1192: // TODO(phajdan.jr): remember it for
future transactions with this server.
please also TODO adding a test for this

http://codereview.chromium.org/3033028/diff/7001/8001#newcode1204
net/ftp/ftp_network_transaction.cc:1204: 
nit: remove blank lines 1201 & 1204

[Paweł Hajdan Jr., 2010-07-27 18:19:44]

http://codereview.chromium.org/3033028/diff/7001/8001
File net/ftp/ftp_network_transaction.cc (right):

http://codereview.chromium.org/3033028/diff/7001/8001#newcode1204
net/ftp/ftp_network_transaction.cc:1204: 
On 2010/07/27 18:16:23, gavinp wrote:
> nit: remove blank lines 1201 & 1204

IMHO that would make the code more cluttered and harder to read. I'd say the
style guide doesn't tell us to remove blank lines in such cases.

[eroman, 2010-07-28 00:05:29]

lgtm