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)
Core
JavaScript Engine
Tracking
()
People
(Reporter: atsai, Assigned: baku)
References
Details
Attachments
(3 files, 5 obsolete files)
2.60 KB,
patch
|
baku
:
review+
Waldo
:
feedback+
|
Details | Diff | Splinter Review |
3.02 KB,
patch
|
Details | Diff | Splinter Review | |
2.18 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•11 years ago
|
blocking-basecamp: --- → ?
Updated•11 years ago
|
blocking-basecamp: ? → +
Priority: -- → P2
Target Milestone: --- → B2G C4 (2jan on)
Comment 2•11 years ago
|
||
Berlin to Dublin also doesn't change the time zone.
Comment 3•11 years ago
|
||
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
Comment 4•11 years ago
|
||
Maybe something wrong here http://mxr.mozilla.org/mozilla-central/source/js/src/vm/DateTime.cpp#67
Comment 5•11 years ago
|
||
(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.
Comment 6•11 years ago
|
||
(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.
Comment 7•11 years ago
|
||
Is this a platform bug?
Comment 8•11 years ago
|
||
I assume so.
Comment 10•11 years ago
|
||
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.
Comment 11•11 years ago
|
||
http://www.timeanddate.com/worldclock/timezone.html?n=136&syear=1970 http://mxr.mozilla.org/mozilla-central/source/js/src/vm/DateTime.cpp#47 quick fix: if (!ComputeLocalTime(1356998400, &tm)) // 2012/1/1
Comment 12•11 years ago
|
||
(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
Comment 13•11 years ago
|
||
I think we should then just use 1972 instead of 1970?
Updated•11 years ago
|
Status: NEW → ASSIGNED
Updated•11 years ago
|
Component: General → JavaScript Engine
OS: Gonk (Firefox OS) → All
Product: Boot2Gecko → Core
Hardware: ARM → All
Updated•11 years ago
|
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
Comment 14•11 years ago
|
||
(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?
Comment 15•11 years ago
|
||
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.
Comment 16•11 years ago
|
||
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()|
Comment 17•11 years ago
|
||
(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.
Comment 18•11 years ago
|
||
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.
Comment 19•11 years ago
|
||
(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.
Comment 21•11 years ago
|
||
(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."
Assignee | ||
Comment 22•11 years ago
|
||
See the discussion in the bug. This patch uses the current date.
Assignee: mrbkap → amarchesini
Attachment #700094 -
Flags: review?(jwalden+bmo)
Comment 23•11 years ago
|
||
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+
Comment 24•11 years ago
|
||
(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?
Comment 25•11 years ago
|
||
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.
Comment 26•11 years ago
|
||
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!
Comment 27•11 years ago
|
||
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?
Comment 28•11 years ago
|
||
(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
Comment 29•11 years ago
|
||
Attachment #700209 -
Flags: feedback?(jwalden+bmo)
Updated•11 years ago
|
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 30•11 years ago
|
||
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 :(
Comment 31•11 years ago
|
||
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?
Comment 32•11 years ago
|
||
(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;
Comment 33•11 years ago
|
||
(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.
Comment 34•11 years ago
|
||
(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.
Updated•11 years ago
|
Whiteboard: [likely won't be reviewed until evening CET]
Comment 35•11 years ago
|
||
(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.
Comment 36•11 years ago
|
||
(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.
Updated•11 years ago
|
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
Assignee | ||
Comment 37•11 years ago
|
||
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.
Comment 38•11 years ago
|
||
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)
Comment 40•11 years ago
|
||
(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 41•11 years ago
|
||
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)
Comment 42•11 years ago
|
||
(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?
Comment 43•11 years ago
|
||
Sorry, I mean decrease the result of PRMJ_LocalGMTDifference().
Comment 44•11 years ago
|
||
(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.
Comment 45•11 years ago
|
||
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 46•11 years ago
|
||
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)
Updated•11 years ago
|
Whiteboard: [likely won't be reviewed until evening CET]
Assignee | ||
Comment 47•11 years ago
|
||
> 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.
Assignee | ||
Comment 48•11 years ago
|
||
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)
Comment 49•11 years ago
|
||
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.
Comment 50•11 years ago
|
||
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.
Comment 51•11 years ago
|
||
(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.
Comment 52•11 years ago
|
||
Yeah, comment #51 is correct. We have to return the negative of the offset, so the current code is correct. r=me
Comment 53•11 years ago
|
||
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 54•11 years ago
|
||
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+
Assignee | ||
Comment 55•11 years ago
|
||
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)
Assignee | ||
Comment 56•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a88ed3a3ace
Comment 57•11 years ago
|
||
No empty { } around single-line consequence please. And an empty line after the single line if.
Comment 58•11 years ago
|
||
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
Assignee | ||
Comment 59•11 years ago
|
||
I'll land this patch to b2g18 tomorrow.
Comment 60•11 years ago
|
||
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+
Assignee | ||
Comment 61•11 years ago
|
||
I'll land this patch tomorrow because now the tree mozilla-inbound is closed.
Attachment #700748 -
Flags: review+
Comment 62•11 years ago
|
||
(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.
Comment 63•11 years ago
|
||
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.
Comment 64•11 years ago
|
||
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.
Assignee | ||
Comment 65•11 years ago
|
||
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+
Assignee | ||
Comment 66•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g18/rev/412dcf5498d4
Comment 67•11 years ago
|
||
resolved fixed per the new jst rules.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Updated•11 years ago
|
status-firefox21:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•