FTP: fix directory listing parser for ftp://ftp.dd-wrt.com/others/juno6/

FTP: fix directory listing parser for ftp://ftp.dd-wrt.com/others/juno6/

Now the "ls -l" parser ignores lines with empty file names
instead of failing.

BUG=60065
TEST=net_unittests

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

[Paweł Hajdan Jr., 2010-10-27 09:36:22]

ping

[wtc, 2010-10-28 01:05:41]

LGTM.

http://codereview.chromium.org/4182002/diff/3001/4001
File net/data/ftp/dir-listing-ls-19 (right):

http://codereview.chromium.org/4182002/diff/3001/4001#newcode1
net/data/ftp/dir-listing-ls-19:1: drwxr-xr-x    2 0        0            4096 Mar
18  2007
I suspect this is the current directory ".".

http://codereview.chromium.org/4182002/diff/3001/4004
File net/ftp/ftp_directory_listing_parser_ls.cc (right):

http://codereview.chromium.org/4182002/diff/3001/4004#newcode130
net/ftp/ftp_directory_listing_parser_ls.cc:130: if (columns.size() < 6U +
column_offset)
It would be nice to document what are the 6 columns that
should normally be there.

http://codereview.chromium.org/4182002/diff/3001/4005
File net/ftp/ftp_directory_listing_parser_ls_unittest.cc (right):

http://codereview.chromium.org/4182002/diff/3001/4005#newcode109
net/ftp/ftp_directory_listing_parser_ls_unittest.cc:109:
EXPECT_TRUE(parser.ConsumeLine(UTF8ToUTF16(ignored_cases[i])));
In the original code, we EXPECT_FALSE here (see line 124 of
the original code).  Why do we EXPECT_TRUE now?  Just curious.

[Paweł Hajdan Jr., 2010-10-28 07:15:34]

http://codereview.chromium.org/4182002/diff/3001/4004
File net/ftp/ftp_directory_listing_parser_ls.cc (right):

http://codereview.chromium.org/4182002/diff/3001/4004#newcode130
net/ftp/ftp_directory_listing_parser_ls.cc:130: if (columns.size() < 6U +
column_offset)
On 2010/10/28 01:05:41, wtc wrote:
> It would be nice to document what are the 6 columns that
> should normally be there.

Good idea, done.

http://codereview.chromium.org/4182002/diff/3001/4005
File net/ftp/ftp_directory_listing_parser_ls_unittest.cc (right):

http://codereview.chromium.org/4182002/diff/3001/4005#newcode109
net/ftp/ftp_directory_listing_parser_ls_unittest.cc:109:
EXPECT_TRUE(parser.ConsumeLine(UTF8ToUTF16(ignored_cases[i])));
On 2010/10/28 01:05:41, wtc wrote:
> In the original code, we EXPECT_FALSE here (see line 124 of
> the original code).  Why do we EXPECT_TRUE now?  Just curious.

When ConsumeLine returns false, it means that the parser doesn't recognize the
listing format. In this case we do recognize the format, but the line is
useless. Note that this test also verifies that the parser does not generate any
listing entries for the input.

Previously, the parser was failing on the input with empty file names, and that
was the root cause of the bug.

I prefer to explicitly ignore lines that we want to ignore rather than all lines
that we don't recognize (that's what Firefox does), because it can lead to
silently "eating" some listing lines. We had a bug earlier (fixed now), that
Chrome was failing to parse the listing with an error (and a link to the raw
listing), while Firefox was silently ignoring and not displaying some lines,
making the problem much harder to detect.