Get the latest ParseFTPList code from Mozilla, and apply only the absolutely

Get the latest ParseFTPList code from Mozilla, and apply only the absolutely
required changes.

This way future merging would be much easier.

TEST=none
BUG=none

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

[Paweł Hajdan Jr., 2009-09-04 23:51:04]

I'm thinking about moving the support #defines to the .cc file to not pollute
our files which will include the header.

[wtc, 2009-09-05 02:12:54]

Just some quick comments on reducing the size of our patch.

http://codereview.chromium.org/201034/diff/1/5
File net/third_party/parseftp/chromium.patch (right):

http://codereview.chromium.org/201034/diff/1/5#newcode394
Line 394: -**                 to end at the first '\0' or '\n' or '\r\n'.
I can't tell what's the difference between these lines.
If we want to stay as close to the Mozilla code as possible,
we should not make any style or cpplint change.

http://codereview.chromium.org/201034/diff/1/5#newcode450
Line 450: -struct list_state
It's fine to keep the list_state and list_result names.

To avoid confusion, our code should always say
  struct list_state
and
  struct list_result

We see this kind of type identifier in Unix code a lot,
for example, pid_t, pthread_mutex_t, struct sockaddr_in.
The _t suffix or the struct tells us they're types.

http://codereview.chromium.org/201034/diff/1/5#newcode485
Line 485: +  char                 fe_type;      // 'd'(dir) or 'l'(link) or
'f'(file)
May want to use 'int' to avoid surprises.
I would not change the C /* */ comment delimiters.

http://codereview.chromium.org/201034/diff/1/5#newcode496
Line 496: -int ParseFTPList(const char *line,
Don't change the placement of *.

[Paweł Hajdan Jr., 2009-09-08 17:27:08]

So, the patch is now more minimal. Please review.

[wtc, 2009-09-09 01:50:06]

The mapping from PRTime::tm_month to
base::Time::Exploded::month needs work.

The former (tm_month) is 0-based month, 0 = January.
The latter (month) is 1-based month, 1 = January.

I verified that no other members have this issue.

http://codereview.chromium.org/201034/diff/1/4
File net/third_party/parseftp/README.chromium (right):

http://codereview.chromium.org/201034/diff/1/4#newcode1
Line 1: This directory contains Mozilla FTP LIST response parsing routines.
Should we still have README.chromium in the CL?

http://codereview.chromium.org/201034/diff/1/4#newcode6
Line 6: - some style adjustments for Chromium (kept to the minimum)
This probaly should say
  eliminated dependencies on NSPR
and cover the third item (PRExplodedTime)

http://codereview.chromium.org/201034/diff/4001/5003
File net/third_party/parseftp/chromium.patch (right):

http://codereview.chromium.org/201034/diff/4001/5003#newcode35
Line 35: +                uint64 seconds = StringToInt64(p + 1);
Nit: let's use p+1 to match the style of this file.

http://codereview.chromium.org/201034/diff/4001/5003#newcode51
Line 51: +              uint64 size = strtoul(tokens[1], NULL, 10) * 512;
We should use a type that matches the "%lld" format below.
Probably 'long long'.

http://codereview.chromium.org/201034/diff/4001/5003#newcode66
Line 66: +          result->fe_time.year = atoi(p+4); // NSPR wants year as XXXX
Nit: remove the "NSPR wants year as XXXX" comment.

http://codereview.chromium.org/201034/diff/4001/5003#newcode125
Line 125: +        if (result->fe_time.month != 0)
This may need to be result->fe_time.month != 1?

http://codereview.chromium.org/201034/diff/4001/5003#newcode390
Line 390: +#include "base/time.h"
Nit: could you move this line to around line 383,
where the original code includes "nspr.h"?  This
way it's clearer that "base/time.h" replaces "nspr.h".

http://codereview.chromium.org/201034/diff/4001/5004
File net/url_request/url_request_new_ftp_job.cc (right):

http://codereview.chromium.org/201034/diff/4001/5004#newcode288
Line 288: const struct net::list_state& list_state) {
Perhaps we just need to pass list_state.lstyle as the
argument to this function?

[wtc, 2009-09-09 15:47:17]

FYI, the PRExplodedTime structure is documented at
https://developer.mozilla.org/en/PRExplodedTime

Compare its members with the members of base::Time::Exploded.

[Paweł Hajdan Jr., 2009-09-10 17:40:55]

On 2009/09/09 01:50:06, wtc wrote:
> The mapping from PRTime::tm_month to
> base::Time::Exploded::month needs work.
> 
> The former (tm_month) is 0-based month, 0 = January.
> The latter (month) is 1-based month, 1 = January.

Ooops. I missed that. Fixed.

> http://codereview.chromium.org/201034/diff/1/4
> File net/third_party/parseftp/README.chromium (right):
> 
> http://codereview.chromium.org/201034/diff/1/4#newcode1
> Line 1: This directory contains Mozilla FTP LIST response parsing routines.
> Should we still have README.chromium in the CL?

I believe yes. Why not?

> http://codereview.chromium.org/201034/diff/1/4#newcode6
> Line 6: - some style adjustments for Chromium (kept to the minimum)
> This probaly should say
>   eliminated dependencies on NSPR
> and cover the third item (PRExplodedTime)

If you have idea how to improve the comment, I'll gladly do that (slightly
improved since that revision)

