PlzNavigate: implement CommitNavigation on the browser side

content/browser/frame_host/render_frame_host_manager_unittest.cc

 // Copyright 2013 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/files/file_path.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/time/time.h"
 #include "content/browser/frame_host/cross_site_transferring_request.h"
 #include "content/browser/frame_host/navigation_before_commit_info.h"
@@ skipping 21 matching lines @@
 #include "content/public/common/page_transition_types.h"
 #include "content/public/common/url_constants.h"
 #include "content/public/common/url_utils.h"
 #include "content/public/test/mock_render_process_host.h"
 #include "content/public/test/test_notification_tracker.h"
 #include "content/test/test_content_browser_client.h"
 #include "content/test/test_content_client.h"
 #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 {
 namespace {
 
 class RenderFrameHostManagerTestWebUIControllerFactory
     : public WebUIControllerFactory {
  public:
   RenderFrameHostManagerTestWebUIControllerFactory()
     : should_create_webui_(false) {
@@ skipping 328 matching lines @@
 
   NavigationRequest* GetNavigationRequestForRenderFrameManager(
       RenderFrameHostManager* manager) const {
     return manager->navigation_request_for_testing();
   }
 
   void EnableBrowserSideNavigation() {
     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_;
 };
 
 // Tests that when you navigate from a chrome:// url to another page, and
 // then do that same thing in another tab, that the two resulting pages have
 // different SiteInstances, BrowsingInstances, and RenderProcessHosts. This is
 // a regression test for bug 9364.
 TEST_F(RenderFrameHostManagerTest, NewTabPageProcesses) {
@@ skipping 1303 matching lines @@
   // when CommitNavigation is properly implemented.
   // Navigate to the first page.
   contents()->NavigateAndCommit(kUrl1);
 
   EnableBrowserSideNavigation();
   // Add a subframe.
   TestRenderFrameHost* subframe_rfh = static_cast<TestRenderFrameHost*>(
       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.
   EXPECT_EQ(
       kUrl1, subframe_request->info().first_party_for_cookies);
   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);
   EXPECT_TRUE(main_request->info().is_main_frame);
   EXPECT_FALSE(main_request->info().parent_is_main_frame);
 }
 
 // PlzNavigate: Test that RequestNavigation creates a NavigationRequest and that
 // RenderFrameHost is not modified when the navigation commits.
 TEST_F(RenderFrameHostManagerTest,
        BrowserSideNavigationRequestNavigationNoLiveRenderer) {
   const GURL kUrl("http://www.google.com/");
 
   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.
   EXPECT_TRUE(main_request != NULL);
   RenderFrameHostImpl* rfh = main_test_rfh();
 
   // Now commit the same url.
   NavigationBeforeCommitInfo commit_info;
   commit_info.navigation_url = kUrl;
   render_manager->CommitNavigation(commit_info);
@@ skipping 15 matching lines @@
   // Navigate to the first page.
   contents()->NavigateAndCommit(kUrl1);
   TestRenderViewHost* rvh1 = test_rvh();
   EXPECT_EQ(RenderViewHostImpl::STATE_DEFAULT, rvh1->rvh_state());
   RenderFrameHostImpl* rfh = main_test_rfh();
   RenderFrameHostManager* render_manager =
       main_test_rfh()->frame_tree_node()->render_manager();
 
   EnableBrowserSideNavigation();
   // Navigate to a different site.
+  StartBrowserSideNavigation(kUrl2, render_manager);
   main_test_rfh()->SendBeginNavigationWithURL(kUrl2);
   NavigationRequest* main_request =
       GetNavigationRequestForRenderFrameManager(render_manager);
   ASSERT_TRUE(main_request);
 
   NavigationBeforeCommitInfo commit_info;
   commit_info.navigation_url = kUrl2;
   render_manager->CommitNavigation(commit_info);
   main_request = GetNavigationRequestForRenderFrameManager(render_manager);
   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