Summary: | Implement montonicallyIncreasingClock() on Qt | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | James Simonsen <simonjam> | ||||||||||||||||||
Component: | JavaScriptCore | Assignee: | Pierre Rossi <pierre.rossi> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Normal | CC: | ademar, gustavo.noronha, gustavo, igor.oliveira, jamesr, jasonliuwebkit, kling, mark, menard, ossy, pierre.rossi, tonikitoo, webkit.review.bot, xan.lopez | ||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||
Bug Depends on: | 37743, 75087 | ||||||||||||||||||||
Bug Blocks: | |||||||||||||||||||||
Attachments: |
|
Description
James Simonsen
2011-06-06 15:49:47 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 (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). |