[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]

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.