[webkit-reviews] review denied: [Bug 28455] ThreadTimer: avoid blocking UI when too many timers ready to fire : [Attachment 35123] the patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Yong Li <yong.li at torchmobile.com>'s
request for review:
Bug 28455: ThreadTimer: avoid blocking UI when too many timers ready to fire
https://bugs.webkit.org/show_bug.cgi?id=28455

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

------- Additional Comments from Darin Adler <darin at apple.com>
The concept of the code is here OK, but the conditionals need to be done
better.

I think you should have the TimerBeingFired struct all the time, and just make
the fire time member be conditional.

Normally we do not use the m_ prefix for struct members, only for classes where
they are private.

> +    for (Vector<TimerBeingFired>::const_iterator i = firingTimers.begin(); i
!= firingTimers.end(); ++i) {
> +	   TimerBase* timer = i->m_timer;

>      for (size_t i = 0; i != size; ++i) {
>	   TimerBase* timer = firingTimers[i];

We do not normally use iterators to go through a Vector. Instead we just use
indexing as you would with an array. That's what we did in the existing code,
and there's no reason to change that just because the timers are a struct.

> +	   // 30ms
> +	   if (currentTime() - startTime > 0.03) {

The time interval should be a constant at the top of the file, and the comment
should explain why that particular interval was chosen.

> +	       for (++i; i != firingTimers.end(); ++i) {
> +		   timer = i->m_timer;
> +		   if (timer->m_nextFireTime == 0 &&
m_timersReadyToFire.contains(timer))
> +		       timer->setNextFireTime(i->m_fireTime);
> +	       }

I wrote the class, and yet I do not understand this code. It may be correct.
There needs to be a comment explaining why this is the right thing to do.