Remove SiteIsolationPolicy::IsSwappedOutStateForbidden().

Remove SiteIsolationPolicy::IsSwappedOutStateForbidden().

IsSwappedOutStateForbidden has been hardcoded to return true for the last
two milestones and it is now time to remove it.

BUG=357747, 253537
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

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

[nasko, 2016-03-11 19:30:21]

Description was changed from

==========
Remove SiteIsolationPolicy::IsSwappedOutStateForbidden().

IsSwappedOutStateForbidden has been hardcoded to return true for the last
two milestones and it is now time to remove it.

BUG=357747
==========

to

==========
Remove SiteIsolationPolicy::IsSwappedOutStateForbidden().

IsSwappedOutStateForbidden has been hardcoded to return true for the last
two milestones and it is now time to remove it.

BUG=357747
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[nasko, 2016-03-11 19:57:08]

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

[nasko, 2016-03-11 19:57:09]

Yay! The time has come!

Daniel, can you do the honors of reviewing this one?

Thanks in advance,
Nasko

[dmazzoni, 2016-03-11 20:31:33]

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

[dmazzoni, 2016-03-11 20:31:34]

https://codereview.chromium.org/1785153005/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1785153005/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:981: is_swapped_out_(false),
Why not delete this as part of the same change?

[dcheng, 2016-03-11 20:32:28]

dcheng@chromium.org changed reviewers:
- dmazzoni@chromium.org

[dcheng, 2016-03-11 20:32:28]

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

https://codereview.chromium.org/1785153005/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager_unittest.cc:401: // Creates
a test RenderViewHost that's swapped out.
It seems weird that the function is called CreateSwappedOutRenderViewHost but it
returns a RenderFrameHost. The return value also appears to be unused.

https://codereview.chromium.org/1785153005/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (left):

https://codereview.chromium.org/1785153005/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:1665: // !is_main_frame below.
This TODO is stale right? I don't see a !is_main_frame case at all anywhere, but
I want to make sure I'm not missing something.

[Charlie Reis, 2016-03-11 21:56:55]

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

[Charlie Reis, 2016-03-11 21:56:55]

This is so nice to see!  Just a few thoughts, but I'm excited to land this.

Let's mention in the CL description that this is just the first step: removing
the actual code hidden behind IsSwappedOutStateForbidden.  There will be more
removals to come.

(I'll leave the choice to you whether to remove dead code now as well, such as
NavigateToSwappedOutURL().)

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

https://codereview.chromium.org/1785153005/diff/1/content/browser/frame_host/...
content/browser/frame_host/frame_tree.cc:255: // Create the swapped out RVH for
the new SiteInstance. This will create
Stale comment.

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

https://codereview.chromium.org/1785153005/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager.cc:1576: bool swapped_out =
!!(flags & CREATE_RF_SWAPPED_OUT);
TODO to remove CREATE_RF_SWAPPED_OUT flag?

https://codereview.chromium.org/1785153005/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_manager_unittest.cc (left):

https://codereview.chromium.org/1785153005/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager_unittest.cc:825: // This
test is invalid in --site-per-process mode, as swapped-out is no
Is there a reasons to keep the rest of this test?

https://codereview.chromium.org/1785153005/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager_unittest.cc:852: // This
test is invalid in --site-per-process mode, as swapped-out is no
Is there a reason to keep the rest of this test?

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

https://codereview.chromium.org/1785153005/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1889: // rfh1
should be deleted
nit: Add period.

https://codereview.chromium.org/1785153005/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1785153005/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:4370: int
WebContentsImpl::CreateSwappedOutRenderView(
Looks like there's no remaining callers of this?  I'm not sure why it had a
branch for SiteIsolationPolicy::IsSwappedOutStateForbidden(), but it looks
unused.

https://codereview.chromium.org/1785153005/diff/1/content/renderer/accessibil...
File content/renderer/accessibility/renderer_accessibility_browsertest.cc
(right):

https://codereview.chromium.org/1785153005/diff/1/content/renderer/accessibil...
content/renderer/accessibility/renderer_accessibility_browsertest.cc:176: #endif
Delete this stuff too, and we can close http://crbug.com/253537.

https://codereview.chromium.org/1785153005/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1785153005/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:981: is_swapped_out_(false),
On 2016/03/11 20:31:34, dmazzoni wrote:
> Why not delete this as part of the same change?

We're just doing it in pieces to keep the size reasonable.  That'll happen soon.
 :)

