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)
*** Bug 131437 has been marked as a duplicate of this bug. ***
Created attachment 229133 [details] Patch This patch will fix the offset problems on the day of DST change.
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
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
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
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
Created attachment 229431 [details] Patch
@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?
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.
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.
(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?
Created attachment 229609 [details] Patch
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')
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
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
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
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
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
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
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.
Created attachment 229724 [details] Patch
Created attachment 229741 [details] Patch
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.
Created attachment 238783 [details] Patch
Created attachment 238785 [details] Patch
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
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
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
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
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
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
Created attachment 238894 [details] Patch
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.
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.
We can get the result 2 of "new Date(1982, 2, 14, 2).getHours()" on Firefox when using PST/PDT Timezone.
<rdar://problem/18507871>
Created attachment 239077 [details] Patch
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
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
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
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
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
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
Created attachment 239091 [details] Patch
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).
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?
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.
Created attachment 239313 [details] Patch
Created attachment 239314 [details] Patch
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*!
Created attachment 239986 [details] Patch
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”?
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.
Created attachment 240259 [details] Patch
@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.
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?
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.
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".
Created attachment 240307 [details] Patch
@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;
r=me
Comment on attachment 240307 [details] Patch Clearing flags on attachment: 240307 Committed r175078: <http://trac.webkit.org/changeset/175078>
All reviewed patches have been landed. Closing bug.
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.