386742 - get offset from UTC out of NSPR

Status: RESOLVED FIXED

(NSPR :: NSPR, enhancement)

Description

[Christian Eyrich]

For at least two features in Mozilla mail code it's necessary to have information on the timezone a date information originated in. Bugs are for example bug 118899 (though closed as fixed it's only fixed by a workaround) bug 360266 and bug 218926.

This information is available in NSPR's PR_ParseTimeString() but not outside of it. Four years ago I had a short correspondence with Wan-Teh in which he mentioned the possibility of adding a new function PR_ParseTimeStringToExplodedTime that has this prototype:

NSPR_API(PRStatus) PR_ParseTimeStringToExplodedTime (
         const char *string,
         PRBool default_to_gmt,
         PRExplodedTime *result);

I now want to solve the issue for real. As far as I can see this would need to duplicate the whole content code of PR_ParseTimeString(). So my question, is this (still) acceptable -- or is there another solution?

Comment 1

[Christian Eyrich]

Urm, dumb me. No huge code duplication needed if moving the most of it to the new function and using the old one as caller to it.
I'm currently about to test how to do it best and if the mail code can benefit from it.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

It appears to me that the whole point of the PR_ParseTimeStringToExplodedTime
proposal is to give the caller access to the time zone info in 
PRExplodedTime.tm_params.tp_gmt_offset

But note that the existing code does not set the contents of PRExplodedTime.tm_params at all.  So that code would need to be enhanced to
do that.  

Comment 3

[Christian Eyrich]

Attachment: proposed patch

Yes, the sole purpose of PR_ParseTimeStringToExplodedTime is to provide callers that access.

Attached patch that by moving the existing code to the new function and populate tm_params. The old function only calls the new one.
Doing that was fairly straightforward with one exception: no zone was specified, and we're to assume that everything is local. I replaced that code (and its call to mktime) by a call to PR_LocalTimeParameters. The alternative would have been adding additional calls to implode and explode in the new function, so I decided using before named approved function. I didn't notice drawbacks because of that.

So, please comment on that change.

Comment 4

[Wan-Teh Chang]

Comment on attachment 271841 [details] [diff] [review]
proposed patch

Christian, thank you for the patch.

There is a problem with the case where no zone was specified.

>   if (zone_offset == -1)
>          {
>            /* no zone was specified, and we're to assume that everything
>              is local. */
>+           PR_ASSERT(result->tm_month > -1
>+                     && result->tm_mday > 0
>+                     && result->tm_hour > -1
>+                     && result->tm_min > -1
>+                     && result->tm_sec > -1);
>+
>+           /* month, day, hours, mins and secs are always non-negative
>+              so we dont need to worry about them. */
>+           if (result->tm_year >= 1970)
>+                 result->tm_params = PR_LocalTimeParameters(result);
>+         }

The PR_LocalTimeParameters call is wrong.  The input parameter of
PR_LocalTimeParameters is a broken-down time (PRExplodedTime) in
GMT.  See http://developer.mozilla.org/en/docs/PR_LocalTimeParameters.
But 'result' is assumed to be in local time.

You should be able to solve this problem by using the original code
to get the PRTime value, and then calling PR_ExplodeTime with
PR_LocalTimeParameters as the second argument to get the
PRExplodedTime result.  This is inefficient, but hopefully this case is
rare that it doesn't matter.

Comment 5

[Christian Eyrich]

Attachment: patch v2

I wasn't aware of effects when calling PR_LocalTimeParameters with non-GMT-time. But I now tested edge cases around switching from/to DST.
So this patch now uses and additional PR_ExplodeTime(secs64, PR_LocalTimeParameters, result) as you proposed.

Comment 6

[Wan-Teh Chang]

Comment on attachment 271997 [details] [diff] [review]
patch v2

Christian, this patch looks good.  Please make the following changes.

>-                      LL_I2L(*result, secs);
>+                      PRTime secs64;
>+                      LL_I2L(secs64, secs);

Nit: please move the "PRTime secs64" declaration to the beginning of
the if block, before the #if defined(XP_MAC) && (__MSL__ < 0x6000) line.
Or you can remove the entire #if defined(XP_MAC) && (__MSL__ < 0x6000)
block, which is dead code.

Nit: this variable should be named usecs64 because PRTime is in
microseconds.

>+  /* mainly to compute wday and yday */
>+  PR_NormalizeTime(result, PR_GMTParameters);
>+  result->tm_params.tp_gmt_offset = zone_offset * 60;

Note that we do not set tm_params.tp_dst_offset.  We must document
this in prtime.h as a limitation of the PR_ParseTimeStringToExplodedTime
function, that it always returns a tp_dst_offset of 0, and the
returned tp_gmt_offset is the sum of the actual GMT and DST offsets.

Note: for some time strings, for example those that use the US time zone
names PST/PDT, EST/EDT, etc., the daylight savings time info is
actually available.  So I guess we could also work harder to extract
that information.

>+PR_IMPLEMENT(PRStatus)
>+PR_ParseTimeString(
>+        const char *string,
>+        PRBool default_to_gmt,
>+        PRTime *result)
>+{
>+  PRExplodedTime exploded;

Nit: you may be able to make the diffs smaller by naming this variable
'tm' as in the original PR_ParseTimeString function.

>+  PR_ParseTimeStringToExplodedTime(string,
>+                                   default_to_gmt,
>+                                   &exploded);

You should check the return value of this function.

In prtime.h, please add a comment documenting the new
PR_ParseTimeStringToExplodedTime function.  You can say it is like
the PR_ParseTimeString function except that it returns the result
as a PRExplodedTime, and note the limitation with tp_dst_offset
(of the current implementation).

Comment 7

[Christian Eyrich]

Attachment: patch addressing comment #6

Wan-Teh, thanks for your comments. I've tried to address them in this patch, also providing separate tp_dst_offset when this information is available.
I hope the comments in the header file ar to your liking.

Comment 8

[Wan-Teh Chang]

Comment on attachment 272308 [details] [diff] [review]
patch addressing comment #6

r=wtc.  Thanks!

We also know the DST offset for British Summer Time, so we should add
the following change to prtime.c:

                 case TT_GMT: zone_offset =  0 * 60; break;
-                case TT_BST: zone_offset =  1 * 60; break;
+                case TT_BST: zone_offset =  0 * 60; dst_offset = 1 * 60; break;                 case TT_MET: zone_offset =  1 * 60; break;

and "GMT/BST" to the comment for PR_ParseTimeStringToExplodedTime in
prtime.h.  I can take care of this when I check in your patch.

Comment 9

[Christian Eyrich]

Oh, indeed, thanks. So you're going to check that in without needing further steps from me?

Comment 10

[Wan-Teh Chang]

Attachment: Patch as checked in

I made some minor changes to the comments and formatting,
and added the necessary change to nspr.def to export this
new function (only used on Solaris right now).

I checked in the patch on the NSPR trunk for NSPR 4.7.

Checking in include/prtime.h;
/cvsroot/mozilla/nsprpub/pr/include/prtime.h,v  <--  prtime.h
new revision: 3.11; previous revision: 3.10
done
Checking in src/nspr.def;
/cvsroot/mozilla/nsprpub/pr/src/nspr.def,v  <--  nspr.def
new revision: 1.14; previous revision: 1.13
done
Checking in src/misc/prtime.c;
/cvsroot/mozilla/nsprpub/pr/src/misc/prtime.c,v  <--  prtime.c
new revision: 3.29; previous revision: 3.28
done