Using base::Time::Exploded instead of "tm" (FTP PATCH SET 5)

[Replaced by http://codereview.chromium.org/149772 ]

BUG=4965
R=darin,wtc

Description:

Use of base::Time::Exploded instead of "tm" in ftp_directory_parser.cc

[ibrar, 2009-06-13 21:01:15]

Please review

[ibrar, 2009-06-13 21:53:06]

Now I am getting "File Modified Date" as in Mozilla. My only worry is; it is not
the same as in existing chrome ftp implementation.

Darin: Need comment from you too

[darin (slow to review), 2009-06-16 22:40:12]

Looks OK to me.  Some nits:

http://codereview.chromium.org/126096/diff/16/1013
File net/ftp/ftp_directory_parser.cc (right):

http://codereview.chromium.org/126096/diff/16/1013#newcode1367
Line 1367: base::Time tim = Time::FromTimeT(now);
nit: how about "time" instead of "tim" or maybe just use the variable name "t" ?

http://codereview.chromium.org/126096/diff/16/1012
File net/ftp/ftp_directory_parser.h (right):

http://codereview.chromium.org/126096/diff/16/1012#newcode57
Line 57: base::Time::Exploded now_tm;  // needed for year determination.
nit: it is a little odd to see "now" appear as a structure member.  normally
that is only used as a stack variable since the intent is for it to hold the
time at that moment.  is there maybe a better name?  response_time perhaps? 
hmm, but there is also now_time...

http://codereview.chromium.org/126096/diff/16/1014
File net/url_request/url_request_new_ftp_job.cc (right):

http://codereview.chromium.org/126096/diff/16/1014#newcode132
Line 132: // base::Time::Exploded. 1 month and and 1 day is known but
nit: "and and"

http://codereview.chromium.org/126096/diff/16/1014#newcode133
Line 133: // 16 hour differnce is yet to be investigated. We also find
nit: s/differnce/difference/

[ibrar, 2009-06-17 16:21:38]

http://codereview.chromium.org/126096/diff/16/1013
File net/ftp/ftp_directory_parser.cc (right):

http://codereview.chromium.org/126096/diff/16/1013#newcode1367
Line 1367: base::Time tim = Time::FromTimeT(now);
On 2009/06/16 22:40:12, darin wrote:
> nit: how about "time" instead of "tim" or maybe just use the variable name "t"
?

Done.

http://codereview.chromium.org/126096/diff/16/1012
File net/ftp/ftp_directory_parser.h (right):

http://codereview.chromium.org/126096/diff/16/1012#newcode57
Line 57: base::Time::Exploded now_tm;  // needed for year determination.
On 2009/06/16 22:40:12, darin wrote:
> nit: it is a little odd to see "now" appear as a structure member.  normally
> that is only used as a stack variable since the intent is for it to hold the
> time at that moment.  is there maybe a better name?  response_time perhaps? 
> hmm, but there is also now_time...

Both are not my choice. These are part of Mozilla code. Do you really want me to
change that. If yes let me know I will do that

http://codereview.chromium.org/126096/diff/16/1014
File net/url_request/url_request_new_ftp_job.cc (right):

http://codereview.chromium.org/126096/diff/16/1014#newcode132
Line 132: // base::Time::Exploded. 1 month and and 1 day is known but
On 2009/06/16 22:40:12, darin wrote:
> nit: "and and"

Now we are compatible with Mozilla, so there is no need of this comment

http://codereview.chromium.org/126096/diff/16/1014#newcode133
Line 133: // 16 hour differnce is yet to be investigated. We also find
On 2009/06/16 22:40:12, darin wrote:
> nit: s/differnce/difference/

Done.

[darin (slow to review), 2009-06-17 17:02:22]

http://codereview.chromium.org/126096/diff/16/1012
File net/ftp/ftp_directory_parser.h (right):

http://codereview.chromium.org/126096/diff/16/1012#newcode57
Line 57: base::Time::Exploded now_tm;  // needed for year determination.
No need then.  Thanks.

[wtc, 2009-06-17 17:56:36]

Ibrar,

I don't know why we're getting different times from
the current FTP code (which uses WinInet).  What are
the differences?  Could it be a GMT vs. local time issue?

Since we're porting Mozilla code, it seems that our first
goal is to get the same times as Mozilla/Firefox.  Then
we can investigate the difference from the current FTP code.

I found a lot of errors in assignments of the 'month'
and 'year' members of base::Time::Exploded.

1. 'month' is 1-based [1,12] where as 'tm_month' of struct tm
(used by the old code) or PRExplodedTime (used by original
Mozilla code) is 0-based [0,11].

2. 'year' is a regular four-digit year, whereas 'tm_year'
of struct tm is "Years since 1900."

I tried to correct all the errors I could find, but I may
have missed some, so please review those parts of your
code again carefully and make sure there are no off-by-one
or off-by-1900 errors.

Here are the references for struct tm and PRExplodedTime:
http://www.opengroup.org/onlinepubs/000095399/basedefs/time.h.html
https://developer.mozilla.org/en/PRExplodedTime

Only the 'year' and 'month' members need care.  I verified
that all other members have the same offsets and ranges in
all three structures.  You may want to doublecheck that.

http://codereview.chromium.org/126096/diff/16/1013
File net/ftp/ftp_directory_parser.cc (right):

http://codereview.chromium.org/126096/diff/16/1013#newcode1367
Line 1367: base::Time tim = Time::FromTimeT(now);
On 2009/06/16 22:40:12, darin wrote:
> nit: how about "time" instead of "tim" or maybe just use the variable name "t"
?

I suspect that he's avoiding the variable name "time"
because "time" is the name of a libc function.  I think
we can just use the variable name "t", or better yet, rewrite
these four lines of code so that we don't need to go
through time_t.

http://codereview.chromium.org/126096/diff/1018/20#newcode174
Line 174: time_t tim_t = static_cast<time_t>(seconds);
The variable name "tim_t" looks strange.  (The _t suffix
suggests it's a type.)  I think Darin suggested that you
use just "t".

http://codereview.chromium.org/126096/diff/1018/20#newcode175
Line 175: base::Time tim = Time::FromTimeT(tim_t);
Nit: since you have "using base::Time" in this file, you
can just say "Time time = ...".

http://codereview.chromium.org/126096/diff/1018/20#newcode176
Line 176: tim.LocalExplode(&(result->fe_time));
Nit: it's not necessary to add parantheses around
result->fe_time.  Expressions like &result->fe_time
are very often used in C code that experienced C
programmers should know the precedence between & and ->
by heart.

Note: The important change you made here is that you're
storing local time instead of GMT in result->fe_time.
I just wanted to confirm that.

http://codereview.chromium.org/126096/diff/1018/20#newcode474
Line 474: result->fe_time.month  = month_num;
I believe this should be
  result->fe_time.month  = month_num + 1;
because result->fe_time.month is 1-based month
[1,12] whereas tm_mon is 0-based
month [0,11].

http://codereview.chromium.org/126096/diff/1018/20#newcode477
Line 477: // NSPR wants year as XXXX
Remove this comment because this file no longer uses NSPR.
You may replace it with
  // base::Time::Exploded wants year as XXXX.

http://codereview.chromium.org/126096/diff/1018/20#newcode592
Line 592: result->fe_time.year = StringToInt(p + 0) - 1900;
This is wrong.  It should be:
  result->fe_time.year = StringToInt(p + 0);

http://codereview.chromium.org/126096/diff/1018/20#newcode593
Line 593: result->fe_time.month  = StringToInt(p + 5) - 1;
This should be
  result->fe_time.month  = StringToInt(p + 5);

http://codereview.chromium.org/126096/diff/1018/20#newcode597
Line 597: result->fe_time.month  = StringToInt(p) - 1;
This should be
  result->fe_time.month  = StringToInt(p);

http://codereview.chromium.org/126096/diff/1018/20#newcode601
Line 601: result->fe_time.year += 100;
This should be followed by
  result->fe_time.year += 1900;

http://codereview.chromium.org/126096/diff/1018/20#newcode716
Line 716: result->fe_time.year += 100;
This should be followed by
  result->fe_time.year += 1900;

http://codereview.chromium.org/126096/diff/1018/20#newcode810
Line 810: result->fe_time.month = StringToInt(&p[35-18]) - 1;
This should be
  result->fe_time.month = StringToInt(&p[35-18]);

http://codereview.chromium.org/126096/diff/1018/20#newcode814
Line 814: result->fe_time.year += 100;
This should be followed by
  result->fe_time.year += 1900;

http://codereview.chromium.org/126096/diff/1018/20#newcode991
Line 991: time_t now = time(NULL);
You can rewrite these four lines of code using only
base::Time, without going through time_t.

Note that you are now storing local time instead of GMT
in state->now_tm.  I just wanted to confirm that.

Same nit: write &state->now_tm, without parentheses.

http://codereview.chromium.org/126096/diff/1018/20#newcode1139
Line 1139: result->fe_time.month = pos/3;
This should be
  result->fe_time.month = pos/3 + 1;

http://codereview.chromium.org/126096/diff/1018/20#newcode1141
Line 1141: result->fe_time.year = StringToInt(tokens[4]) - 1900;
This should be
  result->fe_time.year = StringToInt(tokens[4]);

http://codereview.chromium.org/126096/diff/1018/20#newcode1151
Line 1151: result->fe_time.year += 100;
This should be followed by
  result->fe_time.year += 1900;

http://codereview.chromium.org/126096/diff/1018/20#newcode1349
Line 1349: result->fe_time.month = month_num - 1;
This should be
  result->fe_time.month = month_num;

http://codereview.chromium.org/126096/diff/1018/20#newcode1365
Line 1365: time_t now = time(NULL);
Rewrite these four lines using base::Time, without going
through time_t.

http://codereview.chromium.org/126096/diff/1018/19
File net/ftp/ftp_directory_parser.h (right):

http://codereview.chromium.org/126096/diff/1018/19#newcode45
Line 45: #include "base/message_loop.h"
Are you sure "base/message_loop.h" is the right header to
include?  I don't see any MessageLoop in this header.
Maybe you want "base/time.h" instead?

http://codereview.chromium.org/126096/diff/1018/19#newcode51
Line 51: int64         now_time;       // needed for year determination.
Note: we could probably use the base::Time type for this.
In the original Mozilla code, this member has the type PRTime,
which is equivalent to base::Time except that it has a
different time unit and epoch (starting point).

http://codereview.chromium.org/126096/diff/1018/19#newcode57
Line 57: base::Time::Exploded now_tm;  // needed for year determination.
Why did you move this member to the end of the struct?
It would be better to move it back to the original position
so we can compare this file with the original Mozilla file
more easily.  It's also nice for it to be next to the related
now_time member.

http://codereview.chromium.org/126096/diff/1018/21
File net/url_request/url_request_new_ftp_job.cc (right):

http://codereview.chromium.org/126096/diff/1018/21#newcode126
Line 126: while (getline(iss, line)) {
Undo the indentation change.  'while' should be aligned with
the previous line.

http://codereview.chromium.org/126096/diff/1018/21#newcode131
Line 131: result.fe_time.month++;
I believe you're incrementing result.fe_time.month here to
compensate for all those off-by-one errors in ftp_directory_parser.cc.

[ibrar, 2009-06-20 18:11:35]

I am still looking.

http://codereview.chromium.org/126096/diff/1018/20
File net/ftp/ftp_directory_parser.cc (right):

http://codereview.chromium.org/126096/diff/1018/20#newcode175
Line 175: base::Time time = Time::FromTimeT(tim_t);
On 2009/06/17 17:56:36, wtc wrote:
> Nit: since you have "using base::Time" in this file, you
> can just say "Time time = ...".

Done.

http://codereview.chromium.org/126096/diff/1018/20#newcode176
Line 176: time.LocalExplode(&(result->fe_time));
On 2009/06/17 17:56:36, wtc wrote:
> Nit: it's not necessary to add parantheses around
> result->fe_time.  Expressions like &result->fe_time
> are very often used in C code that experienced C
> programmers should know the precedence between & and ->
> by heart.
> 
> Note: The important change you made here is that you're
> storing local time instead of GMT in result->fe_time.
> I just wanted to confirm that.

Done.

http://codereview.chromium.org/126096/diff/1018/20#newcode474
Line 474: result->fe_time.month  = month_num;
On 2009/06/17 17:56:36, wtc wrote:
> I believe this should be
>   result->fe_time.month  = month_num + 1;
> because result->fe_time.month is 1-based month
> [1,12] whereas tm_mon is 0-based
> month [0,11].

Done.

http://codereview.chromium.org/126096/diff/1018/20#newcode477
Line 477: // NSPR wants year as XXXX
On 2009/06/17 17:56:36, wtc wrote:
> Remove this comment because this file no longer uses NSPR.
> You may replace it with
>   // base::Time::Exploded wants year as XXXX.

Done.

http://codereview.chromium.org/126096/diff/1018/20#newcode592
Line 592: result->fe_time.year = StringToInt(p + 0) - 1900;
On 2009/06/17 17:56:36, wtc wrote:
> This is wrong.  It should be:
>   result->fe_time.year = StringToInt(p + 0);

Done.

http://codereview.chromium.org/126096/diff/1018/20#newcode593
Line 593: result->fe_time.month  = StringToInt(p + 5) - 1;
On 2009/06/17 17:56:36, wtc wrote:
> This should be
>   result->fe_time.month  = StringToInt(p + 5);

Done.

http://codereview.chromium.org/126096/diff/1018/20#newcode597
Line 597: result->fe_time.month  = StringToInt(p) - 1;
On 2009/06/17 17:56:36, wtc wrote:
> This should be
>   result->fe_time.month  = StringToInt(p);

Done.

http://codereview.chromium.org/126096/diff/1018/20#newcode601
Line 601: result->fe_time.year += 100;
On 2009/06/17 17:56:36, wtc wrote:
> This should be followed by
>   result->fe_time.year += 1900;

Done.

http://codereview.chromium.org/126096/diff/1018/20#newcode716
Line 716: result->fe_time.year += 100;
On 2009/06/17 17:56:36, wtc wrote:
> This should be followed by
>   result->fe_time.year += 1900;

Done.

http://codereview.chromium.org/126096/diff/1018/20#newcode810
Line 810: result->fe_time.month = StringToInt(&p[35-18]) - 1;
On 2009/06/17 17:56:36, wtc wrote:
> This should be
>   result->fe_time.month = StringToInt(&p[35-18]);

Done.

http://codereview.chromium.org/126096/diff/1018/20#newcode814
Line 814: result->fe_time.year += 100;
On 2009/06/17 17:56:36, wtc wrote:
> This should be followed by
>   result->fe_time.year += 1900;

Done.

http://codereview.chromium.org/126096/diff/1018/20#newcode1139
Line 1139: result->fe_time.month = pos/3;
On 2009/06/17 17:56:36, wtc wrote:
> This should be
>   result->fe_time.month = pos/3 + 1;

Done.

http://codereview.chromium.org/126096/diff/1018/20#newcode1141
Line 1141: result->fe_time.year = StringToInt(tokens[4]) - 1900;
On 2009/06/17 17:56:36, wtc wrote:
> This should be
>   result->fe_time.year = StringToInt(tokens[4]);

Done.

http://codereview.chromium.org/126096/diff/1018/20#newcode1151
Line 1151: result->fe_time.year += 100;
On 2009/06/17 17:56:36, wtc wrote:
> This should be followed by
>   result->fe_time.year += 1900;

Done.

http://codereview.chromium.org/126096/diff/1018/20#newcode1349
Line 1349: result->fe_time.month = month_num - 1;
On 2009/06/17 17:56:36, wtc wrote:
> This should be
>   result->fe_time.month = month_num;

Done.

http://codereview.chromium.org/126096/diff/1018/20#newcode1365
Line 1365: time_t now = time(NULL);
On 2009/06/17 17:56:36, wtc wrote:
> Rewrite these four lines using base::Time, without going
> through time_t.

Done.

http://codereview.chromium.org/126096/diff/1018/19
File net/ftp/ftp_directory_parser.h (right):

http://codereview.chromium.org/126096/diff/1018/19#newcode45
Line 45: #include "base/message_loop.h"
On 2009/06/17 17:56:36, wtc wrote:
> Are you sure "base/message_loop.h" is the right header to
> include?  I don't see any MessageLoop in this header.
> Maybe you want "base/time.h" instead?

Done.

http://codereview.chromium.org/126096/diff/1018/19#newcode57
Line 57: base::Time::Exploded now_tm;  // needed for year determination.
On 2009/06/17 17:56:36, wtc wrote:
> Why did you move this member to the end of the struct?
> It would be better to move it back to the original position
> so we can compare this file with the original Mozilla file
> more easily.  It's also nice for it to be next to the related
> now_time member.
> 

Done.

http://codereview.chromium.org/126096/diff/1018/21
File net/url_request/url_request_new_ftp_job.cc (right):

http://codereview.chromium.org/126096/diff/1018/21#newcode126
Line 126: while (getline(iss, line)) {
On 2009/06/17 17:56:36, wtc wrote:
> Undo the indentation change.  'while' should be aligned with
> the previous line.

Done.

http://codereview.chromium.org/126096/diff/1018/21#newcode131
Line 131: result.fe_time.month++;
On 2009/06/17 17:56:36, wtc wrote:
> I believe you're incrementing result.fe_time.month here to
> compensate for all those off-by-one errors in ftp_directory_parser.cc.

True :),because don't want to change too much mozilla code. But I have changed.

[wtc, 2009-06-23 01:22:06]

On 2009/06/20 18:11:35, ibrar wrote:
> I am still looking.

OK, let me know when it's ready for me to review again.

In the future, you don't need to respond with "Done" to my suggested
changes invididually.  You can just tell me "I made all of your
suggested changes".

[ibrar, 2009-06-26 07:39:51]

Please review!

[wtc, 2009-06-26 22:30:08]

Ibrar,

Thanks a lot for the new Patch Set.  Please fix the problems
below.  I will have to review your new Patch Set and check
it in after I come back from vacation (7/11).

You can ask phajdan.jr to review if you would like a review
sooner.

http://codereview.chromium.org/126096/diff/5001/5003
File net/ftp/ftp_directory_parser.cc (right):

http://codereview.chromium.org/126096/diff/5001/5003#newcode599
Line 599: result->fe_time.year += 1900;
This is wrong.  What I meant is this:
  if (result->fe_time.year < 70)
    result->fe_time.year += 100;
  result->fe_time.year += 1900;

http://codereview.chromium.org/126096/diff/5001/5003#newcode714
Line 714: result->fe_time.year += 1900;
Same here.  What I meant is:
  if (result->fe_time.year < 80)
    result->fe_time.year += 100;
  result->fe_time.year += 1900;

http://codereview.chromium.org/126096/diff/5001/5003#newcode812
Line 812: result->fe_time.year += 1900;
This should be
  if (result->fe_time.year < 80)
    result->fe_time.year += 100;
  result->fe_time.year += 1900;

http://codereview.chromium.org/126096/diff/5001/5003#newcode988
Line 988: if (!state->now_time) {
Assuming we change now_time to a bool now_tm_valid member,
this block of code should look like this:
  if (!state->now_tm_valid) {
    Time t = Time::Now();
    t.LocalExplode(&state->now_tm);
    state->now_tm_valid = true;
  }

http://codereview.chromium.org/126096/diff/5001/5003#newcode1137
Line 1137: result->fe_time.month = pos / 3;
Are you sure you don't need to + 1 here?

http://codereview.chromium.org/126096/diff/5001/5003#newcode1148
Line 1148: if (result->fe_time.year < 80)  // SuperTCP
What I meant is this:
  if (result->fe_time.year < 80)  // SuperTCP
    result->fe_time.year += 100;
  result->fe_time.year += 1900;

http://codereview.chromium.org/126096/diff/5001/5003#newcode1362
Line 1362: if (!state->now_time) {
Assuming we change now_time to a bool now_tm_valid member,
this block of code should look like this:
  if (!state->now_tm_valid) {
    Time t = Time::Now();
    t.LocalExplode(&state->now_tm);
    state->now_tm_valid = true;
  }

http://codereview.chromium.org/126096/diff/5001/5002
File net/ftp/ftp_directory_parser.h (right):

http://codereview.chromium.org/126096/diff/5001/5002#newcode51
Line 51: // TODO(ibrar): Yet to see now_time should be int64 or base::Time
I studied the code.  now_time is used as a boolean to indicate
whether we need to compute now_tm.

So I suggest that we replace now_time with a bool member:
  bool       now_tm_valid;    // now_tm contains a valid time

[ibrar, 2009-07-11 08:11:20]

http://codereview.chromium.org/126096/diff/5001/5003
File net/ftp/ftp_directory_parser.cc (right):

http://codereview.chromium.org/126096/diff/5001/5003#newcode599
Line 599: result->fe_time.year += 1900;
On 2009/06/26 22:30:08, wtc wrote:
> This is wrong.  What I meant is this:
>   if (result->fe_time.year < 70)
>     result->fe_time.year += 100;
>   result->fe_time.year += 1900;

Done.

http://codereview.chromium.org/126096/diff/5001/5003#newcode714
Line 714: result->fe_time.year += 1900;
On 2009/06/26 22:30:08, wtc wrote:
> Same here.  What I meant is:
>   if (result->fe_time.year < 80)
>     result->fe_time.year += 100;
>   result->fe_time.year += 1900;

Done.

http://codereview.chromium.org/126096/diff/5001/5003#newcode812
Line 812: result->fe_time.year += 1900;
On 2009/06/26 22:30:08, wtc wrote:
> This should be
>   if (result->fe_time.year < 80)
>     result->fe_time.year += 100;
>   result->fe_time.year += 1900;

Done.

http://codereview.chromium.org/126096/diff/5001/5003#newcode988
Line 988: if (!state->now_time) {
On 2009/06/26 22:30:08, wtc wrote:
> Assuming we change now_time to a bool now_tm_valid member,
> this block of code should look like this:
>   if (!state->now_tm_valid) {
>     Time t = Time::Now();
>     t.LocalExplode(&state->now_tm);
>     state->now_tm_valid = true;
>   }

Done.

http://codereview.chromium.org/126096/diff/5001/5003#newcode1137
Line 1137: result->fe_time.month = pos / 3;
On 2009/06/26 22:30:08, wtc wrote:
> Are you sure you don't need to + 1 here?

Done.

http://codereview.chromium.org/126096/diff/5001/5003#newcode1148
Line 1148: if (result->fe_time.year < 80)  // SuperTCP
On 2009/06/26 22:30:08, wtc wrote:
> What I meant is this:
>   if (result->fe_time.year < 80)  // SuperTCP
>     result->fe_time.year += 100;
>   result->fe_time.year += 1900;

Done.

http://codereview.chromium.org/126096/diff/5001/5003#newcode1362
Line 1362: if (!state->now_time) {
On 2009/06/26 22:30:08, wtc wrote:
> Assuming we change now_time to a bool now_tm_valid member,
> this block of code should look like this:
>   if (!state->now_tm_valid) {
>     Time t = Time::Now();
>     t.LocalExplode(&state->now_tm);
>     state->now_tm_valid = true;
>   }

Done.