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 3b80d6304c89347ee86018c36e53f56f4acd543c..bf1a6f534b8d055ddc48c8012bdffb359d69a6da 100644
--- a/content/browser/frame_host/render_frame_host_manager.h
+++ b/content/browser/frame_host/render_frame_host_manager.h
@@ -31,6 +31,7 @@ class InterstitialPageImpl;
 class NavigationControllerImpl;
 class NavigationEntry;
 class NavigationEntryImpl;
+class NavigatorTestWithBrowserSideNavigation;
 class RenderFrameHost;
 class RenderFrameHostDelegate;
 class RenderFrameHostImpl;
@@ -42,6 +43,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
@@ -289,7 +291,8 @@ class CONTENT_EXPORT RenderFrameHostManager : public NotificationObserver {
 
   // Called when a renderer's frame navigates.
   void DidNavigateFrame(RenderFrameHostImpl* render_frame_host,
-                        bool was_caused_by_user_gesture);
+                        bool was_caused_by_user_gesture,
+                        bool was_within_same_page);

[nasko, 2015/01/06 00:03:18]

Why is this parameter added? It isn't used anywhere and all calls pass false to
it. Let's not add it in this CL as it is irrelevant.

[carlosk, 2015/01/08 16:05:56]

clamy@ asked for this to be added to be already in tune with the upcoming
changes to cancelling a browser side navigation. It's currently used in a DCHECK
in that method and there's a TODO there.

[nasko, 2015/01/09 18:13:07]

In general, we don't add code that isn't used in a CL, just because it might be
used in the future.

[carlosk, 2015/01/12 14:35:25]

Talked to clamy@ and we agreed to revert it.

 
   // Called when a renderer sets its opener to null.
   void DidDisownOpener(RenderFrameHost* render_frame_host);
