PlzNavigate: implement CommitNavigation on the browser side

content/browser/frame_host/render_frame_host_manager_unittest.cc

Index: content/browser/frame_host/render_frame_host_manager_unittest.cc
diff --git a/content/browser/frame_host/render_frame_host_manager_unittest.cc b/content/browser/frame_host/render_frame_host_manager_unittest.cc
index acf5f139c4767ecea696694d8bceea8537a41bcd..0cda064c62e4d35f08f5bffbb201e26e747a19f1 100644
--- a/content/browser/frame_host/render_frame_host_manager_unittest.cc
+++ b/content/browser/frame_host/render_frame_host_manager_unittest.cc
@@ -39,6 +39,7 @@
 #include "content/test/test_render_frame_host.h"
 #include "content/test/test_render_view_host.h"
 #include "content/test/test_web_contents.h"
+#include "net/base/load_flags.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace content {
@@ -387,6 +388,44 @@ class RenderFrameHostManagerTest
     CommandLine::ForCurrentProcess()->AppendSwitch(
         switches::kEnableBrowserSideNavigation);
   }
+
+  void StartBrowserSideNavigation(const GURL& url,

[(Do not use) nasko, 2014/08/28 16:39:09]

nit: In general we talk about navigating in a node of the frame tree. It is
really not much different than level of indication, but might be nice to take
FTN instead of RFHM.

[clamy, 2014/09/02 18:25:19]

Done.

+                                  RenderFrameHostManager* manager) {
+    StartBrowserSideNavigationWithParameters(
+        url, manager, Referrer(), PAGE_TRANSITION_LINK,
+        NavigationController::NO_RELOAD);
+  }
+
+  void StartBrowserSideNavigationWithParameters(
+      const GURL& url,
+      RenderFrameHostManager* manager,
+      const Referrer& referrer,
+      PageTransition transition_type,
+      NavigationController::ReloadType reload_type) {
+    scoped_ptr<NavigationEntryImpl> entry(
+        NavigationEntryImpl::FromNavigationEntry(
+            NavigationController::CreateNavigationEntry(
+            url, referrer, transition_type, false, std::string(),
+            controller().GetBrowserContext())));
+    FrameMsg_Navigate_Params navigate_params;
+    NavigatorImpl::MakeNavigateParams(
+        *entry.get(),

[(Do not use) nasko, 2014/08/28 16:39:09]

nit: no need to call .get() on a scoped_ptr. It has operator* and operator->
overloaded to get the internal pointer.

[clamy, 2014/09/02 18:25:19]

Done.

+        static_cast<NavigatorImpl*>(manager->frame_tree_node_->navigator()),
+        reload_type,
+        base::TimeTicks::Now(), &navigate_params);
+    manager->RequestNavigation(
+        NavigatorImpl::MakeCommitNavigationParams(
+            *entry.get(),
+            static_cast<NavigatorImpl*>(manager->frame_tree_node_->navigator()),
+            reload_type),
+        navigate_params);
+  }
+
+  FrameMsg_CommitNavigation_Params* GetCommitParamsForRenderFrameManager(

[(Do not use) nasko, 2014/08/28 16:39:09]

nit: The name of this method is almost as long as the actual code. Is it worth
it? I have harder time reading the name than the actual code inside it.

[clamy, 2014/09/02 18:25:19]

Done.

+      RenderFrameHostManager* manager) {
+    return manager->commit_navigation_params_.get();
+  }
+
  private:
   RenderFrameHostManagerTestWebUIControllerFactory factory_;
   scoped_ptr<FrameLifetimeConsistencyChecker> lifetime_checker_;
@@ -1710,11 +1749,13 @@ TEST_F(RenderFrameHostManagerTest, BrowserSideNavigationBeginNavigation) {
       contents()->GetFrameTree()->AddFrame(
           contents()->GetFrameTree()->root(), 14, "Child"));
 
+  RenderFrameHostManager* subframe_manager =
+      subframe_rfh->frame_tree_node()->render_manager();
+  StartBrowserSideNavigation(kUrl2, subframe_manager);
   // Simulate a BeginNavigation IPC on the subframe.
   subframe_rfh->SendBeginNavigationWithURL(kUrl2);
   NavigationRequest* subframe_request =
-      GetNavigationRequestForRenderFrameManager(
-          subframe_rfh->frame_tree_node()->render_manager());
+      GetNavigationRequestForRenderFrameManager(subframe_manager);
   ASSERT_TRUE(subframe_request);
   EXPECT_EQ(kUrl2, subframe_request->info().navigation_params.url);
   // First party for cookies url should be that of the main frame.
