Fix NavigationController::ClassifyNavigation() to treat redirection correctly.

When we enter an address that redirects to some other page to the address box,
it should show the address where we are redirected to after navigation
completes. Currently, it doesn't when we follow this procedure more than once
as described in BUG 5374.

After redirection, the renderer says that it navigated to the same page by
returning existing page_id in ViewHostMsg_FrameNavigate_Params, but
NavigationController::ClassifyNavigation() compares params.url with
pending_entry_->url() and says it's not SAME_PAGE. Therefore, the result is
not correctly treated and goes into EXISTING_PAGE handler. It compares
the existing entry for the page with pending_entry_. It's false, so it doesn't
call DiscardNonCommittedEntriesInternal(). pending_entry_ left not released.
Toolbar's view uses this pending_entry_'s url for showing address on the
address bar.

We should not check URL. Just overwrite entry_ with the response from renderer.

TEST=
Access http://www.google.com , https://www.google.com , and then
https://www.google.com . The address bar should show http://www.google.com .
Prepare some site http://example.com which redirects to http://example.org ,
and then access http://example.com , and then change the redirection to
http://example.net . Retry accessing http://example.com . The address bar
should show http://example.net .

BUG=5374
Original review: http://codereview.chromium.org/115916
Patch by tyoshino@google.com


git-svn-id: svn://svn.chromium.org/chrome/trunk/src@17511 0039d316-1c4b-4281-b951-d872f2087c98

2 files changed, 58 insertions, 4 deletions

diff --git a/chrome/browser/tab_contents/navigation_controller.cc b/chrome/browser/tab_contents/navigation_controller.cc
index 4a19e58..e33898d 100644
--- a/chrome/browser/tab_contents/navigation_controller.cc
+++ b/chrome/browser/tab_contents/navigation_controller.cc
@@ -552,10 +552,8 @@ NavigationType::Type NavigationController::ClassifyNavigation(
   NavigationEntry* existing_entry = entries_[existing_entry_index].get();
 
   if (pending_entry_ &&
-      pending_entry_->url() == params.url &&
       existing_entry != pending_entry_ &&
-      pending_entry_->page_id() == -1 &&
-      pending_entry_->url() == existing_entry->url()) {
+      pending_entry_->page_id() == -1) {
     // In this case, we have a pending entry for a URL but WebCore didn't do a
     // new navigation. This happens when you press enter in the URL bar to
     // reload. We will create a pending entry, but WebKit will convert it to
@@ -668,6 +666,9 @@ void NavigationController::RendererDidNavigateToSamePage(
   // a regular user-initiated navigation.
   existing_entry->set_unique_id(pending_entry_->unique_id());
 
+  // The URL may have changed due to redirects.
+  existing_entry->set_url(params.url);
+
   DiscardNonCommittedEntries();
 }
 
@@ -961,7 +962,7 @@ void NavigationController::DiscardNonCommittedEntriesInternal() {
 void NavigationController::DiscardTransientEntry() {
   if (transient_entry_index_ == -1)
     return;
-  entries_.erase(entries_.begin() + transient_entry_index_ );
+  entries_.erase(entries_.begin() + transient_entry_index_);
   transient_entry_index_ = -1;
 }
 
diff --git a/chrome/browser/tab_contents/navigation_controller_unittest.cc b/chrome/browser/tab_contents/navigation_controller_unittest.cc
index 38c539a..fb0761f 100644
--- a/chrome/browser/tab_contents/navigation_controller_unittest.cc
+++ b/chrome/browser/tab_contents/navigation_controller_unittest.cc
@@ -714,6 +714,59 @@ TEST_F(NavigationControllerTest, Forward_GeneratesNewPage) {
   EXPECT_FALSE(controller().CanGoForward());
 }
 
+// Two consequent navigation for the same URL entered in should be considered
+// as SAME_PAGE navigation even when we are redirected to some other page.
+TEST_F(NavigationControllerTest, Redirect) {
+  TestNotificationTracker notifications;
+  RegisterForAllNavNotifications(&notifications, &controller());
+
+  const GURL url1("http://foo1");
+  const GURL url2("http://foo2");  // Redirection target
+
+  // First request
+  controller().LoadURL(url1, GURL(), PageTransition::TYPED);
+
+  EXPECT_EQ(0U, notifications.size());
+  rvh()->SendNavigate(0, url2);
+  EXPECT_TRUE(notifications.Check1AndReset(
+      NotificationType::NAV_ENTRY_COMMITTED));
+
+  // Second request
+  controller().LoadURL(url1, GURL(), PageTransition::TYPED);
+
+  EXPECT_TRUE(controller().pending_entry());
+  EXPECT_EQ(controller().pending_entry_index(), -1);
+  EXPECT_EQ(url1, controller().GetActiveEntry()->url());
+
+  ViewHostMsg_FrameNavigate_Params params;
+  params.page_id = 0;
+  params.url = url2;
+  params.transition = PageTransition::SERVER_REDIRECT;
+  params.redirects.push_back(GURL("http://foo1"));
+  params.redirects.push_back(GURL("http://foo2"));
+  params.should_update_history = false;
+  params.gesture = NavigationGestureAuto;
+  params.is_post = false;
+
+  NavigationController::LoadCommittedDetails details;
+
+  EXPECT_EQ(0U, notifications.size());
+  EXPECT_TRUE(controller().RendererDidNavigate(params, &details));
+  EXPECT_TRUE(notifications.Check1AndReset(
+      NotificationType::NAV_ENTRY_COMMITTED));
+
+  EXPECT_TRUE(details.type == NavigationType::SAME_PAGE);
+  EXPECT_EQ(controller().entry_count(), 1);
+  EXPECT_EQ(controller().last_committed_entry_index(), 0);
+  EXPECT_TRUE(controller().GetLastCommittedEntry());
+  EXPECT_EQ(controller().pending_entry_index(), -1);
+  EXPECT_FALSE(controller().pending_entry());
+  EXPECT_EQ(url2, controller().GetActiveEntry()->url());
+
+  EXPECT_FALSE(controller().CanGoBack());
+  EXPECT_FALSE(controller().CanGoForward());
+}
+
 // Tests navigation via link click within a subframe. A new navigation entry
 // should be created.
 TEST_F(NavigationControllerTest, NewSubframe) {