[DevTools] Unpause renderer when paused RenderFrame is closed.

[DevTools] Unpause renderer when paused RenderFrame is closed.

BUG=469421
Committed: https://crrev.com/cf9039cd9c0f1515fce14d2e2c5a8886bd1aa131
Cr-Commit-Position: refs/heads/master@{#323902}

[dgozman, 2015-03-30 17:04:36]

dgozman@chromium.org changed reviewers:
+ creis@chromium.org, kenrb@chromium.org

[dgozman, 2015-03-30 17:04:36]

Hi,

https://codereview.chromium.org/1047653003/diff/1/content/renderer/devtools/d...
File content/renderer/devtools/devtools_agent.cc (right):

https://codereview.chromium.org/1047653003/diff/1/content/renderer/devtools/d...
content/renderer/devtools/devtools_agent.cc:99: message.type() ==
ViewMsg_Close::ID)
This message is no longer catch in RenderFrameObserver.

[dgozman, 2015-03-30 17:07:30]

Could you please take a look at this?

We no longer catch ViewMsg_Close IPC because of RenderFrameObserver. I don't
like |WidgetWillClose| observer method, so I ask for advice here.

I don't think we can use FrameWillClose here (also included in CL), since it's
called once blink side is almost down, but |ContinueProgram| does heavy work in
blink/V8. Things may go wrong in this case.

Thanks,
Dmitry

[Charlie Reis, 2015-03-30 18:37:17]

creis@chromium.org changed reviewers:
+ nasko@chromium.org

[Charlie Reis, 2015-03-30 18:37:17]

[+nasko]

I think there's a high level question here about where widget level observer
events should live.  I think it's probably reasonable for the time being to put
them on RenderFrameObserver since RenderViewObserver is going away.

Ken, do you see any issues here?

https://codereview.chromium.org/1047653003/diff/1/content/renderer/devtools/d...
File content/renderer/devtools/devtools_agent.cc (right):

https://codereview.chromium.org/1047653003/diff/1/content/renderer/devtools/d...
content/renderer/devtools/devtools_agent.cc:105: void
DevToolsAgent::FrameWillClose() {
Why do you need both?  In your comment you mentioned this one might not work.

[dgozman, 2015-03-30 18:43:33]

https://codereview.chromium.org/1047653003/diff/1/content/renderer/devtools/d...
File content/renderer/devtools/devtools_agent.cc (right):

https://codereview.chromium.org/1047653003/diff/1/content/renderer/devtools/d...
content/renderer/devtools/devtools_agent.cc:105: void
DevToolsAgent::FrameWillClose() {
On 2015/03/30 18:37:17, Charlie Reis wrote:
> Why do you need both?  In your comment you mentioned this one might not work.

This one is here just to show what I was talking about. I will remove either one
or another.

Or maybe we should have another notification about "frame will close", but
earlier in pipeline (e.g. when IPC comes)? Because DevToolsAgent does not really
care that widget closes, ViewMsg_CLose just happens to be at the right time.

[dgozman, 2015-03-31 17:12:38]

dgozman@chromium.org changed reviewers:
+ pfeldman@chromium.org

[dgozman, 2015-03-31 17:12:39]

Please take a look.

[Charlie Reis, 2015-03-31 20:00:58]

Please include a test for this, but otherwise LGTM.

[kenrb, 2015-03-31 20:53:53]

lgtm from me also.

[dgozman, 2015-04-01 13:28:44]

Added test. Turned out to be a tricky one. Thank you for review!

[dgozman, 2015-04-01 13:28:48]

The CQ bit was checked by dgozman@chromium.org

[dgozman, 2015-04-01 13:28:49]

The patchset sent to the CQ was uploaded after l-g-t-m from kenrb@chromium.org,
creis@chromium.org
Link to the patchset: https://codereview.chromium.org/1047653003/#ps40001
(title: "Added test")

[commit-bot: I haz the power, 2015-04-01 13:28:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1047653003/40001

[commit-bot: I haz the power, 2015-04-01 14:19:19]

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

[commit-bot: I haz the power, 2015-04-01 14:19:20]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dgozman, 2015-04-01 15:51:39]

The CQ bit was checked by dgozman@chromium.org

[dgozman, 2015-04-01 15:51:40]

The patchset sent to the CQ was uploaded after l-g-t-m from kenrb@chromium.org,
creis@chromium.org
Link to the patchset: https://codereview.chromium.org/1047653003/#ps60001
(title: "CONTENT_EXPORT")

[commit-bot: I haz the power, 2015-04-01 15:52:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1047653003/60001

[commit-bot: I haz the power, 2015-04-01 16:42:44]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-04-01 16:44:34]

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

[mfomitchev1, 2015-04-01 20:20:48]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/1053833002/ by mfomitchev@google.com.

The reason for reverting is: Broke Linux ASan LSan Tests (1):

https://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20T....

[dgozman, 2015-04-06 11:07:45]

Relanding after blink-side fix: https://codereview.chromium.org/1058653004/.

[dgozman, 2015-04-06 11:07:50]

The CQ bit was checked by dgozman@chromium.org

[dgozman, 2015-04-06 11:07:51]

The patchset sent to the CQ was uploaded after l-g-t-m from kenrb@chromium.org,
creis@chromium.org
Link to the patchset: https://codereview.chromium.org/1047653003/#ps80001
(title: "rebased")

[commit-bot: I haz the power, 2015-04-06 11:08:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1047653003/80001

[commit-bot: I haz the power, 2015-04-06 12:01:45]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-04-06 12:02:37]

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