Bug 138873

Created attachment 241864 [details] Patch

Comment on attachment 241864 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=241864&action=review > LayoutTests/fast/dom/repeating-timer-element-overflow-throttling.html:4 > +<ul id="testElement" style="height: 0; overflow: visible;"> overflow: visible is the default.

What about opacity:0 or visibility:hidden elements? What if the timer toggles the size of the element to/from 0? What if the timer does other useful work other than touch style? If a timer touches several elements, only some of which are outside the viewport, does it get throttled? What if the page uses requestAnimationFrame to touch style on hidden elements?

(In reply to comment #3) > What about opacity:0 or visibility:hidden elements? I will have to check. If it returns in the element having no renderer (like 'display: none'), it would cause the timer to get throttled. If they have a renderer, we may need to add code for handling this to try and throttle timers that change the style of the element (as they are not visible to the user). Likely, such timers don't get throttled right now, but this is safe and can be improved later. > What if the timer > toggles the size of the element to/from 0? It should cause layout, right? and on layout we will re-evaluate if the elements causing the timer to be throttled is still not visible. > What if the timer does other > useful work other than touch style? Yes, this is the risk. If we find such cases, we could add more metrics to detect if a timer has visible side-effects. We have been throttling timers that interact with non-visible plugins for a while now, and so far, this hasn't been an issue. > If a timer touches several elements, > only some of which are outside the viewport, does it get throttled? No, if the timer touches the style of *any* element inside the viewport, we won't throttle it. I will add test coverage for this. > What if the page uses requestAnimationFrame to touch style on hidden > elements? Then it is not using DOMTimers (AFAIK), and thus won't be affected by throttling.

Created attachment 241865 [details] Patch

(In reply to comment #4) > > What if the page uses requestAnimationFrame to touch style on hidden > > elements? > > Then it is not using DOMTimers (AFAIK), and thus won't be affected by > throttling. rAF is increasingly being used for very CPU-heave ads (e.g. those generated by Swiffy), so we should consider throttling here, particularly when the content is inside an iframe. Can you add a test for timer throttling inside an iframe that's scrolled out of view? That's very common.

> > What if the timer does other > > useful work other than touch style? > > Yes, this is the risk. If we find such cases, we could add more metrics to > detect if a timer has visible side-effects. We have been throttling timers > that interact with non-visible plugins for a while now, and so far, this > hasn't been an issue. Also note that we don't throttle timers that alter the DOM tree for this reason as well.

(In reply to comment #6) > (In reply to comment #4) > > > > What if the page uses requestAnimationFrame to touch style on hidden > > > elements? > > > > Then it is not using DOMTimers (AFAIK), and thus won't be affected by > > throttling. > > rAF is increasingly being used for very CPU-heave ads (e.g. those generated > by Swiffy), so we should consider throttling here, particularly when the > content is inside an iframe. I can look at rAF later, see if we can apply a similar policy there. Right now, throttling DOM timers seems risky enough :) I think it is best not to extend this approach too much until we get sufficient living-on testing of this, to make sure this does not break content. > Can you add a test for timer throttling inside an iframe that's scrolled out > of view? That's very common. Yes, sounds like a good thing to test indeed.

Comment on attachment 241865 [details] Patch Clearing flags on attachment: 241865 Committed r176323: <http://trac.webkit.org/changeset/176323>

All reviewed patches have been landed. Closing bug.