Bug 61257 - Rename WTF::currentTime() and WTF::currentTimeMS()
Summary: Rename WTF::currentTime() and WTF::currentTimeMS()
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Leo Yang
URL:
Keywords:
Depends on:
Blocks: 37743
  Show dependency treegraph
 
Reported: 2011-05-22 20:59 PDT by Leo Yang
Modified: 2011-06-06 21:18 PDT (History)
7 users (show)

See Also:


Attachments
Patch (144.78 KB, patch)
2011-05-22 21:25 PDT, Leo Yang
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (1.03 MB, application/zip)
2011-05-22 22:16 PDT, WebKit Review Bot
no flags Details
Patch - Fix builds (159.36 KB, patch)
2011-05-22 22:29 PDT, Leo Yang
no flags Details | Formatted Diff | Diff
Patch - attempt to fix cr-linux layout tests (155.60 KB, patch)
2011-05-22 23:52 PDT, Leo Yang
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Patch - attempt to fix mac build (156.19 KB, patch)
2011-05-23 18:55 PDT, Leo Yang
no flags Details | Formatted Diff | Diff
Rebased patch (156.22 KB, patch)
2011-05-23 20:55 PDT, Leo Yang
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Patch - attempt to fix mac build again (157.08 KB, patch)
2011-05-24 02:15 PDT, Leo Yang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Leo Yang 2011-05-22 20:59:40 PDT
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().
Comment 1 Leo Yang 2011-05-22 21:25:12 PDT
Created attachment 94362 [details]
Patch
Comment 2 WebKit Review Bot 2011-05-22 22:16:10 PDT
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
Comment 3 WebKit Review Bot 2011-05-22 22:16:14 PDT
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
Comment 4 Leo Yang 2011-05-22 22:29:11 PDT
Created attachment 94367 [details]
Patch - Fix builds
Comment 5 Leo Yang 2011-05-22 23:52:37 PDT
Created attachment 94376 [details]
Patch - attempt to fix cr-linux layout tests
Comment 6 WebKit Commit Bot 2011-05-23 15:51:44 PDT
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
Comment 7 Leo Yang 2011-05-23 18:55:21 PDT
Created attachment 94541 [details]
Patch - attempt to fix mac build
Comment 8 Leo Yang 2011-05-23 20:55:53 PDT
Created attachment 94555 [details]
Rebased patch
Comment 9 WebKit Commit Bot 2011-05-24 01:24:09 PDT
Comment on attachment 94555 [details]
Rebased patch

Attachment 94555 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/8731321
Comment 10 Leo Yang 2011-05-24 02:15:29 PDT
Created attachment 94585 [details]
Patch - attempt to fix mac build again
Comment 11 Darin Adler 2011-05-24 09:37:41 PDT
(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.
Comment 12 Chang Shu 2011-05-25 11:00:16 PDT
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)?
Comment 13 Darin Adler 2011-05-25 13:39:47 PDT
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.
Comment 14 Leo Yang 2011-05-25 18:56:03 PDT
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().
Comment 15 Chang Shu 2011-05-26 07:15:33 PDT
(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().
Comment 16 James Robinson 2011-06-06 16:01:20 PDT
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'?
Comment 17 Leo Yang 2011-06-06 21:17:40 PDT
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.