Bug 61257

Created attachment 94362 [details] Patch

Comment on attachment 94362 [details] Patch Attachment 94362 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8723719 New failing tests: http/tests/appcache/different-origin-manifest.html ietestcenter/Javascript/10.6-12-1.html ietestcenter/Javascript/10.4.2-1-3.html ietestcenter/Javascript/10.4.2-2-c-1.html dom/html/level2/html/HTMLAnchorElement02.html ietestcenter/Javascript/10.6-6-3.html canvas/philip/tests/2d.clearRect.basic.html ietestcenter/Javascript/10.6-5-1.html canvas/philip/tests/2d.clearRect.globalcomposite.html canvas/philip/tests/2d.clearRect.path.html http/tests/appcache/cyrillic-uri.html dom/html/level2/html/HTMLAnchorElement05.html http/tests/appcache/crash-when-navigating-away-then-back.html dom/html/level2/html/HTMLAnchorElement14.html http/tests/appcache/deferred-events-delete-while-raising.html dom/html/level2/html/HTMLAnchorElement11.html canvas/philip/tests/2d.clearRect+fillRect.alpha0.5.html dom/html/level2/html/HTMLAnchorElement08.html http/tests/appcache/destroyed-frame.html canvas/philip/tests/2d.clearRect.zero.html

Created attachment 94366 [details] Archive of layout-test-results from ec2-cr-linux-02 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-02 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Created attachment 94367 [details] Patch - Fix builds

Created attachment 94376 [details] Patch - attempt to fix cr-linux layout tests

Comment on attachment 94376 [details] Patch - attempt to fix cr-linux layout tests Attachment 94376 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/8727406

Created attachment 94541 [details] Patch - attempt to fix mac build

Created attachment 94555 [details] Rebased patch

Comment on attachment 94555 [details] Rebased patch Attachment 94555 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/8731321

Created attachment 94585 [details] Patch - attempt to fix mac build again

(In reply to comment #0) > WTF::currentTime() actually returns current UTC time in second and WTF::currentTimeMS() returns current UTC time in millisecond. So it's more precise to rename WTF::currentTime() to WTF::currentUTCTime() and WTF::currentTimeMS() to WTF::currentUTCTimeMS(). I don’t think adding UTC to the names of these functions is helpful. This rename seems like a bad idea, which will not add clarity.

Are there any other reasons you have to do the renaming than the one you said in the ChangeLog? Are you going to reuse WTF::currentTime() for something else? or it causes some conflicts (it should not since it's namespaced)?

Maybe the attempt is to distinguish time from tick count? I’m not sure that adding UTC to the name is the clearest way to make that distinction.

This bug stems from 37743. In 37743, we are going to add a function monotonicTime() or something like tickTime() to serve as monotonically increasing time. The reason to add monotonicTime() is that in many cases (timer, animation) we should not use clock time, which can be adjusted by the use. As the comments of currentTime(), it's actually current UTC time. I add UTC to the names for 2 reasons: 1) make it more clear -- it's UTC time, not local time. 2) make it distinguish from monotonicTime() or tickTime().

(In reply to comment #14) > This bug stems from 37743. In 37743, we are going to add a function monotonicTime() or something like tickTime() to serve as monotonically increasing time. The reason to add monotonicTime() is that in many cases (timer, animation) we should not use clock time, which can be adjusted by the use. As the comments of currentTime(), it's actually current UTC time. I add UTC to the names for 2 reasons: 1) make it more clear -- it's UTC time, not local time. 2) make it distinguish from monotonicTime() or tickTime(). The name monotonicTime()/tickTime() already distinguishes itself from currentTime(). It doesn't seem to be a blocking issue to do the renaming to UTCTime().

This doesn't seem like a very useful thing to do. What property of currentTime are you hoping to make clearer? Do you have any evidence that people are confused about whether this value is in 'local time' or not? The comment here http://trac.webkit.org/browser/trunk/Source/JavaScriptCore/wtf/CurrentTime.h#L39 seems pretty clear, and it's consistent with unix timestamps which are always expressed as time since the "Unix Epoch" of 00:00:00 UTC Jan 1 1970 I think the risk of confusion is pretty low. Do you know of any code in WebKit that is or was incorrectly assuming this value was somehow in 'local time'?

Seems people don't like this rename. This bug was just for making bug 37743 move forward. People have agreed to add a new function for 37743 and the patch of it has landed. So close this bug.