Bug 28126 – [Haiku] Adding SharedTimer to WebCore.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Maxime Simon]

Patch to add the SharedTimerHaiku.* files to WebCore.

[Maxime Simon]

Created attachment 34412 [details]
Patch to add the SharedTimer files for Haiku.

[Eric Seidel (no email)]

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.

[Maxime Simon]

(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

[Ryan Leavengood]

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

[Maxime Simon]

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.

[Eric Seidel (no email)]

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.

[Maxime Simon]

Created attachment 34719 [details]
Adding the SharedTimer file for WebCore.


---
 2 files changed, 143 insertions(+), 0 deletions(-)

[Eric Seidel (no email)]

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?

[Maxime Simon]

(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

[Maxime Simon]

Created attachment 34891 [details]
Adding the SharedTimer file for WebCore.

[Eric Seidel (no email)]

Comment on attachment 34891 [details]
Adding the SharedTimer file for WebCore.

OK.

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

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 [e-mail]...
Rejecting patch 34891 from commit-queue.  This patch will require manual commit.

[Adam Barth]

Comment on attachment 34891 [details]
Adding the SharedTimer file for WebCore.

This patch looks beautiful.  Let's try again.

[Eric Seidel (no email)]

Comment on attachment 34891 [details]
Adding the SharedTimer file for WebCore.

Clearing flags on attachment: 34891

Committed r47480: <http://trac.webkit.org/changeset/47480>

[Eric Seidel (no email)]

All reviewed patches have been landed.  Closing bug.