From 5bc9548dfaffb2f9f3603bbfef9942b3bcc445e3 Mon Sep 17 00:00:00 2001 From: "sky@chromium.org" Date: Sat, 3 Sep 2011 19:51:13 +0000 Subject: 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 --- .../browser/tab_contents/navigation_controller.cc | 30 +------- .../browser/tab_contents/navigation_controller.h | 6 +- .../tab_contents/navigation_controller_unittest.cc | 86 +--------------------- 3 files changed, 5 insertions(+), 117 deletions(-) (limited to 'content') 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 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 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 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 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) { -- cgit v1.1