@@ -367,10 +370,26 @@ class CONTENT_EXPORT RenderFrameHostManager : public NotificationObserver {
   // RenderFrameHostManager. Returns MSG_ROUTING_NONE if none is found.
   int GetRoutingIdForSiteInstance(SiteInstance* site_instance);
 
-  // PlzNavigate: Called when a navigation is ready to commit, to select the
-  // renderer that will commit it.
-  RenderFrameHostImpl* GetFrameHostForNavigation(const GURL& url,

[nasko, 2015/01/06 00:03:18]

Why renaming from Get to Select? Select implies returning an existing item,
whereas this method will create one if needed.

[carlosk, 2015/01/08 16:05:56]

I discussed with clamy@ and this seemed like the best option. "Get" made it look
like a getter, which this method is not as it changes internal state. It might
create a new one and set is as the speculative. It might simply use the current
one. And it returns whichever it chose just to inform the caller.

To use "Select" seemed to imply more what you are saying. Other options used
"Update" but clamy@ was not fond of it as it seemed as it was just updating an
existing one.

I have no better options and am open for suggestions...

[nasko, 2015/01/08 23:36:37]

While somewhat true, there is a coding convention in Chrome where accessors are
hacker_cased, so it is clear they don't do any extra work. CamelCased methods do
extra work or are part of public interface. Since this isn't a public API and
there is already a pattern in Chrome that Get* can create the object, I think it
is best to leave it unchanged.
 

[carlosk, 2015/01/09 14:48:52]

Acknowledged. Reverted to "Get".

-                                                 ui::PageTransition transition);
+  // PlzNavigate
+  // Notifies the RFHM that a navigation has begun so that it can speculatively
+  // create a new renderer process if needed.

[nasko, 2015/01/06 00:03:18]

nit: s/a new renderer process/a new RenderFrameHost (and potentially a new
process)/

[carlosk, 2015/01/08 16:05:56]

Done.

+  void BeginNavigation(const CommonNavigationParams& common_params);
+
+  // PlzNavigate
+  // Called (possibly several times) during a navigation to select an

[nasko, 2015/01/06 00:03:18]

Do we really expect this to be called more than twice? Once at the beginning and
once when we are ready to commit.

[carlosk, 2015/01/08 16:05:56]

For every redirect once we begin monitoring them.

[nasko, 2015/01/08 23:36:37]

Why do we need to monitor redirects? We should be creating RFH at the initial
request and at commit time, if it doesn't match the original.

[carlosk, 2015/01/09 14:48:52]

This is a performance improvement change and the earlier we create the renderer
the higher the chance it will be ready and waiting for the navigation to commit.
So it's preferable to update it upon request and at every redirect as they will
happen earlier than the commit.

So instead of two updates, on request and commit, we will update on request and
on every redirect so that upon commit we already have the appropriate
RFH/renderer created.

[nasko, 2015/01/09 18:13:07]

I'm sorry, but I do not agree with this. While it is indeed a performance
improvement change, it is an optimization change too. The amount of work needed
to tear down and spin up RFHs on redirects might negate whatever optimizations
we aim to achieve.
Also, there is cost in complexity of the codebase and potential for bugs.
Based on these, I don't believe we should be creating speculative RFHs on
redirects.
Unless we have hard data showing that this will buy is a tremendous performance
improvement in real world browsing, I don't agree we should be doing it.

[carlosk, 2015/01/12 14:35:25]

I agree that backing this with data is the right thing to do so that's what
we'll do once we get there.

+  // appropriate RenderFrameHost for the provided URL.
+  RenderFrameHostImpl* SelectFrameHostForNavigation(
+      const GURL& url,
+      ui::PageTransition transition);
+
+  // PlzNavigate
+  // Cancels any state for any ongoing navigation.

[nasko, 2015/01/06 00:03:18]

nit: State cannot be cancelled, it can be cleaned up or invalidated.

[carlosk, 2015/01/08 16:05:56]

Changed to Clean up.

+  void CleanUpNavigation();
+
+  // PlzNavigate
+  // Clears the speculative members, returning the RenderFrameHost to the caller
+  // for disposal.
+  scoped_ptr<RenderFrameHostImpl> UnsetSpeculativeRenderFrameHost();
 
   // Notification methods to tell this RenderFrameHostManager that the frame it
   // is responsible for has started or stopped loading a document.
@@ -378,6 +397,7 @@ class CONTENT_EXPORT RenderFrameHostManager : public NotificationObserver {
   void OnDidStopLoading();
 
  private:
+  friend class NavigatorTestWithBrowserSideNavigation;
   friend class RenderFrameHostManagerTest;
   friend class TestWebContents;
 
@@ -478,6 +498,15 @@ class CONTENT_EXPORT RenderFrameHostManager : public NotificationObserver {
                                                         int frame_routing_id,
                                                         int flags);
 
+  // PlzNavigate
+  // Creates and initializes a speculative RenderFrameHost and/or WebUI for an
+  // ongoing navigation. They might be destroyed and re-created later if the
+  // navigation is redirected to a different site.

[nasko, 2015/01/06 00:03:18]

nit: s/site/SiteInstance/

[carlosk, 2015/01/08 16:05:56]

Done.

+  bool CreateSpeculativeRenderFrameHost(const GURL& url,
+                                        SiteInstance* old_instance,
+                                        SiteInstance* new_instance,
+                                        int bindings);
+
   // 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
@@ -497,6 +526,8 @@ 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.
+  // If PlzNavigate is enabled the method will set the speculative (not pending)
+  // RenderFrameHost to be the active one.
   void CommitPending();
 
   // Runs the unload handler in the old RenderFrameHost, after the new
@@ -583,6 +614,7 @@ class CONTENT_EXPORT RenderFrameHostManager : public NotificationObserver {
   // transitioning between two Web UI pages: the RFH won't be swapped, so the
   // pending pointer will be unused, but there will be a pending Web UI
   // associated with the navigation.
+  // Note: This is not used in PlzNavigate.
   scoped_ptr<RenderFrameHostImpl> pending_render_frame_host_;
 
   // If a pending request needs to be transferred to another process, this
@@ -598,6 +630,7 @@ class CONTENT_EXPORT RenderFrameHostManager : public NotificationObserver {
   // page. The scoped_ptr is used if pending_web_ui_ != web_ui_, the WeakPtr is
   // used for when they reference the same object. If either is non-NULL, the
   // other should be NULL.
+  // Note: These are not used in PlzNavigate.
   scoped_ptr<WebUIImpl> pending_web_ui_;
   base::WeakPtr<WebUIImpl> pending_and_current_web_ui_;
 
@@ -617,6 +650,23 @@ class CONTENT_EXPORT RenderFrameHostManager : public NotificationObserver {
 
   NotificationRegistrar registrar_;
 
+  // PlzNavigate
+  // These members store a speculative RenderFrameHost and WebUI. They are
+  // created early in a navigation so the renderer process can be started in

[nasko, 2015/01/06 00:03:18]

nit: s/the renderer process/a renderer process/
s/in parallel/in parallel, if needed/

[carlosk, 2015/01/08 16:05:56]

Done.

+  // parallel. This is purely a performance optimization and is not required for
+  // correct behavior. The created RenderFrameHost might be discarded later on
+  // if the final URL's SiteInstance isn't compatible with what was used to
+  // create it.
+  // Note: PlzNavigate only uses speculative RenderFrameHost and WebUI, not
+  // the pending ones.
+  scoped_ptr<RenderFrameHostImpl> speculative_render_frame_host_;
+  scoped_ptr<WebUIImpl> speculative_web_ui_;
+
+  // PlzNavigate
+  // If true at navigation commit time the current WebUI will be kept instead of
+  // creating a new one.
+  bool should_reuse_web_ui_;
+
   base::WeakPtrFactory<RenderFrameHostManager> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(RenderFrameHostManager);