365997 - Changes in Daylight Savings Time computations

Status: RESOLVED FIXED

(NSPR :: NSPR, defect, P1)

Description

[Nelson Bolyard (seldom reads bugmail)]

prtime.c seems to know that DST starts on the first Sunday in April and ends
on the last Sunday in October.  That's changing this year.  Need fix by 
mid-March.

Excerpt from cited URL:

"The United States has planned a change to its DST observance beginning in 2007. The Energy Policy Act of 2005 mandates that DST will start on the second Sunday 
in March and end on the first Sunday in November. In 2007, the start and stop dates will be March 11 and November 4, respectively. These dates are different from previous DST start and stop dates. In 2006, the dates were the first Sunday in April (April 2, 2006) and the last Sunday in October (October 29, 2006).

Some countries are still evaluating whether they will adopt the new rules for themselves. You should anticipate more changes in DST and time zone rules for countries that typically align with U.S. DST rules."

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch for PR_USPacificTimeParameters - v1

The only function I could find that seemed to know when 
Daylight Savings Time is in effect was PR_USPacificTimeParameters.
I believe this patch fixes it.  Please review.

Is there any other function that needs to be fixed?

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch for PR_USPacificTimeParameters - v2

D'oh!

Comment 3

[Wan-Teh Chang]

Comment on attachment 250563 [details] [diff] [review]
patch for PR_USPacificTimeParameters - v2

r=wtc.  Thank you for fixing this, Nelson.  Good patch.

PR_USPacificTimeParameters is the only function with the knowledge
of US Daylight Saving Time rules.  PR_LocalTimeParameters relies on
the OS for DST.

PR_USPacificTimeParameters is sample code to help people write a
PRTimeParamFn function.  Even though most applications probably
don't use it, it's good to keep it up to date.

>+#define firstSunday(mday, wday) (((mday + 6 - wday) % 7) + 1)

[nit] I find it easier to understand to rewrite to inner expression
as (mday - wday + 6).  A comment would help people understand the
formula.  This is how I understand it:
    mday - wday is a Sunday.  To compute the first Sunday, we
    want to reduce mday - wday modulo 7, but since mday starts
    from 1 instead of 0, we need to first subtract 1 from
    mday - wday, reduce modulo 7, and add 1 back.
    -1 equals 6 mod 7.  mday - wday + 6 guarantees the number
    we reduce modulo 7 is positive.

You may have a different way to explain the formula, in which
the expression (mday + 6 - wday) is more natural.

>+/* returns the mday for the N'th Sunday of the month. 
>+ * mday and wday are  first a given day in the month.
>+ * ndays is the number of days in that month.
>+ */

[nit] Format the block comment like this:

    /*
     * line 1
     * line 2
     * line 3
     */

It would be useful to explain what N = 0, 1, -1 means here,
rather than repeatedly where we set dst_start_Nth_Sunday and
dst_end_Nth_Sunday.

The sentence "mday and wday are  first a given day in the month."
needs work -- probably remove "first" or change it to "for"?

[nit] It may be more self-explanatory for NthSunday to be renamed
mdayNthSunday or mdayOfNthSunday.

>+    PRInt32 dst_start_month_days;

>+    PRInt32 dst_end_month_days;

[nit] Change "days" to "ndays".

[nit] The style of this file is to use variable names fooBarBaz
rather than foo_bar_baz.
 
>+	if (st.tm_mday < NthSun) {              /* Before starting Sunday
>+	    retVal.tp_dst_offset = 0L;
>+        } else if (st.tm_mday == NthSun) {      /* Starting Sunday */
>+	    /* 01:59:59 PST -> 03:00:00 PDT */
>+	    if (st.tm_hour < 2) {
>+		retVal.tp_dst_offset = 0L;
>+	    } else {
>+		retVal.tp_dst_offset = 3600L;
>+	    }
>+	} else {                                /* After starting Sunday */
>+	    retVal.tp_dst_offset = 3600L;
>         }

You can simplify the above as:

       /* On the starting Sunday, 01:59:59 PST -> 03:00:00 PDT */
       if ((st.tm_mday < NthSun) || (st.tm_mday == NthSun) && (st.tm_hour < 2)) {
           retVal.tp_dst_offset = 0L;
       } else {
           retVal.tp_dst_offset = 3600L;
       }

and similarly for the handling of the ending Sunday.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch for PR_USPacificTimeParameters, v3

This patch addresses wan-teh's comments above.
I noticed that the file's style for struct member names is xxx_xxx_xxx
so I changed the DST data to be part of a struct that has two instances.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch for NSPR QA test program "timetest"

This patch changes timetest to test the years 2005-2008 rather than
the years 1993-1996.  This tests its ability to use both the old and
new DST rules.  I'm satisfied that the new prtime code is working as 
intended, based on the results of this program in debug mode.

I also fixed some debug print code so that it now consistently prints
all or none of the debug info according to the debug mode flag.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 250727 [details] [diff] [review]
patch for PR_USPacificTimeParameters, v3

Wan-Teh has asked that I request these reviews from someone other than him.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 250728 [details] [diff] [review]
patch for NSPR QA test program "timetest"

Seeking review from other NSPR peers and owners.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Looking for timely reviewers

Comment 9

[Wan-Teh Chang]

Comment on attachment 250727 [details] [diff] [review]
patch for PR_USPacificTimeParameters, v3

r=wtc.  Good patch.

In the PR_USPacificTimeParameters function:

>+    DSTParams * dst;

should be declared with 'const'.

You didn't implement my last suggested change in comment 3.

Comment 10

[Wan-Teh Chang]

Comment on attachment 250727 [details] [diff] [review]
patch for PR_USPacificTimeParameters, v3

I checked in the "patch for PR_USPacificTimeParameters v3" on
the NSPR trunk (NSPR 4.7) and the NSPRPUB_PRE_4_2_CLIENT_BRANCH
(Mozilla 1.9 alpha 2).

Checking in prtime.c;
/cvsroot/mozilla/nsprpub/pr/src/misc/prtime.c,v  <--  prtime.c
new revision: 3.27; previous revision: 3.26
done

Checking in prtime.c;
/cvsroot/mozilla/nsprpub/pr/src/misc/prtime.c,v  <--  prtime.c
new revision: 3.12.2.14; previous revision: 3.12.2.13
done

Comment 11

[Wan-Teh Chang]

Comment on attachment 250728 [details] [diff] [review]
patch for NSPR QA test program "timetest"

I checked in the "patch for NSPR QA test program 'timetest'" on
the NSPR trunk (NSPR 4.7) and the NSPRPUB_PRE_4_2_CLIENT_BRANCH
(Mozilla 1.9 alpha 2).

Checking in timetest.c;
/cvsroot/mozilla/nsprpub/pr/tests/timetest.c,v  <--  timetest.c
new revision: 3.9; previous revision: 3.8
done

Checking in timetest.c;
/cvsroot/mozilla/nsprpub/pr/tests/timetest.c,v  <--  timetest.c
new revision: 3.7.4.2; previous revision: 3.7.4.1
done

Comment 12

[Wan-Teh Chang]

Both patches have been checked in on the NSPR_4_6_BRANCH for
NSPR 4.6.5.