RESOLVED FIXED 83156
[WIN] Simplify implementation of currentTime()
https://bugs.webkit.org/show_bug.cgi?id=83156
Summary [WIN] Simplify implementation of currentTime()
Patrick R. Gansterer
Reported 2012-04-04 06:28:30 PDT
[WIN] Simplify implementation of currentTime()
Attachments
Patch (4.04 KB, patch)
2012-04-04 06:51 PDT, Patrick R. Gansterer
no flags
Patch (4.08 KB, patch)
2012-04-08 13:02 PDT, Patrick R. Gansterer
no flags
Patch (4.15 KB, patch)
2012-04-09 10:31 PDT, Patrick R. Gansterer
aroben: review-
Patch (4.46 KB, patch)
2012-05-30 13:49 PDT, Patrick R. Gansterer
bfulgham: review+
Patrick R. Gansterer
Comment 1 2012-04-04 06:51:22 PDT
Patrick R. Gansterer
Comment 2 2012-04-08 13:02:24 PDT
Adam Roben (:aroben)
Comment 3 2012-04-09 09:36:48 PDT
Comment on attachment 136155 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=136155&action=review > Source/WTF/wtf/CurrentTime.cpp:81 > + union { > + FILETIME fileTime; > + ULONGLONG quadPart; > + } u; Is a union safe? MSDN says, "Do not cast a pointer to a FILETIME structure to either a ULARGE_INTEGER* or __int64* value because it can cause alignment faults on 64-bit Windows."
Patrick R. Gansterer
Comment 4 2012-04-09 10:31:01 PDT
Adam Roben (:aroben)
Comment 5 2012-04-12 12:24:28 PDT
Comment on attachment 136262 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=136262&action=review > Source/WTF/wtf/CurrentTime.cpp:86 > + ULONGLONG dateTime = (static_cast<ULONGLONG>(fileTime.dwHighDateTime) << 32) | fileTime.dwLowDateTime; I think it would be nicer to copy fileTime into a ULARGE_INTEGER (without using a union) and then use its QuadPart member. That's what MSDN recommends. Sorry for not being more explicit before.
Patrick R. Gansterer
Comment 6 2012-05-30 13:49:05 PDT
Brent Fulgham
Comment 7 2012-06-11 17:47:01 PDT
Comment on attachment 144916 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144916&action=review I think this change looks great. Please consider retaining the comments I mention in the detailed notes when you commit the change. > Source/WTF/wtf/CurrentTime.cpp:-222 > - I don't like losing the following comments, which I found very illuminating about this routine. Could you please copy these into your revised implementation so that non-Windows people are aware that Windows ticks are in 100s of nanoseconds, etc.?
Patrick R. Gansterer
Comment 8 2012-06-17 14:06:33 PDT
Darin Adler
Comment 9 2012-06-20 18:58:12 PDT
This rewrite of the function was wrong. See bug 89433 for the problem and fix. But also, what was so wrong with the old version?
Brent Fulgham
Comment 10 2012-06-20 20:55:52 PDT
Although there was a mistake in the units returned (milliseconds rather than seconds), I think the intent of the change was good because it reduced the code differences between the standard Windows and WinCE implementations. There doesn't seem to be any meaningful difference in accurracy or precision between _ftime and GetSystemTimeAsFileTime, but since the routine is peppered with Windows-specific magic numbers related to "ticks per epoch" and so forth, it probably makes sense to stick with the Windows API calls so that the whole things hangs together.
Note You need to log in before you can comment on or make changes to this bug.