Modify --isolate-extensions to not isolate hosted apps.

Modify --isolate-extensions to not isolate hosted apps. This requires
shifting some of the burden of isolation logic from SiteIsolationPolicy
(which is content/common) to SiteInstanceImpl (which can call into
content/browser specific code).

Make the SiteDetails UMA stats match this behavior.

Doing things this way requires content to call back into the
embedder during each navigation policy check, which means
we're retreating from the "register isolated schemes" data flow.
This new approach will be flexible enough to support
"--isolate-hosted-apps" if we want to implement that in the
future.

BUG=535073
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
TEST=browser_tests

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

[ncarter (slow), 2015-10-12 22:24:28]

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

[ncarter (slow), 2015-10-12 22:24:29]

Charlie, this is ready for you now

[Charlie Reis, 2015-10-12 23:13:28]

Nice.  (Aren't hosted apps fun?)  Sorry you had to undo the scheme whitelist,
but I think this should work out ok.

LGTM.

https://codereview.chromium.org/1377933004/diff/100001/chrome/browser/site_de...
File chrome/browser/site_details.cc (right):

https://codereview.chromium.org/1377933004/diff/100001/chrome/browser/site_de...
chrome/browser/site_details.cc:46: return extension &&
!extension->is_hosted_app();
This is fairly similar to
ChromeContentBrowserClientExtensionsPart::DoesSiteRequireDedicatedProcess, but I
think I'm ok with having separate copies rather than sharing an implementation. 
This one can't depend on the command line flag, and otherwise we're just
checking if it's a hosted app.

https://codereview.chromium.org/1377933004/diff/100001/chrome/browser/site_de...
File chrome/browser/site_details_browsertest.cc (right):

https://codereview.chromium.org/1377933004/diff/100001/chrome/browser/site_de...
chrome/browser/site_details_browsertest.cc:695: // isolates hosted apps too. It
shouldn't. http://crbug.com/535073
Don't forget to update this comment.

https://codereview.chromium.org/1377933004/diff/100001/chrome/browser/site_de...
chrome/browser/site_details_browsertest.cc:740: // Reload the same two pages.
...and verify that the hosted app still is not isolated.

https://codereview.chromium.org/1377933004/diff/100001/content/public/common/...
File content/public/common/content_client.h (right):

https://codereview.chromium.org/1377933004/diff/100001/content/public/common/...
content/public/common/content_client.h:151: // used by the content layer.
nit: Let's say a bit more of what this implies (e.g., that content will call
DoesSiteRequireDedicatedProcess when making process model decisions).

[ncarter (slow), 2015-10-13 20:21:47]

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

[ncarter (slow), 2015-10-13 20:22:11]

https://codereview.chromium.org/1377933004/diff/100001/chrome/browser/site_de...
File chrome/browser/site_details.cc (right):

https://codereview.chromium.org/1377933004/diff/100001/chrome/browser/site_de...
chrome/browser/site_details.cc:46: return extension &&
!extension->is_hosted_app();
On 2015/10/12 23:13:28, Charlie Reis wrote:
> This is fairly similar to
> ChromeContentBrowserClientExtensionsPart::DoesSiteRequireDedicatedProcess, but
I
> think I'm ok with having separate copies rather than sharing an
implementation. 
> This one can't depend on the command line flag, and otherwise we're just
> checking if it's a hosted app.

I'd thought about this too, and think that duplication is the right tradeoff,
for three reasons:

(1) There's a behavioral difference, in that DoesSiteRequireDedicatedProcess
also looks at the command line flag values. Theoretically, though, you could
have the flag state passed as a parameter to the
ChromeContentBrowserClientExtensionsPart function.

(2) I think it would be pretty weird for
ChromeContentBrowserClientExtensionsPart to be invoked without the content layer
originating the call (it's flat out impossible for chrome to call into
ChromeContentBrowser, for example).

(3) I think of this file, really, as a mock implementation of SiteInstanceImpl,
so duplication of logic just isn't that evil.

https://codereview.chromium.org/1377933004/diff/100001/chrome/browser/site_de...
File chrome/browser/site_details_browsertest.cc (right):

https://codereview.chromium.org/1377933004/diff/100001/chrome/browser/site_de...
chrome/browser/site_details_browsertest.cc:695: // isolates hosted apps too. It
shouldn't. http://crbug.com/535073
On 2015/10/12 23:13:28, Charlie Reis wrote:
> Don't forget to update this comment.

Done.

https://codereview.chromium.org/1377933004/diff/100001/chrome/browser/site_de...
chrome/browser/site_details_browsertest.cc:740: // Reload the same two pages.
On 2015/10/12 23:13:28, Charlie Reis wrote:
> ...and verify that the hosted app still is not isolated.

Done. Thanks for asking about this -- I actually think it's worthwhile now to
add --site-per-process asserts to this test (to make sure that hosted apps
aren't exempt from isolation in site-per-process) which I've done.

https://codereview.chromium.org/1377933004/diff/100001/content/public/common/...
File content/public/common/content_client.h (right):

https://codereview.chromium.org/1377933004/diff/100001/content/public/common/...
content/public/common/content_client.h:151: // used by the content layer.
On 2015/10/12 23:13:28, Charlie Reis wrote:
> nit: Let's say a bit more of what this implies (e.g., that content will call
> DoesSiteRequireDedicatedProcess when making process model decisions).

Done.

[ncarter (slow), 2015-10-13 20:23:23]

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

[ncarter (slow), 2015-10-13 20:23:24]

+jam, for chrome/ OWNERS

[commit-bot: I haz the power, 2015-10-13 20:25:13]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1377933004/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1377933004/120001

[Charlie Reis, 2015-10-13 21:20:04]

Great.  Still LGTM.

https://codereview.chromium.org/1377933004/diff/100001/chrome/browser/site_de...
File chrome/browser/site_details_browsertest.cc (right):

https://codereview.chromium.org/1377933004/diff/100001/chrome/browser/site_de...
chrome/browser/site_details_browsertest.cc:740: // Reload the same two pages.
On 2015/10/13 20:22:11, ncarter wrote:
> On 2015/10/12 23:13:28, Charlie Reis wrote:
> > ...and verify that the hosted app still is not isolated.
> 
> Done. Thanks for asking about this -- I actually think it's worthwhile now to
> add --site-per-process asserts to this test (to make sure that hosted apps
> aren't exempt from isolation in site-per-process) which I've done.

Nice, thanks!

[commit-bot: I haz the power, 2015-10-13 21:59:46]

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

[commit-bot: I haz the power, 2015-10-13 21:59:47]

Dry run: This issue passed the CQ dry run.

[jam, 2015-10-14 15:22:43]

lgtm

[ncarter (slow), 2015-10-14 16:21:45]

The CQ bit was checked by nick@chromium.org

[commit-bot: I haz the power, 2015-10-14 16:22:33]

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

[commit-bot: I haz the power, 2015-10-14 16:27:21]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-10-14 16:28:17]

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