Refactor the loading tracking logic in WebContentsImpl.

content/browser/web_contents/web_contents_impl_unittest.cc

Index: content/browser/web_contents/web_contents_impl_unittest.cc
diff --git a/content/browser/web_contents/web_contents_impl_unittest.cc b/content/browser/web_contents/web_contents_impl_unittest.cc
index da3c82e7d4826fe968344dadc907178eca8bb1d1..1a94f1c5a27ac9d353b91beb63f759ff5e440b30 100644
--- a/content/browser/web_contents/web_contents_impl_unittest.cc
+++ b/content/browser/web_contents/web_contents_impl_unittest.cc
@@ -2879,6 +2879,61 @@ TEST_F(WebContentsImplTest, StartStopEventsBalance) {
   EXPECT_FALSE(observer.is_loading());
 }
 
+// Ensure that WebContentsImpl does not stop loading too early when there still
+// is a pending renderer. The sequence to reproduce the issue is as follows:
+// * StartLoading on a frame.
+// * Start another navigation before the previous frame has finished loading.
+// * StopLoading on the previous navigation.
+// At this point, the WebContentsImpl should still be loading as there is still
+// one pending navigation.
+TEST_F(WebContentsImplTest, NoEarlyStop) {
+  const GURL orig_url("http://www.chromium.org");
+  const GURL new_url("http://www.google.com");
+
+  TestRenderFrameHost* orig_rfh = contents()->GetMainFrame();
+
+  // Navigate the RenderFrame, simulate the DidStartLoading, and commit.

[nasko, 2015/02/26 23:30:34]

nit: RenderFrameHost

[Fabrice (no longer in Chrome), 2015/02/27 17:45:25]

Acknowledged.

+  // The frame should still be loading.
+  controller().LoadURL(
+      orig_url, Referrer(), ui::PAGE_TRANSITION_TYPED, std::string());
+  orig_rfh->OnMessageReceived(
+      FrameHostMsg_DidStartLoading(orig_rfh->GetRoutingID(), false));
+  contents()->TestDidNavigate(orig_rfh, 1, orig_url, ui::PAGE_TRANSITION_TYPED);
+  EXPECT_FALSE(contents()->cross_navigation_pending());
+  EXPECT_EQ(orig_rfh, contents()->GetMainFrame());
+  EXPECT_TRUE(contents()->IsLoading());
+
+  // Navigate to new site, simulate the DidStartLoading, and commit.
+  // The frame should still be loading.
+  controller().LoadURL(
+      new_url, Referrer(), ui::PAGE_TRANSITION_TYPED, std::string());
+  if (base::CommandLine::ForCurrentProcess()->HasSwitch(
+      switches::kEnableBrowserSideNavigation)) {
+    orig_rfh->PrepareForCommit(new_url);

[nasko, 2015/02/26 23:30:33]

Should this be in an if statement? I thought PrepareForCommit does the right
thing in all cases.

[Fabrice (no longer in Chrome), 2015/02/27 17:45:25]

You are correct, fixed.

+  }
+  EXPECT_TRUE(contents()->cross_navigation_pending());
+
+  TestRenderFrameHost* new_rfh = contents()->GetPendingMainFrame();
+  ASSERT_NE(new_rfh, nullptr);
+  EXPECT_NE(orig_rfh, new_rfh);
+  new_rfh->OnMessageReceived(
+      FrameHostMsg_DidStartLoading(new_rfh->GetRoutingID(), false));
+  contents()->TestDidNavigate(new_rfh, 1, new_url, ui::PAGE_TRANSITION_TYPED);
+  EXPECT_TRUE(contents()->IsLoading());
+
+  // Simulate the DidStopLoading on the original navigation. The frame should
+  // still be loading.
+  orig_rfh->OnMessageReceived(
+      FrameHostMsg_DidStopLoading(orig_rfh->GetRoutingID()));
+  EXPECT_TRUE(contents()->IsLoading());
+
+  // Simulate the DidStopLoading on the new navigation. The frame should now
+  // have stopped loading.
+  new_rfh->OnMessageReceived(
+      FrameHostMsg_DidStopLoading(new_rfh->GetRoutingID()));
+  EXPECT_FALSE(contents()->IsLoading());
+}
+
 TEST_F(WebContentsImplTest, MediaPowerSaveBlocking) {
   // PlayerIDs are actually pointers cast to int64, so verify that both negative
   // and positive player ids don't blow up.