[webkit-reviews] review granted: [Bug 130967] erroneous date calculations on march 30, 2014 (CET/CEST time zone) the day on which daylight savings time changes : [Attachment 239314] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Byungseon Shin <sun.shin at lge.com>'s
request for review:
Bug 130967: erroneous date calculations on march 30, 2014 (CET/CEST time zone)
the day on which daylight savings time changes
https://bugs.webkit.org/show_bug.cgi?id=130967

Attachment 239314: Patch
https://bugs.webkit.org/attachment.cgi?id=239314&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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*!