Directory listing code for new Portable FTP(Patch SET-2)

(Replaced by http://codereview.chromium.org/42261,
in preparation for checkin.)

BUG=4965
R=darin,wtc,dank,eroman


Description:

Directory listing code for new Portable FTP, I forked this code from mozilla 

mozilla/netwerk/streamconv/converters/ParseFTPList.cpp


and make it compatible with chrome.

[darin (slow to review), 2009-03-03 01:16:06]

sorry, incomplete code review.  i still need to look at the .cc file.

http://codereview.chromium.org/28298/diff/1/2
File net/url_request/directory_parser.h (right):

http://codereview.chromium.org/28298/diff/1/2#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
2006-2008 -> 2009

the chromium license header is also repeated down below, which seems wrong.

http://codereview.chromium.org/28298/diff/1/2#newcode47
Line 47: #define SUPPORT_LSL   // /bin/ls -l and dozens of variations therof
can these be moved into the .cc file?

http://codereview.chromium.org/28298/diff/1/2#newcode75
Line 75: struct list_state {
ListState

http://codereview.chromium.org/28298/diff/1/2#newcode77
Line 77: __int64 now_time;   // needed for year determination
__int64 -> int64 (see base/basictypes.h)

http://codereview.chromium.org/28298/diff/1/2#newcode86
Line 86: struct list_result {
ListResult

http://codereview.chromium.org/28298/diff/1/2#newcode150
Line 150: struct list_state *state,
nit: please indent the parameters so that they are all start at the same column

[ibrar, 2009-03-03 18:16:35]

http://codereview.chromium.org/28298/diff/1/2
File net/url_request/directory_parser.h (right):

http://codereview.chromium.org/28298/diff/1/2#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
On 2009/03/03 01:16:06, darin wrote:
> 2006-2008 -> 2009
> 
> the chromium license header is also repeated down below, which seems wrong.

Done.

http://codereview.chromium.org/28298/diff/1/2#newcode40
Line 40: 
Duplicate license info removed

http://codereview.chromium.org/28298/diff/1/2#newcode47
Line 47: #define SUPPORT_LSL   // /bin/ls -l and dozens of variations therof
On 2009/03/03 01:16:06, darin wrote:
> can these be moved into the .cc file?

Done.

http://codereview.chromium.org/28298/diff/1/2#newcode75
Line 75: struct list_state {
On 2009/03/03 01:16:06, darin wrote:
> ListState

Done.

http://codereview.chromium.org/28298/diff/1/2#newcode77
Line 77: __int64 now_time;   // needed for year determination
On 2009/03/03 01:16:06, darin wrote:
> __int64 -> int64 (see base/basictypes.h)

Done.

http://codereview.chromium.org/28298/diff/1/2#newcode86
Line 86: struct list_result {
On 2009/03/03 01:16:06, darin wrote:
> ListResult

Done.

http://codereview.chromium.org/28298/diff/1/2#newcode150
Line 150: struct list_state *state,
On 2009/03/03 01:16:06, darin wrote:
> nit: please indent the parameters so that they are all start at the same
column

Done.

[ibrar, 2009-03-04 12:40:50]

Some FIXME fixed

[darin (slow to review), 2009-03-06 16:57:46]

it would be good to also write some unit tests.

http://codereview.chromium.org/28298/diff/2006/1006
File net/url_request/directory_parser.cc (right):

http://codereview.chromium.org/28298/diff/2006/1006#newcode48
Line 48: #include "net/url_request/directory_parser.h"
please move the header corresponding to this .cc file to the top.

http://codereview.chromium.org/28298/diff/2006/1005
File net/url_request/directory_parser.h (right):

http://codereview.chromium.org/28298/diff/2006/1005#newcode38
Line 38: #ifndef NET_FTP_DIRECTORY_PARSER_H_
please move these files into the net/ftp/ directory, and call them
ftp_directory_parser.{h,cc}

http://codereview.chromium.org/28298/diff/2006/1005#newcode42
Line 42: 
nit: kill the extra newline

http://codereview.chromium.org/28298/diff/2006/1005#newcode42
Line 42: 
nit: kill the extra new line

http://codereview.chromium.org/28298/diff/2006/1005#newcode49
Line 49: struct ExplodedTime {
can you use Time::Exploded or even just 'struct tm' instead of creating this
struct?

http://codereview.chromium.org/28298/diff/2006/1005#newcode65
Line 65: int64 now_time;   // needed for year determination
please include base/basictypes.h for int64

http://codereview.chromium.org/28298/diff/2006/1005#newcode76
Line 76: const char * fe_fname;  // pointer to filename */
please delete the trailing */

http://codereview.chromium.org/28298/diff/2006/1005#newcode78
Line 78: const char * fe_lname;  // pointer to symlink name */
nit: no space before the "*"

http://codereview.chromium.org/28298/diff/2006/1005#newcode88
Line 88: virtual ~DirectoryParser();
no need for this to be virtual.  in fact, it looks like there is no need for
this class to have a constructor or destructor since it is not intended to be
instantiated.

http://codereview.chromium.org/28298/diff/2006/1005#newcode137
Line 137: static int ParseFTPList(const char *line,
since this function is intended to be called multiple times, once per line of
input.  The ListState structure seems like it could be maintained as private
data.  it seems like it would be nicer as a class that you do instantiate:

FtpDirectoryParser parser;
while (...) {
  parser.ParseLine(... next line of input ...);
  switch (parser.line_type()) {
    ...
  }
}

or, maybe it would be better to create such a class on top of this C interface. 
yeah... that seems best.  that way the C interface stays closer to the original
code, which should make it easier to port changes made to the original code in
the future.

i took a look at FTPDirectoryParser.h, and i think its interface is pretty
decent.

[ibrar, 2009-03-10 17:31:34]

Thanks Darin for comments. Previously I was trying not to change to much code.
Now I have done what you suggest. Please give another shot. 

BTW I will add test cases soon.

[wtc, 2009-03-13 02:59:28]

Note: if you only made trivial changes to the original
files from Mozilla, please ignore my suggested formatting
changes below, because it's better to keep our copies as
close to the "upstream" versions as possible, so that
we can merge new versions of these files from the upstream
more easily.

http://codereview.chromium.org/28298/diff/4004/4006
File net/ftp/ftp_directory_parser.cc (right):

http://codereview.chromium.org/28298/diff/4004/4006#newcode40
Line 40: // mozilla/netwerk/streamconv/converters/ParseFTPList.cpp
Good.  Could you add the revision number?

This file looks like C code...

http://codereview.chromium.org/28298/diff/4004/4006#newcode65
Line 65: struct ListResult *result) {
The parameters should be formatted according to the style
guide.

http://codereview.chromium.org/28298/diff/4004/4005
File net/ftp/ftp_directory_parser.h (right):

http://codereview.chromium.org/28298/diff/4004/4005#newcode5
Line 5: // Version: MPL 1.1/GPL 2.0/LGPL 2.1
For a third-party code, I like to record two pieces of info.

1. The original file's pathname.

2. The revision number.

http://codereview.chromium.org/28298/diff/4004/4005#newcode37
Line 37: #include "base/basictypes.h"
This should be moved inside the header include guard below.

http://codereview.chromium.org/28298/diff/4004/4005#newcode46
Line 46: struct tm     now_tm;  // needed for year determination.
Should this comment be aligned with the other comments in
this struct?

http://codereview.chromium.org/28298/diff/4004/4005#newcode66
Line 66: const char *  fe_lname;        // pointer to symlink name
Nit: remove the space before *.

http://codereview.chromium.org/28298/diff/4004/4005#newcode70
Line 70: struct tm fe_time;             // last-modified time
Nit: align fe_time with the members above.

http://codereview.chromium.org/28298/diff/4004/4005#newcode121
Line 121: struct ListState *state,
Nit: align these two parameters with the first parameter
above.

[ibrar, 2009-03-13 17:49:38]

http://codereview.chromium.org/28298/diff/4004/4006
File net/ftp/ftp_directory_parser.cc (right):

http://codereview.chromium.org/28298/diff/4004/4006#newcode40
Line 40: // mozilla/netwerk/streamconv/converters/ParseFTPList.cpp
On 2009/03/13 02:59:28, wtc wrote:
> Good.  Could you add the revision number?
> 
> This file looks like C code...

Done.

http://codereview.chromium.org/28298/diff/4004/4006#newcode65
Line 65: struct ListResult *result) {
On 2009/03/13 02:59:28, wtc wrote:
> The parameters should be formatted according to the style
> guide.

Done.

http://codereview.chromium.org/28298/diff/4004/4005
File net/ftp/ftp_directory_parser.h (right):

http://codereview.chromium.org/28298/diff/4004/4005#newcode5
Line 5: // Version: MPL 1.1/GPL 2.0/LGPL 2.1
On 2009/03/13 02:59:28, wtc wrote:
> For a third-party code, I like to record two pieces of info.
> 
> 1. The original file's pathname.
> 
> 2. The revision number.


Path name and revision added

http://codereview.chromium.org/28298/diff/4004/4005#newcode37
Line 37: #include "base/basictypes.h"
On 2009/03/13 02:59:28, wtc wrote:
> This should be moved inside the header include guard below.

Done.

http://codereview.chromium.org/28298/diff/4004/4005#newcode46
Line 46: struct tm     now_tm;  // needed for year determination.
On 2009/03/13 02:59:28, wtc wrote:
> Should this comment be aligned with the other comments in
> this struct?

Done.

http://codereview.chromium.org/28298/diff/4004/4005#newcode66
Line 66: const char *  fe_lname;        // pointer to symlink name
On 2009/03/13 02:59:28, wtc wrote:
> Nit: remove the space before *.

Done.

http://codereview.chromium.org/28298/diff/4004/4005#newcode70
Line 70: struct tm fe_time;             // last-modified time
On 2009/03/13 02:59:28, wtc wrote:
> Nit: align fe_time with the members above.

Done.

http://codereview.chromium.org/28298/diff/4004/4005#newcode121
Line 121: struct ListState *state,
On 2009/03/13 02:59:28, wtc wrote:
> Nit: align these two parameters with the first parameter
> above.

Done.

[wtc, 2009-03-16 22:47:12]

Ibrar,

Do you want me to add these two files, without corresponding
changes to the project/build files?

[ibrar, 2009-03-17 18:58:21]

I have added entries in "net.vcproj", "net.gyp", "net_lib.scons"