Bug 62159

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

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

Created attachment 96168 [details] Patch patch

Created attachment 96172 [details] patch patch on Qt/linux

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

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

Created attachment 96920 [details] patch patch

Created attachment 96928 [details] Patch patch

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

Created attachment 97056 [details] patch patch

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

Created attachment 97221 [details] Patch Patch

Comment on attachment 97221 [details] Patch Is this Qt-specific really?

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

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

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

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

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 on attachment 96920 [details] patch Attachment 96920 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/10256179

Created attachment 120081 [details] Patch

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.

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.

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

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

Created attachment 120219 [details] Patch

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.

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

(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 on attachment 120219 [details] Patch remove r+ from landed patch

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

Forgot to close that one as well.

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