Proposed new approach for SetBackgrounded.

Proposed new approach for SetBackgrounded.

BUG=

[gab, 2015-07-08 18:50:55]

gab@chromium.org changed reviewers:
+ gab@chromium.org

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

Couple drive-by comments in case this is re-used as-is for
https://codereview.chromium.org/1214883004

https://codereview.chromium.org/1214393004/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.cc (left):

https://codereview.chromium.org/1214393004/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:2249: return;
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.

https://codereview.chromium.org/1214393004/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/1214393004/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:1079:
DCHECK_GE(visible_widgets_, 1);
DCHECK_GT(visible_wigets, 0); 

slightly better IMO?

https://codereview.chromium.org/1214393004/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:2237:
base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
const

https://codereview.chromium.org/1214393004/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:2245: bool
backgrounded =
s/backgrounded/should_background/ ?

https://codereview.chromium.org/1214393004/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:2253:
TRACE_EVENT1("renderer_host", "RenderProcessHostImpl::UpdateProcessPriority",
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]

Feel free to take over this CL or merge it into yours. I am stability sheriff
for the next two days and won't have any time to work on it.

https://codereview.chromium.org/1214393004/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.cc (left):

https://codereview.chromium.org/1214393004/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:2249: return;
On 2015/07/08 18:50:56, gab wrote:
> 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.

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.

https://codereview.chromium.org/1214393004/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/1214393004/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:2253:
TRACE_EVENT1("renderer_host", "RenderProcessHostImpl::UpdateProcessPriority",
On 2015/07/08 18:50:56, gab wrote:
> 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).

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.

[gab, 2015-08-18 01:42:37]

Closing this CL as it was integrated in
https://codereview.chromium.org/1214883004/

Thanks,
Gab

[ncarter (slow), 2015-08-24 17:20:24]

Thanks for closing.