PlzNavigate: identify same-page browser-initiated navigation.

content/browser/frame_host/navigation_controller_impl_browsertest.cc

Index: content/browser/frame_host/navigation_controller_impl_browsertest.cc
diff --git a/content/browser/frame_host/navigation_controller_impl_browsertest.cc b/content/browser/frame_host/navigation_controller_impl_browsertest.cc
index af3a4999865428e40593d9cde1d99fc5b55f3435..533082ca99dee4b76058245029e0121e6ed48e44 100644
--- a/content/browser/frame_host/navigation_controller_impl_browsertest.cc
+++ b/content/browser/frame_host/navigation_controller_impl_browsertest.cc
@@ -6166,10 +6166,23 @@ IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest,
   EXPECT_EQ(cross_origin_url, web_contents->GetLastCommittedURL());
   EXPECT_EQ(2, web_contents->GetController().GetLastCommittedEntryIndex());
 
+  if (IsBrowserSideNavigationEnabled()) {
+    // With browser-side-navigation, the history navigation is dropped.

[nasko, 2017/01/13 02:51:09]

I thought the discussion we had concluded that we would like to just issue a new
navigation when the race condition is hit.

[arthursonzogni, 2017/01/13 15:18:37]

It's difficult to relaunch every navigations. I have problems with history
navigation especially.
Maybe we can discuss about it again.

[nasko, 2017/01/13 19:36:56]

Why would you need to relaunch every navigation? I think the resolution in the
renderer is already present - it converts the SameDocument load type into a
DifferentDocument load type, which should cause a new navigation to be started.
Sure, feel free to start an email discussion or schedule a VC chat.

[clamy, 2017/01/16 12:16:11]

In order to properly resolve it as a different document navigation it needs to
be sent back to the browser. Since we're trying to commit the navigation, it
won't be sent to the browser by default (we have checkForBrowserSideNavigation
set to false in commits). We somehow need to set it to true, so that the now
different document navigation will be sent to the browser.

We could avoid passing the boolean to the FrameLoader by creating the
WebURLRequest with checkForBrowserSideNavigation set to true when we expect to
commit a same site navigation, and false when we expect a different document
load. That said, I'm not sure it's clearer compared to setting it to false when
we detect that the same document navigation won't be able to commit in the
FrameLoader.

We should really think about whether we want to try to relaunch the navigation
instead of dropping it altogether. Another issue with relaunching it from the
renderer is that we drop the information that it was browser-initiated, which is
problematic.

+    // Verify the expected origin through JavaScript. It also has the additional

[nasko, 2017/01/13 02:51:09]

This comment doesn't seem correct, as the code doesn't verify the origin, it
only checks for the string 'alive'.

[arthursonzogni, 2017/01/13 15:18:37]

Done.

+    // verification of the process also being still alive.
+    std::string is_alive;
+    EXPECT_TRUE(ExecuteScriptAndExtractString(
+        web_contents, "domAutomationController.send('alive')", &is_alive));
+    EXPECT_EQ("alive", is_alive);
+    EXPECT_EQ(2, web_contents->GetController().GetLastCommittedEntryIndex());
+    EXPECT_EQ(3, web_contents->GetController().GetEntryCount());
+    return;

[nasko, 2017/01/13 02:51:09]

Let's use an else block so it is more clear that they are exclusive.

[arthursonzogni, 2017/01/13 15:18:37]

Done.

+  }
   // Wait for the back navigation to commit as well.
   history_commit_observer.Wait();
-  EXPECT_EQ(start_url, web_contents->GetLastCommittedURL());

[nasko, 2017/01/13 02:51:09]

Why drop this expectation?

[arthursonzogni, 2017/01/13 15:18:37]

It's a mistake, thanks!

+
   EXPECT_EQ(0, web_contents->GetController().GetLastCommittedEntryIndex());
+  EXPECT_EQ(3, web_contents->GetController().GetEntryCount());
 
   // Verify the expected origin through JavaScript. It also has the additional
   // verification of the process also being still alive.