Makes deleting history no longer delete starred urls. Thiseffectively reenables the code in ExpireHistoryBackend. I also madethe code consistent so that when we delete visits as the result ofhistory deletion we don't change the typed/visit count of theunderlying url.BUG=1214201 1256202TEST=none

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

14 files changed, 603 insertions, 241 deletions

diff --git a/chrome/browser/history/expire_history_backend.cc b/chrome/browser/history/expire_history_backend.cc
index 63983a0..fcd8f67 100644
--- a/chrome/browser/history/expire_history_backend.cc
+++ b/chrome/browser/history/expire_history_backend.cc
@@ -8,6 +8,7 @@
 #include <limits>
 
 #include "base/file_util.h"
+#include "chrome/browser/bookmarks/bookmark_service.h"
 #include "chrome/browser/history/archived_database.h"
 #include "chrome/browser/history/history_database.h"
 #include "chrome/browser/history/history_notifications.h"
@@ -63,14 +64,16 @@ const int kExpirationEmptyDelayMin = 5;
 }  // namespace
 
 ExpireHistoryBackend::ExpireHistoryBackend(
-    BroadcastNotificationDelegate* delegate)
+    BroadcastNotificationDelegate* delegate,
+    BookmarkService* bookmark_service)
     : delegate_(delegate),
       main_db_(NULL),
       archived_db_(NULL),
       thumb_db_(NULL),
       text_db_(NULL),
 #pragma warning(suppress: 4355)  // Okay to pass "this" here.
