Remove dependency on NavigationEntry to get a SiteInstance

content/browser/frame_host/render_frame_host_manager.h

 // 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.
 
 #ifndef CONTENT_BROWSER_FRAME_HOST_RENDER_FRAME_HOST_MANAGER_H_
 #define CONTENT_BROWSER_FRAME_HOST_RENDER_FRAME_HOST_MANAGER_H_
 
 #include "base/basictypes.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/weak_ptr.h"
+#include "content/browser/frame_host/navigation_entry_impl.h"
 #include "content/browser/renderer_host/render_view_host_delegate.h"
 #include "content/browser/site_instance_impl.h"
 #include "content/common/content_export.h"
 #include "content/public/browser/global_request_id.h"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
 #include "content/public/common/referrer.h"
 
 struct FrameHostMsg_BeginNavigation_Params;
 
@@ skipping 357 matching lines @@
   // Used with FrameTree::ForEach to erase RenderFrameProxyHosts from a
   // FrameTreeNode's RenderFrameHostManager.
   static bool ClearProxiesInSiteInstance(int32 site_instance_id,
                                          FrameTreeNode* node);
 
   // Returns whether this tab should transition to a new renderer for
   // cross-site URLs.  Enabled unless we see the --process-per-tab command line
   // switch.  Can be overridden in unit tests.
   bool ShouldTransitionCrossSite();
 
-  // Returns true if for the navigation from |current_entry| to |new_entry|,
+  // Returns true if for the navigation from |current_entry| to |new_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.
-  // Either of the entries may be NULL.
+  // |current_entry| and |new_site_instance| may be null.
   bool ShouldSwapBrowsingInstancesForNavigation(
       const NavigationEntry* current_entry,

[Charlie Reis, 2014/08/14 06:28:09]

Looks like we can eliminate |current_entry| from this method as well, which
would make it a more consistent set of parameters (even if we can't yet remove
it from UpdateStateForNavigate). 

[clamy, 2014/08/14 10:02:59]

Done.

-      const NavigationEntryImpl* new_entry) const;
+      SiteInstance* new_site_instance,
+      const GURL& new_url,

[Charlie Reis, 2014/08/14 06:28:09]

It's very important that this is the result of GetEffectiveURL, and not just the
entry's URL (for hosted apps).  Let's call it |new_effective_url| and document
the requirement.

[clamy, 2014/08/14 10:02:59]

Done.

+      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|.
   bool ShouldReuseWebUI(
       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. |navigation_instance| will be used if it is not null.
+  SiteInstance* GetSiteInstanceForURL(
+      const GURL& dest_url,
+      SiteInstance* navigation_instance,

[Charlie Reis, 2014/08/14 06:28:09]

We should be clearer about which parameters are for the destination, by grouping
them together and giving them a common prefix:

dest_url, dest_site_instance, dest_transition, dest_is_restore,
dest_is_view_source_mode, current_instance, force_browsing_instance_swap

[clamy, 2014/08/14 10:02:59]

Done.

       SiteInstance* current_instance,
-      bool force_browsing_instance_swap);
+      PageTransition page_transition,
+      NavigationEntryImpl::RestoreType restore_type,

[Charlie Reis, 2014/08/14 06:28:09]

We can make this a bool |dest_is_restore| to avoid having to include
navigation_entry_impl.h in this header file.

[clamy, 2014/08/14 10:02:59]

Done.

+      bool force_browsing_instance_swap,
+      bool is_view_source_mode);
 
   // Creates a RenderFrameHost and corresponding RenderViewHost if necessary.
   scoped_ptr<RenderFrameHostImpl> CreateRenderFrameHost(SiteInstance* instance,
                                                         int view_routing_id,
                                                         int frame_routing_id,
                                                         bool swapped_out,
                                                         bool hidden);
 
   // Sets up the necessary state for a new RenderViewHost with the given opener,
   // if necessary.  It creates a RenderFrameProxy in the target renderer process
@@ skipping 110 matching lines @@
   scoped_ptr<NavigationRequest> navigation_request_;
 
   base::WeakPtrFactory<RenderFrameHostManager> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(RenderFrameHostManager);
 };
 
 }  // namespace content
 
 #endif  // CONTENT_BROWSER_FRAME_HOST_RENDER_FRAME_HOST_MANAGER_H_