Unpin deleted URLs + better deleting algorithm.

BUG=none
TEST=TopSitesTest

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

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

chrome/browser/history/top_sites.cc

@@ -532,6 +532,14 @@ void TopSites::StartQueryForThumbnail(size_t index) {
   cancelable_consumer_.SetClientData(hs, handle, index);
 }
 
+void TopSites::GenerateCanonicalURLs() {
+  canonical_urls_.clear();
+  for (size_t i = 0; i < top_sites_.size(); i++) {
+    const MostVisitedURL& mv = top_sites_[i];
+    StoreRedirectChain(mv.redirects, i);
+  }
+}
+
 void TopSites::StoreMostVisited(MostVisitedURLList* most_visited) {
   DCHECK(ChromeThread::CurrentlyOn(ChromeThread::DB));
   // Take ownership of the most visited data.
@@ -540,11 +548,10 @@ void TopSites::StoreMostVisited(MostVisitedURLList* most_visited) {
   waiting_for_results_ = false;
 
   // Save the redirect information for quickly mapping to the canonical URLs.
-  canonical_urls_.clear();
+  GenerateCanonicalURLs();
+
   for (size_t i = 0; i < top_sites_.size(); i++) {
     const MostVisitedURL& mv = top_sites_[i];
-    StoreRedirectChain(mv.redirects, i);
-
     std::map<GURL, Images>::iterator it = temp_thumbnails_map_.begin();
     GURL canonical_url = GetCanonicalURL(mv.url);
     for (; it != temp_thumbnails_map_.end(); it++) {
@@ -802,17 +809,24 @@ void TopSites::Observe(NotificationType type,
       ChromeThread::PostTask(ChromeThread::DB, FROM_HERE,
                              NewRunnableMethod(this, &TopSites::ResetDatabase));
     } else {
+      std::set<size_t> indices_to_delete;  // Indices into top_sites_.
       std::set<GURL>::iterator it;
       for (it = deleted_details->urls.begin();
            it != deleted_details->urls.end(); ++it) {
-        for (size_t i = 0; i < top_sites_.size(); i++) {
-          if (top_sites_[i].url == *it) {
-            top_sites_.erase(top_sites_.begin() + i);
-            break;
-          }
-        }
+        std::map<GURL,size_t>::const_iterator found = canonical_urls_.find(*it);
+        if (found != canonical_urls_.end())
+          indices_to_delete.insert(found->second);
+      }
+
+      for (std::set<size_t>::reverse_iterator i = indices_to_delete.rbegin();
+           i != indices_to_delete.rend(); i++) {
+        size_t index = *i;
+        RemovePinnedURL(top_sites_[index].url);
+        top_sites_.erase(top_sites_.begin() + index);
       }
     }
+    // Canonical URLs are not valid any more.
+    GenerateCanonicalURLs();
     StartQueryForMostVisited();
   } else if (type == NotificationType::NAV_ENTRY_COMMITTED) {
     if (top_sites_.size() < kTopSitesNumber) {

chrome/browser/history/top_sites.h

@@ -142,6 +142,7 @@ class TopSites : public NotificationObserver,
   FRIEND_TEST_ALL_PREFIXES(TopSitesTest, RealDatabase);
   FRIEND_TEST_ALL_PREFIXES(TopSitesTest, MockDatabase);
   FRIEND_TEST_ALL_PREFIXES(TopSitesTest, DeleteNotifications);
+  FRIEND_TEST_ALL_PREFIXES(TopSitesTest, PinnedURLsDeleted);
   FRIEND_TEST_ALL_PREFIXES(TopSitesTest, GetUpdateDelay);
   FRIEND_TEST_ALL_PREFIXES(TopSitesTest, Migration);
   FRIEND_TEST_ALL_PREFIXES(TopSitesTest, QueueingRequestsForTopSites);
@@ -183,6 +184,9 @@ class TopSites : public NotificationObserver,
   void OnThumbnailAvailable(CancelableRequestProvider::Handle handle,
                             scoped_refptr<RefCountedBytes> thumbnail);
 
+  // Sets canonical_urls_ from top_sites_.
+  void GenerateCanonicalURLs();
+
   // Saves the set of the top URLs visited by this user. The 0th item is the
   // most popular.
   // DANGER! This will clear all data from the input argument.

chrome/browser/history/top_sites_unittest.cc

@@ -790,6 +790,80 @@ TEST_F(TopSitesTest, DeleteNotifications) {
   EXPECT_EQ(themes_url(), urls()[1].url);
 }
 
+TEST_F(TopSitesTest, PinnedURLsDeleted) {
+  ChromeThread db_loop(ChromeThread::DB, MessageLoop::current());
+  GURL google1_url("http://google.com");
+  GURL google2_url("http://google.com/redirect");
+  GURL google3_url("http://www.google.com");
+  string16 google_title(ASCIIToUTF16("Google"));
+  GURL news_url("http://news.google.com");
+  string16 news_title(ASCIIToUTF16("Google News"));
+
+  MockHistoryServiceImpl hs;
+
+  top_sites().Init(file_name());
+
+  hs.AppendMockPage(google1_url, google_title);
+  hs.AppendMockPage(news_url, news_title);
+  top_sites().SetMockHistoryService(&hs);
+
+  top_sites().StartQueryForMostVisited();
+  MessageLoop::current()->RunAllPending();
+  top_sites().GetMostVisitedURLs(
+      consumer(),
+      NewCallback(static_cast<TopSitesTest*>(this),
+                  &TopSitesTest::OnTopSitesAvailable));
+  MessageLoop::current()->RunAllPending();
+  EXPECT_EQ(1u, number_of_callbacks());
+  // 2 extra prepopulated URLs.
+  ASSERT_EQ(4u, urls().size());
+
+  top_sites().AddPinnedURL(news_url, 3);
+  EXPECT_TRUE(top_sites().IsURLPinned(news_url));
+
+  hs.RemoveMostVisitedURL();
+  history::URLsDeletedDetails history_details;
+  history_details.all_history = false;
+  history_details.urls.insert(news_url);
+  Details<URLsDeletedDetails> details(&history_details);
+  top_sites().Observe(NotificationType::HISTORY_URLS_DELETED,
+                      Source<Profile> (&profile()),
+                      details);
+  MessageLoop::current()->RunAllPending();
+  top_sites().GetMostVisitedURLs(
+      consumer(),
+      NewCallback(static_cast<TopSitesTest*>(this),
+                  &TopSitesTest::OnTopSitesAvailable));
+  MessageLoop::current()->RunAllPending();
+  EXPECT_EQ(2u, number_of_callbacks());
+  ASSERT_EQ(3u, urls().size());
+  EXPECT_FALSE(top_sites().IsURLPinned(news_url));
+
+  hs.RemoveMostVisitedURL();
+  history_details.all_history = true;
+  details = Details<HistoryDetails>(&history_details);
+  top_sites().Observe(NotificationType::HISTORY_URLS_DELETED,
+                      Source<Profile> (&profile()),
+                      details);
+  MessageLoop::current()->RunAllPending();
+  top_sites().GetMostVisitedURLs(
+      consumer(),
+      NewCallback(static_cast<TopSitesTest*>(this),
+                  &TopSitesTest::OnTopSitesAvailable));
+  ASSERT_EQ(2u, urls().size());
+  MessageLoop::current()->RunAllPending();
+
+  top_sites().StartQueryForMostVisited();
+  MessageLoop::current()->RunAllPending();
+  top_sites().GetMostVisitedURLs(
+      consumer(),
+      NewCallback(static_cast<TopSitesTest*>(this),
+                  &TopSitesTest::OnTopSitesAvailable));
+  ASSERT_EQ(2u, urls().size());
+  EXPECT_EQ(welcome_url(), urls()[0].url);
+  EXPECT_EQ(themes_url(), urls()[1].url);
+}
+
 TEST_F(TopSitesTest, GetUpdateDelay) {
   top_sites().last_num_urls_changed_ = 0;
   EXPECT_EQ(30, top_sites().GetUpdateDelay().InSeconds());