Fix the process creation problem. This forces transitions between...

Fix the process creation problem. This forces transitions between
BrowsingInstances when we force a transition for DOM UIs.

BUG=9364
TEST=see bug

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=13892

[Charlie Reis, 2009-04-16 05:57:43]

I think this is probably the easiest way to fix the major problem we have now,
and it looks good.

Here's one case I don't think it handles.  Suppose the user has two connected
tabs (e.g., both in Google Docs), and then goes to "chrome-ui://newtab" in one
of them.  This new logic will kick in, creating a new
SiteInstance/BrowsingInstance.  Then the user goes to www.google.com, which hits
this logic again, creating another new BrowsingInstance (and thus SiteInstance).
 However, this tab should really go back to being part of its previous
BrowsingInstance (and thus SiteInstance), as it would if the user clicked Back.

This is a pretty rare case in practice, since all menu items for DOM UI pages
tend to open in new tabs.  As far as I know, the user would have to manually
type in a chrome-ui URL to make it happen, so it might be ok if we don't handle
it perfectly (by splitting the BrowsingInstance).

My recommendation would be to go with this approach now, with a low priority bug
to refactor it later (CC'd or assigned to me).

Is there a good place to write UI tests that check whether processes are created
properly?  I'd been putting them in browser_uitest.cc (e.g.,
NullOpenerRedirectForksProcess).  Seems reasonable to test this: create two
NTPs, navigate them to the same site, and make sure the right number of
processes exist.

Thanks for writing this up!

http://codereview.chromium.org/67201/diff/1/3
File chrome/browser/tab_contents/render_view_host_manager.cc (right):

http://codereview.chromium.org/67201/diff/1/3#newcode387
Line 387: delegate_->GetControllerForRenderManager()->profile());
Yes, this is just the spot I was looking at for this, too.

[brettw, 2009-04-16 21:35:53]

New snap up. This includes a unittest.

[Charlie Reis, 2009-04-16 22:00:56]

LGTM-- just a few nits on comments.  Thanks!

http://codereview.chromium.org/67201/diff/11/13
File chrome/browser/tab_contents/render_view_host_manager.cc (right):

http://codereview.chromium.org/67201/diff/11/13#newcode385
Line 385: // BrowsingInstance.
I had trouble following the last sentence.  Perhaps this?

This addresses special cases where we use a single BrowsingInstance for all
pages of a certain type (e.g., New Tab Pages), keeping them in the same process.

http://codereview.chromium.org/67201/diff/11/12
File chrome/browser/tab_contents/render_view_host_manager.h (right):

http://codereview.chromium.org/67201/diff/11/12#newcode191
Line 191: // be the same. It also forces new SiteInstances for these
transitions.
"new SiteInstances and BrowsingInstances for these transitions."

Maybe also point out examples of when this is this case?  DOM UI pages, and soon
extensions (according to Matt).

http://codereview.chromium.org/67201/diff/11/15
File chrome/browser/tab_contents/render_view_host_manager_unittest.cc (right):

http://codereview.chromium.org/67201/diff/11/15#newcode14
Line 14: // then do that sam ething in another tab, that the two resulting pages
have
Typo: same thing

http://codereview.chromium.org/67201/diff/11/15#newcode31
Line 31: // Load the two URLs in the second tab. Note that the first situation
create
"situation create" -> "navigation creates"
(I was confused thinking that "situation" referred to the navigations in the
first tab.)