[Webkit-unassigned] [Bug 23488] Make TimerBase thread-aware. For use in Workers.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=23488


darin at apple.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #26985|review?                     |review-
               Flag|                            |




------- Comment #4 from darin at apple.com  2009-01-24 13:35 PDT -------
(From update of attachment 26985)
> +    class SharedTimer {
> +    public:
> +        virtual ~SharedTimer() {}
> +        virtual void setFiredFunction(void (*)()) = 0;
> +        // The fire time is relative to the classic POSIX epoch of January 1, 1970,
> +        // as the result of currentTime() is.
> +        virtual void setFireTime(double) = 0;
> +        virtual void stop() = 0;
> +    };

The formatting was slightly better in the old version, where there was a blank
line before the comment. It's a little harder to understand what the comment is
grouped with here with everything mushed together.

> +Vector<TimerBase*>* ThreadTimers::timerHeap()
> +{
> +    return &m_timerHeap;
> +}

I think this should return a reference instead of a pointer. Callers currently
have code to handle 0, and I'm not sure why.

> +HashSet<const TimerBase*>* ThreadTimers::timersReadyToFire()
> +{
> +    return m_timersReadyToFire;
> +}

This too.

> -        ASSERT(m_index < (timerHeap ? static_cast<int>(timerHeap->size()) : 0));
> +        ASSERT(m_index < (timerHeap() ? static_cast<int>(timerHeap()->size()) : 0));

Can timerHeap() be zero? If not, lets remove the unneeded code.

> -        ASSERT_UNUSED(offset, m_index + offset <= (timerHeap ? static_cast<int>(timerHeap->size()) : 0));
> +        ASSERT_UNUSED(offset, m_index + offset <= (timerHeap() ? static_cast<int>(timerHeap()->size()) : 0));

Same question here.

> -    return m_nextFireTime || (timersReadyToFire && timersReadyToFire->contains(this));
> +    return m_nextFireTime || (timersReadyToFire() && timersReadyToFire()->contains(this));

Can timersReadToFire() be zero? If not, lets remove the unneeded code.

> +class SharedTimer;

Forward declarations should all go at the top of the file, not in the middle.

> +// A collection of timers per thread. Kept in ThreadGlobalData.
> +class ThreadTimers : Noncopyable {

We normally want one class per header file. Can this class go in its own
header? Also, can it be private? Do source files using the Timer class need to
see this class's definition?

Can TimerBase still be the public interface to fireTimersInNestedEventLoop --
after all, it can just call through to the ThreadTimers function.

I think the patch would be so much better if you used references instead of
pointers that I'm going to say review- and wait for a new patch. But generally,
this patch seems great, and I was really close to saying review+.


-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.