PlzNavigate: have renderer-initiated navigations be same-process

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 86426798941987851a0fdb610797fbb7f9769caa..944181cfb277be5526e070b056ef53e3c44b0bc6 100644
--- a/content/browser/frame_host/navigation_controller_impl_unittest.cc
+++ b/content/browser/frame_host/navigation_controller_impl_unittest.cc
@@ -35,6 +35,7 @@
 #include "content/public/test/mock_render_process_host.h"
 #include "content/public/test/test_notification_tracker.h"
 #include "content/public/test/test_utils.h"
+#include "content/test/test_navigation_url_loader.h"
 #include "content/test/test_render_frame_host.h"
 #include "content/test/test_render_view_host.h"
 #include "content/test/test_web_contents.h"
@@ -245,6 +246,29 @@ class NavigationControllerTest
     return get<0>(nav_params).common_params.url;
   }
 
+  void SimulateServerRedirectIfPlzNavigate(TestRenderFrameHost* test_rfh,

[clamy, 2015/02/23 10:52:34]

Please rename that method SimulateServerRedirect and add a // PlzNavigate
comment above. We do not want to have the name PlzNavigate anywhere in function
names and variables.

[carlosk, 2015/03/04 19:42:40]

Change was moved to another CL.

+                                           const GURL& redirect_url) {
+    if (!base::CommandLine::ForCurrentProcess()->HasSwitch(
+            switches::kEnableBrowserSideNavigation)) {
+      return;
+    }
+
+    NavigatorImpl* navigator_impl =
+        static_cast<NavigatorImpl*>(test_rfh->frame_tree_node()->navigator());
+    NavigationRequest* request =
+        navigator_impl->GetNavigationRequestForNodeForTesting(
+            test_rfh->frame_tree_node());
+    CHECK(request);
+
+    if (request->state() == NavigationRequest::WAITING_FOR_RENDERER_RESPONSE)
+      test_rfh->SendBeforeUnloadACK(true);
+

[clamy, 2015/02/23 10:52:34]

CHECK that the request status is STARTED here (the patch on data urls introduce
a path that may result in the request going from WAITING_FOR_RENDERER_RESPONSE
to RESPONSE_STARTED).

[carlosk, 2015/03/04 19:42:40]

Change was moved to another CL.

+    TestNavigationURLLoader* loader =
+        static_cast<TestNavigationURLLoader*>(request->loader_for_testing());
+    CHECK(loader);
+    loader->SimulateServerRedirect(redirect_url);
+  }
+
  protected:
   GURL navigated_url_;
   size_t navigation_entry_committed_counter_;
@@ -320,6 +344,7 @@ TEST_F(NavigationControllerTest, GoToOffset) {
     urls[i] = GURL(base::StringPrintf("http://www.a.com/%d", i));
   }
 
+  main_test_rfh()->RendererRequestIfPlzNavigate(urls[0], true);

[carlosk, 2015/02/20 19:16:51]

For each SendNavigate call the intent might be different as it might be:
* A reply from the renderer carrying on with a browser-initiated navigation
* Same as above but after one or more server redirects happened
* A renderer-initiated user-initiated navigation
* A renderer-initiated non-user-initiated navigation

As those cases are handled differently in PlzNavigate world I did my best to
interpret each situation. I made comments on a few cases but please let me know
if you notice something I missed.

   main_test_rfh()->PrepareForCommit(urls[0]);
   main_test_rfh()->SendNavigate(0, urls[0]);
   EXPECT_EQ(1U, navigation_entry_committed_counter_);