-      factory_(this) {
+      factory_(this),
+      bookmark_service_(bookmark_service) {
 }
 
 ExpireHistoryBackend::~ExpireHistoryBackend() {
@@ -94,10 +97,6 @@ void ExpireHistoryBackend::DeleteURL(const GURL& url) {
   if (!main_db_->GetRowForURL(url, &url_row))
     return;  // Nothing to delete.
 
-  // The URL may be in the text database manager's temporary cache.
-  if (text_db_)
-    text_db_->DeleteURLFromUncommitted(url);
-
   // Collect all the visits and delete them. Note that we don't give up if
   // there are no visits, since the URL could still have an entry that we should
   // delete.
@@ -111,11 +110,18 @@ void ExpireHistoryBackend::DeleteURL(const GURL& url) {
   // We skip ExpireURLsForVisits (since we are deleting from the URL, and not
   // starting with visits in a given time range). We therefore need to call the
   // deletion and favicon update functions manually.
-  DeleteOneURL(url_row, &dependencies);
-  DeleteFaviconsIfPossible(dependencies.affected_favicons);
+
+  BookmarkService* bookmark_service = GetBookmarkService();
+  bool is_bookmarked =
+      (bookmark_service && bookmark_service->IsBookmarked(url));
+
+  DeleteOneURL(url_row, is_bookmarked, &dependencies);
+  if (!is_bookmarked)
+    DeleteFaviconsIfPossible(dependencies.affected_favicons);
 
   if (text_db_)
     text_db_->OptimizeChangedDatabases(dependencies.text_db_changes);
+
   BroadcastDeleteNotifications(&dependencies);
 }
 
@@ -243,20 +249,28 @@ void ExpireHistoryBackend::DeleteVisitRelatedInfo(
 
 void ExpireHistoryBackend::DeleteOneURL(
     const URLRow& url_row,
+    bool is_bookmarked,
     DeleteDependencies* dependencies) {
-  dependencies->deleted_urls.push_back(url_row);
-
-  // Delete stuff that references this URL.
-  if (thumb_db_)
-    thumb_db_->DeleteThumbnail(url_row.id());
   main_db_->DeleteSegmentForURL(url_row.id());
 
-  // Collect shared information.
-  if (url_row.favicon_id())
-    dependencies->affected_favicons.insert(url_row.favicon_id());
+  // The URL may be in the text database manager's temporary cache.
+  if (text_db_)
+    text_db_->DeleteURLFromUncommitted(url_row.url());
 
-  // Last, delete the URL entry.
-  main_db_->DeleteURLRow(url_row.id());
+  if (!is_bookmarked) {
+    dependencies->deleted_urls.push_back(url_row);
+
+    // Delete stuff that references this URL.
+    if (thumb_db_)
+      thumb_db_->DeleteThumbnail(url_row.id());
+
+    // Collect shared information.
+    if (url_row.favicon_id())
+      dependencies->affected_favicons.insert(url_row.favicon_id());
+
+    // Last, delete the URL entry.
+    main_db_->DeleteURLRow(url_row.id());
+  }
 }
 
 URLID ExpireHistoryBackend::ArchiveOneURL(const URLRow& url_row) {
@@ -304,6 +318,7 @@ void ExpireHistoryBackend::ExpireURLsForVisits(
   }
 
   // Check each unique URL with deleted visits.
+  BookmarkService* bookmark_service = GetBookmarkService();
   for (std::map<URLID, ChangedURL>::const_iterator i = changed_urls.begin();
        i != changed_urls.end(); ++i) {
     // The unique URL rows should already be filled into the dependencies.
@@ -320,20 +335,22 @@ void ExpireHistoryBackend::ExpireURLsForVisits(
     else
       url_row.set_last_visit(Time());
 
-    // Don't delete URLs with visits still in the DB.
-    if (!url_row.last_visit().is_null()) {
-      // We're not deleting the URL, update its counts when we're deleting those
-      // visits.
+    // Don't delete URLs with visits still in the DB, or bookmarked.
+    bool is_bookmarked =
+        (bookmark_service && bookmark_service->IsBookmarked(url_row.url()));
+    if (!is_bookmarked && url_row.last_visit().is_null()) {
+      // Not bookmarked and no more visits. Nuke the url.
+      DeleteOneURL(url_row, is_bookmarked, dependencies);
+    } else {
       // NOTE: The calls to std::max() below are a backstop, but they should
       // never actually be needed unless the database is corrupt (I think).
       url_row.set_visit_count(
           std::max(0, url_row.visit_count() - i->second.visit_count));
       url_row.set_typed_count(
           std::max(0, url_row.typed_count() - i->second.typed_count));
+
+      // Update the db with the new details.
       main_db_->UpdateURLRow(url_row.id(), url_row);
-    } else {
-      // This URL is toast.
-      DeleteOneURL(url_row, dependencies);
     }
   }
 }
@@ -467,5 +484,15 @@ void ExpireHistoryBackend::ParanoidExpireHistory() {
   // FIXME(brettw): Bug 1067331: write this to clean up any errors.
 }
 
+BookmarkService* ExpireHistoryBackend::GetBookmarkService() {
+  // We use the bookmark service to determine if a URL is bookmarked. The
+  // bookmark service is loaded on a separate thread and may not be done by the
+  // time we get here. We therefor block until the bookmarks have finished
+  // loading.
+  if (bookmark_service_)
+    bookmark_service_->BlockTillLoaded();
+  return bookmark_service_;
+}
+
 }  // namespace history
 
diff --git a/chrome/browser/history/expire_history_backend.h b/chrome/browser/history/expire_history_backend.h
index 78c7c75..d795f88 100644
--- a/chrome/browser/history/expire_history_backend.h
+++ b/chrome/browser/history/expire_history_backend.h
@@ -15,6 +15,7 @@
 #include "chrome/browser/history/text_database_manager.h"
 #include "testing/gtest/include/gtest/gtest_prod.h"
 
+class BookmarkService;
 class GURL;
 enum NotificationType;
 
@@ -43,7 +44,11 @@ class BroadcastNotificationDelegate {
 class ExpireHistoryBackend {
  public:
   // The delegate pointer must be non-NULL. We will NOT take ownership of it.
-  explicit ExpireHistoryBackend(BroadcastNotificationDelegate* delegate);
+  // BookmarkService may be NULL. The BookmarkService is used when expiring
+  // URLs so that we don't remove any URLs or favicons that are bookmarked
+  // (visits are removed though).
+  ExpireHistoryBackend(BroadcastNotificationDelegate* delegate,
+                       BookmarkService* bookmark_service);
   ~ExpireHistoryBackend();
 
   // Completes initialization by setting the databases that this class will use.
@@ -80,6 +85,7 @@ class ExpireHistoryBackend {
   FRIEND_TEST(ExpireHistoryTest, DeleteTextIndexForURL);
   FRIEND_TEST(ExpireHistoryTest, DeleteFaviconsIfPossible);
   FRIEND_TEST(ExpireHistoryTest, ArchiveSomeOldHistory);
+  friend class TestingProfile;
 
   struct DeleteDependencies {
     // The time range affected. These can be is_null() to be unbounded in one
@@ -144,7 +150,13 @@ class ExpireHistoryBackend {
   // check for shared information at the end).
   //
   // Assumes the main_db_ is non-NULL.
-  void DeleteOneURL(const URLRow& url_row, DeleteDependencies* dependencies);
+  //
+  // NOTE: If the url is bookmarked only the segments and text db are updated,
+  // everything else is unchanged. This is done so that bookmarks retain their
+  // favicons and thumbnails.
+  void DeleteOneURL(const URLRow& url_row,
+                    bool is_bookmarked,
+                    DeleteDependencies* dependencies);
 
   // Adds or merges the given URL row with the archived database, returning the
   // ID of the URL in the archived database, or 0 on failure. The main (source)
@@ -191,12 +203,7 @@ class ExpireHistoryBackend {
   // care about favicons so much, so don't want to stop everything if it fails).
   void DeleteFaviconsIfPossible(const std::set<FavIconID>& favicon_id);
 
-  // Broadcasts that the given URLs and star entries were deleted. Either list
-  // can be empty, in which case no notification will be sent for that type.
-  //
-  // The passed-in arguments will be cleared becuase they will be swapped into
-  // the destination message to avoid copying). However, ownership of the
-  // dependencies object will not transfer.
+  // Broadcast the URL deleted notification.
   void BroadcastDeleteNotifications(DeleteDependencies* dependencies);
 
   // Schedules a call to DoArchiveIteration at the given time in the
@@ -217,6 +224,10 @@ class ExpireHistoryBackend {
   // and deletes items. For example, URLs with no visits.
   void ParanoidExpireHistory();
 
+  // Returns the BookmarkService, blocking until it is loaded. This may return
+  // NULL.
+  BookmarkService* GetBookmarkService();
+
   // Non-owning pointer to the notification delegate (guaranteed non-NULL).
   BroadcastNotificationDelegate* delegate_;
 
@@ -234,10 +245,15 @@ class ExpireHistoryBackend {
   // the archived database.
   TimeDelta expiration_threshold_;
 
+  // The BookmarkService; may be null. This is owned by the Profile.
+  //
+  // Use GetBookmarkService to access this, which makes sure the service is
+  // loaded.
+  BookmarkService* bookmark_service_;
+
   DISALLOW_EVIL_CONSTRUCTORS(ExpireHistoryBackend);
 };
 
 }  // namespace history
 
 #endif  // CHROME_BROWSER_HISTORY_EXPIRE_HISTORY_BACKEND_H__
-
diff --git a/chrome/browser/history/expire_history_backend_unittest.cc b/chrome/browser/history/expire_history_backend_unittest.cc
index fba70eb..05c382a 100644
--- a/chrome/browser/history/expire_history_backend_unittest.cc
+++ b/chrome/browser/history/expire_history_backend_unittest.cc
@@ -7,6 +7,7 @@
 #include "base/file_util.h"
 #include "base/path_service.h"
 #include "base/scoped_ptr.h"
+#include "chrome/browser/bookmark_bar_model.h"
 #include "chrome/browser/history/archived_database.h"
 #include "chrome/browser/history/expire_history_backend.h"
 #include "chrome/browser/history/history_database.h"
@@ -29,7 +30,9 @@ class ExpireHistoryTest : public testing::Test,
                           public BroadcastNotificationDelegate {
  public:
   ExpireHistoryTest()
-      : ALLOW_THIS_IN_INITIALIZER_LIST(expirer_(this)), now_(Time::Now()) {
+      : bookmark_model_(NULL),
+        ALLOW_THIS_IN_INITIALIZER_LIST(expirer_(this, &bookmark_model_)),
+        now_(Time::Now()) {
   }
 
  protected:
@@ -55,8 +58,15 @@ class ExpireHistoryTest : public testing::Test,
     notifications_.clear();
   }
 
+  void StarURL(const GURL& url) {
+    bookmark_model_.AddURL(
+        bookmark_model_.GetBookmarkBarNode(), 0, std::wstring(), url);
+  }
+
   static bool IsStringInFile(std::wstring& filename, const char* str);
 
+  BookmarkBarModel bookmark_model_;
+
   MessageLoop message_loop_;
 
   ExpireHistoryBackend expirer_;
@@ -438,6 +448,47 @@ TEST_F(ExpireHistoryTest, DeleteURLWithoutFavicon) {
   EXPECT_TRUE(HasFavIcon(last_row.favicon_id()));
 }
 
+// DeleteURL should not delete starred urls.
+TEST_F(ExpireHistoryTest, DontDeleteStarredURL) {
+  URLID url_ids[3];
+  Time visit_times[4];
+  AddExampleData(url_ids, visit_times);
+
+  URLRow url_row;
+  ASSERT_TRUE(main_db_->GetURLRow(url_ids[2], &url_row));
+
+  // Star the last URL.
+  StarURL(url_row.url());
+
+  // Attempt to delete the url.
+  expirer_.DeleteURL(url_row.url());
+
+  // Because the url is starred, it shouldn't be deleted.
+  GURL url = url_row.url();
+  ASSERT_TRUE(main_db_->GetRowForURL(url, &url_row));
+
+  // And the favicon should exist.
+  EXPECT_TRUE(HasFavIcon(url_row.favicon_id()));
+
+  // But there should be no fts. 
+  ASSERT_EQ(0, CountTextMatchesForURL(url_row.url()));
+
+  // And no visits.
+  VisitVector visits;
+  main_db_->GetVisitsForURL(url_row.id(), &visits);
+  ASSERT_EQ(0, visits.size());
+
+  // Should still have the thumbnail.
+  ASSERT_TRUE(HasThumbnail(url_row.id()));
+
+  // Unstar the URL and delete again.
+  bookmark_model_.SetURLStarred(url, std::wstring(), false);
+  expirer_.DeleteURL(url);
+  
+  // Now it should be completely deleted.
+  EnsureURLInfoGone(url_row);
+}
+
 // Expires all URLs more recent than a given time, with no starred items.
 // Our time threshold is such that one URL should be updated (we delete one of
 // the two visits) and one is deleted.
@@ -492,6 +543,49 @@ TEST_F(ExpireHistoryTest, FlushRecentURLsUnstarred) {
   EXPECT_FALSE(HasFavIcon(url_row2.favicon_id()));
 }
 
+// Expire a starred URL, it shouldn't get deleted
+TEST_F(ExpireHistoryTest, FlushRecentURLsStarred) {
+  URLID url_ids[3];
+  Time visit_times[4];
+  AddExampleData(url_ids, visit_times);
+
+  URLRow url_row1, url_row2;
+  ASSERT_TRUE(main_db_->GetURLRow(url_ids[1], &url_row1));
+  ASSERT_TRUE(main_db_->GetURLRow(url_ids[2], &url_row2));
+
+  // Star the last two URLs.
+  StarURL(url_row1.url());
+  StarURL(url_row2.url());
+
+  // This should delete the last two visits.
+  expirer_.ExpireHistoryBetween(visit_times[2], Time());
+
+  // The URL rows should still exist.
+  URLRow new_url_row1, new_url_row2;
+  ASSERT_TRUE(main_db_->GetURLRow(url_ids[1], &new_url_row1));
+  ASSERT_TRUE(main_db_->GetURLRow(url_ids[2], &new_url_row2));
+
+  // The visit times should be updated.
+  EXPECT_TRUE(new_url_row1.last_visit() == visit_times[1]);
+  EXPECT_TRUE(new_url_row2.last_visit().is_null());  // No last visit time.
+
+  // Visit/typed count should not be updated for bookmarks.
+  EXPECT_EQ(0, new_url_row1.typed_count());
+  EXPECT_EQ(1, new_url_row1.visit_count());
+  EXPECT_EQ(0, new_url_row2.typed_count());
+  EXPECT_EQ(0, new_url_row2.visit_count());
+
+  // Thumbnails and favicons should still exist. Note that we keep thumbnails
+  // that may have been updated since the time threshold. Since the URL still
+  // exists in history, this should not be a privacy problem, we only update
+  // the visit counts in this case for consistency anyway.
+  EXPECT_TRUE(HasFavIcon(new_url_row1.favicon_id()));
+  EXPECT_TRUE(HasThumbnail(new_url_row1.id()));
+  EXPECT_TRUE(HasFavIcon(new_url_row2.favicon_id()));
+  EXPECT_TRUE(HasThumbnail(new_url_row2.id()));
+
+}
+
 TEST_F(ExpireHistoryTest, ArchiveHistoryBeforeUnstarred) {
   URLID url_ids[3];
   Time visit_times[4];
@@ -530,6 +624,51 @@ TEST_F(ExpireHistoryTest, ArchiveHistoryBeforeUnstarred) {
   EXPECT_EQ(1, archived_visits.size());
 }
 
+TEST_F(ExpireHistoryTest, ArchiveHistoryBeforeStarred) {
+  URLID url_ids[3];
+  Time visit_times[4];
+  AddExampleData(url_ids, visit_times);
+
+  URLRow url_row0, url_row1;
+  ASSERT_TRUE(main_db_->GetURLRow(url_ids[0], &url_row0));
+  ASSERT_TRUE(main_db_->GetURLRow(url_ids[1], &url_row1));
+
+  // Star the URLs.
+  StarURL(url_row0.url());
+  StarURL(url_row1.url());
+
+  // Now archive the first three visits (first two URLs). The first two visits
+  // should be, the third deleted, but the URL records should not.
+  expirer_.ArchiveHistoryBefore(visit_times[2]);
+
+  // The first URL should have its visit deleted, but it should still be present
+  // in the main DB and not in the archived one since it is starred.
+  URLRow temp_row;
+  ASSERT_TRUE(main_db_->GetURLRow(url_ids[0], &temp_row));
+  // Note that the ID is different in the archived DB, so look up by URL.
+  EXPECT_FALSE(archived_db_->GetRowForURL(temp_row.url(), NULL));
+  VisitVector visits;
+  main_db_->GetVisitsForURL(temp_row.id(), &visits);
+  EXPECT_EQ(0, visits.size());
+
+  // The second URL should have its first visit deleted and its second visit
+  // archived. It should be present in both the main DB (because it's starred)
+  // and the archived DB (for the archived visit).
+  ASSERT_TRUE(main_db_->GetURLRow(url_ids[1], &temp_row));
+  main_db_->GetVisitsForURL(temp_row.id(), &visits);
+  EXPECT_EQ(0, visits.size());
+
+  // Note that the ID is different in the archived DB, so look up by URL.
+  ASSERT_TRUE(archived_db_->GetRowForURL(temp_row.url(), &temp_row));
+  archived_db_->GetVisitsForURL(temp_row.id(), &visits);
+  ASSERT_EQ(1, visits.size());
+  EXPECT_TRUE(visit_times[2] == visits[0].visit_time);
+
+  // The third URL should be unchanged.
+  EXPECT_TRUE(main_db_->GetURLRow(url_ids[2], &temp_row));
+  EXPECT_FALSE(archived_db_->GetRowForURL(temp_row.url(), NULL));
+}
+
 // Tests the return values from ArchiveSomeOldHistory. The rest of the
 // functionality of this function is tested by the ArchiveHistoryBefore*
 // tests which use this function internally.
@@ -557,4 +696,3 @@ TEST_F(ExpireHistoryTest, ArchiveSomeOldHistory) {
 // Maybe also refer to invalid favicons.
 
 }  // namespace history
-
diff --git a/chrome/browser/history/history.cc b/chrome/browser/history/history.cc
index abbde8b..94f980e 100644
--- a/chrome/browser/history/history.cc
+++ b/chrome/browser/history/history.cc
@@ -78,6 +78,11 @@ class HistoryService::BackendDelegate : public HistoryBackend::Delegate {
         &HistoryService::BroadcastNotifications, type, details));
   }
 
+  virtual void DBLoaded() {
+    message_loop_->PostTask(FROM_HERE, NewRunnableMethod(history_service_.get(),
+        &HistoryService::OnDBLoaded));
+  }
+
  private:
   scoped_refptr<HistoryService> history_service_;
   MessageLoop* message_loop_;
@@ -88,7 +93,8 @@ const history::StarID HistoryService::kBookmarkBarID = 1;
 
 HistoryService::HistoryService()
     : thread_(new ChromeThread(ChromeThread::HISTORY)),
-      profile_(NULL) {
+      profile_(NULL),
+      backend_loaded_(false) {
   if (NotificationService::current()) {  // Is NULL when running generate_profile.
     NotificationService::current()->AddObserver(
         this, NOTIFY_HISTORY_URLS_DELETED, Source<Profile>(profile_));
@@ -97,7 +103,8 @@ HistoryService::HistoryService()
 
 HistoryService::HistoryService(Profile* profile)
     : thread_(new ChromeThread(ChromeThread::HISTORY)),
-      profile_(profile) {
+      profile_(profile),
+      backend_loaded_(false) {
   NotificationService::current()->AddObserver(
       this, NOTIFY_HISTORY_URLS_DELETED, Source<Profile>(profile_));
 }
@@ -113,13 +120,15 @@ HistoryService::~HistoryService() {
   }
 }
 
-bool HistoryService::Init(const std::wstring& history_dir) {
+bool HistoryService::Init(const std::wstring& history_dir,
+                          BookmarkService* bookmark_service) {
   if (!thread_->Start())
     return false;
 
   // Create the history backend.
   scoped_refptr<HistoryBackend> backend(
-      new HistoryBackend(history_dir, new BackendDelegate(this)));
+      new HistoryBackend(history_dir, new BackendDelegate(this),
+                         bookmark_service));
   history_backend_.swap(backend);
 
   ScheduleAndForget(PRIORITY_UI, &HistoryBackend::Init);
@@ -206,6 +215,11 @@ HistoryService::Handle HistoryService::GetMostRecentKeywordSearchTerms(
                   keyword_id, prefix, max_count);
 }
 
+void HistoryService::URLsNoLongerBookmarked(const std::set<GURL>& urls) {
+  ScheduleAndForget(PRIORITY_NORMAL, &HistoryBackend::URLsNoLongerBookmarked,
+                    urls);
+}
+
 HistoryService::Handle HistoryService::ScheduleDBTask(
     HistoryDBTask* task,
     CancelableRequestConsumerBase* consumer) {
@@ -216,13 +230,6 @@ HistoryService::Handle HistoryService::ScheduleDBTask(
                   request);
 }
 
-HistoryService::Handle HistoryService::ScheduleEmptyCallback(
-    CancelableRequestConsumerBase* consumer,
-    EmptyHistoryCallback* callback) {
-  return Schedule(PRIORITY_UI, &HistoryBackend::ProcessEmptyRequest,
-                  consumer, new history::EmptyHistoryRequest(callback));
-}
-
 HistoryService::Handle HistoryService::QuerySegmentUsageSince(
     CancelableRequestConsumerBase* consumer,
     const Time from_time,
@@ -629,3 +636,9 @@ void HistoryService::BroadcastNotifications(
   NotificationService::current()->Notify(type, source, det);
 }
 
+void HistoryService::OnDBLoaded() {
+  backend_loaded_ = true;
+  NotificationService::current()->Notify(NOTIFY_HISTORY_LOADED,
+                                         Source<Profile>(profile_),
+                                         Details<HistoryService>(this));
+}
diff --git a/chrome/browser/history/history.h b/chrome/browser/history/history.h
index ec42335..74ef67b 100644
--- a/chrome/browser/history/history.h
+++ b/chrome/browser/history/history.h
@@ -25,7 +25,7 @@
 #include "chrome/common/page_transition_types.h"
 #include "chrome/common/ref_counted_util.h"
 
-class BookmarkBarModel;
+class BookmarkService;
 struct DownloadCreateInfo;
 class GURL;
 class HistoryURLProvider;
@@ -98,8 +98,12 @@ class HistoryService : public CancelableRequestProvider,
 
   // Initializes the history service, returning true on success. On false, do
   // not call any other functions. The given directory will be used for storing
-  // the history files.
-  bool Init(const std::wstring& history_dir);
+  // the history files. The BookmarkService is used when deleting URLs to
+  // test if a URL is bookmarked; it may be NULL during testing.
+  bool Init(const std::wstring& history_dir, BookmarkService* bookmark_service);
+
+  // Did the backend finish loading the databases?
+  bool backend_loaded() const { return backend_loaded_; }
 
   // Called on shutdown, this will tell the history backend to complete and
   // will release pointers to it. No other functions should be called once
@@ -487,6 +491,11 @@ class HistoryService : public CancelableRequestProvider,
       CancelableRequestConsumerBase* consumer,
       GetMostRecentKeywordSearchTermsCallback* callback);
 
+  // Bookmarks -----------------------------------------------------------------
+
+  // Notification that a URL is no longer bookmarked.
+  void URLsNoLongerBookmarked(const std::set<GURL>& urls);
+
   // Generic Stuff -------------------------------------------------------------
 
   typedef Callback0::Type HistoryDBTaskCallback;
@@ -496,13 +505,6 @@ class HistoryService : public CancelableRequestProvider,
   Handle ScheduleDBTask(HistoryDBTask* task,
                         CancelableRequestConsumerBase* consumer);
 
-  typedef Callback0::Type EmptyHistoryCallback;
-
-  // Schedules a task that does nothing on the backend. This can be used to get
-  // notification when then history backend is done processing requests.
-  Handle ScheduleEmptyCallback(CancelableRequestConsumerBase* consumer,
-                               EmptyHistoryCallback* callback);
-
   // Testing -------------------------------------------------------------------
 
   // Designed for unit tests, this passes the given task on to the history
@@ -536,6 +538,16 @@ class HistoryService : public CancelableRequestProvider,
  private:
   class BackendDelegate;
   friend class BackendDelegate;
+  friend class history::HistoryBackend;
+  friend class history::HistoryQueryTest;
+  friend class HistoryOperation;
+  friend class HistoryURLProvider;
+  friend class HistoryURLProviderTest;
+  template<typename Info, typename Callback> friend class DownloadRequest;
+  friend class PageUsageRequest;
+  friend class RedirectRequest;
+  friend class FavIconRequest;
+  friend class TestingProfile;
 
   // These are not currently used, hopefully we can do something in the future
   // to ensure that the most important things happen first.
@@ -545,17 +557,6 @@ class HistoryService : public CancelableRequestProvider,
     PRIORITY_LOW,     // Low priority things like indexing or expiration.
   };
 
-  friend class BookmarkBarModel;
-  friend class HistoryURLProvider;
-  friend class history::HistoryBackend;
-  template<typename Info, typename Callback> friend class DownloadRequest;
-  friend class PageUsageRequest;
-  friend class RedirectRequest;
-  friend class FavIconRequest;
-  friend class history::HistoryQueryTest;
-  friend class HistoryOperation;
-  friend class HistoryURLProviderTest;
-
   // Implementation of NotificationObserver.
   virtual void Observe(NotificationType type,
                        const NotificationSource& source,
@@ -577,6 +578,10 @@ class HistoryService : public CancelableRequestProvider,
   void BroadcastNotifications(NotificationType type,
                               history::HistoryDetails* details_deleted);
 
+  // Notification from the backend that it has finished loading. Sends
+  // notification (NOTIFY_HISTORY_LOADED) and sets backend_loaded_ to true.
+  void OnDBLoaded();
+
   // Returns true if this looks like the type of URL we want to add to the
   // history. We filter out some URLs such as JavaScript.
   bool CanAddURL(const GURL& url) const;
@@ -747,8 +752,11 @@ class HistoryService : public CancelableRequestProvider,
   // The profile, may be null when testing.
   Profile* profile_;
 
+  // Has the backend finished loading? The backend is loaded once Init has
+  // completed.
+  bool backend_loaded_;
+
   DISALLOW_EVIL_CONSTRUCTORS(HistoryService);
 };
 
 #endif  // CHROME_BROWSER_HISTORY_HISTORY_H__
-
diff --git a/chrome/browser/history/history_backend.cc b/chrome/browser/history/history_backend.cc
index 7682f6074..21f77fb 100644
--- a/chrome/browser/history/history_backend.cc
+++ b/chrome/browser/history/history_backend.cc
@@ -13,6 +13,7 @@
 #include "base/string_util.h"
 #include "base/time.h"
 #include "chrome/browser/autocomplete/history_url_provider.h"
+#include "chrome/browser/bookmarks/bookmark_service.h"
 #include "chrome/browser/history/download_types.h"
 #include "chrome/browser/history/in_memory_history_backend.h"
 #include "chrome/browser/history/page_usage_data.h"
@@ -25,8 +26,7 @@
 /* The HistoryBackend consists of a number of components:
 
     HistoryDatabase (stores past 3 months of history)
-      StarredURLDatabase (stores starred pages)
-        URLDatabase (stores a list of URLs)
+      URLDatabase (stores a list of URLs)
       DownloadDatabase (stores a list of downloads)
       VisitDatabase (stores a list of visits for the URLs)
       VisitSegmentDatabase (stores groups of URLs for the most visited view).
@@ -36,8 +36,7 @@
       DownloadDatabase (stores a list of downloads)
       VisitDatabase (stores a list of visits for the URLs)
 
-      (this does not store starred things or visit segments, all starred info
-      is stored in HistoryDatabase, and visit segments expire after 3 mos.)
+      (this does not store visit segments as they expire after 3 mos.)
 
     TextDatabaseManager (manages multiple text database for different times)
       TextDatabase (represents a single month of full-text index).
@@ -187,15 +186,17 @@ class HistoryBackend::URLQuerier {
 // HistoryBackend --------------------------------------------------------------
 
 HistoryBackend::HistoryBackend(const std::wstring& history_dir,
-                               Delegate* delegate)
+                               Delegate* delegate,
+                               BookmarkService* bookmark_service)
     : delegate_(delegate),
       history_dir_(history_dir),
 #pragma warning(suppress: 4355)  // OK to pass "this" here.
-      expirer_(this),
+      expirer_(this, bookmark_service),
       backend_destroy_message_loop_(NULL),
       recent_redirects_(kMaxRedirectCount),
       backend_destroy_task_(NULL),
-      segment_queried_(false) {
+      segment_queried_(false),
+      bookmark_service_(bookmark_service) {
 }
 
 HistoryBackend::~HistoryBackend() {
@@ -229,103 +230,8 @@ HistoryBackend::~HistoryBackend() {
 }
 
 void HistoryBackend::Init() {
-  DCHECK(!db_.get()) << "Initializing HistoryBackend twice";
-  // In the rare case where the db fails to initialize a dialog may get shown
-  // the blocks the caller, yet allows other messages through. For this reason
-  // we only set db_ to the created database if creation is successful. That
-  // way other methods won't do anything as db_ is still NULL.
-
-  TimeTicks beginning_time = TimeTicks::Now();
-
-  // Compute the file names. Note that the index file can be removed when the
-  // text db manager is finished being hooked up.
-  std::wstring history_name = history_dir_;
-  file_util::AppendToPath(&history_name, chrome::kHistoryFilename);
-  std::wstring thumbnail_name = GetThumbnailFileName();
-  std::wstring archived_name = GetArchivedFileName();
-  std::wstring tmp_bookmarks_file = history_dir_;
-  file_util::AppendToPath(&tmp_bookmarks_file,
-                          chrome::kHistoryBookmarksFileName);
-
-  // History database.
-  db_.reset(new HistoryDatabase());
-  switch (db_->Init(history_name, tmp_bookmarks_file)) {
-    case INIT_OK:
-      break;
-    case INIT_FAILURE:
-      // A NULL db_ will cause all calls on this object to notice this error
-      // and to not continue.
-      LOG(WARNING) << "Unable to initialize history DB.";
-      db_.reset();
-      return;
-    case INIT_TOO_NEW:
-      delegate_->NotifyTooNew();
-      db_.reset();
-      return;
-    default:
-      NOTREACHED();
-  }
-
-  // Fill the in-memory database and send it back to the history service on the
-  // main thread.
-  InMemoryHistoryBackend* mem_backend = new InMemoryHistoryBackend;
-  if (mem_backend->Init(history_name))
-    delegate_->SetInMemoryBackend(mem_backend);  // Takes ownership of pointer.
-  else
-    delete mem_backend;  // Error case, run without the in-memory DB.
-  db_->BeginExclusiveMode();  // Must be after the mem backend read the data.
-
-  // Full-text database. This has to be first so we can pass it to the
-  // HistoryDatabase for migration.
-  text_database_.reset(new TextDatabaseManager(history_dir_, db_.get()));
-  if (!text_database_->Init()) {
-    LOG(WARNING) << "Text database initialization failed, running without it.";
-    text_database_.reset();
-  }
-
-  // Thumbnail database.
-  thumbnail_db_.reset(new ThumbnailDatabase());
-  if (thumbnail_db_->Init(thumbnail_name) != INIT_OK) {
-    // Unlike the main database, we don't error out when the database is too
-    // new because this error is much less severe. Generally, this shouldn't
-    // happen since the thumbnail and main datbase versions should be in sync.
-    // We'll just continue without thumbnails & favicons in this case or any
-    // other error.
-    LOG(WARNING) << "Could not initialize the thumbnail database.";
-    thumbnail_db_.reset();
-  }
-
-  // Archived database.
-  archived_db_.reset(new ArchivedDatabase());
-  if (!archived_db_->Init(archived_name)) {
-    LOG(WARNING) << "Could not initialize the archived database.";
-    archived_db_.reset();
-  }
-
-  // Tell the expiration module about all the nice databases we made. This must
-  // happen before db_->Init() is called since the callback ForceArchiveHistory
-  // may need to expire stuff.
-  //
-  // *sigh*, this can all be cleaned up when that migration code is removed.
-  // The main DB initialization should intuitively be first (not that it
-  // actually matters) and the expirer should be set last.
-  expirer_.SetDatabases(db_.get(), archived_db_.get(),
-                        thumbnail_db_.get(), text_database_.get());
-
-  // Open the long-running transaction.
-  db_->BeginTransaction();
-  if (thumbnail_db_.get())
-    thumbnail_db_->BeginTransaction();
-  if (archived_db_.get())
-    archived_db_->BeginTransaction();
-  if (text_database_.get())
-    text_database_->BeginTransaction();
-
-  // Start expiring old stuff.
-  expirer_.StartArchivingOldStuff(TimeDelta::FromDays(kArchiveDaysThreshold));
-
-  HISTOGRAM_TIMES(L"History.InitTime",
-                  TimeTicks::Now() - beginning_time);
+  InitImpl();
+  delegate_->DBLoaded();
 }
 
 void HistoryBackend::SetOnBackendDestroyTask(MessageLoop* message_loop,
@@ -574,6 +480,106 @@ void HistoryBackend::AddPage(scoped_refptr<HistoryAddPageArgs> request) {
   ScheduleCommit();
 }
 
+void HistoryBackend::InitImpl() {
+  DCHECK(!db_.get()) << "Initializing HistoryBackend twice";
+  // In the rare case where the db fails to initialize a dialog may get shown
+  // the blocks the caller, yet allows other messages through. For this reason
+  // we only set db_ to the created database if creation is successful. That
+  // way other methods won't do anything as db_ is still NULL.
+
+  TimeTicks beginning_time = TimeTicks::Now();
+
+  // Compute the file names. Note that the index file can be removed when the
+  // text db manager is finished being hooked up.
+  std::wstring history_name = history_dir_;
+  file_util::AppendToPath(&history_name, chrome::kHistoryFilename);
+  std::wstring thumbnail_name = GetThumbnailFileName();
+  std::wstring archived_name = GetArchivedFileName();
+  std::wstring tmp_bookmarks_file = history_dir_;
+  file_util::AppendToPath(&tmp_bookmarks_file,
+                          chrome::kHistoryBookmarksFileName);
+
+  // History database.
+  db_.reset(new HistoryDatabase());
+  switch (db_->Init(history_name, tmp_bookmarks_file)) {
+    case INIT_OK:
+      break;
+    case INIT_FAILURE:
+      // A NULL db_ will cause all calls on this object to notice this error
+      // and to not continue.
+      LOG(WARNING) << "Unable to initialize history DB.";
+      db_.reset();
+      return;
+    case INIT_TOO_NEW:
+      delegate_->NotifyTooNew();
+      db_.reset();
+      return;
+    default:
+      NOTREACHED();
+  }
+
+  // Fill the in-memory database and send it back to the history service on the
+  // main thread.
+  InMemoryHistoryBackend* mem_backend = new InMemoryHistoryBackend;
+  if (mem_backend->Init(history_name))
+    delegate_->SetInMemoryBackend(mem_backend);  // Takes ownership of pointer.
+  else
+    delete mem_backend;  // Error case, run without the in-memory DB.
+  db_->BeginExclusiveMode();  // Must be after the mem backend read the data.
+
+  // Full-text database. This has to be first so we can pass it to the
+  // HistoryDatabase for migration.
+  text_database_.reset(new TextDatabaseManager(history_dir_, db_.get()));
+  if (!text_database_->Init()) {
+    LOG(WARNING) << "Text database initialization failed, running without it.";
+    text_database_.reset();
+  }
+
+  // Thumbnail database.
+  thumbnail_db_.reset(new ThumbnailDatabase());
+  if (thumbnail_db_->Init(thumbnail_name) != INIT_OK) {
+    // Unlike the main database, we don't error out when the database is too
+    // new because this error is much less severe. Generally, this shouldn't
+    // happen since the thumbnail and main datbase versions should be in sync.
+    // We'll just continue without thumbnails & favicons in this case or any
+    // other error.
+    LOG(WARNING) << "Could not initialize the thumbnail database.";
+    thumbnail_db_.reset();
+  }
+
+  // Archived database.
+  archived_db_.reset(new ArchivedDatabase());
+  if (!archived_db_->Init(archived_name)) {
+    LOG(WARNING) << "Could not initialize the archived database.";
+    archived_db_.reset();
+  }
+
+  // Tell the expiration module about all the nice databases we made. This must
+  // happen before db_->Init() is called since the callback ForceArchiveHistory
+  // may need to expire stuff.
+  //
+  // *sigh*, this can all be cleaned up when that migration code is removed.
+  // The main DB initialization should intuitively be first (not that it
+  // actually matters) and the expirer should be set last.
+  expirer_.SetDatabases(db_.get(), archived_db_.get(),
+                        thumbnail_db_.get(), text_database_.get());
+
+  // Open the long-running transaction.
+  db_->BeginTransaction();
+  if (thumbnail_db_.get())
+    thumbnail_db_->BeginTransaction();
+  if (archived_db_.get())
+    archived_db_->BeginTransaction();
+  if (text_database_.get())
+    text_database_->BeginTransaction();
+
+  // Start expiring old stuff.
+  expirer_.StartArchivingOldStuff(TimeDelta::FromDays(kArchiveDaysThreshold));
+
+  HISTOGRAM_TIMES(L"History.InitTime",
+                  TimeTicks::Now() - beginning_time);
+}
+
 std::pair<URLID, VisitID> HistoryBackend::AddPageVisit(
     const GURL& url,
     Time time,
@@ -1052,8 +1058,7 @@ void HistoryBackend::QueryHistoryFTS(const std::wstring& text_query,
 
   URLQuerier querier(db_.get(), archived_db_.get(), true);
 
-  // Now get the row and visit information for each one, optionally
-  // filtering on starred items.
+  // Now get the row and visit information for each one.
   URLResult url_result;  // Declare outside loop to prevent re-construction.
   for (size_t i = 0; i < fts_matches.size(); i++) {
     if (options.max_count != 0 &&
@@ -1291,7 +1296,7 @@ void HistoryBackend::SetImportedFavicons(
 
   Time now = Time::Now();
 
-  // Track all starred URLs that had their favicons set or updated.
+  // Track all URLs that had their favicons set or updated.
   std::set<GURL> favicons_changed;
 
   for (size_t i = 0; i < favicon_usage.size(); i++) {
@@ -1320,7 +1325,7 @@ void HistoryBackend::SetImportedFavicons(
   }
 
   if (!favicons_changed.empty()) {
-    // Send the notification about the changed favicons for starred URLs.
+    // Send the notification about the changed favicon URLs.
     FavIconChangeDetails* changed_details = new FavIconChangeDetails;
     changed_details->urls.swap(favicons_changed);
     BroadcastNotifications(NOTIFY_FAVICON_CHANGED, changed_details);
@@ -1602,6 +1607,23 @@ void HistoryBackend::ExpireHistoryBetween(
   request->ForwardResult(ExpireHistoryRequest::TupleType());
 }
 
+void HistoryBackend::URLsNoLongerBookmarked(const std::set<GURL>& urls) {
+  if (!db_.get())
+    return;
+
+  for (std::set<GURL>::const_iterator i = urls.begin(); i != urls.end(); ++i) {
+    URLRow url_row;
+    if (!db_->GetRowForURL(*i, &url_row))
+      continue;  // The URL isn't in the db; nothing to do.
+
+    VisitVector visits;
+    db_->GetVisitsForURL(url_row.id(), &visits);
+
+    if (visits.empty())
+      expirer_.DeleteURL(*i);  // There are no more visits; nuke the URL.
+  }
+}
+
 void HistoryBackend::ProcessDBTask(
     scoped_refptr<HistoryDBTaskRequest> request) {
   DCHECK(request.get());
@@ -1619,11 +1641,6 @@ void HistoryBackend::ProcessDBTask(
   }
 }
 
-void HistoryBackend::ProcessEmptyRequest(
-    scoped_refptr<EmptyHistoryRequest> request) {
-  request->ForwardResult(EmptyHistoryRequest::TupleType());
-}
-
 void HistoryBackend::BroadcastNotifications(
     NotificationType type,
     HistoryDetails* details_deleted) {
@@ -1645,29 +1662,23 @@ void HistoryBackend::DeleteAllHistory() {
   // Since we are likely to have very few bookmarks and their dependencies
   // compared to all history, this is also much faster than just deleting from
   // the original tables directly.
-  //
-  // TODO(brettw): bug 989802: When we store bookmarks in a separate file, this
-  // function can be simplified to having all the database objects close their
-  // connections and we just delete the files.
 
-  // Get starred entries and their corresponding URL rows.
-  std::vector<StarredEntry> starred_entries;
+  // Get the bookmarked URLs.
+  std::vector<GURL> starred_urls;
+  BookmarkService* bookmark_service = GetBookmarkService();
+  if (bookmark_service)
+    bookmark_service_->GetBookmarks(&starred_urls);
 
   std::vector<URLRow> kept_urls;
-  for (size_t i = 0; i < starred_entries.size(); i++) {
-    if (starred_entries[i].type != StarredEntry::URL)
-      continue;
-
+  for (size_t i = 0; i < starred_urls.size(); i++) {
     URLRow row;
-    if (!db_->GetURLRow(starred_entries[i].url_id, &row))
+    if (!db_->GetRowForURL(starred_urls[i], &row))
       continue;
 
     // Clear the last visit time so when we write these rows they are "clean."
-    // We keep the typed and visit counts. Since the kept URLs are bookmarks,
-    // we can assume that the user isn't trying to hide that they like them,
-    // and we can use these counts for giving better autocomplete suggestions.
     row.set_last_visit(Time());
-
+    row.set_visit_count(0);
+    row.set_typed_count(0);
     kept_urls.push_back(row);
   }
 
@@ -1681,7 +1692,7 @@ void HistoryBackend::DeleteAllHistory() {
 
   // ClearAllMainHistory will change the IDs of the URLs in kept_urls. Therfore,
   // we clear the list afterwards to make sure nobody uses this invalid data.
-  if (!ClearAllMainHistory(&starred_entries, kept_urls))
+  if (!ClearAllMainHistory(kept_urls))
     LOG(ERROR) << "Main history could not be cleared";
   kept_urls.clear();
 
@@ -1775,7 +1786,6 @@ bool HistoryBackend::ClearAllThumbnailHistory(
 }
 
 bool HistoryBackend::ClearAllMainHistory(
-    std::vector<StarredEntry>* starred_entries,
     const std::vector<URLRow>& kept_urls) {
   // Create the duplicate URL table. We will copy the kept URLs into this.
   if (!db_->CreateTemporaryURLTable())
@@ -1797,7 +1807,7 @@ bool HistoryBackend::ClearAllMainHistory(
     return false;
 
   // Delete the old tables and recreate them empty.
-  db_->RecreateAllButStarAndURLTables();
+  db_->RecreateAllTablesButURL();
 
   // Vacuum to reclaim the space from the dropped tables. This must be done
   // when there is no transaction open, and we assume that our long-running
@@ -1808,5 +1818,11 @@ bool HistoryBackend::ClearAllMainHistory(
   return true;
 }
 
+BookmarkService* HistoryBackend::GetBookmarkService() {
+  if (bookmark_service_)
+    bookmark_service_->BlockTillLoaded();
+  return bookmark_service_;
+}
+
 }  // namespace history
 
diff --git a/chrome/browser/history/history_backend.h b/chrome/browser/history/history_backend.h
index 1143cef..dd37c3e 100644
--- a/chrome/browser/history/history_backend.h
+++ b/chrome/browser/history/history_backend.h
@@ -25,6 +25,7 @@
 #include "chrome/common/scoped_vector.h"
 #include "testing/gtest/include/gtest/gtest_prod.h"
 
+class BookmarkService;
 struct ThumbnailScore;
 
 namespace history {
@@ -75,6 +76,9 @@ class HistoryBackend : public base::RefCountedThreadSafe<HistoryBackend>,
     // Ownership of the HistoryDetails is transferred to this function.
     virtual void BroadcastNotifications(NotificationType type,
                                         HistoryDetails* details) = 0;
+
+    // Invoked when the backend has finished loading the db.
+    virtual void DBLoaded() = 0;
   };
 
   // Init must be called to complete object creation. This object can be
@@ -85,8 +89,13 @@ class HistoryBackend : public base::RefCountedThreadSafe<HistoryBackend>,
   // See the definition of BroadcastNotificationsCallback above. This function
   // takes ownership of the callback pointer.
   //
+  // |bookmark_service| is used to determine bookmarked URLs when deleting and
+  // may be NULL.
+  //
   // This constructor is fast and does no I/O, so can be called at any time.
-  HistoryBackend(const std::wstring& history_dir, Delegate* delegate);
+  HistoryBackend(const std::wstring& history_dir,
+                 Delegate* delegate,
+                 BookmarkService* bookmark_service);
 
   ~HistoryBackend();
 
@@ -218,8 +227,6 @@ class HistoryBackend : public base::RefCountedThreadSafe<HistoryBackend>,
 
   void ProcessDBTask(scoped_refptr<HistoryDBTaskRequest> request);
 
-  void ProcessEmptyRequest(scoped_refptr<EmptyHistoryRequest> request);
-
   // Deleting ------------------------------------------------------------------
 
   void DeleteURL(const GURL& url);
@@ -229,6 +236,12 @@ class HistoryBackend : public base::RefCountedThreadSafe<HistoryBackend>,
                             Time begin_time,
                             Time end_time);
 
+  // Bookmarks -----------------------------------------------------------------
+
+  // Notification that a URL is no longer bookmarked. If there are no visits
+  // for the specified url, it is deleted.
+  void URLsNoLongerBookmarked(const std::set<GURL>& urls);
+
   // Testing -------------------------------------------------------------------
 
   // Sets the task to run and the message loop to run it on when this object
@@ -244,6 +257,8 @@ class HistoryBackend : public base::RefCountedThreadSafe<HistoryBackend>,
   friend class CommitLaterTask;  // The commit task needs to call Commit().
   friend class HistoryTest;  // So the unit tests can poke our innards.
   FRIEND_TEST(HistoryBackendTest, DeleteAll);
+  FRIEND_TEST(HistoryBackendTest, URLsNoLongerBookmarked);
+  friend class TestingProfile;
 
   // For invoking methods that circumvent requests.
   friend class HistoryTest;
@@ -255,6 +270,9 @@ class HistoryBackend : public base::RefCountedThreadSafe<HistoryBackend>,
   class URLQuerier;
   friend class URLQuerier;
 
+  // Does the work of Init.
+  void InitImpl();
+
   // Adds a single visit to the database, updating the URL information such
   // as visit and typed count. The visit ID of the added visit and the URL ID
   // of the associated URL (whether added or not) is returned. Both values will
@@ -374,15 +392,16 @@ class HistoryBackend : public base::RefCountedThreadSafe<HistoryBackend>,
   // vector to reference the new IDs.
   bool ClearAllThumbnailHistory(std::vector<URLRow>* kept_urls);
 
-  // Deletes all information in the history database, except the star table
-  // (all entries should be in the given vector) and the given URLs in the URL
-  // table (these should correspond to the bookmarked URLs).
+  // Deletes all information in the history database, except for the supplied
+  // set of URLs in the URL table (these should correspond to the bookmarked
+  // URLs).
   //
-  // The IDs of the URLs may change, and the starred table will be updated
-  // accordingly. This function will also update the |starred_entries| input
-  // vector.
-  bool ClearAllMainHistory(std::vector<StarredEntry>* starred_entries,
-                           const std::vector<URLRow>& kept_urls);
+  // The IDs of the URLs may change.
+  bool ClearAllMainHistory(const std::vector<URLRow>& kept_urls);
+
+  // Returns the BookmarkService, blocking until it is loaded. This may return
+  // NULL during testing.
+  BookmarkService* GetBookmarkService();
 
   // Data ----------------------------------------------------------------------
 
@@ -457,10 +476,16 @@ class HistoryBackend : public base::RefCountedThreadSafe<HistoryBackend>,
   // done.
   std::list<HistoryDBTaskRequest*> db_task_requests_;
 
+  // Used to determine if a URL is bookmarked. This is owned by the Profile and
+  // may be NULL (during testing).
+  //
+  // Use GetBookmarkService to access this, which makes sure the service is
+  // loaded.
+  BookmarkService* bookmark_service_;
+
   DISALLOW_EVIL_CONSTRUCTORS(HistoryBackend);
 };
 
 }  // namespace history
 
 #endif  // CHROME_BROWSER_HISTORY_HISTORY_BACKEND_H__
-
diff --git a/chrome/browser/history/history_backend_unittest.cc b/chrome/browser/history/history_backend_unittest.cc
index baf5f0e..4067440 100644
--- a/chrome/browser/history/history_backend_unittest.cc
+++ b/chrome/browser/history/history_backend_unittest.cc
@@ -5,6 +5,7 @@
 #include "base/file_util.h"
 #include "base/path_service.h"
 #include "base/scoped_ptr.h"
+#include "chrome/browser/bookmark_bar_model.h"
 #include "chrome/browser/history/history_backend.h"
 #include "chrome/browser/history/in_memory_history_backend.h"
 #include "chrome/browser/history/in_memory_database.h"
@@ -34,6 +35,7 @@ class HistoryBackendTestDelegate : public HistoryBackend::Delegate {
   virtual void SetInMemoryBackend(InMemoryHistoryBackend* backend);
   virtual void BroadcastNotifications(NotificationType type,
                                       HistoryDetails* details);
+  virtual void DBLoaded();
 
  private:
   // Not owned by us.
@@ -44,6 +46,7 @@ class HistoryBackendTestDelegate : public HistoryBackend::Delegate {
 
 class HistoryBackendTest : public testing::Test {
  public:
+  HistoryBackendTest() : bookmark_model_(NULL), loaded_(false) {}
   virtual ~HistoryBackendTest() {
   }
 
@@ -66,6 +69,11 @@ class HistoryBackendTest : public testing::Test {
     backend_->AddPage(request);
   }
 
+  BookmarkBarModel bookmark_model_;
+
+ protected:
+  bool loaded_;
+
  private:
   friend HistoryBackendTestDelegate;
 
@@ -74,7 +82,8 @@ class HistoryBackendTest : public testing::Test {
     if (!file_util::CreateNewTempDirectory(L"BackendTest", &test_dir_))
       return;
     backend_ = new HistoryBackend(test_dir_,
-                                  new HistoryBackendTestDelegate(this));
+                                  new HistoryBackendTestDelegate(this),
+                                  &bookmark_model_);
     backend_->Init();
   }
   virtual void TearDown() {
@@ -113,6 +122,14 @@ void HistoryBackendTestDelegate::BroadcastNotifications(
   test_->BroadcastNotifications(type, details);
 }
 
+void HistoryBackendTestDelegate::DBLoaded() {
+  test_->loaded_ = true;
+}
+
+TEST_F(HistoryBackendTest, Loaded) {
+  ASSERT_TRUE(backend_.get());
+  ASSERT_TRUE(loaded_);
+}
 
 TEST_F(HistoryBackendTest, DeleteAll) {
   ASSERT_TRUE(backend_.get());
@@ -181,6 +198,10 @@ TEST_F(HistoryBackendTest, DeleteAll) {
      JPEGCodec::Decode(kWeewarThumbnail, sizeof(kWeewarThumbnail)));
   backend_->thumbnail_db_->SetPageThumbnail(row2_id, *weewar_bitmap, score);
 
+  // Star row1.
+  bookmark_model_.AddURL(
+      bookmark_model_.GetBookmarkBarNode(), 0, std::wstring(), row1.url());
+
   // Set full text index for each one.
   backend_->text_database_->AddPageData(row1.url(), row1_id, visit1_id,
                                         row1.last_visit(),
@@ -192,8 +213,11 @@ TEST_F(HistoryBackendTest, DeleteAll) {
   // Now finally clear all history.
   backend_->DeleteAllHistory();
 
-  // The first URL should be deleted.
-  EXPECT_FALSE(backend_->db_->GetRowForURL(row1.url(), &outrow1));
+  // The first URL should be preserved but the time should be cleared.
+  EXPECT_TRUE(backend_->db_->GetRowForURL(row1.url(), &outrow1));
+  EXPECT_EQ(0, outrow1.visit_count());
+  EXPECT_EQ(0, outrow1.typed_count());
+  EXPECT_TRUE(Time() == outrow1.last_visit());
 
   // The second row should be deleted.
   URLRow outrow2;
@@ -210,15 +234,22 @@ TEST_F(HistoryBackendTest, DeleteAll) {
                                                          &out_data));
   EXPECT_FALSE(backend_->thumbnail_db_->GetPageThumbnail(row2_id, &out_data));
 
-  // Make sure the favicons were deleted.
-  // TODO(sky): would be nice if this didn't happen.
+  // We should have a favicon for the first URL only. We look them up by favicon
+  // URL since the IDs may hav changed.
   FavIconID out_favicon1 = backend_->thumbnail_db_->
       GetFavIconIDForFavIconURL(favicon_url1);
-  EXPECT_FALSE(out_favicon1);
+  EXPECT_TRUE(out_favicon1);
   FavIconID out_favicon2 = backend_->thumbnail_db_->
       GetFavIconIDForFavIconURL(favicon_url2);
   EXPECT_FALSE(out_favicon2) << "Favicon not deleted";
 
+  // The remaining URL should still reference the same favicon, even if its
+  // ID has changed.
+  EXPECT_EQ(out_favicon1, outrow1.favicon_id());
+
+  // The first URL should still be bookmarked.
+  EXPECT_TRUE(bookmark_model_.IsBookmarked(row1.url()));
+
   // The full text database should have no data.
   std::vector<TextDatabase::Match> text_matches;
   Time first_time_searched;
@@ -228,6 +259,94 @@ TEST_F(HistoryBackendTest, DeleteAll) {
   EXPECT_EQ(0, text_matches.size());
 }
 
+TEST_F(HistoryBackendTest, URLsNoLongerBookmarked) {
+  GURL favicon_url1("http://www.google.com/favicon.ico");
+  GURL favicon_url2("http://news.google.com/favicon.ico");
+  FavIconID favicon2 = backend_->thumbnail_db_->AddFavIcon(favicon_url2);
+  FavIconID favicon1 = backend_->thumbnail_db_->AddFavIcon(favicon_url1);
+
+  std::vector<unsigned char> data;
+  data.push_back('1');
+  EXPECT_TRUE(backend_->thumbnail_db_->SetFavIcon(
+                  favicon1, data, Time::Now()));
+
+  data[0] = '2';
+  EXPECT_TRUE(backend_->thumbnail_db_->SetFavIcon(
+                  favicon2, data, Time::Now()));
+
+  // First visit two URLs.
+  URLRow row1(GURL("http://www.google.com/"));
+  row1.set_visit_count(2);
+  row1.set_typed_count(1);
+  row1.set_last_visit(Time::Now());
+  row1.set_favicon_id(favicon1);
+
+  URLRow row2(GURL("http://news.google.com/"));
+  row2.set_visit_count(1);
+  row2.set_last_visit(Time::Now());
+  row2.set_favicon_id(favicon2);
+
+  std::vector<URLRow> rows;
+  rows.push_back(row2);  // Reversed order for the same reason as favicons.
+  rows.push_back(row1);
+  backend_->AddPagesWithDetails(rows);
+
+  URLID row1_id = backend_->db_->GetRowForURL(row1.url(), NULL);
+  URLID row2_id = backend_->db_->GetRowForURL(row2.url(), NULL);
+
+  // Star the two URLs.
+  bookmark_model_.SetURLStarred(row1.url(), std::wstring(), true);
+  bookmark_model_.SetURLStarred(row2.url(), std::wstring(), true);
+
+  // Delete url 2. Because url 2 is starred this won't delete the URL, only
+  // the visits.
+  backend_->expirer_.DeleteURL(row2.url());
+
+  // Make sure url 2 is still valid, but has no visits.
+  URLRow tmp_url_row;
+  EXPECT_EQ(row2_id, backend_->db_->GetRowForURL(row2.url(), NULL));
+  VisitVector visits;
+  backend_->db_->GetVisitsForURL(row2_id, &visits);
+  EXPECT_EQ(0, visits.size());
+  // The favicon should still be valid.
+  EXPECT_EQ(favicon2,
+      backend_->thumbnail_db_->GetFavIconIDForFavIconURL(favicon_url2));
+
+  // Unstar row2.
+  bookmark_model_.SetURLStarred(row2.url(), std::wstring(), false);
+  // Tell the backend it was unstarred. We have to explicitly do this as
+  // BookmarkBarModel isn't wired up to the backend during testing.
+  std::set<GURL> unstarred_urls;
+  unstarred_urls.insert(row2.url());
+  backend_->URLsNoLongerBookmarked(unstarred_urls);
+
+  // The URL should no longer exist.
+  EXPECT_FALSE(backend_->db_->GetRowForURL(row2.url(), &tmp_url_row));
+  // And the favicon should be deleted.
+  EXPECT_EQ(0,
+      backend_->thumbnail_db_->GetFavIconIDForFavIconURL(favicon_url2));
+
+  // Unstar row 1.
+  bookmark_model_.SetURLStarred(row1.url(), std::wstring(), false);
+  // Tell the backend it was unstarred. We have to explicitly do this as
+  // BookmarkBarModel isn't wired up to the backend during testing.
+  unstarred_urls.clear();
+  unstarred_urls.insert(row1.url());
+  backend_->URLsNoLongerBookmarked(unstarred_urls);
+
+  // The URL should still exist (because there were visits).
+  EXPECT_EQ(row1_id, backend_->db_->GetRowForURL(row1.url(), NULL));
+
+  // There should still be visits.
+  visits.clear();
+  backend_->db_->GetVisitsForURL(row1_id, &visits);
+  EXPECT_EQ(1, visits.size());
+
+  // The favicon should still be valid.
+  EXPECT_EQ(favicon1,
+      backend_->thumbnail_db_->GetFavIconIDForFavIconURL(favicon_url1));
+}
+
 TEST_F(HistoryBackendTest, GetPageThumbnailAfterRedirects) {
   ASSERT_TRUE(backend_.get());
 
diff --git a/chrome/browser/history/history_database.cc b/chrome/browser/history/history_database.cc
index c33dee8..cf49883 100644
--- a/chrome/browser/history/history_database.cc
+++ b/chrome/browser/history/history_database.cc
@@ -127,7 +127,7 @@ void HistoryDatabase::CommitTransaction() {
   }
 }
 
-bool HistoryDatabase::RecreateAllButStarAndURLTables() {
+bool HistoryDatabase::RecreateAllTablesButURL() {
   if (!DropVisitTable())
     return false;
   if (!InitVisitTable())
@@ -143,7 +143,7 @@ bool HistoryDatabase::RecreateAllButStarAndURLTables() {
   if (!InitSegmentTables())
     return false;
 
-  // We also add the supplimentary URL indices at this point. This index is
+  // We also add the supplementary URL indices at this point. This index is
   // over parts of the URL table that weren't automatically created when the
   // temporary URL table was
   CreateSupplimentaryURLIndices();
diff --git a/chrome/browser/history/history_database.h b/chrome/browser/history/history_database.h
index edde249..c69150b 100644
--- a/chrome/browser/history/history_database.h
+++ b/chrome/browser/history/history_database.h
@@ -88,8 +88,8 @@ class HistoryDatabase : public DownloadDatabase,
     return transaction_nesting_;
   }
 
-  // Drops all tables except the URL, starred and download tables, and recreates
-  // them from scratch. This is done to rapidly clean up stuff when deleting all
+  // Drops all tables except the URL, and download tables, and recreates them
+  // from scratch. This is done to rapidly clean up stuff when deleting all
   // history. It is faster and less likely to have problems that deleting all
   // rows in the tables.
   //
@@ -102,10 +102,10 @@ class HistoryDatabase : public DownloadDatabase,
   // This should be treated the same as an init failure, and the database
   // should not be used any more.
   //
-  // This will also recreate the supplimentary URL indices, since these
+  // This will also recreate the supplementary URL indices, since these
   // indices won't be created automatically when using the temporary URL
-  // talbe (what the caller does right before calling this).
-  bool RecreateAllButStarAndURLTables();
+  // table (what the caller does right before calling this).
+  bool RecreateAllTablesButURL();
 
   // Vacuums the database. This will cause sqlite to defragment and collect
   // unused space in the file. It can be VERY SLOW.
diff --git a/chrome/browser/history/history_marshaling.h b/chrome/browser/history/history_marshaling.h
index 146e6d4..fa02095 100644
--- a/chrome/browser/history/history_marshaling.h
+++ b/chrome/browser/history/history_marshaling.h
@@ -117,9 +117,6 @@ typedef CancelableRequest1<HistoryService::HistoryDBTaskCallback,
                            scoped_refptr<HistoryDBTask> >
     HistoryDBTaskRequest;
 
-typedef CancelableRequest<HistoryService::EmptyHistoryCallback>
-    EmptyHistoryRequest;
-
 }  // namespace history
 
 #endif  // CHROME_BROWSER_HISTORY_HISTORY_MARSHALING_H__
diff --git a/chrome/browser/history/history_querying_unittest.cc b/chrome/browser/history/history_querying_unittest.cc
index c62fc40..625bc5b 100644
--- a/chrome/browser/history/history_querying_unittest.cc
+++ b/chrome/browser/history/history_querying_unittest.cc
@@ -86,7 +86,7 @@ class HistoryQueryTest : public testing::Test {
     file_util::CreateDirectory(history_dir_);
 
     history_ = new HistoryService;
-    if (!history_->Init(history_dir_)) {
+    if (!history_->Init(history_dir_, NULL)) {
       history_ = NULL;  // Tests should notice this NULL ptr & fail.
       return;
     }
diff --git a/chrome/browser/history/history_unittest.cc b/chrome/browser/history/history_unittest.cc
index 29f674e..c2ec919 100644
--- a/chrome/browser/history/history_unittest.cc
+++ b/chrome/browser/history/history_unittest.cc
@@ -90,6 +90,7 @@ class BackendDelegate : public HistoryBackend::Delegate {
   virtual void SetInMemoryBackend(InMemoryHistoryBackend* backend);
   virtual void BroadcastNotifications(NotificationType type,
                                       HistoryDetails* details);
+  virtual void DBLoaded() {}
 
  private:
   HistoryTest* history_test_;
@@ -122,7 +123,8 @@ class HistoryTest : public testing::Test {
   // Creates the HistoryBackend and HistoryDatabase on the current thread,
   // assigning the values to backend_ and db_.
   void CreateBackendAndDatabase() {
-    backend_ = new HistoryBackend(history_dir_, new BackendDelegate(this));
+    backend_ =
+        new HistoryBackend(history_dir_, new BackendDelegate(this), NULL);
     backend_->Init();
     db_ = backend_->db_.get();
     DCHECK(in_mem_backend_.get()) << "Mem backend should have been set by "
@@ -373,7 +375,7 @@ TEST_F(HistoryTest, ClearBrowsingData_Downloads) {
 TEST_F(HistoryTest, AddPage) {
   scoped_refptr<HistoryService> history(new HistoryService);
   history_service_ = history;
-  ASSERT_TRUE(history->Init(history_dir_));
+  ASSERT_TRUE(history->Init(history_dir_, NULL));
 
   // Add the page once from a child frame.
   const GURL test_url("http://www.google.com/");
@@ -397,7 +399,7 @@ TEST_F(HistoryTest, AddPage) {
 TEST_F(HistoryTest, AddPageSameTimes) {
   scoped_refptr<HistoryService> history(new HistoryService);
   history_service_ = history;
-  ASSERT_TRUE(history->Init(history_dir_));
+  ASSERT_TRUE(history->Init(history_dir_, NULL));
 
   Time now = Time::Now();
   const GURL test_urls[] = {
@@ -437,7 +439,7 @@ TEST_F(HistoryTest, AddPageSameTimes) {
 TEST_F(HistoryTest, AddRedirect) {
   scoped_refptr<HistoryService> history(new HistoryService);
   history_service_ = history;
-  ASSERT_TRUE(history->Init(history_dir_));
+  ASSERT_TRUE(history->Init(history_dir_, NULL));
 
   const wchar_t* first_sequence[] = {
     L"http://first.page/",
@@ -508,7 +510,7 @@ TEST_F(HistoryTest, AddRedirect) {
 TEST_F(HistoryTest, Typed) {
   scoped_refptr<HistoryService> history(new HistoryService);
   history_service_ = history;
-  ASSERT_TRUE(history->Init(history_dir_));
+  ASSERT_TRUE(history->Init(history_dir_, NULL));
 
   // Add the page once as typed.
   const GURL test_url("http://www.google.com/");
@@ -551,7 +553,7 @@ TEST_F(HistoryTest, Typed) {
 TEST_F(HistoryTest, SetTitle) {
   scoped_refptr<HistoryService> history(new HistoryService);
   history_service_ = history;
-  ASSERT_TRUE(history->Init(history_dir_));
+  ASSERT_TRUE(history->Init(history_dir_, NULL));
 
   // Add a URL.
   const GURL existing_url(L"http://www.google.com/");
@@ -582,7 +584,7 @@ TEST_F(HistoryTest, Segments) {
   scoped_refptr<HistoryService> history(new HistoryService);
   history_service_ = history;
 
-  ASSERT_TRUE(history->Init(history_dir_));
+  ASSERT_TRUE(history->Init(history_dir_, NULL));
 
   static const void* scope = static_cast<void*>(this);
 
@@ -648,7 +650,7 @@ TEST_F(HistoryTest, Segments) {
 TEST_F(HistoryTest, Thumbnails) {
   scoped_refptr<HistoryService> history(new HistoryService);
   history_service_ = history;
-  ASSERT_TRUE(history->Init(history_dir_));
+  ASSERT_TRUE(history->Init(history_dir_, NULL));
 
   scoped_ptr<SkBitmap> thumbnail(
       JPEGCodec::Decode(kGoogleThumbnail, sizeof(kGoogleThumbnail)));
@@ -798,7 +800,7 @@ class HistoryDBTaskImpl : public HistoryDBTask {
 TEST_F(HistoryTest, HistoryDBTask) {
   CancelableRequestConsumerT<int, 0> request_consumer;
   HistoryService* history = new HistoryService();
-  ASSERT_TRUE(history->Init(history_dir_));
+  ASSERT_TRUE(history->Init(history_dir_, NULL));
   scoped_refptr<HistoryDBTaskImpl> task(new HistoryDBTaskImpl());
   history_service_ = history;
   history->ScheduleDBTask(task.get(), &request_consumer);
@@ -816,7 +818,7 @@ TEST_F(HistoryTest, HistoryDBTask) {
 TEST_F(HistoryTest, HistoryDBTaskCanceled) {
   CancelableRequestConsumerT<int, 0> request_consumer;
   HistoryService* history = new HistoryService();
-  ASSERT_TRUE(history->Init(history_dir_));
+  ASSERT_TRUE(history->Init(history_dir_, NULL));
   scoped_refptr<HistoryDBTaskImpl> task(new HistoryDBTaskImpl());
   history_service_ = history;
   history->ScheduleDBTask(task.get(), &request_consumer);
diff --git a/chrome/browser/history/url_database.cc b/chrome/browser/history/url_database.cc
index 27a4994..aeb9ed2 100644
--- a/chrome/browser/history/url_database.cc
+++ b/chrome/browser/history/url_database.cc
@@ -224,8 +224,9 @@ bool URLDatabase::IsFavIconUsed(FavIconID favicon_id) {
 void URLDatabase::AutocompleteForPrefix(const std::wstring& prefix,
                                         size_t max_results,
                                         std::vector<history::URLRow>* results) {
-  // TODO(sky): this query should order by starred as the second order by
-  // clause.
+  // NOTE: this query originally sorted by starred as the second parameter. But
+  // as bookmarks is no longer part of the db we no longer include the order
+  // by clause.
   results->clear();
   SQLITE_UNIQUE_STATEMENT(statement, GetStatementCache(),
       "SELECT" HISTORY_URL_ROW_FIELDS "FROM urls "