Bug 28126 - [Haiku] Adding SharedTimer to WebCore.
Summary: [Haiku] Adding SharedTimer to WebCore.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Other
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-08-09 05:30 PDT by Maxime Simon
Modified: 2009-08-18 20:33 PDT (History)
2 users (show)

See Also:


Attachments
Patch to add the SharedTimer files for Haiku. (8.00 KB, patch)
2009-08-09 05:34 PDT, Maxime Simon
eric: review-
Details | Formatted Diff | Diff
Patch to add the SharedTimer file for Haiku. (4.95 KB, patch)
2009-08-12 08:44 PDT, Maxime Simon
no flags Details | Formatted Diff | Diff
Adding the SharedTimer file for WebCore. (4.50 KB, patch)
2009-08-13 01:12 PDT, Maxime Simon
no flags Details | Formatted Diff | Diff
Adding the SharedTimer file for WebCore. (4.54 KB, patch)
2009-08-15 00:46 PDT, Maxime Simon
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Maxime Simon 2009-08-09 05:30:35 PDT
Patch to add the SharedTimerHaiku.* files to WebCore.
Comment 1 Maxime Simon 2009-08-09 05:34:14 PDT
Created attachment 34412 [details]
Patch to add the SharedTimer files for Haiku.
Comment 2 Eric Seidel (no email) 2009-08-09 08:06:49 PDT
Comment on attachment 34412 [details]
Patch to add the SharedTimer files for Haiku.

The compiler won't read your comments. :)
 // Static instance
 88 SharedTimerHaiku* getSharedTimerHaiku()