@@ -330,6 +355,7 @@ TEST_F(NavigationControllerTest, GoToOffset) {
   EXPECT_FALSE(controller.CanGoToOffset(1));
 
   for (int i = 1; i <= 4; ++i) {
+    main_test_rfh()->RendererRequestIfPlzNavigate(urls[i], true);
     main_test_rfh()->PrepareForCommit(urls[i]);
     main_test_rfh()->SendNavigate(i, urls[i]);
     EXPECT_EQ(1U, navigation_entry_committed_counter_);
@@ -844,6 +870,7 @@ TEST_F(NavigationControllerTest, LoadURL_ExistingPending) {
 
   // Before that commits, do a new navigation.
   const GURL kNewURL("http://foo/see");
+  main_test_rfh()->RendererRequestIfPlzNavigate(kNewURL, true);
   main_test_rfh()->PrepareForCommit(kNewURL);
   main_test_rfh()->SendNavigate(3, kNewURL);
 
@@ -897,6 +924,7 @@ TEST_F(NavigationControllerTest, LoadURL_PrivilegedPending) {
 
   // Before that commits, do a new navigation.
   const GURL kNewURL("http://foo/bee");
+  foo_rfh->RendererRequestIfPlzNavigate(kNewURL, true);
   foo_rfh->PrepareForCommit(kNewURL);
   foo_rfh->SendNavigate(3, kNewURL);
 
@@ -945,6 +973,7 @@ TEST_F(NavigationControllerTest, LoadURL_BackPreemptsPending) {
   EXPECT_EQ(1, controller.GetLastCommittedEntryIndex());
 
   // Before that commits, a back navigation from the renderer commits.
+  main_test_rfh()->RendererRequestIfPlzNavigate(kExistingURL1, true);
   main_test_rfh()->PrepareForCommit(kExistingURL1);
   main_test_rfh()->SendNavigate(0, kExistingURL1);
 
@@ -985,6 +1014,7 @@ TEST_F(NavigationControllerTest, LoadURL_IgnorePreemptsPending) {
 
   // Before that commits, a document.write and location.reload can cause the
   // renderer to send a FrameNavigate with page_id -1.
+  main_test_rfh()->RendererRequestIfPlzNavigate(kExistingURL, true);

[carlosk, 2015/02/20 19:16:51]

This one I am really unsure about. From test comment it seems like a
non-user-initiated navigation.
Now from the referred bug's description it seems like it would be triggered by a
re-layout of the page that I think could be caused by the user re-sizing the
window so user initiated? I assumed this was the case.
Ideas?

[clamy, 2015/02/23 10:52:34]

The way blink determines if a request has a user gesture is whether it was
processing a user gesture at the same time. So if it is coming from a resize
then I guess it would be user initiated, but it could also be coming from pure
javascript in which case it would not. In any case, we expect a commit here, so
I guess it should probably stay as user-initiated.

[carlosk, 2015/03/04 19:42:40]

Ack. Also with the current cancellation policy a non-user-initiated request here
would be ignored.

   main_test_rfh()->PrepareForCommit(kExistingURL);
   main_test_rfh()->SendNavigate(-1, kExistingURL);
 
@@ -1249,6 +1279,7 @@ TEST_F(NavigationControllerTest, Reload_GeneratesNewPage) {
   controller.Reload(true);
   EXPECT_EQ(0U, notifications.size());
 
+  SimulateServerRedirectIfPlzNavigate(main_test_rfh(), url2);

[carlosk, 2015/02/20 19:16:51]

I guess that "a reload navigation produces a new page" means that the user did
hit Reload but the server redirected it to a new page.

[clamy, 2015/02/23 10:52:34]

Acknowledged.

   main_test_rfh()->PrepareForCommit(url2);
   main_test_rfh()->SendNavigate(1, url2);
   EXPECT_EQ(1U, navigation_entry_committed_counter_);
@@ -1309,6 +1340,7 @@ TEST_F(NavigationControllerTest, ReloadOriginalRequestURL) {
   controller.LoadURL(
       original_url, Referrer(), ui::PAGE_TRANSITION_TYPED, std::string());
   EXPECT_EQ(0U, notifications.size());
+  SimulateServerRedirectIfPlzNavigate(main_test_rfh(), final_url);
   main_test_rfh()->PrepareForCommit(final_url);
   main_test_rfh()->SendNavigateWithOriginalRequestURL(
       0, final_url, original_url);
@@ -1342,6 +1374,7 @@ TEST_F(NavigationControllerTest, ReloadOriginalRequestURL) {
   EXPECT_TRUE(controller.GetVisibleEntry()->GetTitle().empty());
 
   // Send that the navigation has proceeded; say it got redirected again.
+  SimulateServerRedirectIfPlzNavigate(main_test_rfh(), final_url);
   main_test_rfh()->PrepareForCommit(final_url);
   main_test_rfh()->SendNavigate(0, final_url);
   EXPECT_EQ(1U, navigation_entry_committed_counter_);
@@ -1536,6 +1569,7 @@ TEST_F(NavigationControllerTest, Back_GeneratesNewPage) {
   EXPECT_FALSE(controller.CanGoBack());
   EXPECT_TRUE(controller.CanGoForward());
 
+  SimulateServerRedirectIfPlzNavigate(main_test_rfh(), url3);

[carlosk, 2015/02/20 19:16:51]

Same as previous comment for "a reload navigation produces a new page" but now
for "back".

[clamy, 2015/02/23 10:52:34]

Acknowledged.

   main_test_rfh()->PrepareForCommit(url3);
   main_test_rfh()->SendNavigate(2, url3);
   EXPECT_EQ(1U, navigation_entry_committed_counter_);
@@ -1594,14 +1628,18 @@ TEST_F(NavigationControllerTest, Back_OtherBackPending) {
   const GURL kUrl3("http://foo/3");
 
   // First navigate three places so we have some back history.
+  main_test_rfh()->RendererRequestIfPlzNavigate(kUrl1, true);
   main_test_rfh()->PrepareForCommit(kUrl1);
   main_test_rfh()->SendNavigate(0, kUrl1);
+  main_test_rfh()->RendererRequestIfPlzNavigate(kUrl2, true);
   main_test_rfh()->PrepareForCommit(kUrl2);
   main_test_rfh()->SendNavigate(1, kUrl2);
+  main_test_rfh()->RendererRequestIfPlzNavigate(kUrl3, true);
   main_test_rfh()->PrepareForCommit(kUrl3);
   main_test_rfh()->SendNavigate(2, kUrl3);
 
   // With nothing pending, say we get a navigation to the second entry.
+  main_test_rfh()->RendererRequestIfPlzNavigate(kUrl2, true);
   main_test_rfh()->PrepareForCommit(kUrl2);
   main_test_rfh()->SendNavigate(1, kUrl2);
 
@@ -1627,8 +1665,9 @@ TEST_F(NavigationControllerTest, Back_OtherBackPending) {
   EXPECT_EQ(1, controller.GetPendingEntryIndex());
   EXPECT_EQ(2, controller.GetLastCommittedEntryIndex());
 
-  // Not synthesize a totally new back event to the first page. This will not
+  // Now synthesize a totally new back event to the first page. This will not
   // match the pending one.
+  main_test_rfh()->RendererRequestIfPlzNavigate(kUrl1, true);
   main_test_rfh()->PrepareForCommit(kUrl1);
   main_test_rfh()->SendNavigate(0, kUrl1);
 
@@ -2624,8 +2663,8 @@ TEST_F(NavigationControllerTest, Interstitial) {
 
   // Now navigate somewhere with an interstitial.
   const GURL url2("http://bar");
-  controller.LoadURL(
-      url1, Referrer(), ui::PAGE_TRANSITION_TYPED, std::string());
+  controller.LoadURL(url2, Referrer(), ui::PAGE_TRANSITION_TYPED,
+                     std::string());

[carlosk, 2015/02/20 19:16:51]

The previous URL value here seemed to not be what the author meant to use.

[clamy, 2015/02/23 10:52:34]

Acknowledged.

   NavigationEntryImpl::FromNavigationEntry(controller.GetPendingEntry())->
       set_page_type(PAGE_TYPE_INTERSTITIAL);
 
@@ -2807,6 +2846,7 @@ TEST_F(NavigationControllerTest, TransientEntry) {
   transient_entry->SetURL(transient_url);
   controller.SetTransientEntry(transient_entry);
   EXPECT_EQ(transient_url, controller.GetVisibleEntry()->GetURL());
+  main_test_rfh()->RendererRequestIfPlzNavigate(url3, true);

[carlosk, 2015/02/20 19:16:51]

Again not really sure about these ones in this test. I assumed they were all
renderer-initiated requests.

[clamy, 2015/02/23 10:52:34]

Acknowledged.

   main_test_rfh()->PrepareForCommit(url3);
   main_test_rfh()->SendNavigate(3, url3);
   // Transient entry should be gone.
@@ -2836,6 +2876,8 @@ TEST_F(NavigationControllerTest, TransientEntry) {
   // Transient entry should be gone.
   EXPECT_EQ(url4, controller.GetVisibleEntry()->GetURL());
   EXPECT_EQ(controller.GetEntryCount(), 5);
+
+  main_test_rfh()->RendererRequestIfPlzNavigate(url3, false);
   main_test_rfh()->PrepareForCommit(url3);
   main_test_rfh()->SendNavigate(3, url3);
 
@@ -2889,6 +2931,8 @@ TEST_F(NavigationControllerTest, TransientEntry) {
   transient_entry->SetURL(transient_url);
   controller.SetTransientEntry(transient_entry);
   EXPECT_EQ(transient_url, controller.GetVisibleEntry()->GetURL());
+
+  main_test_rfh()->RendererRequestIfPlzNavigate(url3_ref, false);
   main_test_rfh()->PrepareForCommit(url3_ref);
   main_test_rfh()->SendNavigate(3, url3_ref);
   // Transient entry should be gone.