Clean up ChromeContentBrowserClient::ShouldSwapProcessesForNavigation.

Clean up ChromeContentBrowserClient::ShouldSwapProcessesForNavigation.

The current logic forces a BrowsingInstance swap when going to/from
hosted apps, which breaks postMessage.  Rely on RenderViewHostManager
to do a SiteInstance swap instead.

BUG=123007
TEST=postMessage works after navigating a hosted app popup cross-process.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=238304

[Charlie Reis, 2013-11-26 05:46:16]

@kalman, would you be able to take a look?  Normally I would ask @mpcomplete but
it looks like he's OOO.

I'm cleaning up some of the process swap logic in http://crbug.com/123007 and
https://codereview.chromium.org/76063007, but I wanted to split this part out
separately since it's a behavior change.

The main issue is that this function was forcing a BrowsingInstance swap when
going to/from hosted apps, which meant postMessage between hosted apps and other
related tabs was broken.  (A BrowsingInstance is a group of related
SiteInstances that can postMessage each other.)

[Charlie Reis, 2013-12-02 20:02:54]

Matt, can you review instead?

[Matt Perry, 2013-12-02 21:45:20]

LGTM, but see below about whether it could be simpler.

https://codereview.chromium.org/86973002/diff/20001/chrome/browser/chrome_con...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/86973002/diff/20001/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:1325: bool
ChromeContentBrowserClient::ShouldSwapProcessesForNavigation(
So this method really decides whether to swap BrowsingInstances? Would it
instead be simpler to have it swap SiteInstances, so postMessage would work
without the extra logic?

[Charlie Reis, 2013-12-02 21:59:03]

https://codereview.chromium.org/86973002/diff/20001/chrome/browser/chrome_con...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/86973002/diff/20001/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:1325: bool
ChromeContentBrowserClient::ShouldSwapProcessesForNavigation(
On 2013/12/02 21:45:20, Matt Perry wrote:
> So this method really decides whether to swap BrowsingInstances? Would it
> instead be simpler to have it swap SiteInstances, so postMessage would work
> without the extra logic?

I think we do want this method to swap BrowsingInstances for extensions, just
not for hosted apps.

Also, I've actually already renamed it to
ShouldSwapBrowsingInstancesForNavigation.  :)  I just rebased so you can see the
change.  In the past, this method was more about forcing a process swap even
when in a mode that cross-site navigations don't cause process swaps (e.g.,
process-per-tab).  That happened to translate as a BrowsingInstance swap, and
now that's explicit.

(Note that the existing navigation logic in RenderViewHostManager would already
cause a SiteInstance swap for extensions since they look like different sites,
but the intent was to put them in their own BrowsingInstances.)

[Matt Perry, 2013-12-02 22:01:44]

OK, sounds good. LGTM

[commit-bot: I haz the power, 2013-12-02 22:04:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/creis@chromium.org/86973002/40001

[not at google - send to devlin, 2013-12-02 23:40:09]

Sorry that I didn't manage to get to this.

[commit-bot: I haz the power, 2013-12-03 00:08:32]

Retried try job too often on mac_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-12-03 02:01:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/creis@chromium.org/86973002/40001

[commit-bot: I haz the power, 2013-12-03 05:16:11]

Change committed as 238304

[nasko, 2013-12-03 18:06:58]

https://codereview.chromium.org/86973002/diff/40001/chrome/browser/chrome_con...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/86973002/diff/40001/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:1330: // in
RenderViewHostManager to decide when to swap.
drive-by nit: There is no RenderViewHostManager anymore. Same in the new comment
below.

[Charlie Reis, 2013-12-03 19:22:55]

https://codereview.chromium.org/86973002/diff/40001/chrome/browser/chrome_con...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/86973002/diff/40001/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:1330: // in
RenderViewHostManager to decide when to swap.
On 2013/12/03 18:06:59, nasko wrote:
> drive-by nit: There is no RenderViewHostManager anymore. Same in the new
comment
> below.

Oops!  I'll fix those comments in another CL.  Thanks for catching it.