Bug 142837 - Clean up DOMTimer related settings
Summary: Clean up DOMTimer related settings
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords:
: 61214 (view as bug list)
Depends on:
Blocks:
 
Reported: 2015-03-18 13:40 PDT by Chris Dumez
Modified: 2015-03-19 12:23 PDT (History)
6 users (show)

See Also:


Attachments
Patch (21.55 KB, patch)
2015-03-18 13:51 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (22.09 KB, patch)
2015-03-18 15:30 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (22.30 KB, patch)
2015-03-18 16:32 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (21.39 KB, patch)
2015-03-19 11:35 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2015-03-18 13:40:43 PDT
Clean up DOMTimer related settings
Comment 1 Chris Dumez 2015-03-18 13:51:19 PDT
Created attachment 248962 [details]
Patch
Comment 2 Chris Dumez 2015-03-18 15:30:27 PDT
Created attachment 248974 [details]
Patch
Comment 3 Chris Dumez 2015-03-18 16:32:10 PDT
Created attachment 248982 [details]
Patch
Comment 4 Chris Dumez 2015-03-18 20:35:40 PDT
*** Bug 61214 has been marked as a duplicate of this bug. ***
Comment 5 Darin Adler 2015-03-19 08:39:04 PDT
Comment on attachment 248982 [details]
Patch

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

> Source/WebCore/page/DOMTimer.cpp:62
> +const double DOMTimer::gDefaultMinimumInterval = 0.004; // 4 milliseconds.
> +const double DOMTimer::gDefaultAlignmentInterval = 0;
> +const double DOMTimer::gHiddenPageAlignmentInterval = 1.0; // 1 second.

I now see that you just moved this, but still:

Is this "g" prefix something we use in WebKit? I don’t think it is. I would have used no prefix at all, but others, I guess, use an "s_" prefix. Even better, just put these constants into the header file inside the static function member return statements and don’t bother with data members.

> Source/WebCore/page/Settings.cpp:449
> +    for (Frame* frame = &m_page->mainFrame(); frame; frame = frame->tree().traverseNextWithWrap(false)) {

There is no reason to use traverseNextWithWrap(false) instead of traverseNext(). This seems to be a mistake in Page::setMinimumTimerInterval and Page::setTimerThrottlingEnabled that we should fix and not repeat.

> Source/WebCore/page/Settings.h:166
> +    WEBCORE_EXPORT void setMinimumDOMTimerInterval(double); // Per-page; initialized to default value.

I don’t understand what the comment “per-page” means. Aren’t all settings on Settings equally per-page or not-per-page?
Comment 6 Darin Adler 2015-03-19 08:50:19 PDT
Comment on attachment 248982 [details]
Patch

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

> Source/WebCore/page/DOMTimer.h:53
> +        static double hiddenPageAlignmentInterval() { return gHiddenPageAlignmentInterval; }
> +        static double defaultAlignmentInterval() { return gDefaultAlignmentInterval; }

It’s more logical to list the default alignment interval first and then the hidden page alignment interval.
Comment 7 Chris Dumez 2015-03-19 11:35:49 PDT
Created attachment 249038 [details]
Patch
Comment 8 WebKit Commit Bot 2015-03-19 12:23:00 PDT
Comment on attachment 249038 [details]
Patch

Clearing flags on attachment: 249038

Committed r181753: <http://trac.webkit.org/changeset/181753>
Comment 9 WebKit Commit Bot 2015-03-19 12:23:05 PDT
All reviewed patches have been landed.  Closing bug.