Bug 62175

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 Flags
Patch
mrobinson: review-
A HTML file used to unit test the change
none
Modified patch after review comments
none
Patch
none
Patch
webkit-ews: commit-queue-
Implementation of monotonically increasing clock on GTK
none
Patch none

Description James Simonsen 2011-06-06 18:59:12 PDT
In order to support some of the new HTML5 specs, and to fix other bugs, we need to implement a monotonically increasing clock on all platforms.

See https://bugs.webkit.org/show_bug.cgi?id=37743 for more details.
Comment 1 Gavin Barraclough 2011-06-23 14:01:18 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.
Comment 2 Devdatta Deshpande 2011-07-21 16:08:47 PDT
Created attachment 101659 [details]
Patch
Comment 3 Devdatta Deshpande 2011-07-21 16:12:47 PDT
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 4 Martin Robinson 2011-07-28 10:17:11 PDT
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.
Comment 5 Devdatta Deshpande 2011-07-28 12:37:10 PDT
Created attachment 102287 [details]
Modified patch after review comments

Incorporated the review comments by Martin.
Comment 6 Early Warning System Bot 2011-07-28 14:43:05 PDT
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
Comment 7 Devdatta Deshpande 2011-07-28 15:06:52 PDT
Created attachment 102304 [details]
Patch
Comment 8 Gyuyoung Kim 2011-07-28 15:09:38 PDT
Comment on attachment 102304 [details]
Patch

Attachment 102304 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/9262333
Comment 9 Early Warning System Bot 2011-07-28 15:12:24 PDT
Comment on attachment 102304 [details]
Patch

Attachment 102304 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9268213
Comment 10 Devdatta Deshpande 2011-07-28 15:23:40 PDT
Created attachment 102307 [details]
Patch
Comment 11 WebKit Review Bot 2011-07-28 16:05:30 PDT
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 12 Early Warning System Bot 2011-07-28 18:15:20 PDT
Comment on attachment 102307 [details]
Patch

Attachment 102307 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9264342
Comment 13 Devdatta Deshpande 2011-08-01 15:35:19 PDT
Created attachment 102574 [details]
Implementation of monotonically increasing clock on GTK
Comment 14 Devdatta Deshpande 2011-08-01 15:38:20 PDT
(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 15 Martin Robinson 2011-08-08 01:15:11 PDT
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);
Comment 16 Devdatta Deshpande 2011-08-10 05:28:18 PDT
Created attachment 103468 [details]
Patch
Comment 17 WebKit Review Bot 2011-08-11 09:49:12 PDT
Comment on attachment 103468 [details]
Patch

Clearing flags on attachment: 103468

Committed r92859: <http://trac.webkit.org/changeset/92859>
Comment 18 WebKit Review Bot 2011-08-11 09:49:18 PDT
All reviewed patches have been landed.  Closing bug.