Schedule idle handler in the foreground tab based on CPU usage and user activity.

Schedule idle handler in the foreground tab based on CPU usage and user activity.

This improves memory usage when the user leaves a tab open for a long time.

BUG=
TEST=


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

[ulan, 2011-11-16 14:14:45]

Please take a look. This CL is extracted from
http://codereview.chromium.org/8574012/ 

It schedules the idle handler in the foreground tab and addresses your comment
about user interaction.

http://codereview.chromium.org/8513008/diff/1/content/renderer/idle_user_dete...
File content/renderer/idle_user_detector.cc (right):

http://codereview.chromium.org/8513008/diff/1/content/renderer/idle_user_dete...
content/renderer/idle_user_detector.cc:30: RunIdleHandlerWhenWidgetsHidden()) {
I use "RunIdleHandlerWhenWidgetsHidden" as an indicator of whether the renderer
belongs to an extension or not.
Should I rename it to "IsExtension"?

http://codereview.chromium.org/8513008/diff/1/content/renderer/idle_user_dete...
File content/renderer/idle_user_detector.h (right):

http://codereview.chromium.org/8513008/diff/1/content/renderer/idle_user_dete...
content/renderer/idle_user_detector.h:13: class IdleUserDetector : public
content::RenderViewObserver {
Is overhead of having additional RenderViewObserver too large?

http://codereview.chromium.org/8513008/diff/1/content/renderer/render_thread_...
File content/renderer/render_thread_impl.cc (right):

http://codereview.chromium.org/8513008/diff/1/content/renderer/render_thread_...
content/renderer/render_thread_impl.cc:822: double cpu_usage =
process_metrics_->GetCPUUsage();
I didn't place GetCPUUsage inside the "if" statement below because I want to
sample CPU usage on each timer event, so that it is more accurate.

[Matt Perry, 2011-11-16 19:23:52]

http://codereview.chromium.org/8513008/diff/1/content/renderer/idle_user_dete...
File content/renderer/idle_user_detector.cc (right):

http://codereview.chromium.org/8513008/diff/1/content/renderer/idle_user_dete...
content/renderer/idle_user_detector.cc:30: RunIdleHandlerWhenWidgetsHidden()) {
On 2011/11/16 14:14:46, ulan wrote:
> I use "RunIdleHandlerWhenWidgetsHidden" as an indicator of whether the
renderer
> belongs to an extension or not.
> Should I rename it to "IsExtension"?

I think you can just call PostponeIdleNotification() for extensions as well. If
an extension receives an input event, that's an indication it is not idle.

http://codereview.chromium.org/8513008/diff/1/content/renderer/render_thread_...
File content/renderer/render_thread_impl.cc (right):

http://codereview.chromium.org/8513008/diff/1/content/renderer/render_thread_...
content/renderer/render_thread_impl.cc:577: bool is_extension =
!content::GetContentClient()->renderer()->
extensions are a chrome concept, and this code lives in content/, so you
shouldn't mention them here.

http://codereview.chromium.org/8513008/diff/1/content/renderer/render_thread_...
content/renderer/render_thread_impl.cc:814: void
RenderThreadImpl::IdleHandlerInForegroundTab() {
could you move this next to the other IdleHandler, so they are close to each
other?

http://codereview.chromium.org/8513008/diff/1/content/renderer/render_thread_...
content/renderer/render_thread_impl.cc:822: double cpu_usage =
process_metrics_->GetCPUUsage();
On 2011/11/16 14:14:46, ulan wrote:
> I didn't place GetCPUUsage inside the "if" statement below because I want to
> sample CPU usage on each timer event, so that it is more accurate.

Add this as a comment in the code, so someone reading it later will know it's
there for a reason.

[ulan, 2011-11-16 20:13:21]

Uploaded new patch set.

http://codereview.chromium.org/8513008/diff/1/content/renderer/idle_user_dete...
File content/renderer/idle_user_detector.cc (right):

http://codereview.chromium.org/8513008/diff/1/content/renderer/idle_user_dete...
content/renderer/idle_user_detector.cc:30: RunIdleHandlerWhenWidgetsHidden()) {
On 2011/11/16 19:23:52, Matt Perry wrote:
> On 2011/11/16 14:14:46, ulan wrote:
> > I use "RunIdleHandlerWhenWidgetsHidden" as an indicator of whether the
> renderer
> > belongs to an extension or not.
> > Should I rename it to "IsExtension"?
> 
> I think you can just call PostponeIdleNotification() for extensions as well.
If
> an extension receives an input event, that's an indication it is not idle.
I will do it in the second CL which will simplify idle handler scheduling for
background tabs and extensions.

http://codereview.chromium.org/8513008/diff/1/content/renderer/render_thread_...
File content/renderer/render_thread_impl.cc (right):

http://codereview.chromium.org/8513008/diff/1/content/renderer/render_thread_...
content/renderer/render_thread_impl.cc:577: bool is_extension =
!content::GetContentClient()->renderer()->
On 2011/11/16 19:23:52, Matt Perry wrote:
> extensions are a chrome concept, and this code lives in content/, so you
> shouldn't mention them here.

Done.

http://codereview.chromium.org/8513008/diff/1/content/renderer/render_thread_...
content/renderer/render_thread_impl.cc:814: void
RenderThreadImpl::IdleHandlerInForegroundTab() {
On 2011/11/16 19:23:52, Matt Perry wrote:
> could you move this next to the other IdleHandler, so they are close to each
> other?

Done.

http://codereview.chromium.org/8513008/diff/1/content/renderer/render_thread_...
content/renderer/render_thread_impl.cc:822: double cpu_usage =
process_metrics_->GetCPUUsage();
On 2011/11/16 19:23:52, Matt Perry wrote:
> On 2011/11/16 14:14:46, ulan wrote:
> > I didn't place GetCPUUsage inside the "if" statement below because I want to
> > sample CPU usage on each timer event, so that it is more accurate.
> 
> Add this as a comment in the code, so someone reading it later will know it's
> there for a reason.

Done.

[Matt Perry, 2011-11-16 21:25:23]

lgtm

[commit-bot: I haz the power, 2011-11-17 08:59:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ulan@chromium.org/8513008/6001

[commit-bot: I haz the power, 2011-11-17 08:59:51]

Presubmit check for 8513008-6001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
content/renderer/render_thread_impl.cc,content/renderer/render_thread_impl.h,content/public/renderer/render_thread.h,content/renderer/render_view_impl.cc,content/renderer/idle_user_detector.h,content/renderer/idle_user_detector.cc

Presubmit checks took 1.3s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[ulan, 2011-11-17 09:07:42]

John, could you please take a look too?

[jam, 2011-11-23 17:35:19]

http://codereview.chromium.org/8513008/diff/6001/content/public/renderer/rend...
File content/public/renderer/render_thread.h (right):

http://codereview.chromium.org/8513008/diff/6001/content/public/renderer/rend...
content/public/renderer/render_thread.h:102: virtual void
PostponeIdleNotification() = 0;
we only add stuff to the public API that's needed by embedders (i.e. chrome). in
this case this function is only used by content. so can you just add this to
RenderThreadImpl? the code that calls this can use RenderThreadImpl::current()
to get to it (should probably null check since it'll be null in tests).

[ulan, 2011-11-23 18:39:15]

Thanks for the suggestion! Upload new patch set.

http://codereview.chromium.org/8513008/diff/6001/content/public/renderer/rend...
File content/public/renderer/render_thread.h (right):

http://codereview.chromium.org/8513008/diff/6001/content/public/renderer/rend...
content/public/renderer/render_thread.h:102: virtual void
PostponeIdleNotification() = 0;
On 2011/11/23 17:35:19, John Abd-El-Malek wrote:
> we only add stuff to the public API that's needed by embedders (i.e. chrome).
in
> this case this function is only used by content. so can you just add this to
> RenderThreadImpl? the code that calls this can use RenderThreadImpl::current()
> to get to it (should probably null check since it'll be null in tests).

Done.

[jam, 2011-11-23 18:42:42]

lgtm

http://codereview.chromium.org/8513008/diff/13001/content/renderer/idle_user_...
File content/renderer/idle_user_detector.cc (right):

http://codereview.chromium.org/8513008/diff/13001/content/renderer/idle_user_...
content/renderer/idle_user_detector.cc:12: using content::RenderThread;
nit: not needed

[commit-bot: I haz the power, 2011-11-24 09:45:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ulan@chromium.org/8513008/15002

[commit-bot: I haz the power, 2011-11-24 10:53:50]

Change committed as 111520