Nukes remove_first_entry from CopyStateFromAndPrune. This turned out bad for two reasons:

. Doing this results in the browser thinking the current page is
  the last page the user was on. For example, if you start on the new
  tab page do an instant search and press enter before the page pushes a
  navigation then the browser shows chrome://newtab until a navigation
  is pushed.
. If a message is in flight from the renderer at the time this happens
  NavigationController can't locate a NavigationEntry, thinks the
  renderer is spamming it and kills the page.

BUG=95228
TEST=see bug
R=cbentzel@chromium.org

Review URL: http://codereview.chromium.org/7828051

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

3 files changed, 5 insertions, 117 deletions

diff --git a/content/browser/tab_contents/navigation_controller.cc b/content/browser/tab_contents/navigation_controller.cc
index 1ee7492..221ac5e 100644
--- a/content/browser/tab_contents/navigation_controller.cc
+++ b/content/browser/tab_contents/navigation_controller.cc
@@ -903,8 +903,7 @@ void NavigationController::CopyStateFrom(const NavigationController& source) {
   FinishRestore(source.last_committed_entry_index_, false);
 }
 
-void NavigationController::CopyStateFromAndPrune(NavigationController* source,
-                                                 bool remove_first_entry) {
+void NavigationController::CopyStateFromAndPrune(NavigationController* source) {
   // The SiteInstance and page_id of the last committed entry needs to be
   // remembered at this point, in case there is only one committed entry
   // and it is pruned.
@@ -920,33 +919,6 @@ void NavigationController::CopyStateFromAndPrune(NavigationController* source,
                              pending_entry_index_ == entry_count() - 1)) ||
          (!pending_entry_ && last_committed_entry_index_ == entry_count() - 1));
 
-  if (remove_first_entry && entry_count()) {
-    // If there is only one committed entry and |remove_first_entry| is true,
-    // it needs to be pruned. This is accomplished by specifying a larger
-    // |minimum_page_id| than the committed entry's page_id in the
-    // ViewMsg_SetHistoryLengthAndPrune message. However, any pages which are
-    // committed between now and when the RenderView handles the message will
-    // need to be retained. Both constraints can be met by incrementing the
-    // |minimum_page_id| by 1.
-    DCHECK(minimum_page_id >= 0);
-    if (entry_count() == 1)
-      ++minimum_page_id;
-    // Save then restore the pending entry (RemoveEntryAtIndexInternal chucks
-    // the pending entry).
-    NavigationEntry* pending_entry = pending_entry_;
-    pending_entry_ = NULL;
-    int pending_entry_index = pending_entry_index_;
-    RemoveEntryAtIndexInternal(0);
-    // Restore the pending entry.
-    if (pending_entry_index != -1) {
-      pending_entry_index_ = pending_entry_index - 1;
-      if (pending_entry_index_ != -1)
-        pending_entry_ = entries_[pending_entry_index_].get();
-    } else if (pending_entry) {
-      pending_entry_ = pending_entry;
-    }
-  }
-
   // Remove all the entries leaving the active entry.
   PruneAllButActive();
 
