TabRestoreService improvements

this patch brings more code from the views into the model (TabRestoreService) so the views don't all duplicate each other, or forget to duplicate each other's behavior and then act inexplicably different. For example, only RecentlyClosedHandler was removing duplicates, and it doesn't make sense to diverge on that point. (In the end, this patch actually removes all de-duping because two tabs may diverge in their history even if their current navigation is the same.)

This also fixes the issue where filtering was done after the list had been pruned to 10 entries. The issue with doing this is that filtered entries (such as New Tabs or duplicate tabs) counted against the total of 10 entries, but wouldn't show up in the list, so you'd frequently show <10 tabs even though there are 10 valid, closed tabs we could show.

BUG=83211, 81805
TEST=
1. open 10 tabs, close them all. Start opening and closing new tabs --- the recently closed list shouldn't be affected.
2. The jumplist on Windows and the global history menu on mac/gtk should not show duplicate entries
3. if you close a window with 2 normal tabs and one ntp in it, it should show up as "3 tabs", not "2 tabs"

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

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

16 files changed, 283 insertions, 346 deletions

diff --git a/chrome/browser/automation/automation_provider_observers.cc b/chrome/browser/automation/automation_provider_observers.cc
index 26125f7..2224032 100644
--- a/chrome/browser/automation/automation_provider_observers.cc
+++ b/chrome/browser/automation/automation_provider_observers.cc
@@ -50,7 +50,6 @@
 #include "chrome/browser/search_engines/template_url_service.h"
 #include "chrome/browser/search_engines/template_url_service_factory.h"
 #include "chrome/browser/sessions/restore_tab_helper.h"
-#include "chrome/browser/sessions/session_utils.h"
 #include "chrome/browser/sessions/tab_restore_service.h"
 #include "chrome/browser/sessions/tab_restore_service_factory.h"
 #include "chrome/browser/tab_contents/confirm_infobar_delegate.h"
