PlzNavigate: Speculatively spawns a renderer process for navigations.

content/browser/frame_host/render_frame_host_manager.h

Index: content/browser/frame_host/render_frame_host_manager.h
diff --git a/content/browser/frame_host/render_frame_host_manager.h b/content/browser/frame_host/render_frame_host_manager.h
index 7394628bcbf3136c133ebe367c0852818915106c..7acab9b3b4bf3e233cbba8e2fed8a64fc9ce3eb9 100644
--- a/content/browser/frame_host/render_frame_host_manager.h
+++ b/content/browser/frame_host/render_frame_host_manager.h
@@ -21,19 +21,20 @@
 #include "ui/base/page_transition_types.h"
 
 struct FrameMsg_Navigate_Params;
+struct FrameHostMsg_BeginNavigation_Params;

[Charlie Reis, 2014/11/05 17:50:01]

nit: Alphabetize

[carlosk, 2014/11/05 20:28:59]

Done.

 
 namespace content {
 class BrowserContext;
 class CrossProcessFrameConnector;
 class CrossSiteTransferringRequest;
-class InterstitialPageImpl;
 class FrameTreeNode;
+class InterstitialPageImpl;
 class NavigationControllerImpl;
 class NavigationEntry;
 class NavigationEntryImpl;
 class RenderFrameHost;
-class RenderFrameHostDelegate;
 class RenderFrameHost;

[Charlie Reis, 2014/11/05 17:50:01]

This is a duplicate.  (Wonder how it snuck in twice before.)

[carlosk, 2014/11/05 20:28:59]

Done. I wonder why the compiler didn't complain...

+class RenderFrameHostDelegate;
 class RenderFrameHostImpl;
 class RenderFrameHostManagerTest;
 class RenderFrameProxyHost;
@@ -43,6 +44,7 @@ class RenderWidgetHostDelegate;
 class RenderWidgetHostView;
 class TestWebContents;
 class WebUIImpl;
+struct CommonNavigationParams;
 
 // Manages RenderFrameHosts for a FrameTreeNode. It maintains a
 // current_frame_host() which is the content currently visible to the user. When
@@ -294,6 +296,10 @@ class CONTENT_EXPORT RenderFrameHostManager : public NotificationObserver {
   // Called when a renderer's frame navigates.
   void DidNavigateFrame(RenderFrameHostImpl* render_frame_host);
 
+  // PlzNavigate
+  // Cleans up ongoing navigation data.
+  void CleanUpCurrentNavigation();
+
   // Called when a renderer sets its opener to null.
   void DidDisownOpener(RenderFrameHost* render_frame_host);
 
@@ -362,6 +368,12 @@ class CONTENT_EXPORT RenderFrameHostManager : public NotificationObserver {
   // RenderFrameHostManager. Returns MSG_ROUTING_NONE if none is found.
   int GetRoutingIdForSiteInstance(SiteInstance* site_instance);
 
+  // PlzNavigate: Notifies the RFHM that a navigation is about to begin so that
+  // it can do stuff in advance like trying to speculatively spawn a new

[Charlie Reis, 2014/11/05 17:50:01]

Drop "do stuff in advance like trying to"

[carlosk, 2014/11/05 20:28:59]

Done. That one fell through the gaps of my "let's turn this prototype into real
code" review. :)

+  // renderer process.
+  void WillBeginNavigating(const FrameHostMsg_BeginNavigation_Params& params,
+                           const CommonNavigationParams& common_params);
+
   // PlzNavigate: Called when a navigation is ready to commit, to select the
   // renderer that will commit it.
   RenderFrameHostImpl* GetFrameHostForNavigation(const GURL& url,
@@ -416,21 +428,32 @@ class CONTENT_EXPORT RenderFrameHostManager : public NotificationObserver {
       SiteInstance* dest_instance,
       ui::PageTransition dest_transition,
       bool dest_is_restore,
-      bool dest_is_view_source_mode);
+      bool dest_is_view_source_mode,
+      bool create_unbounded_site_instance);

[Charlie Reis, 2014/11/05 17:50:01]

unbounded isn't the right word, but I also think we shouldn't have this concept
(see my comment in SiteInstanceImpl).

[carlosk, 2014/11/05 20:28:58]

Woudl "unset" be better? Or "without URL"?

 
   // 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. |dest_instance| will be used if it is not null.
   // This is a helper function for GetSiteInstanceForNavigation.
-  SiteInstance* GetSiteInstanceForURL(
-      const GURL& dest_url,
-      SiteInstance* dest_instance,
-      ui::PageTransition dest_transition,
-      bool dest_is_restore,
-      bool dest_is_view_source_mode,
-      SiteInstance* current_instance,
-      bool force_browsing_instance_swap);
+  SiteInstance* GetSiteInstanceForURL(const GURL& dest_url,
+                                      SiteInstance* dest_instance,
+                                      ui::PageTransition dest_transition,
+                                      bool dest_is_restore,
+                                      bool dest_is_view_source_mode,
+                                      SiteInstance* current_instance,
+                                      bool force_browsing_instance_swap,
+                                      bool create_unbounded_site_instance);
+
+  SiteInstance* CreateSiteInstanceForURL(BrowserContext* browser_context,

[Charlie Reis, 2014/11/05 17:50:01]

I'm hoping we can avoid these methods (see SiteInstanceImpl).

[carlosk, 2014/11/05 20:28:59]

Acknowledged.

+                                         const GURL& creation_url,
+                                         bool create_unbounded_site_instance,
+                                         SiteInstanceImpl* current_instance);
+
+  SiteInstance* GetRelatedSiteInstanceForURL(
+      SiteInstanceImpl* current_instance,
+      const GURL& creation_url,
+      bool create_unbounded_site_instance);
 
   // Determines the appropriate url to use as the current url for SiteInstance
   // selection.
@@ -452,6 +475,15 @@ class CONTENT_EXPORT RenderFrameHostManager : public NotificationObserver {
                                                         bool swapped_out,
                                                         bool hidden);
 
+  // PlzNavigate
+  // Creates a new RenderFrameHost and does all required supporting work for the
+  // speculative creation of a new renderer for an ongoing navigation request.
+  // This new instance might be later destroyed if the final navigation

[Charlie Reis, 2014/11/05 17:50:01]

new instance -> RenderFrameHost

[carlosk, 2014/11/05 20:28:59]

Done.

+  // destination doesn't match the initial one.
+  bool CreateSpeculativeRenderFrameHost(const GURL& url,
+                                        SiteInstance* old_instance,
+                                        SiteInstance* new_instance);
+
   // Sets up the necessary state for a new RenderViewHost with the given opener,
   // if necessary.  It creates a RenderFrameProxy in the target renderer process
   // with the given |proxy_routing_id|, which is used to route IPC messages when
@@ -471,13 +503,18 @@ class CONTENT_EXPORT RenderFrameHostManager : public NotificationObserver {
   // Sets the pending RenderFrameHost/WebUI to be the active one. Note that this
   // doesn't require the pending render_frame_host_ pointer to be non-NULL,
   // since there could be Web UI switching as well. Call this for every commit.
-  void CommitPending();
+  void CommitPending(bool use_speculative_rfh);

[Charlie Reis, 2014/11/05 17:50:01]

Let's give a second paragraph in the comment about how this behaves for
PlzNavigate, and clarify that |use_speculative_rfh| is only used in that case
(and pending RFHs aren't).

[carlosk, 2014/11/05 20:28:58]

Done.

 
   // Runs the unload handler in the old RenderFrameHost, after the new
   // RenderFrameHost has committed.  |old_render_frame_host| will either be
   // deleted or put on the pending delete list during this call.
   void SwapOutOldFrame(scoped_ptr<RenderFrameHostImpl> old_render_frame_host);
 
+  // Decides if the provided RenderFrameHostImpl instance should be destructed
+  // or swapped out and executes it.
+  void RecycleRenderFrameHost(

[Charlie Reis, 2014/11/05 17:50:01]

If we need this, we should use a more common name (e.g., DestroyRenderFrameHost)
rather than recycle/execute.

[carlosk, 2014/11/05 20:28:59]

As in fact the RFH might not be destroyed (when it's just swapped out) I was
trying to name it in a way that didn't imply "destruction". 

Maybe my comment was not clear (?) so I updated it.

+      scoped_ptr<RenderFrameHostImpl> render_frame_host);
+
   // Holds |render_frame_host| until it can be deleted when its swap out ACK
   // arrives.
   void MoveToPendingDeleteHosts(
@@ -584,6 +621,13 @@ class CONTENT_EXPORT RenderFrameHostManager : public NotificationObserver {
 
   base::WeakPtrFactory<RenderFrameHostManager> weak_factory_;
 
+  // Members to store the speculative RFH created upon starting a navigation

[Charlie Reis, 2014/11/05 17:50:01]

Important: Preface this with "PlzNavigate" and make it clear that speculative
RFHs and pending RFHs are mutually exclusive approaches (i.e., we only use
pending RFHs for default behavior and only use speculative RFHs in PlzNavigate).

[carlosk, 2014/11/05 20:28:59]

Done. Just made it clear that for now PlzNavigate and pendind_* instances are
not yet mutually exclusive.

+  // allowing the creation of the renderer process as early as possible.
+  // Might be discarded later on if the final URL's SiteInstnace mismatches

[Charlie Reis, 2014/11/05 17:50:01]

nit: SiteInstance
nit: doesn't match

[carlosk, 2014/11/05 20:28:59]

Done.

+  // what was assumed in the beginning.
+  scoped_ptr<RenderFrameHostImpl> speculative_render_frame_host_;
+  scoped_ptr<WebUIImpl> speculative_web_ui_;
+
   DISALLOW_COPY_AND_ASSIGN(RenderFrameHostManager);
 };