Bug 130967 – erroneous date calculations on march 30, 2014 (CET/CEST time zone) the day on which daylight savings time changes

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

How to reproduce:

1) make sure the computer has the time set to central european time (CET/CEST)
2) run this line of code at the console prompt:
var a = new Date(2014,2,30); new Date(a.setHours(1))

Expected result: 
Sun Mar 30 2014 01:00:00 GMT+0100 (CET) (one o'clock)

Actual result:
Sun Mar 30 2014 00:00:00 GMT+0100 (CET) (midnight)

[Csaba Osztrogonác]

*** Bug 131437 has been marked as a duplicate of this bug. ***

[Tibor Mészáros]

Created attachment 229133 [details]
Patch

This patch will fix the offset problems on the day of DST change.

[Build Bot]

Comment on attachment 229133 [details]
Patch

Attachment 229133 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5983393799995392

New failing tests:
js/date-constructor.html
js/dom/date-DST-time-cusps.html
js/dom/date-big-setdate.html

[Build Bot]

Created attachment 229139 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 229133 [details]
Patch

Attachment 229133 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6486867583172608

New failing tests:
js/date-constructor.html
js/dom/date-DST-time-cusps.html
js/dom/date-big-setdate.html

[Build Bot]

Created attachment 229140 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Byungseon Shin]

Created attachment 229431 [details]
Patch

[Byungseon Shin]

@Tibor, I have tested with your patch and could not get correct result.
Please check my patch and update your opinion it.

@Mark, Could you please review current patch?

[Mark Lam]

Comment on attachment 229431 [details]
Patch

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.

[Byungseon Shin]

Comment on attachment 229431 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:10
>> +        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()?

Could you please test it without setting Timezone explicitly while setting your system timezone to Spain(Madrid)?
It will shows you "Sun Mar 30 2014 00:00:00 GMT+0100 (CET)" instead of "Sun Mar 30 2014 01:00:00 GMT+0100 (CET)."

This issue came from parseDateFromNullTerminatedCharacters just convert "Mar 30 2014 01:00:00" to milliseconds without considering UTC when no TimeZone information provided.

>> Source/WTF/wtf/DateMath.cpp:470
>> +}
> 
> If we really need to export this, then let’s export calculateUTCOffset().  No need to create another function.

I will do it.

>> Source/WTF/wtf/DateMath.h:159
>>  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.

I will do it.

[Mark Lam]

(In reply to comment #10)
> Could you please test it without setting Timezone explicitly while setting your system timezone to Spain(Madrid)?
> It will shows you "Sun Mar 30 2014 00:00:00 GMT+0100 (CET)" instead of "Sun Mar 30 2014 01:00:00 GMT+0100 (CET)."
> 
> This issue came from parseDateFromNullTerminatedCharacters just convert "Mar 30 2014 01:00:00" to milliseconds without considering UTC when no TimeZone information provided.

OK, I can see the issue if I change my machine time zone.  Can you think of a way to write a test for this that does not require a machine time zone change in order to verify the issue?

[Byungseon Shin]

Created attachment 229609 [details]
Patch

[Byungseon Shin]

We couldn't make a unit test for it, because there are no JavaScript API to specify System TimeZone.

It only possible to do manual test by creating Date() and see if it shows correctly.
I have checked that PDT/PST also has same issue.

ex)  Date('Mar 09 2014 03:00:00') ~  Date('Mar 09 2014 09:59:59')

[Build Bot]

Comment on attachment 229609 [details]
Patch

Attachment 229609 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4802016119357440

New failing tests:
js/regress-131248.html

[Build Bot]

Created attachment 229614 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 229609 [details]
Patch

Attachment 229609 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6235957523120128

New failing tests:
js/regress-131248.html

[Build Bot]

Created attachment 229622 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 229609 [details]
Patch

Attachment 229609 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4751280744431616

New failing tests:
js/regress-131248.html

[Build Bot]

Created attachment 229629 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Philipp Metzler]

In order to write a unit test for that a wrapper function could be used in the lower level. It returns the system time and when run from a unit test it uses the datetime provided as a parameter.

[Byungseon Shin]

Created attachment 229724 [details]
Patch

[Byungseon Shin]

Created attachment 229741 [details]
Patch

[Mark Lam]

Comment on attachment 229741 [details]
Patch

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.

[Byungseon(Sun) Shin]

Created attachment 238783 [details]
Patch

[Byungseon(Sun) Shin]

Created attachment 238785 [details]
Patch

[Build Bot]

Comment on attachment 238785 [details]
Patch

Attachment 238785 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4615080092106752

New failing tests:
js/dom/date-DST-time-cusps.html

[Build Bot]

Created attachment 238786 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 238785 [details]
Patch

Attachment 238785 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4797469367992320

New failing tests:
js/dom/date-DST-time-cusps.html

[Build Bot]

