Bug 154578 – Add a mechanism to automatically ramp up timer alignment.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Gavin Barraclough]

Allow timer throttling to be proportional to the duration of time the page has been hidden.

[Gavin Barraclough]

Created attachment 272004 [details]
Fix

[Antti Koivisto]

Comment on attachment 272004 [details]
Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=272004&action=review

> Source/WebCore/page/Page.h:620
> +    Optional<double> m_timerThrottlingEnabledTime;

It would be more stylish to use std::chrono::steady_clock::time_point and pals.

> Source/WebCore/page/Settings.cpp:215
> +    , m_hiddenPageDOMTimerThrottlingAutoIncreaseLimit(0.0)

You could use { } initialization in the header.

> Source/WebCore/page/Settings.h:345
>      bool m_hiddenPageDOMTimerThrottlingEnabled : 1;
>  #endif
> +    double m_hiddenPageDOMTimerThrottlingAutoIncreaseLimit;
> +
>      bool m_hiddenPageCSSAnimationSuspensionEnabled : 1;
>      bool m_fontFallbackPrefersPictographs : 1;

Please don't put this in the middle of a bitfield (though the bitfield itself is silly and could be removed).

[Chris Dumez]

Comment on attachment 272004 [details]
Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=272004&action=review

> Source/WebCore/page/Page.cpp:1177
> +    m_timerThrottlingEnabledTime = enabled ? monotonicallyIncreasingTime() : Optional<double>();

m_timerThrottlingEnabledTime = enabled ? monotonicallyIncreasingTime() : NullOpt; did not build?

> Source/WebCore/page/Page.cpp:1194
> +            if (frame->document())

Would be nice to store the returned document in a variable:
if (auto* document = frame->document())
    document->didChangeTimerAlignmentInterval();

[Gavin Barraclough]

(In reply to comment #3)
> Comment on attachment 272004 [details]
> Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=272004&action=review
> 
> > Source/WebCore/page/Page.cpp:1177
> > +    m_timerThrottlingEnabledTime = enabled ? monotonicallyIncreasingTime() : Optional<double>();
> 
> m_timerThrottlingEnabledTime = enabled ? monotonicallyIncreasingTime() :
> NullOpt; did not build?
> 
> > Source/WebCore/page/Page.cpp:1194
> > +            if (frame->document())
> 
> Would be nice to store the returned document in a variable:
> if (auto* document = frame->document())
>     document->didChangeTimerAlignmentInterval();

(In reply to comment #2)
> Comment on attachment 272004 [details]
> Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=272004&action=review
> 
> > Source/WebCore/page/Page.h:620
> > +    Optional<double> m_timerThrottlingEnabledTime;
> 
> It would be more stylish to use std::chrono::steady_clock::time_point and
> pals.

Will do - I'm going to land as is (since everything is currently using double), and make a patch to push std::chrono types through all the timer alignment code.
 
> > Source/WebCore/page/Settings.cpp:215
> > +    , m_hiddenPageDOMTimerThrottlingAutoIncreaseLimit(0.0)
> 
> You could use { } initialization in the header.

Done.

> > Source/WebCore/page/Settings.h:345
> >      bool m_hiddenPageDOMTimerThrottlingEnabled : 1;
> >  #endif
> > +    double m_hiddenPageDOMTimerThrottlingAutoIncreaseLimit;
> > +
> >      bool m_hiddenPageCSSAnimationSuspensionEnabled : 1;
> >      bool m_fontFallbackPrefersPictographs : 1;
> 
> Please don't put this in the middle of a bitfield (though the bitfield
> itself is silly and could be removed).

Fixed.

(In reply to comment #3)
> Comment on attachment 272004 [details]
> Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=272004&action=review
> 
> > Source/WebCore/page/Page.cpp:1177
> > +    m_timerThrottlingEnabledTime = enabled ? monotonicallyIncreasingTime() : Optional<double>();
> 
> m_timerThrottlingEnabledTime = enabled ? monotonicallyIncreasingTime() :
> NullOpt; did not build?

I can't :'-( - one side of the ?: needs to explicitly be the optional type.
 
> > Source/WebCore/page/Page.cpp:1194
> > +            if (frame->document())
> 
> Would be nice to store the returned document in a variable:
> if (auto* document = frame->document())
>     document->didChangeTimerAlignmentInterval();

Done.

Based on feedback from Sam going to move the limit to ChromeClient (since we've going to vary this dynamically). For now going to just remove the limit (am landing disabled anyway), and will reintroduce this in another patch.

[Gavin Barraclough]

Created attachment 272059 [details]
Update with review fixes, setting -> bool (temporarily removed limit)

[WebKit Commit Bot]

Attachment 272059 [details] did not pass style-queue:


ERROR: Source/WebCore/page/Page.cpp:1202:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Comment on attachment 272059 [details]
Update with review fixes, setting -> bool (temporarily removed limit)

View in context: https://bugs.webkit.org/attachment.cgi?id=272059&action=review

r=me

> Source/WebCore/ChangeLog:10
> +        in exponential steps, spaced exponentially far apart, until a limit is

The "until a limit is reached" part is no longer true.

[Gavin Barraclough]

Transmitting file data .....
Committed revision 197006.