PlzNavigate: Avoid duplicate SiteInstance creation during navigation.

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 <list>
 
 #include "base/basictypes.h"
@@ skipping 407 matching lines @@
                                    SiteInstance* instance);
 
  private:
   friend class NavigatorTestWithBrowserSideNavigation;
   friend class RenderFrameHostManagerTest;
   friend class TestWebContents;
 
   FRIEND_TEST_ALL_PREFIXES(CrossProcessFrameTreeBrowserTest,
                            CreateCrossProcessSubframeProxies);
 
+  // PlzNavigate
+  enum RenderFrameHostToUse {
+    INVALID,
+    REUSE_CURRENT,
+    NEW_SPECULATIVE,
+    REUSE_SPECULATIVE,
+  };
+
+  // Stores information regarding a SiteInstance targeted at a specific URL. It
+  // can whether point to an existent one or store the details needed to create

[Charlie Reis, 2015/03/25 00:09:25]

nit: can point (drop "whether")

s/existent/existing/ (everywhere)

[carlosk, 2015/03/30 14:37:38]

Done.

+  // a new one.
+  struct SiteInstanceDescriptor {
+    explicit SiteInstanceDescriptor(content::SiteInstance* site_instance)
+        : existent_site_instance(site_instance),
+          new_site_is_related_to_current(false) {}
+
+    SiteInstanceDescriptor(GURL site_url, bool related_to_current)
+        : existent_site_instance(nullptr),
+          new_site_url(site_url),
+          new_site_is_related_to_current(related_to_current) {}
+
+    // Set with an existent SiteInstance to be reused.
+    content::SiteInstance* existent_site_instance;
+    // In case |existent_site_instance| is null, specify a new site URL.
+    GURL new_site_url;
+    // In case |existent_site_instance| is null, specify if the new site should
+    // be created in new BrowsingInstance or not.
+    bool new_site_is_related_to_current;

[Charlie Reis, 2015/03/25 00:09:26]

"site" doesn't fit well in this name.  This descriptor is about SiteInstances,
not sites.

Let's call this new_is_related_to_current instead.  In that sense,
"new_site_url" means "the site URL for the new SiteInstance" and
"new_is_related_to_current" means "the new SiteInstance is related to the
current SiteInstance."

[carlosk, 2015/03/30 14:37:38]

Done.

+  };
+
   // Used with FrameTree::ForEach to erase RenderFrameProxyHosts from a
   // FrameTreeNode's RenderFrameHostManager.
   static bool ClearProxiesInSiteInstance(int32 site_instance_id,
                                          FrameTreeNode* node);
   // Used with FrameTree::ForEach to reset initialized state of
   // RenderFrameProxyHosts from a FrameTreeNode's RenderFrameHostManager.
   static bool ResetProxiesInSiteInstance(int32 site_instance_id,
                                          FrameTreeNode* node);
 
   // Returns whether this tab should transition to a new renderer for
@@ skipping 28 matching lines @@
   // Returns true if it is safe to reuse the current WebUI when navigating from
   // |current_entry| to |new_url|.
   bool ShouldReuseWebUI(
       const NavigationEntry* current_entry,
       const GURL& new_url) const;
 
   // Returns the SiteInstance to use for the navigation.
   SiteInstance* GetSiteInstanceForNavigation(const GURL& dest_url,
                                              SiteInstance* source_instance,
                                              SiteInstance* dest_instance,
+                                             SiteInstance* candidate_instance,
                                              ui::PageTransition transition,
                                              bool dest_is_restore,
                                              bool dest_is_view_source_mode);
 
-  // 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. |source_instance| is the SiteInstance of the frame that initiated the
-  // navigation. |current_instance| is the SiteInstance of the frame that is
-  // currently navigating. |dest_instance|, is a predetermined SiteInstance
-  // that'll be used if not null.
+  // PlzNavigate
+  // Returns true if |candidate_instance| confirms to the SiteInstance specified

[Charlie Reis, 2015/03/25 00:09:25]

"confirms to" isn't grammatically correct.  "matches" would work.

[carlosk, 2015/03/30 14:37:38]

It was a typo: I meant to write "conforms". But "matches" sounds good as well.
Done.

