Bug 54312

Summary: Allow controlling minimum DOMTimer interval on a per-page basis
Product: WebKit Reporter: Kenneth Russell <kbr>
Component: WebCore Misc.Assignee: Kenneth Russell <kbr>
Status: RESOLVED FIXED    
Severity: Normal CC: akling, aroben, buildbot, davemoore, dbates, fishd, gns, gustavo.noronha, hausmann, hyatt, jamesr, manyoso, mdelaney7, mrobinson, noam, sam, simon.fraser, tonikitoo, webkit-ews, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 54772, 55014, 55176, 67012    
Attachments:
Description Flags
Patch
none
Fixed GTK build failure
none
Addressed code review feedback.
none
Fixed qt and win build failures
none
Addressed review feedback
none
Minor Win port change
none
Try to make Win bot notice IDL change
none
GTK build fix
none
Final patch none

Description Kenneth Russell 2011-02-11 14:12:34 PST
It is desirable to be able to control the minimum DOMTimer interval on a per-page basis. In particular, to avoid consuming excessive CPU and battery life on mobile devices, some ports would like to be able to increase the minimum interval to a large value, like 500 ms, when a tab is in the background, and reduce it when the tab is made visible again. When adding this support to WebCore, it's important to add hooks to the LayoutTestController so that automated tests can be written.
Comment 1 Kenneth Russell 2011-02-11 15:26:45 PST
Created attachment 82192 [details]
Patch
Comment 2 Kenneth Russell 2011-02-11 15:28:58 PST
Note to reviewers: the style violations are in files that don't currently match WebKit style. The new code was made consistent with the preexisting code. The failures were:

Source/WebKit/gtk/webkit/webkitwebview.h:407:  The parameter name "webView" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/gtk/webkit/webkitwebview.h:407:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit/gtk/webkit/webkitwebview.h:410:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit/gtk/webkit/webkitwebview.h:410:  The parameter name "webView" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/gtk/webkit/webkitwebview.h:411:  Extra space between gdouble and interval_in_seconds  [whitespace/declaration] [3]
Source/WebCore/page/DOMTimer.h:37:  Code inside a namespace should not be indented.  [whitespace/indent] [4]