@@ -2083,11 +2082,9 @@ NTPInfoObserver::NTPInfoObserver(
   ntp_info_->Set("apps", apps_list);
 
   // Get the info that would be displayed in the recently closed section.
-  TabRestoreService::Entries entries;
-  SessionUtils::FilteredEntries(service->entries(), &entries);
   ListValue* recently_closed_list = new ListValue;
-  RecentlyClosedTabsHandler::AddRecentlyClosedEntries(entries,
-                                                      recently_closed_list);
+  RecentlyClosedTabsHandler::CreateRecentlyClosedValues(service->entries(),
+                                                        recently_closed_list);
   ntp_info_->Set("recently_closed", recently_closed_list);
 
   // Add default site URLs.
diff --git a/chrome/browser/jumplist_win.cc b/chrome/browser/jumplist_win.cc
index 02e8d35..2413c9e 100644
--- a/chrome/browser/jumplist_win.cc
+++ b/chrome/browser/jumplist_win.cc
@@ -665,16 +665,12 @@ bool JumpList::AddTab(const TabRestoreService::Tab* tab,
                       size_t max_items) {
   // This code adds the URL and the title strings of the given tab to the
   // specified list.
-  // This code is copied from RecentlyClosedTabsHandler::TabToValue().
-  if (tab->navigations.empty() || list->size() >= max_items)
+  if (list->size() >= max_items)
     return false;
 
+  scoped_refptr<ShellLinkItem> link(new ShellLinkItem);
   const TabNavigation& current_navigation =
       tab->navigations.at(tab->current_navigation_index);
-  if (current_navigation.virtual_url() == GURL(chrome::kChromeUINewTabURL))
-    return false;
-
-  scoped_refptr<ShellLinkItem> link(new ShellLinkItem);
   std::string url = current_navigation.virtual_url().spec();
   link->SetArguments(UTF8ToWide(url));
   link->SetTitle(current_navigation.title());
@@ -683,22 +679,17 @@ bool JumpList::AddTab(const TabRestoreService::Tab* tab,
   return true;
 }
 
-bool JumpList::AddWindow(const TabRestoreService::Window* window,
+void JumpList::AddWindow(const TabRestoreService::Window* window,
                          ShellLinkItemList* list,
                          size_t max_items) {
   // This code enumerates al the tabs in the given window object and add their
   // URLs and titles to the list.
-  // This code is copied from RecentlyClosedTabsHandler::WindowToValue().
-  if (window->tabs.empty()) {
-    NOTREACHED();
-    return false;
-  }
+  DCHECK(!window->tabs.empty());
+
   for (size_t i = 0; i < window->tabs.size(); ++i) {
     if (!AddTab(&window->tabs[i], list, max_items))
-      return false;
+      return;
   }
-
-  return true;
 }
 
 bool JumpList::StartLoadingFavicon() {
diff --git a/chrome/browser/jumplist_win.h b/chrome/browser/jumplist_win.h
index 7f02788..75057f5 100644
--- a/chrome/browser/jumplist_win.h
+++ b/chrome/browser/jumplist_win.h
@@ -153,7 +153,7 @@ class JumpList : public TabRestoreServiceObserver,
   bool AddTab(const TabRestoreService::Tab* tab,
               ShellLinkItemList* list,
               size_t max_items);
-  bool AddWindow(const TabRestoreService::Window* window,
+  void AddWindow(const TabRestoreService::Window* window,
                  ShellLinkItemList* list,
                  size_t max_items);
 
diff --git a/chrome/browser/sessions/session_types.h b/chrome/browser/sessions/session_types.h
index 6a17e5a..01ed823 100644
--- a/chrome/browser/sessions/session_types.h
+++ b/chrome/browser/sessions/session_types.h
@@ -59,6 +59,7 @@ class TabNavigation {
   const GURL& referrer() const { return referrer_; }
 
   // The title of the page.
+  void set_title(const string16& title) { title_ = title; }
   const string16& title() const { return title_; }
 
   // State bits.
diff --git a/chrome/browser/sessions/session_utils.cc b/chrome/browser/sessions/session_utils.cc
deleted file mode 100644
index 8ae293c..0000000
--- a/chrome/browser/sessions/session_utils.cc
+++ /dev/null
@@ -1,61 +0,0 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#include "chrome/browser/sessions/session_utils.h"
-
-#include <set>
-
-#include "chrome/browser/sessions/tab_restore_service.h"
-
-namespace {
-
-// This class is used to compare two entries and considers that they are
-// identical if they share the same URL. See FilteredEntries() below for how
-// this is used.
-class EntryComparator {
- public:
-  bool operator() (const TabRestoreService::Entry* lhs,
-                   const TabRestoreService::Entry* rhs) {
-    // Two entries are deemed identical when their visible title and URL are
-    // the same.
-    if (lhs->type == TabRestoreService::WINDOW ||
-        rhs->type == TabRestoreService::WINDOW)
-      return false;
-
-    DCHECK(lhs->type == TabRestoreService::TAB);
-    DCHECK(rhs->type == TabRestoreService::TAB);
-
-    const TabRestoreService::Tab* rh_tab =
-    static_cast<const TabRestoreService::Tab*>(rhs);
-    const TabRestoreService::Tab* lh_tab =
-    static_cast<const TabRestoreService::Tab*>(lhs);
-
-    const TabNavigation& rh_entry =
-    rh_tab->navigations[rh_tab->current_navigation_index];
-    const TabNavigation& lh_entry =
-    lh_tab->navigations[lh_tab->current_navigation_index];
-
-    if (rh_entry.title() == lh_entry.title())
-      return rh_entry.virtual_url().spec() < lh_entry.virtual_url().spec();
-
-    return rh_entry.title() < lh_entry.title();
-  }
-};
-}  // namespace
-
-void SessionUtils::FilteredEntries(const TabRestoreService::Entries& entries,
-    TabRestoreService::Entries* filtered_entries) {
-  // A set to remember the entries we already seen.
-  std::set<TabRestoreService::Entry*, EntryComparator> uniquing;
-
-  for (TabRestoreService::Entries::const_iterator iter = entries.begin();
-       iter != entries.end(); ++iter) {
-    TabRestoreService::Entry* entry = *iter;
-
-    if (uniquing.insert(entry).second)
-      // This entry was not seen before, add it to the list.
-      filtered_entries->push_back(entry);
-  }
-}
-
diff --git a/chrome/browser/sessions/session_utils.h b/chrome/browser/sessions/session_utils.h
deleted file mode 100644
index c148487..0000000
--- a/chrome/browser/sessions/session_utils.h
+++ /dev/null
@@ -1,26 +0,0 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#ifndef CHROME_BROWSER_SESSIONS_SESSION_UTILS_H_
-#define CHROME_BROWSER_SESSIONS_SESSION_UTILS_H_
-#pragma once
-
-#include "chrome/browser/sessions/tab_restore_service.h"
-
-class SessionUtils {
- public:
-  // Fill the passed list from the entries passed as argument filtering out the
-  // ones with the same title and URL as a previous entry. This avoid filling
-  // the list with things that look like duplicates.
-  //
-  // The |filtering_entries| returned are simply references to the one passed in
-  // without copy: There is no transfer of ownership.
-  static void FilteredEntries(const TabRestoreService::Entries& entries,
-                              TabRestoreService::Entries* filtered_entries);
-
- private:
-  DISALLOW_IMPLICIT_CONSTRUCTORS(SessionUtils);
-};
-
-#endif  // CHROME_BROWSER_SESSIONS_SESSION_UTILS_H_
diff --git a/chrome/browser/sessions/session_utils_unittest.cc b/chrome/browser/sessions/session_utils_unittest.cc
deleted file mode 100644
index 21c11fb..0000000
--- a/chrome/browser/sessions/session_utils_unittest.cc
+++ /dev/null
@@ -1,76 +0,0 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#include "chrome/browser/sessions/session_utils.h"
-
-#include "base/stl_util.h"
-#include "base/string16.h"
-#include "base/utf_string_conversions.h"
-#include "chrome/browser/sessions/session_types.h"
-#include "googleurl/src/gurl.h"
-#include "testing/gtest/include/gtest/gtest.h"
-
-class TabNavigationMock : public TabNavigation {
- public:
-  TabNavigationMock(const char* url, const char* title)
-      : TabNavigation(0,                                  // index
-                      GURL(string16(ASCIIToUTF16(url))),  // virtual_url
-                      GURL(),                             // referrer
-                      string16(ASCIIToUTF16(title)),      // title
-                      "",                                 // state
-                      content::PAGE_TRANSITION_FROM_ADDRESS_BAR) {
-  }
-};
-
-class SessionUtilsTest : public testing::Test {
- protected:
-  class TabMock : public TabRestoreService::Tab {
-   public:
-    TabMock(const char* url, const char* title) {
-      navigations.push_back(TabNavigationMock(url, title));
-      current_navigation_index = 0;
-    }
-  };
-
-  virtual void SetUp() {
-    // prefill the entries
-
-    // Two identical
-    entries_.push_back(new TabMock("http://a", "a"));
-    entries_.push_back(new TabMock("http://a", "a"));
-
-    // Different URL
-    entries_.push_back(new TabMock("http://b", "b"));
-    entries_.push_back(new TabMock("http://c", "b"));
-
-    // Different Title
-    entries_.push_back(new TabMock("http://d", "d"));
-    entries_.push_back(new TabMock("http://d", "e"));
-
-    // Nothing in common
-    entries_.push_back(new TabMock("http://f", "f"));
-    entries_.push_back(new TabMock("http://g", "g"));
-  }
-
-  void TearDown() {
-    STLDeleteElements(&entries_);
-  }
-
-  TabRestoreService::Entries entries_;
-};
-
-TEST_F(SessionUtilsTest, SessionUtilsFilter) {
-  TabRestoreService::Entries filtered_entries;
-
-  SessionUtils::FilteredEntries(entries_, &filtered_entries);
-  ASSERT_EQ(7U, filtered_entries.size());
-
-  // The filtering should have removed the second item
-  TabRestoreService::Entries expected = entries_;
-  TabRestoreService::Entry* first = expected.front();
-  expected.pop_front();
-  expected.pop_front();
-  expected.push_front(first);
-  ASSERT_EQ(expected, filtered_entries);
-}
diff --git a/chrome/browser/sessions/tab_restore_service.cc b/chrome/browser/sessions/tab_restore_service.cc
index 0e26eaf..99d1153 100644
--- a/chrome/browser/sessions/tab_restore_service.cc
+++ b/chrome/browser/sessions/tab_restore_service.cc
@@ -26,6 +26,7 @@
 #include "chrome/browser/ui/tab_contents/tab_contents_wrapper.h"
 #include "chrome/common/extensions/extension.h"
 #include "chrome/common/extensions/extension_constants.h"
+#include "chrome/common/url_constants.h"
 #include "content/browser/tab_contents/navigation_controller.h"
 #include "content/browser/tab_contents/navigation_entry.h"
 #include "content/browser/tab_contents/tab_contents.h"
@@ -56,7 +57,7 @@ TabRestoreService::Entry::~Entry() {}
 // TabRestoreService ----------------------------------------------------------
 
 // static
-const size_t TabRestoreService::kMaxEntries = 10;
+const size_t TabRestoreService::kMaxEntries = 25;
 
 // Identifier for commands written to file.
 // The ordering in the file is as follows:
@@ -191,7 +192,6 @@ TabRestoreService::TabRestoreService(Profile* profile,
                          FilePath()),
       load_state_(NOT_LOADED),
       restoring_(false),
-      reached_max_(false),
       entries_to_write_(0),
       entries_written_(0),
       time_factory_(time_factory) {
@@ -418,7 +418,7 @@ void TabRestoreService::RestoreEntryById(TabRestoreServiceDelegate* delegate,
 }
 
 void TabRestoreService::LoadTabsFromLastSession() {
-  if (load_state_ != NOT_LOADED || reached_max_)
+  if (load_state_ != NOT_LOADED || entries_.size() == kMaxEntries)
     return;
 
   load_state_ = LOADING;
@@ -527,25 +527,40 @@ void TabRestoreService::NotifyTabsChanged() {
 }
 
 void TabRestoreService::AddEntry(Entry* entry, bool notify, bool to_front) {
+  if (!FilterEntry(entry) || (entries_.size() >= kMaxEntries && !to_front)) {
+    delete entry;
+    return;
+  }
+
   if (to_front)
     entries_.push_front(entry);
   else
     entries_.push_back(entry);
+
   if (notify)
-    PruneAndNotify();
+    NotifyTabsChanged();
+
   // Start the save timer, when it fires we'll generate the commands.
   StartSaveTimer();
   entries_to_write_++;
 }
 
-void TabRestoreService::PruneAndNotify() {
-  while (entries_.size() > kMaxEntries) {
-    delete entries_.back();
-    entries_.pop_back();
-    reached_max_ = true;
+void TabRestoreService::PruneEntries() {
+  Entries new_entries;
+
+  for (TabRestoreService::Entries::const_iterator iter = entries_.begin();
+       iter != entries_.end(); ++iter) {
+    TabRestoreService::Entry* entry = *iter;
+
+    if (FilterEntry(entry) &&
+        new_entries.size() < kMaxEntries) {
+      new_entries.push_back(entry);
+    } else {
+      delete entry;
+    }
   }
 
-  NotifyTabsChanged();
+  entries_ = new_entries;
 }
 
 TabRestoreService::Entries::iterator TabRestoreService::GetEntryIteratorById(
@@ -885,8 +900,7 @@ void TabRestoreService::CreateEntriesFromCommands(
     }
   }
 
-  // If there was corruption some of the entries won't be valid. Prune any
-  // entries with no navigations.
+  // If there was corruption some of the entries won't be valid.
   ValidateAndDeleteEmptyEntries(&(entries.get()));
 
   loaded_entries->swap(entries.get());
@@ -942,42 +956,91 @@ bool TabRestoreService::ValidateTab(Tab* tab) {
   tab->current_navigation_index =
       std::max(0, std::min(tab->current_navigation_index,
                            static_cast<int>(tab->navigations.size()) - 1));
+
   return true;
 }
 
+bool TabRestoreService::ValidateWindow(Window* window) {
+  window->selected_tab_index =
+      std::max(0, std::min(window->selected_tab_index,
+                           static_cast<int>(window->tabs.size() - 1)));
+
+  int i = 0;
+  for (std::vector<Tab>::iterator tab_i = window->tabs.begin();
+       tab_i != window->tabs.end();) {
+    if (!ValidateTab(&(*tab_i))) {
+      tab_i = window->tabs.erase(tab_i);
+      if (i < window->selected_tab_index)
+        window->selected_tab_index--;
+      else if (i == window->selected_tab_index)
+        window->selected_tab_index = 0;
+    } else {
+      ++tab_i;
+      ++i;
+    }
+  }
+
+  if (window->tabs.empty())
+    return false;
+
+  return true;
+}
+
+bool TabRestoreService::ValidateEntry(Entry* entry) {
+  if (entry->type == TAB)
+    return ValidateTab(static_cast<Tab*>(entry));
+
+  if (entry->type == WINDOW)
+    return ValidateWindow(static_cast<Window*>(entry));
+
+  NOTREACHED();
+  return false;
+}
+
+bool TabRestoreService::IsTabInteresting(const Tab* tab) {
+  if (tab->navigations.empty())
+    return false;
+
+  if (tab->navigations.size() > 1)
+    return true;
+
+  return tab->pinned ||
+      tab->navigations.at(0).virtual_url() !=
+          GURL(chrome::kChromeUINewTabURL);
+}
+
+bool TabRestoreService::IsWindowInteresting(const Window* window) {
+  if (window->tabs.empty())
+    return false;
+
+  if (window->tabs.size() > 1)
+    return true;
+
+  return IsTabInteresting(&window->tabs[0]);
+}
+
+bool TabRestoreService::FilterEntry(Entry* entry) {
+  if (!ValidateEntry(entry))
+    return false;
+
+  if (entry->type == TAB)
+    return IsTabInteresting(static_cast<Tab*>(entry));
+  else if (entry->type == WINDOW)
+    return IsWindowInteresting(static_cast<Window*>(entry));
+
+  NOTREACHED();
+  return false;
+}
+
 void TabRestoreService::ValidateAndDeleteEmptyEntries(
     std::vector<Entry*>* entries) {
   std::vector<Entry*> valid_entries;
   std::vector<Entry*> invalid_entries;
 
-  size_t max_valid = kMaxEntries - entries_.size();
   // Iterate from the back so that we keep the most recently closed entries.
   for (std::vector<Entry*>::reverse_iterator i = entries->rbegin();
        i != entries->rend(); ++i) {
-    bool valid_entry = false;
-    if (valid_entries.size() != max_valid) {
-      if ((*i)->type == TAB) {
-        Tab* tab = static_cast<Tab*>(*i);
-        if (ValidateTab(tab))
-          valid_entry = true;
-      } else {
-        Window* window = static_cast<Window*>(*i);
-        for (std::vector<Tab>::iterator tab_i = window->tabs.begin();
-             tab_i != window->tabs.end();) {
-          if (!ValidateTab(&(*tab_i)))
-            tab_i = window->tabs.erase(tab_i);
-          else
-            ++tab_i;
-        }
-        if (!window->tabs.empty()) {
-          window->selected_tab_index =
-              std::max(0, std::min(window->selected_tab_index,
-                                   static_cast<int>(window->tabs.size() - 1)));
-          valid_entry = true;
-        }
-      }
-    }
-    if (valid_entry)
+    if (ValidateEntry(*i))
       valid_entries.push_back(*i);
     else
       invalid_entries.push_back(*i);
@@ -1058,7 +1121,7 @@ void TabRestoreService::LoadStateChanged() {
   // We're done loading.
   load_state_ ^= LOADING;
 
-  if (staging_entries_.empty() || reached_max_) {
+  if (entries_.size() == kMaxEntries) {
     STLDeleteElements(&staging_entries_);
     return;
   }
@@ -1091,7 +1154,8 @@ void TabRestoreService::LoadStateChanged() {
   // the front, not the end and we just added the entries to the end).
   entries_to_write_ = staging_entries_.size();
 
-  PruneAndNotify();
+  PruneEntries();
+  NotifyTabsChanged();
 }
 
 Time TabRestoreService::TimeNow() const {
diff --git a/chrome/browser/sessions/tab_restore_service.h b/chrome/browser/sessions/tab_restore_service.h
index bdd5f6a..ee8dba1 100644
--- a/chrome/browser/sessions/tab_restore_service.h
+++ b/chrome/browser/sessions/tab_restore_service.h
@@ -10,6 +10,7 @@
 #include <set>
 #include <vector>
 
+#include "base/gtest_prod_util.h"
 #include "base/observer_list.h"
 #include "base/time.h"
 #include "chrome/browser/sessions/base_session_service.h"
@@ -174,6 +175,9 @@ class TabRestoreService : public BaseSessionService {
   virtual void Save() OVERRIDE;
 
  private:
+  friend class TabRestoreServiceTest;
+  FRIEND_TEST_ALL_PREFIXES(TabRestoreServiceTest, PruneEntries);
+
   // Used to indicate what has loaded.
   enum LoadState {
     // Indicates we haven't loaded anything.
@@ -208,8 +212,9 @@ class TabRestoreService : public BaseSessionService {
   // tab/window closes from the previous session are added to the back.
   void AddEntry(Entry* entry, bool prune, bool to_front);
 
-  // Prunes entries_ to contain only kMaxEntries and invokes NotifyTabsChanged.
-  void PruneAndNotify();
+  // Prunes entries_ to contain only kMaxEntries, and removes uninteresting
+  // entries.
+  void PruneEntries();
 
   // Returns an iterator into entries_ whose id matches |id|. If |id| identifies
   // a Window, then its iterator position will be returned. If it identifies a
@@ -269,7 +274,23 @@ class TabRestoreService : public BaseSessionService {
   // Returns true if |tab| has more than one navigation. If |tab| has more
   // than one navigation |tab->current_navigation_index| is constrained based
   // on the number of navigations.
-  bool ValidateTab(Tab* tab);
+  static bool ValidateTab(Tab* tab);
+
+  // Validates all the tabs in a window, plus the window's active tab index.
+  static bool ValidateWindow(Window* window);
+
+  // Calls either ValidateTab or ValidateWindow as appropriate.
+  static bool ValidateEntry(Entry* entry);
+
+  // Returns true if |tab| is one we care about restoring.
+  static bool IsTabInteresting(const Tab* tab);
+
+  // Checks whether |window| is interesting --- if it only contains a single,
+  // uninteresting tab, it's not interesting.
+  static bool IsWindowInteresting(const Window* window);
+
+  // Validates and checks |entry| for interesting.
+  static bool FilterEntry(Entry* entry);
 
   // Validates all entries in |entries|, deleting any with no navigations.
   // This also deletes any entries beyond the max number of entries we can
@@ -300,7 +321,7 @@ class TabRestoreService : public BaseSessionService {
   // Gets the current time. This uses the time_factory_ if there is one.
   base::Time TimeNow() const;
 
-  // Set of entries.
+  // Set of entries. They are ordered from most to least recent.
   Entries entries_;
 
   // Whether we've loaded the last session.
@@ -310,9 +331,6 @@ class TabRestoreService : public BaseSessionService {
   // historical tab.
   bool restoring_;
 
-  // Have the max number of entries ever been created?
-  bool reached_max_;
-
   // The number of entries to write.
   int entries_to_write_;
 
diff --git a/chrome/browser/sessions/tab_restore_service_browsertest.cc b/chrome/browser/sessions/tab_restore_service_browsertest.cc
index 6284ea1..b11738b 100644
--- a/chrome/browser/sessions/tab_restore_service_browsertest.cc
+++ b/chrome/browser/sessions/tab_restore_service_browsertest.cc
@@ -2,6 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "base/stringprintf.h"
+#include "base/utf_string_conversions.h"
 #include "chrome/browser/sessions/session_types.h"
 #include "chrome/browser/sessions/session_service.h"
 #include "chrome/browser/sessions/session_service_factory.h"
@@ -17,6 +19,8 @@
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebKit.h"
 
+typedef TabRestoreService::Tab Tab;
+
 // Create subclass that overrides TimeNow so that we can control the time used
 // for closed tabs and windows.
 class TabRestoreTimeFactory : public TabRestoreService::TimeFactory {
@@ -136,7 +140,7 @@ TEST_F(TabRestoreServiceTest, Basic) {
   // Make sure the entry matches.
   TabRestoreService::Entry* entry = service_->entries().front();
   ASSERT_EQ(TabRestoreService::TAB, entry->type);
-  TabRestoreService::Tab* tab = static_cast<TabRestoreService::Tab*>(entry);
+  Tab* tab = static_cast<Tab*>(entry);
   EXPECT_FALSE(tab->pinned);
   EXPECT_TRUE(tab->extension_app_id.empty());
   ASSERT_EQ(3U, tab->navigations.size());
@@ -158,7 +162,7 @@ TEST_F(TabRestoreServiceTest, Basic) {
   // Make sure the entry matches
   entry = service_->entries().front();
   ASSERT_EQ(TabRestoreService::TAB, entry->type);
-  tab = static_cast<TabRestoreService::Tab*>(entry);
+  tab = static_cast<Tab*>(entry);
   EXPECT_FALSE(tab->pinned);
   ASSERT_EQ(3U, tab->navigations.size());
   EXPECT_EQ(url1_, tab->navigations[0].virtual_url());
@@ -198,7 +202,7 @@ TEST_F(TabRestoreServiceTest, DontRestorePrintPreviewTab) {
   // And verify the entry.
   TabRestoreService::Entry* entry = service_->entries().front();
   ASSERT_EQ(TabRestoreService::TAB, entry->type);
-  TabRestoreService::Tab* tab = static_cast<TabRestoreService::Tab*>(entry);
+  Tab* tab = static_cast<Tab*>(entry);
   EXPECT_FALSE(tab->pinned);
 
   // Verify that print preview tab is not restored.
@@ -226,7 +230,7 @@ TEST_F(TabRestoreServiceTest, Restore) {
   // And verify the entry.
   TabRestoreService::Entry* entry = service_->entries().front();
   ASSERT_EQ(TabRestoreService::TAB, entry->type);
-  TabRestoreService::Tab* tab = static_cast<TabRestoreService::Tab*>(entry);
+  Tab* tab = static_cast<Tab*>(entry);
   EXPECT_FALSE(tab->pinned);
   ASSERT_EQ(3U, tab->navigations.size());
   EXPECT_TRUE(url1_ == tab->navigations[0].virtual_url());
@@ -251,7 +255,7 @@ TEST_F(TabRestoreServiceTest, RestorePinnedAndApp) {
   // these tests.
   TabRestoreService::Entry* entry = service_->entries().front();
   ASSERT_EQ(TabRestoreService::TAB, entry->type);
-  TabRestoreService::Tab* tab = static_cast<TabRestoreService::Tab*>(entry);
+  Tab* tab = static_cast<Tab*>(entry);
   tab->pinned = true;
   const std::string extension_app_id("test");
   tab->extension_app_id = extension_app_id;
@@ -265,7 +269,7 @@ TEST_F(TabRestoreServiceTest, RestorePinnedAndApp) {
   // And verify the entry.
   entry = service_->entries().front();
   ASSERT_EQ(TabRestoreService::TAB, entry->type);
-  tab = static_cast<TabRestoreService::Tab*>(entry);
+  tab = static_cast<Tab*>(entry);
   EXPECT_TRUE(tab->pinned);
   ASSERT_EQ(3U, tab->navigations.size());
   EXPECT_TRUE(url1_ == tab->navigations[0].virtual_url());
@@ -295,8 +299,8 @@ TEST_F(TabRestoreServiceTest, DontPersistPostData) {
   const TabRestoreService::Entry* restored_entry = service_->entries().front();
   ASSERT_EQ(TabRestoreService::TAB, restored_entry->type);
 
-  const TabRestoreService::Tab* restored_tab =
-      static_cast<const TabRestoreService::Tab*>(restored_entry);
+  const Tab* restored_tab =
+      static_cast<const Tab*>(restored_entry);
   // There should be 3 navs.
   ASSERT_EQ(3U, restored_tab->navigations.size());
   EXPECT_EQ(time_factory_->TimeNow().ToInternalValue(),
@@ -406,7 +410,7 @@ TEST_F(TabRestoreServiceTest, LoadPreviousSessionAndTabs) {
   // Then the closed tab.
   entry = *(++service_->entries().begin());
   ASSERT_EQ(TabRestoreService::TAB, entry->type);
-  TabRestoreService::Tab* tab = static_cast<TabRestoreService::Tab*>(entry);
+  Tab* tab = static_cast<Tab*>(entry);
   ASSERT_FALSE(tab->pinned);
   ASSERT_EQ(3U, tab->navigations.size());
   EXPECT_EQ(2, tab->current_navigation_index);
@@ -448,7 +452,7 @@ TEST_F(TabRestoreServiceTest, LoadPreviousSessionAndTabsPinned) {
   // Then the closed tab.
   entry = *(++service_->entries().begin());
   ASSERT_EQ(TabRestoreService::TAB, entry->type);
-  TabRestoreService::Tab* tab = static_cast<TabRestoreService::Tab*>(entry);
+  Tab* tab = static_cast<Tab*>(entry);
   ASSERT_FALSE(tab->pinned);
   ASSERT_EQ(3U, tab->navigations.size());
   EXPECT_EQ(2, tab->current_navigation_index);
@@ -507,7 +511,7 @@ TEST_F(TabRestoreServiceTest, TimestampSurvivesRestore) {
   // Make sure the entry matches.
   TabRestoreService::Entry* entry = service_->entries().front();
   ASSERT_EQ(TabRestoreService::TAB, entry->type);
-  TabRestoreService::Tab* tab = static_cast<TabRestoreService::Tab*>(entry);
+  Tab* tab = static_cast<Tab*>(entry);
   tab->timestamp = tab_timestamp;
 
   // Set this, otherwise previous session won't be loaded.
@@ -521,8 +525,94 @@ TEST_F(TabRestoreServiceTest, TimestampSurvivesRestore) {
   // And verify the entry.
   TabRestoreService::Entry* restored_entry = service_->entries().front();
   ASSERT_EQ(TabRestoreService::TAB, restored_entry->type);
-  TabRestoreService::Tab* restored_tab =
-      static_cast<TabRestoreService::Tab*>(restored_entry);
+  Tab* restored_tab =
+      static_cast<Tab*>(restored_entry);
   EXPECT_EQ(tab_timestamp.ToInternalValue(),
             restored_tab->timestamp.ToInternalValue());
 }
+
+TEST_F(TabRestoreServiceTest, PruneEntries) {
+  service_->ClearEntries();
+  ASSERT_TRUE(service_->entries().empty());
+
+  const size_t max_entries = TabRestoreService::kMaxEntries;
+  for (size_t i = 0; i < max_entries + 5; i++) {
+    TabNavigation navigation;
+    navigation.set_virtual_url(GURL(StringPrintf("http://%d",
+                                                 static_cast<int>(i))));
+    navigation.set_title(ASCIIToUTF16(StringPrintf("%d", static_cast<int>(i))));
+
+    Tab* tab = new Tab();
+    tab->navigations.push_back(navigation);
+    tab->current_navigation_index = 0;
+
+    service_->entries_.push_back(tab);
+  }
+
+  // Only keep kMaxEntries around.
+  EXPECT_EQ(max_entries + 5, service_->entries_.size());
+  service_->PruneEntries();
+  EXPECT_EQ(max_entries, service_->entries_.size());
+  // Pruning again does nothing.
+  service_->PruneEntries();
+  EXPECT_EQ(max_entries, service_->entries_.size());
+
+  // Prune older first.
+  TabNavigation navigation;
+  navigation.set_virtual_url(GURL("http://recent"));
+  navigation.set_title(ASCIIToUTF16("Most recent"));
+  Tab* tab = new Tab();
+  tab->navigations.push_back(navigation);
+  tab->current_navigation_index = 0;
+  service_->entries_.push_front(tab);
+  EXPECT_EQ(max_entries + 1, service_->entries_.size());
+  service_->PruneEntries();
+  EXPECT_EQ(max_entries, service_->entries_.size());
+  EXPECT_EQ(GURL("http://recent"),
+      static_cast<Tab*>(service_->entries_.front())->
+          navigations[0].virtual_url());
+
+  // Ignore NTPs.
+  navigation.set_virtual_url(GURL(chrome::kChromeUINewTabURL));
+  navigation.set_title(ASCIIToUTF16("New tab"));
+
+  tab = new Tab();
+  tab->navigations.push_back(navigation);
+  tab->current_navigation_index = 0;
+  service_->entries_.push_front(tab);
+
+  EXPECT_EQ(max_entries + 1, service_->entries_.size());
+  service_->PruneEntries();
+  EXPECT_EQ(max_entries, service_->entries_.size());
+  EXPECT_EQ(GURL("http://recent"),
+      static_cast<Tab*>(service_->entries_.front())->
+          navigations[0].virtual_url());
+
+  // Don't prune pinned NTPs.
+  tab = new Tab();
+  tab->pinned = true;
+  tab->current_navigation_index = 0;
+  tab->navigations.push_back(navigation);
+  service_->entries_.push_front(tab);
+  EXPECT_EQ(max_entries + 1, service_->entries_.size());
+  service_->PruneEntries();
+  EXPECT_EQ(max_entries, service_->entries_.size());
+  EXPECT_EQ(GURL(chrome::kChromeUINewTabURL),
+      static_cast<Tab*>(service_->entries_.front())->
+          navigations[0].virtual_url());
+
+  // Don't prune NTPs that have multiple navigations.
+  // (Erase the last NTP first.)
+  service_->entries_.erase(service_->entries_.begin());
+  tab = new Tab();
+  tab->current_navigation_index = 1;
+  tab->navigations.push_back(navigation);
+  tab->navigations.push_back(navigation);
+  service_->entries_.push_front(tab);
+  EXPECT_EQ(max_entries, service_->entries_.size());
+  service_->PruneEntries();
+  EXPECT_EQ(max_entries, service_->entries_.size());
+  EXPECT_EQ(GURL(chrome::kChromeUINewTabURL),
+      static_cast<Tab*>(service_->entries_.front())->
+          navigations[1].virtual_url());
+}
diff --git a/chrome/browser/ui/cocoa/history_menu_bridge.mm b/chrome/browser/ui/cocoa/history_menu_bridge.mm
index 93c7003..99c5523 100644
--- a/chrome/browser/ui/cocoa/history_menu_bridge.mm
+++ b/chrome/browser/ui/cocoa/history_menu_bridge.mm
@@ -425,14 +425,10 @@ void HistoryMenuBridge::OnVisitedHistoryResults(
 
 HistoryMenuBridge::HistoryItem* HistoryMenuBridge::HistoryItemForTab(
     const TabRestoreService::Tab& entry) {
-  if (entry.navigations.empty())
-    return NULL;
+  DCHECK(!entry.navigations.empty());
 
   const TabNavigation& current_navigation =
       entry.navigations.at(entry.current_navigation_index);
-  if (current_navigation.virtual_url() == GURL(chrome::kChromeUINewTabURL))
-    return NULL;
-
   HistoryItem* item = new HistoryItem();
   item->title = current_navigation.title();
   item->url = current_navigation.virtual_url();
diff --git a/chrome/browser/ui/gtk/global_history_menu.cc b/chrome/browser/ui/gtk/global_history_menu.cc
index d075b1a..22e4170 100644
--- a/chrome/browser/ui/gtk/global_history_menu.cc
+++ b/chrome/browser/ui/gtk/global_history_menu.cc
@@ -166,24 +166,8 @@ GlobalHistoryMenu::HistoryItem* GlobalHistoryMenu::HistoryItemForMenuItem(
   return it != menu_item_history_map_.end() ? it->second : NULL;
 }
 
-bool GlobalHistoryMenu::HasValidHistoryItemForTab(
-    const TabRestoreService::Tab& entry) {
-  if (entry.navigations.empty())
-    return false;
-
-  const TabNavigation& current_navigation =
-      entry.navigations.at(entry.current_navigation_index);
-  if (current_navigation.virtual_url() == GURL(chrome::kChromeUINewTabURL))
-    return false;
-
-  return true;
-}
-
 GlobalHistoryMenu::HistoryItem* GlobalHistoryMenu::HistoryItemForTab(
     const TabRestoreService::Tab& entry) {
-  if (!HasValidHistoryItemForTab(entry))
-    return NULL;
-
   const TabNavigation& current_navigation =
       entry.navigations.at(entry.current_navigation_index);
   HistoryItem* item = new HistoryItem();
@@ -315,27 +299,11 @@ void GlobalHistoryMenu::TabRestoreServiceChanged(TabRestoreService* service) {
       if (tabs.empty())
         continue;
 
-      // Check that this window has valid content. Sometimes it is possible for
-      // there to not be any subitems for a given window; if that is the case,
-      // do not add the entry to the main menu.
-      int valid_tab_count = 0;
-      std::vector<TabRestoreService::Tab>::const_iterator it;
-      for (it = tabs.begin(); it != tabs.end(); ++it) {
-        if (HasValidHistoryItemForTab(*it))
-          valid_tab_count++;
-      }
-      if (valid_tab_count == 0)
-        continue;
-
-      // Create the item for the parent/window. Do not set the title yet
-      // because the actual number of items that are in the menu will not be
-      // known until things like the NTP are filtered out, which is done when
-      // the tab items are actually created.
+      // Create the item for the parent/window.
       HistoryItem* item = new HistoryItem();
       item->session_id = entry_win->id;
 
       GtkWidget* submenu = gtk_menu_new();
-
       GtkWidget* restore_item = gtk_menu_item_new_with_label(
           l10n_util::GetStringUTF8(
               IDS_HISTORY_CLOSED_RESTORE_WINDOW_LINUX).c_str());
@@ -358,22 +326,18 @@ void GlobalHistoryMenu::TabRestoreServiceChanged(TabRestoreService* service) {
 
       // Loop over the window's tabs and add them to the submenu.
       int subindex = 2;
-      for (it = tabs.begin(); it != tabs.end(); ++it) {
-        TabRestoreService::Tab tab = *it;
+      std::vector<TabRestoreService::Tab>::const_iterator iter;
+      for (iter = tabs.begin(); iter != tabs.end(); ++iter) {
+        TabRestoreService::Tab tab = *iter;
         HistoryItem* tab_item = HistoryItemForTab(tab);
-        if (tab_item) {
-          item->tabs.push_back(tab_item);
-          AddHistoryItemToMenu(tab_item,
-                               submenu,
-                               GlobalMenuBar::TAG_RECENTLY_CLOSED,
-                               subindex++);
-        }
+        item->tabs.push_back(tab_item);
+        AddHistoryItemToMenu(tab_item,
+                             submenu,
+                             GlobalMenuBar::TAG_RECENTLY_CLOSED,
+                             subindex++);
       }
 
-      // Now that the number of tabs that has been added is known, set the
-      // title of the parent menu item.
-      std::string title =
-          (item->tabs.size() == 1) ?
+      std::string title = item->tabs.size() == 1 ?
           l10n_util::GetStringUTF8(
               IDS_NEW_TAB_RECENTLY_CLOSED_WINDOW_SINGLE) :
           l10n_util::GetStringFUTF8(
@@ -381,8 +345,7 @@ void GlobalHistoryMenu::TabRestoreServiceChanged(TabRestoreService* service) {
               base::IntToString16(item->tabs.size()));
 
       // Create the menu item parent. Unlike mac, it's can't be activated.
-      GtkWidget* parent_item = gtk_menu_item_new_with_label(
-          title.c_str());
+      GtkWidget* parent_item = gtk_menu_item_new_with_label(title.c_str());
       gtk_widget_show(parent_item);
       g_object_set_data(G_OBJECT(parent_item), "type-tag",
                         GINT_TO_POINTER(GlobalMenuBar::TAG_RECENTLY_CLOSED));
@@ -392,16 +355,13 @@ void GlobalHistoryMenu::TabRestoreServiceChanged(TabRestoreService* service) {
                             index++);
       ++added_count;
     } else if (entry->type == TabRestoreService::TAB) {
-      TabRestoreService::Tab* tab =
-          static_cast<TabRestoreService::Tab*>(entry);
+      TabRestoreService::Tab* tab = static_cast<TabRestoreService::Tab*>(entry);
       HistoryItem* item = HistoryItemForTab(*tab);
-      if (item) {
-        AddHistoryItemToMenu(item,
-                             history_menu_.get(),
-                             GlobalMenuBar::TAG_RECENTLY_CLOSED,
-                             index++);
-        ++added_count;
-      }
+      AddHistoryItemToMenu(item,
+                           history_menu_.get(),
+                           GlobalMenuBar::TAG_RECENTLY_CLOSED,
+                           index++);
+      ++added_count;
     }
   }
 }
diff --git a/chrome/browser/ui/webui/ntp/recently_closed_tabs_handler.cc b/chrome/browser/ui/webui/ntp/recently_closed_tabs_handler.cc
index 5781f2c..9d364399 100644
--- a/chrome/browser/ui/webui/ntp/recently_closed_tabs_handler.cc
+++ b/chrome/browser/ui/webui/ntp/recently_closed_tabs_handler.cc
@@ -8,7 +8,6 @@
 #include "base/bind_helpers.h"
 #include "base/metrics/histogram.h"
 #include "chrome/browser/profiles/profile.h"
-#include "chrome/browser/sessions/session_utils.h"
 #include "chrome/browser/sessions/tab_restore_service_delegate.h"
 #include "chrome/browser/sessions/tab_restore_service_factory.h"
 #include "chrome/browser/ui/webui/ntp/new_tab_ui.h"
@@ -17,40 +16,30 @@
 
 namespace {
 
-bool TabToValue(const TabRestoreService::Tab& tab,
+void TabToValue(const TabRestoreService::Tab& tab,
                 DictionaryValue* dictionary) {
-  if (tab.navigations.empty())
-    return false;
-
   const TabNavigation& current_navigation =
       tab.navigations.at(tab.current_navigation_index);
-  if (current_navigation.virtual_url() == GURL(chrome::kChromeUINewTabURL))
-    return false;
   NewTabUI::SetURLTitleAndDirection(dictionary, current_navigation.title(),
                                     current_navigation.virtual_url());
   dictionary->SetString("type", "tab");
   dictionary->SetDouble("timestamp", tab.timestamp.ToDoubleT());
-  return true;
 }
 
-bool WindowToValue(const TabRestoreService::Window& window,
+void WindowToValue(const TabRestoreService::Window& window,
                    DictionaryValue* dictionary) {
-  if (window.tabs.empty()) {
-    NOTREACHED();
-    return false;
-  }
+  DCHECK(!window.tabs.empty());
+
   scoped_ptr<ListValue> tab_values(new ListValue());
   for (size_t i = 0; i < window.tabs.size(); ++i) {
-    scoped_ptr<DictionaryValue> tab_value(new DictionaryValue());
-    if (TabToValue(window.tabs[i], tab_value.get()))
-      tab_values->Append(tab_value.release());
+    DictionaryValue* tab_value = new DictionaryValue();
+    TabToValue(window.tabs[i], tab_value);
+    tab_values->Append(tab_value);
   }
-  if (tab_values->GetSize() == 0)
-    return false;
+
   dictionary->SetString("type", "window");
   dictionary->SetDouble("timestamp", window.timestamp.ToDoubleT());
   dictionary->Set("tabs", tab_values.release());
-  return true;
 }
 
 }  // namespace
@@ -80,8 +69,7 @@ void RecentlyClosedTabsHandler::HandleReopenTab(const ListValue* args) {
   if (!ExtractIntegerValue(args, &session_to_restore))
     return;
 
-  TabRestoreService::Entries entries;
-  SessionUtils::FilteredEntries(tab_restore_service_->entries(), &entries);
+  const TabRestoreService::Entries& entries = tab_restore_service_->entries();
   int index = 0;
   for (TabRestoreService::Entries::const_iterator iter = entries.begin();
        iter != entries.end(); ++iter, ++index) {
@@ -120,10 +108,8 @@ void RecentlyClosedTabsHandler::HandleGetRecentlyClosedTabs(
 void RecentlyClosedTabsHandler::TabRestoreServiceChanged(
     TabRestoreService* service) {
   ListValue list_value;
-  TabRestoreService::Entries entries;
-  SessionUtils::FilteredEntries(service->entries(), &entries);
-
-  AddRecentlyClosedEntries(entries, &list_value);
+  TabRestoreService::Entries entries = service->entries();
+  CreateRecentlyClosedValues(entries, &list_value);
 
   web_ui_->CallJavascriptFunction("recentlyClosedTabs", list_value);
 }
@@ -134,7 +120,7 @@ void RecentlyClosedTabsHandler::TabRestoreServiceDestroyed(
 }
 
 // static
-void RecentlyClosedTabsHandler::AddRecentlyClosedEntries(
+void RecentlyClosedTabsHandler::CreateRecentlyClosedValues(
     const TabRestoreService::Entries& entries, ListValue* entry_list_value) {
   const int max_count = 10;
   int added_count = 0;
@@ -145,17 +131,17 @@ void RecentlyClosedTabsHandler::AddRecentlyClosedEntries(
        it != entries.end() && added_count < max_count; ++it) {
     TabRestoreService::Entry* entry = *it;
     scoped_ptr<DictionaryValue> entry_dict(new DictionaryValue());
-    if ((entry->type == TabRestoreService::TAB &&
-         TabToValue(
-             *static_cast<TabRestoreService::Tab*>(entry),
-             entry_dict.get())) ||
-        (entry->type == TabRestoreService::WINDOW &&
-         WindowToValue(
-             *static_cast<TabRestoreService::Window*>(entry),
-             entry_dict.get()))) {
-      entry_dict->SetInteger("sessionId", entry->id);
-      entry_list_value->Append(entry_dict.release());
-      added_count++;
+    if (entry->type == TabRestoreService::TAB) {
+      TabToValue(*static_cast<TabRestoreService::Tab*>(entry),
+                 entry_dict.get());
+    } else  {
+      DCHECK_EQ(entry->type, TabRestoreService::WINDOW);
+      WindowToValue(*static_cast<TabRestoreService::Window*>(entry),
+                    entry_dict.get());
     }
+
+    entry_dict->SetInteger("sessionId", entry->id);
+    entry_list_value->Append(entry_dict.release());
+    added_count++;
   }
 }
diff --git a/chrome/browser/ui/webui/ntp/recently_closed_tabs_handler.h b/chrome/browser/ui/webui/ntp/recently_closed_tabs_handler.h
index 3fe981a..79bcdee 100644
--- a/chrome/browser/ui/webui/ntp/recently_closed_tabs_handler.h
+++ b/chrome/browser/ui/webui/ntp/recently_closed_tabs_handler.h
@@ -40,7 +40,7 @@ class RecentlyClosedTabsHandler : public WebUIMessageHandler,
 
   // Converts a list of TabRestoreService entries to the JSON format required
   // by the NTP and adds them to the given list value.
-  static void AddRecentlyClosedEntries(
+  static void CreateRecentlyClosedValues(
       const TabRestoreService::Entries& entries,
       base::ListValue* entry_list_value);
 
diff --git a/chrome/chrome_browser.gypi b/chrome/chrome_browser.gypi
index 9cdd3a6..09b7f49 100644
--- a/chrome/chrome_browser.gypi
+++ b/chrome/chrome_browser.gypi
@@ -2112,8 +2112,6 @@
         'browser/sessions/session_service_factory.h',
         'browser/sessions/session_types.cc',
         'browser/sessions/session_types.h',
-        'browser/sessions/session_utils.cc',
-        'browser/sessions/session_utils.h',
         'browser/sessions/tab_restore_service.cc',
         'browser/sessions/tab_restore_service.h',
         'browser/sessions/tab_restore_service_factory.cc',
diff --git a/chrome/chrome_tests.gypi b/chrome/chrome_tests.gypi
index 52fddbe..2aeda0d 100644
--- a/chrome/chrome_tests.gypi
+++ b/chrome/chrome_tests.gypi
@@ -1601,7 +1601,6 @@
         'browser/search_engines/template_url_unittest.cc',
         'browser/sessions/session_backend_unittest.cc',
         'browser/sessions/session_service_unittest.cc',
-        'browser/sessions/session_utils_unittest.cc',
         'browser/shell_integration_unittest.cc',
         'browser/speech/speech_input_bubble_controller_unittest.cc',
         'browser/spellchecker/spellchecker_platform_engine_unittest.cc',