FTP: add directory listing parser for OS/2 format.

FTP: add directory listing parser for OS/2 format.

BUG=92154
TEST=navigate to ftp://ftp.os4.su/ - no errors should appear

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

[eroman, 2011-08-10 23:52:16]

lgtm

http://codereview.chromium.org/7590011/diff/1/net/ftp/ftp_directory_listing_p...
File net/ftp/ftp_directory_listing_parser_os2.cc (right):

http://codereview.chromium.org/7590011/diff/1/net/ftp/ftp_directory_listing_p...
net/ftp/ftp_directory_listing_parser_os2.cc:18: bool
WindowsDateListingToTime(const std::vector<string16>& columns,
Delete this.

http://codereview.chromium.org/7590011/diff/1/net/ftp/ftp_directory_listing_p...
File net/ftp/ftp_directory_listing_parser_os2.h (right):

http://codereview.chromium.org/7590011/diff/1/net/ftp/ftp_directory_listing_p...
net/ftp/ftp_directory_listing_parser_os2.h:19: NET_TEST bool
ParseFtpDirectoryListingOS2(
Why is this NET_TEST? Can it be NET_API instead

http://codereview.chromium.org/7590011/diff/1/net/ftp/ftp_util.cc
File net/ftp/ftp_util.cc (right):

http://codereview.chromium.org/7590011/diff/1/net/ftp/ftp_util.cc#newcode259
net/ftp/ftp_util.cc:259: bool FtpUtil::WindowsDateListingToTime(const string16&
date,
I didn't review this, I presume it is just a move.

http://codereview.chromium.org/7590011/diff/1/net/ftp/ftp_util_unittest.cc
File net/ftp/ftp_util_unittest.cc (right):

http://codereview.chromium.org/7590011/diff/1/net/ftp/ftp_util_unittest.cc#ne...
net/ftp/ftp_util_unittest.cc:217: }
nit: add a newline.

[Paweł Hajdan Jr., 2011-08-11 00:03:19]

http://codereview.chromium.org/7590011/diff/1/net/ftp/ftp_directory_listing_p...
File net/ftp/ftp_directory_listing_parser_os2.cc (right):

http://codereview.chromium.org/7590011/diff/1/net/ftp/ftp_directory_listing_p...
net/ftp/ftp_directory_listing_parser_os2.cc:18: bool
WindowsDateListingToTime(const std::vector<string16>& columns,
On 2011/08/10 23:52:16, eroman wrote:
> Delete this.

Good catch, done.

http://codereview.chromium.org/7590011/diff/1/net/ftp/ftp_directory_listing_p...
File net/ftp/ftp_directory_listing_parser_os2.h (right):

http://codereview.chromium.org/7590011/diff/1/net/ftp/ftp_directory_listing_p...
net/ftp/ftp_directory_listing_parser_os2.h:19: NET_TEST bool
ParseFtpDirectoryListingOS2(
On 2011/08/10 23:52:16, eroman wrote:
> Why is this NET_TEST? Can it be NET_API instead

This should be used only via ParseFtpDirectoryListing, which is NET_API. All
other "sub-parsers" are NET_TEST.

http://codereview.chromium.org/7590011/diff/1/net/ftp/ftp_util.cc
File net/ftp/ftp_util.cc (right):

http://codereview.chromium.org/7590011/diff/1/net/ftp/ftp_util.cc#newcode259
net/ftp/ftp_util.cc:259: bool FtpUtil::WindowsDateListingToTime(const string16&
date,
On 2011/08/10 23:52:16, eroman wrote:
> I didn't review this, I presume it is just a move.

Not really, I changed it a bit to make AM/PM optional.

http://codereview.chromium.org/7590011/diff/1/net/ftp/ftp_util_unittest.cc
File net/ftp/ftp_util_unittest.cc (right):

http://codereview.chromium.org/7590011/diff/1/net/ftp/ftp_util_unittest.cc#ne...
net/ftp/ftp_util_unittest.cc:217: }
On 2011/08/10 23:52:16, eroman wrote:
> nit: add a newline.

Done.

[eroman, 2011-08-11 00:09:20]

lgtm

http://codereview.chromium.org/7590011/diff/1/net/ftp/ftp_directory_listing_p...
File net/ftp/ftp_directory_listing_parser_os2.h (right):

http://codereview.chromium.org/7590011/diff/1/net/ftp/ftp_directory_listing_p...
net/ftp/ftp_directory_listing_parser_os2.h:19: NET_TEST bool
ParseFtpDirectoryListingOS2(
On 2011/08/11 00:03:19, Paweł Hajdan Jr. wrote:
> On 2011/08/10 23:52:16, eroman wrote:
> > Why is this NET_TEST? Can it be NET_API instead
> 
> This should be used only via ParseFtpDirectoryListing, which is NET_API. All
> other "sub-parsers" are NET_TEST.

Ah, I was confused. Yes, this is right then.