[webkit-reviews] review denied: [Bug 23488] Make TimerBase thread-aware. For use in Workers. : [Attachment 26985] Updated patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Dmitry Titov <dimich at chromium.org>'s
request for review:
Bug 23488: Make TimerBase thread-aware. For use in Workers.
https://bugs.webkit.org/show_bug.cgi?id=23488

Attachment 26985: Updated patch
https://bugs.webkit.org/attachment.cgi?id=26985&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +    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+.