PlzNavigate: Properly set the pending entry in the NavigatorImpl unit test.

content/browser/frame_host/navigator_impl_unittest.cc

 // Copyright 2014 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.
 
 #include "base/command_line.h"
 #include "base/macros.h"
 #include "base/time/time.h"
 #include "content/browser/frame_host/navigation_controller_impl.h"
 #include "content/browser/frame_host/navigation_entry_impl.h"
 #include "content/browser/frame_host/navigation_request.h"
@@ skipping 29 matching lines @@
     EnableBrowserSideNavigation();
     RenderViewHostImplTestHarness::SetUp();
   }
 
   TestNavigationURLLoader* GetLoaderForNavigationRequest(
       NavigationRequest* request) const {
     return static_cast<TestNavigationURLLoader*>(request->loader_for_testing());
   }
 
   void RequestNavigation(FrameTreeNode* node, const GURL& url) {
-    RequestNavigationWithParameters(node, url, Referrer(),
-                                    ui::PAGE_TRANSITION_LINK,
+    NavigationEntryImpl* entry = NavigationEntryImpl::FromNavigationEntry(
+        NavigationController::CreateNavigationEntry(
+            url,
+            Referrer(),
+            ui::PAGE_TRANSITION_LINK,
+            false,
+            std::string(),
+            controller().GetBrowserContext()));
+
+    RequestNavigationWithParameters(entry, node, url,
                                     NavigationController::NO_RELOAD);
   }
 
-  void RequestNavigationWithParameters(
+  void RequestReloadNavigation(
       FrameTreeNode* node,
       const GURL& url,
-      const Referrer& referrer,
-      ui::PageTransition transition_type,
       NavigationController::ReloadType reload_type) {
-    scoped_ptr<NavigationEntryImpl> entry(
-        NavigationEntryImpl::FromNavigationEntry(
-            NavigationController::CreateNavigationEntry(
-                url,
-                referrer,
-                transition_type,
-                false,
-                std::string(),
-                controller().GetBrowserContext())));
+    NavigationEntryImpl* entry = NavigationEntryImpl::FromNavigationEntry(
+        controller().GetLastCommittedEntry());
+    RequestNavigationWithParameters(entry, node, url, reload_type);
+  }
+
+  void RequestNavigationWithParameters(
+      NavigationEntryImpl* entry,
+      FrameTreeNode* node,
+      const GURL& url,
+      NavigationController::ReloadType reload_type) {
+    // In ordinary code, NavigatorImpl::RequestNavigation is called from
+    // NavigatorImpl::NavigateToEntry which must be called only with the pending
+    // entry (and thus is only called by NavigatorImpl::NavigateToPendingEntry).
+    // Therefore, if we are calling directly into RequestNavigation, we must
+    // satisfy that constraint.
+    NavigationControllerImpl& navigation_controller_impl =
+        static_cast<NavigationControllerImpl&>(controller());
+    navigation_controller_impl.SetPendingEntry(entry);
     static_cast<NavigatorImpl*>(node->navigator())->RequestNavigation(

[clamy, 2015/03/19 12:18:04]

What I had in mind is to replace this call to NavigatorImpl::RequestNavigation
by a call to NavigationController::LoadURL or
NavigationController::LoadURLWithParams. Both will properly set the pending
entry and then eventually call  NavigatorImpl::RequestNavigation. Also since it
is in the helper function you don't need to rewrite all the tests. It is
possible that the subframe test will fail below, but that's because there are
things we don't properly handle there, so it can be disabled until we do that.

         node, *entry, reload_type, base::TimeTicks::Now());
   }
 
   NavigationRequest* GetNavigationRequestForFrameTreeNode(
       FrameTreeNode* frame_tree_node) {
     return static_cast<NavigatorImpl*>(frame_tree_node->navigator())
         ->GetNavigationRequestForNodeForTesting(frame_tree_node);
   }
 
   TestRenderFrameHost* GetSpeculativeRenderFrameHost(FrameTreeNode* node) {
@@ skipping 96 matching lines @@
   EXPECT_FALSE(GetSpeculativeRenderFrameHost(node));
 
   // Have the current RenderFrameHost commit the navigation.
   scoped_refptr<ResourceResponse> response(new ResourceResponse);
   GetLoaderForNavigationRequest(request)
       ->CallOnResponseStarted(response, MakeEmptyStream());
   EXPECT_TRUE(DidRenderFrameHostRequestCommit(main_test_rfh()));
   EXPECT_EQ(NavigationRequest::RESPONSE_STARTED, request->state());
 
   // Commit the navigation.
-  main_test_rfh()->SendNavigate(0, kUrl2);
+  main_test_rfh()->SendNavigate(1, kUrl2);
   EXPECT_EQ(RenderFrameHostImpl::STATE_DEFAULT, main_test_rfh()->rfh_state());
   EXPECT_EQ(SiteInstanceImpl::GetSiteForURL(browser_context(), kUrl2),
             main_test_rfh()->GetSiteInstance()->GetSiteURL());
   EXPECT_EQ(kUrl2, contents()->GetLastCommittedURL());
   EXPECT_FALSE(GetNavigationRequestForFrameTreeNode(node));
   EXPECT_FALSE(GetSpeculativeRenderFrameHost(node));
   EXPECT_FALSE(node->render_manager()->pending_frame_host());
 }
 
 // PlzNavigate: Test a complete renderer-initiated navigation that should be
@@ skipping 24 matching lines @@
   EXPECT_FALSE(GetSpeculativeRenderFrameHost(node));
 
   // Have the current RenderFrameHost commit the navigation.
   scoped_refptr<ResourceResponse> response(new ResourceResponse);
   GetLoaderForNavigationRequest(request)
       ->CallOnResponseStarted(response, MakeEmptyStream());
   EXPECT_TRUE(DidRenderFrameHostRequestCommit(main_test_rfh()));
   EXPECT_EQ(NavigationRequest::RESPONSE_STARTED, request->state());
 
   // Commit the navigation.
-  main_test_rfh()->SendNavigate(0, kUrl2);
+  main_test_rfh()->SendNavigate(1, kUrl2);
   EXPECT_EQ(RenderFrameHostImpl::STATE_DEFAULT, main_test_rfh()->rfh_state());
   EXPECT_EQ(kUrl2, contents()->GetLastCommittedURL());
   EXPECT_FALSE(GetNavigationRequestForFrameTreeNode(node));
   EXPECT_FALSE(GetSpeculativeRenderFrameHost(node));
   EXPECT_FALSE(node->render_manager()->pending_frame_host());
 
   // The SiteInstance did not change.
   EXPECT_EQ(site_instance_id_1, main_test_rfh()->GetSiteInstance()->GetId());
 }
 
@@ skipping 363 matching lines @@
   // Confirm that the speculative RenderFrameHost was destroyed.
   EXPECT_FALSE(GetSpeculativeRenderFrameHost(node));
 
   // Have the RenderFrameHost commit the navigation.
   scoped_refptr<ResourceResponse> response(new ResourceResponse);
   GetLoaderForNavigationRequest(request2)
       ->CallOnResponseStarted(response, MakeEmptyStream());
   EXPECT_TRUE(DidRenderFrameHostRequestCommit(main_test_rfh()));
 
   // Commit the navigation.
-  main_test_rfh()->SendNavigate(0, kUrl2);
+  main_test_rfh()->SendNavigate(1, kUrl2);
 
   // Confirm that the commit corresponds to the new request.
   ASSERT_TRUE(main_test_rfh());
   EXPECT_EQ(kUrl2, contents()->GetLastCommittedURL());
 }
 
 // PlzNavigate: Test that a renderer-initiated user-initiated navigation is NOT
 // canceled if a renderer-initiated non-user-initiated request is issued in the
 // meantime.
 TEST_F(NavigatorTestWithBrowserSideNavigation,
@@ skipping 27 matching lines @@
   EXPECT_TRUE(request2->begin_params().has_user_gesture);
   EXPECT_FALSE(GetSpeculativeRenderFrameHost(node));
 
   // Have the RenderFrameHost commit the navigation.
   scoped_refptr<ResourceResponse> response(new ResourceResponse);
   GetLoaderForNavigationRequest(request2)
       ->CallOnResponseStarted(response, MakeEmptyStream());
   EXPECT_TRUE(DidRenderFrameHostRequestCommit(main_test_rfh()));
 
   // Commit the navigation.
-  main_test_rfh()->SendNavigate(0, kUrl1);
+  main_test_rfh()->SendNavigate(1, kUrl1);
   EXPECT_EQ(kUrl1, contents()->GetLastCommittedURL());
 }
 
 // PlzNavigate: Test that a browser-initiated navigation is NOT canceled if a
 // renderer-initiated non-user-initiated request is issued in the meantime.
 TEST_F(NavigatorTestWithBrowserSideNavigation,
        RendererNonUserInitiatedNavigationDoesntCancelBrowserInitiated) {
   const GURL kUrl0("http://www.wikipedia.org/");
   const GURL kUrl1("http://www.chromium.org/");
   const GURL kUrl2("http://www.google.com/");
@@ skipping 75 matching lines @@
   // Confirm that the first loader got destroyed.
   EXPECT_FALSE(loader1);
 
   // Have the RenderFrameHost commit the navigation.
   scoped_refptr<ResourceResponse> response(new ResourceResponse);
   GetLoaderForNavigationRequest(request2)
       ->CallOnResponseStarted(response, MakeEmptyStream());
   EXPECT_TRUE(DidRenderFrameHostRequestCommit(main_test_rfh()));
 
   // Commit the navigation.
-  main_test_rfh()->SendNavigate(0, kUrl2);
+  main_test_rfh()->SendNavigate(1, kUrl2);
   EXPECT_EQ(kUrl2, contents()->GetLastCommittedURL());
 
   // The SiteInstance did not change.
   EXPECT_EQ(site_instance_id_0, main_test_rfh()->GetSiteInstance()->GetId());
 }
 
 // PlzNavigate: Test that a reload navigation is properly signaled to the
 // RenderFrame when the navigation can commit. A speculative RenderFrameHost
 // should not be created at any step.
 TEST_F(NavigatorTestWithBrowserSideNavigation, Reload) {
   const GURL kUrl("http://www.google.com/");
   contents()->NavigateAndCommit(kUrl);
 
   FrameTreeNode* node = main_test_rfh()->frame_tree_node();
-  RequestNavigationWithParameters(node, kUrl, Referrer(),
-                                  ui::PAGE_TRANSITION_LINK,
-                                  NavigationController::RELOAD);
+  RequestReloadNavigation(node, kUrl, NavigationController::RELOAD);
   // A NavigationRequest should have been generated.
   NavigationRequest* main_request = GetNavigationRequestForFrameTreeNode(node);
   ASSERT_TRUE(main_request != NULL);
   EXPECT_EQ(FrameMsg_Navigate_Type::RELOAD,
             main_request->common_params().navigation_type);
   main_test_rfh()->PrepareForCommit();
   EXPECT_FALSE(GetSpeculativeRenderFrameHost(node));
 
   main_test_rfh()->SendNavigate(0, kUrl);
   EXPECT_FALSE(GetSpeculativeRenderFrameHost(node));
 
   // Now do a shift+reload.
-  RequestNavigationWithParameters(node, kUrl, Referrer(),
-                                  ui::PAGE_TRANSITION_LINK,
-                                  NavigationController::RELOAD_IGNORING_CACHE);
+  RequestReloadNavigation(
+      node, kUrl, NavigationController::RELOAD_IGNORING_CACHE);
   // A NavigationRequest should have been generated.
   main_request = GetNavigationRequestForFrameTreeNode(node);
   ASSERT_TRUE(main_request != NULL);
   EXPECT_EQ(FrameMsg_Navigate_Type::RELOAD_IGNORING_CACHE,
             main_request->common_params().navigation_type);
   main_test_rfh()->PrepareForCommit();
   EXPECT_FALSE(GetSpeculativeRenderFrameHost(node));
 }
 
 // PlzNavigate: Confirm that a speculative RenderFrameHost is used when
@@ skipping 199 matching lines @@
   main_rfh->SendRendererInitiatedNavigationRequest(kUrl2, true);
   navigation_request = GetNavigationRequestForFrameTreeNode(node);
   ASSERT_TRUE(navigation_request);
   EXPECT_EQ(NavigationRequest::RESPONSE_STARTED,
             navigation_request->state());
   EXPECT_FALSE(navigation_request->loader_for_testing());
   EXPECT_FALSE(GetSpeculativeRenderFrameHost(node));
 }
 
 }  // namespace content