Bug 154578

Summary: Add a mechanism to automatically ramp up timer alignment.
Product: WebKit Reporter: Gavin Barraclough <barraclough>
Component: WebKit Misc.Assignee: Gavin Barraclough <barraclough>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue
Priority: P2    
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Fix
koivisto: review+
Update with review fixes, setting -> bool (temporarily removed limit) cdumez: review+

Description Gavin Barraclough 2016-02-23 02:06:06 PST
Allow timer throttling to be proportional to the duration of time the page has been hidden.
Comment 1 Gavin Barraclough 2016-02-23 02:17:39 PST
Created attachment 272004 [details]
Fix
Comment 2 Antti Koivisto 2016-02-23 03:46:54 PST
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).
Comment 3 Chris Dumez 2016-02-23 09:01:52 PST
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();
Comment 4 Gavin Barraclough 2016-02-23 15:51:38 PST
(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.
Comment 5 Gavin Barraclough 2016-02-23 15:57:57 PST
Created attachment 272059 [details]
Update with review fixes, setting -> bool (temporarily removed limit)
Comment 6 WebKit Commit Bot 2016-02-23 15:59:37 PST
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.
Comment 7 Chris Dumez 2016-02-23 16:04:46 PST
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.
Comment 8 Gavin Barraclough 2016-02-23 16:32:55 PST
Transmitting file data .....
Committed revision 197006.