Send RenderViewReady when a RVH is reused and becomes active.

Send RenderViewReady when a RVH is reused and becomes active.

Normally, RenderViewHostDelegate::RenderViewReady is dispatched
whenever a RenderView is created from RVHI::CreateRenderView (as soon
as its process is ready).  However, if we reused a RenderViewHost
which was previously swapped out for a new navigation, which made the
RenderViewHost active, the RenderViewReady was not dispatched.  One of
the consumers of the corresponding
WebContentsObserver::RenderViewReady method was the SadTabHelper.
This meant that if a tab was showing a sad tab, and a subsequent
navigation reused a RVH bringing it into an active state, the sad tab
was not hidden.  To fix this, this CL dispatches RenderViewReady in
that case.

BUG=591984
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/61e1fcd02e595f2e882f9e702f69f3363ca9ae6b
Cr-Commit-Position: refs/heads/master@{#411436}

[alexmos, 2016-08-11 17:29:33]

Description was changed from

==========
Send RenderViewReady when a RVH is reused and becomes active.

Normally, RenderViewHostDelegate::RenderViewReady is dispatched
whenever a RenderView is created from RVHI::CreateRenderView (as soon
as its process is ready).  However, if we reused a RenderViewHost
which was previously swapped out for a new navigation, which made the
RenderViewHost active, the RenderViewReady was not dispatched.  One of
the consumers of the corresponding
WebContentsObserver::RenderViewReady method was the SadTabHelper.
This meant that if a tab was showing a sad tab, and a subsequent
navigation reused a RVH bringing it into an active state, the sad tab
was not hidden.  To fix this, this CL dispatches RenderViewReady in
that case.

BUG=591984
==========

to

==========
Send RenderViewReady when a RVH is reused and becomes active.

Normally, RenderViewHostDelegate::RenderViewReady is dispatched
whenever a RenderView is created from RVHI::CreateRenderView (as soon
as its process is ready).  However, if we reused a RenderViewHost
which was previously swapped out for a new navigation, which made the
RenderViewHost active, the RenderViewReady was not dispatched.  One of
the consumers of the corresponding
WebContentsObserver::RenderViewReady method was the SadTabHelper.
This meant that if a tab was showing a sad tab, and a subsequent
navigation reused a RVH bringing it into an active state, the sad tab
was not hidden.  To fix this, this CL dispatches RenderViewReady in
that case.

BUG=591984
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[alexmos, 2016-08-11 17:29:35]

The CQ bit was checked by alexmos@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-11 17:29:57]

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

[alexmos, 2016-08-11 18:06:56]

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

[alexmos, 2016-08-11 18:06:56]

Charlie, can you please take a look?

This is the short-term fix.  Nasko and I were chatting about moving SadTabHelper
to use main frame's RenderFrameCreated instead, which should be pretty easy to
do, but I'm worried about the 7 other consumers of WCO::RenderViewReady still
being broken in this scenario.  So I think it might make sense to make a fix
like this to take care of all those places, then gradually move them away from
RenderViewReady.  I can do that for SadTabHelper in a followup.

[commit-bot: I haz the power, 2016-08-11 18:25:05]

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

[commit-bot: I haz the power, 2016-08-11 18:25:06]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Charlie Reis, 2016-08-11 18:55:53]

LGTM!

On 2016/08/11 18:06:56, alexmos wrote:
> Charlie, can you please take a look?
> 
> This is the short-term fix.  Nasko and I were chatting about moving
SadTabHelper
> to use main frame's RenderFrameCreated instead, which should be pretty easy to
> do, but I'm worried about the 7 other consumers of WCO::RenderViewReady still
> being broken in this scenario.  So I think it might make sense to make a fix
> like this to take care of all those places, then gradually move them away from
> RenderViewReady.  I can do that for SadTabHelper in a followup.

Yeah, that strategy makes sense.  The followup to remove RenderViewReady will be
great, though.  Thanks!

https://codereview.chromium.org/2242493002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_manager_browsertest.cc
(right):

https://codereview.chromium.org/2242493002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager_browsertest.cc:2862:
base::TERMINATION_STATUS_PROCESS_WAS_KILLED);
nit: Reverse order (expected, actual)

Also, this seems to be failing on Windows.  (Or maybe the one below?)

[alexmos, 2016-08-11 21:34:30]

Thanks!

https://codereview.chromium.org/2242493002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_manager_browsertest.cc
(right):

https://codereview.chromium.org/2242493002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager_browsertest.cc:2862:
base::TERMINATION_STATUS_PROCESS_WAS_KILLED);
On 2016/08/11 18:55:52, Charlie Reis wrote:
> nit: Reverse order (expected, actual)
> 
> Also, this seems to be failing on Windows.  (Or maybe the one below?)

Done.  Apparently on Windows, GetCrashedStatus() returns
TERMINATION_STATUS_NORMAL_TERMINATION instead of 
TERMINATION_STATUS_PROCESS_WAS_KILLED here.  I fixed it by comparing that the
status is not RUNNING.

[alexmos, 2016-08-11 21:34:38]

The CQ bit was checked by alexmos@chromium.org

[alexmos, 2016-08-11 21:34:38]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org
Link to the patchset: https://codereview.chromium.org/2242493002/#ps20001
(title: "Charlie's nit and Windows test fix")

[commit-bot: I haz the power, 2016-08-11 21:35:43]

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-08-11 23:29:14]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-08-11 23:32:04]

Description was changed from

==========
Send RenderViewReady when a RVH is reused and becomes active.

Normally, RenderViewHostDelegate::RenderViewReady is dispatched
whenever a RenderView is created from RVHI::CreateRenderView (as soon
as its process is ready).  However, if we reused a RenderViewHost
which was previously swapped out for a new navigation, which made the
RenderViewHost active, the RenderViewReady was not dispatched.  One of
the consumers of the corresponding
WebContentsObserver::RenderViewReady method was the SadTabHelper.
This meant that if a tab was showing a sad tab, and a subsequent
navigation reused a RVH bringing it into an active state, the sad tab
was not hidden.  To fix this, this CL dispatches RenderViewReady in
that case.

BUG=591984
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Send RenderViewReady when a RVH is reused and becomes active.

Normally, RenderViewHostDelegate::RenderViewReady is dispatched
whenever a RenderView is created from RVHI::CreateRenderView (as soon
as its process is ready).  However, if we reused a RenderViewHost
which was previously swapped out for a new navigation, which made the
RenderViewHost active, the RenderViewReady was not dispatched.  One of
the consumers of the corresponding
WebContentsObserver::RenderViewReady method was the SadTabHelper.
This meant that if a tab was showing a sad tab, and a subsequent
navigation reused a RVH bringing it into an active state, the sad tab
was not hidden.  To fix this, this CL dispatches RenderViewReady in
that case.

BUG=591984
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/61e1fcd02e595f2e882f9e702f69f3363ca9ae6b
Cr-Commit-Position: refs/heads/master@{#411436}
==========

[commit-bot: I haz the power, 2016-08-11 23:32:05]

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