Summary: | Add runtime setting for hidden page DOM timer throttling and CSS animation suspension | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kiran Muppala <cmuppala> | ||||||||||
Component: | Platform | Assignee: | Kiran Muppala <cmuppala> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | andersca, barraclough, ddkilzer, dstockwell, noel.gordon, simon.fraser, thorton, webkit-ews | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=207042 | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 112323 | ||||||||||||
Attachments: |
|
Description
Kiran Muppala
2013-03-13 18:24:47 PDT
The settings should be disabled by default to minimize compatibility issues. Created attachment 193040 [details]
Patch
Comment on attachment 193040 [details] Patch Attachment 193040 [details] did not pass qt-wk2-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/17166388 Comment on attachment 193040 [details] Patch Attachment 193040 [details] did not pass efl-ews (efl): Output: http://webkit-commit-queue.appspot.com/results/17133130 Created attachment 193051 [details]
Patch
On further discussion with Maciej, it was decided that this setting should be enabled by default in WebKit2, since compatibility is less of a issue there. Created attachment 193185 [details]
Patch
(In reply to comment #8) > Created an attachment (id=193185) [details] > Patch Updated patch to make the settings default to false in WebKit1 and true in WebKit2. Created attachment 193422 [details]
Patch
(In reply to comment #10) > Created an attachment (id=193422) [details] > Patch Modified patch to only enable by default on Mac. Since WebKit2 might be a public API on other platforms. Comment on attachment 193422 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=193422&action=review > Source/WebCore/page/Page.cpp:1453 > +#if ENABLE(PAGE_VISIBILITY_API) Why this #ifdef? – setVisibilityState is compiled in if ENABLE(HIDDEN_PAGE_DOM_TIMER_THROTTLING) and not ENABLE(PAGE_VISIBILITY_API) is defined, "setTimerAlignmentInterval(Settings::hiddenPageDOMTimerAlignmentInterval());" may have been called (line 1239/1240). > Source/WebCore/page/Page.cpp:1458 > + setTimerAlignmentInterval(Settings::defaultDOMTimerAlignmentInterval()); This will unnecessarily reset the timer interval where m_visibilityState != WebCore::PageVisibilityStateHidden. Could this not be more like hiddenPageCSSAnimationSuspensionStateChanged, below? #if ENABLE(HIDDEN_PAGE_DOM_TIMER_THROTTLING) void Page::hiddenPageDOMTimerThrottlingStateChanged() { if (m_visibilityState == WebCore::PageVisibilityStateHidden) { if (m_settings->hiddenPageDOMTimerThrottlingEnabled()) setTimerAlignmentInterval(Settings::hiddenPageDOMTimerAlignmentInterval()); else setTimerAlignmentInterval(Settings::defaultDOMTimerAlignmentInterval()); } } #endif (In reply to comment #12) > (From update of attachment 193422 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=193422&action=review > > > Source/WebCore/page/Page.cpp:1453 > > +#if ENABLE(PAGE_VISIBILITY_API) > > Why this #ifdef? – setVisibilityState is compiled in if ENABLE(HIDDEN_PAGE_DOM_TIMER_THROTTLING) and not ENABLE(PAGE_VISIBILITY_API) is defined, "setTimerAlignmentInterval(Settings::hiddenPageDOMTimerAlignmentInterval());" may have been called (line 1239/1240). > > > Source/WebCore/page/Page.cpp:1458 > > + setTimerAlignmentInterval(Settings::defaultDOMTimerAlignmentInterval()); > > This will unnecessarily reset the timer interval where m_visibilityState != WebCore::PageVisibilityStateHidden. Could this not be more like hiddenPageCSSAnimationSuspensionStateChanged, below? > HIDDEN_PAGE_DOM_TIMER_THROTTLING doesn't compile in m_visibilityState needed to remember the last set value. Only PAGE_VISIBILITY_API does that. On Mac, this is not a concern because we enable both. And I have a separate bug https://bugs.webkit.org/show_bug.cgi?id=112323, to remove HIDDEN_PAGE_DOM_TIMER_THROTTLING entirely and just use PAGE_VISIBILITY_API. Then the code will match CSS animation suspension. > #if ENABLE(HIDDEN_PAGE_DOM_TIMER_THROTTLING) > void Page::hiddenPageDOMTimerThrottlingStateChanged() > { > if (m_visibilityState == WebCore::PageVisibilityStateHidden) { > if (m_settings->hiddenPageDOMTimerThrottlingEnabled()) > setTimerAlignmentInterval(Settings::hiddenPageDOMTimerAlignmentInterval()); > else > setTimerAlignmentInterval(Settings::defaultDOMTimerAlignmentInterval()); > } > } > #endif r is me. It's a bit gross that we can forget the state we're in, but I guess this is at least conservative in favouring the default timeout. Committed r146704: <http://trac.webkit.org/changeset/146704> (In reply to comment #15) > Committed r146704: <http://trac.webkit.org/changeset/146704> Build fix in <http://trac.webkit.org/r146729> (In reply to comment #16) > (In reply to comment #15) > > Committed r146704: <http://trac.webkit.org/changeset/146704> > > Build fix in <http://trac.webkit.org/r146729> I missed that, EWS wasn't of help. Thanks for the fix. |