Beginnings of our own FTP LIST parsing code.

First parts of new FTP LIST response parsing code.

Added parser for "ls" listing style, and tests. This is not yet used by the browser (will do that in a following CL).

TEST=Covered by net_unittests.
BUG=25520
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=30354

[Paweł Hajdan Jr., 2009-10-19 17:43:37]

This is a very, very early preview of my FTP LIST parsing code idea. It is
incomplete, especially FtpDirectoryListingBuffer (it will generally just
dispatch things to FtpDirectoryListingParsers). I plan to make the
implementation of the Unix directory listing parser much cleaner. This is just a
quickly hacked version to see how easy it is to write in this class design.

Please take a look and see if something looks very wrong to you from a design
POV. The general idea I have is:

- The buffer handles encoding detection and splitting the listing into physical
lines. It has a set of parsers, and feeds the data to all of them, until only
one can successfully parse the data, and then switches to that parser
(current_parser_).

- The Unix parser is quite simple, and parses a line immediately after receiving
it. The VMS parser is going to be more complicated, because in VMS listing a one
logical listing line may consist of two physical lines.

- I want the parsers to be sure what they are parsing, so that we don't turn
garbage into garbage. For compatibility, I plan to fall back to Mozilla code
when no parser can make sense of the listing, and display a message to the user
("please report that this site uses some interesting listing format").

- I also plan to add integration tests for the listing buffer, to just feed it
an existing LISTing from the network and see if it chooses a correct parser and
it works.

You'll probably have more questions - I'll be happy to answer them.

[wtc, 2009-10-20 03:00:19]

Pawel,

I'm attending a conference on Monday - Wednesday.  I won't be able to
review your CL until Thursday.

[Paweł Hajdan Jr., 2009-10-20 16:12:32]

eroman: ping

wtc: no problem, this is still work in progress.

[eroman, 2009-10-20 17:50:47]

The general design looks good.

> - The buffer handles encoding detection and splitting the listing into
physical
> lines. It has a set of parsers, and feeds the data to all of them, until only
> one can successfully parse the data, and then switches to that parser
> (current_parser_).

Nit: I would say this object should be called a "Parser" rather than a "Buffer".
In my mind this is simply another implementation of the Parser interface, which
is implemented by dispatching to sub-Parsers.

> - I want the parsers to be sure what they are parsing, so that we don't turn
> garbage into garbage. For compatibility, I plan to fall back to Mozilla code
> when no parser can make sense of the listing, and display a message to the
user
> ("please report that this site uses some interesting listing format").

That sounds pretty useful to me, but be sure to run any UI decision by Brian or
Glen.

> - I also plan to add integration tests for the listing buffer, to just feed it
> an existing LISTing from the network and see if it chooses a correct parser
and
> it works.

As an aside, I recommend that you write your tests using data files (as opposed
to inlining the strings in a .cc file).
You were probably already going to do this anyway, but if not please see some
current examples under net/data.
This will also make it easy to have a huge number of input files, and drop in a
new listing sample as we discover incompatibilities.

> Description:
> [WIP] Beginnings of our own FTP LIST parsing code.

Is there a bug number for this issue?


This is going to be *a lot* of parsing code..... good luck :)

[Paweł Hajdan Jr., 2009-10-20 17:56:05]

Thanks for the review.

On 2009/10/20 17:50:47, eroman wrote:
> As an aside, I recommend that you write your tests using data files (as
opposed
> to inlining the strings in a .cc file).
> You were probably already going to do this anyway, but if not please see some
> current examples under net/data.

Yeah, I'm going to do that for the FtpDirectoryListingBuffer (and rename it).
For individual parsers I think it'll be easier to have the cases inline, just to
test the corner cases. It will probably be more readable than many very small
files.

> This will also make it easy to have a huge number of input files, and drop in
a
> new listing sample as we discover incompatibilities.
> 
> > Description:
> > [WIP] Beginnings of our own FTP LIST parsing code.
> 
> Is there a bug number for this issue?

I plan to rename the "oddball FTP parsers" bug to "rewrite the thing" bug.
During development I plan to fall back to Mozilla code as written before, but
the goal is to get rid of it.

