PlzNavigate: identify same-page browser-initiated navigation.

content/browser/frame_host/navigation_controller_impl_browsertest.cc

 // Copyright 2014 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 "content/browser/frame_host/navigation_controller_impl.h"
 
 #include <stdint.h>
 #include <algorithm>
 #include <utility>
 #include <vector>
@@ skipping 32 matching lines @@
 #include "content/public/test/browser_test_utils.h"
 #include "content/public/test/content_browser_test.h"
 #include "content/public/test/content_browser_test_utils.h"
 #include "content/public/test/test_navigation_observer.h"
 #include "content/public/test/test_utils.h"
 #include "content/shell/browser/shell.h"
 #include "content/shell/common/shell_switches.h"
 #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"
 
 namespace {
 
 static std::string kAddNamedFrameScript =
       "var f = document.createElement('iframe');"
       "f.name = 'foo-frame-name';"
       "document.body.appendChild(f);";
 static std::string kAddFrameScript =
       "var f = document.createElement('iframe');"
       "document.body.appendChild(f);";
 static std::string kRemoveFrameScript =
       "var f = document.querySelector('iframe');"
       "f.parentNode.removeChild(f);";
 
 }  // namespace
 
 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
 // --site-per-process, but that they only go cross-process in the latter.
 IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest, LoadCrossSiteSubframe) {
   // Load a main frame with a subframe.
   GURL main_url(embedded_test_server()->GetURL(
       "/navigation_controller/page_with_iframe.html"));
   EXPECT_TRUE(NavigateToURL(shell(), main_url));
   FrameTreeNode* root = static_cast<WebContentsImpl*>(shell()->web_contents())
@@ skipping 6938 matching lines @@
   // Do a browser-initiated fragment navigation.
   NavigationHandleCommitObserver handle_observer(shell()->web_contents(),
                                                  kFragmentURL);
   EXPECT_TRUE(NavigateToURL(shell(), kFragmentURL));
 
   EXPECT_TRUE(handle_observer.has_committed());
   EXPECT_TRUE(handle_observer.was_same_page());
   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