Remove GetPendingSiteInstance from NavigationControllerDelegate.

content/browser/frame_host/navigation_controller_impl.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 /*
  * Copyright (C) 2006, 2007, 2008, 2009 Apple Inc. All rights reserved.
  * Copyright (C) 2008 Nokia Corporation and/or its subsidiary(-ies)
  * Copyright (C) 2008, 2009 Torch Mobile Inc. All rights reserved.
  *     (http://www.torchmobile.com/)
  *
@@ skipping 795 matching lines @@
 
   // Save the previous state before we clobber it.
   if (GetLastCommittedEntry()) {
     details->previous_url = GetLastCommittedEntry()->GetURL();
     details->previous_entry_index = GetLastCommittedEntryIndex();
   } else {
     details->previous_url = GURL();
     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());
+  // If there is a pending entry at this point, it should have a SiteInstance,
+  // except for restored entries.
+  DCHECK(pending_entry_index_ == -1 ||
+         pending_entry_->site_instance() != nullptr ||
+         pending_entry_->restore_type() != NavigationEntryImpl::RESTORE_NONE);
+  if (pending_entry_index_ != -1 &&
+      pending_entry_->site_instance() == nullptr &&
+      pending_entry_->restore_type() != NavigationEntryImpl::RESTORE_NONE)

[Charlie Reis, 2015/07/30 16:55:14]

Let's simplify these to:

DCHECK(!pending_entry_ || pending_entry_->site_instance() || ...restore...);
if (pending_entry_ && pending_entry_->restore_type() != ...)
  ...set_restore_type...


Reasoning:
1) pending_entry_index_ isn't quite sufficient, even in the old check. 
pending_entry_ can be set even when the index is -1, which corresponds to a new
navigation.  Those won't be session restore cases, but they should have a
SiteInstance.
2) No need for the "== nullptr", since most of the code around here doesn't use
that (e.g., line 808, 871, etc).
3) The SiteInstance check in the if block isn't really necessary (though I agree
we had it before).  If it's a restore, we should clear the restore type, whether
it has a SiteInstance already or not.

[Fabrice (no longer in Chrome), 2015/07/30 17:18:41]

Done.
I find the explicit nullptr comparison to be easier to read and it seems some of
the more recent code and tests have adopted this - though I agree it's
semantically identical.

+    pending_entry_->set_restore_type(NavigationEntryImpl::RESTORE_NONE);
 
   // 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
   // doing a cross-site redirect navigation, we will do a similar thing.
   //
   // If this is an error load, we may have already removed the pending entry
   // when we got the notice of the load failure. If so, look at the copy of the
   // pending parameters that were saved.
   if (params.url_is_unreachable && failed_pending_entry_id_ != 0) {
@@ skipping 320 matching lines @@
   if (entry->update_virtual_url_with_url())
     UpdateVirtualURLToURL(entry, params.url);
 
   // The redirected to page should not inherit the favicon from the previous
   // page.
   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 ||
+  DCHECK(entry->site_instance() == nullptr ||
          entry->site_instance() == rfh->GetSiteInstance());
   entry->set_site_instance(
       static_cast<SiteInstanceImpl*>(rfh->GetSiteInstance()));
 
   entry->SetHasPostData(params.is_post);
   entry->SetPostID(params.post_id);
 
   // The entry we found in the list might be pending if the user hit
   // back/forward/reload. This load should commit it (since it's already in the
   // list, we can just discard the pending pointer).  We should also discard the
@@ skipping 545 matching lines @@
   }
 
   // This call does not support re-entrancy.  See http://crbug.com/347742.
   CHECK(!in_navigate_to_pending_entry_);
   in_navigate_to_pending_entry_ = true;
   bool success = NavigateToPendingEntryInternal(reload_type);
   in_navigate_to_pending_entry_ = false;
 
   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);
-  }
 }
 
 bool NavigationControllerImpl::NavigateToPendingEntryInternal(
     ReloadType reload_type) {
   DCHECK(pending_entry_);
   FrameTreeNode* root = delegate_->GetFrameTree()->root();
 
   // In default Chrome, there are no subframe FrameNavigationEntries.  Either
   // navigate the main frame or use the main frame's FrameNavigationEntry to
   // tell the indicated frame where to go.
@@ skipping 234 matching lines @@
     }
   }
 }
 
 void NavigationControllerImpl::SetGetTimestampCallbackForTest(
     const base::Callback<base::Time()>& get_timestamp_callback) {
   get_timestamp_callback_ = get_timestamp_callback;
 }
 
 }  // namespace content