Hook up V8 idle notifications for extension prcoesses.

Hook up V8 idle notifications for extension prcoesses.

We reset the idle timer after any activity, which includes an extension event
or message, or a new extension view being created. The timer runs after 10
seconds of idleness.

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

[Mike Belshe, 2009-12-04 22:37:50]

http://codereview.chromium.org/460062/diff/2001/2002
File chrome/renderer/render_thread.cc (right):

http://codereview.chromium.org/460062/diff/2001/2002#newcode96
chrome/renderer/render_thread.cc:96: static const int64
kMaxExtensionIdleHandlerDelayS = 10*60 /* seconds */;
nit:  spacing "10 * 60"
60 seconds seems pretty long?

http://codereview.chromium.org/460062/diff/2001/2002#newcode151
chrome/renderer/render_thread.cc:151: idle_notification_delay_in_s_ =
is_extension_process_ ?
I think we can get rid of this line?  (In fact, I think we can delete the member
variable - make IdleHandler() take an argument, and then we no longer need the
member variable.  Before, when there was only one type of timer, it wasn't too
confusing.  But now it is kind of confusing because of all the different
settings.


Both extension processes & other will intialize idle_notification_delay_in_s_ by
way of calling ScheduleIdleHandler()?

http://codereview.chromium.org/460062/diff/2001/2002#newcode264
chrome/renderer/render_thread.cc:264: 
DCHECK(is_extension_process()) ?

http://codereview.chromium.org/460062/diff/2001/2002#newcode579
chrome/renderer/render_thread.cc:579: void RenderThread::IdleHandler() {
so make this function take a current_delay argument...

[Matt Perry, 2009-12-04 23:10:56]

http://codereview.chromium.org/460062/diff/2001/2002
File chrome/renderer/render_thread.cc (right):

http://codereview.chromium.org/460062/diff/2001/2002#newcode96
chrome/renderer/render_thread.cc:96: static const int64
kMaxExtensionIdleHandlerDelayS = 10*60 /* seconds */;
On 2009/12/04 22:37:50, Mike Belshe wrote:
> nit:  spacing "10 * 60"
> 60 seconds seems pretty long?

It's actually 10 minutes. This controls when we call the idle handler even if
we're not idle - like if the extension stays busy for a long period of time. Ten
minutes does seem long still. I dropped it to 2 min.

http://codereview.chromium.org/460062/diff/2001/2002#newcode151
chrome/renderer/render_thread.cc:151: idle_notification_delay_in_s_ =
is_extension_process_ ?
On 2009/12/04 22:37:50, Mike Belshe wrote:
> I think we can get rid of this line?  (In fact, I think we can delete the
member
> variable - make IdleHandler() take an argument, and then we no longer need the
> member variable.  Before, when there was only one type of timer, it wasn't too
> confusing.  But now it is kind of confusing because of all the different
> settings.
> 
> 
> Both extension processes & other will intialize idle_notification_delay_in_s_
by
> way of calling ScheduleIdleHandler()?

Unfortunately the Timer code only works on methods with no parameters. We still
need the member var to do the delay dampening in IdleHandler.

As for this line, it ensures this variable is set in case the force_idle_timer_
fires before we schedule the first idle timer.

http://codereview.chromium.org/460062/diff/2001/2002#newcode264
chrome/renderer/render_thread.cc:264: 
On 2009/12/04 22:37:50, Mike Belshe wrote:
> DCHECK(is_extension_process()) ?
> 

Done.

[Erik does not do reviews, 2009-12-05 15:09:47]

LGTM

I'm not sure about the delays you chose, but I'm not sure how to suggest we pick
them better.  My instinct would be to shrink the min value from 10 seconds to
more like 2 or 3 seconds and to increase the max value back to 5 or 10 minutes.

One other random TODO you may want to consider for a future CL.  For normal
renderers, the damping effect keeps going, so when you have a tab that's been in
the background for a really long time, the gap has grown large as well.  In our
case, it never gets longer than the max timeout, even if the extension really
hasn't done anything in forever.  Maybe this is OK and V8 has a fast path when
it hasn't done anything since the last time.

[Erik does not do reviews, 2009-12-07 20:33:41]

LGTM