Move RenderViewHost swap out state to RenderFrameHost.

Move RenderViewHost swap out state to RenderFrameHost.

Also tracks the number of active frames in a SiteInstance, rather than
the number of views.  These changes are necessary for tracking the
pending swap-out state of frames.

This involves a lot of test cleanup as well.

BUG=418264
TEST=Subframes can swap out.  (No visible behavior change.)

Committed: https://crrev.com/cce56cd951f6685a0120db63418aa7e6d3df28f2
Cr-Commit-Position: refs/heads/master@{#297293}

[Charlie Reis, 2014-09-29 16:50:24]

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

[Charlie Reis, 2014-09-29 16:52:57]

Nasko: I think this is finally ready for review!  It was tricky to move some of
the state, but I think it's working now.  Comments below on some of the more
interesting / less mechanical changes.

https://codereview.chromium.org/606113005/diff/100001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/606113005/diff/100001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1423: // If this is a
subframe, it should have a CrossProcessFrameConnector
In its old location, this block wasn't updating the the proxy's view for a
second cross-process navigation in an OOPIF.  We never noticed because the RVH
state check on line 1380 was broken.

I'll clean all of this up in my in-progress CommitPending CL, but this was
needed to get the CrossSiteIframe test to pass again.

https://codereview.chromium.org/606113005/diff/100001/content/browser/frame_h...
File content/browser/frame_host/render_frame_proxy_host.cc (right):

https://codereview.chromium.org/606113005/diff/100001/content/browser/frame_h...
content/browser/frame_host/render_frame_proxy_host.cc:92:
render_frame_host_->set_render_frame_proxy_host(this);
The proxy host wasn't getting set on main frames that were created in the
swapped out state, so I added this call here to match the PassOwnership case.

https://codereview.chromium.org/606113005/diff/100001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_delegate.h (right):

https://codereview.chromium.org/606113005/diff/100001/content/browser/rendere...
content/browser/renderer_host/render_view_host_delegate.h:158: virtual void
RendererUnresponsive(RenderViewHost* render_view_host) {}
There's no need to pass the extra parameters, since WebContentsImpl can check
them on the RFH when they're needed.

https://codereview.chromium.org/606113005/diff/100001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_impl.cc (left):

https://codereview.chromium.org/606113005/diff/100001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.cc:500: if (timed_out) {
All this is basically a no-op leftover from 104346 investigations, so I've
removed it.

The rest of the method has moved to RFH.

https://codereview.chromium.org/606113005/diff/100001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_impl.h (left):

https://codereview.chromium.org/606113005/diff/100001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.h:99:
STATE_WAITING_FOR_CLOSE,
"Waiting for close" is now kept outside the state machine in RVH.  We can put it
back in the state machine when we move Close (http://crbug.com/418265).

https://codereview.chromium.org/606113005/diff/100001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_impl.h (right):

https://codereview.chromium.org/606113005/diff/100001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.h:162: virtual
SiteInstanceImpl* GetSiteInstance() const OVERRIDE;
Covariant return type for convenience.  (Cleans up a ton of unnecessary casts in
the unit tests.)

https://codereview.chromium.org/606113005/diff/100001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.h:221: is_swapped_out_ =
is_swapped_out;
I was hoping to get by with is_active_, but that doesn't work in
OnMessageReceived (which needs to process things in the pending swap out state
but not the swapped out state).  RVH doesn't have access to RFH, so we have to
store this here for now.

I'm at least not exposing the value to others.

[nasko, 2014-09-29 20:28:10]

This is awesome! I can't wait for RVH to be completely eliminated from
navigations and we are getting close.
I have mostly nits and a few clarifying questions.

https://codereview.chromium.org/606113005/diff/100001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/606113005/diff/100001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:336: // TODO(nasko): Only
check RenderViewHost state, as this object's own state
Shouldn't this TODO be removed? We don't check RVH state here anymore.

https://codereview.chromium.org/606113005/diff/100001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1197: // Whenever we change
the RFH state to and from live or swapped out state, we
nit: you changed "live" to "active" in the first comment, should it change here
as well?

https://codereview.chromium.org/606113005/diff/100001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1199: // to be safe, since
they can cause navigations to be ignored in OnNavigate.
nit: s/OnNavigate/OnDidCommitProvisionalLoad/

https://codereview.chromium.org/606113005/diff/100001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/606113005/diff/100001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:232: // Whether this
RenderFrameHost has been swapped out to be displayed by a
nit: I'm not quite sure I understand the comment. The "to be displayed by a
different process" is probably why. In swapped out state, RFH isn't displayed at
all. The frame represented by FTN is displayed by a different RFH and process.

https://codereview.chromium.org/606113005/diff/100001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/606113005/diff/100001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1370: // TODO(creis):
Swap out the subframe in --site-per-process.
Does this TODO still apply? It is a bit generic and in --site-per-process we are
using proxies.

https://codereview.chromium.org/606113005/diff/100001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1423: // If this is a
subframe, it should have a CrossProcessFrameConnector
On 2014/09/29 16:52:57, Charlie Reis wrote:
> In its old location, this block wasn't updating the the proxy's view for a
> second cross-process navigation in an OOPIF.  We never noticed because the RVH
> state check on line 1380 was broken.
> 
> I'll clean all of this up in my in-progress CommitPending CL, but this was
> needed to get the CrossSiteIframe test to pass again.

Good catch! It looks like CrossSiteIframe is indeed valuable test : ).

https://codereview.chromium.org/606113005/diff/100001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager_unittest.cc (right):

https://codereview.chromium.org/606113005/diff/100001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_unittest.cc:626:
TEST_F(RenderFrameHostManagerTest, ActiveFrameCountWhileSwappingInandOut) {
nit: s/InandOut/InAndOut/

https://codereview.chromium.org/606113005/diff/100001/content/browser/frame_h...
File content/browser/frame_host/render_frame_proxy_host.cc (right):

https://codereview.chromium.org/606113005/diff/100001/content/browser/frame_h...
content/browser/frame_host/render_frame_proxy_host.cc:92:
render_frame_host_->set_render_frame_proxy_host(this);
On 2014/09/29 16:52:57, Charlie Reis wrote:
> The proxy host wasn't getting set on main frames that were created in the
> swapped out state, so I added this call here to match the PassOwnership case.

Thanks!

https://codereview.chromium.org/606113005/diff/100001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_impl.h (right):

https://codereview.chromium.org/606113005/diff/100001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.h:162: virtual
SiteInstanceImpl* GetSiteInstance() const OVERRIDE;
On 2014/09/29 16:52:57, Charlie Reis wrote:
> Covariant return type for convenience.  (Cleans up a ton of unnecessary casts
in
> the unit tests.)

Acknowledged.

https://codereview.chromium.org/606113005/diff/100001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.h:221: is_swapped_out_ =
is_swapped_out;
On 2014/09/29 16:52:57, Charlie Reis wrote:
> I was hoping to get by with is_active_, but that doesn't work in
> OnMessageReceived (which needs to process things in the pending swap out state
> but not the swapped out state).  RVH doesn't have access to RFH, so we have to
> store this here for now.
> 
> I'm at least not exposing the value to others.

Acknowledged.

https://codereview.chromium.org/606113005/diff/100001/content/public/test/tes...
File content/public/test/test_renderer_host.h (right):

https://codereview.chromium.org/606113005/diff/100001/content/public/test/tes...
content/public/test/test_renderer_host.h:57: // Calls OnMsgNavigate on the
RenderFrameHost with the given information,
nit: s/OnMsgNavigate/OnDidCommitProvisionalLoad/

[Charlie Reis, 2014-09-29 20:49:21]

Thanks!  New patch uploaded.

https://codereview.chromium.org/606113005/diff/100001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/606113005/diff/100001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:336: // TODO(nasko): Only
check RenderViewHost state, as this object's own state
On 2014/09/29 20:28:09, nasko wrote:
> Shouldn't this TODO be removed? We don't check RVH state here anymore.

Yep!

https://codereview.chromium.org/606113005/diff/100001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1197: // Whenever we change
the RFH state to and from live or swapped out state, we
On 2014/09/29 20:28:09, nasko wrote:
> nit: you changed "live" to "active" in the first comment, should it change
here
> as well?

Done.

https://codereview.chromium.org/606113005/diff/100001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1199: // to be safe, since
they can cause navigations to be ignored in OnNavigate.
On 2014/09/29 20:28:09, nasko wrote:
> nit: s/OnNavigate/OnDidCommitProvisionalLoad/

Done.

https://codereview.chromium.org/606113005/diff/100001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/606113005/diff/100001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:232: // Whether this
RenderFrameHost has been swapped out to be displayed by a
On 2014/09/29 20:28:09, nasko wrote:
> nit: I'm not quite sure I understand the comment. The "to be displayed by a
> different process" is probably why. In swapped out state, RFH isn't displayed
at
> all. The frame represented by FTN is displayed by a different RFH and process.

Oops, I agree.  (I was just moving the old comment from RVH.)  Updated.

https://codereview.chromium.org/606113005/diff/100001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/606113005/diff/100001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1370: // TODO(creis):
Swap out the subframe in --site-per-process.
On 2014/09/29 20:28:09, nasko wrote:
> Does this TODO still apply? It is a bit generic and in --site-per-process we
are
> using proxies.

Yeah, sounds stale.  Removed.

https://codereview.chromium.org/606113005/diff/100001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager_unittest.cc (right):

https://codereview.chromium.org/606113005/diff/100001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_unittest.cc:626:
TEST_F(RenderFrameHostManagerTest, ActiveFrameCountWhileSwappingInandOut) {
On 2014/09/29 20:28:09, nasko wrote:
> nit: s/InandOut/InAndOut/

Done.

https://codereview.chromium.org/606113005/diff/100001/content/public/test/tes...
File content/public/test/test_renderer_host.h (right):

https://codereview.chromium.org/606113005/diff/100001/content/public/test/tes...
content/public/test/test_renderer_host.h:57: // Calls OnMsgNavigate on the
RenderFrameHost with the given information,
On 2014/09/29 20:28:09, nasko wrote:
> nit: s/OnMsgNavigate/OnDidCommitProvisionalLoad/

Done.

[nasko, 2014-09-29 21:53:50]

LGTM!

[Charlie Reis, 2014-09-29 21:55:29]

The CQ bit was checked by creis@chromium.org

[commit-bot: I haz the power, 2014-09-29 21:56:03]

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

[commit-bot: I haz the power, 2014-09-29 22:05:32]

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

[commit-bot: I haz the power, 2014-09-29 22:05:33]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Charlie Reis, 2014-09-29 22:09:40]

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

[Charlie Reis, 2014-09-29 22:09:41]

jam: Can you take a quick look at browser_with_test_window_test.cc for owners?

[jam, 2014-09-29 22:24:18]

On 2014/09/29 22:09:41, Charlie Reis wrote:
> jam: Can you take a quick look at browser_with_test_window_test.cc for owners?

lgtm

[Charlie Reis, 2014-09-29 22:24:43]

The CQ bit was checked by creis@chromium.org

[commit-bot: I haz the power, 2014-09-29 22:26:15]

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

[commit-bot: I haz the power, 2014-09-29 22:45:29]

Committed patchset #7 (id:120001) as c74391e5802ba257a2732805fd0628925feb1383

[commit-bot: I haz the power, 2014-09-29 22:48:22]

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