Bug 29148 - replace platform time code with WTF::currentTime()
Summary: replace platform time code with WTF::currentTime()
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
Depends on:
Reported: 2009-09-10 13:54 PDT by Yong Li
Modified: 2009-09-24 15:01 PDT (History)
8 users (show)

See Also:

the patch (3.75 KB, patch)
2009-09-10 15:13 PDT, Yong Li
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yong Li 2009-09-10 13:54:02 PDT
We have WTF::currentTime() now. We should use it to replace those duplicate platform-dependent code.

1. In TimeoutChecker.cpp: getCPUTime currently returns system time except for PLATFORM(DARWIN) and PLATFORM(WIN_OS) (they can get actual thread time)

2. jsc.cpp: a lot of time related code can be simplified with currentTime()

Patch is coming
Comment 1 Yong Li 2009-09-10 15:13:05 PDT
Created attachment 39385 [details]
the patch

the patch
Comment 2 Ariya Hidayat 2009-09-11 06:21:08 PDT
Comment on attachment 39385 [details]
the patch

CurrentTime.h is missing in the patch.
Comment 3 Yong Li 2009-09-11 07:04:26 PDT
(In reply to comment #2)
> (From update of attachment 39385 [details])
> CurrentTime.h is missing in the patch.

What do you mean? CurrentTime.h already exists in JavaScriptCore/wtf.
Comment 4 Joe Mason 2009-09-11 11:41:50 PDT
I believe that in the current codebase, currentTime() is only ever used to check relative times, even though the descriptions specifies that it should return seconds since the Unix epoch.  That could let ports take shortcuts by implementing it to return seconds since some other epoch, or since the system boot, or whatever's easiest for them.  

I don't know if any ports actually DO take such shortcuts, but this should be double-checked if this patch is landed.

I was about to suggest renaming currentTime and changing the description to make it clear the way it was actually used, but I like this approach better.  No need to duplicate code.
Comment 5 Adam Barth 2009-09-23 22:24:30 PDT
Comment on attachment 39385 [details]
the patch

I'm not an expert here, but this seems reasonable and this patch has been setting in the review queue for a while with little discussion.
Comment 6 Eric Seidel (no email) 2009-09-24 13:44:46 PDT
Looks reasonable to me too.  CCing Darin and Maciej who I believe worked on this code long ago, so that they at least see this go by.
Comment 7 WebKit Commit Bot 2009-09-24 14:31:40 PDT
Comment on attachment 39385 [details]
the patch

Rejecting patch 39385 from commit-queue.

This patch will require manual commit. Failed to run "['git', 'svn', 'dcommit']"  exit_code: 1  cwd: None
Comment 8 Eric Seidel (no email) 2009-09-24 14:33:34 PDT
Comment on attachment 39385 [details]
the patch

Sigh.  Bug 28316, sorry.
Comment 9 WebKit Commit Bot 2009-09-24 15:00:54 PDT
Comment on attachment 39385 [details]
the patch

Clearing flags on attachment: 39385

Committed r48736: <http://trac.webkit.org/changeset/48736>
Comment 10 WebKit Commit Bot 2009-09-24 15:01:00 PDT
All reviewed patches have been landed.  Closing bug.