[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 229741] 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 229741: Patch
https://bugs.webkit.org/attachment.cgi?id=229741&action=review

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


> Source/JavaScriptCore/ChangeLog:9
> +	   There are duplicataions on WTF and JavaScript Core,
> +	   so we remove the Timezone conversion and call the function in WTF.

You misspelled "duplications" here.  In addition, from looking at the code, I'm
not sure that it is really a duplication because the underlying implementation
of local time offset computation is different between what's in JSDateMath and
WTF::DateMath.

> Source/JavaScriptCore/runtime/JSDateMath.cpp:236
> -static double parseDateFromNullTerminatedCharacters(VM& vm, const char*
dateString)
> +static double parseDateFromNullTerminatedCharacters(const char* dateString)
>  {
> -    bool haveTZ;
> -    int offset;
> -    double ms = WTF::parseDateFromNullTerminatedCharacters(dateString,
haveTZ, offset);
> +    double ms = WTF::parseDateFromNullTerminatedCharacters(dateString);
>      if (std::isnan(ms) || fabs(ms) > WTF::maxECMAScriptTime)
>	   return std::numeric_limits<double>::quiet_NaN();
>  
> -    // fall back to local timezone
> -    if (!haveTZ)
> -	   offset = localTimeOffset(vm, ms).offset / WTF::msPerMinute;
> -
> -    return ms - (offset * WTF::msPerMinute);
> +    return ms;

Here, you are effectively delegating the localTimeOffset computation to the WTF
implementation.  However, at least on the surface, I see that the WTF
implementation of calculateLocalTimeOffset() differs from the JSDateMath
localTimeOffset().  I learned that ES5 has different rules for how JS is
supposed to handle day light savings time computations, and I think the local
time offset is responsible for that difference.  I'm not too familiar with what
the ES5 spec says specifically here, but I'd be reluctant to go with this
change unless we can have a greater understanding of what the ES5 spec says in
this area, and are able to show that the WTF implementation satisfies the ES5
requirements.

Another thing that clued me into this issue is that, after applying your patch,
there are still other parts of JSDateMath that uses the JSDateMath
localTineOffset() and not the WTF version.  If it can be shown that the WTF
implementation is correct and spec compliant for ES5, then everything should be
switched over so that we don't have inconsistent behavior in different parts of
the code.