Bug 62159 – Implement montonicallyIncreasingClock() on Qt

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[James Simonsen]

See https://bugs.webkit.org/show_bug.cgi?id=37743 for more details.

[James Robinson]

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

[James Simonsen]

(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.

[Jason Liu]

Created attachment 96168 [details]
Patch

patch

[Jason Liu]

Created attachment 96172 [details]
patch

patch on Qt/linux

[Collabora GTK+ EWS bot]

Comment on attachment 96168 [details]
Patch

Attachment 96168 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/8807004

[James Simonsen]

Sounds like this is a QT only patch. I've updated the summary and I'll file a new bug for GTK.

[Jason Liu]

Created attachment 96920 [details]
patch

patch

[Jason Liu]

Created attachment 96928 [details]
Patch

patch

[Gyuyoung Kim]

Comment on attachment 96928 [details]
Patch

Attachment 96928 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/8835245

[Jason Liu]

Created attachment 97056 [details]
patch

patch

[Andreas Kling]

@Jason: Please mark only the relevant patch(es) for review.

[Jason Liu]

Created attachment 97221 [details]
Patch

Patch

[Antonio Gomes]

Comment on attachment 97221 [details]
Patch

Is this Qt-specific really?

[Jason Liu]

(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!

[Igor Trindade Oliveira]

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

[Pierre Rossi]

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

[Alexis Menard (darktears)]

(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?

[Simon Hausmann]

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

[WebKit Review Bot]

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.

[Early Warning System Bot]

Comment on attachment 96920 [details]
patch

Attachment 96920 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10256179

[Mark Dyer]

Created attachment 120081 [details]
Patch

[WebKit Review Bot]

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.

[Mark Dyer]

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.

[Pierre Rossi]

(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.

[Pierre Rossi]

(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.

[Pierre Rossi]

Created attachment 120219 [details]
Patch

[WebKit Review Bot]

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.

[Pierre Rossi]

Committed r103467: <http://trac.webkit.org/changeset/103467>

[Csaba Osztrogonác]

(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

[Csaba Osztrogonác]

Comment on attachment 120219 [details]
Patch

remove r+ from landed patch

[Csaba Osztrogonác]

I filed a new bug report to fix the regression: https://bugs.webkit.org/show_bug.cgi?id=75087

[Pierre Rossi]

Forgot to close that one as well.

[Eric Seidel (no email)]

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).