diff --git a/content/browser/tab_contents/navigation_controller.h b/content/browser/tab_contents/navigation_controller.h
index a410c4d..d5b24ce 100644
--- a/content/browser/tab_contents/navigation_controller.h
+++ b/content/browser/tab_contents/navigation_controller.h
@@ -280,11 +280,7 @@ class NavigationController {
   // this:   E F *G*   (last must be active or pending)
   // result: A B *G*
   // This ignores the transient index of the source and honors that of 'this'.
-  //
-  // If |remove_first_entry| is true, the first NavigationEntry is removed
-  // from this before merging.
-  void CopyStateFromAndPrune(NavigationController* source,
-                             bool remove_first_entry);
+  void CopyStateFromAndPrune(NavigationController* source);
 
   // Removes all the entries except the active entry. If there is a new pending
   // navigation it is preserved.
diff --git a/content/browser/tab_contents/navigation_controller_unittest.cc b/content/browser/tab_contents/navigation_controller_unittest.cc
index fbb9b01..e33eddf 100644
--- a/content/browser/tab_contents/navigation_controller_unittest.cc
+++ b/content/browser/tab_contents/navigation_controller_unittest.cc
@@ -1933,7 +1933,7 @@ TEST_F(NavigationControllerTest, CopyStateFromAndPrune) {
   other_contents->ExpectSetHistoryLengthAndPrune(
       other_controller.GetEntryAtIndex(0)->site_instance(), 2,
       other_controller.GetEntryAtIndex(0)->page_id());
-  other_controller.CopyStateFromAndPrune(&controller(), false);
+  other_controller.CopyStateFromAndPrune(&controller());
 
   // other_controller should now contain the 3 urls: url1, url2 and url3.
 
@@ -1960,7 +1960,7 @@ TEST_F(NavigationControllerTest, CopyStateFromAndPrune2) {
   scoped_ptr<TestTabContents> other_contents(CreateTestTabContents());
   NavigationController& other_controller = other_contents->controller();
   other_contents->ExpectSetHistoryLengthAndPrune(NULL, 1, -1);
-  other_controller.CopyStateFromAndPrune(&controller(), false);
+  other_controller.CopyStateFromAndPrune(&controller());
 
   // other_controller should now contain the 1 url: url1.
 
@@ -1986,7 +1986,7 @@ TEST_F(NavigationControllerTest, CopyStateFromAndPrune3) {
   NavigationController& other_controller = other_contents->controller();
   other_controller.LoadURL(url3, GURL(), PageTransition::TYPED);
   other_contents->ExpectSetHistoryLengthAndPrune(NULL, 1, -1);
-  other_controller.CopyStateFromAndPrune(&controller(), false);
+  other_controller.CopyStateFromAndPrune(&controller());
 
   // other_controller should now contain 1 entry for url1, and a pending entry
   // for url3.
@@ -2003,86 +2003,6 @@ TEST_F(NavigationControllerTest, CopyStateFromAndPrune3) {
   EXPECT_EQ(url3, other_controller.pending_entry()->url());
 }
 
-// Test CopyStateFromAndPrune with 1 url in source, nothing in target and
-// remove_first = true.
-TEST_F(NavigationControllerTest, CopyStateFromAndPrune4) {
-  const GURL url1("http://foo1");
-
-  NavigateAndCommit(url1);
-
-  scoped_ptr<TestTabContents> other_contents(CreateTestTabContents());
-  NavigationController& other_controller = other_contents->controller();
-  other_contents->ExpectSetHistoryLengthAndPrune(NULL, 1, -1);
-  other_controller.CopyStateFromAndPrune(&controller(), true);
-
-  // other_controller should now contain 1 entry for url1.
-
-  ASSERT_EQ(1, other_controller.entry_count());
-
-  EXPECT_EQ(0, other_controller.GetCurrentEntryIndex());
-
-  EXPECT_EQ(url1, other_controller.GetEntryAtIndex(0)->url());
-
-  // And there should be a pending entry for url3.
-  ASSERT_FALSE(other_controller.pending_entry());
-}
-
-// Test CopyStateFromAndPrune with 1 url in source, 1 in target and
-// remove_first = true.
-TEST_F(NavigationControllerTest, CopyStateFromAndPrune5) {
-  const GURL url1("http://foo1");
-  const GURL url2("http://foo2");
-
-  NavigateAndCommit(url1);
-
-  scoped_ptr<TestTabContents> other_contents(CreateTestTabContents());
-  NavigationController& other_controller = other_contents->controller();
-  other_contents->NavigateAndCommit(url2);
-  other_contents->ExpectSetHistoryLengthAndPrune(
-      other_controller.GetEntryAtIndex(0)->site_instance(),
-      1,
-      other_controller.GetEntryAtIndex(0)->page_id() + 1);
-  other_controller.CopyStateFromAndPrune(&controller(), true);
-
-  // other_controller should now contain 1 entry, url1.
-
-  ASSERT_EQ(1, other_controller.entry_count());
-  EXPECT_EQ(0, other_controller.GetCurrentEntryIndex());
-  EXPECT_EQ(url1, other_controller.GetEntryAtIndex(0)->url());
-
-  // And there should be a pending entry for url3.
-  ASSERT_FALSE(other_controller.pending_entry());
-}
-
-// Test CopyStateFromAndPrune with 1 url in source, 2 in target and
-// remove_first = true.
-TEST_F(NavigationControllerTest, CopyStateFromAndPrune6) {
-  const GURL url1("http://foo1");
-  const GURL url2("http://foo2");
-  const GURL url3("http://foo2");
-
-  NavigateAndCommit(url1);
-
-  scoped_ptr<TestTabContents> other_contents(CreateTestTabContents());
-  NavigationController& other_controller = other_contents->controller();
-  other_contents->NavigateAndCommit(url2);
-  other_controller.LoadURL(url3, GURL(), PageTransition::TYPED);
-  other_contents->ExpectSetHistoryLengthAndPrune(
-      other_controller.GetEntryAtIndex(0)->site_instance(),
-      1,
-      other_controller.GetEntryAtIndex(0)->page_id() + 1);
-  other_controller.CopyStateFromAndPrune(&controller(), true);
-
-  // other_controller should now contain 2 entries: url1, and url3.
-  ASSERT_EQ(1, other_controller.entry_count());
-  EXPECT_EQ(0, other_controller.GetCurrentEntryIndex());
-  EXPECT_EQ(url1, other_controller.GetEntryAtIndex(0)->url());
-
-  // And there should be a pending entry for url3.
-  ASSERT_TRUE(other_controller.pending_entry());
-  EXPECT_EQ(url3, other_controller.pending_entry()->url());
-}
-
 // Tests that navigations initiated from the page (with the history object)
 // work as expected without navigation entries.
 TEST_F(NavigationControllerTest, HistoryNavigate) {