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

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has denied Byungseon Shin <xingri at gmail.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 229431: Patch
https://bugs.webkit.org/attachment.cgi?id=229431&action=review

------- Additional Comments from Mark Lam <mark.lam at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=229431&action=review


> Source/JavaScriptCore/ChangeLog:10
> +	   String(new Date(Mar 30 2014 01:00:00)) is wrong in CET
> +	   https://bugs.webkit.org/show_bug.cgi?id=130967
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   According to calculateLocalTimeOffset define,
> +	    it accepts utcInMilliseconds as an Argument
> +	   So, we need to calculate UTC instead of localTime milliseconds

What is the issue?  I tried evaluating "String(new Date("Mar 30 2014 01:00:00
UTC+0100”))” in Safari, Chrome, and Firefox (note: CET is UTC+1 according to
http://www.timeanddate.com/library/abbreviations/timezones/), and they all show
the same string.  You need a test case that demonstrates the issue and will
serve as a regression test in the future.  If appropriate, please add the test
case to an existing Date test.

Also, I fail to follow your logic here.  What does calculateLocalTimeOffset()
have to do with your change in parseDateFromNullTerminatedCharacters()?

> Source/WTF/wtf/DateMath.cpp:470
> +double getUTCOffset() 
> +{
> +    return calculateUTCOffset();
> +}

If we really need to export this, then let’s export calculateUTCOffset().  No
need to create another function.

> Source/WTF/wtf/DateMath.h:159
> +using WTF::getUTCOffset;
>  using WTF::calculateLocalTimeOffset;

This shows that we should export WTF::calculateTimeOffset (if needed) instead
of creating a WTF::getUTCOffset just to wrap it but adds nothing.