Move existing kSitePerProcess checks to a policy-oracle object

Move existing kSitePerProcess checks to a policy-oracle object

Introduces SiteIsolationPolicy, which interprets the kSitePerProcess
switch (and eventually others too), in order to make decisions about
oopifs, oopif-related features, and site isolation policy.

Replace explicit calls to HasSwitch(content::kSitePerProcess) with
calls to appropriate methods of SiteIsolationPolicy,
BrowserPluginGuestMode, or content's browser_test_utils.
SiteIsolationPolicy is content-internal, and I expect it eventually
to become a stateful object.

There are six cases:

1. SiteIsolationPolicy::DoesSiteRequireDedicatedProcess(url)  This
   anticipates site isolation being launched for a subset of
   sites/schemes.
2. BrowserPluginGuestMode::UseCrossProcessFramesForGuests()  Tracks
   some current feature work that requires out of process iframes and
   so piggybacks on --site-per-process. We ought to control this by a
   different flag
3. SiteIsolationPolicy::AreCrossProcessFramesPossible()  For dchecks
   and determining whether to create proxies -- basically it is the
   "or" of all of the above functions.
4. SiteIsolationPolicy::UseSubframeNavigationEntries()  Tracks some
   current feature work related to navigation, that's tied to --site-
   per-process. Expected to be shortlived.
5. IsSwappedOutStateForbidden()  (on RFHM/RFProxy) Another class of
   temporary feature work.
6. content::AreAllSitesIsolatedForTesting()  For bailing out of
   tests.

BUG=481066
Committed: https://crrev.com/d30fd969388ba0ffe122eb63c22776aa02a8836f
Cr-Commit-Position: refs/heads/master@{#340570}

[ncarter (slow), 2015-07-06 23:04:46]

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

[ncarter (slow), 2015-07-06 23:04:48]

nasko: please review -- you'll want to start with site_isolation_policy.h, and
site_instance.h, before starting at the top.

[nasko, 2015-07-08 12:52:24]

Thanks for taking this on! A bunch of comments, but mostly nits.

https://codereview.chromium.org/1208143002/diff/160001/chrome/renderer/chrome...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/1208143002/diff/160001/chrome/renderer/chrome...
chrome/renderer/chrome_content_renderer_client.cc:1404: // |opener_frame|/its
ancestors.
I think this one can be using AreCrossProcessFramesPossible, as if not possible,
then we know that the top frame is in the same process and we can reach its
document.

https://codereview.chromium.org/1208143002/diff/160001/content/browser/child_...
File content/browser/child_process_security_policy_impl.cc (right):

https://codereview.chromium.org/1208143002/diff/160001/content/browser/child_...
content/browser/child_process_security_policy_impl.cc:573: // TODO(nick): Can we
trust |resource_type| here?
I don't think we can do that before PlzNavigate, since the bit comes from the
renderer. I don't know if somewhere in the network stack this is checked for
sanity, but I would assume no.

https://codereview.chromium.org/1208143002/diff/160001/content/browser/frame_...
File content/browser/frame_host/frame_tree_unittest.cc (right):

https://codereview.chromium.org/1208143002/diff/160001/content/browser/frame_...
content/browser/frame_host/frame_tree_unittest.cc:16: #include
"content/public/common/site_isolation_policy.h"
Why is this include needed?

https://codereview.chromium.org/1208143002/diff/160001/content/browser/frame_...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/1208143002/diff/160001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:355: bool oopifs_possible =
SiteIsolationPolicy::AreCrossProcessFramesPossible();
nit: oopifs_enabled?

https://codereview.chromium.org/1208143002/diff/160001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1208143002/diff/160001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.cc:923: //
TODO(nick/nasko): Should this be IsSwappedOutStateForbidden, to match
nit: ',' in stead of '/'.

https://codereview.chromium.org/1208143002/diff/160001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.cc:1987:
CHECK(SiteIsolationPolicy::AreCrossProcessFramesPossible());
I'd suggest consulting with Alex on this one. If we are going to be creating
proxies in regular mode Chrome because of the openers work he is doing, then
this is a change in behavior and the CHECK will be hit.

https://codereview.chromium.org/1208143002/diff/160001/content/browser/loader...
File content/browser/loader/cross_site_resource_handler.cc (right):

https://codereview.chromium.org/1208143002/diff/160001/content/browser/loader...
content/browser/loader/cross_site_resource_handler.cc:103: // considered by
RenderFrameHostManager::UpdateStateForNavigate.
We should update this comment to point to
RenderFrameHostManager::DetermineSiteInstanceForURL instead of
UpdateStateForNavigate.

https://codereview.chromium.org/1208143002/diff/160001/content/browser/loader...
content/browser/loader/cross_site_resource_handler.cc:170: // TODO(nick): What
does "other cross-process navigations" mean in the
There are two types of cross-process navigations:
* We determine we need to do a cross-process navigation, allocate the new
renderer, then make the request. This is usually how browser initiated
navigations happen.
* A resource request for the navigation is made and at the network stack/IO
thread we check whether a cross-process navigation is needed, then we pause the
response, hop to the UI thread, allocate the new renderer, update the response
metadata with the new renderer id, then resume the response, so it goes to the
new renderer.

I think the above comment means the first type when it says "other", since it
didn't go through this transfer logic.

https://codereview.chromium.org/1208143002/diff/160001/content/browser/loader...
content/browser/loader/cross_site_resource_handler.cc:174: bool
embedder_requests_transfer =
What is "embedder" in this context?

https://codereview.chromium.org/1208143002/diff/160001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/1208143002/diff/160001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:1901: // TODO(nick):
Shouldn't this consult the SiteIsolationPolicy?
Your comment in RenderProcessHost::ShouldTryToUseExistingProcessHost below
already states that checks with SiteIsolationPolicy should be performed, so I
think the answer is indeed Yes. Maybe rephrase the TODO to be a statement
instead of a question?

https://codereview.chromium.org/1208143002/diff/160001/content/browser/site_i...
File content/browser/site_instance_impl.cc (right):

https://codereview.chromium.org/1208143002/diff/160001/content/browser/site_i...
content/browser/site_instance_impl.cc:352: // sites. (file a bug for this)
I assume the "file a bug for this" is to happen before committing this CL,
right?

https://codereview.chromium.org/1208143002/diff/160001/content/public/browser...
File content/public/browser/site_instance.h (right):

https://codereview.chromium.org/1208143002/diff/160001/content/public/browser...
content/public/browser/site_instance.h:38: // PROCESS PER SITE INSTANCE (the
current default), SiteInstances are created
Why all caps?

