Proposed new approach for SetBackgrounded.

content/browser/renderer_host/render_process_host_impl.cc

Index: content/browser/renderer_host/render_process_host_impl.cc
diff --git a/content/browser/renderer_host/render_process_host_impl.cc b/content/browser/renderer_host/render_process_host_impl.cc
index 248ed1d8b17158adeb873798ca038fb954fa0f68..85c9869fc87dd12ae3b656de9c07e67ebdf895b3 100644
--- a/content/browser/renderer_host/render_process_host_impl.cc
+++ b/content/browser/renderer_host/render_process_host_impl.cc
@@ -480,7 +480,7 @@ RenderProcessHostImpl::RenderProcessHostImpl(
       pending_views_(0),
       mojo_application_host_(new MojoApplicationHost),
       visible_widgets_(0),
-      backgrounded_(true),
+      is_process_backgrounded_(false),
       is_initialized_(false),
       id_(ChildProcessHostImpl::GenerateChildProcessUniqueId()),
       browser_context_(browser_context),
@@ -518,11 +518,6 @@ RenderProcessHostImpl::RenderProcessHostImpl(
   subscribe_uniform_enabled_ =
       base::CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kEnableSubscribeUniformExtension);
-
-  // Note: When we create the RenderProcessHostImpl, it's technically
-  //       backgrounded, because it has no visible listeners.  But the process
-  //       doesn't actually exist yet, so we'll Background it later, after
-  //       creation.
 }
 
 // static
@@ -1071,22 +1066,20 @@ void RenderProcessHostImpl::ShutdownForBadMessage() {
 
 void RenderProcessHostImpl::WidgetRestored() {
   // Verify we were properly backgrounded.
-  DCHECK_EQ(backgrounded_, (visible_widgets_ == 0));
   visible_widgets_++;
-  SetBackgrounded(false);
+  UpdateProcessPriority();
+  DCHECK(!is_process_backgrounded_);
 }
 
 void RenderProcessHostImpl::WidgetHidden() {
-  // On startup, the browser will call Hide
-  if (backgrounded_)
+  // On startup, the browser will call Hide.  We ignore these calls.
+  if (visible_widgets_ == 0)
     return;
 
-  DCHECK_EQ(backgrounded_, (visible_widgets_ == 0));
-  visible_widgets_--;
-  DCHECK_GE(visible_widgets_, 0);
-  if (visible_widgets_ == 0) {
-    DCHECK(!backgrounded_);
-    SetBackgrounded(true);
+  DCHECK_GE(visible_widgets_, 1);

[gab, 2015/07/08 18:50:56]

DCHECK_GT(visible_wigets, 0); 

slightly better IMO?

+  if (--visible_widgets_ == 0) {
+    DCHECK(!is_process_backgrounded_);
+    UpdateProcessPriority();
   }
 }
 
@@ -2116,6 +2109,8 @@ void RenderProcessHostImpl::ProcessDied(bool already_dead,
     delete queued_messages_.front();
     queued_messages_.pop();
   }
+  UpdateProcessPriority();
+  DCHECK(!is_process_backgrounded_);
 
   within_process_died_observer_ = true;
   NotificationService::current()->Notify(
@@ -2238,24 +2233,29 @@ void RenderProcessHostImpl::SuddenTerminationChanged(bool enabled) {
   SetSuddenTerminationAllowed(enabled);
 }
 
-void RenderProcessHostImpl::SetBackgrounded(bool backgrounded) {
-  TRACE_EVENT1("renderer_host", "RenderProcessHostImpl::SetBackgrounded",
-               "backgrounded", backgrounded);
-  // Note: we always set the backgrounded_ value.  If the process is NULL
-  // (and hence hasn't been created yet), we will set the process priority
-  // later when we create the process.
-  backgrounded_ = backgrounded;
-  if (!child_process_launcher_.get() || child_process_launcher_->IsStarting())
+void RenderProcessHostImpl::UpdateProcessPriority() {
+  base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();

[gab, 2015/07/08 18:50:56]

const

+  bool process_exists =
+      child_process_launcher_.get() && !child_process_launcher_->IsStarting();
+
+  // We background a process as soon as it hosts no active audio streams and no
+  // visible widgets -- the callers must call this function whenever we
+  // transition in/out of those states. Processes are initially launched with
+  // foreground priority, so when the process doesn't exist.
+  bool backgrounded =

[gab, 2015/07/08 18:50:56]

s/backgrounded/should_background/ ?

+      process_exists && visible_widgets_ == 0 &&
+      !audio_renderer_host_->HasActiveAudio() &&
+      !command_line->HasSwitch(switches::kDisableRendererBackgrounding);
+
+  if (is_process_backgrounded_ == backgrounded)
     return;

[gab, 2015/07/08 18:50:56]

I would just keep this early return, I find it easier to follow then molding it
into the boolean and then checking it independently for early return anyways
later.

[ncarter (slow), 2015/07/08 19:07:19]

Good point.

Though, it's important to actually set is_process_backgrounded_ to false in the
!child_process_launcher case (otherwise, I expect that when the process that was
backgrounded is re-created after crash, we might do the wrong thing). That's why
I structured it as I did. However, I think what we can do is keep the early
return but also assign the is_process_backgrounded_ = false.

 
-  // Don't background processes which have active audio streams.
-  if (backgrounded_ && audio_renderer_host_->HasActiveAudio())
-    return;
+  TRACE_EVENT1("renderer_host", "RenderProcessHostImpl::UpdateProcessPriority",

[gab, 2015/07/08 18:50:56]

I think the trace should remain at the top (it's not filled in UMA so early
returns don't affect it, having a TRACE_EVENT in the middle is unusual IMO).

[ncarter (slow), 2015/07/08 19:07:19]

Because we're now computing |backgrounded| in the function, it has to go after
that computation. My thinking was that skipping the trivial/early return cases
where we don't actually do any work would make the about:tracing results more
interesting.

+               "backgrounded", backgrounded);
+  is_process_backgrounded_ = backgrounded;
 
-  const base::CommandLine* command_line =
-      base::CommandLine::ForCurrentProcess();
-  if (command_line->HasSwitch(switches::kDisableRendererBackgrounding))
-    return;
+  if (!process_exists)
+    return;  // Nothing to background.
 
 #if defined(OS_WIN)
   // The cbstext.dll loads as a global GetMessage hook in the browser process
@@ -2311,7 +2311,8 @@ void RenderProcessHostImpl::OnProcessLaunched() {
         FROM_HERE_WITH_EXPLICIT_FUNCTION(
             "465841 RenderProcessHostImpl::OnProcessLaunched::Backgrounded"));
     DCHECK(child_process_launcher_->GetProcess().IsValid());
-    SetBackgrounded(backgrounded_);
+    DCHECK(!is_process_backgrounded_);
+    UpdateProcessPriority();
   }
 
   // TODO(erikchen): Remove ScopedTracker below once http://crbug.com/465841