Fix a bug with parsing ftp directory listing lines.

Fix a bug with parsing ftp directory listing lines.

BUG=49997
TEST=ftp_directory_listing_parser_windows_unittest.cc


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

[michaeln, 2010-07-23 01:09:16]

this is not ready yet

http://codereview.chromium.org/3013029/diff/2001/3001
File net/ftp/ftp_directory_listing_parser_windows.cc (right):

http://codereview.chromium.org/3013029/diff/2001/3001#newcode53
net/ftp/ftp_directory_listing_parser_windows.cc:53: time_exploded.hour +=
(time_exploded.hour < 12) ? 12 : 0;
Looks like there's a bug with listings of the form "12:04 AM" too. I think
chrome will interpret that as 12:04 PM instead.

[Paweł Hajdan Jr., 2010-07-23 01:16:00]

Please make sure I am included as a reviewer. Adding myself to the list.

[michaeln, 2010-07-23 23:37:33]

Please take a look.

[Paweł Hajdan Jr., 2010-07-24 00:22:34]

+brettw for changes in base/

http://codereview.chromium.org/3013029/diff/10001/11004
File net/ftp/ftp_directory_listing_parser_windows_unittest.cc (right):

http://codereview.chromium.org/3013029/diff/10001/11004#newcode76
net/ftp/ftp_directory_listing_parser_windows_unittest.cc:76: "11-02-09  05:32PM 
               -1   NT",
It'd be nice to add few bad cases as well.

[brettw, 2010-07-24 00:59:53]

I think check_ranges shouldn't be inlined or use unix_hacker_style
(since it's not really a getter or setter) and should be called
HasValidValues() or something like that.

I don't think we should add the function to basictypes.h. I would just
make an int version (non-templatized) in time.cc for this case.

Brett

On Fri, Jul 23, 2010 at 4:37 PM,  <michaeln@chromium.org> wrote:
> Please take a look.
>
> http://codereview.chromium.org/3013029/show
>

[michaeln, 2010-07-26 23:59:01]

> http://codereview.chromium.org/3013029/diff/10001/11004#newcode76
> net/ftp/ftp_directory_listing_parser_windows_unittest.cc:76: "11-02-09 
05:32PM 
>                -1   NT",
> It'd be nice to add few bad cases as well.

done

On 2010/07/24 00:59:53, brettw wrote:
> I think check_ranges shouldn't be inlined or use unix_hacker_style
> (since it's not really a getter or setter) and should be called
> HasValidValues() or something like that.

done, thnx moving it to the .cc helped, no longer wondering where to put the
range checking helper.

> 
> I don't think we should add the function to basictypes.h. I would just
> make an int version (non-templatized) in time.cc for this case.

done

[Paweł Hajdan Jr., 2010-07-27 00:04:04]

LGTM

[brettw, 2010-07-27 00:10:27]

http://codereview.chromium.org/3013029/diff/3002/18002
File base/time.h (right):

http://codereview.chromium.org/3013029/diff/3002/18002#newcode217
base/time.h:217: bool HasValidValues() const;
Can you add a comment saying what this does?

[michaeln, 2010-07-27 00:19:09]

On 2010/07/27 00:10:27, brettw wrote:
> http://codereview.chromium.org/3013029/diff/3002/18002
> File base/time.h (right):
> 
> http://codereview.chromium.org/3013029/diff/3002/18002#newcode217
> base/time.h:217: bool HasValidValues() const;
> Can you add a comment saying what this does?

will do

[michaeln, 2010-07-27 00:22:26]

On 2010/07/27 00:19:09, michaeln wrote:
> On 2010/07/27 00:10:27, brettw wrote:
> > http://codereview.chromium.org/3013029/diff/3002/18002
> > File base/time.h (right):
> > 
> > http://codereview.chromium.org/3013029/diff/3002/18002#newcode217
> > base/time.h:217: bool HasValidValues() const;
> > Can you add a comment saying what this does?
> 
> will do

    // A cursory test for whether the data members are within thier
    // respective ranges. A 'true' return value does not guarantee the
    // Exploded value can be successfully converted to a Time value.