[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

Mark Lam <mark.lam at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #239986|                            |review+
              Flags|                            |

--- Comment #52 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 239986
  --> https://bugs.webkit.org/attachment.cgi?id=239986
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=239986&action=review

Almost there.  Please fix the remaining issues.  Thanks.

> Source/JavaScriptCore/ChangeLog:11
> +        By adding a argument to distinguish UTC and local time, we can get correct offset.

nit: “we can get correct offset” ==> "we can get the correct offset."

> Source/WTF/ChangeLog:11
> +        By adding a argument to distinguish UTC and local time, we can get correct offset

ditto.

> Source/JavaScriptCore/runtime/JSDateMath.cpp:197
>      double result = (day * WTF::msPerDay) + ms;

Rather than adding the comment below, I suggest renaming “result” to “localTimeResult”.  I think the code will be clearer that way.

> Source/JavaScriptCore/runtime/JSDateMath.cpp:232
>      double ms = WTF::parseDateFromNullTerminatedCharacters(dateString, haveTZ, offset);

Ditto: rename “ms” to “localTimeMS”.  With this the added comment below becomes unnecessary.

> Source/JavaScriptCore/runtime/VM.h:152
> +    WTF::InputTimeType inputTimeType;

I would express this as:
    WTF::TimeType typeTime;

I think that the fact that it was used as the input value to some time function is irrelevant to the nature of the type.  Hence, I don’t think we should be using “Input” in its name.

> Source/WTF/wtf/DateMath.cpp:474
> +LocalTimeOffset calculateLocalTimeOffset(double ms, InputTimeType inputTimeType)

You can leave the argument name here as “inputTimeType”.  I agree that it’s appropriate in this context.  But please replace the argument type with “TimeType”.

> Source/WTF/wtf/DateMath.cpp:479
> +    // Convert local time milliseconds to UTC milliseconds, if input time type is LocalTime.
> +    double inputTimeOffset = inputTimeType == LocalTime ? calculateUTCOffset() : 0;
> +    if (inputTimeOffset)
> +        ms -= inputTimeOffset;

I would remove this comment and rename “inputTimeOffset” to “localToUTCTimeOffset”.

> Source/WTF/wtf/DateMath.cpp:1104
> +        offset = calculateLocalTimeOffset(ms, LocalTime).offset / msPerMinute; // ms value is local time milliseconds.

nit: “is local” ==> “is in local”.

> LayoutTests/ChangeLog:8
> +        Set latest DST timezone bounday values on

typo: “bounday” ==> “boundary”?

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20141021/8e86ac16/attachment-0002.html>