Also parse file size in new FTP LIST parsing code.

Also parse file size in new FTP LIST parsing code.

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

[Paweł Hajdan Jr., 2009-10-28 18:57:44]

I somehow forgot to do it in the original CL.

[eroman, 2009-10-28 19:09:35]

lgtm

[wtc, 2009-10-28 19:12:08]

http://codereview.chromium.org/343022/diff/1/4
File net/ftp/ftp_directory_listing_buffer_unittest.cc (right):

http://codereview.chromium.org/343022/diff/1/4#newcode51
Line 51: ASSERT_EQ(0U, lines.size() % 8);
The CL's description doesn't describe the changes in this
file.  They seem to be fixing off-by-one errors.

http://codereview.chromium.org/343022/diff/1/5
File net/ftp/ftp_directory_listing_parsers.cc (right):

http://codereview.chromium.org/343022/diff/1/5#newcode135
Line 135: if (entry.type != FtpDirectoryListingEntry::FILE)
Why don't we test entry.type earlier, like this?

  if (entry.type == FtpDirectoryListingEntry::FILE) {
    if (!StringToInt64(columns[4], &entry.size) || entry.size < 0)
      return false;
  } else {
    entry.size = -1;  // File size not applicable.
  }
    
This avoids the unnecessary StringToInt64 call for non-file
entries.

[Paweł Hajdan Jr., 2009-10-28 20:51:13]

On 2009/10/28 19:12:08, wtc wrote:
> http://codereview.chromium.org/343022/diff/1/4
> File net/ftp/ftp_directory_listing_buffer_unittest.cc (right):
> 
> http://codereview.chromium.org/343022/diff/1/4#newcode51
> Line 51: ASSERT_EQ(0U, lines.size() % 8);
> The CL's description doesn't describe the changes in this
> file.  They seem to be fixing off-by-one errors.

The gold file is newline-delimited. I'm adding one more "thing" the test is
checking in it: the file size. This means that instead of previous 7 fields in
one test case (a file or directory entry) there are 8.

> http://codereview.chromium.org/343022/diff/1/5
> File net/ftp/ftp_directory_listing_parsers.cc (right):
> 
> http://codereview.chromium.org/343022/diff/1/5#newcode135
> Line 135: if (entry.type != FtpDirectoryListingEntry::FILE)
> Why don't we test entry.type earlier, like this?
> This avoids the unnecessary StringToInt64 call for non-file
> entries.

That's right. I'd prefer checking the format anyway, always. This way the parser
is always "sure" what it is parsing, and does not rely too much on "fuzziness".
This should allow me to improve the parser for listing styles it can't handle
(which means the code will be more solid) rather than relying on the
"fuzziness", which cannot be reliable. An example of that is current Mozilla
parser: when it sees VMS server emulating Unix (badly), it uses a Unix parser,
badly (the links you finally get are broken) - but says that everything is ok.
My parser would say the truth in a similar case: that it doesn't understand the
format. On the other hand, it allows differences in areas which are probably
less important, like number of spaces between fields.

[wtc, 2009-10-28 23:51:58]

LGTM.

http://codereview.chromium.org/343022/diff/1/5
File net/ftp/ftp_directory_listing_parsers.cc (right):

http://codereview.chromium.org/343022/diff/1/5#newcode135
Line 135: if (entry.type != FtpDirectoryListingEntry::FILE)
I see.  Your code will return false if the format is
wrong, whereas my suggested code will return true with
entry.size = -1.