Merge 48153 - Swap the check for "same page" navigations to happen after the check for "auto

subframes". Previously, we would check for SAME_PAGE first, which would catch
auto subframe navigations when there was a pending new navigation, which would
confuse the current navigation state.

TEST=covered by unit test
BUG=43967
Review URL: http://codereview.chromium.org/2180001

TBR=brettw@chromium.org
Review URL: http://codereview.chromium.org/2770015

git-svn-id: svn://svn.chromium.org/chrome/branches/375/src@49560 0039d316-1c4b-4281-b951-d872f2087c98

2 files changed, 43 insertions, 8 deletions

diff --git a/chrome/browser/tab_contents/navigation_controller.cc b/chrome/browser/tab_contents/navigation_controller.cc
index 141712f..d413a8b 100644
--- a/chrome/browser/tab_contents/navigation_controller.cc
+++ b/chrome/browser/tab_contents/navigation_controller.cc
@@ -648,6 +648,15 @@ NavigationType::Type NavigationController::ClassifyNavigation(
   }
   NavigationEntry* existing_entry = entries_[existing_entry_index].get();
 
+  if (!PageTransition::IsMainFrame(params.transition)) {
+    // All manual subframes would get new IDs and were handled above, so we
+    // know this is auto. Since the current page was found in the navigation
+    // entry list, we're guaranteed to have a last committed entry.
+    DCHECK(GetLastCommittedEntry());
+    return NavigationType::AUTO_SUBFRAME;
+  }
+
+  // Anything below here we know is a main frame navigation.
   if (pending_entry_ &&
       existing_entry != pending_entry_ &&
       pending_entry_->page_id() == -1) {
@@ -661,14 +670,6 @@ NavigationType::Type NavigationController::ClassifyNavigation(
     return NavigationType::SAME_PAGE;
   }
 
-  if (!PageTransition::IsMainFrame(params.transition)) {
-    // All manual subframes would get new IDs and were handled above, so we
-    // know this is auto. Since the current page was found in the navigation
-    // entry list, we're guaranteed to have a last committed entry.
-    DCHECK(GetLastCommittedEntry());
-    return NavigationType::AUTO_SUBFRAME;
-  }
-
   // Any toplevel navigations with the same base (minus the reference fragment)
   // are in-page navigations. We weeded out subframe navigations above. Most of
   // the time this doesn't matter since WebKit doesn't tell us about subframe
diff --git a/chrome/browser/tab_contents/navigation_controller_unittest.cc b/chrome/browser/tab_contents/navigation_controller_unittest.cc
index f5d3ae1..261b2e2 100644
--- a/chrome/browser/tab_contents/navigation_controller_unittest.cc
+++ b/chrome/browser/tab_contents/navigation_controller_unittest.cc
@@ -1595,6 +1595,40 @@ TEST_F(NavigationControllerTest, CloneOmitsInterstitials) {
   ASSERT_EQ(2, clone->controller().entry_count());
 }
 
+// Tests a subframe navigation while a toplevel navigation is pending.
+// http://crbug.com/43967
+TEST_F(NavigationControllerTest, SubframeWhilePending) {
+  // Load the first page.
+  const GURL url1("http://foo/");
+  NavigateAndCommit(url1);
+
+  // Now start a pending load to a totally different page, but don't commit it.
+  const GURL url2("http://bar/");
+  controller().LoadURL(url2, GURL(), PageTransition::TYPED);
+
+  // Send a subframe update from the first page, as if one had just
+  // automatically loaded. Auto subframes don't increment the page ID.
+  const GURL url1_sub("http://foo/subframe");
+  ViewHostMsg_FrameNavigate_Params params = {0};
+  params.page_id = controller().GetLastCommittedEntry()->page_id();
+  params.url = url1_sub;
+  params.transition = PageTransition::AUTO_SUBFRAME;
+  params.should_update_history = false;
+  params.gesture = NavigationGestureAuto;
+  params.is_post = false;
+  NavigationController::LoadCommittedDetails details;
+
+  // This should return false meaning that nothing was actually updated.
+  EXPECT_FALSE(controller().RendererDidNavigate(params, 0, &details));
+
+  // The notification should have updated the last committed one, and not
+  // the pending load.
+  EXPECT_EQ(url1, controller().GetLastCommittedEntry()->url());
+
+  // The active entry should be unchanged by the subframe load.
+  EXPECT_EQ(url2, controller().GetActiveEntry()->url());
+}
+
 /* TODO(brettw) These test pass on my local machine but fail on the XP buildbot
    (but not Vista) cleaning up the directory after they run.
    This should be fixed.