Closed Bug 827816 Opened 11 years ago Closed 11 years ago

Wrong timezone offset for UK and Ireland as they had year-round DST from 1969-1971

Categories

(Core :: JavaScript Engine, defect, P2)

defect

Tracking

()

RESOLVED FIXED
B2G C4 (2jan on)
blocking-basecamp +
Tracking Status
firefox19 --- wontfix
firefox20 --- wontfix
firefox21 --- fixed
b2g18 --- fixed

People

(Reporter: atsai, Assigned: baku)

References

Details

Attachments

(3 files, 5 obsolete files)

BUILD ID: 20130107105806

Setting time zone from Berlin to London doesn't change the time.
Setting time zone from Moscow to London works fine.
For now, only Berlin to London has problems.
blocking-basecamp: --- → ?
blocking-basecamp: ? → +
Priority: -- → P2
Target Milestone: --- → B2G C4 (2jan on)
I'll have a look at it.
Assignee: nobody → ttaubert
Berlin to Dublin also doesn't change the time zone.
Strangely, after tzset(), localtime() reports the correct time:

http://mxr.mozilla.org/mozilla-central/source/hal/gonk/GonkHal.cpp#775

For some reason the statusbar displays the wrong time:

https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/statusbar.js#L288
(In reply to Benjamin Chen from comment #4)
> Maybe something wrong here
> 
> http://mxr.mozilla.org/mozilla-central/source/js/src/vm/DateTime.cpp#67

Thank you for the hint, will like into that.
(In reply to Tim Taubert [:ttaubert] from comment #5)
> Thank you for the hint, will like into that.

Haha, I meant 'look'. Gosh, I was tired.
Is this a platform bug?
I assume so.
Let's move to platform ;) (Gaia-=1)
Component: Gaia::Settings → General
PRMJ_LocalGMTDifference (from Gecko 18) returns -3600 for London as well as Berlin. Might be related to a TZ cache in the JS Engine if I understood it correctly.
(In reply to Benjamin Chen from comment #11)
> http://www.timeanddate.com/worldclock/timezone.html?n=136&syear=1970

Oh god, very good catch.

"The United Kingdom and Ireland experimented with year-round DST from 1968 to 1971 but abandoned it because of its unpopularity, particularly in northern regions."

https://en.wikipedia.org/wiki/Daylight_saving_time
I think we should then just use 1972 instead of 1970?
Status: NEW → ASSIGNED
Component: General → JavaScript Engine
OS: Gonk (Firefox OS) → All
Product: Boot2Gecko → Core
Hardware: ARM → All
Summary: [Settings] setting time zone from Berlin to London, the time does not change → Wrong timezone offset for UK and Ireland as they had year-round DST from 1969-1971
(In reply to Tim Taubert [:ttaubert] from comment #13)
> I think we should then just use 1972 instead of 1970?

OTOH, there could be some other country doing this in 1972. What about countries that changed their TZ offset since 1972? We're basically using a snapshot of 1970 TZs at the moment, right?
This also affects |new Date().getTimezoneOffset()|. It should be -60 for London for dates in DST or between 1969 and 1971 but it's now -60 as well after changing the TZ to Europe/London and we're not in DST.
PHP, for example, seems to have a big built-in database of all DST change timestamps and checks them for a given time. So that this would be true:

|new Date(1971, 1, 1).getTimezoneOffset() < new Date(1972, 1, 1).getTimezoneOffset()|
Blocks: 828315
(In reply to Tim Taubert [:ttaubert] from comment #16)
> PHP, for example, seems to have a big built-in database of all DST change
> timestamps and checks them for a given time. So that this would be true:
There is already a database 
/system/usr/share/zoneinfo/zoneinfo.dat zoneinfo.idx zoneinfo.version

if (!ComputeLocalTime(1356998400, &tm))
                      ^^^^^^^^^^
I think we should fill "current utc offset" here. Problem may be solved.
Sitting in Berlin with the appropriate timezone configured I get the following:

new Date(2012, 09, 28).getTimezoneOffset() == -120
new Date(2012, 09, 29).getTimezoneOffset() == -60

That would indicate a change to DST on the 29th of September. According to

http://www.timeanddate.com/worldclock/timezone.html?n=37

this should take place on the 28th of October 2012.
No longer blocks: 828315
(In reply to Tim Taubert [:ttaubert] from comment #18)
> Sitting in Berlin with the appropriate timezone configured I get the
> following:
> 
> new Date(2012, 09, 28).getTimezoneOffset() == -120
> new Date(2012, 09, 29).getTimezoneOffset() == -60
> 
> That would indicate a change to DST on the 29th of September. According to
> 
> http://www.timeanddate.com/worldclock/timezone.html?n=37
> 
> this should take place on the 28th of October 2012.

if (!ComputeLocalTime(1356998400, &tm))
                      ^^^^^^^^^^
I guess we also need to fill "given utc offset" for this case when |new Date("given date")|

There is another thing we might check here is that "what's the legal date range?". 
ex: Calendar can new a date(2030/1/1) but the database only covers 1970~2025.
Blake said he could take this.
Assignee: ttaubert → mrbkap
(In reply to Tim Taubert [:ttaubert] from comment #16)
> |new Date(1971, 1, 1).getTimezoneOffset() < new Date(1972, 1, 1).getTimezoneOffset()|

Nevermind comment #16, according to ES5 15.9.1.8., both .getTimezoneOffset() values must be zero for Jan 1st in Ireland/UK.

"The implementation of ECMAScript should not try to determine whether the exact time was subject to daylight saving time, but just whether daylight saving time would have been in effect if the current daylight saving time algorithm had been used at the time. This avoids complications such as taking into account the years that the locale observed daylight saving time year round."
Attached patch patch (obsolete) — Splinter Review
See the discussion in the bug. This patch uses the current date.
Assignee: mrbkap → amarchesini
Attachment #700094 - Flags: review?(jwalden+bmo)
Comment on attachment 700094 [details] [diff] [review]
patch

Review of attachment 700094 [details] [diff] [review]:
-----------------------------------------------------------------

I would like an automated JS test for this, but seeing as this relies on the result of time(), I don't think it's possible to do, unfortunately.  :-(  So this is good enough.

::: js/src/vm/DateTime.cpp
@@ +38,5 @@
>  
>      /*
>       * Get the difference between this time zone and GMT, by checking the local
> +     * time for the current date or the current date + 180 days using a date
> +     * for which daylight savings time was not in effect.

"daylight saving time", if you're going to touch this.  ;-)

Going by the previous discussion here, the assumption here is wrong: current date + 180 days isn't always going to get a non-DST date.  (Suppose someone's running a computer in the UK and sets its current time to 1970.)  Could you file a new bug on figuring out a way to do this that doesn't depend on this buggy (at least as I'm understanding it) algorithm?

@@ +43,3 @@
>       */
>      int day = 0;
> +    time_t t = time(0);

s/0/NULL/ (note that js/src doesn't yet use nullptr, and never used nsnull)
Attachment #700094 - Flags: review?(jwalden+bmo) → review+
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #23)
> Comment on attachment 700094 [details] [diff] [review]
> 
> Going by the previous discussion here, the assumption here is wrong: current
> date + 180 days isn't always going to get a non-DST date.  (Suppose
> someone's running a computer in the UK and sets its current time to 1970.) 
> Could you file a new bug on figuring out a way to do this that doesn't
> depend on this buggy (at least as I'm understanding it) algorithm?
> 

OOC, why would anyone set their current time to something other than actual current time (or at least something reasonable... ie not 1970)?  Is this condition something we've actually seen in the wild and spend time correcting?
I can think of cases where someone might want to run with a clock not synced to the current time -- think for VMs, possibly.  It's a correctness issue regardless, tho.  (If a rather small one.)

But is that the only place where DST reigns more than 180 days in a row?  Or ever will reign?  I wouldn't assume so.  And indeed, cursory Wikipedia consultation reveals that Russia and Belarus are on permanent DST now:

http://en.wikipedia.org/wiki/Daylight_saving_time
http://www.timeanddate.com/news/time/russia-winter-time.html
http://www.timeanddate.com/news/time/belarus-eternal-dst.html

So this +180d thing really is dodgy in certain areas, and definitely does deserve a new bug.
Although, um.  Come to think of it, does that patch possibly just shift the problem away from UK/Ireland, and over to Russia/Belarus?  Thus making it not a strict correctness win, but rather a win for the UK, and a loss for Russia?  If so -- someone please test this -- then maybe I shouldn't be r+ing it.

Someone with more b2g chops and awareness of needs there should comment on this!
How about this: just remove the DST offset after ComputeLocalTime()

//int tm_isdst;/* DST.		[-1/0/1]*/

if (tm.tm_isdst == 1) {
  LOG("tm_isdst +1");
  time -= SecondsPerDay*1000;
}
else if(tm.tm_isdst == -1)
{
  LOG("tm_isdst -1");
  time += SecondsPerDay*1000;
}

==
Does this patch attachment 700094 [details] [diff] [review] (using current UTC offeset) solve the case at comment 18 comment 19 for a given date?
(In reply to Benjamin Chen from comment #27)
> How about this: just remove the DST offset after ComputeLocalTime()
> 
> //int tm_isdst;/* DST.		[-1/0/1]*/
> 
> if (tm.tm_isdst == 1) {
>   LOG("tm_isdst +1");
>   time -= SecondsPerDay*1000;
> }
> else if(tm.tm_isdst == -1)
> {
>   LOG("tm_isdst -1");
>   time += SecondsPerDay*1000;
> }
s/SecondsPerDay/SecondsPerHour
Summary: Wrong timezone offset for UK and Ireland as they had year-round DST from 1969-1971 → [Settings] setting time zone from Berlin to London, the time does not change
Comment on attachment 700209 [details] [diff] [review]
1. use current UTC offset in LocalUTCDifferenceSeconds. 2. fix computeDSTOffsetMillis

Review of attachment 700209 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/DateTime.cpp
@@ +45,3 @@
>  
> +    int32_t time;
> +    time = (tm.tm_gmtoff - (tm.tm_isdst * SecondsPerHour)) % SecondsPerDay;

Looks like tm_gmtoff is a glibc invention, so no Windows support here :(
Why did you change the bug summary back? We know the cause of the bug and it's not very specific to the B2G settings app?
(In reply to Tim Taubert [:ttaubert] from comment #30)
> Comment on attachment 700209 [details] [diff] [review]
> 1. use current UTC offset in LocalUTCDifferenceSeconds. 2. fix
> computeDSTOffsetMillis
> 
> Review of attachment 700209 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/vm/DateTime.cpp
> @@ +45,3 @@
> >  
> > +    int32_t time;
> > +    time = (tm.tm_gmtoff - (tm.tm_isdst * SecondsPerHour)) % SecondsPerDay;
> 
> Looks like tm_gmtoff is a glibc invention, so no Windows support here :(

Maybe I should write like this:

int32_t time = (tm.tm_hour * 3600) + (tm.tm_min * 60) + tm.tm_sec;
time -= tm.tm_isdst * SecondsPerHour;
(In reply to Tim Taubert [:ttaubert] from comment #31)
> Why did you change the bug summary back? We know the cause of the bug and
> it's not very specific to the B2G settings app?

I change it back because the timezone of UK at 1970 is the same as Berlin (+1).
http://www.timeanddate.com/worldclock/timezone.html?n=136&syear=1970

What's wrong here is that we are using a snapshot of 1970 for timezone at comment 14.
(In reply to Benjamin Chen from comment #32)
> Maybe I should write like this:
> 
> int32_t time = (tm.tm_hour * 3600) + (tm.tm_min * 60) + tm.tm_sec;
> time -= tm.tm_isdst * SecondsPerHour;

You'd then just take the current time offset from the start of the day and subtract the DST offset, no?

(In reply to Benjamin Chen from comment #33)
> I change it back because the timezone of UK at 1970 is the same as Berlin
> (+1).
> http://www.timeanddate.com/worldclock/timezone.html?n=136&syear=1970
> 
> What's wrong here is that we are using a snapshot of 1970 for timezone at
> comment 14.

Sorry, that doesn't make sense to me. The old summary describes exactly that.
Whiteboard: [likely won't be reviewed until evening CET]
(In reply to Tim Taubert [:ttaubert] from comment #34)
> (In reply to Benjamin Chen from comment #32)
> > Maybe I should write like this:
> > 
> > int32_t time = (tm.tm_hour * 3600) + (tm.tm_min * 60) + tm.tm_sec;
> > time -= tm.tm_isdst * SecondsPerHour;
> 
> You'd then just take the current time offset from the start of the day and
> subtract the DST offset, no?
Yes, 
but now I am worried about that my patch only return positive value for LocalUTCDifferenceSeconds. But the function looks like return both positive and negative value.

> (In reply to Benjamin Chen from comment #33)
> > I change it back because the timezone of UK at 1970 is the same as Berlin
> > (+1).
> > http://www.timeanddate.com/worldclock/timezone.html?n=136&syear=1970
> > 
> > What's wrong here is that we are using a snapshot of 1970 for timezone at
> > comment 14.
> 
> Sorry, that doesn't make sense to me. The old summary describes exactly that.
Sorry I misunderstand the summary you changed. I'll change it back.
(In reply to Benjamin Chen from comment #35)
> > You'd then just take the current time offset from the start of the day and
> > subtract the DST offset, no?
> Yes, 
> but now I am worried about that my patch only return positive value for
> LocalUTCDifferenceSeconds. But the function looks like return both positive
> and negative value.

Yes, indeed. It needs to return -3600 for UTC+1 and e.g. +3600 for UTC-1.
Summary: [Settings] setting time zone from Berlin to London, the time does not change → Wrong timezone offset for UK and Ireland as they had year-round DST from 1969-1971
Benjamin, with your patch the timezone change seems working fine but the time is broken.
it seems you are working on this bug, so I was wondering if you want to continue or not.
Otherwise I can take it can propose a patch based on your contribute.
Attached patch compare localtime() and gmtime() (obsolete) — Splinter Review
How about just comparing the results of gmtime() and localtime() like this? This way we could just leverage the system's TZ database. This would automatically include DST offsets.
Attachment #700321 - Flags: feedback?(bechen)
Attachment #700321 - Flags: feedback?(amarchesini)
(In reply to Andrea Marchesini (:baku) from comment #37)
> Benjamin, with your patch the timezone change seems working fine but the
> time is broken.
> it seems you are working on this bug, so I was wondering if you want to
> continue or not.
> Otherwise I can take it can propose a patch based on your contribute.
Of course you can take it, I'm alright :) .
Comment on attachment 700321 [details] [diff] [review]
compare localtime() and gmtime()

(In reply to Tim Taubert [:ttaubert] from comment #38)
> Created attachment 700321 [details] [diff] [review]
> compare localtime() and gmtime()
> 
> How about just comparing the results of gmtime() and localtime() like this?
> This way we could just leverage the system's TZ database. This would
> automatically include DST offsets.
If the function's return value contains DST offset and also modify computeDSTOffsetMilliseconds funcion.
I guess the DSTOffesetCache can't be purge properly.
mxr.mozilla.org/mozilla-central/source/js/src/vm/DateTime.cpp#65

But maybe the final date result is correct.
mxr.mozilla.org/mozilla-central/source/js/src/jsdate.cpp#407

ex: From timezone "London +0 DST +1" change to "Berlin +1 DST +0"
Attachment #700321 - Flags: feedback?(bechen)
(In reply to Benjamin Chen from comment #41)
> ex: From timezone "London +0 DST +1" change to "Berlin +1 DST +0"

Hmmm, I see what you mean. I'm not sure I'm following computeDSTOffsetMilliseconds() and what that's supposed to return. Anyway we could just decrease the result by 3600 if |local.tm_isdst == 1|, no?
Sorry, I mean decrease the result of PRMJ_LocalGMTDifference().
(In reply to Tim Taubert [:ttaubert] from comment #42)
> (In reply to Benjamin Chen from comment #41)
> > ex: From timezone "London +0 DST +1" change to "Berlin +1 DST +0"
> 
> Hmmm, I see what you mean. I'm not sure I'm following
> computeDSTOffsetMilliseconds() and what that's supposed to return. Anyway we
> could just decrease the result by 3600 if |local.tm_isdst == 1|, no?
Seems not, I found I was wrong at comment 28.

http://linux.die.net/man/3/gmtime_r
tm_isdst
A flag that indicates whether daylight saving time is in effect at the time described. The value is positive if daylight saving time is in effect, zero if it is not, and negative if the information is not available.
Sorry, I'm not following. Decreasing by 3600 should be right if we're in DST. gmtime() will always return tm_isdst==0 because GMT is never in DST. localtime() will behave as expected and set the value.
Comment on attachment 700209 [details] [diff] [review]
1. use current UTC offset in LocalUTCDifferenceSeconds. 2. fix computeDSTOffsetMillis

This patch is obsolete per baku.
Attachment #700209 - Attachment is obsolete: true
Attachment #700209 - Flags: feedback?(jwalden+bmo)
Whiteboard: [likely won't be reviewed until evening CET]
> http://linux.die.net/man/3/gmtime_r
> tm_isdst
> A flag that indicates whether daylight saving time is in effect at the time
> described. The value is positive if daylight saving time is in effect, zero
> if it is not, and negative if the information is not available.

Gmtime returns UTC and UTC does not change with a change of seasons. That flag is always 0 for gmtime.
Attached patch patc b (obsolete) — Splinter Review
My patch is strongly based on the Tim's one. Feedback?
Attachment #700094 - Attachment is obsolete: true
Attachment #700321 - Attachment is obsolete: true
Attachment #700321 - Flags: feedback?(amarchesini)
Attachment #700664 - Flags: review?(jwalden+bmo)
Attachment #700664 - Flags: feedback?(bechen)
Andrea explain to me the patch. The basic idea here is that we are currently calculating the difference between non-timezone-aware hour and timezone-aware hour based on epoch 0, which has a different timezone layout that today. Instead, we should do the same calculation based on today's day. Stealing the review to get this moving. Will flag Waldo for feedback, post-landing review, follow-up fixes and also suggestions on how the hell to fix this.
Talked to Andrea. I think we have to do local time - utc time in the first if branch. He will draw this again and comment.
(In reply to Andreas Gal :gal from comment #50)
> Talked to Andrea. I think we have to do local time - utc time in the first
> if branch. He will draw this again and comment.

That would result in a return value of +3600 for Berlin - it has to be -3600. For some reason positive UTC offsets return a negative value here, IIRC.
Yeah, comment #51 is correct. We have to return the negative of the offset, so the current code is correct. r=me
Comment on attachment 700664 [details] [diff] [review]
patc  b

># HG changeset patch
># Parent fc3ed72129d9f6709d1bf7706417d3d949e7c927
># User Andrea Marchesini <amarchesini@mozilla.com>
>Bug 827816 - Wrong timezone offset for UK and Ireland as they had year-round DST from 1969-1971
>
>diff --git a/js/src/vm/DateTime.cpp b/js/src/vm/DateTime.cpp
>--- a/js/src/vm/DateTime.cpp
>+++ b/js/src/vm/DateTime.cpp
>@@ -19,51 +19,61 @@ ComputeLocalTime(time_t local, struct tm
>     struct tm *otm = localtime(&local);
>     if (!otm)
>         return false;
>     *ptm = *otm;
>     return true;
> #endif
> }
> 
>+static bool
>+ComputeUTCTime(time_t t, struct tm *ptm)
>+{
>+#ifdef HAVE_GMTIME_R
>+    return gmtime_r(&t, ptm);
>+#else
>+    struct tm *otm = gmtime(&t);
>+    if (!otm)
>+        return false;
>+    *ptm = *otm;
>+    return true;
>+#endif
>+}
>+
> static int32_t
> LocalUTCDifferenceSeconds()
> {
>     using js::SecondsPerDay;
>+    using js::SecondsPerHour;
> 
> #if defined(XP_WIN)
>     // Windows doesn't follow POSIX: updates to the TZ environment variable are
>     // not reflected immediately on that platform as they are on other systems
>     // without this call.
>     _tzset();
> #endif
> 
>-    /*
>-     * Get the difference between this time zone and GMT, by checking the local
>-     * time for days 0 and 180 of 1970, using a date for which daylight savings
>-     * time was not in effect.
>-     */
>-    int day = 0;
>-    struct tm tm;
>+    time_t t = time(NULL);
>+    struct tm local, utc;
> 
>-    if (!ComputeLocalTime(0, &tm))
>+    if (!ComputeLocalTime(t, &local) || !ComputeUTCTime(t, &utc))
>         return 0;
>-    if (tm.tm_isdst > 0) {
>-        day = 180;
>-        if (!ComputeLocalTime(SecondsPerDay * day, &tm))
>-            return 0;
>+
>+    int utc_secs = utc.tm_hour * SecondsPerHour + utc.tm_min * 60;
>+    int local_secs = local.tm_hour * SecondsPerHour + local.tm_min * 60;
>+
>+    if (utc.tm_mday == local.tm_mday) {
>+      return utc_secs - local_secs;
>+    } else if (utc_secs > local_secs) {

No else needed here after return.

>+      // Local date comes after UTC (offset in the positive range).
>+      return utc_secs - SecondsPerDay - local_secs;

No else needed here either.

>+    } else {
>+      // Local date comes before UTC (offset in the negative range).
>+      return SecondsPerDay - local_secs + utc_secs;
>     }
>-
>-    int32_t time = (tm.tm_hour * 3600) + (tm.tm_min * 60) + tm.tm_sec;
>-    time = SecondsPerDay - time;
>-
>-    if (tm.tm_yday == day)
>-        time -= SecondsPerDay;
>-
>-    return time;
> }
> 
> void
> js::DateTimeInfo::updateTimeZoneAdjustment()
> {
>     double newTZA = -(LocalUTCDifferenceSeconds() * msPerSecond);
>     if (newTZA == localTZA_)
>         return;
Attachment #700664 - Flags: review?(jwalden+bmo)
Attachment #700664 - Flags: review+
Attachment #700664 - Flags: feedback?(jwalden+bmo)
Comment on attachment 700664 [details] [diff] [review]
patc  b

>+    if (utc.tm_mday == local.tm_mday) {
>+      return utc_secs - local_secs;
>+    } else if (utc_secs > local_secs) {
>+      // Local date comes after UTC (offset in the positive range).
>+      return utc_secs - SecondsPerDay - local_secs;
>+    } else {
>+      // Local date comes before UTC (offset in the negative range).
>+      return SecondsPerDay - local_secs + utc_secs;
>     }

else after return (twice) is a non-sequitur.

The comment 49 through coment 53 information should be summarized and added as a code comment in the patch.

r=me with these fixed. Thanks!

/be
Attachment #700664 - Flags: review+
Attached patch patchSplinter Review
Attachment #700664 - Attachment is obsolete: true
Attachment #700664 - Flags: feedback?(jwalden+bmo)
Attachment #700664 - Flags: feedback?(bechen)
Attachment #700717 - Flags: review+
Attachment #700717 - Flags: feedback?(jwalden+bmo)
No empty { } around single-line consequence please. And an empty line after the single line if.
Yow, I did not look closely enough.

Andrea: besides the over-bracing of single-line "then" clause raised in comment 57, the c-basic-offset for this code is 4 not 2. Please fix indentation to match, rs=me (rs meaning "rubberstamp") and cite the hg.mozilla.org link here.

/be
Attached patch patch for b2g18Splinter Review
I'll land this patch to b2g18 tomorrow.
Comment on attachment 700717 [details] [diff] [review]
patch

Review of attachment 700717 [details] [diff] [review]:
-----------------------------------------------------------------

The approach looks worlds better than the previous way, or the previously proposed way.  r/f/whatever=me for the approach here.

However, I do have a few style nits that need fixing, and some comments that are very confusing to read.  I don't think I need to see any of those changes, so rs=me on that.

Once you land these nitpicks I'll take on doing the followup naming/semantics cleanups I mentioned.

::: js/src/vm/DateTime.cpp
@@ +41,2 @@
>  static int32_t
>  LocalUTCDifferenceSeconds()

This name's insufficiently informative about exactly what difference is being returned.  And perhaps we want to return the other kind of offset, even -- traditional notation is GMT-8 and such, not +8 as the offset, so there's a good argument for returning the UTC-to-local offset, not the local-to-UTC offset as we do here currently.

But we don't really want to fix that here, I think, as we're in a b2g rush and looking to minimize the change/semantics delta.  I'll do it in a followup.

@@ +58,4 @@
>          return 0;
> +
> +    int utc_secs = utc.tm_hour * SecondsPerHour + utc.tm_min * 60;
> +    int local_secs = local.tm_hour * SecondsPerHour + local.tm_min * 60;

60 should be SecondsPerMinute, using'd from js like the others.

@@ +68,1 @@
>      }

No braces around this, and four-space indentation.  (Someone else might have said this, to be sure; I only very very lightly skimmed previous review/feedback here.)

@@ +68,3 @@
>      }
> +    if (utc_secs > local_secs) {
> +      // Local date comes before UTC (offset in the negative range).

This comment confuses me greatly.  Local time and UTC are different days here.  When utc_seconds is greater than local_secs, this is when UTC is near the end of a day, and local is near the start of the next.  So then isn't the local time *after* UTC?

Or is this a matter of understanding of the word "come", where, say, 17:42 is reached in the local time zone before it's reached in UTC?  Sigh.

@@ +68,4 @@
>      }
> +    if (utc_secs > local_secs) {
> +      // Local date comes before UTC (offset in the negative range).
> +      return utc_secs - SecondsPerDay - local_secs;

I think this makes slightly clearer what we're doing here:

        return utc_secs - (local_secs + SecondsPerDay);

Also four-space indentation again.

@@ +70,5 @@
> +      // Local date comes before UTC (offset in the negative range).
> +      return utc_secs - SecondsPerDay - local_secs;
> +    }
> +    // Local date comes after UTC (offset in the positive range).
> +    return SecondsPerDay - local_secs + utc_secs;

return (utc_secs + SecondsPerDay) - local_secs;
Attachment #700717 - Flags: feedback?(jwalden+bmo) → feedback+
Attached patch patch - follow up (obsolete) — Splinter Review
I'll land this patch tomorrow because now the tree mozilla-inbound is closed.
Attachment #700748 - Flags: review+
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #60)
> @@ +68,3 @@
> >      }
> > +    if (utc_secs > local_secs) {
> > +      // Local date comes before UTC (offset in the negative range).
> 
> This comment confuses me greatly.  Local time and UTC are different days
> here.  When utc_seconds is greater than local_secs, this is when UTC is near
> the end of a day, and local is near the start of the next.  So then isn't
> the local time *after* UTC?

Yeah, looks like there was some confusion. Attachment #700664 [details] [diff] had the right description. Local time comes after UTC with a positive offset.
I don't really understand what's going on here, but the only way to get this stuff right in the general case is to use the Olsen timezone database: https://www.iana.org/time-zones Not only are the rules different in almost every country there is, they change at least twice a year.  B2G is going to need to ship a copy of this database and have a scheme for pulling updates.
Zack, ECMAScript permits two different approaches here.  You can either use fully accurate time zone information, which is what you suggest in comment 63.  Or you can just pretend the time zone info *at this moment* applies at all times, which ECMAScript allows as a simplification (even if it's arguably total evil lies).  We do the latter right now.  I agree there's a pretty good case that we should do the former.  But there's certainly not time to do it now.
https://hg.mozilla.org/integration/mozilla-inbound/rev/22b5a4e8c639

Thanks for the comments. I fixed them and I landed the patch to inbound.
Attachment #700748 - Attachment is obsolete: true
Attachment #700928 - Flags: review+
resolved fixed per the new jst rules.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Blocks: 830257
Depends on: 937261
Depends on: 912710
Depends on: 879261
You need to log in before you can comment on or make changes to this bug.