+  // by |descriptor|.
+  bool SiteInstanceMatchesDescription(SiteInstance* candidate_instance,

[carlosk, 2015/03/24 15:30:06]

I wanted to move this method into an unnamed namespace but it would require
un-privatizing SiteInstanceDescriptor. So... WDYT?

[Charlie Reis, 2015/03/25 00:09:25]

Please keep SiteInstanceDescriptor private.

[carlosk, 2015/03/30 14:37:38]

Acknowledged.

+                                      const SiteInstanceDescriptor& descriptor);
+
+  // Returns a descriptor of the appropriate SiteInstance object for the given
+  // |dest_url|, possibly reusing the current, source or destination
+  // SiteInstance. It will check ShouldTransitionCrossSite and

[Charlie Reis, 2015/03/25 00:09:25]

This second sentence is an implementation detail and isn't needed here anymore.

[carlosk, 2015/03/30 14:37:38]

Done.

+  // ShouldSwapBrowsingInstancesForNavigation.
+  // |source_instance| is the SiteInstance of the frame that initiated the

[Charlie Reis, 2015/03/25 00:09:25]

nit: Add an empty comment line above this.

[carlosk, 2015/03/30 14:37:38]

Done.

+  // navigation. |dest_instance|, is a predetermined SiteInstance

[Charlie Reis, 2015/03/25 00:09:25]

Remove comma.

[carlosk, 2015/03/30 14:37:38]

Done.

+  // that'll be used if not null. Internally, |current_instance| is the

[Charlie Reis, 2015/03/25 00:09:25]

that'll -> that will

[carlosk, 2015/03/30 14:37:38]

Done.

+  // SiteInstance of the frame that is currently navigating.
   // For example, if you have a parent frame A, which has a child frame B, and
   // A is trying to change the src attribute of B, this will cause a navigation
   // where the source SiteInstance is A and B is the current SiteInstance.
-  // This is a helper function for GetSiteInstanceForNavigation.
-  SiteInstance* GetSiteInstanceForURL(const GURL& dest_url,
-                                      SiteInstance* source_instance,
-                                      SiteInstance* current_instance,
-                                      SiteInstance* dest_instance,
-                                      ui::PageTransition transition,
-                                      bool dest_is_restore,
-                                      bool dest_is_view_source_mode,
-                                      bool force_browsing_instance_swap);
+  // This is a helper function for GetSiteInstanceForNavigation and

[Charlie Reis, 2015/03/25 00:09:26]

nit: Add an empty comment line above this.

[carlosk, 2015/03/30 14:37:38]

Done.

+  // IsCorrectSiteInstanceForURL.
+  SiteInstanceDescriptor DetermineSiteInstanceForURL(
+      const GURL& dest_url,
+      SiteInstance* source_instance,
+      SiteInstance* dest_instance,
+      ui::PageTransition transition,
+      bool dest_is_restore,
+      bool dest_is_view_source_mode);
+  SiteInstanceDescriptor DetermineSiteInstanceForURLInternal(

[Charlie Reis, 2015/03/25 00:09:25]

These functions are already private internal details.  Please just have the
latter one and drop the word "Internal" from the name.

[carlosk, 2015/03/30 14:37:38]

I explained this here:
https://codereview.chromium.org/967383002/diff/80001/content/browser/frame_ho...

But following the suggestion from another comment the *Internal version was in
fact eliminated.

+      const GURL& dest_url,
+      SiteInstance* source_instance,
+      SiteInstance* current_instance,
+      SiteInstance* dest_instance,
+      ui::PageTransition transition,
+      bool dest_is_restore,
+      bool dest_is_view_source_mode,
+      bool force_browsing_instance_swap);
 
   // Determines the appropriate url to use as the current url for SiteInstance
   // selection.
   const GURL& GetCurrentURLForSiteInstance(
       SiteInstance* current_instance,
       NavigationEntry* current_entry);
 
   // Creates a new RenderFrameHostImpl for the |new_instance| and assign it to
   // |pending_render_frame_host_| while respecting the opener route if needed
   // and stores it in pending_render_frame_host_.
@@ skipping 196 matching lines @@
   bool should_reuse_web_ui_;
 
   base::WeakPtrFactory<RenderFrameHostManager> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(RenderFrameHostManager);
 };
 
 }  // namespace content
 
 #endif  // CONTENT_BROWSER_FRAME_HOST_RENDER_FRAME_HOST_MANAGER_H_