Set the site instance in all cases when we start a new load. If you navigate ...

Set the site instance in all cases when we start a new load. If you navigate away from the new tab page (or another DOMUI page) before the DOMUI page commits, the subsequent page will inherit the SiteInstance of the original page.

BUG=12718, 40575
TEST=included unit test

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

[Charlie Reis, 2010-04-07 16:51:54]

Thanks Brett!  A suggestion inline to make this fix more specific.

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

http://codereview.chromium.org/1519025/diff/1/3#oldcode355
chrome/browser/tab_contents/render_view_host_manager.cc:355: if
(entry.restore_type() != NavigationEntry::RESTORE_NONE)
I'd rather keep this if and add an "or" clause to check whether
DOMUIFactory::HasDOMUIScheme(dest_url).  This will still fix the bug: navigating
to the NTP will immediately set the site, preventing the SiteInstance from being
reused.

I'd argue for this because your current change doesn't fix the "set site lazily"
problem in general-- it only affects the newly opened tab case.  It's an
improvement if we're going to a NTP and don't want the possibility of a race,
but it's worse if we're going to "mail/" and it redirects to "mail.google.com". 
In that case, the SiteInstance gets mislabeled as "mail", which won't ever get
used again.

I can file a new bug to add better SiteInstance support for redirects (since the
current approach has problems too), but if we're doing a point-fix, I'd like to
avoid making any cases worse.

(Also, please add a note to the comment on line 339 about making an exception
for DOMUI pages, just like we do for session restore.)

http://codereview.chromium.org/1519025/diff/1/2
File chrome/browser/tab_contents/render_view_host_manager_unittest.cc (right):

http://codereview.chromium.org/1519025/diff/1/2#newcode177
chrome/browser/tab_contents/render_view_host_manager_unittest.cc:177:
EXPECT_TRUE(host->site_instance()->has_site());
With my other suggestion, this should get rolled back.  We should add a similar
(smaller) test where we navigate to chrome://newtab/ and verify that has_site()
is true immediately after the call to manager.Navigate(...).

[brettw, 2010-04-07 17:49:54]

New snap up, thanks for the comments.

[Charlie Reis, 2010-04-07 17:55:23]

Fantastic-- LGTM.