PlzNavigate: track pending commits

content/browser/frame_host/navigation_controller_impl_unittest.cc

Index: content/browser/frame_host/navigation_controller_impl_unittest.cc
diff --git a/content/browser/frame_host/navigation_controller_impl_unittest.cc b/content/browser/frame_host/navigation_controller_impl_unittest.cc
index c1fce3c30fec4e115e8f926bf45b66b0ef7033ca..b85cce931d9a3d1bf5b754dbc5caccf020895422 100644
--- a/content/browser/frame_host/navigation_controller_impl_unittest.cc
+++ b/content/browser/frame_host/navigation_controller_impl_unittest.cc
@@ -709,6 +709,7 @@ TEST_F(NavigationControllerTest, LoadURL_Discarded) {
   controller.LoadURL(
       url1, Referrer(), ui::PAGE_TRANSITION_TYPED, std::string());
   EXPECT_EQ(0U, notifications.size());
+  main_test_rfh()->PrepareForCommit();
   main_test_rfh()->SendNavigate(0, url1);
   EXPECT_EQ(1U, navigation_entry_committed_counter_);
   navigation_entry_committed_counter_ = 0;
@@ -747,13 +748,14 @@ TEST_F(NavigationControllerTest, LoadURL_NoPending) {
   const GURL kExistingURL1("http://eh");
   controller.LoadURL(
       kExistingURL1, Referrer(), ui::PAGE_TRANSITION_TYPED, std::string());
+  main_test_rfh()->PrepareForCommit();
   main_test_rfh()->SendNavigate(0, kExistingURL1);

[carlosk, 2015/04/08 16:35:20]

For these cases (here and elsewhere) where a complete browser-initiated
navigation is made without any checks in between the 3 steps, to simplify things
we could also replace them with  TestWebContents::NavigateAndCommit.

Might be something for a following patch. WDYT?

[clamy, 2015/04/10 14:40:15]

Well, I won't do it in this patch which is already big enough, and I don't think
it is of very high priority.

   EXPECT_EQ(1U, navigation_entry_committed_counter_);
   navigation_entry_committed_counter_ = 0;
 
   // Do a new navigation without making a pending one.
   const GURL kNewURL("http://see");
-  main_test_rfh()->SendNavigate(99, kNewURL);
+  main_test_rfh()->NavigateAndCommitRendererInitiated(99, kNewURL);
 
   // There should no longer be any pending entry, and the third navigation we
   // just made should be committed.
@@ -1062,6 +1064,7 @@ TEST_F(NavigationControllerTest, LoadURL_RedirectAbortDoesntShowPendingURL) {
   const GURL kExistingURL("http://foo/eh");
   controller.LoadURL(kExistingURL, content::Referrer(),
                      ui::PAGE_TRANSITION_TYPED, std::string());
+  main_test_rfh()->PrepareForCommit();
   main_test_rfh()->SendNavigate(1, kExistingURL);
   EXPECT_EQ(1U, navigation_entry_committed_counter_);
   navigation_entry_committed_counter_ = 0;
@@ -1270,6 +1273,7 @@ TEST_F(NavigationControllerTest, ReloadWithGuest) {
   const GURL url1("http://foo1");
   controller.LoadURL(
       url1, Referrer(), ui::PAGE_TRANSITION_TYPED, std::string());
+  main_test_rfh()->PrepareForCommit();
   main_test_rfh()->SendNavigate(0, url1);
   ASSERT_TRUE(controller.GetVisibleEntry());
 
@@ -1364,6 +1368,7 @@ TEST_F(NavigationControllerTest, ResetEntryValuesAfterCommit) {
   const GURL url0("http://foo/0");
   controller.LoadURL(
       url0, Referrer(), ui::PAGE_TRANSITION_TYPED, std::string());
+  main_test_rfh()->PrepareForCommit();
   main_test_rfh()->SendNavigate(0, url0);
 
   // Set up the pending entry.
@@ -1394,6 +1399,7 @@ TEST_F(NavigationControllerTest, ResetEntryValuesAfterCommit) {
   EXPECT_TRUE(pending_entry->should_clear_history_list());
 
   // Fake a commit response.
+  main_test_rfh()->PrepareForCommit();
   main_test_rfh()->SendNavigate(1, url1);
 
   // Certain values that are only used for pending entries get reset after
@@ -1411,24 +1417,26 @@ TEST_F(NavigationControllerTest, ResetEntryValuesAfterCommit) {
 TEST_F(NavigationControllerTest, RedirectsAreNotResetByCommit) {
   NavigationControllerImpl& controller = controller_impl();
   const GURL url1("http://foo1");
+  const GURL url2("http://foo2");
   controller.LoadURL(
       url1, Referrer(), ui::PAGE_TRANSITION_TYPED, std::string());
 
   // Set up some redirect values.
   std::vector<GURL> redirects;
-  redirects.push_back(GURL("http://foo2"));
+  redirects.push_back(url2);
 
   // Set redirects on the pending entry.
   NavigationEntryImpl* pending_entry = controller.GetPendingEntry();
   pending_entry->SetRedirectChain(redirects);
   EXPECT_EQ(1U, pending_entry->GetRedirectChain().size());
-  EXPECT_EQ(GURL("http://foo2"), pending_entry->GetRedirectChain()[0]);
+  EXPECT_EQ(url2, pending_entry->GetRedirectChain()[0]);
 
   // Normal navigation will preserve redirects in the committed entry.
+  main_test_rfh()->PrepareForCommitWithServerRedirect(url2);
   main_test_rfh()->SendNavigateWithRedirects(0, url1, redirects);
   NavigationEntryImpl* committed_entry = controller.GetLastCommittedEntry();
   ASSERT_EQ(1U, committed_entry->GetRedirectChain().size());
-  EXPECT_EQ(GURL("http://foo2"), committed_entry->GetRedirectChain()[0]);
+  EXPECT_EQ(url2, committed_entry->GetRedirectChain()[0]);
 }
 
 // Tests what happens when we navigate back successfully
@@ -1438,12 +1446,12 @@ TEST_F(NavigationControllerTest, Back) {
   RegisterForAllNavNotifications(&notifications, &controller);
 
   const GURL url1("http://foo1");
-  main_test_rfh()->SendNavigate(0, url1);
+  main_test_rfh()->NavigateAndCommitRendererInitiated(0, url1);
   EXPECT_EQ(1U, navigation_entry_committed_counter_);
   navigation_entry_committed_counter_ = 0;
 
   const GURL url2("http://foo2");
-  main_test_rfh()->SendNavigate(1, url2);
+  main_test_rfh()->NavigateAndCommitRendererInitiated(1, url2);
   EXPECT_EQ(1U, navigation_entry_committed_counter_);
   navigation_entry_committed_counter_ = 0;
 
@@ -1467,7 +1475,7 @@ TEST_F(NavigationControllerTest, Back) {
   EXPECT_GE(controller.GetEntryAtIndex(1)->GetTimestamp(),
             controller.GetEntryAtIndex(0)->GetTimestamp());
 
-  main_test_rfh()->SendNavigate(0, url2);
+  main_test_rfh()->NavigateAndCommitRendererInitiated(0, url2);
   EXPECT_EQ(1U, navigation_entry_committed_counter_);
   navigation_entry_committed_counter_ = 0;
 
@@ -1552,12 +1560,12 @@ TEST_F(NavigationControllerTest, Back_NewPending) {
   const GURL kUrl3("http://foo3");
 
   // First navigate two places so we have some back history.
-  main_test_rfh()->SendNavigate(0, kUrl1);
+  main_test_rfh()->NavigateAndCommitRendererInitiated(0, kUrl1);
   EXPECT_EQ(1U, navigation_entry_committed_counter_);
   navigation_entry_committed_counter_ = 0;
 
   // controller.LoadURL(kUrl2, ui::PAGE_TRANSITION_TYPED);
-  main_test_rfh()->SendNavigate(1, kUrl2);
+  main_test_rfh()->NavigateAndCommitRendererInitiated(1, kUrl2);
   EXPECT_EQ(1U, navigation_entry_committed_counter_);
   navigation_entry_committed_counter_ = 0;
 
@@ -1773,6 +1781,7 @@ TEST_F(NavigationControllerTest, Redirect) {
       url1, Referrer(), ui::PAGE_TRANSITION_TYPED, std::string());
 
   EXPECT_EQ(0U, notifications.size());
+  main_test_rfh()->PrepareForCommit();
   main_test_rfh()->SendNavigate(0, url2);
   EXPECT_EQ(1U, navigation_entry_committed_counter_);
   navigation_entry_committed_counter_ = 0;
@@ -1833,6 +1842,7 @@ TEST_F(NavigationControllerTest, PostThenRedirect) {
   controller.GetVisibleEntry()->SetHasPostData(true);
 
   EXPECT_EQ(0U, notifications.size());
+  main_test_rfh()->PrepareForCommit();
   main_test_rfh()->SendNavigate(0, url2);
   EXPECT_EQ(1U, navigation_entry_committed_counter_);
   navigation_entry_committed_counter_ = 0;
@@ -1933,7 +1943,7 @@ TEST_F(NavigationControllerTest, NewSubframe) {
   RegisterForAllNavNotifications(&notifications, &controller);
 
   const GURL url1("http://foo1");
-  main_test_rfh()->SendNavigate(0, url1);
+  main_test_rfh()->NavigateAndCommitRendererInitiated(0, url1);
   EXPECT_EQ(1U, navigation_entry_committed_counter_);
   navigation_entry_committed_counter_ = 0;
 
@@ -1975,7 +1985,7 @@ TEST_F(NavigationControllerTest, AutoSubframe) {
   RegisterForAllNavNotifications(&notifications, &controller);
 
   const GURL url1("http://foo/1");
-  main_test_rfh()->SendNavigate(1, url1);
+  main_test_rfh()->NavigateAndCommitRendererInitiated(1, url1);
   EXPECT_EQ(1U, navigation_entry_committed_counter_);
   navigation_entry_committed_counter_ = 0;
 
@@ -2074,7 +2084,7 @@ TEST_F(NavigationControllerTest, BackSubframe) {
 
   // Main page.
   const GURL url1("http://foo1");
-  main_test_rfh()->SendNavigate(0, url1);
+  main_test_rfh()->NavigateAndCommitRendererInitiated(0, url1);
   EXPECT_EQ(1U, navigation_entry_committed_counter_);
   navigation_entry_committed_counter_ = 0;
 
@@ -2146,11 +2156,11 @@ TEST_F(NavigationControllerTest, LinkClick) {
   const GURL url1("http://foo1");
   const GURL url2("http://foo2");
 
-  main_test_rfh()->SendNavigate(0, url1);
+  main_test_rfh()->NavigateAndCommitRendererInitiated(0, url1);
   EXPECT_EQ(1U, navigation_entry_committed_counter_);
   navigation_entry_committed_counter_ = 0;
 
-  main_test_rfh()->SendNavigate(1, url2);
+  main_test_rfh()->NavigateAndCommitRendererInitiated(1, url2);
   EXPECT_EQ(1U, navigation_entry_committed_counter_);
   navigation_entry_committed_counter_ = 0;
 
@@ -2171,7 +2181,7 @@ TEST_F(NavigationControllerTest, InPage) {
 
   // Main page.
   const GURL url1("http://foo");
-  main_test_rfh()->SendNavigate(0, url1);
+  main_test_rfh()->NavigateAndCommitRendererInitiated(0, url1);
   EXPECT_EQ(1U, navigation_entry_committed_counter_);
   navigation_entry_committed_counter_ = 0;
 
@@ -2280,7 +2290,7 @@ TEST_F(NavigationControllerTest, InPage_Replace) {
 
   // Main page.
   const GURL url1("http://foo");
-  main_test_rfh()->SendNavigate(0, url1);
+  main_test_rfh()->NavigateAndCommitRendererInitiated(0, url1);
   EXPECT_EQ(1U, navigation_entry_committed_counter_);
   navigation_entry_committed_counter_ = 0;
 
@@ -2321,7 +2331,7 @@ TEST_F(NavigationControllerTest, ClientRedirectAfterInPageNavigation) {
   // Load an initial page.
   {
     const GURL url("http://foo/");
-    main_test_rfh()->SendNavigate(0, url);
+    main_test_rfh()->NavigateAndCommitRendererInitiated(0, url);
     EXPECT_EQ(1U, navigation_entry_committed_counter_);
     navigation_entry_committed_counter_ = 0;
   }
@@ -2329,7 +2339,7 @@ TEST_F(NavigationControllerTest, ClientRedirectAfterInPageNavigation) {
   // Navigate to a new page.
   {
     const GURL url("http://foo2/");
-    main_test_rfh()->SendNavigate(1, url);
+    main_test_rfh()->NavigateAndCommitRendererInitiated(1, url);
     EXPECT_EQ(1U, navigation_entry_committed_counter_);
     navigation_entry_committed_counter_ = 0;
   }
@@ -2387,6 +2397,7 @@ TEST_F(NavigationControllerTest, ClientRedirectAfterInPageNavigation) {
   {
     const GURL url("http://foo2/");
     controller.GoBack();
+    main_test_rfh()->PrepareForCommit();
     main_test_rfh()->SendNavigate(1, url);
     EXPECT_EQ(1U, navigation_entry_committed_counter_);
     navigation_entry_committed_counter_ = 0;
@@ -2403,6 +2414,8 @@ TEST_F(NavigationControllerTest, PushStateWithoutPreviousEntry)
   params.url = url;
   params.page_state = PageState::CreateFromURL(url);
   params.was_within_same_page = true;
+  main_test_rfh()->SendRendererInitiatedNavigationRequest(url, false);
+  main_test_rfh()->PrepareForCommit();
   contents()->GetMainFrame()->SendNavigateWithParams(&params);

[carlosk, 2015/04/08 16:35:20]

In the same line as my previous comment, it seems these 3 calls could be
replaced with the new NavigateAndCommitRendererInitiated. This might be the case
in other places as well.

[clamy, 2015/04/10 14:40:15]

No because we call SendNavigateWithParams and not SendNavigate, and I don't want
to define a TestWebContents::NavigateAndCommitRendererInitiatedWithparams.

   // We pass if we don't crash.
 }
@@ -3005,6 +3018,8 @@ TEST_F(NavigationControllerTest, RendererInitiatedPendingEntries) {
   EXPECT_TRUE(controller.GetPendingEntry()->is_renderer_initiated());
 
   // If the user clicks another link, we should replace the pending entry.
+  main_test_rfh()->SendRendererInitiatedNavigationRequest(url2, false);
+  main_test_rfh()->PrepareForCommit();
   navigator->DidStartProvisionalLoad(main_test_rfh(), url2, false);
   EXPECT_EQ(url2, controller.GetPendingEntry()->GetURL());
   EXPECT_EQ(url2, controller.GetPendingEntry()->GetVirtualURL());
@@ -3025,11 +3040,11 @@ TEST_F(NavigationControllerTest, RendererInitiatedPendingEntries) {
   // http://crbug.com/308444.
   navigator->DidStartProvisionalLoad(main_test_rfh(), url1, false);
   controller.GetPendingEntry()->set_should_replace_entry(true);
+
+  main_test_rfh()->SendRendererInitiatedNavigationRequest(url2, false);
+  main_test_rfh()->PrepareForCommit();
   navigator->DidStartProvisionalLoad(main_test_rfh(), url2, false);
   EXPECT_TRUE(controller.GetPendingEntry()->should_replace_entry());
-  // TODO(nasko): Until OnNavigate is moved to RenderFrameHost, we need
-  // to go through the RenderViewHost. The TestRenderViewHost routes navigations
-  // to the main frame.
   main_test_rfh()->SendNavigate(0, url2);
   EXPECT_EQ(url2, controller.GetLastCommittedEntry()->GetURL());
 }
@@ -3230,6 +3245,7 @@ TEST_F(NavigationControllerTest, DontShowRendererURLInNewTabAfterCommit) {
   EXPECT_FALSE(contents()->HasAccessedInitialDocument());
 
   // Simulate a commit and then starting a new pending navigation.
+  main_test_rfh()->PrepareForCommit();
   main_test_rfh()->SendNavigate(0, url1);
   NavigationController::LoadURLParams load_url2_params(url2);
   load_url2_params.transition_type = ui::PAGE_TRANSITION_LINK;
@@ -3260,12 +3276,12 @@ TEST_F(NavigationControllerTest, IsInPageNavigation) {
   // was the first document in the given frame, but we don't have enough
   // information to identify that case currently.
   const GURL blank_url(url::kAboutBlankURL);
-  main_test_rfh()->SendNavigate(0, blank_url);
+  main_test_rfh()->NavigateAndCommitRendererInitiated(0, blank_url);
   EXPECT_TRUE(controller.IsURLInPageNavigation(url, true,
       main_test_rfh()));
 
   // Navigate to URL with no refs.
-  main_test_rfh()->SendNavigate(0, url);
+  main_test_rfh()->NavigateAndCommitRendererInitiated(0, url);
 
   // Reloading the page is not an in-page navigation.
   EXPECT_FALSE(controller.IsURLInPageNavigation(url, false,
@@ -3278,7 +3294,7 @@ TEST_F(NavigationControllerTest, IsInPageNavigation) {
       main_test_rfh()));
 
   // Navigate to URL with refs.
-  main_test_rfh()->SendNavigate(1, url_with_ref);
+  main_test_rfh()->NavigateAndCommitRendererInitiated(1, url_with_ref);
 
   // Reloading the page is not an in-page navigation.
   EXPECT_FALSE(controller.IsURLInPageNavigation(url_with_ref, false,
@@ -3317,14 +3333,14 @@ TEST_F(NavigationControllerTest, IsInPageNavigation) {
   EXPECT_TRUE(prefs.allow_universal_access_from_file_urls);
   // Allow in page navigation if existing URL is file scheme.
   const GURL file_url("file:///foo/index.html");
-  main_test_rfh()->SendNavigate(0, file_url);
+  main_test_rfh()->NavigateAndCommitRendererInitiated(0, file_url);
   EXPECT_EQ(0, rph->bad_msg_count());
   EXPECT_TRUE(controller.IsURLInPageNavigation(different_origin_url, true,
       main_test_rfh()));
   EXPECT_EQ(0, rph->bad_msg_count());
   // Don't honor allow_universal_access_from_file_urls if existing URL is
   // not file scheme.
-  main_test_rfh()->SendNavigate(0, url);
+  main_test_rfh()->NavigateAndCommitRendererInitiated(0, url);
   EXPECT_FALSE(controller.IsURLInPageNavigation(different_origin_url, true,
       main_test_rfh()));
   EXPECT_EQ(1, rph->bad_msg_count());
@@ -3351,7 +3367,7 @@ TEST_F(NavigationControllerTest, SameSubframe) {
   NavigationControllerImpl& controller = controller_impl();
   // Navigate the main frame.
   const GURL url("http://www.google.com/");
-  main_test_rfh()->SendNavigate(0, url);
+  main_test_rfh()->NavigateAndCommitRendererInitiated(0, url);
 
   // We should be at the first navigation entry.
   EXPECT_EQ(controller.GetEntryCount(), 1);
@@ -4242,7 +4258,7 @@ TEST_F(NavigationControllerTest, IsInitialNavigation) {
 
   // After commit, it stays false.
   const GURL url1("http://foo1");
-  main_test_rfh()->SendNavigate(0, url1);
+  main_test_rfh()->NavigateAndCommitRendererInitiated(0, url1);
   EXPECT_EQ(1U, navigation_entry_committed_counter_);
   navigation_entry_committed_counter_ = 0;
   EXPECT_FALSE(controller.IsInitialNavigation());
@@ -4265,7 +4281,7 @@ TEST_F(NavigationControllerTest, ClearFaviconOnRedirect) {
   TestNotificationTracker notifications;
   RegisterForAllNavNotifications(&notifications, &controller);
 
-  main_test_rfh()->SendNavigate(0, kPageWithFavicon);
+  main_test_rfh()->NavigateAndCommitRendererInitiated(0, kPageWithFavicon);
   EXPECT_EQ(1U, navigation_entry_committed_counter_);
   navigation_entry_committed_counter_ = 0;
 
@@ -4280,6 +4296,9 @@ TEST_F(NavigationControllerTest, ClearFaviconOnRedirect) {
   favicon_status.valid = true;
   EXPECT_FALSE(DoImagesMatch(kDefaultFavicon, entry->GetFavicon().image));
 
+  main_test_rfh()->SendRendererInitiatedNavigationRequest(kPageWithoutFavicon,
+                                                          false);
+  main_test_rfh()->PrepareForCommit();
   main_test_rfh()->SendNavigateWithTransition(
       0,  // same page ID.
       kPageWithoutFavicon,
@@ -4305,7 +4324,7 @@ TEST_F(NavigationControllerTest, BackNavigationDoesNotClearFavicon) {
   TestNotificationTracker notifications;
   RegisterForAllNavNotifications(&notifications, &controller);
 
-  main_test_rfh()->SendNavigate(0, kUrl1);
+  main_test_rfh()->NavigateAndCommitRendererInitiated(0, kUrl1);
   EXPECT_EQ(1U, navigation_entry_committed_counter_);
   navigation_entry_committed_counter_ = 0;
 
@@ -4319,9 +4338,11 @@ TEST_F(NavigationControllerTest, BackNavigationDoesNotClearFavicon) {
   favicon_status.valid = true;
 
   // Navigate to another page and go back to the original page.
-  main_test_rfh()->SendNavigate(1, kUrl2);
+  main_test_rfh()->NavigateAndCommitRendererInitiated(1, kUrl2);
   EXPECT_EQ(1U, navigation_entry_committed_counter_);
   navigation_entry_committed_counter_ = 0;
+  main_test_rfh()->SendRendererInitiatedNavigationRequest(kUrl1, false);
+  main_test_rfh()->PrepareForCommit();
   main_test_rfh()->SendNavigateWithTransition(
       0,
       kUrl1,
@@ -4425,7 +4446,7 @@ TEST_F(NavigationControllerTest, MAYBE_PurgeScreenshot) {
 
 TEST_F(NavigationControllerTest, PushStateUpdatesTitleAndFavicon) {
   // Navigate.
-  contents()->GetMainFrame()->SendNavigate(1, GURL("http://foo"));
+  main_test_rfh()->NavigateAndCommitRendererInitiated(1, GURL("http://foo"));
 
   // Set title and favicon.
   base::string16 title(base::ASCIIToUTF16("Title"));
@@ -4437,12 +4458,14 @@ TEST_F(NavigationControllerTest, PushStateUpdatesTitleAndFavicon) {
 
   // history.pushState() is called.
   FrameHostMsg_DidCommitProvisionalLoad_Params params;
-  GURL url("http://foo#foo");
+  GURL kUrl2("http://foo#foo");
   params.page_id = 2;
-  params.url = url;
-  params.page_state = PageState::CreateFromURL(url);
+  params.url = kUrl2;
+  params.page_state = PageState::CreateFromURL(kUrl2);
   params.was_within_same_page = true;
-  contents()->GetMainFrame()->SendNavigateWithParams(&params);
+  main_test_rfh()->SendRendererInitiatedNavigationRequest(kUrl2, false);
+  main_test_rfh()->PrepareForCommit();
+  main_test_rfh()->SendNavigateWithParams(&params);
 
   // The title should immediately be visible on the new NavigationEntry.
   base::string16 new_title =
@@ -4522,6 +4545,8 @@ TEST_F(NavigationControllerTest, PostThenReplaceStateThenReload) {
   params.was_within_same_page = false;
   params.is_post = true;
   params.post_id = 2;
+  main_test_rfh()->SendRendererInitiatedNavigationRequest(url, false);
+  main_test_rfh()->PrepareForCommit();
   contents()->GetMainFrame()->SendNavigateWithParams(&params);
 
   // history.replaceState() is called.
@@ -4534,6 +4559,8 @@ TEST_F(NavigationControllerTest, PostThenReplaceStateThenReload) {
   params.was_within_same_page = true;
   params.is_post = false;
   params.post_id = -1;
+  main_test_rfh()->SendRendererInitiatedNavigationRequest(replace_url, false);
+  main_test_rfh()->PrepareForCommit();
   contents()->GetMainFrame()->SendNavigateWithParams(&params);
 
   // Now reload. replaceState overrides the POST, so we should not show a