Remove dependency on NavigationEntry to get a SiteInstance

Remove the dependency on a NavigationEntry in
RenderFrameHostManager::ShouldSwapBrowsingInstancesForNavigation and
RenderFrameHostManager::GetSiteInstanceForEntry. This is needed by the
PlzNavigate project, as the SiteInstance to use is determined when the
navigation is about to commit. At this time, there is no convinient access to a
SNavigationEntry.

BUG=376082

Review URL: https://codereview.chromium.org/471603002

Cr-Commit-Position: refs/heads/master@{#289681}
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@289681 0039d316-1c4b-4281-b951-d872f2087c98

content/browser/frame_host/render_frame_host_manager.cc

@@ -645,62 +645,53 @@ bool RenderFrameHostManager::ShouldTransitionCrossSite() {
 }
 
 bool RenderFrameHostManager::ShouldSwapBrowsingInstancesForNavigation(
-    const NavigationEntry* current_entry,
-    const NavigationEntryImpl* new_entry) const {
-  DCHECK(new_entry);
-
+    const GURL& current_effective_url,
+    bool current_is_view_source_mode,
+    SiteInstance* new_site_instance,
+    const GURL& new_effective_url,
+    bool new_is_view_source_mode) const {
   // If new_entry already has a SiteInstance, assume it is correct.  We only
   // need to force a swap if it is in a different BrowsingInstance.
-  if (new_entry->site_instance()) {
-    return !new_entry->site_instance()->IsRelatedSiteInstance(
+  if (new_site_instance) {
+    return !new_site_instance->IsRelatedSiteInstance(
         render_frame_host_->GetSiteInstance());
   }
 
   // Check for reasons to swap processes even if we are in a process model that
   // doesn't usually swap (e.g., process-per-tab).  Any time we return true,
   // the new_entry will be rendered in a new SiteInstance AND BrowsingInstance.
-
-  // We use the effective URL here, since that's what is used in the
-  // SiteInstance's site and when we later call IsSameWebSite.  If there is no
-  // current_entry, check the current SiteInstance's site, which might already
-  // be committed to a Web UI URL (such as the NTP).
   BrowserContext* browser_context =
       delegate_->GetControllerForRenderManager().GetBrowserContext();
-  const GURL& current_url = (current_entry) ?
-      SiteInstanceImpl::GetEffectiveURL(browser_context,
-                                        current_entry->GetURL()) :
-      render_frame_host_->GetSiteInstance()->GetSiteURL();
-  const GURL& new_url = SiteInstanceImpl::GetEffectiveURL(browser_context,
-                                                          new_entry->GetURL());
 
   // Don't force a new BrowsingInstance for debug URLs that are handled in the
   // renderer process, like javascript: or chrome://crash.
-  if (IsRendererDebugURL(new_url))
+  if (IsRendererDebugURL(new_effective_url))
     return false;
 
   // For security, we should transition between processes when one is a Web UI
   // page and one isn't.
   if (WebUIControllerFactoryRegistry::GetInstance()->UseWebUIForURL(
-          browser_context, current_url)) {
+          browser_context, current_effective_url)) {
     // If so, force a swap if destination is not an acceptable URL for Web UI.
     // Here, data URLs are never allowed.
     if (!WebUIControllerFactoryRegistry::GetInstance()->IsURLAcceptableForWebUI(
-            browser_context, new_url)) {
+            browser_context, new_effective_url)) {
       return true;
     }
   } else {
     // Force a swap if it's a Web UI URL.
     if (WebUIControllerFactoryRegistry::GetInstance()->UseWebUIForURL(
-            browser_context, new_url)) {
+            browser_context, new_effective_url)) {
       return true;
     }
   }
 
-  // Check with the content client as well.  Important to pass current_url here,
-  // which uses the SiteInstance's site if there is no current_entry.
+  // Check with the content client as well.  Important to pass
+  // current_effective_url here, which uses the SiteInstance's site if there is
+  // no current_entry.
   if (GetContentClient()->browser()->ShouldSwapBrowsingInstancesForNavigation(
           render_frame_host_->GetSiteInstance(),
-          current_url, new_url)) {
+          current_effective_url, new_effective_url)) {
     return true;
   }
 
@@ -708,8 +699,7 @@ bool RenderFrameHostManager::ShouldSwapBrowsingInstancesForNavigation(
   // without screwing up the session history sometimes (when navigating between
   // "view-source:http://foo.com/" and "http://foo.com/", Blink doesn't treat
   // it as a new navigation). So require a BrowsingInstance switch.
-  if (current_entry &&
-      current_entry->IsViewSourceMode() != new_entry->IsViewSourceMode())
+  if (current_is_view_source_mode != new_is_view_source_mode)
     return true;
 
   return false;
@@ -727,24 +717,26 @@ bool RenderFrameHostManager::ShouldReuseWebUI(
           controller.GetBrowserContext(), new_entry->GetURL()));
 }
 
