Set the minimum timer interval on a per-page basis, and adjust it when...

Set the minimum timer interval on a per-page basis, and adjust it when
tabs are brought to the foreground and sent to the background.

This CL does not actually increase the background timer interval. That
will be done separately, so that it can easily be reverted without
removing all of the associated code.

BUG=66078
TEST=none (tested manually with minimal test case)


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=75404

[Ken Russell (switch to Gerrit), 2011-02-17 02:05:09]

Please review. Thanks.

[jamesr, 2011-02-17 02:13:32]

Why do we need to modify the WebPreferences object, IPC format and all that
stuff?  I was thinking we would set this directly from the renderer side (in the
hide/show callbacks), no browser side logic needed.

[Ken Russell (switch to Gerrit), 2011-02-17 02:22:48]

On 2011/02/17 02:13:32, jamesr wrote:
> Why do we need to modify the WebPreferences object, IPC format and all that
> stuff?  I was thinking we would set this directly from the renderer side (in
the
> hide/show callbacks), no browser side logic needed.

Darin asked me to initialize this value explicitly for every page, so that the
Chromium port can stop setting the default value up in WebKit. As far as I
understand, the place where the initial WebSettings are applied is in
webpreferences.{cc,h}, so this implies changing the associated IPC code.

[darin (slow to review), 2011-02-17 06:42:32]

http://codereview.chromium.org/6532012/diff/1/chrome/renderer/render_view.cc
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/6532012/diff/1/chrome/renderer/render_view.cc#...
chrome/renderer/render_view.cc:136: #include
"third_party/WebKit/Source/WebKit/chromium/public/WebKit.h"
this include seems unnecessary.  it looks like leftovers from the way you were
previously exposing the setter for changing the timer preference.

http://codereview.chromium.org/6532012/diff/1/webkit/glue/webkit_glue.h
File webkit/glue/webkit_glue.h (right):

http://codereview.chromium.org/6532012/diff/1/webkit/glue/webkit_glue.h#newco...
webkit/glue/webkit_glue.h:54: const double kForegroundTabTimerInterval = 0.004;
i'm tempted to pull these constants out of webkit_glue.h as webkit_glue.h is a
messy dumping ground.  adding more stuff to it seems unsavory to me.

how about src/webkit/glue/webkit_constants.h?

http://codereview.chromium.org/6532012/diff/1/webkit/glue/webpreferences.cc
File webkit/glue/webpreferences.cc (right):

http://codereview.chromium.org/6532012/diff/1/webkit/glue/webpreferences.cc#n...
webkit/glue/webpreferences.cc:202:
settings->setMinimumTimerInterval(minimum_timer_interval);
here, you can just pass webkit_glue::kForegroundTabTimerInterval

actually, a RenderView starts its life hidden.  maybe you should pass
kBackgroundTabTimerInterval here instead?

http://codereview.chromium.org/6532012/diff/1/webkit/glue/webpreferences.h
File webkit/glue/webpreferences.h (right):

http://codereview.chromium.org/6532012/diff/1/webkit/glue/webpreferences.h#ne...
webkit/glue/webpreferences.h:80: double minimum_timer_interval;
I don't see this field ever being used.  Since we don't need to pass this value
from the browser to the renderer, I would just leave this out.  I would still
modify the Apply method to assign the default value to the WebView's
WebSettings.

http://codereview.chromium.org/6532012/diff/1/webkit/support/webkit_support.h
File webkit/support/webkit_support.h (right):

http://codereview.chromium.org/6532012/diff/1/webkit/support/webkit_support.h...
webkit/support/webkit_support.h:47: const double ForegroundTabTimerInterval =
0.004;
maybe it would be better to make this a function call?  then, we could implement
it in terms of shared code, so that we do not need to worry about the values
getting out of sync.

[Ken Russell (switch to Gerrit), 2011-02-18 01:05:54]

http://codereview.chromium.org/6532012/diff/1/chrome/renderer/render_view.cc
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/6532012/diff/1/chrome/renderer/render_view.cc#...
chrome/renderer/render_view.cc:136: #include
"third_party/WebKit/Source/WebKit/chromium/public/WebKit.h"
On 2011/02/17 06:42:32, darin wrote:
> this include seems unnecessary.  it looks like leftovers from the way you were
> previously exposing the setter for changing the timer preference.

You're right; sorry about that. Removed.

http://codereview.chromium.org/6532012/diff/1/webkit/glue/webkit_glue.h
File webkit/glue/webkit_glue.h (right):

http://codereview.chromium.org/6532012/diff/1/webkit/glue/webkit_glue.h#newco...
webkit/glue/webkit_glue.h:54: const double kForegroundTabTimerInterval = 0.004;
On 2011/02/17 06:42:32, darin wrote:
> i'm tempted to pull these constants out of webkit_glue.h as webkit_glue.h is a
> messy dumping ground.  adding more stuff to it seems unsavory to me.
> 
> how about src/webkit/glue/webkit_constants.h?

Sure, good idea. Done.

http://codereview.chromium.org/6532012/diff/1/webkit/glue/webpreferences.cc
File webkit/glue/webpreferences.cc (right):

http://codereview.chromium.org/6532012/diff/1/webkit/glue/webpreferences.cc#n...
webkit/glue/webpreferences.cc:202:
settings->setMinimumTimerInterval(minimum_timer_interval);
On 2011/02/17 06:42:32, darin wrote:
> here, you can just pass webkit_glue::kForegroundTabTimerInterval
> 
> actually, a RenderView starts its life hidden.  maybe you should pass
> kBackgroundTabTimerInterval here instead?

Using kBackgroundTabTimerInterval seems to work well. Done.

http://codereview.chromium.org/6532012/diff/1/webkit/glue/webpreferences.h
File webkit/glue/webpreferences.h (right):

http://codereview.chromium.org/6532012/diff/1/webkit/glue/webpreferences.h#ne...
webkit/glue/webpreferences.h:80: double minimum_timer_interval;
On 2011/02/17 06:42:32, darin wrote:
> I don't see this field ever being used.  Since we don't need to pass this
value
> from the browser to the renderer, I would just leave this out.  I would still
> modify the Apply method to assign the default value to the WebView's
> WebSettings.

That sounds good and will address jamesr's concern as well. Done.

http://codereview.chromium.org/6532012/diff/1/webkit/support/webkit_support.h
File webkit/support/webkit_support.h (right):

http://codereview.chromium.org/6532012/diff/1/webkit/support/webkit_support.h...
webkit/support/webkit_support.h:47: const double ForegroundTabTimerInterval =
0.004;
On 2011/02/17 06:42:32, darin wrote:
> maybe it would be better to make this a function call?  then, we could
implement
> it in terms of shared code, so that we do not need to worry about the values
> getting out of sync.

Changed to "double GetForegroundTabTimerInterval()".

[darin (slow to review), 2011-02-18 05:15:52]

LGTM