Expose Resume() in RenderProcessHost.

Expose Resume() in RenderProcessHost.

- The Resume() is used for purge+suspend.
- intent-to-implement-and-ship of background renderer's purge + suspend
  is
  https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/DK189tnM8l4
- one of the documents attached in the above intent is
  https://docs.google.com/document/d/1EgLimgxWK5DGhptnNVbEGSvVn6Q609ZJaBkLjEPRJvI/edit?usp=sharing
- The feature is not enabled by default because
  purge-and-suspend-time is 0.

BUG=635419
Committed: https://crrev.com/a079ac4eebb754e20ce651d4e593286c630ab368
Cr-Commit-Position: refs/heads/master@{#428266}

[tasak, 2016-10-26 06:25:40]

The CQ bit was checked by tasak@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-26 06:26:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tasak, 2016-10-26 06:31:29]

tasak@google.com changed reviewers:
+ avi@chromium.org, chrisha@chromium.org, haraken@chromium.org

[tasak, 2016-10-26 06:31:30]

Would you review this CL?

This CL is created from https://codereview.chromium.org/2387603003

[haraken, 2016-10-26 07:13:38]

LGTM

[commit-bot: I haz the power, 2016-10-26 08:35:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-26 08:35:27]

Dry run: This issue passed the CQ dry run.

[Avi (use Gerrit), 2016-10-26 14:10:55]

LGTM

You will need an IPC reviewer too.

[haraken, 2016-10-26 15:31:33]

Do we have a test for Suspend and Resume?

[tasak, 2016-10-27 03:24:33]

tasak@google.com changed reviewers:
+ dcheng@chromium.org

[tasak, 2016-10-27 03:24:34]

dcheng, would you review IPC part of this CL?

I'm trying to add ChildProcessMsg_Resume for Purge+Suspend.
The IPC message: ChildProcessMsg_PurgeAndSuspend (used for Suspend) was added in

https://codereview.chromium.org/1914143002

[dcheng, 2016-10-27 03:41:58]

https://codereview.chromium.org/2453713002/diff/1/content/renderer/render_thr...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/2453713002/diff/1/content/renderer/render_thr...
content/renderer/render_thread_impl.cc:1882: if (!is_renderer_suspended_)
Do we expect this conditional to be hit in practice? I would expect the browser
to have perfect knowledge of the state, or can it get out of sync?

[tasak, 2016-10-27 04:49:39]

The CQ bit was checked by tasak@google.com to run a CQ dry run

[tasak, 2016-10-27 04:49:40]

Thank you for review.

https://codereview.chromium.org/2453713002/diff/1/content/renderer/render_thr...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/2453713002/diff/1/content/renderer/render_thr...
content/renderer/render_thread_impl.cc:1882: if (!is_renderer_suspended_)
On 2016/10/27 03:41:58, dcheng wrote:
> Do we expect this conditional to be hit in practice? I would expect the
browser
> to have perfect knowledge of the state, or can it get out of sync?

Replaced the "if" with DCHECK.

TabManager will have the state, i.e. RUNNING, SUSPENDED, and RESUMED, and change
RenderThreadImpl's state by using IPC messages.

[tasak, 2016-10-27 04:57:43]

On 2016/10/26 15:31:33, haraken wrote:
> Do we have a test for Suspend and Resume?

I have no idea about how to test RenderThreadImpl::OnProcessResume and so on.

I looked at render_thread_impl_unittest and render_thread_impl_browsertest....
The tests don't check any OnProcessBackground,... and so on's behavior.

So is it ok to do something like:

RenderThreadImplUnitTest:
  render_thread_impl_->OnProcessSuspend();
  EXPECT_TRUE(render_thread_impl_->is_renderer_suspended);
  render_thread_impl_->OnProcessResume();
  EXPECT_TRUE(!render_thread_impl_->is_renderer_suspended);
  render_thread_impl_->OnProcessBackground(false);
  EXPECT_TRUE(!render_thread_impl_->is_renderer_suspended);
?

[dcheng, 2016-10-27 05:51:40]

ipc lgtm

https://codereview.chromium.org/2453713002/diff/20001/content/renderer/render...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/2453713002/diff/20001/content/renderer/render...
content/renderer/render_thread_impl.cc:1769: if (is_renderer_suspended_ ||
!RendererIsHidden())
(I wonder if this should also be a DCHECK for the is_renderer_suspended_ bit)

[tasak, 2016-10-27 08:52:03]

https://codereview.chromium.org/2453713002/diff/20001/content/renderer/render...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/2453713002/diff/20001/content/renderer/render...
content/renderer/render_thread_impl.cc:1769: if (is_renderer_suspended_ ||
!RendererIsHidden())
On 2016/10/27 05:51:40, dcheng wrote:
> (I wonder if this should also be a DCHECK for the is_renderer_suspended_ bit)

Acknowledged.
Currently TabManager sends PurgeAndSuspend message periodically (every
purge-and-suspend-time seconds)...
I will replace this with DCHECK(!is_renderer_suspended_) after committing
TabManager's patch.

[tasak, 2016-10-27 08:52:12]

The CQ bit was checked by tasak@google.com

[tasak, 2016-10-27 08:52:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, avi@chromium.org
Link to the patchset: https://codereview.chromium.org/2453713002/#ps20001
(title: "Use DCHECK.")

[commit-bot: I haz the power, 2016-10-27 08:52:33]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-27 10:00:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-27 10:00:36]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[tasak, 2016-10-28 01:50:16]

The CQ bit was checked by tasak@google.com

[commit-bot: I haz the power, 2016-10-28 01:50:41]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-28 03:11:51]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-10-28 03:15:02]

Description was changed from

==========
Expose Resume() in RenderProcessHost.

- The Resume() is used for purge+suspend.
- intent-to-implement-and-ship of background renderer's purge + suspend
  is
  https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/DK189tnM8l4
- one of the documents attached in the above intent is
 
https://docs.google.com/document/d/1EgLimgxWK5DGhptnNVbEGSvVn6Q609ZJaBkLjEPRJ...
- The feature is not enabled by default because
  purge-and-suspend-time is 0.

BUG=635419
==========

to

==========
Expose Resume() in RenderProcessHost.

- The Resume() is used for purge+suspend.
- intent-to-implement-and-ship of background renderer's purge + suspend
  is
  https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/DK189tnM8l4
- one of the documents attached in the above intent is
 
https://docs.google.com/document/d/1EgLimgxWK5DGhptnNVbEGSvVn6Q609ZJaBkLjEPRJ...
- The feature is not enabled by default because
  purge-and-suspend-time is 0.

BUG=635419

Committed: https://crrev.com/a079ac4eebb754e20ce651d4e593286c630ab368
Cr-Commit-Position: refs/heads/master@{#428266}
==========

[commit-bot: I haz the power, 2016-10-28 03:15:03]

Patchset 2 (id:??) landed as
https://crrev.com/a079ac4eebb754e20ce651d4e593286c630ab368
Cr-Commit-Position: refs/heads/master@{#428266}