[webkit-reviews] review denied: [Bug 18044] Timer Bug : [Attachment 20114] Nested event loop patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Michael Emmel <mike.emmel at gmail.com>'s
request for review:
Bug 18044: Timer Bug
http://bugs.webkit.org/show_bug.cgi?id=18044

Attachment 20114: Nested event loop patch
http://bugs.webkit.org/attachment.cgi?id=20114&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
Could you fix the formatting of this patch? The comments are not matching the
style of the surrounding comments (all lowercase, no space after the //,
multiple spaces in one place, no capital letters on the sentences).

Could you reword the comments to be clearer and use more straightforward
technology? What's a "bool guard"?

This sequence seems obviously wrong:

+	 if (timersReadyToFire)
+	     timersReadyToFire->remove(timer);
+	 if(!timersReadyToFire->size())
+	     timersReadyToFire = 0;

If timersReadyToFire is 0, then we're going to crash dereferencing the pointer.


I don't understand the change log comment at all. In what sense does this
change make anything "generic"?

I think this change to the Timer class may be an incorrect workaround for a bug
elsewhere.

The logic here seems really muddled. The old change gave a special meaning to a
timersReadyToFire value of 0. Just setting it to zero because you fired the
last timer is not at all consistent with that design.

I believe the correct fix is to add a call to fireTimersInNestedEventLoop
somewhere.