> http://codereview.chromium.org/201034/diff/4001/5003
> File net/third_party/parseftp/chromium.patch (right):
> 
> http://codereview.chromium.org/201034/diff/4001/5003#newcode35
> Line 35: +                uint64 seconds = StringToInt64(p + 1);
> Nit: let's use p+1 to match the style of this file.

Done.

> http://codereview.chromium.org/201034/diff/4001/5003#newcode51
> Line 51: +              uint64 size = strtoul(tokens[1], NULL, 10) * 512;
> We should use a type that matches the "%lld" format below.
> Probably 'long long'.

Done.

> http://codereview.chromium.org/201034/diff/4001/5003#newcode66
> Line 66: +          result->fe_time.year = atoi(p+4); // NSPR wants year as
XXXX
> Nit: remove the "NSPR wants year as XXXX" comment.

Done.

> http://codereview.chromium.org/201034/diff/4001/5003#newcode125
> Line 125: +        if (result->fe_time.month != 0)
> This may need to be result->fe_time.month != 1?

I'm fixing all these issue in url_request_new_ftp_job.cc, after our discussion.
The bit-shifts are nasty.

> http://codereview.chromium.org/201034/diff/4001/5003#newcode390
> Line 390: +#include "base/time.h"
> Nit: could you move this line to around line 383,
> where the original code includes "nspr.h"?  This
> way it's clearer that "base/time.h" replaces "nspr.h".

Done.

> http://codereview.chromium.org/201034/diff/4001/5004
> File net/url_request/url_request_new_ftp_job.cc (right):
> 
> http://codereview.chromium.org/201034/diff/4001/5004#newcode288
> Line 288: const struct net::list_state& list_state) {
> Perhaps we just need to pass list_state.lstyle as the
> argument to this function?

I'd really prefer to do it in a separate, cleanup CL.

[wtc, 2009-09-10 18:10:46]

http://codereview.chromium.org/201034/diff/1/4
File net/third_party/parseftp/README.chromium (right):

http://codereview.chromium.org/201034/diff/1/4#newcode1
Line 1: This directory contains Mozilla FTP LIST response parsing routines.
On 2009/09/09 01:50:06, wtc wrote:
> Should we still have README.chromium in the CL?

The reason I asked this is that README.chromium is not in
Patch Set 2 of the CL.

[wtc, 2009-09-10 18:55:32]

LGTM, with the following changes.

(I reviewed chromium.patch instead of the changes
to ParseFTPList.{h,cpp}.)

http://codereview.chromium.org/201034/diff/8001/4010
File net/third_party/parseftp/chromium.patch (right):

http://codereview.chromium.org/201034/diff/8001/4010#newcode39
Line 39: +                t.LocalExplode(&(result->fe_time));
We need to subtract 1 from result->fe_time.month after
the t.LocalExplode call.

http://codereview.chromium.org/201034/diff/8001/4010#newcode236
Line 236: +            t.LocalExplode(&(state->now_tm));
We need to subtract 1 from state->now_tm.month after
the t.LocalExplode call.

http://codereview.chromium.org/201034/diff/8001/4010#newcode355
Line 355: +                  t.LocalExplode(&(state->now_tm));
We need to subtract 1 from state->now_tm.month after
the t.LocalExplode call.

http://codereview.chromium.org/201034/diff/8001/4010#newcode396
Line 396: +namespace net {
Let's add a comment here about the 'month' field
of base::Time::Exploded being 0-11 (0 = January) to
match PRExplodedTime, and that the caller of ParseFTPList
is responsible for adding 1 to 'month' after ParseFTPList
returns if it uses result->fe_time.

Another option is to include "base/third_party/nspr/prtime.h".
This gives us the definitions of not only PRExplodedTime
but also PRInt32, PRUint32, PRBool, and PRTime.  We
need to do the base::Time::Exploded => PRExplodedTime
conversion in ParseFTPList.cpp (after the t.LocalExplode()
calls), and the caller of ParseFTPList() needs to do the
PRExplodedTime => base::Time::Exploded conversion
in url_request_new_ftp_job.cc.  The conversion requries
extra copying, but it makes the code much easier to
understand, and makes the diffs smaller.

http://codereview.chromium.org/201034/diff/8001/4011
File net/url_request/url_request_new_ftp_job.cc (right):

http://codereview.chromium.org/201034/diff/8001/4011#newcode257
Line 257: // the third-party parsing code uses bit-shifting on the month,
I now understand the bit-shifting code in ParseFTPList.cpp.
The bit-shifting code works whether 'month' is 0-based or
1-based.