Various cleanups FTP-related.

Various cleanups FTP-related.

- use better name for FTP LIST parsing code in about:credits
- don't open a second data socket
- add a comment explaining why we close the data socket at one point

TEST=Covered by net_unittests.
BUG=none

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

[Paweł Hajdan Jr., 2009-09-17 16:30:33]

I collected the comments left after I committed several of my FTP patches, and
made the fixes here.

Please review. If you have any more nit-like comments, I can fix them as well in
this CL (and not nit-like in another CL).

[eroman, 2009-09-18 01:50:11]

lgtm

[wtc, 2009-09-18 17:24:39]

LGTM!

http://codereview.chromium.org/207014/diff/1/2
File chrome/browser/resources/about_credits.html (right):

http://codereview.chromium.org/207014/diff/1/2#newcode1330
Line 1330: <span class="homepage"><a
href="http://mxr.mozilla.org/mozilla-central/source/netwerk/streamconv/converters/ParseFTPList.cpp">homepage</a></span>
Daniel Berlin wanted us to point to the revision of the file
that we derived from.  I haven't figured out the right away
to do that in Mozilla's MXR browser.  This is probably OK.
http://hg.mozilla.org/mozilla-central/file/179c6f22d6f2/netwerk/streamconv/co...

http://codereview.chromium.org/207014/diff/1/5
File net/url_request/url_request_new_ftp_job.cc (right):

http://codereview.chromium.org/207014/diff/1/5#newcode319
Line 319: void URLRequestNewFtpJob::LogFtpServerType(char server_type) {
Nit: may want to name the parameter "list_style" and declare
it as 'int' because state.lstyle is an 'int'.  (It's
conventional in C code to use the 'int' type for
function parameters that are 'char'.  I don't know why.)

[Paweł Hajdan Jr., 2009-09-18 17:31:08]

On Fri, Sep 18, 2009 at 10:24, <wtc@chromium.org> wrote:

> http://codereview.chromium.org/207014/diff/1/2
> File chrome/browser/resources/about_credits.html (right):
>
> http://codereview.chromium.org/207014/diff/1/2#newcode1330
> Line 1330: <span class="homepage"><a
> href="
>
http://mxr.mozilla.org/mozilla-central/source/netwerk/streamconv/converters/P...
> ">homepage</a></span>
> Daniel Berlin wanted us to point to the revision of the file
> that we derived from.  I haven't figured out the right away
> to do that in Mozilla's MXR browser.  This is probably OK.
>
>
http://hg.mozilla.org/mozilla-central/file/179c6f22d6f2/netwerk/streamconv/co...


Hmm... this is going to be a maintainability problem. I generally want to
stay on Mozilla's trunk, and in the long term rewrite the code. Mozilla's
VMS parsing code is not even correct, so we don't get any advantage for
"oddball" servers by using it. I was recently rewriting some other piece of
Mozilla code, and it became shorter, and better tested. It was also fun!


> http://codereview.chromium.org/207014/diff/1/5
> File net/url_request/url_request_new_ftp_job.cc (right):
>
> http://codereview.chromium.org/207014/diff/1/5#newcode319
> Line 319: void URLRequestNewFtpJob::LogFtpServerType(char server_type) {
> Nit: may want to name the parameter "list_style" and declare
> it as 'int' because state.lstyle is an 'int'.  (It's
> conventional in C code to use the 'int' type for
> function parameters that are 'char'.  I don't know why.)


In Chromium code I'd prefer more explicit type. Well, it is a char. Even old
C++ code sometimes uses int for bools... If it's not a strong nit, I'd
rather leave it as-is.