While I tested these changes in the Safari and Chromium DRT ports, the other ports' changes were made blindly, so I'm relying on the EWS and bots to catch errors.
Comment 3 WebKit Review Bot 2011-02-11 15:30:50 PST
Attachment 82192 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebKit/gtk/webkit/webkitwebview.h:407:  The parameter name "webView" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/gtk/webkit/webkitwebview.h:407:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit/gtk/webkit/webkitwebview.h:410:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit/gtk/webkit/webkitwebview.h:410:  The parameter name "webView" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/gtk/webkit/webkitwebview.h:411:  Extra space between gdouble and interval_in_seconds  [whitespace/declaration] [3]
Source/WebCore/page/DOMTimer.h:37:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 6 in 57 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 James Robinson 2011-02-11 15:32:08 PST
Do we have to expose API for every port?  I'm not sure everyone will want to expose setters for these things to embedders (even though I know we want to in chromium) - we should leave that choice to the various port maintainers.
Comment 5 Collabora GTK+ EWS bot 2011-02-11 15:41:10 PST
Attachment 82192 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7868739
Comment 6 Kenneth Russell 2011-02-11 15:55:14 PST
(In reply to comment #4)
> Do we have to expose API for every port?  I'm not sure everyone will want to expose setters for these things to embedders (even though I know we want to in chromium) - we should leave that choice to the various port maintainers.

In order to implement the new LayoutTestController method, the new APIs per port are needed. I thought it would be best to make the new tests work on all ports, but can understand that the port maintainers might need to decide when to add the new embedding APIs. If people CC'd on the bug could weigh in one way or the other, that would be helpful. Minimally I'll do Chromium, Mac and Win (unless there are objections for the latter two).
Comment 7 Martin Robinson 2011-02-11 15:58:24 PST
I'm not sure if the GTK+ port wants to expose this (probably not at the moment), but if you have an extra moment, it'd be great if you could expose the necessary functionality to the DRT through Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp, which is what we use to expose WebCore to the layout tests in order to test future APIs. Thanks!
Comment 8 Kenneth Russell 2011-02-11 15:59:26 PST
Created attachment 82204 [details]
Fixed GTK build failure
Comment 9 WebKit Review Bot 2011-02-11 16:00:56 PST
Attachment 82204 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebKit/gtk/webkit/webkitwebview.h:407:  The parameter name "webView" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/gtk/webkit/webkitwebview.h:407:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit/gtk/webkit/webkitwebview.h:410:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit/gtk/webkit/webkitwebview.h:410:  The parameter name "webView" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/gtk/webkit/webkitwebview.h:411:  Extra space between gdouble and interval_in_seconds  [whitespace/declaration] [3]
Source/WebCore/page/DOMTimer.h:37:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 6 in 57 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 10 Adam Roben (:aroben) 2011-02-11 16:03:49 PST
Comment on attachment 82204 [details]
Fixed GTK build failure

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

> Source/WebKit/win/WebView.cpp:738
> +HRESULT STDMETHODCALLTYPE WebView::defaultMinimumTimerInterval(double* interval)
> +{
> +    *interval = Settings::defaultMinDOMTimerInterval();
> +    return S_OK;
> +}
> +
> +HRESULT STDMETHODCALLTYPE WebView::setMinimumTimerInterval(double interval)

No need for the "STDMETHODCALLTYPE on either of these.

You should return E_POINTER if interval is null.

> Source/WebKit/win/Interfaces/IWebView.idl:755
> +    /*!
> +        @method defaultMinimumTimerInterval
> +        @discussion Returns the default minimum interval for DOMTimers on all pages, not
> +        just the one associated with this receiver.
> +        - (double)interval;
> +    */
> +    HRESULT defaultMinimumTimerInterval([out, retval] double* interval);
> +
> +    /*!
> +        @method setMinimumTimerInterval
> +        @discussion Sets the minimum interval for DOMTimers on the web page associated
> +        with the receiver.
> +        - (double)interval;
> +    */
> +    HRESULT setMinimumTimerInterval([in] double interval);

It would be a little better to add these to IWebViewPrivate.
Comment 11 Simon Fraser (smfr) 2011-02-11 16:04:44 PST
Comment on attachment 82204 [details]
Fixed GTK build failure

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

> Source/WebCore/dom/Document.h:1138
> +    virtual double getMinimumTimerInterval();

This method should be const.

> Source/WebCore/dom/ScriptExecutionContext.h:156
> +        void minimumTimerIntervalChanged(double oldMinimumTimerInterval);

I think this method needs a better name; it sounds like an internal method that would get called from a setter.

Maybe adjustMinimumTimerInterval()?

> Source/WebCore/dom/ScriptExecutionContext.h:157
> +        virtual double getMinimumTimerInterval();

This should be const. We normally don't use 'get'.

> Source/WebCore/page/DOMTimer.h:61
> +        double intervalClampedToMinimum(int timeout, double minimumTimerInterval);

Should be const.
Comment 12 Kenneth Russell 2011-02-11 16:17:59 PST
(In reply to comment #7)
> I'm not sure if the GTK+ port wants to expose this (probably not at the moment), but if you have an extra moment, it'd be great if you could expose the necessary functionality to the DRT through Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp, which is what we use to expose WebCore to the layout tests in order to test future APIs. Thanks!

I see. Sure, I'll add the needed methods to DumpRenderTreeSupportGtk.cpp instead.
Comment 13 Kenneth Russell 2011-02-11 16:22:43 PST
(In reply to comment #10)
> (From update of attachment 82204 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=82204&action=review
> 
> > Source/WebKit/win/WebView.cpp:738
> > +HRESULT STDMETHODCALLTYPE WebView::defaultMinimumTimerInterval(double* interval)
> > +{
> > +    *interval = Settings::defaultMinDOMTimerInterval();
> > +    return S_OK;
> > +}
> > +
> > +HRESULT STDMETHODCALLTYPE WebView::setMinimumTimerInterval(double interval)
> 
> No need for the "STDMETHODCALLTYPE on either of these.

Really? Shouldn't the definition match the declaration? The majority of the COM methods in WebView.h and .cpp seem to have STDMETHODCALLTYPE.

> You should return E_POINTER if interval is null.

Thanks, will fix.

> > Source/WebKit/win/Interfaces/IWebView.idl:755
> > +    /*!
> > +        @method defaultMinimumTimerInterval
> > +        @discussion Returns the default minimum interval for DOMTimers on all pages, not
> > +        just the one associated with this receiver.
> > +        - (double)interval;
> > +    */
> > +    HRESULT defaultMinimumTimerInterval([out, retval] double* interval);
> > +
> > +    /*!
> > +        @method setMinimumTimerInterval
> > +        @discussion Sets the minimum interval for DOMTimers on the web page associated
> > +        with the receiver.
> > +        - (double)interval;
> > +    */
> > +    HRESULT setMinimumTimerInterval([in] double interval);
> 
> It would be a little better to add these to IWebViewPrivate.

Can do. Keep in mind that if the browser wants to use this functionality, rather than just DRT, that these methods will need to be called. Given that, should they live on IWebView or IWebViewPrivate?
Comment 14 Kenneth Russell 2011-02-11 16:26:53 PST
(In reply to comment #11)
> (From update of attachment 82204 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=82204&action=review
> 
> > Source/WebCore/dom/Document.h:1138
> > +    virtual double getMinimumTimerInterval();
> 
> This method should be const.

Will fix.

> > Source/WebCore/dom/ScriptExecutionContext.h:156
> > +        void minimumTimerIntervalChanged(double oldMinimumTimerInterval);
> 
> I think this method needs a better name; it sounds like an internal method that would get called from a setter.
> 
> Maybe adjustMinimumTimerInterval()?

Sure, that sounds fine. Will change here and in DOMTimer.

> > Source/WebCore/dom/ScriptExecutionContext.h:157
> > +        virtual double getMinimumTimerInterval();
> 
> This should be const. We normally don't use 'get'.

Will fix.

> > Source/WebCore/page/DOMTimer.h:61
> > +        double intervalClampedToMinimum(int timeout, double minimumTimerInterval);
> 
> Should be const.

Will fix.
Comment 15 Build Bot 2011-02-11 18:01:08 PST
Attachment 82204 [details] did not build on win:
Build output: http://queues.webkit.org/results/7874811
Comment 16 Build Bot 2011-02-11 18:54:41 PST
Attachment 82204 [details] did not build on win:
Build output: http://queues.webkit.org/results/7874829
Comment 17 Early Warning System Bot 2011-02-11 20:18:38 PST
Attachment 82204 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7874841
Comment 18 Collabora GTK+ EWS bot 2011-02-11 20:27:21 PST
Attachment 82204 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7874842
Comment 19 Kenneth Russell 2011-02-14 15:23:57 PST
Created attachment 82372 [details]
Addressed code review feedback.

Addressed all code review feedback so far. On Mac, Win, Gtk and Qt ports, moved supporting APIs for LayoutTestController to private section or DumpRenderTreeSupport. Re-tested in Chromium and WebKit/Mac DRT ports.
Comment 20 WebKit Review Bot 2011-02-14 15:28:01 PST
Attachment 82372 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/page/DOMTimer.h:37:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 57 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 21 Darin Fisher (:fishd, Google) 2011-02-14 16:20:23 PST
Comment on attachment 82372 [details]
Addressed code review feedback.

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

> Source/WebKit/chromium/public/WebSettings.h:113
> +    WEBKIT_API static double defaultMinDOMTimerInterval();

why does WebKit need default getter/setters?  every Page has a Settings,
so we shouldn't need any other way to get/set this value.

> Source/WebKit/chromium/public/WebSettings.h:115
> +    virtual double minDOMTimerInterval() = 0;

it is unusual for WebSettings to provide a getter.  is that really needed?

also maybe "DOMTimerInterval" is not the best name for this given that this
will eventually be used to adjust the timer minimum timer interval for web
workers.  maybe instead of "DOM" you should say "JavaScript"?
Comment 22 Early Warning System Bot 2011-02-14 16:26:30 PST
Attachment 82372 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7913648
Comment 23 Kenneth Russell 2011-02-14 16:41:28 PST
(In reply to comment #21)
> (From update of attachment 82372 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=82372&action=review
> 
> > Source/WebKit/chromium/public/WebSettings.h:113
> > +    WEBKIT_API static double defaultMinDOMTimerInterval();
> 
> why does WebKit need default getter/setters?  every Page has a Settings,
> so we shouldn't need any other way to get/set this value.

The reason is that the default value (0.004, for Chromium) is set from within WebKit code (WebKit.cpp), but the value will be adjusted on a per-page basis by Chromium code. If we provide the default value, then we don't need to store the "saved" value when the tab is hidden so that we can restore it when the tab is made visible. The using code is thereby made more robust.

The setter for the default value is only being provided for completeness. I could remove that API if you want.

> > Source/WebKit/chromium/public/WebSettings.h:115
> > +    virtual double minDOMTimerInterval() = 0;
> 
> it is unusual for WebSettings to provide a getter.  is that really needed?

Strictly, no. I added it only for completeness and would be happy to remove it.

> also maybe "DOMTimerInterval" is not the best name for this given that this
> will eventually be used to adjust the timer minimum timer interval for web
> workers.  maybe instead of "DOM" you should say "JavaScript"?

This name was chosen to match that in the Settings class, and that one was essentially pre-existing. Given that in WebKit, DOMTimer instances are in fact used on worker threads, I'd prefer to leave the name as is.
Comment 24 Build Bot 2011-02-14 16:45:32 PST
Attachment 82372 [details] did not build on win:
Build output: http://queues.webkit.org/results/7908736
Comment 25 Kenneth Russell 2011-02-14 17:20:07 PST
Created attachment 82391 [details]
Fixed qt and win build failures
Comment 26 WebKit Review Bot 2011-02-14 17:22:27 PST
Attachment 82391 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/page/DOMTimer.h:37:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 57 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 27 Build Bot 2011-02-14 17:33:02 PST
Attachment 82372 [details] did not build on win:
Build output: http://queues.webkit.org/results/7912651
Comment 28 Build Bot 2011-02-14 18:59:42 PST
Attachment 82391 [details] did not build on win:
Build output: http://queues.webkit.org/results/7908768
Comment 29 Kenneth Russell 2011-02-14 19:00:25 PST
Created attachment 82404 [details]
Addressed review feedback

Addressed online and offline review feedback from Darin Fisher. Removed all methods from WebSettings except setMinDOMTimerInterval. Constants will be added to Chromium's webkit_glue and webkit_support namespaces to govern pages' behavior in the browser and DRT. A temporary constant has been added to Tools/DumpRenderTree/chromium/WebViewHost.cpp until the Chromium changes land and are rolled forward. Retested in Chromium and DRT.
Comment 30 WebKit Review Bot 2011-02-14 19:03:32 PST
Attachment 82404 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/page/DOMTimer.h:37:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 57 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 31 Kenneth Russell 2011-02-14 19:07:01 PST
Created attachment 82405 [details]
Minor Win port change

Minor change to remove STDMETHODCALLTYPE from new methods in WebView.cpp. Don't expect this to fix the EWS bot build failures.
Comment 32 WebKit Review Bot 2011-02-14 19:10:40 PST
Attachment 82405 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/page/DOMTimer.h:37:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 57 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 33 Kenneth Russell 2011-02-14 19:13:36 PST
Created attachment 82406 [details]
Try to make Win bot notice IDL change

Whitespace-only change to WebKit.idl to try to make the Win EWS bot notice the IWebPagePrivate change in this patch.
Comment 34 WebKit Review Bot 2011-02-14 19:18:07 PST
Attachment 82406 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/page/DOMTimer.h:37:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 58 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 35 Build Bot 2011-02-14 20:02:02 PST
Attachment 82404 [details] did not build on win:
Build output: http://queues.webkit.org/results/7907770
Comment 36 Build Bot 2011-02-14 20:20:55 PST
Attachment 82405 [details] did not build on win:
Build output: http://queues.webkit.org/results/7905821
Comment 37 Collabora GTK+ EWS bot 2011-02-15 00:25:25 PST
Attachment 82406 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7907848
Comment 38 Kenneth Russell 2011-02-15 10:08:24 PST
Created attachment 82481 [details]
GTK build fix

GTK build fix; documented whitespace-only change to WebKit.idl for Win port, which I'll submit with this patch to keep the bots green and remove it immediately afterward.
Comment 39 WebKit Review Bot 2011-02-15 10:10:58 PST
Attachment 82481 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/page/DOMTimer.h:37:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 58 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 40 Darin Fisher (:fishd, Google) 2011-02-15 10:20:42 PST
> > also maybe "DOMTimerInterval" is not the best name for this given that this
> > will eventually be used to adjust the timer minimum timer interval for web
> > workers.  maybe instead of "DOM" you should say "JavaScript"?
> 
> This name was chosen to match that in the Settings class, and that one was essentially pre-existing. Given that in WebKit, DOMTimer instances are in fact used on worker threads, I'd prefer to leave the name as is.

I usually try to make the WebKit API make sense on its own, without knowledge of how it is implemented.  Keeping the publicly facing API consistent with WebCore can be beneficial to API maintainers, but it does nothing to help users of the API.  I think it makes sense to favor users.

Don't you agree that it sounds odd to use "DOMTimer" as a name to describe a WebWorker timer?  It looks like WindowTimers is the name that the HTML spec uses for this, and I'm not sure that that name is any better than DOMTimer.  I'm OK with sticking with DOMTimer in this case.
Comment 41 Kenneth Russell 2011-02-15 10:54:20 PST
(In reply to comment #40)
> > > also maybe "DOMTimerInterval" is not the best name for this given that this
> > > will eventually be used to adjust the timer minimum timer interval for web
> > > workers.  maybe instead of "DOM" you should say "JavaScript"?
> > 
> > This name was chosen to match that in the Settings class, and that one was essentially pre-existing. Given that in WebKit, DOMTimer instances are in fact used on worker threads, I'd prefer to leave the name as is.
> 
> I usually try to make the WebKit API make sense on its own, without knowledge of how it is implemented.  Keeping the publicly facing API consistent with WebCore can be beneficial to API maintainers, but it does nothing to help users of the API.  I think it makes sense to favor users.
> 
> Don't you agree that it sounds odd to use "DOMTimer" as a name to describe a WebWorker timer?  It looks like WindowTimers is the name that the HTML spec uses for this, and I'm not sure that that name is any better than DOMTimer.  I'm OK with sticking with DOMTimer in this case.

There's a comment at the top of WebSettings.h indicating that its methods mostly map 1:1 to those in the Settings class. I agree that the name "DOMTimer" doesn't capture the meaning for both the DOM's and workers' timers, but am not convinced that changing it and introducing a discrepancy is worth it. If you feel strongly, my suggestion would be "setMinimumTimerInterval", similar to the new method in LayoutTestController.
Comment 42 Darin Fisher (:fishd, Google) 2011-02-15 11:13:16 PST
(In reply to comment #41)
> There's a comment at the top of WebSettings.h indicating that its methods mostly map 1:1 to those in the Settings class. I agree that the name "DOMTimer" doesn't capture the meaning for both the DOM's and workers' timers, but am not convinced that changing it and introducing a discrepancy is worth it. If you feel strongly, my suggestion would be "setMinimumTimerInterval", similar to the new method in LayoutTestController.

It is OK to leave it as is, although I like setMinimumTimerInterval too.

Not to beat this horse to death, but I will add that I think the comment at the top of WebSettings.h should be removed.  If someone were to change WebCore::Settings in some material way (e.g., renaming it), they would not think to update WebSettings.h.  The point of the WebKit API is to provide stable insulation around WebCore, so WebCore'isms shouldn't leak to the public.
Comment 43 Kenneth Russell 2011-02-15 11:54:19 PST
(In reply to comment #42)
> (In reply to comment #41)
> > There's a comment at the top of WebSettings.h indicating that its methods mostly map 1:1 to those in the Settings class. I agree that the name "DOMTimer" doesn't capture the meaning for both the DOM's and workers' timers, but am not convinced that changing it and introducing a discrepancy is worth it. If you feel strongly, my suggestion would be "setMinimumTimerInterval", similar to the new method in LayoutTestController.
> 
> It is OK to leave it as is, although I like setMinimumTimerInterval too.

OK. I'll rename it to setMinimumTimerInterval as I like that name better too in the long term. Will upload an updated patch once this one clears the EWS bots (most concerned about GTK).

> Not to beat this horse to death, but I will add that I think the comment at the top of WebSettings.h should be removed.  If someone were to change WebCore::Settings in some material way (e.g., renaming it), they would not think to update WebSettings.h.  The point of the WebKit API is to provide stable insulation around WebCore, so WebCore'isms shouldn't leak to the public.

I'm happy to volunteer to remove the comment, but in a subsequent patch as that change is unrelated.

Still looking for an r+ on this so that we can proceed with the dependent Chromium changes.
Comment 44 Darin Fisher (:fishd, Google) 2011-02-15 13:55:14 PST
Comment on attachment 82481 [details]
GTK build fix

R=me
Comment 45 Kenneth Russell 2011-02-15 14:38:05 PST
Created attachment 82521 [details]
Final patch

Final version of patch to be committed, incorporating Chromium API naming change. GTK EWS bot seems stuck, so am going ahead with landing.
Comment 46 Kenneth Russell 2011-02-15 14:54:10 PST
Committed r78620: <http://trac.webkit.org/changeset/78620>
Comment 47 Kenneth Russell 2011-02-15 16:36:38 PST
Committed r78643: <http://trac.webkit.org/changeset/78643>
Comment 48 Kenneth Russell 2011-02-15 16:37:33 PST
(In reply to comment #47)
> Committed r78643: <http://trac.webkit.org/changeset/78643>

Note: this removed the whitespace-only change in Source/WebKit/win/Interfaces/WebKit.idl introduced in http://trac.webkit.org/changeset/78620 .
Comment 49 James Robinson 2011-02-24 11:53:52 PST
fast/dom/timer-increase-then-decrease-min-interval.html turns out to be really flaky, especially in debug builds.  See http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&group=%40ToT%20-%20chromium.org&tests=fast%2Fdom%2Ftimer-increase-then-decrease-min-interval.html.  Is there any way to test this functionality more reliably?