Allow controlling minimum DOMTimer interval on a per-page basis

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Kenneth Russell]

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.

[Kenneth Russell]

Created attachment 82192 [details]
Patch

[Kenneth Russell]

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.

[WebKit Review Bot]

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.

[James Robinson]

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.

[Collabora GTK+ EWS bot]

Attachment 82192 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7868739

[Kenneth Russell]

(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).

[Martin Robinson]

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!

[Kenneth Russell]

Created attachment 82204 [details]
Fixed GTK build failure

[WebKit Review Bot]

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.

[Adam Roben (:aroben)]

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.

[Simon Fraser (smfr)]

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.

[Kenneth Russell]

(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.

[Kenneth Russell]

(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?

[Kenneth Russell]

(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.

[Build Bot]

Attachment 82204 [details] did not build on win:
Build output: http://queues.webkit.org/results/7874811

[Build Bot]

Attachment 82204 [details] did not build on win:
Build output: http://queues.webkit.org/results/7874829

[Early Warning System Bot]

Attachment 82204 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7874841

[Collabora GTK+ EWS bot]

Attachment 82204 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7874842

[Kenneth Russell]

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.

[WebKit Review Bot]

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.

[Darin Fisher (:fishd, Google)]

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"?

[Early Warning System Bot]

Attachment 82372 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7913648

[Kenneth Russell]

(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.

[Build Bot]

Attachment 82372 [details] did not build on win:
Build output: http://queues.webkit.org/results/7908736

[Kenneth Russell]

Created attachment 82391 [details]
Fixed qt and win build failures

[WebKit Review Bot]

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.

[Build Bot]

Attachment 82372 [details] did not build on win:
Build output: http://queues.webkit.org/results/7912651

[Build Bot]

Attachment 82391 [details] did not build on win:
Build output: http://queues.webkit.org/results/7908768

[Kenneth Russell]

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.

[WebKit Review Bot]

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.

[Kenneth Russell]

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.

[WebKit Review Bot]

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.

[Kenneth Russell]

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.

[WebKit Review Bot]

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.

[Build Bot]

Attachment 82404 [details] did not build on win:
Build output: http://queues.webkit.org/results/7907770

[Build Bot]

Attachment 82405 [details] did not build on win:
Build output: http://queues.webkit.org/results/7905821

[Collabora GTK+ EWS bot]

Attachment 82406 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7907848

[Kenneth Russell]

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.

[WebKit Review Bot]

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.

[Darin Fisher (:fishd, Google)]

> > 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.

[Kenneth Russell]

(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.

[Darin Fisher (:fishd, Google)]

(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.

[Kenneth Russell]

(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.

[Darin Fisher (:fishd, Google)]

Comment on attachment 82481 [details]
GTK build fix

R=me

[Kenneth Russell]

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.

[Kenneth Russell]

Committed r78620: <http://trac.webkit.org/changeset/78620>

[Kenneth Russell]

Committed r78643: <http://trac.webkit.org/changeset/78643>

[Kenneth Russell]

(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 .

[James Robinson]

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?