Bug 62159 - Implement montonicallyIncreasingClock() on Qt
Summary: Implement montonicallyIncreasingClock() on Qt
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Pierre Rossi
URL:
Keywords:
Depends on: 37743 75087
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-06 15:49 PDT by James Simonsen
Modified: 2012-01-09 13:28 PST (History)
14 users (show)

See Also:


Attachments
Patch (1.68 KB, patch)
2011-06-06 18:15 PDT, Jason Liu
no flags Details | Formatted Diff | Diff
patch (1.71 KB, patch)
2011-06-06 18:44 PDT, Jason Liu
no flags Details | Formatted Diff | Diff
patch (1.73 KB, patch)
2011-06-12 22:47 PDT, Jason Liu
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
Patch (1.81 KB, patch)
2011-06-12 23:59 PDT, Jason Liu
no flags Details | Formatted Diff | Diff
patch (1.72 KB, patch)
2011-06-13 19:31 PDT, Jason Liu
no flags Details | Formatted Diff | Diff
Patch (1.72 KB, patch)
2011-06-14 19:23 PDT, Jason Liu
hausmann: review-
Details | Formatted Diff | Diff
Patch (2.61 KB, patch)
2011-12-20 14:11 PST, Mark Dyer
no flags Details | Formatted Diff | Diff
Patch (1.59 KB, patch)
2011-12-21 14:36 PST, Pierre Rossi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Simonsen 2011-06-06 15:49:47 PDT
See https://bugs.webkit.org/show_bug.cgi?id=37743 for more details.
Comment 1 James Robinson 2011-06-06 16:02:08 PDT
You should clarify whether you mean GTK or QT here, since they both run on 'linux' and will likely have different implementations.  I'd recommend filing bugs on ports (GTK/QT/etc) rather than OSes
Comment 2 James Simonsen 2011-06-06 16:08:12 PDT
(In reply to comment #1)
> You should clarify whether you mean GTK or QT here, since they both run on 'linux' and will likely have different implementations.  I'd recommend filing bugs on ports (GTK/QT/etc) rather than OSes

The other patch on bug 33743 was #if OS(LINUX). I'm not sure which one that's for or if it's intend it to work for both. Maybe Jason can follow up with which one he intended and we can update/create new bugs as needed.
Comment 3 Jason Liu 2011-06-06 18:15:45 PDT
Created attachment 96168 [details]
Patch

patch
Comment 4 Jason Liu 2011-06-06 18:44:23 PDT
Created attachment 96172 [details]
patch

patch on Qt/linux
Comment 5 Collabora GTK+ EWS bot 2011-06-06 18:51:23 PDT
Comment on attachment 96168 [details]
Patch

Attachment 96168 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/8807004
Comment 6 James Simonsen 2011-06-06 18:53:51 PDT
Sounds like this is a QT only patch. I've updated the summary and I'll file a new bug for GTK.
Comment 7 Jason Liu 2011-06-12 22:47:06 PDT
Created attachment 96920 [details]
patch

patch
Comment 8 Jason Liu 2011-06-12 23:59:20 PDT
Created attachment 96928 [details]
Patch

patch
Comment 9 Gyuyoung Kim 2011-06-13 09:43:35 PDT
Comment on attachment 96928 [details]
Patch

Attachment 96928 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/8835245
Comment 10 Jason Liu 2011-06-13 19:31:06 PDT
Created attachment 97056 [details]
patch

patch
Comment 11 Andreas Kling 2011-06-14 04:16:50 PDT
@Jason: Please mark only the relevant patch(es) for review.
Comment 12 Jason Liu 2011-06-14 19:23:59 PDT
Created attachment 97221 [details]
Patch

Patch
Comment 13 Antonio Gomes 2011-06-14 20:51:01 PDT
Comment on attachment 97221 [details]
Patch

Is this Qt-specific really?
Comment 14 Jason Liu 2011-06-14 21:15:22 PDT
(In reply to comment #13)
> (From update of attachment 97221 [details])
> Is this Qt-specific really?

Yes.Would you pls help me to review it ?
Thanks a lot!
Comment 15 Igor Trindade Oliveira 2011-06-15 06:50:26 PDT
Great patch.

I have just few comments.
clock_gettime works on all Unix systems since Linux until AIX, so it is not Qt specific.
About clock_gettime with CLOCK_MONOTONIC clock it is not completely monotonic (Linux case), because if the system is using NTP(Network Time Protocol) and the time changes the clock_gettime returns a wrong time. The solution for Linux using kernel 2.6.28 is the option CLOCK_MONOTONIC_RAW.
But for now i think CLOCK_MONOTONIC is enough.


(In reply to comment #12)
> Created an attachment (id=97221) [details]
> Patch
> 
> Patch
Comment 16 Pierre Rossi 2011-07-04 07:57:27 PDT
If you want to make this specific to the Qt port, I guess you could simply use QElapsedTimer directly.
See:
http://doc.qt.nokia.com/4.8-snapshot/qelapsedtimer.html
Comment 17 Alexis Menard (darktears) 2011-08-26 07:54:12 PDT
(In reply to comment #16)
> If you want to make this specific to the Qt port, I guess you could simply use QElapsedTimer directly.
> See:
> http://doc.qt.nokia.com/4.8-snapshot/qelapsedtimer.html

Yep no need to use a low level API no?
Comment 18 Simon Hausmann 2011-11-01 14:45:41 PDT
Comment on attachment 97221 [details]
Patch

I agree with the two gentlemen :). Could you implement this using QElapsedTimer?

Plus there's something wrong in the function declaration. From what I can tell from the CurrentTime.cpp the patch should look something like this:

+#elif PLATFORM(QT)
+
+double monotonicallyIncreasingClock()
+{
...
+}
+
#else
Comment 19 WebKit Review Bot 2011-11-01 22:39:06 PDT
Attachment 96920 [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/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 20 Early Warning System Bot 2011-11-01 23:54:39 PDT
Comment on attachment 96920 [details]
patch

Attachment 96920 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10256179
Comment 21 Mark Dyer 2011-12-20 14:11:17 PST
Created attachment 120081 [details]
Patch
Comment 22 WebKit Review Bot 2011-12-20 14:17:18 PST
Attachment 120081 [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:63:  "sys/time.h" already included at Source/JavaScriptCore/wtf/CurrentTime.cpp:38  [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 23 Mark Dyer 2011-12-21 12:22:17 PST
I've implemented monotonicallyIncreasingTime() using QElapsedTimer, but it is failing to pass check-webkit-style because when that script checks for multiple includes of the same file, it does not take into account platform specific includes (ie includes inside #if PLATFORM(MAC)...#elif PLATFORM(QT)...  being required by more than one platform)

Using #if HAVE(SYS_TIME_H) like is done in DateMath.cpp, I can get rid of the ""sys/time.h" already included", but there will still be an "Alphabetical sorting problem". I can't see a way to get rid of that without making that part of the file completely unreadable.
Comment 24 Pierre Rossi 2011-12-21 13:11:30 PST
(In reply to comment #23)
> I've implemented monotonicallyIncreasingTime() using QElapsedTimer, but it is failing to pass check-webkit-style because when that script checks for multiple includes of the same file, it does not take into account platform specific includes (ie includes inside #if PLATFORM(MAC)...#elif PLATFORM(QT)...  being required by more than one platform)
> 
> Using #if HAVE(SYS_TIME_H) like is done in DateMath.cpp, I can get rid of the ""sys/time.h" already included", but there will still be an "Alphabetical sorting problem". I can't see a way to get rid of that without making that part of the file completely unreadable.

I actually fail to see why you would want to manually include "sys/time.h" at all. This would most likely be a problem on platforms like Windows for instance. The platform-specific implementations of QElapsedTimer will take care of using what's right on the various platform Qt runs on for you.
Comment 25 Pierre Rossi 2011-12-21 13:11:52 PST
(In reply to comment #23)
> I've implemented monotonicallyIncreasingTime() using QElapsedTimer, but it is failing to pass check-webkit-style because when that script checks for multiple includes of the same file, it does not take into account platform specific includes (ie includes inside #if PLATFORM(MAC)...#elif PLATFORM(QT)...  being required by more than one platform)
> 
> Using #if HAVE(SYS_TIME_H) like is done in DateMath.cpp, I can get rid of the ""sys/time.h" already included", but there will still be an "Alphabetical sorting problem". I can't see a way to get rid of that without making that part of the file completely unreadable.

I actually fail to see why you would want to manually include "sys/time.h" at all. This would most likely be a problem on platforms like Windows for instance. The platform-specific implementations of QElapsedTimer will take care of using what's right on the various platform Qt runs on for you.
Comment 26 Pierre Rossi 2011-12-21 14:36:02 PST
Created attachment 120219 [details]
Patch
Comment 27 WebKit Review Bot 2011-12-21 14:38:05 PST
Attachment 120219 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
From git://git.webkit.org/WebKit
   4428c79..c4e9134  master     -> origin/master
Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ...
Currently at 103445 = 4428c79e3922c44a4537acf0b21a4e4540a3eecb
r103450 = 66364db0ef636bce41f0699b135aa8278cf5cdf8
r103452 = c4e9134aef89c0b495d95a6923bb9491218b33e4
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Applying: Inform the scrolling coordinator when scrollbar layers come and go
Using index info to reconstruct a base tree...
<stdin>:474806: trailing whitespace.
        [Chromium] DatabaseTrackerChromium: iterating DatabaseSet races with Database disposal on worker thread 
<stdin>:474827: trailing whitespace.
        Nothing to test, just removing redundant code. Correct behavior tested by 
<stdin>:475346: trailing whitespace.
    
warning: 3 lines add whitespace errors.
Falling back to patching base and 3-way merge...
warning: too many files (created: 167249 deleted: 3), skipping inexact rename detection
Auto-merging LayoutTests/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Auto-merging LayoutTests/platform/chromium/test_expectations.txt
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Auto-merging Source/WebKit2/ChangeLog
CONFLICT (content): Merge conflict in Source/WebKit2/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 Inform the scrolling coordinator when scrollbar layers come and go

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 158.


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 28 Pierre Rossi 2011-12-21 15:40:34 PST
Committed r103467: <http://trac.webkit.org/changeset/103467>
Comment 29 Csaba Osztrogonác 2011-12-22 02:44:28 PST
(In reply to comment #28)
> Committed r103467: <http://trac.webkit.org/changeset/103467>

Reopen, because it broke fast/images/animated-gif-restored-from-bfcache.html on Qt:

--- /ramdisk/qt-linux-64-release/build/layout-test-results/fast/images/animated-gif-restored-from-bfcache-expected.txt 
+++ /ramdisk/qt-linux-64-release/build/layout-test-results/fast/images/animated-gif-restored-from-bfcache-actual.txt 
@@ -1,5 +1,5 @@
 Test that animated GIFs resume animating after restoring a page from the back forward cache. To test manually, click here to visit about:blank, then press the browser's back button. On success, you should see a square that continuously animates through various shades of green followed by the word 'PASS'.
 
 
-PASS
+FAIL
Comment 30 Csaba Osztrogonác 2011-12-22 02:45:12 PST
Comment on attachment 120219 [details]
Patch

remove r+ from landed patch
Comment 31 Csaba Osztrogonác 2011-12-22 07:25:24 PST
I filed a new bug report to fix the regression: https://bugs.webkit.org/show_bug.cgi?id=75087
Comment 32 Pierre Rossi 2011-12-23 05:36:34 PST
Forgot to close that one as well.
Comment 33 Eric Seidel (no email) 2012-01-09 13:28:19 PST
Comment on attachment 120081 [details]
Patch

Cleared review? from attachment 120081 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).