[Paweł Hajdan Jr., 2009-10-22 18:02:08]

Please do a full review.

[eroman, 2009-10-23 04:03:25]

Overall this is looking good, here are some comments.

http://codereview.chromium.org/244008/diff/9001/9005
File net/ftp/ftp_directory_listing_buffer.cc (right):

http://codereview.chromium.org/244008/diff/9001/9005#newcode42
Line 42: parsers_.insert(new FtpLsDirectoryListingParser());
I don't see anywhere that frees this allocation. Are you sure this doesn't leak?

http://codereview.chromium.org/244008/diff/9001/9005#newcode55
Line 55: if (rv != OK)
unless you plan to add something below this, "return rv;" is sufficient.

http://codereview.chromium.org/244008/diff/9001/9005#newcode85
Line 85: if (!IsStringASCII(from))
It seems like you could still call CodePageToUTF16 for the case where
|encoding_.empty()| -- you just need to pass some default codepage value like
ascii or iso=8859-1.

http://codereview.chromium.org/244008/diff/9001/9005#newcode101
Line 101: for (size_t i = 0; i < buffer_.length(); i++) {
nit: for consistency use ++i, since lower in the file u use ++parser.

http://codereview.chromium.org/244008/diff/9001/9005#newcode102
Line 102: if (i >= 1 && buffer_[i - 1] == '\r' && buffer_[i] == '\n') {
Might be easier to use StringTokenizer on "\r\n".
(Although it would be more permissive in what the line breaks are).

http://codereview.chromium.org/244008/diff/9001/9005#newcode108
Line 108: return ERR_FAILED;
Please pick new error codes.

http://codereview.chromium.org/244008/diff/9001/9005#newcode123
Line 123: return ERR_FAILED;
How about if current_parser_->ConsumeLine(line) returned an error code instead?

http://codereview.chromium.org/244008/diff/9001/9005#newcode125
Line 125: std::set<FtpDirectoryListingParser*>::iterator parser;
Two nits:

(1) Please add a typedef for ParserList/ParserSet or something. So then the
iterator type is ParserList::iterator.

(2) "parser" isn't a great name for an iterator. How about parser_iter or just
iter ?

http://codereview.chromium.org/244008/diff/9001/9005#newcode128
Line 128: parsers_.erase(parser++);
I don't think this is right. You will end up incrementing the iterator twice.

Also, you need to free *parser.

http://codereview.chromium.org/244008/diff/9001/9006
File net/ftp/ftp_directory_listing_buffer.h (right):

http://codereview.chromium.org/244008/diff/9001/9006#newcode25
Line 25: // Called when data is received from the data socket. Returns error
code.
nit: instead of "returns error code" might be more specific and say "network
error code".

http://codereview.chromium.org/244008/diff/9001/9006#newcode50
Line 50: // Detected encoding of the response.
Can you describe the meaning of empty string? (seems to correspond with
"encoding unknown or is ascii").

http://codereview.chromium.org/244008/diff/9001/9006#newcode56
Line 56: // CRLF-delimited lines, without the CRLF, not yet parsed.
Might phrase it as "not yet consumed by parser".

http://codereview.chromium.org/244008/diff/9001/9006#newcode60
Line 60: std::set<FtpDirectoryListingParser*> parsers_;
Please mention who owns these pointers (and is responsible for deleting them).

http://codereview.chromium.org/244008/diff/9001/9007
File net/ftp/ftp_directory_listing_buffer_unittest.cc (right):

http://codereview.chromium.org/244008/diff/9001/9007#newcode12
Line 12: namespace {
I like putting my unit tests into namespace net, to avoid having loads of
"net::" prefix pollution on symbols:

namespace net {
namespace {

<Tests here>

}
}

Up to you.

http://codereview.chromium.org/244008/diff/9001/9007#newcode14
Line 14: class FtpDirectoryListingBufferTest : public testing::Test {
This seems unused, why have a test fixture? (You could remove this and simply
use TEST(...) in place of TEST_F(...).

http://codereview.chromium.org/244008/diff/9001/9007#newcode24
Line 24: const char* test_cases[] = {
Might be useful to either add a comment, or change the name, to indicate these
are filenames.

http://codereview.chromium.org/244008/diff/9001/9007#newcode44
Line 44: << test_cases[i];
Instead of adding << test_cases[i] to each expectation, I like to use
SCOPED_TRACE().

You can see net/proxy/proxy_service_unittest.cc for an example, but the general
pattern would be to add:

SCOPED_TRACE(StringPrintf("Test[%d]: %s", i, test_cases[i]));

At the start of the for block... Now if anything fails it will annotate with
that.

http://codereview.chromium.org/244008/diff/9001/9007#newcode50
Line 50: // TODO(phajdan.jr): Verify the parsed results.
Yes, please!

http://codereview.chromium.org/244008/diff/9001/9008
File net/ftp/ftp_directory_listing_parsers.cc (right):

http://codereview.chromium.org/244008/diff/9001/9008#newcode17
Line 17: (text[2] == 'x' || text[2] == 's' || text[2] == '-'));
Isn't the setuid bit going to be a capital "S"?

% touch foo.html
% chmod u+s foo.html
% ls -l foo.html
-rwSr--r--  1 ericroman  sys  0 Oct 22 20:45 foo.html

http://codereview.chromium.org/244008/diff/9001/9008#newcode34
Line 34: bool IsStringNonNegativeNumber(const string16& text) {
nit: perhaps this should be called "NonNegativeInteger"

http://codereview.chromium.org/244008/diff/9001/9008#newcode49
Line 49: std::string text_ascii = StringToLowerASCII(UTF16ToASCII(text));
What if instead in your loop u did:

LowerCaseEqualsASCII(text, months[i])

http://codereview.chromium.org/244008/diff/9001/9008#newcode67
Line 67: memset(&time_exploded, 0, sizeof(time_exploded));
How about instead defining time_exploded as:

base::Time::Exploded time_exploded = { 0 };

That should zero all the fields

http://codereview.chromium.org/244008/diff/9001/9008#newcode92
Line 92: *time = base::Time::FromLocalExploded(time_exploded);
I don't know what the right thing to do is, but I think this deserves a comment,
since we are guessing as to what the actual time was.

http://codereview.chromium.org/244008/diff/9001/9009
File net/ftp/ftp_directory_listing_parsers.h (right):

http://codereview.chromium.org/244008/diff/9001/9009#newcode26
Line 26: base::Time last_modified;
Can you describe what this field represents?

In particular, I am not convinced we can determine accurate timestamp for
entries, without knowing what timezone the server was using (looks like we
assume client's timezone in the calculations).

So my main nit with this comment is not that the value is going to be something
weird, but that we don't document to the consumer the way in which it is weird
:)

http://codereview.chromium.org/244008/diff/9001/9009#newcode34
Line 34: virtual FtpServerType server_type() = 0;
Please don't use hacker_style_names for virtual functions, as they are not
accessors.

You could alternately make the base class take a FtpServerType in its
constructor and make this non-virtual.

http://codereview.chromium.org/244008/diff/9001/9009#newcode53
Line 53: virtual bool ConsumeLine(const string16& line);
style nit: I suggest labeling all of these as "FtpDirectoryListingParser
methods:" or something (and collapsing the newlines in between.

http://codereview.chromium.org/244008/diff/9001/9010
File net/ftp/ftp_directory_listing_parsers_unittest.cc (right):

http://codereview.chromium.org/244008/diff/9001/9010#newcode10
Line 10: namespace {
Similar comment to earlier, i encourage you to wrap this within an namespace net
(that is my preference anyway)

http://codereview.chromium.org/244008/diff/9001/9010#newcode67
Line 67: EXPECT_NO_FATAL_FAILURE(RunSingleLineTestCase(&parser, good_cases[i]))
This might be another place you can use SCOPED_TRACE()

[Paweł Hajdan Jr., 2009-10-23 19:11:10]

eroman, thanks for review. This patchset should be much better.

> http://codereview.chromium.org/244008/diff/9001/9005#newcode102
> Line 102: if (i >= 1 && buffer_[i - 1] == '\r' && buffer_[i] == '\n') {
> Might be easier to use StringTokenizer on "\r\n".
> (Although it would be more permissive in what the line breaks are).

> http://codereview.chromium.org/244008/diff/9001/9005#newcode123
> Line 123: return ERR_FAILED;
> How about if current_parser_->ConsumeLine(line) returned an error code
instead?

I don't think it would be useful. We'd mostly return ERR_INVALID_RESPONSE. More
detailed error codes wouldn't be very helpful for the end user.

> http://codereview.chromium.org/244008/diff/9001/9007#newcode12
> Line 12: namespace {
> I like putting my unit tests into namespace net, to avoid having loads of
> "net::" prefix pollution on symbols:
> Up to you.

I didn't used that in ftp tests, but noticed that in other tests. I didn't
update the patch to use it (not a lot of net:: symbols), but will use it when
there are more.

> http://codereview.chromium.org/244008/diff/9001/9007#newcode44
> Line 44: << test_cases[i];
> Instead of adding << test_cases[i] to each expectation, I like to use
> SCOPED_TRACE().

Good tip! Done.

> http://codereview.chromium.org/244008/diff/9001/9007#newcode50
> Line 50: // TODO(phajdan.jr): Verify the parsed results.
> Yes, please!

What do you think would be the best format to store the expected results? I was
thinking about JSON, but maybe just kind-of CSV would be ok.

> http://codereview.chromium.org/244008/diff/9001/9008
> File net/ftp/ftp_directory_listing_parsers.cc (right):
> 
> http://codereview.chromium.org/244008/diff/9001/9008#newcode17
> Line 17: (text[2] == 'x' || text[2] == 's' || text[2] == '-'));
> Isn't the setuid bit going to be a capital "S"?
> 
> % touch foo.html
> % chmod u+s foo.html
> % ls -l foo.html
> -rwSr--r--  1 ericroman  sys  0 Oct 22 20:45 foo.html

S = setuid but not executable
s = setuid and executable

Updated to match both.

> http://codereview.chromium.org/244008/diff/9001/9008#newcode34
> Line 34: bool IsStringNonNegativeNumber(const string16& text) {
> nit: perhaps this should be called "NonNegativeInteger"

Hmm... I like the prefix "Is", but indeed the name could be shorter. How about
IsNonNegativeInteger?

> http://codereview.chromium.org/244008/diff/9001/9008#newcode49
> Line 49: std::string text_ascii = StringToLowerASCII(UTF16ToASCII(text));
> What if instead in your loop u did:
> 
> LowerCaseEqualsASCII(text, months[i])

Another simplifying tip. Done.

> http://codereview.chromium.org/244008/diff/9001/9009#newcode34
> Line 34: virtual FtpServerType server_type() = 0;
> Please don't use hacker_style_names for virtual functions, as they are not
> accessors.

I temporarily removed that part, as it's not yet used. It will be when I connect
it to the code in renderer (and then I'll use correct style).

[Paweł Hajdan Jr., 2009-10-26 21:18:59]

ping

[eroman, 2009-10-27 08:28:05]

LGTM

http://codereview.chromium.org/244008/diff/12004/11004
File base/string_util.h (right):

http://codereview.chromium.org/244008/diff/12004/11004#newcode177
Line 177: string16 CollapseWhitespace(const string16& text,
Is this going to compile when WCHAR_T_IS_UTF16?

(wandering if there are going to be duplicate declaration complaints when
string16 is std::wstring).

http://codereview.chromium.org/244008/diff/12004/11011
File net/ftp/ftp_directory_listing_parsers.cc (right):

http://codereview.chromium.org/244008/diff/12004/11011#newcode44
Line 44: const char* months[] = { "jan", "feb", "mar", "apr", "may", "jun",
nit: not sure if it will make any difference in the generated code, but may want
to mark this as |static|.

http://codereview.chromium.org/244008/diff/12004/11011#newcode61
Line 61: if (!ThreeLetterMonthToNumber(columns[5], &time_exploded.month))
Might be helpful to add a DCHECK on columns.size() at the start (both as a
safeguard, but also as a form of documentation). Looks like we expect 8 columns.

http://codereview.chromium.org/244008/diff/12004/11011#newcode96
Line 96: FtpLsDirectoryListingParser::FtpLsDirectoryListingParser() {
Can you move this definition above the destructor?
(Or you could put it in the header file).

http://codereview.chromium.org/244008/diff/12004/11011#newcode118
Line 118: if (columns[0][0] == 'l') {
This might looks nice as a switch(). Up to you.

http://codereview.chromium.org/244008/diff/12004/11013
File net/ftp/ftp_directory_listing_parsers_unittest.cc (right):

http://codereview.chromium.org/244008/diff/12004/11013#newcode22
Line 22: class FtpDirectoryListingParsersTest : public testing::Test {
Note that you don't strictly need to have a fixture for this.
RunSingleLineTestCase() could be a function in the anonymous namespace. I am
fine with either way, just point out that EXPECT_EQ can run from anywhere.

http://codereview.chromium.org/244008/diff/12004/11013#newcode38
Line 38: EXPECT_EQ(test_case.month, time_exploded.month);
please check day_of_month as well.

http://codereview.chromium.org/244008/diff/12004/11013#newcode40
Line 40: EXPECT_EQ(test_case.minute, time_exploded.minute);
is it worth checking seconds and milliseconds too?

http://codereview.chromium.org/244008/diff/12004/11013#newcode72
Line 72: const char* bad_cases[] = {
minor nit: may want to split this off into its own test case, since I can
imagine this "Ls" test growing larger :)

[Paweł Hajdan Jr., 2009-10-27 18:34:37]

> http://codereview.chromium.org/244008/diff/12004/11004
> File base/string_util.h (right):
> 
> http://codereview.chromium.org/244008/diff/12004/11004#newcode177
> Line 177: string16 CollapseWhitespace(const string16& text,
> Is this going to compile when WCHAR_T_IS_UTF16?

Submitted to trybots to find out.

> http://codereview.chromium.org/244008/diff/12004/11011
> File net/ftp/ftp_directory_listing_parsers.cc (right):
> 
> http://codereview.chromium.org/244008/diff/12004/11011#newcode44
> Line 44: const char* months[] = { "jan", "feb", "mar", "apr", "may", "jun",
> nit: not sure if it will make any difference in the generated code, but may
want
> to mark this as |static|.

Done.

> http://codereview.chromium.org/244008/diff/12004/11011#newcode61
> Line 61: if (!ThreeLetterMonthToNumber(columns[5], &time_exploded.month))
> Might be helpful to add a DCHECK on columns.size() at the start (both as a
> safeguard, but also as a form of documentation). Looks like we expect 8
columns.

Done.

> http://codereview.chromium.org/244008/diff/12004/11011#newcode96
> Line 96: FtpLsDirectoryListingParser::FtpLsDirectoryListingParser() {
> Can you move this definition above the destructor?
> (Or you could put it in the header file).

The above destructor is a for different class (the pure virtual one). I think
that's ok...

> http://codereview.chromium.org/244008/diff/12004/11013#newcode38
> Line 38: EXPECT_EQ(test_case.month, time_exploded.month);
> please check day_of_month as well.

Oh, good catch. Fixed.

> http://codereview.chromium.org/244008/diff/12004/11013#newcode40
> Line 40: EXPECT_EQ(test_case.minute, time_exploded.minute);
> is it worth checking seconds and milliseconds too?

Yeah, no problem.

> http://codereview.chromium.org/244008/diff/12004/11013#newcode72
> Line 72: const char* bad_cases[] = {
> minor nit: may want to split this off into its own test case, since I can
> imagine this "Ls" test growing larger :)

Will do that when it really grows.

By the way, the FtpDirectoryListingBuffer is now verifying all the results.
Please review.

[eroman, 2009-10-27 19:31:37]

lgtm.

minor suggestion: the expected results file might be easier to read if it had an
extra newline separating entries.

[Paweł Hajdan Jr., 2009-10-28 17:02:22]

On 2009/10/27 19:31:37, eroman wrote:
> lgtm.
> 
> minor suggestion: the expected results file might be easier to read if it had
an
> extra newline separating entries.

Good idea. Done and committed (no changes to code were required).