https://codereview.chromium.org/1208143002/diff/160001/content/public/browser...
content/public/browser/site_instance.h:47: // and is made possible by our
support for out-of-process iframes.  A subframe
nit: s/our support/supporting/

https://codereview.chromium.org/1208143002/diff/160001/content/public/browser...
content/public/browser/site_instance.h:49: // containing document.  Cross-site
navigation of top-level frames or subframes
nit: The usage of one or two spaces after a sentence end is inconsistent. I
don't have strong preference for which one should be used, but I do like to see
things being consistent.

https://codereview.chromium.org/1208143002/diff/160001/content/public/browser...
content/public/browser/site_instance.h:49: // containing document.  Cross-site
navigation of top-level frames or subframes
If the wording is "top-level frames or subframes" it seems to encompass the
entire set of possible frames. Can it be dropped?

https://codereview.chromium.org/1208143002/diff/160001/content/public/browser...
content/public/browser/site_instance.h:52: // just one site, allowing the same
origin policy to be enforced by the IPC
We have a sandbox and an IPC channel to communicate outside of it, so "IPC
sandbox" sounds a bit strange.

https://codereview.chromium.org/1208143002/diff/160001/content/public/common/...
File content/public/common/site_isolation_policy.h (right):

https://codereview.chromium.org/1208143002/diff/160001/content/public/common/...
content/public/common/site_isolation_policy.h:26: // Note: Since cross process
frames will soon be possible by default (e.g. for
nit: You've standardized on having cross-process hyphenated, but here it isn't.

https://codereview.chromium.org/1208143002/diff/160001/content/public/common/...
content/public/common/site_isolation_policy.h:34: static bool
AreCrossProcessFramesPossible();
nit: AreCrossProcessFramesEnabled? Possible to me implies that it is not
guaranteed to be true. While it is correct not all frames will be cross-process
and it will be dependent on policy, I think for this one we care whether we
should be doing extra work in the case there is at least one cross-process
iframe.

https://codereview.chromium.org/1208143002/diff/160001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1208143002/diff/160001/content/renderer/rende...
content/renderer/render_frame_impl.cc:3901: // In --site-per-process, just send
a single HistoryItem for this frame,
Should we be dropping these "In --site-per-process" prefixes in the comments?
Similar comment is present further down in this file.

[ncarter (slow), 2015-07-08 23:28:38]

https://codereview.chromium.org/1208143002/diff/160001/content/public/browser...
File content/public/browser/site_instance.h (right):

https://codereview.chromium.org/1208143002/diff/160001/content/public/browser...
content/public/browser/site_instance.h:52: // just one site, allowing the same
origin policy to be enforced by the IPC
On 2015/07/08 12:52:23, nasko (paris) wrote:
> We have a sandbox and an IPC channel to communicate outside of it, so "IPC
> sandbox" sounds a bit strange. 

What would you say instead? I was actually paraphrasing something you wrote in
email that was equally imprecise, but which I really liked the gist of: "Our
Site Isolation team has been hard at work building the infrastructure for
out-of-process iframes (OOPIFs) so that Chrome's sandbox can finally help
enforce the Same Origin Policy."

It's not clear to me whether the term 'sandbox' includes the abilities that are
granted to a child process via IPC messages, or just the OS level permissions
(other than the ability to do IPC communication) that are taken away from a
process by the OS-level sandbox.

[Charlie Reis, 2015-07-08 23:42:58]

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

[Charlie Reis, 2015-07-08 23:42:59]

[+site-isolation-reviews]

I haven't looked at most of this yet (though I'm excited to once Nasko's ready),
but I wanted to reply to the sandbox question.  A few nearby nits snuck in as
well.  :)

https://codereview.chromium.org/1208143002/diff/160001/content/public/browser...
File content/public/browser/site_instance.h (right):

https://codereview.chromium.org/1208143002/diff/160001/content/public/browser...
content/public/browser/site_instance.h:27: // from (the site) and which tabs
have references to each other (the instance).
s/tabs/frames/

https://codereview.chromium.org/1208143002/diff/160001/content/public/browser...
content/public/browser/site_instance.h:32: // all tabs that might be able to
script each other because of how they were
s/tabs/frames/

https://codereview.chromium.org/1208143002/diff/160001/content/public/browser...
content/public/browser/site_instance.h:38: // PROCESS PER SITE INSTANCE (the
current default), SiteInstances are created
On 2015/07/08 12:52:23, nasko (paris) wrote:
> Why all caps?

I'm ok with either caps or a numbered list.

Please use a colon rather than a comma after the close paren, though.

https://codereview.chromium.org/1208143002/diff/160001/content/public/browser...
content/public/browser/site_instance.h:46: // SITE PER PROCESS (currently
experimental) is the most granular process model,
Colon after close paren.

https://codereview.chromium.org/1208143002/diff/160001/content/public/browser...
content/public/browser/site_instance.h:52: // just one site, allowing the same
origin policy to be enforced by the IPC
On 2015/07/08 23:28:38, ncarter wrote:
> On 2015/07/08 12:52:23, nasko (paris) wrote:
> > We have a sandbox and an IPC channel to communicate outside of it, so "IPC
> > sandbox" sounds a bit strange. 
> 
> What would you say instead? I was actually paraphrasing something you wrote in
> email that was equally imprecise, but which I really liked the gist of: "Our
> Site Isolation team has been hard at work building the infrastructure for
> out-of-process iframes (OOPIFs) so that Chrome's sandbox can finally help
> enforce the Same Origin Policy."
> 
> It's not clear to me whether the term 'sandbox' includes the abilities that
are
> granted to a child process via IPC messages, or just the OS level permissions
> (other than the ability to do IPC communication) that are taken away from a
> process by the OS-level sandbox.

