PlzNavigate: Avoid duplicate SiteInstance creation during navigation.

content/browser/frame_host/navigator_impl_unittest.cc

Index: content/browser/frame_host/navigator_impl_unittest.cc
diff --git a/content/browser/frame_host/navigator_impl_unittest.cc b/content/browser/frame_host/navigator_impl_unittest.cc
index 03112e00020870d5980255ab35b3aff59a7c87a3..9b30881d37dc0cf601c584cbbf4f9736fc3fe364 100644
--- a/content/browser/frame_host/navigator_impl_unittest.cc
+++ b/content/browser/frame_host/navigator_impl_unittest.cc
@@ -36,6 +36,10 @@ namespace content {
 class NavigatorTestWithBrowserSideNavigation
     : public RenderViewHostImplTestHarness {
  public:
+  // Re-defines the private RenderFrameHostManager::SiteInstanceDescriptor here
+  // to allow access to it from tests.
+  typedef RenderFrameHostManager::SiteInstanceDescriptor SiteInstanceDescriptor;
+
   void SetUp() override {
     EnableBrowserSideNavigation();
     RenderViewHostImplTestHarness::SetUp();
@@ -90,6 +94,12 @@ class NavigatorTestWithBrowserSideNavigation
     return commit_message &&
            rfh->GetRoutingID() == commit_message->routing_id();
   }
+
+  SiteInstance* ConvertToSiteInstance(RenderFrameHostManager* rfhm,
+                                      const SiteInstanceDescriptor& descriptor,
+                                      SiteInstance* candidate_instance) {
+    return rfhm->ConvertToSiteInstance(descriptor, candidate_instance);
+  }
 };
 
 // PlzNavigate: Test a complete browser-initiated navigation starting with a
@@ -997,4 +1007,124 @@ TEST_F(NavigatorTestWithBrowserSideNavigation, DataUrls) {
   EXPECT_FALSE(GetSpeculativeRenderFrameHost(node));
 }
 
+TEST_F(NavigatorTestWithBrowserSideNavigation,

[Charlie Reis, 2015/04/02 21:23:25]

Let's give a roadmap for the test in a comment before this line.  Something
like:

Tests several cases for converting SiteInstanceDescriptors into SiteInstances:
1) Pointer to current.
2) Pointer to unrelated.
3) Same-site URL, related.
4) Cross-site URL, related.
5) Same-site URL, unrelated (with and without candidate SiteInstances).
6) Cross-site URL, unrelated (with candidate SiteInstances).

(We can then number each of the cases below to make it easy to skim.)

[carlosk, 2015/04/03 16:11:19]

Done.

