Add tests for navigation type classifications.

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 adeefa1510b8b5918cc1405f10e7f3ba0e5ef971..5d138d435bc53713da00f32b7e222d66fb097670 100644
--- a/content/browser/frame_host/navigation_controller_impl_browsertest.cc
+++ b/content/browser/frame_host/navigation_controller_impl_browsertest.cc
@@ -158,7 +158,7 @@ IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest,
   EXPECT_TRUE(ExecuteScript(shell()->web_contents(),
                             "window.open('" + page_url + "', '_blank')"));
   Shell* shell2 = observer.GetShell();
-  WaitForLoadStop(shell2->web_contents());
+  EXPECT_TRUE(WaitForLoadStop(shell2->web_contents()));
 
   EXPECT_EQ(1, shell2->web_contents()->GetController().GetEntryCount());
   EXPECT_EQ(1, RendererHistoryLength(shell2));
@@ -170,7 +170,7 @@ IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest,
 
 namespace {
 
-struct FrameNavigateParamsCapturer : public WebContentsObserver {
+class FrameNavigateParamsCapturer : public WebContentsObserver {
  public:
   // Observes navigation for the specified |node|.
   explicit FrameNavigateParamsCapturer(FrameTreeNode* node)
@@ -202,6 +202,11 @@ struct FrameNavigateParamsCapturer : public WebContentsObserver {
 
     params_ = params;
     details_ = details;
+    if (!web_contents()->IsLoading())
+      message_loop_runner_->Quit();
+  }
+
+  void DidStopLoading(RenderViewHost* render_view_host) override {
     message_loop_runner_->Quit();
   }
 
@@ -218,7 +223,7 @@ struct FrameNavigateParamsCapturer : public WebContentsObserver {
   scoped_refptr<MessageLoopRunner> message_loop_runner_;
 };
 
-struct LoadCommittedCapturer : public WebContentsObserver {
+class LoadCommittedCapturer : public WebContentsObserver {
  public:
   // Observes the load commit for the specified |node|.
   explicit LoadCommittedCapturer(FrameTreeNode* node)
@@ -263,6 +268,11 @@ struct LoadCommittedCapturer : public WebContentsObserver {
       return;
 
     transition_type_ = transition_type;
+    if (!web_contents()->IsLoading())
+      message_loop_runner_->Quit();
+  }
+
+  void DidStopLoading(RenderViewHost* render_view_host) override {
     message_loop_runner_->Quit();
   }
 
@@ -278,9 +288,258 @@ struct LoadCommittedCapturer : public WebContentsObserver {
 
 }  // namespace
 
-// Verify that the distinction between manual and auto subframes is properly set
-// for subframe navigations. TODO(avi): It's rather bogus that the same info is
-// in two different enums; http://crbug.com/453555.
+// Various tests for navigation type classifications. TODO(avi): It's rather
+// bogus that the same info is in two different enums; http://crbug.com/453555.
+
+// Verify that navigations for NAVIGATION_TYPE_NEW_PAGE are correctly
+// classified.
+IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest,
+                       NavigationTypeClassification_NewPage) {
+  NavigateToURL(shell(), GURL("about:blank"));
+
+  FrameTreeNode* root =
+      static_cast<WebContentsImpl*>(shell()->web_contents())->
+          GetFrameTree()->root();
+
+  {
+    // Simple load.
+    FrameNavigateParamsCapturer capturer(root);
+    GURL frame_url(embedded_test_server()->GetURL(
+        "/navigation_controller/page_with_links.html"));
+    NavigateFrameToURL(root, frame_url);
+    capturer.Wait();
+    // TODO(avi,creis): Why is this (and quite a few others below) a "link"
+    // transition? Lots of these transitions should be cleaned up.
+    EXPECT_EQ(ui::PAGE_TRANSITION_LINK, capturer.params().transition);
+    EXPECT_EQ(NAVIGATION_TYPE_NEW_PAGE, capturer.details().type);
+  }
+
+  {
+    // Load via a fragment link click.
+    FrameNavigateParamsCapturer capturer(root);
+    std::string script = "document.getElementById('fraglink').click()";
+    EXPECT_TRUE(content::ExecuteScript(root->current_frame_host(), script));
+    capturer.Wait();
+    EXPECT_EQ(ui::PAGE_TRANSITION_LINK, capturer.params().transition);
+    // TODO(avi,creis): Why is this not NAVIGATION_TYPE_IN_PAGE? It makes sense

[Charlie Reis, 2015/03/09 20:39:58]

Yes, I think we'll probably accept the current behavior (since both in-page and
cross-page create new NavEntries in the same way and only the back/forward cases
differ) and should do a better job of documenting it.

[Avi (use Gerrit), 2015/03/09 21:35:56]

Acknowledged.

+    // if NEW_PAGE means "new NavEntry".
+    EXPECT_EQ(NAVIGATION_TYPE_NEW_PAGE, capturer.details().type);
+  }
+
+  {
+    // Load via link click.
+    FrameNavigateParamsCapturer capturer(root);
+    std::string script = "document.getElementById('thelink').click()";
+    EXPECT_TRUE(content::ExecuteScript(root->current_frame_host(), script));
+    capturer.Wait();
+    EXPECT_EQ(ui::PAGE_TRANSITION_LINK, capturer.params().transition);
+    EXPECT_EQ(NAVIGATION_TYPE_NEW_PAGE, capturer.details().type);
+  }
+
+  {
+    // location.assign().
+    FrameNavigateParamsCapturer capturer(root);
+    GURL frame_url(embedded_test_server()->GetURL(
+        "/navigation_controller/simple_page_2.html"));
+    std::string script = "location.assign('" + frame_url.spec() + "')";
+    EXPECT_TRUE(content::ExecuteScript(root->current_frame_host(), script));
+    capturer.Wait();
+    EXPECT_EQ(ui::PAGE_TRANSITION_LINK | ui::PAGE_TRANSITION_CLIENT_REDIRECT,

[Charlie Reis, 2015/03/09 20:39:58]

This gets treated as a client redirect?  I'm somewhat surprised.  I thought it
stopped being a client redirect once the onload event of the previous navigation
runs.

(e.g., Client redirect: commit page 1, then navigate to page 2 before onload.
Not client redirect: commit page 1, run onload event, then at some later time
navigate to page 2.)

Guess I was wrong?  Is any non-user-initiated navigation treated as a client
redirect?  Maybe we should mention that in a comment.

[Avi (use Gerrit), 2015/03/09 21:35:56]

I don't know what their intention is. It might be intentional, it might be an
accident of how they implement the handling of real client-side redirects.

We need to very much clarify what the transitions are and when they are
expected. There should be some unit tests here. (See my transition type question
on line 311, etc.) In any case, let's document what is.

[Charlie Reis, 2015/03/09 22:04:05]

Agreed.  (I also don't know the answer to line 311.  It seems like LINK is the
default and may be used in more places than it should.)

+              capturer.params().transition);
+    EXPECT_EQ(NAVIGATION_TYPE_NEW_PAGE, capturer.details().type);
+  }
+
+  {
+    // history.pushState().
+    FrameNavigateParamsCapturer capturer(root);
+    std::string script =
+        "history.pushState({}, 'page 1', 'simple_page_1.html')";
+    EXPECT_TRUE(content::ExecuteScript(root->current_frame_host(), script));
+    capturer.Wait();
+    EXPECT_EQ(ui::PAGE_TRANSITION_LINK | ui::PAGE_TRANSITION_CLIENT_REDIRECT,
+              capturer.params().transition);
+    EXPECT_EQ(NAVIGATION_TYPE_NEW_PAGE, capturer.details().type);
+  }
+}
+
+// Verify that navigations for NAVIGATION_TYPE_EXISTING_PAGE are correctly
+// classified.
+IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest,
+                       NavigationTypeClassification_ExistingPage) {
+  GURL url1(embedded_test_server()->GetURL(
+      "/navigation_controller/simple_page_1.html"));
+  NavigateToURL(shell(), url1);
+  GURL url2(embedded_test_server()->GetURL(
+      "/navigation_controller/simple_page_2.html"));
+  NavigateToURL(shell(), url2);
+
+  FrameTreeNode* root =
+      static_cast<WebContentsImpl*>(shell()->web_contents())->
+          GetFrameTree()->root();
+
+  {
+    // Back.
+    FrameNavigateParamsCapturer capturer(root);
+    shell()->web_contents()->GetController().GoBack();
+    capturer.Wait();
+    EXPECT_EQ(ui::PAGE_TRANSITION_TYPED
+              | ui::PAGE_TRANSITION_FORWARD_BACK
+              | ui::PAGE_TRANSITION_FROM_ADDRESS_BAR,
+              capturer.params().transition);
+    EXPECT_EQ(NAVIGATION_TYPE_EXISTING_PAGE, capturer.details().type);
+  }
+
+  {
+    // Forward.
+    FrameNavigateParamsCapturer capturer(root);
+    shell()->web_contents()->GetController().GoForward();
+    capturer.Wait();
+    EXPECT_EQ(ui::PAGE_TRANSITION_TYPED
+              | ui::PAGE_TRANSITION_FORWARD_BACK
+              | ui::PAGE_TRANSITION_FROM_ADDRESS_BAR,
+              capturer.params().transition);
+    EXPECT_EQ(NAVIGATION_TYPE_EXISTING_PAGE, capturer.details().type);
+  }
+
+  {
+    // Reload.

[Charlie Reis, 2015/03/09 20:39:58]

Can we also test a browser-initiated reload?  I would expect that to not have a
client redirect transition type.

[Avi (use Gerrit), 2015/03/09 21:35:56]

Done.

Also, haha it's PAGE_TRANSITION_RELOAD, while renderer-side is
PAGE_TRANSITION_LINK | PAGE_TRANSITION_CLIENT_REDIRECT. My position is that the
page transition assignments are only vaguely intentional.

+    FrameNavigateParamsCapturer capturer(root);
+    EXPECT_TRUE(content::ExecuteScript(root->current_frame_host(),
+                                       "location.reload()"));
+    capturer.Wait();
+    EXPECT_EQ(ui::PAGE_TRANSITION_LINK | ui::PAGE_TRANSITION_CLIENT_REDIRECT,

[Charlie Reis, 2015/03/09 20:39:58]

Wow, this is a client redirect too?   I guess that's consistent with it being a
non-user-initiated navigation.

[Avi (use Gerrit), 2015/03/09 21:35:56]

Again, it's not clear how intentional this is.

+              capturer.params().transition);
+    EXPECT_EQ(NAVIGATION_TYPE_EXISTING_PAGE, capturer.details().type);
+  }
+
+  {
+    // location.replace().
+    FrameNavigateParamsCapturer capturer(root);
+    GURL frame_url(embedded_test_server()->GetURL(
+        "/navigation_controller/simple_page_1.html"));
+    std::string script = "location.replace('" + frame_url.spec() + "')";
+    EXPECT_TRUE(content::ExecuteScript(root->current_frame_host(), script));
+    capturer.Wait();
+    EXPECT_EQ(ui::PAGE_TRANSITION_LINK | ui::PAGE_TRANSITION_CLIENT_REDIRECT,
+              capturer.params().transition);
+    EXPECT_EQ(NAVIGATION_TYPE_EXISTING_PAGE, capturer.details().type);
+  }
+}
+
+// Verify that navigations for NAVIGATION_TYPE_SAME_PAGE are correctly
+// classified.
+IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest,
+                       NavigationTypeClassification_SamePage) {
+  GURL url1(embedded_test_server()->GetURL(
+      "/navigation_controller/simple_page_1.html"));
+  NavigateToURL(shell(), url1);
+
+  FrameTreeNode* root =
+      static_cast<WebContentsImpl*>(shell()->web_contents())->
+          GetFrameTree()->root();
+
+  {
+    // Simple load.
+    FrameNavigateParamsCapturer capturer(root);
+    GURL frame_url(embedded_test_server()->GetURL(
+        "/navigation_controller/simple_page_1.html"));
+    NavigateFrameToURL(root, frame_url);
+    capturer.Wait();
+    EXPECT_EQ(ui::PAGE_TRANSITION_LINK, capturer.params().transition);
+    EXPECT_EQ(NAVIGATION_TYPE_SAME_PAGE, capturer.details().type);
+  }
+}
+
+// Verify that navigations for NAVIGATION_TYPE_IN_PAGE are correctly
+// classified.
+IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest,
+                       NavigationTypeClassification_InPage) {
+  GURL url1(embedded_test_server()->GetURL(
+      "/navigation_controller/simple_page_1.html"));
+  NavigateToURL(shell(), url1);
+
+  FrameTreeNode* root =
+      static_cast<WebContentsImpl*>(shell()->web_contents())->
+          GetFrameTree()->root();
+
+  {
+    // history.replaceState().
+    FrameNavigateParamsCapturer capturer(root);
+    std::string script =
+        "history.replaceState({}, 'page 1', 'simple_page_2.html')";
+    EXPECT_TRUE(content::ExecuteScript(root->current_frame_host(), script));
+    capturer.Wait();
+    EXPECT_EQ(ui::PAGE_TRANSITION_LINK, capturer.params().transition);
+    // TODO(avi,creis): Huh? Why is this not NAVIGATION_TYPE_EXISTING_PAGE like

[Charlie Reis, 2015/03/09 20:39:58]

Ok, let's think this through.

history.replaceState doesn't change the current document.  It just updates the
current NavigationEntry's URL, etc.  In that sense, it's an in-page navigation
because we don't leave the page.  It's also kind of a history navigation, since
it updates an existing NavigationEntry rather than creating a new one.

location.replace does change the current document, but it replaces the current
NavigationEntry.  In that sense, it's not an in-page navigation.

Maybe the current behavior makes sense?

[Avi (use Gerrit), 2015/03/09 21:35:56]

Acknowledged.

+    // location.replace()?
+    EXPECT_EQ(NAVIGATION_TYPE_IN_PAGE, capturer.details().type);
+  }
+
+  // OK. Now that that weird case is out of the way, let's have the real tests.
+
+  // Back and forward across a fragment navigation.
+
+  GURL url2(embedded_test_server()->GetURL(
+      "/navigation_controller/page_with_links.html"));
+  NavigateToURL(shell(), url2);
+  std::string script = "document.getElementById('fraglink').click()";
+  EXPECT_TRUE(content::ExecuteScript(root->current_frame_host(), script));
+  EXPECT_TRUE(WaitForLoadStop(shell()->web_contents()));
+
+  {
+    // Back.
+    FrameNavigateParamsCapturer capturer(root);
+    shell()->web_contents()->GetController().GoBack();
+    capturer.Wait();
+    EXPECT_EQ(ui::PAGE_TRANSITION_TYPED
+              | ui::PAGE_TRANSITION_FORWARD_BACK
+              | ui::PAGE_TRANSITION_FROM_ADDRESS_BAR,
+              capturer.params().transition);
+    EXPECT_EQ(NAVIGATION_TYPE_IN_PAGE, capturer.details().type);
+  }
+
+  {
+    // Forward.
+    FrameNavigateParamsCapturer capturer(root);
+    shell()->web_contents()->GetController().GoForward();
+    capturer.Wait();
+    EXPECT_EQ(ui::PAGE_TRANSITION_LINK | ui::PAGE_TRANSITION_FORWARD_BACK,
+              capturer.params().transition);
+    EXPECT_EQ(NAVIGATION_TYPE_IN_PAGE, capturer.details().type);
+  }
+
+  // Back and forward across a pushState-created navigation.
+
+  NavigateToURL(shell(), url1);
+  script = "history.pushState({}, 'page 2', 'simple_page_2.html')";
+  EXPECT_TRUE(content::ExecuteScript(root->current_frame_host(), script));
+  EXPECT_TRUE(WaitForLoadStop(shell()->web_contents()));
+
+  {
+    // Back.
+    FrameNavigateParamsCapturer capturer(root);
+    shell()->web_contents()->GetController().GoBack();
+    capturer.Wait();
+    EXPECT_EQ(ui::PAGE_TRANSITION_TYPED
+              | ui::PAGE_TRANSITION_FORWARD_BACK
+              | ui::PAGE_TRANSITION_FROM_ADDRESS_BAR,
+              capturer.params().transition);
+    EXPECT_EQ(NAVIGATION_TYPE_IN_PAGE, capturer.details().type);
+  }
+
+  {
+    // Forward.
+    FrameNavigateParamsCapturer capturer(root);
+    shell()->web_contents()->GetController().GoForward();
+    capturer.Wait();
+    EXPECT_EQ(ui::PAGE_TRANSITION_LINK | ui::PAGE_TRANSITION_FORWARD_BACK,
+              capturer.params().transition);
+    EXPECT_EQ(NAVIGATION_TYPE_IN_PAGE, capturer.details().type);
+  }
+}
+
+// Verify that navigations for NAVIGATION_TYPE_NEW_SUBFRAME and
+// NAVIGATION_TYPE_AUTO_SUBFRAME are properly classified.
 IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest,
                        NavigationTypeClassification_NewAndAutoSubframe) {
   GURL main_url(embedded_test_server()->GetURL(
@@ -296,7 +555,7 @@ IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest,
   ASSERT_NE(nullptr, root->child_at(0));
 
   {
-    // Navigate the iframe to a new URL; expect a manual subframe transition.
+    // Simple load.
     FrameNavigateParamsCapturer capturer(root->child_at(0));
     GURL frame_url(embedded_test_server()->GetURL(
         "/navigation_controller/simple_page_1.html"));
@@ -308,7 +567,7 @@ IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest,
   }
 
   {
-    // Do a history navigation; expect an auto subframe transition.
+    // Back.
     FrameNavigateParamsCapturer capturer(root->child_at(0));
     shell()->web_contents()->GetController().GoBack();
     capturer.Wait();
@@ -317,7 +576,7 @@ IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest,
   }
 
   {
-    // Do a history navigation; expect an auto subframe transition.
+    // Forward.
     FrameNavigateParamsCapturer capturer(root->child_at(0));
     shell()->web_contents()->GetController().GoForward();
     capturer.Wait();
@@ -326,10 +585,10 @@ IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest,
   }
 
   {
-    // Navigate the iframe to a new URL; expect a manual subframe transition.
+    // Simple load.
     FrameNavigateParamsCapturer capturer(root->child_at(0));
     GURL frame_url(embedded_test_server()->GetURL(
-        "/navigation_controller/simple_page_2.html"));
+        "/navigation_controller/page_with_links.html"));
     NavigateFrameToURL(root->child_at(0), frame_url);
     capturer.Wait();
     EXPECT_EQ(ui::PAGE_TRANSITION_MANUAL_SUBFRAME,
@@ -338,7 +597,19 @@ IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest,
   }
 
   {
-    // Use location.assign(); expect a manual subframe transition.
+    // Load via a fragment link click.
+    FrameNavigateParamsCapturer capturer(root->child_at(0));
+    std::string script = "document.getElementById('fraglink').click()";
+    EXPECT_TRUE(content::ExecuteScript(root->child_at(0)->current_frame_host(),
+                                       script));
+    capturer.Wait();
+    EXPECT_EQ(ui::PAGE_TRANSITION_MANUAL_SUBFRAME,
+              capturer.params().transition);
+    EXPECT_EQ(NAVIGATION_TYPE_NEW_SUBFRAME, capturer.details().type);
+  }
+
+  {
+    // location.assign().
     FrameNavigateParamsCapturer capturer(root->child_at(0));
     GURL frame_url(embedded_test_server()->GetURL(
         "/navigation_controller/simple_page_1.html"));
@@ -352,8 +623,7 @@ IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest,
   }
 
   {
-    // Use location.replace(); expect an auto subframe transition. (Replacements
-    // aren't "navigation" so we only see the frame load committing.)
+    // location.replace().
     LoadCommittedCapturer capturer(root->child_at(0));
     GURL frame_url(embedded_test_server()->GetURL(
         "/navigation_controller/simple_page_2.html"));
@@ -365,7 +635,7 @@ IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest,
   }
 
   {
-    // Use history.pushState(); expect a manual subframe transition.
+    // history.pushState().
     FrameNavigateParamsCapturer capturer(root->child_at(0));
     std::string script =
         "history.pushState({}, 'page 1', 'simple_page_1.html')";
@@ -378,9 +648,7 @@ IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest,
   }
 
   {
-    // Use history.replaceState(); expect an auto subframe transition.
-    // (Replacements aren't "navigation" so we only see the frame load
-    // committing.)
+    // history.replaceState().
     LoadCommittedCapturer capturer(root->child_at(0));
     std::string script =
         "history.replaceState({}, 'page 2', 'simple_page_2.html')";
@@ -391,8 +659,7 @@ IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest,
   }
 
   {
-    // Reload the subframe; expect an auto subframe transition. (Reloads aren't
-    // "navigation" so we only see the frame load committing.)
+    // Reload.
     LoadCommittedCapturer capturer(root->child_at(0));
     EXPECT_TRUE(content::ExecuteScript(root->child_at(0)->current_frame_host(),
                                        "location.reload()"));
@@ -401,8 +668,7 @@ IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest,
   }
 
   {
-    // Create an iframe; expect an auto subframe transition. (Initial frame
-    // creation isn't "navigation" so we only see the frame load committing.)
+    // Create an iframe.
     LoadCommittedCapturer capturer(shell()->web_contents());
     GURL frame_url(embedded_test_server()->GetURL(
         "/navigation_controller/simple_page_1.html"));