WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(4.08 KB, patch)
2012-04-08 13:02 PDT
,
Patrick R. Gansterer
no flags
Details
Formatted Diff
Diff
Patch
(4.15 KB, patch)
2012-04-09 10:31 PDT
,
Patrick R. Gansterer
aroben
: review-
Details
Formatted Diff
Diff
Patch
(4.46 KB, patch)
2012-05-30 13:49 PDT
,
Patrick R. Gansterer
bfulgham
: review+
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Patrick R. Gansterer
Comment 1
2012-04-04 06:51:22 PDT
Created
attachment 135583
[details]
Patch
Patrick R. Gansterer
Comment 2
2012-04-08 13:02:24 PDT
Created
attachment 136155
[details]
Patch
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
Created
attachment 136262
[details]
Patch
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
Created
attachment 144916
[details]
Patch
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
Committed
r120552
: <
http://trac.webkit.org/changeset/120552
>
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.
Top of Page
Format For Printing
XML
Clone This Bug