Bug 61257 – Rename WTF::currentTime() and WTF::currentTimeMS()

• Component: JavaScriptCore
• Status: RESOLVED WONTFIX

[Leo Yang]

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

[Leo Yang]

Created attachment 94362 [details]
Patch

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Leo Yang]

Created attachment 94367 [details]
Patch - Fix builds

[Leo Yang]

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

[WebKit Commit Bot]

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

[Leo Yang]

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

[Leo Yang]

Created attachment 94555 [details]
Rebased patch

[WebKit Commit Bot]

Comment on attachment 94555 [details]
Rebased patch

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

[Leo Yang]

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

[Darin Adler]

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

[Chang Shu]

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

[Darin Adler]

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.

[Leo Yang]

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

[Chang Shu]

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

[James Robinson]

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

[Leo Yang]

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.