https://codereview.chromium.org/1785153005/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:4736: if (info.urlRequest.url() !=
GURL(kSwappedOutURL)) {
I suppose this is next on the chopping block?  Maybe add a TODO.

[nasko, 2016-03-11 22:30:11]

Description was changed from

==========
Remove SiteIsolationPolicy::IsSwappedOutStateForbidden().

IsSwappedOutStateForbidden has been hardcoded to return true for the last
two milestones and it is now time to remove it.

BUG=357747
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Remove SiteIsolationPolicy::IsSwappedOutStateForbidden().

IsSwappedOutStateForbidden has been hardcoded to return true for the last
two milestones and it is now time to remove it.

BUG=357747, 253537
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[nasko, 2016-03-11 22:35:33]

Comments addressed.

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

https://codereview.chromium.org/1785153005/diff/1/content/browser/frame_host/...
content/browser/frame_host/frame_tree.cc:255: // Create the swapped out RVH for
the new SiteInstance. This will create
On 2016/03/11 21:56:56, Charlie Reis (slow til 3-15) wrote:
> Stale comment.

Done.

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

https://codereview.chromium.org/1785153005/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager.cc:1576: bool swapped_out =
!!(flags & CREATE_RF_SWAPPED_OUT);
On 2016/03/11 21:56:56, Charlie Reis (slow til 3-15) wrote:
> TODO to remove CREATE_RF_SWAPPED_OUT flag?

I have that and a lot more in a doc. I don't see a good reason to add those
TODOs, as I'm planning to be ripping these out very quickly.

https://codereview.chromium.org/1785153005/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_manager_unittest.cc (left):

https://codereview.chromium.org/1785153005/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager_unittest.cc:825: // This
test is invalid in --site-per-process mode, as swapped-out is no
On 2016/03/11 21:56:56, Charlie Reis (slow til 3-15) wrote:
> Is there a reasons to keep the rest of this test?

I want to keep these two around, as it isn't quite clear yet what "swapped out
RVH" means. I want to ensure that we aren't returning these out to callers when
we aren't supposed to. I forsee them disappearing once we have the RV/RW split
out complete and the IPCs are no longer routed through RVHs.

https://codereview.chromium.org/1785153005/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager_unittest.cc:852: // This
test is invalid in --site-per-process mode, as swapped-out is no
On 2016/03/11 21:56:56, Charlie Reis (slow til 3-15) wrote:
> Is there a reason to keep the rest of this test?

See the above comment.

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

https://codereview.chromium.org/1785153005/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager_unittest.cc:401: // Creates
a test RenderViewHost that's swapped out.
On 2016/03/11 20:32:28, dcheng wrote:
> It seems weird that the function is called CreateSwappedOutRenderViewHost but
it
> returns a RenderFrameHost. The return value also appears to be unused.

Done.

https://codereview.chromium.org/1785153005/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1889: // rfh1
should be deleted
On 2016/03/11 21:56:56, Charlie Reis (slow til 3-15) wrote:
> nit: Add period.

Done.

https://codereview.chromium.org/1785153005/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1785153005/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:4370: int
WebContentsImpl::CreateSwappedOutRenderView(
On 2016/03/11 21:56:56, Charlie Reis (slow til 3-15) wrote:
> Looks like there's no remaining callers of this?  I'm not sure why it had a
> branch for SiteIsolationPolicy::IsSwappedOutStateForbidden(), but it looks
> unused.

Thanks for catching this! Added to the list of things to be removed :).

https://codereview.chromium.org/1785153005/diff/1/content/renderer/accessibil...
File content/renderer/accessibility/renderer_accessibility_browsertest.cc
(right):

https://codereview.chromium.org/1785153005/diff/1/content/renderer/accessibil...
content/renderer/accessibility/renderer_accessibility_browsertest.cc:176: #endif
On 2016/03/11 21:56:56, Charlie Reis (slow til 3-15) wrote:
> Delete this stuff too, and we can close http://crbug.com/253537.

Done.

https://codereview.chromium.org/1785153005/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (left):

https://codereview.chromium.org/1785153005/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:1665: // !is_main_frame below.
On 2016/03/11 20:32:28, dcheng wrote:
> This TODO is stale right? I don't see a !is_main_frame case at all anywhere,
but
> I want to make sure I'm not missing something.

Indeed it no longer applies, which is why I've removed it.

https://codereview.chromium.org/1785153005/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1785153005/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:981: is_swapped_out_(false),
On 2016/03/11 21:56:56, Charlie Reis (slow til 3-15) wrote:
> On 2016/03/11 20:31:34, dmazzoni wrote:
> > Why not delete this as part of the same change?
> 
> We're just doing it in pieces to keep the size reasonable.  That'll happen
soon.
>  :)

Indeed. It is already removed in my next iteration :).

https://codereview.chromium.org/1785153005/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:4736: if (info.urlRequest.url() !=
GURL(kSwappedOutURL)) {
On 2016/03/11 21:56:56, Charlie Reis (slow til 3-15) wrote:
> I suppose this is next on the chopping block?  Maybe add a TODO.

Yes, it is. As mentioned elsewhere, I want to rip these out quickly and I'm
intentionally not putting TODOs. If you feel I should, let me know.

[Charlie Reis, 2016-03-11 23:15:41]

Thanks!  LGTM once the merge conflict is fixed.

[nasko, 2016-03-11 23:29:34]

The CQ bit was checked by nasko@chromium.org

[nasko, 2016-03-11 23:29:34]

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/1785153005/#ps40001
(title: "Rebase on ToT.")

[commit-bot: I haz the power, 2016-03-11 23:30:06]

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

[commit-bot: I haz the power, 2016-03-12 04:43:16]

Description was changed from

==========
Remove SiteIsolationPolicy::IsSwappedOutStateForbidden().

IsSwappedOutStateForbidden has been hardcoded to return true for the last
two milestones and it is now time to remove it.

BUG=357747, 253537
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Remove SiteIsolationPolicy::IsSwappedOutStateForbidden().

IsSwappedOutStateForbidden has been hardcoded to return true for the last
two milestones and it is now time to remove it.

BUG=357747, 253537
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-03-12 04:43:17]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-03-12 04:45:50]

Description was changed from

==========
Remove SiteIsolationPolicy::IsSwappedOutStateForbidden().

IsSwappedOutStateForbidden has been hardcoded to return true for the last
two milestones and it is now time to remove it.

BUG=357747, 253537
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Remove SiteIsolationPolicy::IsSwappedOutStateForbidden().

IsSwappedOutStateForbidden has been hardcoded to return true for the last
two milestones and it is now time to remove it.

BUG=357747, 253537
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

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

[commit-bot: I haz the power, 2016-03-12 04:45:51]

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