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 4b98ab1516f2c28dd46662533ba0461b03ac2d82..b96154d2ec5af45bbe7336a9d9b71ef1c8226c0a 100644
--- a/content/browser/frame_host/navigation_controller_impl_browsertest.cc
+++ b/content/browser/frame_host/navigation_controller_impl_browsertest.cc
@@ -50,6 +50,7 @@
 #include "content/test/content_browser_test_utils_internal.h"
 #include "net/dns/mock_host_resolver.h"
 #include "net/test/embedded_test_server/embedded_test_server.h"
+#include "net/test/embedded_test_server/embedded_test_server_connection_listener.h"
 #include "net/test/embedded_test_server/http_request.h"
 #include "net/test/url_request/url_request_failed_job.h"
 #include "testing/gmock/include/gmock/gmock-matchers.h"
@@ -71,13 +72,54 @@ static std::string kRemoveFrameScript =
 
 namespace content {
 
+// Gets notified by the EmbeddedTestServer on incoming requests.
+class ConnectionListener

[clamy, 2016/12/22 17:30:25]

Did you find this class in use somewhere in Chrome? We *heavily* discourage use
of locks inside chromium: in particular, this may alter interleaving of thread
tasks and lead to different results in the test compared to the actual code.

Instead, if the goal is to check that no network request was made, you could
check that we got a WebContentsObserver::ReadyToCommitNavigation without calling
NavigationThrottle::WillStartRequest &
NavigationThrottle::WillProcessNavigation. This would also be a good place to
check that we properly report the navigation as same page in the
NavigationHandle.

[arthursonzogni, 2017/01/05 17:04:22]

Yes I found it there:
https://cs.chromium.org/chromium/src/chrome/browser/net/predictor_browsertest.cc
They use a lock too.
It is a listener on the embedded_test_server, I like it because it doesn't make
any assumptions on how chromes works.

I will try your suggestion.

+    : public net::test_server::EmbeddedTestServerConnectionListener {
+ public:
+
+  ConnectionListener()
+    : bytes_read(0) {}
+  ~ConnectionListener() override {}
+
+  // Get called from the EmbeddedTestServer thread to be notified that
+  // a connection was accepted.
+  void AcceptedSocket(const net::StreamSocket& connection) override {}
+
+  // Get called from the EmbeddedTestServer thread to be notified that
+  // a connection was read from.
+  void ReadFromSocket(const net::StreamSocket& connection, int bytes) override {
+    base::AutoLock lock(lock_);
+    bytes_read += bytes;
+  }
+
+  int BytesRead() {
+    base::AutoLock lock(lock_);
+    return bytes_read;
+  }
+
+  void ResetBytesRead() {
+    base::AutoLock lock(lock_);
+    bytes_read = 0;
+  }
+
+ private:
+  // This lock protects all the members below, which each are used on both the
+  // IO and UI thread. Members declared after the lock are protected by it.
+  mutable base::Lock lock_;
+  int bytes_read;
+};
+
 class NavigationControllerBrowserTest : public ContentBrowserTest {
  protected:
   void SetUpOnMainThread() override {
     host_resolver()->AddRule("*", "127.0.0.1");
     content::SetupCrossSiteRedirector(embedded_test_server());
+    connection_listener.reset(new ConnectionListener());
+    embedded_test_server()->SetConnectionListener(connection_listener.get());
     ASSERT_TRUE(embedded_test_server()->Start());
   }
+
+  std::unique_ptr<ConnectionListener> connection_listener;
 };
 
 // Ensure that tests can navigate subframes cross-site in both default mode and
@@ -7036,4 +7078,39 @@ IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest,
   EXPECT_FALSE(handle_observer.was_renderer_initiated());
 }
 
+// Ensure that browser-initiated same-document navigations are detected and

[clamy, 2016/12/22 17:30:25]

Could you also add a test for the fallback/drop case?

[arthursonzogni, 2017/01/05 17:04:22]

I don't have any idea how to test it.

[clamy, 2017/01/09 16:58:48]

I'd suggest adding a RenderViewImplTest. See
RenderViewImplTest.OnNavigationHTTPPost for an idea on how to check whether the
renderer emitted an IPC or not. In that case, we want to check that we don't get
a DidCommitProvisionalLoad but either a BeginNavigation or nothing depending on
the option we chose.

+// don't issue network requests.
+// see crbug.com/663777

[clamy, 2016/12/22 17:30:25]

nit: comments start with a capital letter and end with a dot :).

[arthursonzogni, 2017/01/05 17:04:22]

Done.

+IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest,
+                       SamePageBrowserInitiatedNoReload) {
+  GURL url(embedded_test_server()->GetURL("/title1.html"));
+  GURL url_fragment_1(embedded_test_server()->GetURL("/title1.html#id_1"));
+  GURL url_fragment_2(embedded_test_server()->GetURL("/title1.html#id_2"));
+
+  // 1) Perform a new-document navigation.
+  connection_listener->ResetBytesRead();
+  EXPECT_TRUE(NavigateToURL(shell(), url));
+  EXPECT_TRUE(connection_listener->BytesRead() > 0);
+
+  // 2) Perform a same-document navigation by adding a fragment.
+  connection_listener->ResetBytesRead();
+  EXPECT_TRUE(NavigateToURL(shell(), url_fragment_1));
+  EXPECT_TRUE(connection_listener->BytesRead() == 0);
+
+  // 3) Perform a same-document navigation by modifying the fragment.
+  connection_listener->ResetBytesRead();
+  EXPECT_TRUE(NavigateToURL(shell(), url_fragment_2));
+  EXPECT_TRUE(connection_listener->BytesRead() == 0);
+
+  // 4) Redo the last navigation, but this time it should trigger a reload.
+  connection_listener->ResetBytesRead();
+  EXPECT_TRUE(NavigateToURL(shell(), url_fragment_2));
+  EXPECT_TRUE(connection_listener->BytesRead() > 0);
+
+  // 5) Perform a new-document navigation by removing the fragment.
+  connection_listener->ResetBytesRead();
+  EXPECT_TRUE(NavigateToURL(shell(), url));
+  EXPECT_TRUE(connection_listener->BytesRead() > 0);
+}
+
 }  // namespace content