-SiteInstance* RenderFrameHostManager::GetSiteInstanceForEntry(
-    const NavigationEntryImpl& entry,
+SiteInstance* RenderFrameHostManager::GetSiteInstanceForURL(
+    const GURL& dest_url,
+    SiteInstance* dest_instance,
+    PageTransition dest_transition,
+    bool dest_is_restore,
+    bool dest_is_view_source_mode,
     SiteInstance* current_instance,
     bool force_browsing_instance_swap) {
-  // Determine which SiteInstance to use for navigating to |entry|.
-  const GURL& dest_url = entry.GetURL();
   NavigationControllerImpl& controller =
       delegate_->GetControllerForRenderManager();
   BrowserContext* browser_context = controller.GetBrowserContext();
 
   // If the entry has an instance already we should use it.
-  if (entry.site_instance()) {
+  if (dest_instance) {
     // If we are forcing a swap, this should be in a different BrowsingInstance.
     if (force_browsing_instance_swap) {
-      CHECK(!entry.site_instance()->IsRelatedSiteInstance(
+      CHECK(!dest_instance->IsRelatedSiteInstance(
                 render_frame_host_->GetSiteInstance()));
     }
-    return entry.site_instance();
+    return dest_instance;
   }
 
   // If a swap is required, we need to force the SiteInstance AND
@@ -763,8 +755,7 @@ SiteInstance* RenderFrameHostManager::GetSiteInstanceForEntry(
   //       RenderViews in response to a link click.
   //
   if (CommandLine::ForCurrentProcess()->HasSwitch(switches::kProcessPerSite) &&
-      PageTransitionCoreTypeIs(entry.GetTransitionType(),
-                               PAGE_TRANSITION_GENERATED)) {
+      PageTransitionCoreTypeIs(dest_transition, PAGE_TRANSITION_GENERATED)) {
     return current_instance;
   }
 
@@ -806,7 +797,7 @@ SiteInstance* RenderFrameHostManager::GetSiteInstanceForEntry(
     // TODO(nasko): This is the same condition as later in the function. This
     // should be taken into account when refactoring this method as part of
     // http://crbug.com/123007.
-    if (entry.IsViewSourceMode())
+    if (dest_is_view_source_mode)
       return SiteInstance::CreateForURL(browser_context, dest_url);
 
     // If we are navigating from a blank SiteInstance to a WebUI, make sure we
@@ -830,7 +821,7 @@ SiteInstance* RenderFrameHostManager::GetSiteInstanceForEntry(
     // renderers created for particular chrome urls (e.g. the chrome-native://
     // scheme) can be reused for subsequent navigations in the same WebContents.
     // See http://crbug.com/386542.
-    if (entry.restore_type() != NavigationEntryImpl::RESTORE_NONE &&
+    if (dest_is_restore &&
         GetContentClient()->browser()->ShouldAssignSiteForURL(dest_url)) {
       current_site_instance->SetSite(dest_url);
     }
@@ -873,7 +864,7 @@ SiteInstance* RenderFrameHostManager::GetSiteInstanceForEntry(
   // TODO(creis): Refactor this method so this duplicated code isn't needed.
   // See http://crbug.com/123007.
   if (current_entry &&
-      current_entry->IsViewSourceMode() != entry.IsViewSourceMode() &&
+      current_entry->IsViewSourceMode() != dest_is_view_source_mode &&
       !IsRendererDebugURL(dest_url)) {
     return SiteInstance::CreateForURL(browser_context, dest_url);
   }
@@ -1332,10 +1323,31 @@ RenderFrameHostImpl* RenderFrameHostManager::UpdateStateForNavigate(
   // process-per-tab model, such as WebUI pages.
   const NavigationEntry* current_entry =
       delegate_->GetLastCommittedNavigationEntryForRenderManager();
+  BrowserContext* browser_context =
+      delegate_->GetControllerForRenderManager().GetBrowserContext();
+  const GURL& current_effective_url = current_entry ?
+      SiteInstanceImpl::GetEffectiveURL(browser_context,
+                                        current_entry->GetURL()) :
+      render_frame_host_->GetSiteInstance()->GetSiteURL();
+  bool current_is_view_source_mode = current_entry ?
+      current_entry->IsViewSourceMode() : entry.IsViewSourceMode();
   bool force_swap = !is_guest_scheme &&
-      ShouldSwapBrowsingInstancesForNavigation(current_entry, &entry);
-  if (!is_guest_scheme && (ShouldTransitionCrossSite() || force_swap))
-    new_instance = GetSiteInstanceForEntry(entry, current_instance, force_swap);
+      ShouldSwapBrowsingInstancesForNavigation(
+          current_effective_url,
+          current_is_view_source_mode,
+          entry.site_instance(),
+          SiteInstanceImpl::GetEffectiveURL(browser_context, entry.GetURL()),
+          entry.IsViewSourceMode());
+  if (!is_guest_scheme && (ShouldTransitionCrossSite() || force_swap)) {
+    new_instance = GetSiteInstanceForURL(
+        entry.GetURL(),
+        entry.site_instance(),
+        entry.GetTransitionType(),
+        entry.restore_type() != NavigationEntryImpl::RESTORE_NONE,
+        entry.IsViewSourceMode(),
+        current_instance,
+        force_swap);
+  }
 
   // If force_swap is true, we must use a different SiteInstance.  If we didn't,
   // we would have two RenderFrameHosts in the same SiteInstance and the same

content/browser/frame_host/render_frame_host_manager.h

@@ -386,15 +386,25 @@ class CONTENT_EXPORT RenderFrameHostManager : public NotificationObserver {
   // switch.  Can be overridden in unit tests.
   bool ShouldTransitionCrossSite();
 
-  // Returns true if for the navigation from |current_entry| to |new_entry|,
-  // a new SiteInstance and BrowsingInstance should be created (even if we are
-  // in a process model that doesn't usually swap).  This forces a process swap
-  // and severs script connections with existing tabs.  Cases where this can
-  // happen include transitions between WebUI and regular web pages.
-  // Either of the entries may be NULL.
+  // Returns true if for the navigation from |current_effective_url| to
+  // |new_effective_url|, a new SiteInstance and BrowsingInstance should be
+  // created (even if we are in a process model that doesn't usually swap).
+  // This forces a process swap and severs script connections with existing
+  // tabs.  Cases where this can happen include transitions between WebUI and
+  // regular web pages. |new_site_instance| may be null.
+  // If there is no current NavigationEntry, then |current_is_view_source_mode|
+  // should be the same as |new_is_view_source_mode|.
+  //
+  // We use the effective URL here, since that's what is used in the
+  // SiteInstance's site and when we later call IsSameWebSite.  If there is no
+  // current NavigationEntry, check the current SiteInstance's site, which might
+  // already be committed to a Web UI URL (such as the NTP).
   bool ShouldSwapBrowsingInstancesForNavigation(
-      const NavigationEntry* current_entry,
-      const NavigationEntryImpl* new_entry) const;
+      const GURL& current_effective_url,
+      bool current_is_view_source_mode,
+      SiteInstance* new_site_instance,
+      const GURL& new_effective_url,
+      bool new_is_view_source_mode) const;
 
   // Returns true if it is safe to reuse the current WebUI when navigating from
   // |current_entry| to |new_entry|.
@@ -402,12 +412,16 @@ class CONTENT_EXPORT RenderFrameHostManager : public NotificationObserver {
       const NavigationEntry* current_entry,
       const NavigationEntryImpl* new_entry) const;
 
-  // Returns an appropriate SiteInstance object for the given NavigationEntry,
+  // Returns an appropriate SiteInstance object for the given |dest_url|,
   // possibly reusing the current SiteInstance.  If --process-per-tab is used,
   // this is only called when ShouldSwapBrowsingInstancesForNavigation returns
-  // true.
-  SiteInstance* GetSiteInstanceForEntry(
-      const NavigationEntryImpl& entry,
+  // true. |dest_instance| will be used if it is not null.
+  SiteInstance* GetSiteInstanceForURL(
+      const GURL& dest_url,
+      SiteInstance* dest_instance,
+      PageTransition dest_transition,
+      bool dest_is_restore,
+      bool dest_is_view_source_mode,
       SiteInstance* current_instance,
       bool force_browsing_instance_swap);
 

content/browser/frame_host/render_frame_host_manager_unittest.cc

@@ -323,8 +323,21 @@ class RenderFrameHostManagerTest
   bool ShouldSwapProcesses(RenderFrameHostManager* manager,
                            const NavigationEntryImpl* current_entry,
                            const NavigationEntryImpl* new_entry) const {
-    return manager->ShouldSwapBrowsingInstancesForNavigation(current_entry,
-                                                             new_entry);
+    CHECK(new_entry);
+    BrowserContext* browser_context =
+        manager->delegate_->GetControllerForRenderManager().GetBrowserContext();
+    const GURL& current_effective_url = current_entry ?
+        SiteInstanceImpl::GetEffectiveURL(browser_context,
+                                          current_entry->GetURL()) :
+        manager->render_frame_host_->GetSiteInstance()->GetSiteURL();
+    bool current_is_view_source_mode = current_entry ?
+        current_entry->IsViewSourceMode() : new_entry->IsViewSourceMode();
+    return manager->ShouldSwapBrowsingInstancesForNavigation(
+        current_effective_url,
+        current_is_view_source_mode,
+        new_entry->site_instance(),
+        SiteInstanceImpl::GetEffectiveURL(browser_context, new_entry->GetURL()),
+        new_entry->IsViewSourceMode());
   }
 
   // Creates a test RenderViewHost that's swapped out.