[webkit-reviews] review granted: [Bug 136197] DOM timer throttling for hidden plugins : [Attachment 237585] New fix

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has granted Gavin Barraclough
<barraclough at apple.com>'s request for review:
Bug 136197: DOM timer throttling for hidden plugins
https://bugs.webkit.org/show_bug.cgi?id=136197

Attachment 237585: New fix
https://bugs.webkit.org/attachment.cgi?id=237585&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=237585&action=review


r=me, some suggested tweaks below.

> Source/WebCore/ChangeLog:14
> +	       - DOMTimer::fired detect timers that interact with
invisible/inaudible plugins, and flags itself for throtting.

"detects"

> Source/WebCore/html/HTMLPlugInElement.h:91
> +    bool isDetectable() const;

Maybe "isUserObservable"? "isDetectable" doesn't say to whom. Something about
user seems good here.

> Source/WebCore/page/DOMTimer.cpp:51
> +static const int minIntervalForUndetectablePluginScriptTimers = 1000; //
Empirically determined.

Empirically determined... to maximize battery life?

> Source/WebCore/page/DOMTimer.cpp:165
> +    // For worked threads, don't update the current DOMTimerFireState.

"worker"

Should say why: it's both not thread-safe, and not plugin-relevant.

> Source/WebCore/page/DOMTimer.cpp:166
> +    DOMTimerFireState fireState(context->isDocument());

Seems like it might be cleaner to pass the context to the constructor, rather
than a boolean. It's an implementation detail of DOMTimerFireState that it only
wants to track main thread / DOM timers.

> Source/WebCore/page/DOMTimer.cpp:279
> +    // Apply two thottles - the global (per Page) minimum, and also a
per-timer throttle.

"throttles"