Bug 112308 - Add runtime setting for hidden page DOM timer throttling and CSS animation suspension
Summary: Add runtime setting for hidden page DOM timer throttling and CSS animation su...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kiran Muppala
URL:
Keywords: InRadar
Depends on:
Blocks: 112323
  Show dependency treegraph
 
Reported: 2013-03-13 18:24 PDT by Kiran Muppala
Modified: 2020-01-31 08:18 PST (History)
8 users (show)

See Also:


Attachments
Patch (22.89 KB, patch)
2013-03-13 19:43 PDT, Kiran Muppala
no flags Details | Formatted Diff | Diff
Patch (22.59 KB, patch)
2013-03-13 20:19 PDT, Kiran Muppala
no flags Details | Formatted Diff | Diff
Patch (22.60 KB, patch)
2013-03-14 14:41 PDT, Kiran Muppala
no flags Details | Formatted Diff | Diff
Patch (23.63 KB, patch)
2013-03-15 21:42 PDT, Kiran Muppala
barraclough: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kiran Muppala 2013-03-13 18:24:47 PDT
https://bugs.webkit.org/show_bug.cgi?id=98474 added a feature define, HIDDEN_PAGE_DOM_TIMER_THROTTLING, to enable throttling of Javascript timers when the visibility state of a page is set to "Hidden".  Later https://bugs.webkit.org/show_bug.cgi?id=106940 added logic to suspend CSS animations for "Hidden" pages.  These two features can break rendering of off screen web views, particularly Javascript timer throttling.  Hence it is preferable to add a setting for these features.
Comment 1 Kiran Muppala 2013-03-13 18:25:55 PDT
<rdar://problem/13337564>
Comment 2 Kiran Muppala 2013-03-13 19:35:02 PDT
The settings should be disabled by default to minimize compatibility issues.
Comment 3 Kiran Muppala 2013-03-13 19:43:49 PDT
Created attachment 193040 [details]
Patch
Comment 4 Early Warning System Bot 2013-03-13 19:51:31 PDT
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 5 EFL EWS Bot 2013-03-13 20:15:09 PDT
Comment on attachment 193040 [details]
Patch

Attachment 193040 [details] did not pass efl-ews (efl):
Output: http://webkit-commit-queue.appspot.com/results/17133130
Comment 6 Kiran Muppala 2013-03-13 20:19:38 PDT
Created attachment 193051 [details]
Patch
Comment 7 Kiran Muppala 2013-03-14 14:40:24 PDT
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.
Comment 8 Kiran Muppala 2013-03-14 14:41:22 PDT
Created attachment 193185 [details]
Patch
Comment 9 Kiran Muppala 2013-03-14 14:42:56 PDT
(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.
Comment 10 Kiran Muppala 2013-03-15 21:42:29 PDT
Created attachment 193422 [details]
Patch
Comment 11 Kiran Muppala 2013-03-15 21:43:11 PDT
(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 12 Gavin Barraclough 2013-03-20 23:49:41 PDT
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
Comment 13 Kiran Muppala 2013-03-21 00:04:56 PDT
(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
Comment 14 Gavin Barraclough 2013-03-21 00:51:39 PDT
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.
Comment 15 Kiran Muppala 2013-03-22 19:21:43 PDT
Committed r146704: <http://trac.webkit.org/changeset/146704>
Comment 16 David Kilzer (:ddkilzer) 2013-03-24 08:50:21 PDT
(In reply to comment #15)
> Committed r146704: <http://trac.webkit.org/changeset/146704>

Build fix in <http://trac.webkit.org/r146729>
Comment 17 Kiran Muppala 2013-03-24 11:40:58 PDT
(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.