Don't split off NTP subframes with TDI.

Don't split off NTP subframes with TDI.

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

Committed: https://crrev.com/7595a1bcff6400285c27c80c5e48902aad0560b5
Cr-Commit-Position: refs/heads/master@{#432951}

[Avi (use Gerrit), 2016-11-15 16:52:37]

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

[Avi (use Gerrit), 2016-11-15 16:52:38]

Description was changed from

==========
Don't split off NTP subframes with TDI.

BUG=665109
==========

to

==========
Don't split off NTP subframes with TDI.

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

[commit-bot: I haz the power, 2016-11-15 16:53:06]

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

[commit-bot: I haz the power, 2016-11-15 16:56:08]

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

[commit-bot: I haz the power, 2016-11-15 16:56:08]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Avi (use Gerrit), 2016-11-15 17:13:53]

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

[commit-bot: I haz the power, 2016-11-15 17:14:29]

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

[commit-bot: I haz the power, 2016-11-15 18:33:19]

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

[commit-bot: I haz the power, 2016-11-15 18:33:20]

Dry run: This issue passed the CQ dry run.

[Avi (use Gerrit), 2016-11-15 20:54:59]

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

[ncarter (slow), 2016-11-15 21:42:35]

Any chance of a test for this? I see local_ntp_browsertest.cc, not sure what
else there is.

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

https://codereview.chromium.org/2499313002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1423: if
(IsCurrentlySameSite(main_frame, dest_url))
I think you've done the right thing here, but double-check my reasoning:

The other way to do this, is to add the "|| (embedder_disabled_tdi)" to this
case. They're not equivalent though, since one takes
DoesSiteRequireDedicatedProcess() into account. Practically I think either would
work with the oopif modes that exist today, but the way you've implemented it
seems structurally safer, if for some reason we had an interaction between TDI
and a future isolation mode (e.g. running with something like
--isolate-sites-for-testing=chrome*://* and --top-document-isolation).

https://codereview.chromium.org/2499313002/diff/20001/content/public/browser/...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/2499313002/diff/20001/content/public/browser/...
content/public/browser/content_browser_client.h:251: SiteInstance*
site_instance);
WDYT about renaming |site_instance| -> |parent_site_instance| ?

[Avi (use Gerrit), 2016-11-16 20:10:10]

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

[commit-bot: I haz the power, 2016-11-16 20:10:45]

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

[Avi (use Gerrit), 2016-11-16 20:12:11]

This is more of a test of the chrome content browser client. Not sure where the
tests for that are, and so do we instantiate a new SiteInstance with a special
URL?

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

https://codereview.chromium.org/2499313002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1423: if
(IsCurrentlySameSite(main_frame, dest_url))
On 2016/11/15 21:42:35, ncarter wrote:
> I think you've done the right thing here, but double-check my reasoning:
> 
> The other way to do this, is to add the "|| (embedder_disabled_tdi)" to this
> case. They're not equivalent though, since one takes
> DoesSiteRequireDedicatedProcess() into account. Practically I think either
would
> work with the oopif modes that exist today, but the way you've implemented it
> seems structurally safer, if for some reason we had an interaction between TDI
> and a future isolation mode (e.g. running with something like
> --isolate-sites-for-testing=chrome*://* and --top-document-isolation).

Um, yes, this is exactly why I did it.

Really, though, I'm not sure why I put it down there instead of up here, but
that reasoning makes sense for leaving it there.

This whole function needs tests and cleanup, though.

https://codereview.chromium.org/2499313002/diff/20001/content/public/browser/...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/2499313002/diff/20001/content/public/browser/...
content/public/browser/content_browser_client.h:251: SiteInstance*
site_instance);
On 2016/11/15 21:42:35, ncarter wrote:
> WDYT about renaming |site_instance| -> |parent_site_instance| ?

Done.

[commit-bot: I haz the power, 2016-11-16 23:48:12]

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

[commit-bot: I haz the power, 2016-11-16 23:48:14]

Dry run: This issue passed the CQ dry run.

[ncarter (slow), 2016-11-16 23:50:11]

lgtm as-is

re the test question, a few browser tests do SetBrowserClientForTesting but
creating a subclass of ContentBrowserClient feels like a lot to maintain.

For --isolate-extensions testing, we did the trick of adding a contentshell-only
flag, --isolate-sites-for-testing, that was implemented in terms of the
contentclient hooks we added for --isolate-extensions. That approach (creating a
content-only analogue of a chrome feature) also feels like serious overkill
here.

That's why I was thinking of finding an existing NTP test in chrome/ that we can
easily add a EXPECT(# of renderer processes) to.  After all, this isn't the the
only process model behavior specific to the NTP.  I think it also has
process-per-site behavior (two independently opened NTP tabs will share a
process).

[Avi (use Gerrit), 2016-11-17 20:02:54]

The CQ bit was checked by avi@chromium.org

[commit-bot: I haz the power, 2016-11-17 20:04:49]

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

[commit-bot: I haz the power, 2016-11-17 20:24:32]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-11-17 20:29:46]

Description was changed from

==========
Don't split off NTP subframes with TDI.

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

to

==========
Don't split off NTP subframes with TDI.

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

Committed: https://crrev.com/7595a1bcff6400285c27c80c5e48902aad0560b5
Cr-Commit-Position: refs/heads/master@{#432951}
==========

[commit-bot: I haz the power, 2016-11-17 20:29:47]

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

[Charlie Reis, 2016-11-17 21:12:47]

Description was changed from

==========
Don't split off NTP subframes with TDI.

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

Committed: https://crrev.com/7595a1bcff6400285c27c80c5e48902aad0560b5
Cr-Commit-Position: refs/heads/master@{#432951}
==========

to

==========
Don't split off NTP subframes with TDI.

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

Committed: https://crrev.com/7595a1bcff6400285c27c80c5e48902aad0560b5
Cr-Commit-Position: refs/heads/master@{#432951}
==========