This is the successor to http://codereview.chromium.org/67150

Make forward/backward navigation work even when redirection is involved.

Currently, Chrome tries to go back to the page immediately before
the current one. This doesn't work if the current page was visited
by redirection; redirection just occurs again.

With this change, Chrome first tries to find the redirection source
of the current page and then to go back to the page before the source.

BUG=9663,10531

Tested: unit_tests, ui_tests, manually.

Patch contributed by Yuzo Fujishima <yuzo@google.com>

Review: http://codereview.chromium.org/100245

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

10 files changed, 244 insertions, 115 deletions

diff --git a/chrome/browser/tab_contents/navigation_controller.cc b/chrome/browser/tab_contents/navigation_controller.cc
index 4b3e748..a097455 100644
--- a/chrome/browser/tab_contents/navigation_controller.cc
+++ b/chrome/browser/tab_contents/navigation_controller.cc
@@ -605,7 +605,11 @@ void NavigationController::RendererDidNavigateToNewPage(
   new_entry->set_site_instance(tab_contents_->GetSiteInstance());
   new_entry->set_has_post_data(params.is_post);
 
-  InsertEntry(new_entry);
+  // If the current entry is a redirection source, it needs to be replaced with
+  // the new entry to avoid unwanted redirections in navigating backward /
+  // forward. Otherwise, just insert the new entry.
+  InsertOrReplaceEntry(new_entry,
+      PageTransition::IsRedirect(new_entry->transition_type()));
 }
 
 void NavigationController::RendererDidNavigateToExistingPage(
@@ -674,7 +678,7 @@ void NavigationController::RendererDidNavigateInPage(
   NavigationEntry* new_entry = new NavigationEntry(*existing_entry);
   new_entry->set_page_id(params.page_id);
   new_entry->set_url(params.url);
-  InsertEntry(new_entry);
+  InsertOrReplaceEntry(new_entry, false);
 }
 
 void NavigationController::RendererDidNavigateNewSubframe(
@@ -687,7 +691,7 @@ void NavigationController::RendererDidNavigateNewSubframe(
                                   << "that a last committed entry exists.";
   NavigationEntry* new_entry = new NavigationEntry(*GetLastCommittedEntry());
   new_entry->set_page_id(params.page_id);
-  InsertEntry(new_entry);
+  InsertOrReplaceEntry(new_entry, false);
 }
 
 bool NavigationController::RendererDidNavigateAutoSubframe(
@@ -743,15 +747,15 @@ void NavigationController::CommitPendingEntry() {
     last_committed_entry_index_ = new_entry_index;
   } else {
     // This is a new navigation. It's easiest to just copy the entry and insert
-    // it new again, since InsertEntry expects to take ownership and also
-    // discard the pending entry. We also need to synthesize a page ID. We can
-    // only do this because this function will only be called by our custom
+    // it new again, since InsertOrReplaceEntry expects to take ownership and
+    // also discard the pending entry. We also need to synthesize a page ID. We
+    // can only do this because this function will only be called by our custom
     // TabContents types. For TabContents, the IDs are generated by the
     // renderer, so we can't do this.
     details.type = NavigationType::NEW_PAGE;
     pending_entry_->set_page_id(tab_contents_->GetMaxPageID() + 1);
     tab_contents_->UpdateMaxPageID(pending_entry_->page_id());
-    InsertEntry(new NavigationEntry(*pending_entry_));
+    InsertOrReplaceEntry(new NavigationEntry(*pending_entry_), false);
   }
 
   // Broadcast the notification of the navigation.
@@ -807,7 +811,8 @@ void NavigationController::DiscardNonCommittedEntries() {
   }
 }
 
-void NavigationController::InsertEntry(NavigationEntry* entry) {
+void NavigationController::InsertOrReplaceEntry(NavigationEntry* entry,
+                                                bool replace) {
   DCHECK(entry->transition_type() != PageTransition::AUTO_SUBFRAME);
 
   // Copy the pending entry's unique ID to the committed entry.
@@ -821,10 +826,13 @@ void NavigationController::InsertEntry(NavigationEntry* entry) {
 
   int current_size = static_cast<int>(entries_.size());
 
-  // Prune any entries which are in front of the current entry.
   if (current_size > 0) {
+    // Prune any entries which are in front of the current entry.
+    // Also prune the current entry if we are to replace the current entry.
+    int prune_up_to = replace ? last_committed_entry_index_ - 1
+                              : last_committed_entry_index_;
     int num_pruned = 0;
-    while (last_committed_entry_index_ < (current_size - 1)) {
+    while (prune_up_to < (current_size - 1)) {
       num_pruned++;
       entries_.pop_back();
       current_size--;
diff --git a/chrome/browser/tab_contents/navigation_controller.h b/chrome/browser/tab_contents/navigation_controller.h
index 015bdaa..83f0b8f 100644
--- a/chrome/browser/tab_contents/navigation_controller.h
+++ b/chrome/browser/tab_contents/navigation_controller.h
@@ -424,9 +424,9 @@ class NavigationController {
   // contents.
   void FinishRestore(int selected_index);
 
-  // Inserts an entry after the current position, removing all entries after it.
-  // The new entry will become the active one.
-  void InsertEntry(NavigationEntry* entry);
+  // Inserts a new entry or replaces the current entry with a new one, removing
+  // all entries after it. The new entry will become the active one.
+  void InsertOrReplaceEntry(NavigationEntry* entry, bool replace);
 
   // Discards the pending and transient entries.
   void DiscardNonCommittedEntriesInternal();
diff --git a/chrome/test/data/History/HistoryHelper.js b/chrome/test/data/History/HistoryHelper.js
index 93e0a94..efd2369 100644
--- a/chrome/test/data/History/HistoryHelper.js
+++ b/chrome/test/data/History/HistoryHelper.js
@@ -53,3 +53,6 @@ function createCookie(name,value,days) {
 function eraseCookie(name) {
   createCookie(name, "", -1);
 }
+
+var navigate_backward_cookie = "Navigate_Backward_Cookie";
+var navigate_forward_cookie = "Navigate_Forward_Cookie";
diff --git a/chrome/test/data/History/history_length_test1.html b/chrome/test/data/History/history_length_test1.html
index 5700661..e69de29 100644
--- a/chrome/test/data/History/history_length_test1.html
+++ b/chrome/test/data/History/history_length_test1.html
@@ -1,66 +0,0 @@
-<html>
-<head><title>History test1</title>
-<script src="HistoryHelper.js"></script>
-</head>
-
-<body onload="onLoad();">
-<div id="statusPanel" style="border: 1px solid red; width: 100%">
-History Test1 running....
-</body>
-
-<SCRIPT type="text/javascript">
-var first_run_cookie = "First_History_Test_Run";
-var second_run_cookie = "Second_History_Test_Run";
-
-function onLoad() {
-  if (readCookie(second_run_cookie) != null) {
-    setTimeout(OnValidateHistoryForSecondRun, 100);
-    return true;
-  }
-  
-  if (readCookie(first_run_cookie) != null) {
-    setTimeout(OnMoveForwardInHistory, 100);
-    return true;
-  }
-  
-  setTimeout(OnNavigateToPage2, 100);
-  return true;
-}
-
-function OnValidateHistoryForSecondRun() {
-  eraseCookie(first_run_cookie);
-  eraseCookie(second_run_cookie);
-  
-  if (window.history.length != 2) {
-    onFailure("History_Length_Test", 1, "Second run history length mismatch");
-    return false;
-  }
-  
-  onSuccess("History_Length_Test", 1);
-  return true;
-}
-
-function OnMoveForwardInHistory() {
-  if (window.history.length != 2) {
-    onFailure("History_Length_Test", 1, "History length mismatch in MoveForward navigation");
-    return false;
-  }
-
-  createCookie(second_run_cookie, "1", "1");
-  window.history.forward();
-  return true;
-}
-
-function OnNavigateToPage2() {
-  if (window.history.length != 2) {
-    onFailure("History_Length_Test", 1, "History length mismatch in initial navigation");
-    return false;
-  }
-  
-  createCookie(first_run_cookie, "1", "1");
-  window.location.href = "history_length_test2.html";
-  return true;
-}
-
-</SCRIPT>
-</html>
\ No newline at end of file
diff --git a/chrome/test/data/History/history_length_test2.html b/chrome/test/data/History/history_length_test2.html
index 39b5ebc..e69de29 100644
--- a/chrome/test/data/History/history_length_test2.html
+++ b/chrome/test/data/History/history_length_test2.html
@@ -1,32 +0,0 @@
-<html>
-<head><title>History test2</title>
-<script src="HistoryHelper.js"></script>
-</head>
-
-<body onload="onLoad();">
-<div id="statusPanel" style="border: 1px solid red; width: 100%">
-History test2....
-<div id="Div1" style="border: 1px solid red; width: 100%">
-</body>
-
-<SCRIPT type="text/javascript">
-function onLoad() {
-  setTimeout(OnValidateHistoryLength, 100);
-}
-
-function OnValidateHistoryLength() {
- if (window.history.length != 3) {
-   onFailure("History_Length_Test", 1, "History length mismatch in second page OnLoad");
-   return false;
- }
-
- window.history.back();
- return true; 
-}
-
-function OnEchoHistory() {
- alert(window.history.length);
-}
-
-</SCRIPT>
-</html>
\ No newline at end of file
diff --git a/chrome/test/data/History/history_length_test_page_1.html b/chrome/test/data/History/history_length_test_page_1.html
new file mode 100644
index 0000000..b0bfdaf
--- /dev/null
+++ b/chrome/test/data/History/history_length_test_page_1.html
@@ -0,0 +1,46 @@
+<html xmlns="http://www.w3.org/1999/xhtml">
+<head><title>History Test Page 1</title>
+<script type="text/javascript" src="HistoryHelper.js"></script>
+</head>
+
+<body onload="onLoad();">
+<div id="statusPanel" style="border: 1px solid red; width: 100%">
+History Test Page 1....
+</div>
+</body>
+
+<script type="text/javascript">
+
+function onLoad() {
+  if (readCookie(navigate_backward_cookie) != null) {
+    setTimeout(OnNavigateBackward, 100);
+    return true;
+  }
+  setTimeout(OnInitialLoad, 100);
+  return true;
+}
+
+function OnNavigateBackward() {
+  if (window.history.length != 2) {
+    onFailure("History_Length_Test_3", 1,
+              "History length mismatch on navigate backward at page 1");
+    return false;
+  }
+  // Navigate forward from this point on.
+  createCookie(navigate_forward_cookie, "1", "1");
+  window.history.forward();
+  return true;
+}
+
+function OnInitialLoad() {
+  if (window.history.length != 2) {
+    onFailure("History_Length_Test_1", 1,
+              "History length mismatch on initial load at page 1");
+    return false;
+  }
+  onSuccess("History_Length_Test_1", 1);
+  return true;
+}
+
+</script>
+</html>
\ No newline at end of file
diff --git a/chrome/test/data/History/history_length_test_page_2.html b/chrome/test/data/History/history_length_test_page_2.html
new file mode 100644
index 0000000..edcecfe
--- /dev/null
+++ b/chrome/test/data/History/history_length_test_page_2.html
@@ -0,0 +1,37 @@
+<html xmlns="http://www.w3.org/1999/xhtml">
+<head><title>History Test Page 2</title>
+<script type="text/javascript" src="HistoryHelper.js"></script>
+</head>
+
+<body onload="onLoad();">
+<div id="statusPanel" style="border: 1px solid red; width: 100%">
+History Test Page 2....
+</div>
+</body>
+
+<script type="text/javascript">
+
+function onLoad() {
+  if (readCookie(navigate_backward_cookie) != null ||
+      readCookie(navigate_forward_cookie) != null) {
+    onFailure("History_Length_Test_3", 1,
+              "Page 2 must not be visited in navigating backward/forward");
+    return false;
+  }
+  setTimeout(OnInitialLoad, 100);
+  return true;
+}
+
+function OnInitialLoad() {
+  if (window.history.length != 3) {
+    onFailure("History_Length_Test_2", 1,
+              "History length mismatch on initial load at page 2");
+    return false;
+  }
+  // Redirect to page 3.
+  window.location.href = "history_length_test_page_3.html";
+  return true; 
+}
+
+</script>
+</html>
\ No newline at end of file
diff --git a/chrome/test/data/History/history_length_test_page_3.html b/chrome/test/data/History/history_length_test_page_3.html
new file mode 100644
index 0000000..4c9a0a0
--- /dev/null
+++ b/chrome/test/data/History/history_length_test_page_3.html
@@ -0,0 +1,58 @@
+<html xmlns="http://www.w3.org/1999/xhtml">
+<head><title>History Test Page 3</title>
+<script src="HistoryHelper.js"></script>
+</head>
+
+<body onload="onLoad();">
+<div id="statusPanel" style="border: 1px solid red; width: 100%">
+History Test Page 3....
+</div>
+</body>
+
+<script type="text/javascript">
+
+function onLoad() {
+  if (readCookie(navigate_forward_cookie) != null) {
+    setTimeout(OnNavigateForward, 100);
+    return true;
+  }
+  if (readCookie(navigate_backward_cookie)) {
+    setTimeout(OnNavigateBackward, 100);
+    return true;
+  }
+  setTimeout(OnInitialLoad, 100);
+  return true;
+}
+
+function OnInitialLoad() {
+  if (window.history.length != 3) {
+    onFailure("History_Length_Test_2", 1,
+              "History length mismatch on initial load at page 3");
+    return false;
+  }
+  onSuccess("History_Length_Test_2", 1);
+  return true; 
+}
+
+function OnNavigateBackward() {
+  if (window.history.length != 3) {
+    onFailure("History_Length_Test_3", 1,
+              "History length mismatch on navigating backward at page 3");
+    return false;
+  }
+  window.history.back();
+  return true; 
+}
+
+function OnNavigateForward() {
+  if (window.history.length != 3) {
+    onFailure("History_Length_Test_3", 1,
+              "History length mismatch on navigating forward at page 3");
+    return false;
+  }
+  window.history.forward();
+  return true; 
+}
+
+</script>
+</html>
\ No newline at end of file
diff --git a/chrome/test/data/History/history_length_test_page_4.html b/chrome/test/data/History/history_length_test_page_4.html
new file mode 100644
index 0000000..a72889c
--- /dev/null
+++ b/chrome/test/data/History/history_length_test_page_4.html
@@ -0,0 +1,48 @@
+<html xmlns="http://www.w3.org/1999/xhtml">
+<head><title>History Test Page 4</title>
+<script src="HistoryHelper.js"></script>
+</head>
+
+<body onload="onLoad();">
+<div id="statusPanel" style="border: 1px solid red; width: 100%">
+History Test Page 4....
+</div>
+</body>
+
+<SCRIPT type="text/javascript">
+
+function onLoad() {
+  if (readCookie(navigate_forward_cookie) != null) {
+    setTimeout(OnNavigateForward, 100);
+    return true;
+  }
+  setTimeout(OnInitialLoad, 100);
+  return true;
+}
+
+function OnInitialLoad() {
+  if (window.history.length != 4) {
+    onFailure("History_Length_Test_3", 1,
+              "History length mismatch on initial load at page 4");
+    return false;
+  }
+  // Navigate backward from this point on.
+  createCookie(navigate_backward_cookie, "1", "1");
+  window.history.back();
+  return true; 
+}
+
+function OnNavigateForward() {
+  if (window.history.length != 4) {
+    onFailure("History_Length_Test_3", 1,
+              "History length mismatch on navigating forward at page 4");
+    return false;
+  }
+  eraseCookie(navigate_forward_cookie);
+  eraseCookie(navigate_backward_cookie);
+  onSuccess("History_Length_Test_3", 1);
+  return true;
+}
+
+</SCRIPT>
+</html>
\ No newline at end of file
diff --git a/chrome/test/ui/history_uitest.cc b/chrome/test/ui/history_uitest.cc
index 25ee75e..11be6ff 100644
--- a/chrome/test/ui/history_uitest.cc
+++ b/chrome/test/ui/history_uitest.cc
@@ -49,9 +49,36 @@ class HistoryTester : public UITest {
 };
 
 TEST_F(HistoryTester, VerifyHistoryLength) {
-  std::wstring test_case = L"history_length_test1.html";
-  GURL url = GetTestUrl(L"History", test_case);
-  NavigateToURL(url);
-  WaitForFinish("History_Length_Test", "1", url, kTestCompleteCookie,
+  // Test the history length for the following page transitions.
+  //
+  // Test case 1:
+  //   -open-> Page 1.
+  // Test case 2:
+  //   -open-> Page 2 -redirect-> Page 3.
+  // Test case 3:
+  //   -open-> Page 4 -navigate_backward-> Page 3 -navigate_backward->Page 1
+  //   -navigate_forward-> Page 3 -navigate_forward-> Page 4
+  //
+  // Note that Page 2 is not visited on navigating backward/forward.
+
+  // Test case 1
+  std::wstring test_case_1 = L"history_length_test_page_1.html";
+  GURL url_1 = GetTestUrl(L"History", test_case_1);
+  NavigateToURL(url_1);
+  WaitForFinish("History_Length_Test_1", "1", url_1, kTestCompleteCookie,
+                kTestCompleteSuccess, action_max_timeout_ms());
+
+  // Test case 2
+  std::wstring test_case_2 = L"history_length_test_page_2.html";
+  GURL url_2 = GetTestUrl(L"History", test_case_2);
+  NavigateToURL(url_2);
+  WaitForFinish("History_Length_Test_2", "1", url_2, kTestCompleteCookie,
+                kTestCompleteSuccess, action_max_timeout_ms());
+
+  // Test case 3
+  std::wstring test_case_3 = L"history_length_test_page_4.html";
+  GURL url_3 = GetTestUrl(L"History", test_case_3);
+  NavigateToURL(url_3);
+  WaitForFinish("History_Length_Test_3", "1", url_3, kTestCompleteCookie,
                 kTestCompleteSuccess, action_max_timeout_ms());
 }