Created attachment 238788 [details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-02  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 238785 [details]
Patch

Attachment 238785 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6337503980158976

New failing tests:
js/dom/date-DST-time-cusps.html

[Build Bot]

Created attachment 238793 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Byungseon(Sun) Shin]

Created attachment 238894 [details]
Patch

[José Dapena Paz]

Comment on attachment 238894 [details]
Patch

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

Could you detail, in the bug reported issue, the outcome after applying the patch? I.e. what happens at new Date(2014, 3, 30, 2, 0) and new Data(2014, 3, 30, 2, 10)? Both cases getHours should return 3.

> LayoutTests/ChangeLog:9
> +        DST starts at April 25, 1982 at 3:00 am, so it is correct to return 2:00 am.

The meaning of this is: DST starts at 3:00 am. So at 2am, it started to be 3am. So it should return 3:00am PDT. The original test is right.

> LayoutTests/js/dom/script-tests/date-DST-time-cusps.js:12
> +    testCases.push(["(new Date(1982, 2, 14, 2)).getHours()", "2"]);

Original test was right. At 2am it became 3am, and DST started. So the right outcome is 3.

> LayoutTests/js/dom/script-tests/date-DST-time-cusps.js:15
>  }

Likely a good idea would be adding new sets of tests for other timezones, so build bots with different timezones would still exercise this.

[Byungseon(Sun) Shin]

What I mean is that DST starts at April 25, 1982, so March 14, 1982 couldn't be applicable for DST time conversion because it is before DST start time.

[Byungseon(Sun) Shin]

We can get the result 2 of "new Date(1982, 2, 14, 2).getHours()" on Firefox when using PST/PDT Timezone.

[Geoffrey Garen]

<rdar://problem/18507871>

[Byungseon(Sun) Shin]

Created attachment 239077 [details]
Patch

[Build Bot]

Comment on attachment 239077 [details]
Patch

Attachment 239077 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5295305201287168

New failing tests:
js/dom/date-DST-time-cusps.html

[Build Bot]

Created attachment 239083 [details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-02  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 239077 [details]
Patch

Attachment 239077 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6266776337776640

New failing tests:
js/dom/date-DST-time-cusps.html

[Build Bot]

Created attachment 239085 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 239077 [details]
Patch

Attachment 239077 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5159455385714688

New failing tests:
js/dom/date-DST-time-cusps.html

[Build Bot]

Created attachment 239088 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Byungseon(Sun) Shin]

Created attachment 239091 [details]
Patch

[José Dapena Paz]

Comment on attachment 239091 [details]
Patch

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

> LayoutTests/js/dom/script-tests/date-DST-time-cusps.js:12
> +    testCases.push(["(new Date('Mar 09 2014 03:00:00')).getTimezoneOffset()", "420"]);

Here you should exercise Mar 09 2014 02:00:00, and 02:10:00 and getHours should return 3 in both cases (as in the original test case).

[Byungseon Shin]

No, I could not agree with you. That makes WebKit returns wrong date like this bug.
Could you provide any rule on expressing non existence time value?

[José Dapena Paz]

Comment on attachment 239091 [details]
Patch

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

>> LayoutTests/js/dom/script-tests/date-DST-time-cusps.js:12
>> +    testCases.push(["(new Date('Mar 09 2014 03:00:00')).getTimezoneOffset()", "420"]);
> 
> Here you should exercise Mar 09 2014 02:00:00, and 02:10:00 and getHours should return 3 in both cases (as in the original test case).

On Firefox the outcome is 3. On Chrome it is 1. I think both are valid. So we can proceed with this. The ES standard is not clear about how to handle this case anyway.

[Byungseon(Sun) Shin]

Created attachment 239313 [details]
Patch

[Byungseon(Sun) Shin]

Created attachment 239314 [details]
Patch

[Darin Adler]

Comment on attachment 239314 [details]
Patch

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*!

[Byungseon(Sun) Shin]

Created attachment 239986 [details]
Patch

[Mark Lam]

Comment on attachment 239986 [details]
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”?

[Mark Lam]

Comment on attachment 239986 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSDateMath.cpp:199
>      if (!inputIsUTC)

Also, now that you have the WTF::TimeType, please change the gregorianDateTimeToMS() to take a "WTF::TimeType inputTimeType" arg instead of the "bool inputIsUTC".

Similarly, please change msToGregorianDateTime() to take a "WTF::TimeType outputTimeType" arg instead the bool.
Also change setNewValueFromTimeArgs() and setNewValueFromDateArgs() in DatePrototype.cpp accordingly.
Also remove "__ZN3JSC21msToGregorianDateTimeEPNS_9ExecStateEdbRN3WTF17GregorianDateTimeE" and "__ZN3JSC21gregorianDateTimeToMSEPNS_9ExecStateERKN3WTF17GregorianDateTimeEdb" from the JavaScriptCore.order file.

I think this will increase code clarity.