I'd just say sandbox, or renderer sandbox if you want to clarify.  IPC is just
the mechanism code inside the sandbox uses to communicate.  (That's covered more
in http://www.chromium.org/developers/design-documents/sandbox.)

[ncarter (slow), 2015-07-10 23:29:19]

I didn't respond to all of nasko's comments yet, but have uploaded some fixes.
I'd like Charlie to take a pass when possible.

https://codereview.chromium.org/1208143002/diff/160001/chrome/renderer/chrome...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/1208143002/diff/160001/chrome/renderer/chrome...
chrome/renderer/chrome_content_renderer_client.cc:1404: // |opener_frame|/its
ancestors.
On 2015/07/08 12:52:22, nasko (paris) wrote:
> I think this one can be using AreCrossProcessFramesPossible, as if not
possible,
> then we know that the top frame is in the same process and we can reach its
> document.

Alex and I had previously chatted about this, and our sense was that this
shouldn't be driven by the command line flag at all.
AreCrossProcessFramesPossible seems like a particularly bad choice here (no
reason to change the behavior globally if we're just enabled for one
site/scheme).

I think the right thing is to basically find the localroot, so that why I left
this one untouched. Does that make sense to you?

https://codereview.chromium.org/1208143002/diff/160001/content/browser/child_...
File content/browser/child_process_security_policy_impl.cc (right):

https://codereview.chromium.org/1208143002/diff/160001/content/browser/child_...
content/browser/child_process_security_policy_impl.cc:573: // TODO(nick): Can we
trust |resource_type| here?
On 2015/07/08 12:52:22, nasko (paris) wrote:
> I don't think we can do that before PlzNavigate, since the bit comes from the
> renderer. I don't know if somewhere in the network stack this is checked for
> sanity, but I would assume no.

That's good to know. I'm leaving the TODO here because it'll be an important
thing to consider in depth when we get around to this step.

https://codereview.chromium.org/1208143002/diff/160001/content/browser/frame_...
File content/browser/frame_host/frame_tree_unittest.cc (right):

https://codereview.chromium.org/1208143002/diff/160001/content/browser/frame_...
content/browser/frame_host/frame_tree_unittest.cc:16: #include
"content/public/common/site_isolation_policy.h"
On 2015/07/08 12:52:22, nasko (paris) wrote:
> Why is this include needed?

Done.

https://codereview.chromium.org/1208143002/diff/160001/content/browser/frame_...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/1208143002/diff/160001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:355: bool oopifs_possible =
SiteIsolationPolicy::AreCrossProcessFramesPossible();
On 2015/07/08 12:52:23, nasko (paris) wrote:
> nit: oopifs_enabled?

I'm sticking with _possible.

https://codereview.chromium.org/1208143002/diff/160001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1208143002/diff/160001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.cc:923: //
TODO(nick/nasko): Should this be IsSwappedOutStateForbidden, to match
On 2015/07/08 12:52:23, nasko (paris) wrote:
> nit: ',' in stead of '/'.

Done.

https://codereview.chromium.org/1208143002/diff/160001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.cc:1987:
CHECK(SiteIsolationPolicy::AreCrossProcessFramesPossible());
On 2015/07/08 12:52:23, nasko (paris) wrote:
> I'd suggest consulting with Alex on this one. If we are going to be creating
> proxies in regular mode Chrome because of the openers work he is doing, then
> this is a change in behavior and the CHECK will be hit.

A frame with a proxy to its parent is an oopif by definition, right?

https://codereview.chromium.org/1208143002/diff/160001/content/browser/loader...
File content/browser/loader/cross_site_resource_handler.cc (right):

https://codereview.chromium.org/1208143002/diff/160001/content/browser/loader...
content/browser/loader/cross_site_resource_handler.cc:103: // considered by
RenderFrameHostManager::UpdateStateForNavigate.
On 2015/07/08 12:52:23, nasko (paris) wrote:
> We should update this comment to point to
> RenderFrameHostManager::DetermineSiteInstanceForURL instead of
> UpdateStateForNavigate.

Done.

https://codereview.chromium.org/1208143002/diff/160001/content/browser/loader...
content/browser/loader/cross_site_resource_handler.cc:170: // TODO(nick): What
does "other cross-process navigations" mean in the
On 2015/07/08 12:52:23, nasko (paris) wrote:
> There are two types of cross-process navigations:
> * We determine we need to do a cross-process navigation, allocate the new
> renderer, then make the request. This is usually how browser initiated
> navigations happen.
> * A resource request for the navigation is made and at the network stack/IO
> thread we check whether a cross-process navigation is needed, then we pause
the
> response, hop to the UI thread, allocate the new renderer, update the response
> metadata with the new renderer id, then resume the response, so it goes to the
> new renderer.
> 
> I think the above comment means the first type when it says "other", since it
> didn't go through this transfer logic.

Done.

https://codereview.chromium.org/1208143002/diff/160001/content/browser/loader...
content/browser/loader/cross_site_resource_handler.cc:174: bool
embedder_requests_transfer =
On 2015/07/08 12:52:23, nasko (paris) wrote:
> What is "embedder" in this context?

I meant the content embedder (GetContentClient()). I changed the variable name.

https://codereview.chromium.org/1208143002/diff/160001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/1208143002/diff/160001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:1901: // TODO(nick):
Shouldn't this consult the SiteIsolationPolicy?
On 2015/07/08 12:52:23, nasko (paris) wrote:
> Your comment in RenderProcessHost::ShouldTryToUseExistingProcessHost below
> already states that checks with SiteIsolationPolicy should be performed, so I
> think the answer is indeed Yes. Maybe rephrase the TODO to be a statement
> instead of a question?

Done.

https://codereview.chromium.org/1208143002/diff/160001/content/browser/site_i...
File content/browser/site_instance_impl.cc (right):

https://codereview.chromium.org/1208143002/diff/160001/content/browser/site_i...
content/browser/site_instance_impl.cc:352: // sites. (file a bug for this)
On 2015/07/08 12:52:23, nasko (paris) wrote:
> I assume the "file a bug for this" is to happen before committing this CL,
> right?

Done.

https://codereview.chromium.org/1208143002/diff/160001/content/public/common/...
File content/public/common/site_isolation_policy.h (right):

https://codereview.chromium.org/1208143002/diff/160001/content/public/common/...
content/public/common/site_isolation_policy.h:34: static bool
AreCrossProcessFramesPossible();
On 2015/07/08 12:52:23, nasko (paris) wrote:
> nit: AreCrossProcessFramesEnabled? Possible to me implies that it is not
> guaranteed to be true. While it is correct not all frames will be
cross-process
> and it will be dependent on policy, I think for this one we care whether we
> should be doing extra work in the case there is at least one cross-process
> iframe.

I prefer "possible" to "enabled", because there's not going to be a way to
enable or disable cross process iframes. Instead, there are ways to enable
various features that might create cross process iframes. I wanted a name that
was hard to confuse with "--site-per-process enabled", because I think this
function is soon going to be:

 HasSwitch(kSitePerProcess) ||
 isolate_extensions ||
 enable_new_guest_mode

To me, "Possible" communicates that uncertainty in a good way.

https://codereview.chromium.org/1208143002/diff/160001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1208143002/diff/160001/content/renderer/rende...
content/renderer/render_frame_impl.cc:3901: // In --site-per-process, just send
a single HistoryItem for this frame,
On 2015/07/08 12:52:24, nasko (paris) wrote:
> Should we be dropping these "In --site-per-process" prefixes in the comments?
> Similar comment is present further down in this file.

I can try, but I didn't want to throw effort at it until we were all in
agreement about which buckets and to use and what to name them. For the most
part the comments about --site-per-process are still technically correct.

[jam, 2015-07-13 21:53:25]

driveby: the new methods in content/public aren't all used by outside content.
why are they exposed?
(https://www.chromium.org/developers/content-module/content-api)

[Charlie Reis, 2015-07-13 22:13:13]

[+lazyboy to CC for GuestsShouldUseCrossProcessFrames]

Nice.  I'm pretty happy with the buckets you've chosen.  I guess we'll need a
content-internal set of methods for the ones not used outside content to meet
the API guidelines.

Should IsSwappedOutStateForbidden be on SiteIsolationPolicy to be consistent
with the others?

Let's mention IsolateAllSitesForTesting in the CL description.

https://codereview.chromium.org/1208143002/diff/160001/content/browser/frame_...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/1208143002/diff/160001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:355: bool oopifs_possible =
SiteIsolationPolicy::AreCrossProcessFramesPossible();
On 2015/07/10 23:29:18, ncarter wrote:
> On 2015/07/08 12:52:23, nasko (paris) wrote:
> > nit: oopifs_enabled?
> 
> I'm sticking with _possible.

+1 to "possible," since it doesn't imply how widely we use them.

https://codereview.chromium.org/1208143002/diff/160001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1208143002/diff/160001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.cc:1987:
CHECK(SiteIsolationPolicy::AreCrossProcessFramesPossible());
On 2015/07/10 23:29:18, ncarter wrote:
> On 2015/07/08 12:52:23, nasko (paris) wrote:
> > I'd suggest consulting with Alex on this one. If we are going to be creating
> > proxies in regular mode Chrome because of the openers work he is doing, then
> > this is a change in behavior and the CHECK will be hit.
> 
> A frame with a proxy to its parent is an oopif by definition, right?

Calling this by default is a behavior change, but it looks like it should be
safe (since GetProxyToParent returns null for the main frame and we won't even
get to CommitPending for subframes).

https://codereview.chromium.org/1208143002/diff/180001/chrome/renderer/chrome...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/1208143002/diff/180001/chrome/renderer/chrome...
chrome/renderer/chrome_content_renderer_client.cc:1404: // |opener_frame|/its
ancestors.
Yeah, this one's a bit awkward, since there's a chance we might isolate web
pages inside extensions before we isolate extensions inside web pages.  The
local root idea might be on the right track; let's come back to it after this CL
lands.

https://codereview.chromium.org/1208143002/diff/180001/content/browser/child_...
File content/browser/child_process_security_policy_impl.cc (right):

https://codereview.chromium.org/1208143002/diff/180001/content/browser/child_...
content/browser/child_process_security_policy_impl.cc:574: return true;
This whole function is in kind of a weird state.  Robert introduced it way back
in https://codereview.chromium.org/11416121/, and Nasko disabled it when
starting the OOPIF prototype in https://codereview.chromium.org/11772005.

It seems to me that we don't need it anymore.  It was meant as a way to block
cross-site iframes before we had OOPIFs (https://crbug.com/159215).  With
document blocking (https://crbug.com/268640) and killing renderers that commit
bad URLs (https://crbug.com/85128), we don't need to also block the request in
the ResourceLoader in this way.

I'd propose ripping it out entirely, either before or after this CL lands.

https://codereview.chromium.org/1208143002/diff/180001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1208143002/diff/180001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.cc:923: //
TODO(nick,nasko): Should this be IsSwappedOutStateForbidden, to match
That sounds reasonable to me.

https://codereview.chromium.org/1208143002/diff/180001/content/browser/loader...
File content/browser/loader/cross_site_resource_handler.cc (right):

https://codereview.chromium.org/1208143002/diff/180001/content/browser/loader...
content/browser/loader/cross_site_resource_handler.cc:213: // skipped for WebUI
processes for now, since pages like the NTP host multiple
While we're here, let's fix this wording: the NTP is no longer WebUI, but we do
still need the check because chrome://settings has multiple "cross-site"
chrome:// frames that don't work with OOPIFs yet.

https://codereview.chromium.org/1208143002/diff/180001/content/browser/site_i...
File content/browser/site_instance_impl.cc (right):

https://codereview.chromium.org/1208143002/diff/180001/content/browser/site_i...
content/browser/site_instance_impl.cc:350: // protection. If only some sites are
isolated, we need additional logic here
nit: Drop "here," since the additional logic will go in the places this is
checked, not here where it is set.

https://codereview.chromium.org/1208143002/diff/180001/content/public/browser...
File content/public/browser/site_instance.h (right):

https://codereview.chromium.org/1208143002/diff/180001/content/public/browser...
content/public/browser/site_instance.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
I left several comments in this class in patch set 9, which I think still apply.

https://codereview.chromium.org/1208143002/diff/180001/content/public/browser...
content/public/browser/site_instance.h:28: // Here, a "site" is similar to the
page's origin, but includes only the
nit: Drop second comma if you're dropping "it."

https://codereview.chromium.org/1208143002/diff/180001/content/public/browser...
content/public/browser/site_instance.h:125: // TODO(nick): Does this name suck?
It's a bit long, but HasDedicatedProcess doesn't capture the case that the site
needs isolation and the process hasn't been created yet.  I'm ok with it as is.

https://codereview.chromium.org/1208143002/diff/180001/content/public/common/...
File content/public/common/site_isolation_policy.cc (right):

https://codereview.chromium.org/1208143002/diff/180001/content/public/common/...
content/public/common/site_isolation_policy.cc:25: return
AreCrossProcessFramesPossible();
Why does this one call one of the others when the rest check the command line
directly?

https://codereview.chromium.org/1208143002/diff/180001/content/public/common/...
File content/public/common/site_isolation_policy.h (right):

https://codereview.chromium.org/1208143002/diff/180001/content/public/common/...
content/public/common/site_isolation_policy.h:19: class CONTENT_EXPORT
SiteIsolationPolicy {
Let's say what threads these are allowed to be called on.

https://codereview.chromium.org/1208143002/diff/180001/content/public/common/...
content/public/common/site_isolation_policy.h:21: // Returns true if the current
process model might dictate the use of cross-
s/dictate/allow/

(Dictate sounds like they're always necessary.)

https://codereview.chromium.org/1208143002/diff/180001/content/public/common/...
content/public/common/site_isolation_policy.h:22: // process iframes. This
should typically used to avoid executing codepaths
s/used/be consulted/

https://codereview.chromium.org/1208143002/diff/180001/content/public/common/...
content/public/common/site_isolation_policy.h:28: // to temporary stop-gaps.
I agree with this and we can keep the comment, but it's interesting to note that
this is the second most common call (behind only AreAllSitesIsolatedForTesting).
 Do you get the sense that many of these will be easy to convert after this CL
goes in?

https://codereview.chromium.org/1208143002/diff/180001/content/public/common/...
content/public/common/site_isolation_policy.h:43: static bool
DoesSiteRequireDedicatedProcess(const GURL& site);
Let's add a bit to the comment about what's expected as input here: is it only
ok to pass site URLs from SiteInstances, or is it ok to pass a full URL with
hostname and path?  Is it expected to be the effective URL (which would be
implied if it's only site URLs)?

This is important if it's possible to call this from the IO thread, etc, where
we can't yet easily determine the effective URL.

https://codereview.chromium.org/1208143002/diff/180001/content/public/common/...
content/public/common/site_isolation_policy.h:56: static bool
GuestsShouldUseCrossProcessFrames();
Maybe ShouldGuestsUseCrossProcessSubframes or UseCrossProcessFramesForGuests? 
Those seem more consistent with the others, which start with AreX, UseX, DoesX.

https://codereview.chromium.org/1208143002/diff/180001/content/public/common/...
content/public/common/site_isolation_policy.h:65: static void
IsolateAllSitesForTesting(base::CommandLine* command_line);
nit: Let's list this last, since the rest just return bools and this one does an
actual state change.

https://codereview.chromium.org/1208143002/diff/180001/extensions/renderer/di...
File extensions/renderer/dispatcher.cc (right):

https://codereview.chromium.org/1208143002/diff/180001/extensions/renderer/di...
extensions/renderer/dispatcher.cc:477: if
(content::SiteIsolationPolicy::AreCrossProcessFramesPossible()) {
This one is for webview (GuestsShouldUseCrossProcessFrames).

https://codereview.chromium.org/1208143002/diff/180001/extensions/renderer/di...
extensions/renderer/dispatcher.cc:521: if
(content::SiteIsolationPolicy::AreCrossProcessFramesPossible()) {
Ditto.

https://codereview.chromium.org/1208143002/diff/180001/extensions/renderer/di...
extensions/renderer/dispatcher.cc:1435: if
(content::SiteIsolationPolicy::AreCrossProcessFramesPossible()) {
Ditto.

[ncarter (slow), 2015-07-20 17:45:46]

Please take another look. I think I've addressed everything.

And to jam's comment: I've broken this up -- I opted to keep SiteIsolationPolicy
content-internal, moved the Guest case to a content/public, and the ForTesting
cases to browsertest_util. Unfortunately, this split obfuscates the options
available to the caller, and I don't know a good way to fix that while also
being strict about the content layer restrictions.

https://codereview.chromium.org/1208143002/diff/160001/content/browser/frame_...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/1208143002/diff/160001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:355: bool oopifs_possible =
SiteIsolationPolicy::AreCrossProcessFramesPossible();
On 2015/07/13 22:13:14, Charlie Reis wrote:
> On 2015/07/10 23:29:18, ncarter wrote:
> > On 2015/07/08 12:52:23, nasko (paris) wrote:
> > > nit: oopifs_enabled?
> > 
> > I'm sticking with _possible.
> 
> +1 to "possible," since it doesn't imply how widely we use them.

Leaving as Possible.

https://codereview.chromium.org/1208143002/diff/160001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1208143002/diff/160001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.cc:1987:
CHECK(SiteIsolationPolicy::AreCrossProcessFramesPossible());
On 2015/07/13 22:13:14, Charlie Reis wrote:
> On 2015/07/10 23:29:18, ncarter wrote:
> > On 2015/07/08 12:52:23, nasko (paris) wrote:
> > > I'd suggest consulting with Alex on this one. If we are going to be
creating
> > > proxies in regular mode Chrome because of the openers work he is doing,
then
> > > this is a change in behavior and the CHECK will be hit.
> > 
> > A frame with a proxy to its parent is an oopif by definition, right?
> 
> Calling this by default is a behavior change, but it looks like it should be
> safe (since GetProxyToParent returns null for the main frame and we won't even
> get to CommitPending for subframes).

Acknowledged.

https://codereview.chromium.org/1208143002/diff/160001/content/public/browser...
File content/public/browser/site_instance.h (right):

https://codereview.chromium.org/1208143002/diff/160001/content/public/browser...
content/public/browser/site_instance.h:27: // from (the site) and which tabs
have references to each other (the instance).
On 2015/07/08 23:42:59, Charlie Reis wrote:
> s/tabs/frames/

Done.

https://codereview.chromium.org/1208143002/diff/160001/content/public/browser...
content/public/browser/site_instance.h:32: // all tabs that might be able to
script each other because of how they were
On 2015/07/08 23:42:59, Charlie Reis wrote:
> s/tabs/frames/

Done.

https://codereview.chromium.org/1208143002/diff/160001/content/public/browser...
content/public/browser/site_instance.h:38: // PROCESS PER SITE INSTANCE (the
current default), SiteInstances are created
On 2015/07/08 23:42:59, Charlie Reis wrote:
> On 2015/07/08 12:52:23, nasko (paris) wrote:
> > Why all caps?
> 
> I'm ok with either caps or a numbered list.
> 
> Please use a colon rather than a comma after the close paren, though.

Done.

https://codereview.chromium.org/1208143002/diff/160001/content/public/browser...
content/public/browser/site_instance.h:46: // SITE PER PROCESS (currently
experimental) is the most granular process model,
On 2015/07/08 23:42:59, Charlie Reis wrote:
> Colon after close paren.

Done.

https://codereview.chromium.org/1208143002/diff/160001/content/public/browser...
content/public/browser/site_instance.h:47: // and is made possible by our
support for out-of-process iframes.  A subframe
On 2015/07/08 12:52:23, nasko (paris) wrote:
> nit: s/our support/supporting/

Not done.

https://codereview.chromium.org/1208143002/diff/160001/content/public/browser...
content/public/browser/site_instance.h:49: // containing document.  Cross-site
navigation of top-level frames or subframes
On 2015/07/08 12:52:23, nasko (paris) wrote:
> If the wording is "top-level frames or subframes" it seems to encompass the
> entire set of possible frames. Can it be dropped?

I mentioned "top level frames or subframes" explictly to draw attention to the
fact that not just top-level navs were involved in this process model, as a
contrast to process per site instance.

https://codereview.chromium.org/1208143002/diff/160001/content/public/browser...
content/public/browser/site_instance.h:49: // containing document.  Cross-site
navigation of top-level frames or subframes
On 2015/07/08 12:52:23, nasko (paris) wrote:
> nit: The usage of one or two spaces after a sentence end is inconsistent. I
> don't have strong preference for which one should be used, but I do like to
see
> things being consistent.

Done.

https://codereview.chromium.org/1208143002/diff/160001/content/public/browser...
content/public/browser/site_instance.h:52: // just one site, allowing the same
origin policy to be enforced by the IPC
On 2015/07/08 23:42:59, Charlie Reis wrote:
> On 2015/07/08 23:28:38, ncarter wrote:
> > On 2015/07/08 12:52:23, nasko (paris) wrote:
> > > We have a sandbox and an IPC channel to communicate outside of it, so "IPC
> > > sandbox" sounds a bit strange. 
> > 
> > What would you say instead? I was actually paraphrasing something you wrote
in
> > email that was equally imprecise, but which I really liked the gist of: "Our
> > Site Isolation team has been hard at work building the infrastructure for
> > out-of-process iframes (OOPIFs) so that Chrome's sandbox can finally help
> > enforce the Same Origin Policy."
> > 
> > It's not clear to me whether the term 'sandbox' includes the abilities that
> are
> > granted to a child process via IPC messages, or just the OS level
permissions
> > (other than the ability to do IPC communication) that are taken away from a
> > process by the OS-level sandbox.
> 
> I'd just say sandbox, or renderer sandbox if you want to clarify.  IPC is just
> the mechanism code inside the sandbox uses to communicate.  (That's covered
more
> in http://www.chromium.org/developers/design-documents/sandbox.)

Done.

https://codereview.chromium.org/1208143002/diff/180001/chrome/renderer/chrome...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/1208143002/diff/180001/chrome/renderer/chrome...
chrome/renderer/chrome_content_renderer_client.cc:1404: // |opener_frame|/its
ancestors.
On 2015/07/13 22:13:14, Charlie Reis wrote:
> Yeah, this one's a bit awkward, since there's a chance we might isolate web
> pages inside extensions before we isolate extensions inside web pages.  The
> local root idea might be on the right track; let's come back to it after this
CL
> lands.

Acknowledged.

https://codereview.chromium.org/1208143002/diff/180001/content/browser/child_...
File content/browser/child_process_security_policy_impl.cc (right):

https://codereview.chromium.org/1208143002/diff/180001/content/browser/child_...
content/browser/child_process_security_policy_impl.cc:574: return true;
On 2015/07/13 22:13:14, Charlie Reis wrote:
> This whole function is in kind of a weird state.  Robert introduced it way
back
> in https://codereview.chromium.org/11416121/, and Nasko disabled it when
> starting the OOPIF prototype in https://codereview.chromium.org/11772005.
> 
> It seems to me that we don't need it anymore.  It was meant as a way to block
> cross-site iframes before we had OOPIFs (https://crbug.com/159215).  With
> document blocking (https://crbug.com/268640) and killing renderers that commit
> bad URLs (https://crbug.com/85128), we don't need to also block the request in
> the ResourceLoader in this way.
> 
> I'd propose ripping it out entirely, either before or after this CL lands.

I've got another CL to rip this out completely.

https://codereview.chromium.org/1208143002/diff/180001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1208143002/diff/180001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.cc:923: //
TODO(nick,nasko): Should this be IsSwappedOutStateForbidden, to match
On 2015/07/13 22:13:14, Charlie Reis wrote:
> That sounds reasonable to me.

Acknowledged.

https://codereview.chromium.org/1208143002/diff/180001/content/browser/loader...
File content/browser/loader/cross_site_resource_handler.cc (right):

https://codereview.chromium.org/1208143002/diff/180001/content/browser/loader...
content/browser/loader/cross_site_resource_handler.cc:213: // skipped for WebUI
processes for now, since pages like the NTP host multiple
On 2015/07/13 22:13:14, Charlie Reis wrote:
> While we're here, let's fix this wording: the NTP is no longer WebUI, but we
do
> still need the check because chrome://settings has multiple "cross-site"
> chrome:// frames that don't work with OOPIFs yet.

Done.

https://codereview.chromium.org/1208143002/diff/180001/content/browser/site_i...
File content/browser/site_instance_impl.cc (right):

https://codereview.chromium.org/1208143002/diff/180001/content/browser/site_i...
content/browser/site_instance_impl.cc:350: // protection. If only some sites are
isolated, we need additional logic here
On 2015/07/13 22:13:14, Charlie Reis wrote:
> nit: Drop "here," since the additional logic will go in the places this is
> checked, not here where it is set.

Done.

https://codereview.chromium.org/1208143002/diff/180001/content/public/browser...
File content/public/browser/site_instance.h (right):

https://codereview.chromium.org/1208143002/diff/180001/content/public/browser...
content/public/browser/site_instance.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2015/07/13 22:13:14, Charlie Reis wrote:
> I left several comments in this class in patch set 9, which I think still
apply.

Done.

https://codereview.chromium.org/1208143002/diff/180001/content/public/browser...
content/public/browser/site_instance.h:28: // Here, a "site" is similar to the
page's origin, but includes only the
On 2015/07/13 22:13:14, Charlie Reis wrote:
> nit: Drop second comma if you're dropping "it."

Done.

https://codereview.chromium.org/1208143002/diff/180001/content/public/browser...
content/public/browser/site_instance.h:125: // TODO(nick): Does this name suck?
On 2015/07/13 22:13:15, Charlie Reis wrote:
> It's a bit long, but HasDedicatedProcess doesn't capture the case that the
site
> needs isolation and the process hasn't been created yet.  I'm ok with it as
is.

Done.

https://codereview.chromium.org/1208143002/diff/180001/extensions/renderer/di...
File extensions/renderer/dispatcher.cc (right):

https://codereview.chromium.org/1208143002/diff/180001/extensions/renderer/di...
extensions/renderer/dispatcher.cc:477: if
(content::SiteIsolationPolicy::AreCrossProcessFramesPossible()) {
On 2015/07/13 22:13:15, Charlie Reis wrote:
> This one is for webview (GuestsShouldUseCrossProcessFrames).

Done.

https://codereview.chromium.org/1208143002/diff/180001/extensions/renderer/di...
extensions/renderer/dispatcher.cc:521: if
(content::SiteIsolationPolicy::AreCrossProcessFramesPossible()) {
On 2015/07/13 22:13:15, Charlie Reis wrote:
> Ditto.

Done.

https://codereview.chromium.org/1208143002/diff/180001/extensions/renderer/di...
extensions/renderer/dispatcher.cc:1435: if
(content::SiteIsolationPolicy::AreCrossProcessFramesPossible()) {
On 2015/07/13 22:13:15, Charlie Reis wrote:
> Ditto.

Done.

[Charlie Reis, 2015-07-21 16:58:24]

A few minor comments below and it looks like patch set 16 failed to apply, but
otherwise LGTM!

https://codereview.chromium.org/1208143002/diff/300001/content/browser/frame_...
File content/browser/frame_host/navigation_controller_impl_browsertest.cc
(right):

https://codereview.chromium.org/1208143002/diff/300001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:1211: if
(AreAllSitesIsolatedForTesting()) {
On second thought, this is UseSubframeNavigationEntries.  Same for all the ones
below, but not the one above.

https://codereview.chromium.org/1208143002/diff/300001/content/browser/frame_...
File content/browser/frame_host/navigation_controller_impl_unittest.cc (right):

https://codereview.chromium.org/1208143002/diff/300001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl_unittest.cc:2088: if
(AreAllSitesIsolatedForTesting()) {
UseSubframeNavigationEntries.  Same for all the cases below.

https://codereview.chromium.org/1208143002/diff/300001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/1208143002/diff/300001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:1953: // TODO(nick):
This is overly conservative and isn't launchable. Move this
Can you file a bug for this so it doesn't fall between the cracks?

https://codereview.chromium.org/1208143002/diff/300001/content/content_common...
File content/content_common.gypi (right):

https://codereview.chromium.org/1208143002/diff/300001/content/content_common...
content/content_common.gypi:364: 'common/image_messages.h',
Merge conflict?

https://codereview.chromium.org/1208143002/diff/300001/content/public/browser...
File content/public/browser/site_instance.h (right):

https://codereview.chromium.org/1208143002/diff/300001/content/public/browser...
content/public/browser/site_instance.h:47: // model, and is made possible by our
support for out-of-process iframes.  A
nit: no comma

https://codereview.chromium.org/1208143002/diff/300001/content/public/browser...
content/public/browser/site_instance.h:55: // new tab, but not when navigating
across site boundaries.  (unless a process
nit: no period and just one space before open paren.

https://codereview.chromium.org/1208143002/diff/300001/content/public/browser...
content/public/browser/site_instance.h:60: // PROCESS PER SITE: we consolidate
all SiteInstances for a given site into the
nit: Capitalize We.

https://codereview.chromium.org/1208143002/diff/300001/content/public/test/br...
File content/public/test/browser_test_utils.cc (right):

https://codereview.chromium.org/1208143002/diff/300001/content/public/test/br...
content/public/test/browser_test_utils.cc:750: bool
AreAllSitesIsolatedForTesting() {
nit: Move above IsolateAllSitesForTesting so they're in the same order as .h
file.

[ncarter (slow), 2015-07-22 23:29:08]

All should be addressed. Things compile locally so I'm redoing the tryjobs, then
I'll add jam for a top-level approval.

https://codereview.chromium.org/1208143002/diff/300001/content/browser/frame_...
File content/browser/frame_host/navigation_controller_impl_browsertest.cc
(right):

https://codereview.chromium.org/1208143002/diff/300001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:1211: if
(AreAllSitesIsolatedForTesting()) {
On 2015/07/21 16:58:23, Charlie Reis wrote:
> On second thought, this is UseSubframeNavigationEntries.  Same for all the
ones
> below, but not the one above.

Done. I didn't actually switch over the FrameTreeVisualizer cases.

https://codereview.chromium.org/1208143002/diff/300001/content/browser/frame_...
File content/browser/frame_host/navigation_controller_impl_unittest.cc (right):

https://codereview.chromium.org/1208143002/diff/300001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl_unittest.cc:2088: if
(AreAllSitesIsolatedForTesting()) {
On 2015/07/21 16:58:23, Charlie Reis wrote:
> UseSubframeNavigationEntries.  Same for all the cases below.

Done.

https://codereview.chromium.org/1208143002/diff/300001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/1208143002/diff/300001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:1953: // TODO(nick):
This is overly conservative and isn't launchable. Move this
On 2015/07/21 16:58:23, Charlie Reis wrote:
> Can you file a bug for this so it doesn't fall between the cracks?

Done.

https://codereview.chromium.org/1208143002/diff/300001/content/content_common...
File content/content_common.gypi (right):

https://codereview.chromium.org/1208143002/diff/300001/content/content_common...
content/content_common.gypi:364: 'common/image_messages.h',
On 2015/07/21 16:58:24, Charlie Reis wrote:
> Merge conflict?

Very good catch. Done.

https://codereview.chromium.org/1208143002/diff/300001/content/public/browser...
File content/public/browser/site_instance.h (right):

https://codereview.chromium.org/1208143002/diff/300001/content/public/browser...
content/public/browser/site_instance.h:47: // model, and is made possible by our
support for out-of-process iframes.  A
On 2015/07/21 16:58:24, Charlie Reis wrote:
> nit: no comma

Done.

https://codereview.chromium.org/1208143002/diff/300001/content/public/browser...
content/public/browser/site_instance.h:55: // new tab, but not when navigating
across site boundaries.  (unless a process
On 2015/07/21 16:58:24, Charlie Reis wrote:
> nit: no period and just one space before open paren.

Done.

https://codereview.chromium.org/1208143002/diff/300001/content/public/browser...
content/public/browser/site_instance.h:60: // PROCESS PER SITE: we consolidate
all SiteInstances for a given site into the
On 2015/07/21 16:58:24, Charlie Reis wrote:
> nit: Capitalize We.

Done.

https://codereview.chromium.org/1208143002/diff/300001/content/public/test/br...
File content/public/test/browser_test_utils.cc (right):

https://codereview.chromium.org/1208143002/diff/300001/content/public/test/br...
content/public/test/browser_test_utils.cc:750: bool
AreAllSitesIsolatedForTesting() {
On 2015/07/21 16:58:24, Charlie Reis wrote:
> nit: Move above IsolateAllSitesForTesting so they're in the same order as .h
> file.

Done.

[nasko, 2015-07-23 14:29:21]

LGTM

https://codereview.chromium.org/1208143002/diff/160001/chrome/renderer/chrome...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/1208143002/diff/160001/chrome/renderer/chrome...
chrome/renderer/chrome_content_renderer_client.cc:1404: // |opener_frame|/its
ancestors.
On 2015/07/10 23:29:18, ncarter wrote:
> On 2015/07/08 12:52:22, nasko (paris) wrote:
> > I think this one can be using AreCrossProcessFramesPossible, as if not
> possible,
> > then we know that the top frame is in the same process and we can reach its
> > document.
> 
> Alex and I had previously chatted about this, and our sense was that this
> shouldn't be driven by the command line flag at all.
> AreCrossProcessFramesPossible seems like a particularly bad choice here (no
> reason to change the behavior globally if we're just enabled for one
> site/scheme).
> 
> I think the right thing is to basically find the localroot, so that why I left
> this one untouched. Does that make sense to you?

It does.

[ncarter (slow), 2015-07-23 16:43:07]

nick@chromium.org changed reviewers:
+ jam@chromium.org, lazyboy@chromium.org

[ncarter (slow), 2015-07-23 16:43:07]

nick@chromium.org changed required reviewers:
+ jam@chromium.org

[ncarter (slow), 2015-07-23 16:43:08]

+lazyboy to make sure you're okay the proposed "browser_plugin_guest_mode.h"
file, which wound up being the only non-test concept we needed to expose via
content/public

+jam for top-level OWNERS approval

[lazyboy, 2015-07-23 19:36:44]

lgtm

https://codereview.chromium.org/1208143002/diff/420001/content/public/common/...
File content/public/common/browser_plugin_guest_mode.h (right):

https://codereview.chromium.org/1208143002/diff/420001/content/public/common/...
content/public/common/browser_plugin_guest_mode.h:15: // Returns true if
<webview> should be implemented in terms of cross-process
nit: We generally *try* to avoid referring to <webview> inside content/ b/c it
is an extensions/ or chrome/ concept that happens to be driven by BrowserPlugin
at this moment. Maybe using "inner WebContents" is a bit better?

[jam, 2015-07-24 17:07:36]

https://codereview.chromium.org/1208143002/diff/420001/content/public/common/...
File content/public/common/browser_plugin_guest_mode.h (right):

https://codereview.chromium.org/1208143002/diff/420001/content/public/common/...
content/public/common/browser_plugin_guest_mode.h:19: static bool
UseCrossProcessFramesForGuests();
is there no other place to put this method, i.e. in other files related to
browser plugin? seems unfortunate to create this file just for one static
method.

[ncarter (slow), 2015-07-24 17:21:59]

On 2015/07/24 17:07:36, jam wrote:
>
https://codereview.chromium.org/1208143002/diff/420001/content/public/common/...
> File content/public/common/browser_plugin_guest_mode.h (right):
> 
>
https://codereview.chromium.org/1208143002/diff/420001/content/public/common/...
> content/public/common/browser_plugin_guest_mode.h:19: static bool
> UseCrossProcessFramesForGuests();
> is there no other place to put this method, i.e. in other files related to
> browser plugin? seems unfortunate to create this file just for one static
> method.

I looked around. I could not find any browserplugin code that was both common
and public (there's not much code, period, that is). These functions need to be
accessed both inside and out of content, as well as from renderer and browser
process code.

[jabdelmalek, 2015-07-27 17:14:28]

On 2015/07/24 17:21:59, ncarter wrote:
> On 2015/07/24 17:07:36, jam wrote:
> >
>
https://codereview.chromium.org/1208143002/diff/420001/content/public/common/...
> > File content/public/common/browser_plugin_guest_mode.h (right):
> > 
> >
>
https://codereview.chromium.org/1208143002/diff/420001/content/public/common/...
> > content/public/common/browser_plugin_guest_mode.h:19: static bool
> > UseCrossProcessFramesForGuests();
> > is there no other place to put this method, i.e. in other files related to
> > browser plugin? seems unfortunate to create this file just for one static
> > method.
> 
> I looked around. I could not find any browserplugin code that was both common
> and public (there's not much code, period, that is). These functions need to
be
> accessed both inside and out of content, as well as from renderer and browser
> process code.

ok, lgtm

[jam, 2015-07-27 17:21:12]

lgtm

[ncarter (slow), 2015-07-27 17:38:35]

The CQ bit was checked by nick@chromium.org

[ncarter (slow), 2015-07-27 17:38:35]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org,
nasko@chromium.org, jam@chromium.org, lazyboy@chromium.org,
jabdelmalek@google.com
Link to the patchset: https://codereview.chromium.org/1208143002/#ps460001
(title: "Fix new instances of kSitePerProcess.")

[commit-bot: I haz the power, 2015-07-27 17:40:01]

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

[commit-bot: I haz the power, 2015-07-27 17:52:15]

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

[commit-bot: I haz the power, 2015-07-27 17:52:17]

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

[ncarter (slow), 2015-07-27 19:12:15]

The CQ bit was checked by nick@chromium.org

[ncarter (slow), 2015-07-27 19:12:16]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org,
creis@chromium.org, lazyboy@chromium.org, jabdelmalek@google.com,
nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/1208143002/#ps480001
(title: "Attempt to fix compile.")

[commit-bot: I haz the power, 2015-07-27 19:12:46]

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

[commit-bot: I haz the power, 2015-07-27 21:51:17]

Committed patchset #25 (id:480001)

[commit-bot: I haz the power, 2015-07-27 21:51:51]

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