Bug 83156

Summary: [WIN] Simplify implementation of currentTime()
Product: WebKit Reporter: Patrick R. Gansterer <paroga>
Component: PlatformAssignee: Patrick R. Gansterer <paroga>
Status: RESOLVED FIXED    
Severity: Normal CC: aestes, aroben, bfulgham, darin, jberlin, jhoneycutt, sfalken
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Windows 7   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
aroben: review-
Patch bfulgham: review+

Description Patrick R. Gansterer 2012-04-04 06:28:30 PDT
[WIN] Simplify implementation of currentTime()
Comment 1 Patrick R. Gansterer 2012-04-04 06:51:22 PDT
Created attachment 135583 [details]
Patch
Comment 2 Patrick R. Gansterer 2012-04-08 13:02:24 PDT
Created attachment 136155 [details]
Patch
Comment 3 Adam Roben (:aroben) 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."
Comment 4 Patrick R. Gansterer 2012-04-09 10:31:01 PDT
Created attachment 136262 [details]
Patch
Comment 5 Adam Roben (:aroben) 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.
Comment 6 Patrick R. Gansterer 2012-05-30 13:49:05 PDT
Created attachment 144916 [details]
Patch
Comment 7 Brent Fulgham 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.?
Comment 8 Patrick R. Gansterer 2012-06-17 14:06:33 PDT
Committed r120552: <http://trac.webkit.org/changeset/120552>
Comment 9 Darin Adler 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?
Comment 10 Brent Fulgham 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.