Bug 154559 – Some timer alignment cleanup.

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

[Gavin Barraclough]

Document shouldn't override Page's timer alignment policy with a lower alignment, and m_domTimerAlignmentInterval isn't really a Settings, it's just the current state for the page.

[Gavin Barraclough]

Created attachment 271963 [details]
Fix

[Anders Carlsson]

Comment on attachment 271963 [details]
Fix

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

You should change this to use std::chrono::milliseconds!

> Source/WebCore/page/Page.h:620
> -    bool m_timerThrottlingEnabled;
> +    bool m_timerThrottlingEnabledTime;
> +    double m_timerAlignmentInterval;

Can this use Optional instead?

[Gavin Barraclough]

(In reply to comment #2)
> Comment on attachment 271963 [details]
> Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=271963&action=review
> 
> You should change this to use std::chrono::milliseconds!

Heh, probably! - sure I'll look into this. (But as a separate change - these intervals are all currently doubles & I should probably switch double -> chrono::milliseconds in a bunch of other places at the same time).

> > Source/WebCore/page/Page.h:620
> > -    bool m_timerThrottlingEnabled;
> > +    bool m_timerThrottlingEnabledTime;
> > +    double m_timerAlignmentInterval;
> 
> Can this use Optional instead?

Interesting. I don't think this should be an Optional - when timer throttling is disabled the timer alignment property does still hold a value (DOMTimer::defaultAlignmentInterval() - which theoretically could be non-zero).

Also I plan on replacing m_timerThrottlingEnabled with the timestamp at which time throttling is enabled, so we know how long it has been - come to think of it, that probably should be an Optional.

[Chris Dumez]

Comment on attachment 271963 [details]
Fix

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

r=me % nits

> Source/WebCore/dom/Document.cpp:2934
> +    if (Page* page = this->page())

auto*

> Source/WebCore/page/Page.cpp:208
> +    , m_timerThrottlingEnabledTime(false)

I prefer the previous name since this is a boolean.

[Gavin Barraclough]

> > Source/WebCore/page/Page.cpp:208
> > +    , m_timerThrottlingEnabledTime(false)
> 
> I prefer the previous name since this is a boolean.

Oops, yes!

[Gavin Barraclough]

Transmitting file data ......
Committed revision 196971.