>> 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”.

Upon further review, I see what Darin was suggesting.  Let's re-write this (as Darin suggested) as:

#if HAVE(TM_GMTOFF)
    double utcOffset = inputTimeType == LocalTime ? calculateUTCOffset() : 0;
#else
    double utfOffset = calculateUTCOffset();
#endif

    if (inputTimeType == LocalTime)
        ms -= utcOffset;

This way the utfOffset value is initialized in only one place (for better clarity), and there's no chance of calculateUTCOffset() being called 2 times (for when the local time is UTC time).

> Source/WTF/wtf/DateMath.cpp:511
> -    double utcOffset = calculateUTCOffset();
> +    // If we alredy calculated UTC offset to convert input time, then use it as an offset.
> +    double utcOffset = inputTimeOffset ? inputTimeOffset : calculateUTCOffset();

You can remove this code with the above change.

[Byungseon(Sun) Shin]

Created attachment 240259 [details]
Patch

[Byungseon(Sun) Shin]

@Mark, thanks for the review.

I have applied all of requests from you except following:
=========================================================================
> 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.


The result value can be local time and UTC time according to InputTimeType. 
So, I think it would be better to keep use the value name of result.

[Mark Lam]

Comment on attachment 240259 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSDateMath.cpp:200
>      double result = (day * WTF::msPerDay) + ms;
>  
> -    if (!inputIsUTC)
> -        result -= localTimeOffset(vm, result).offset;
> +    if (inputTimeType == WTF::LocalTime)
> +        result -= localTimeOffset(vm, result, inputTimeType).offset; // result is in local time milliseconds.

Previously, you said that "The result value can be local time and UTC time according to InputTimeType.”  To clarify, what do you mean when in this added comment when you said that the "result is in local time milliseconds”?  Did you mean that the previous result was in local time, and that you’re converting it to UTC time now?  Or did you mean the previous result is in UTC time and you’re converting it to local time?

[Byungseon(Sun) Shin]

When the result passed in localTimeOffset(vm, result, inputTimeType), it is in the local time. After result value compensated by it's result by "result -= localTimeOffset(vm, result, inputTimeType).offset;", it is turned into UTC time.

[Mark Lam]

Comment on attachment 240259 [details]
Patch

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

In light of your comment, I'd like to request one more change (and threw in a few more nits while I was at it).  Can you make these changes please?  Thanks.

> Source/JavaScriptCore/runtime/DatePrototype.cpp:944
> +    const WTF::TimeType inputTimeType = WTF::LocalTime;
> +    return setNewValueFromTimeArgs(exec, 1, inputTimeType);

nit: This can be simplified to:
    return setNewValueFromTimeArgs(exec, 1, WTF::LocalTime);

I don't think there's that much to be gained by using 2 statements here.  Can you also apply this to the other functions below which is doing the same thing?

>> Source/JavaScriptCore/runtime/JSDateMath.cpp:200
>> +        result -= localTimeOffset(vm, result, inputTimeType).offset; // result is in local time milliseconds.
> 
> Previously, you said that "The result value can be local time and UTC time according to InputTimeType.”  To clarify, what do you mean when in this added comment when you said that the "result is in local time milliseconds”?  Did you mean that the previous result was in local time, and that you’re converting it to UTC time now?  Or did you mean the previous result is in UTC time and you’re converting it to local time?

(In reply to comment #57)
> When the result passed in localTimeOffset(vm, result, inputTimeType), it is in the local time. After result value compensated by it's result by "result -= localTimeOffset(vm, result, inputTimeType).offset;", it is turned into UTC time.

It looks like the result is always expected to be in UTC time.  In that case, let's rename "result" to "utcTimeResult" and remove the comment.  I think that that should make it clear that we had a local time input here and we want a UTC time result.

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

nit: "is local" ==> "is in local".

[Byungseon(Sun) Shin]

Created attachment 240307 [details]
Patch

[Byungseon(Sun) Shin]

@Mark, 
Since at the moment of calling "localTimeOffset", result value is localTime.
So I changed the value to localTimeResult and added localToUTCTimeOffset to get the offset value returned by localTimeOffset.
=============================================================================
 197 double localTimeResult = (day * WTF::msPerDay) + ms;
 198 double localToUTCTimeOffset = inputTimeType == LocalTime
 199 ? localTimeOffset(vm, localTimeResult, inputTimeType).offset : 0;

[Mark Lam]

r=me

[WebKit Commit Bot]

Comment on attachment 240307 [details]
Patch

Clearing flags on attachment: 240307

Committed r175078: <http://trac.webkit.org/changeset/175078>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Roger Fong]

This broke many JS tests on Windows:
https://build.webkit.org/builders/Apple%20Win%207%20Release%20%28Tests%29/builds/47985/steps/jscore-test/logs/actual.html%20%28source%29

Should they all be rebaselined?
Please advise.