[webkit-reviews] review granted: [Bug 98474] Throttle DOM timers on hidden pages. : [Attachment 167389] Patch

[bugzilla-daemon at webkit.org]

Maciej Stachowiak <mjs at apple.com> has granted Kiran Muppala
<cmuppala at apple.com>'s request for review:
Bug 98474: Throttle DOM timers on hidden pages.
https://bugs.webkit.org/show_bug.cgi?id=98474

Attachment 167389: Patch
https://bugs.webkit.org/attachment.cgi?id=167389&action=review

------- Additional Comments from Maciej Stachowiak <mjs at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=167389&action=review


Looks good to me in general; I would suggest taking one more pass to see if it
is practical to reduce the amount of code in ifdefs.

>>>> Source/WebCore/dom/Document.cpp:2676
>>>> +#endif
>>> 
>>> Why does this and a lot of other code in this patch have to be inside the
#ifdefs? Can't you just have timerAlignmentInterval() always return 0 when this
feature is disabled?
>> 
>> The timer alignment code path needs an additional data member in TimerBase
to keep track of the fire time without alignment.  I didn't want to add it
unconditionally and use 0 for alignment interval.
> 
> So you can #ifdef that too, but in general we prefer to have most of the code
un-ifdeffed.
> 
> Simon

I agree that it makes sense to minimize the ifdefs. Even if some code has to be
ifdef'd, it's still valuable to have as much code as possible shared.