Remove GetPendingSiteInstance from NavigationControllerDelegate.

content/browser/frame_host/navigation_controller_impl.cc

Index: content/browser/frame_host/navigation_controller_impl.cc
diff --git a/content/browser/frame_host/navigation_controller_impl.cc b/content/browser/frame_host/navigation_controller_impl.cc
index 01da9c130f57d116b8f931aa1b342e906fe7d8d5..41485c4d57dc8916a9db32ceafe5ef9cadac824e 100644
--- a/content/browser/frame_host/navigation_controller_impl.cc
+++ b/content/browser/frame_host/navigation_controller_impl.cc
@@ -813,11 +813,6 @@ bool NavigationControllerImpl::RendererDidNavigate(
     details->previous_entry_index = -1;
   }
 
-  // If we have a pending entry at this point, it should have a SiteInstance.
-  // Restored entries start out with a null SiteInstance, but we should have
-  // assigned one in NavigateToPendingEntry.
-  DCHECK(pending_entry_index_ == -1 || pending_entry_->site_instance());

[Charlie Reis, 2015/07/29 23:58:27]

I'd prefer not to remove this DCHECK.  Can we add a condition allowing restored
entries to have a null SiteInstance instead?

[Fabrice (no longer in Chrome), 2015/07/30 15:59:26]

Done.
I moved the restore_type here too.

-
   // If we are doing a cross-site reload, we need to replace the existing
   // navigation entry, not add another entry to the history. This has the side
   // effect of removing forward browsing history, if such existed. Or if we are
@@ -1155,10 +1150,6 @@ void NavigationControllerImpl::RendererDidNavigateToExistingPage(
   if (ui::PageTransitionIsRedirect(params.transition))
     entry->GetFavicon() = FaviconStatus();
 
-  // The site instance will normally be the same except during session restore,
-  // when no site instance will be assigned.
-  DCHECK(entry->site_instance() == NULL ||
-         entry->site_instance() == rfh->GetSiteInstance());

[Charlie Reis, 2015/07/29 23:58:27]

This definitely needs to be kept-- a NavigationEntry should not change its
SiteInstance once it has one.  I've been thinking about upgrading it to a CHECK,
but I think my location.replace CL has to land first.

[Fabrice (no longer in Chrome), 2015/07/30 15:59:26]

Done.

   entry->set_site_instance(
       static_cast<SiteInstanceImpl*>(rfh->GetSiteInstance()));
 
@@ -1723,15 +1714,6 @@ void NavigationControllerImpl::NavigateToPendingEntry(ReloadType reload_type) {
 
   if (!success)
     DiscardNonCommittedEntries();
-
-  // If the entry is being restored and doesn't have a SiteInstance yet, fill
-  // it in now that we know. This allows us to find the entry when it commits.
-  if (pending_entry_ && !pending_entry_->site_instance() &&
-      pending_entry_->restore_type() != NavigationEntryImpl::RESTORE_NONE) {
-    pending_entry_->set_site_instance(static_cast<SiteInstanceImpl*>(
-        delegate_->GetPendingSiteInstance()));
-    pending_entry_->set_restore_type(NavigationEntryImpl::RESTORE_NONE);

[Charlie Reis, 2015/07/29 23:58:27]

When will this be cleared?  We don't want to leave the restore type set forever.

If we're not catching that today, we should add a test for it.  Maybe it can be
cleared in RendererDidNavigate after the SiteInstance DCHECK (around line 819)?

[Fabrice (no longer in Chrome), 2015/07/30 15:59:26]

We are catching it, I just removed the wrong checks in the tests. I restored
these and moved the restore_type after the DCHECK, as you suggested. Thanks!

-  }
 }
 
 bool NavigationControllerImpl::NavigateToPendingEntryInternal(