+       SiteInstanceDescriptionConversion) {
+  // Navigate to set a current SiteInstance on the RenderFrameHost.
+  GURL kUrl1("http://a.com");
+  contents()->NavigateAndCommit(kUrl1);
+  SiteInstance* current_instance = main_test_rfh()->GetSiteInstance();
+  ASSERT_TRUE(current_instance);
+
+  // Convert a descriptor pointing to the current instance.
+  RenderFrameHostManager* rfhm =
+      main_test_rfh()->frame_tree_node()->render_manager();
+  {
+    SiteInstanceDescriptor descriptor(current_instance);
+    SiteInstance* converted_instance =
+        ConvertToSiteInstance(rfhm, descriptor, nullptr);
+    EXPECT_EQ(current_instance, converted_instance);
+  }

[carlosk, 2015/04/02 17:15:05]

I used a lot of scoping here to avoid mixing up things from one step to the
next. The variables that I wanted to keep are all defined at the top test level.

[Charlie Reis, 2015/04/02 21:23:24]

Acknowledged.

+
+  // Convert a descriptor pointing an instance unrelated to the current one,
+  // with a different site.
+  GURL kUrl2("http://b.com");
+  scoped_refptr<SiteInstanceImpl> unrelated_instance(
+      static_cast<SiteInstanceImpl*>(
+          SiteInstanceImpl::Create(browser_context())));
+  unrelated_instance->SetSite(kUrl2);
+  EXPECT_FALSE(
+      current_instance->IsRelatedSiteInstance(unrelated_instance.get()));
+  {
+    SiteInstanceDescriptor descriptor(unrelated_instance.get());
+    SiteInstance* converted_instance =
+        ConvertToSiteInstance(rfhm, descriptor, nullptr);
+    EXPECT_EQ(unrelated_instance.get(), converted_instance);
+  }
+
+  // Convert a descriptor of a related instance with the same site as the
+  // current one.
+  GURL kUrlSameSiteAs1("http://www.a.com/foo");
+  {
+    SiteInstanceDescriptor descriptor(kUrlSameSiteAs1, true, browser_context());
+    SiteInstance* converted_instance =
+        ConvertToSiteInstance(rfhm, descriptor, nullptr);
+    EXPECT_EQ(current_instance, converted_instance);
+  }
+
+  // Convert a descriptor of a related instance with a site different from the
+  // current one.
+  GURL kUrlSameSiteAs2("http://www.b.com/foo");
+  scoped_refptr<SiteInstance> related_instance;
+  {
+    SiteInstanceDescriptor descriptor(kUrlSameSiteAs2, true, browser_context());
+    related_instance = ConvertToSiteInstance(rfhm, descriptor, nullptr);
+    // Should return a new instance, related to the current, set to the new site
+    // URL.
+    EXPECT_TRUE(
+        current_instance->IsRelatedSiteInstance(related_instance.get()));
+    EXPECT_NE(current_instance, related_instance.get());
+    EXPECT_NE(unrelated_instance.get(), related_instance.get());
+    EXPECT_EQ(
+        SiteInstanceImpl::GetSiteForURL(browser_context(), kUrlSameSiteAs2),

[Charlie Reis, 2015/04/02 21:23:25]

nit: s/SiteInstanceImpl/SiteInstance/

Also, I'm noticing that we tend to put BrowserContext as the first parameter on
methods like this, with the URL (etc) following.  Let's do the same on the
SiteInstanceDescriptor to be consistent.

[carlosk, 2015/04/03 16:11:19]

Done.

Also replaced SiteInstanceImpl::Create + SetSite with SiteInstance::CreateForURL
so I didn't need a SiteInstanceImpl pointer anymore.

And I held back my temptation to
s/SiteInstanceImpl::GetSiteForURL/SiteInstance::GetSiteForURL/ all over the
place. :)

+        related_instance->GetSiteURL());
+  }
+
+  // Convert a descriptor of an unrelated instance with the same site as the
+  // current one.
+  {
+    SiteInstanceDescriptor descriptor(kUrlSameSiteAs1, false,
+                                      browser_context());
+    scoped_refptr<SiteInstance> converted_instance_1 =
+        ConvertToSiteInstance(rfhm, descriptor, nullptr);
+    // Should return a new instance, unrelated to the current one, set to the
+    // provided site URL.
+    EXPECT_FALSE(
+        current_instance->IsRelatedSiteInstance(converted_instance_1.get()));
+    EXPECT_NE(current_instance, converted_instance_1.get());
+    EXPECT_NE(unrelated_instance.get(), converted_instance_1.get());
+    EXPECT_EQ(
+        SiteInstanceImpl::GetSiteForURL(browser_context(), kUrlSameSiteAs1),
+        converted_instance_1->GetSiteURL());
+
+    // Does the same but this time using unrelated_instance as a candidate,
+    // which has a different site.
+    scoped_refptr<SiteInstance> converted_instance_2 =
+        ConvertToSiteInstance(rfhm, descriptor, unrelated_instance.get());
+    // Should return yet another new instance, unrelated to the current one, set
+    // to the same site URL.
+    EXPECT_FALSE(
+        current_instance->IsRelatedSiteInstance(converted_instance_2.get()));
+    EXPECT_NE(current_instance, converted_instance_2.get());
+    EXPECT_NE(unrelated_instance.get(), converted_instance_2.get());
+    EXPECT_NE(converted_instance_1.get(), converted_instance_2.get());
+    EXPECT_EQ(
+        SiteInstanceImpl::GetSiteForURL(browser_context(), kUrlSameSiteAs1),
+        converted_instance_2->GetSiteURL());
+
+    // Converts once more but with converted_instance_1 as a candidate.
+    SiteInstance* converted_instance_3 =
+        ConvertToSiteInstance(rfhm, descriptor, converted_instance_1.get());
+    // Should return converted_instance_1

[Charlie Reis, 2015/04/02 21:23:25]

nit: End with period.
Also mention "because the site matches and it is unrelated to the current
SiteInstance."

[carlosk, 2015/04/03 16:11:19]

Done.

+    EXPECT_EQ(converted_instance_1.get(), converted_instance_3);
+  }
+
+  // Convert a descriptor of an unrelated instance with the same site of
+  // related_instance and using it as a candidate.
+  {
+    SiteInstanceDescriptor descriptor(kUrlSameSiteAs2, false,
+                                      browser_context());
+    scoped_refptr<SiteInstance> converted_instance =
+        ConvertToSiteInstance(rfhm, descriptor, related_instance.get());
+    // Should return a new instance, unrelated to the current, set to the
+    // provided site URL.
+    EXPECT_FALSE(
+        current_instance->IsRelatedSiteInstance(converted_instance.get()));
+    EXPECT_NE(related_instance.get(), converted_instance.get());
+    EXPECT_NE(unrelated_instance.get(), converted_instance.get());

[Charlie Reis, 2015/04/02 21:23:25]

As in case #5, can we add a line to ensure that ConvertToSiteInstance(rfhm,
descriptor, unrelated_instance.get()) returns unrelated_instance?

[carlosk, 2015/04/03 16:11:19]

Done.

+    EXPECT_EQ(
+        SiteInstanceImpl::GetSiteForURL(browser_context(), kUrlSameSiteAs2),
+        converted_instance->GetSiteURL());
+  }
+}
+
 }  // namespace content