you want that to say:
static SharedTimerHaiku* sharedTimerHaiku()
(we don't tend to use "get" in function names in WebCore.)

Oh, maybe you don't want it static...  Why is this needed outside of this function?
 52     SharedTimerHaiku* getSharedTimerHaiku();

Also, why doesn't SharedTimerHaiku implement SharedTimer?

Why does SharedTimerHaiku need to *be* a BMessageFilter, can't it just have a BMessageFilter member if necessary?

r- because I'm not sure why you needed to diverge from other SharedTimer implementations here.
Comment 3 Maxime Simon 2009-08-09 11:46:06 PDT
(In reply to comment #2)
> (From update of attachment 34412 [details])
> The compiler won't read your comments. :)
>  // Static instance
>  88 SharedTimerHaiku* getSharedTimerHaiku()
> you want that to say:
> static SharedTimerHaiku* sharedTimerHaiku()
> (we don't tend to use "get" in function names in WebCore.)
> 
> Oh, maybe you don't want it static...  Why is this needed outside of this
> function?
>  52     SharedTimerHaiku* getSharedTimerHaiku();

I think that the purpose of this function is to return a static instance of SharedTimer.

> Also, why doesn't SharedTimerHaiku implement SharedTimer?
> 
> Why does SharedTimerHaiku need to *be* a BMessageFilter, can't it just have a
> BMessageFilter member if necessary?

I didn't implement this class, but it seems that it was designed to fit with the Haiku messaging system. That's why SharedTimerHaiku needs to be a derivated class of BMessageFilter (in Haiku there is a lot of hook functions to implement when derivating a class).

After this I don't say that we should keep this as it. I will see how other ports handle SharedTimer and see if we can make something more smooth and light. :)

Regards,
Maxime
Comment 4 Ryan Leavengood 2009-08-09 13:07:16 PDT
I had implemented SharedTimerHaiku differently in my original code and this version was created by Andrea Anzani. I'm not sure if my original would fit better in WebKit but this version may be better in the sense of fitting in with Haiku.

It might be worth looking back in the history on my subversion copy of my old port to see what my original was like. I'm not sure if you have that checked out Maxime, but it is here:

http://ryanleavengood.com/svn/repo/WebKit/trunk/

Unfortunately I don't see anyway to go back in the history in this dead simple SVN HTTP interface. So you'll need a copy checked out. If this is too difficult I could probably get a copy of my original SharedTimerHaiku pretty fast.

It might also be worthwhile to look at the commit history to see why Andrea replaced my version with this one. I think it was to sort out some threading issues.

Finally BMessageFilter can be used both directly by passing in a filter_hook function and by subclassing. I think in this case using subclassing is better to easily access the members for the timer function and shouldRun boolean.

http://www.haiku-os.org/legacy-docs/bebook/BMessageFilter.html
Comment 5 Maxime Simon 2009-08-12 08:44:25 PDT
Created attachment 34663 [details]
Patch to add the SharedTimer file for Haiku.

I still used a BMessageFilter, but as static variable. We now use the generic SharedTimer.
Comment 6 Eric Seidel (no email) 2009-08-12 10:33:06 PDT
Comment on attachment 34663 [details]
Patch to add the SharedTimer file for Haiku.

+    bigtime_t interval = fireTime - currentTime();
+
+    if (interval < 0)
+        interval = 0;
+    else
+        interval *= 1000;

Should be something like:

bigtime_t millisecondInterval = fireTime - currentTime();
bigtime_t secondInterval = std::max(0, millisecondInterval * 1000);

Important to make the variable names clear as to what they're holding.

Otherwise looks fine.
Comment 7 Maxime Simon 2009-08-13 01:12:47 PDT
Created attachment 34719 [details]
Adding the SharedTimer file for WebCore.


---
 2 files changed, 143 insertions(+), 0 deletions(-)
Comment 8 Eric Seidel (no email) 2009-08-14 17:43:41 PDT
Comment on attachment 34719 [details]
Adding the SharedTimer file for WebCore.

Isn't this reversed?

     bigtime_t interval = fireTime - currentTime();
 90     bigtime_t milliSecondInterval = interval < 0 ? 0 : interval * 1000;

is currentTime() in nanoseconds?  If so "interval" should be labled as such?
Comment 9 Maxime Simon 2009-08-14 22:32:28 PDT
(In reply to comment #8)
> (From update of attachment 34719 [details])
> Isn't this reversed?
> 
>      bigtime_t interval = fireTime - currentTime();
>  90     bigtime_t milliSecondInterval = interval < 0 ? 0 : interval * 1000;
> 
> is currentTime() in nanoseconds?  If so "interval" should be labled as such?

Reversed?

- bigtime_t is a microsecond unit (see extract from the BeBook underneath).
- currentTime() returns a result in seconds (with a precision of a microsecond, see extract from wtf/CurrentTime.cpp below).
- I hope fireTime is in second too.
- Then, the value passed as argument of the BMessageRunner should be a bigtime_t, that's to say a microsecond value.
- It means that interval should be multiplied by 1,000,000 not 1,000. The milliSecondInterval variable must be renamed to microSecondInterval. And, interval should be a double, not a bigtime_t.

Extract from the BeBook:
typedef int64 bigtime_t
This type records the time in microseconds as a 64-bit integer.

Extract from <wtf/CurrentTime.cpp> :
gettimeofday(&now, &zone);
return static_cast<double>(now.tv_sec) + (double)(now.tv_usec / 1000000.0);

So, if my reasoning is right, this code is wrong but not reversed. :)

Regards,
Maxime
Comment 10 Maxime Simon 2009-08-15 00:46:54 PDT
Created attachment 34891 [details]
Adding the SharedTimer file for WebCore.
Comment 11 Eric Seidel (no email) 2009-08-18 15:07:18 PDT
Comment on attachment 34891 [details]
Adding the SharedTimer file for WebCore.

OK.
Comment 12 Eric Seidel (no email) 2009-08-18 16:38:34 PDT
Comment on attachment 34891 [details]
Adding the SharedTimer file for WebCore.

Rejecting patch 34891 from commit-queue.  This patch will require manual commit.

Failed to run "['git', 'svn', 'rebase']"  exit_code: 1  cwd: None
Comment 13 Eric Seidel (no email) 2009-08-18 19:02:24 PDT
ChangeLog changed since.  bugzilla-tool isn't smart enough to call resolve-ChangeLogs yet. :(

all 11101 test cases succeeded
Parsing ChangeLog: WebCore/ChangeLog
Updating working directory
/Users/eseidel/Projects/WebKit2/.git/rebase-apply/patch:62: trailing whitespace.
 * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. 
error: patch failed: WebCore/ChangeLog:1
error: WebCore/ChangeLog: patch does not apply
<stdin>:62: trailing whitespace.
 * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. 
warning: 1 line adds whitespace errors.
rebase refs/remotes/trunk: command returned error: 1

Logging in as eric@webkit.org...
Rejecting patch 34891 from commit-queue.  This patch will require manual commit.
Comment 14 Adam Barth 2009-08-18 20:03:41 PDT
Comment on attachment 34891 [details]
Adding the SharedTimer file for WebCore.

This patch looks beautiful.  Let's try again.
Comment 15 Eric Seidel (no email) 2009-08-18 20:32:59 PDT
Comment on attachment 34891 [details]
Adding the SharedTimer file for WebCore.

Clearing flags on attachment: 34891

Committed r47480: <http://trac.webkit.org/changeset/47480>
Comment 16 Eric Seidel (no email) 2009-08-18 20:33:04 PDT
All reviewed patches have been landed.  Closing bug.