[Webkit-unassigned] [Bug 130967] erroneous date calculations on march 30, 2014 (CET/CEST time zone) the day on which daylight savings time changes
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Oct 15 16:45:26 PDT 2014
https://bugs.webkit.org/show_bug.cgi?id=130967
Darin Adler <darin at apple.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #239314|review? |review+
Flag| |
--- Comment #50 from Darin Adler <darin at apple.com> 2014-10-15 16:45:20 PST ---
(From update of attachment 239314)
View in context: https://bugs.webkit.org/attachment.cgi?id=239314&action=review
Concept of the fix looks fine. Not sure I find the use of a boolean here sufficiently clear.
> Source/JavaScriptCore/ChangeLog:10
> + So, it gives errornous results while calculating offset of DST boundary time.
Typo: errornous instead of erroneous.
> Source/JavaScriptCore/runtime/JSDateMath.cpp:135
> +static LocalTimeOffset localTimeOffset(VM& vm, double ms, bool inputIsUTC = true)
I don’t think a boolean is a good way to express the “inputIsUTC”. It doesn’t make it clear what the input is if not UTC. I think we should use an enum instead with one value being LocalTime and the other being UTC.
> Source/JavaScriptCore/runtime/VM.h:136
> + , isutc(true)
isutc -> isUTC, but also should use that same enum
> Source/WTF/wtf/DateMath.cpp:364
>
> -#if !HAVE(TM_GMTOFF)
>
Should not leave double blank lines around. Please delete one.
> Source/WTF/wtf/DateMath.cpp:407
> +#if !HAVE(TM_GMTOFF)
> #if OS(WINDOWS)
Should have a blank line after the HAVE(TM_GMTOFF) line.
> Source/WTF/wtf/DateMath.cpp:466
> + double utcOffset = 0;
Do we really need to initialize this to zero? Seems like it always gets set before being used.
I would write this:
#if HAVE(TM_GMTOFF)
// Optimize case where the input is already UTC by not even calculating the offset.
double utcOffset = inputIsUTC ? 0 : calculateUTCOffset();
#else
double utcOffset = calculateUTCOffset();
#endif
// Convert local time milliseconds to UTF milliseconds.
if (!inputIsUTC)
ms -= utcOffset;
> Source/WTF/wtf/DateMath.cpp:468
> + // Convert current milliseconds to UTC milliseconds
Extra space after "Convert". Missing period after "milliseconds".
> Source/WTF/wtf/DateMath.cpp:503
> - double utcOffset = calculateUTCOffset();
> + if (inputIsUTC)
> + utcOffset = calculateUTCOffset();
Then I’d just delete this code entirely.
> Source/WTF/wtf/DateMath.cpp:1091
> bool haveTZ;
> int offset;
> +
> double ms = parseDateFromNullTerminatedCharacters(dateString, haveTZ, offset);
Why add this blank line? The old code intentionally groups the definition of the out arguments with the line of code that sets them.
> Source/WTF/wtf/DateMath.cpp:1097
> + offset = calculateLocalTimeOffset(ms, false).offset / msPerMinute; // ms value is not UTC milliseconds
The "false" argument here is really mysterious. And the comment isn’t that great either, because it says what the milliseconds value is *not*, but does not say what it *is*!
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list