@@ -1723,10 +1764,13 @@ TEST_F(RenderFrameHostManagerTest, BrowserSideNavigationBeginNavigation) {
   EXPECT_FALSE(subframe_request->info().is_main_frame);
   EXPECT_TRUE(subframe_request->info().parent_is_main_frame);
 
+  RenderFrameHostManager* main_frame_manager =
+      contents()->GetMainFrame()->frame_tree_node()->render_manager();
+  StartBrowserSideNavigation(kUrl2, main_frame_manager);

[(Do not use) nasko, 2014/08/28 16:39:09]

Reading this as Start and then next line as Send, they seem like they do the
same thing. Maybe rename the initial one to be SendRequestNavigation*, so it
matches the SendBeginNavigation and form the pattern of Send<IPC Name>*.

[clamy, 2014/09/03 22:23:44]

Done.

   // Simulate a BeginNavigation IPC on the main frame.
   contents()->GetMainFrame()->SendBeginNavigationWithURL(kUrl3);
   NavigationRequest* main_request = GetNavigationRequestForRenderFrameManager(
-      contents()->GetMainFrame()->frame_tree_node()->render_manager());
+      main_frame_manager);
   ASSERT_TRUE(main_request);
   EXPECT_EQ(kUrl3, main_request->info().navigation_params.url);
   EXPECT_EQ(kUrl3, main_request->info().first_party_for_cookies);
@@ -1742,10 +1786,9 @@ TEST_F(RenderFrameHostManagerTest,
 
   EnableBrowserSideNavigation();
   EXPECT_FALSE(main_test_rfh()->render_view_host()->IsRenderViewLive());
-  contents()->GetController().LoadURL(
-      kUrl, Referrer(), PAGE_TRANSITION_LINK, std::string());
   RenderFrameHostManager* render_manager =
       main_test_rfh()->frame_tree_node()->render_manager();
+  StartBrowserSideNavigation(kUrl, render_manager);
   NavigationRequest* main_request =
       GetNavigationRequestForRenderFrameManager(render_manager);
   // A NavigationRequest should have been generated.
@@ -1781,6 +1824,7 @@ TEST_F(RenderFrameHostManagerTest,
 
   EnableBrowserSideNavigation();
   // Navigate to a different site.
+  StartBrowserSideNavigation(kUrl2, render_manager);
   main_test_rfh()->SendBeginNavigationWithURL(kUrl2);
   NavigationRequest* main_request =
       GetNavigationRequestForRenderFrameManager(render_manager);
@@ -1793,4 +1837,47 @@ TEST_F(RenderFrameHostManagerTest,
   EXPECT_NE(main_test_rfh(), rfh);
 }
 
+// PlzNavigate: Test that a reload navigation is properly signaled to the
+// renderer when the navigation can commit.

[(Do not use) nasko, 2014/08/28 16:39:09]

Do we need to pass the command line flag to exercise these tests? If yes, we
should probably put that in the comments, so other devs can easily understand
why those aren't run in default case.

[clamy, 2014/09/03 22:23:44]

Done.

+TEST_F(RenderFrameHostManagerTest,
+       BrowserSideNavigationReload) {
+  const GURL kUrl("http://www.google.com/");
+
+  EnableBrowserSideNavigation();
+  RenderFrameHostManager* render_manager =
+      main_test_rfh()->frame_tree_node()->render_manager();
+  StartBrowserSideNavigationWithParameters(

[(Do not use) nasko, 2014/08/28 16:39:09]

It sounds wrong to reload when we have no committed navigation. Can we ensure we
properly handle this case?

[clamy, 2014/09/03 22:23:44]

Done.

+      kUrl, render_manager, Referrer(), PAGE_TRANSITION_LINK,
+      NavigationController::RELOAD);
+  // A NavigationRequest should have been generated.
+  NavigationRequest* main_request =
+      GetNavigationRequestForRenderFrameManager(render_manager);
+  ASSERT_TRUE(main_request != NULL);
+  EXPECT_EQ(net::LOAD_NORMAL | net::LOAD_VALIDATE_CACHE,
+            main_request->info().navigation_params.load_flags);
+  // CommitNavigation parameters should have been generated.
+  FrameMsg_CommitNavigation_Params* commit_navigation_params =
+      GetCommitParamsForRenderFrameManager(render_manager);
+  ASSERT_TRUE(commit_navigation_params != NULL);
+  EXPECT_EQ(FrameMsg_Navigate_Type::RELOAD,
+            commit_navigation_params->navigation_type);
+
+  // Now do a shift+reload.

[(Do not use) nasko, 2014/08/28 16:39:09]

Shouldn't we commit the reload before issuing another one? Unless the intent is
to interleave the two.

[clamy, 2014/09/03 22:23:44]

Done.

+  StartBrowserSideNavigationWithParameters(
+      kUrl, render_manager, Referrer(), PAGE_TRANSITION_LINK,
+      NavigationController::RELOAD_IGNORING_CACHE);
+  // A NavigationRequest should have been generated.
+  main_request =
+      GetNavigationRequestForRenderFrameManager(render_manager);
+  ASSERT_TRUE(main_request != NULL);
+  EXPECT_EQ(net::LOAD_NORMAL | net::LOAD_BYPASS_CACHE,
+            main_request->info().navigation_params.load_flags);
+  // CommitNavigation parameters should have been generated.
+  commit_navigation_params =
+      GetCommitParamsForRenderFrameManager(render_manager);
+  ASSERT_TRUE(commit_navigation_params != NULL);
+  EXPECT_EQ(FrameMsg_Navigate_Type::RELOAD_IGNORING_CACHE,
+            commit_navigation_params->navigation_type);
+}
+
 }  // namespace content