Summary: | [GTK] Implement monotonicallyIncreasingClock() | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | James Simonsen <simonjam> | ||||||||||||||||
Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | barraclough, mrobinson, webkit.devdatta, webkit.review.bot | ||||||||||||||||
Priority: | P2 | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
Bug Depends on: | 37743 | ||||||||||||||||||
Bug Blocks: | |||||||||||||||||||
Attachments: |
|
Description
James Simonsen
2011-06-06 18:59:12 PDT
Doesn't look like this bug wants to be in the JSC component. Maybe 'Platform' would be better? - or possibly WTF, if that is where this will be implemented?, since I'm unsure I'll conservatively file to misc. for now. Created attachment 101659 [details]
Patch
Created attachment 101660 [details]
A HTML file used to unit test the change
Without the patch, the timer stops working if the system time is changed to a time in past. Using monotonically increasing timer instead of system time helps in fixing this issue.
Comment on attachment 101659 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=101659&action=review This looks sane to me. > Source/JavaScriptCore/ChangeLog:9 > + Reviewed by NOBODY (OOPS!). > + > + * wtf/CurrentTime.cpp: > + (WTF::monotonicallyIncreasingTime): Please fill in this ChangeLog, explaining how the existing implementation of monotonicallyIncreasingTime can fail. > Source/JavaScriptCore/wtf/CurrentTime.cpp:313 > +#elif PLATFORM(GTK) I think it would be better to use #if ENABLE(GLIB_SUPPORT) here. Created attachment 102287 [details]
Modified patch after review comments
Incorporated the review comments by Martin.
Comment on attachment 102287 [details] Modified patch after review comments Attachment 102287 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/9221040 Created attachment 102304 [details]
Patch
Comment on attachment 102304 [details] Patch Attachment 102304 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/9262333 Comment on attachment 102304 [details] Patch Attachment 102304 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/9268213 Created attachment 102307 [details]
Patch
Attachment 102307 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
Source/JavaScriptCore/wtf/CurrentTime.cpp:68: "glib.h" already included at Source/JavaScriptCore/wtf/CurrentTime.cpp:60 [build/include] [4]
Total errors found: 1 in 2 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 102307 [details] Patch Attachment 102307 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/9264342 Created attachment 102574 [details]
Implementation of monotonically increasing clock on GTK
(In reply to comment #4) > (From update of attachment 101659 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=101659&action=review > > This looks sane to me. > > > Source/JavaScriptCore/ChangeLog:9 > > + Reviewed by NOBODY (OOPS!). > > + > > + * wtf/CurrentTime.cpp: > > + (WTF::monotonicallyIncreasingTime): > > Please fill in this ChangeLog, explaining how the existing implementation of monotonicallyIncreasingTime can fail. Please find the explanation in the attached patch. > > > Source/JavaScriptCore/wtf/CurrentTime.cpp:313 > > +#elif PLATFORM(GTK) > > I think it would be better to use #if ENABLE(GLIB_SUPPORT) here. g_get_monotonic_time is supported in glib since version 2.28. I am getting compilation errors on QT when I use this API. So, implementing this under PLATFORM(GTK) instead of ENABLE(GLIB_SUPPORT). Comment on attachment 102574 [details] Implementation of monotonically increasing clock on GTK View in context: https://bugs.webkit.org/attachment.cgi?id=102574&action=review Loooks good to me. I'll land this one. > Source/JavaScriptCore/wtf/CurrentTime.cpp:334 > + gint64 nowMicroSecs = g_get_monotonic_time(); > + return static_cast<double>(nowMicroSecs / 1000000.0); This should probably just be return static_cast<double>(g_get_monotonic_time() / 1000000.0); Created attachment 103468 [details]
Patch
Comment on attachment 103468 [details] Patch Clearing flags on attachment: 103468 Committed r92859: <http://trac.webkit.org/changeset/92859> All reviewed patches have been landed. Closing bug. |