Adding TopSitesDatabase::UpdatePageRank to update rank without erasing the thumbnail.

Also, request the thumbnails for the unknown pages when updating the top sites list.

BUG=None
TEST=TopSitesTest

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

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

5 files changed, 223 insertions, 61 deletions

diff --git a/chrome/browser/history/top_sites.cc b/chrome/browser/history/top_sites.cc
index 10b998fa..49a2757 100644
--- a/chrome/browser/history/top_sites.cc
+++ b/chrome/browser/history/top_sites.cc
@@ -19,9 +19,10 @@
 namespace history {
 
 // How many top sites to store in the cache.
-static const int kTopSitesNumber = 20;
+static const size_t kTopSitesNumber = 20;
 static const int kDaysOfHistory = 90;
-static const int64 kUpdateIntervalSecs = 15;  // Time from startup to DB query.
+// Time from startup to first HistoryService query.
+static const int64 kUpdateIntervalSecs = 15;
 // Intervals between requests to HistoryService.
 static const int64 kMinUpdateIntervalMinutes = 1;
 static const int64 kMaxUpdateIntervalMinutes = 60;
@@ -32,6 +33,8 @@ TopSites::TopSites(Profile* profile) : profile_(profile),
                                        last_num_urls_changed_(0) {
   registrar_.Add(this, NotificationType::HISTORY_URLS_DELETED,
                  Source<Profile>(profile_));
+  registrar_.Add(this, NotificationType::NAV_ENTRY_COMMITTED,
+                 NotificationService::AllSources());
 }
 
 TopSites::~TopSites() {
@@ -67,10 +70,14 @@ void TopSites::ReadDatabase() {
   }  // Lock is released here.
 
   for (size_t i = 0; i < top_sites_.size(); i++) {
-    MostVisitedURL url = top_sites_[i];
-    Images thumbnail = thumbnails[url.url];
-    SetPageThumbnailNoDB(url.url, thumbnail.thumbnail,
-                         thumbnail.thumbnail_score);
+    GURL url = top_sites_[i].url;
+    Images thumbnail = thumbnails[url];
+    if (!thumbnail.thumbnail.get() || !thumbnail.thumbnail->size()) {
+      LOG(INFO) << "No thumnbail for " << url.spec();
+    } else {
+      SetPageThumbnailNoDB(url, thumbnail.thumbnail,
+                           thumbnail.thumbnail_score);
+    }
   }
 }
 
@@ -89,7 +96,14 @@ bool TopSites::SetPageThumbnail(const GURL& url,
       &thumbnail_data->data);
   if (!encoded)
     return false;
-  if (!SetPageThumbnailNoDB(url, thumbnail_data, score))
+
+  return SetPageThumbnail(url, thumbnail_data, score);
+}
+
+bool TopSites::SetPageThumbnail(const GURL& url,
+                                const RefCountedBytes* thumbnail,
+                                const ThumbnailScore& score) {
+  if (!SetPageThumbnailNoDB(url, thumbnail, score))
     return false;
 
   // Update the database.
@@ -191,21 +205,44 @@ void TopSites::UpdateMostVisited(MostVisitedURLList most_visited) {
     if (db_.get()) {
       // Write both added and moved urls.
       for (size_t i = 0; i < added.size(); i++) {
-        MostVisitedURL added_url = most_visited[added[i]];
+        MostVisitedURL& added_url = most_visited[added[i]];
         db_->SetPageThumbnail(added_url, added[i], Images());
       }
       for (size_t i = 0; i < moved.size(); i++) {
         MostVisitedURL moved_url = most_visited[moved[i]];
-        db_->SetPageThumbnail(moved_url, moved[i], Images());
+        db_->UpdatePageRank(moved_url, moved[i]);
       }
     }
   }
-  AutoLock lock(lock_);
+
   StoreMostVisited(&most_visited);
+  for (size_t i = 0; i < top_sites_.size(); i++) {
+    GURL& url = top_sites_[i].url;
+    Images& img = top_images_[url];
+    if (!img.thumbnail.get() || !img.thumbnail->size()) {
+      StartQueryForThumbnail(i);
+    }
+  }
+
+  timer_.Stop();
+  timer_.Start(GetUpdateDelay(), this,
+               &TopSites::StartQueryForMostVisited);
+}
+
+void TopSites::StartQueryForThumbnail(size_t index) {
+  HistoryService* hs = profile_->GetHistoryService(Profile::EXPLICIT_ACCESS);
+  // |hs| may be null during unit tests.
+  if (!hs)
+    return;
+  HistoryService::Handle handle =
+      hs->GetPageThumbnail(top_sites_[index].url,
+                           &cancelable_consumer_,
+                           NewCallback(this, &TopSites::OnThumbnailAvailable));
+  cancelable_consumer_.SetClientData(hs, handle, index);
 }
 
 void TopSites::StoreMostVisited(MostVisitedURLList* most_visited) {
-  lock_.AssertAcquired();
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::DB));
   // Take ownership of the most visited data.
   top_sites_.clear();
   top_sites_.swap(*most_visited);
@@ -218,7 +255,7 @@ void TopSites::StoreMostVisited(MostVisitedURLList* most_visited) {
 
 void TopSites::StoreRedirectChain(const RedirectList& redirects,
                                   size_t destination) {
-  lock_.AssertAcquired();
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::DB));
 
   if (redirects.empty()) {
     NOTREACHED();
@@ -316,16 +353,15 @@ void TopSites::StartQueryForMostVisited() {
       LOG(INFO) << "History Service not available.";
     }
   }
-
-  timer_.Stop();
-  timer_.Start(GetUpdateDelay(), this,
-               &TopSites::StartQueryForMostVisited);
 }
 
 base::TimeDelta TopSites::GetUpdateDelay() {
+  if (top_sites_.size() == 0)
+    return base::TimeDelta::FromSeconds(30);
+
   int64 range = kMaxUpdateIntervalMinutes - kMinUpdateIntervalMinutes;
   int64 minutes = kMaxUpdateIntervalMinutes -
-      last_num_urls_changed_ * range / kTopSitesNumber;
+      last_num_urls_changed_ * range / top_sites_.size();
   return base::TimeDelta::FromMinutes(minutes);
 }
 
@@ -336,6 +372,17 @@ void TopSites::OnTopSitesAvailable(
       this, &TopSites::UpdateMostVisited, pages));
 }
 
+void TopSites::OnThumbnailAvailable(CancelableRequestProvider::Handle handle,
+                                    scoped_refptr<RefCountedBytes> thumbnail) {
+  HistoryService* hs = profile_ ->GetHistoryService(Profile::EXPLICIT_ACCESS);
+  size_t index = cancelable_consumer_.GetClientData(hs, handle);
+  DCHECK(static_cast<size_t>(index) < top_sites_.size());
+  if (!thumbnail.get() || !thumbnail->size())
+    return;
+  const MostVisitedURL& url = top_sites_[index];
+  SetPageThumbnail(url.url, thumbnail, ThumbnailScore());
+}
+
 void TopSites::SetMockHistoryService(MockHistoryService* mhs) {
   mock_history_service_ = mhs;
 }
@@ -343,17 +390,18 @@ void TopSites::SetMockHistoryService(MockHistoryService* mhs) {
 void TopSites::Observe(NotificationType type,
                        const NotificationSource& source,
                        const NotificationDetails& details) {
-  if (type != NotificationType::HISTORY_URLS_DELETED) {
-    NOTREACHED();
-    return;
-  }
-
-  Details<history::URLsDeletedDetails> deleted_details(details);
-  if (deleted_details->all_history) {
-    ChromeThread::PostTask(ChromeThread::DB, FROM_HERE,
-                           NewRunnableMethod(this, &TopSites::ResetDatabase));
+  if (type == NotificationType::HISTORY_URLS_DELETED) {
+    Details<history::URLsDeletedDetails> deleted_details(details);
+    if (deleted_details->all_history) {
+      ChromeThread::PostTask(ChromeThread::DB, FROM_HERE,
+                             NewRunnableMethod(this, &TopSites::ResetDatabase));
+    }
+    StartQueryForMostVisited();
+  } else if (type == NotificationType::NAV_ENTRY_COMMITTED) {
+    if (top_sites_.size() < kTopSitesNumber) {
+      StartQueryForMostVisited();
+    }
   }
-  StartQueryForMostVisited();
 }
 
 void TopSites::ResetDatabase() {
diff --git a/chrome/browser/history/top_sites.h b/chrome/browser/history/top_sites.h
index cbcd7f5..95f512c 100644
--- a/chrome/browser/history/top_sites.h
+++ b/chrome/browser/history/top_sites.h
@@ -102,12 +102,27 @@ class TopSites : public NotificationObserver,
                             const RefCountedBytes* thumbnail_data,
                             const ThumbnailScore& score);
 
+  // A version of SetPageThumbnail that takes RefCountedBytes as
+  // returned by HistoryService.
+  bool SetPageThumbnail(const GURL& url,
+                        const RefCountedBytes* thumbnail,
+                        const ThumbnailScore& score);
+
+  // Query history service for the list of available thumnbails.
   void StartQueryForMostVisited();
 
-  // Handler for the query response.
+  // Query history service for the thumnbail for a given url. |index|
+  // is the index into top_sites_.
+  void StartQueryForThumbnail(size_t index);
+
+  // Called when history service returns a list of top URLs.
   void OnTopSitesAvailable(CancelableRequestProvider::Handle handle,
                            MostVisitedURLList data);
 
+  // Called when history service returns a thumbnail.
+  void OnThumbnailAvailable(CancelableRequestProvider::Handle handle,
+                            scoped_refptr<RefCountedBytes> thumnbail);
+
   // 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.
@@ -179,7 +194,7 @@ class TopSites : public NotificationObserver,
   // A mockup to use for testing. If NULL, use the real HistoryService
   // from the profile_. See SetMockHistoryService.
   MockHistoryService* mock_history_service_;
-  CancelableRequestConsumerTSimple<PageUsageData*> cancelable_consumer_;
+  CancelableRequestConsumerTSimple<size_t> cancelable_consumer_;
   mutable Lock lock_;
 
   // The cached version of the top sites. The 0th item in this vector is the
diff --git a/chrome/browser/history/top_sites_database.cc b/chrome/browser/history/top_sites_database.cc
index 680abb8..d563fa3a 100644
--- a/chrome/browser/history/top_sites_database.cc
+++ b/chrome/browser/history/top_sites_database.cc
@@ -104,14 +104,54 @@ void TopSitesDatabaseImpl::SetRedirects(const std::string& redirects,
     url->redirects.push_back(GURL(redirects_vector[i]));
 }
 
-
 void TopSitesDatabaseImpl::SetPageThumbnail(const MostVisitedURL& url,
                                             int new_rank,
                                             const TopSites::Images& thumbnail) {
   sql::Transaction transaction(&db_);
   transaction.Begin();
 
-  int prev_rank = GetURLRank(url);
+  int rank = GetURLRank(url);
+  if (rank == -1) {
+    AddPageThumbnail(url, new_rank, thumbnail);
+  } else {
+    UpdatePageRankNoTransaction(url, new_rank);
+    UpdatePageThumbnail(url, thumbnail);
+  }
+
+  transaction.Commit();
+}
+
+void TopSitesDatabaseImpl::UpdatePageThumbnail(
+    const MostVisitedURL& url, const TopSites::Images& thumbnail) {
+  sql::Statement statement(db_.GetCachedStatement(
+      SQL_FROM_HERE,
+      "UPDATE thumbnails SET "
+      "title = ?, thumbnail = ?, redirects = ?, "
+      "boring_score = ?, good_clipping = ?, at_top = ?, last_updated = ? "
+      "WHERE url = ? "));
+  if (!statement)
+    return;
+
+  statement.BindString16(0, url.title);
+  if (thumbnail.thumbnail.get()) {
+    statement.BindBlob(1, &thumbnail.thumbnail->data.front(),
+                       static_cast<int>(thumbnail.thumbnail->data.size()));
+  }
+  statement.BindString(2, GetRedirects(url));
+  const ThumbnailScore& score = thumbnail.thumbnail_score;
+  statement.BindDouble(3, score.boring_score);
+  statement.BindBool(4, score.good_clipping);
+  statement.BindBool(5, score.at_top);
+  statement.BindInt64(6, score.time_at_snapshot.ToInternalValue());
+  statement.BindString(7, url.url.spec());
+  if (!statement.Run())
+    NOTREACHED() << db_.GetErrorMessage();
+}
+
+void TopSitesDatabaseImpl::AddPageThumbnail(const MostVisitedURL& url,
+                                            int new_rank,
+                                            const TopSites::Images& thumbnail) {
+  int count = GetRowCount();
 
   sql::Statement statement(db_.GetCachedStatement(
       SQL_FROM_HERE,
@@ -123,7 +163,7 @@ void TopSitesDatabaseImpl::SetPageThumbnail(const MostVisitedURL& url,
     return;
 
   statement.BindString(0, url.url.spec());
-  statement.BindInt(1, -1);  // Temporary value
+  statement.BindInt(1, count);  // Make it the last url.
   statement.BindString16(2, url.title);
   if (thumbnail.thumbnail.get()) {
     statement.BindBlob(3, &thumbnail.thumbnail->data.front(),
@@ -135,21 +175,31 @@ void TopSitesDatabaseImpl::SetPageThumbnail(const MostVisitedURL& url,
   statement.BindBool(6, score.good_clipping);
   statement.BindBool(7, score.at_top);
   statement.BindInt64(8, score.time_at_snapshot.ToInternalValue());
-
   if (!statement.Run())
     NOTREACHED() << db_.GetErrorMessage();
 
-  // Shift the ranks.
+  UpdatePageRankNoTransaction(url, new_rank);
+}
+
+void TopSitesDatabaseImpl::UpdatePageRank(const MostVisitedURL& url,
+                                          int new_rank) {
+  sql::Transaction transaction(&db_);
+  transaction.Begin();
+  UpdatePageRankNoTransaction(url, new_rank);
+  transaction.Commit();
+}
+
+// Caller should have a transaction open.
+void TopSitesDatabaseImpl::UpdatePageRankNoTransaction(
+    const MostVisitedURL& url, int new_rank) {
+  int prev_rank = GetURLRank(url);
   if (prev_rank == -1) {
-    // Shift up
-    sql::Statement shift_statement(db_.GetCachedStatement(
-        SQL_FROM_HERE,
-        "UPDATE thumbnails "
-        "SET url_rank = url_rank + 1 WHERE url_rank >= ?"));
-    shift_statement.BindInt(0, new_rank);
-    if (shift_statement)
-      shift_statement.Run();
-  } else if (prev_rank > new_rank) {
+    NOTREACHED() << "Updating rank of an unknown URL.";
+    return;
+  }
+
+  // Shift the ranks.
+  if (prev_rank > new_rank) {
     // Shift up
     sql::Statement shift_statement(db_.GetCachedStatement(
         SQL_FROM_HERE,
@@ -173,16 +223,16 @@ void TopSitesDatabaseImpl::SetPageThumbnail(const MostVisitedURL& url,
       shift_statement.Run();
   }
 
-  // Set the real rank.
+  // Set the url's rank.
   sql::Statement set_statement(db_.GetCachedStatement(
       SQL_FROM_HERE,
       "UPDATE thumbnails "
       "SET url_rank = ? "
-      "WHERE url_rank == -1"));
+      "WHERE url == ?"));
   set_statement.BindInt(0, new_rank);
+  set_statement.BindString(1, url.url.spec());
   if (set_statement)
     set_statement.Run();
-  transaction.Commit();
 }
 
 bool TopSitesDatabaseImpl::GetPageThumbnail(const GURL& url,
@@ -212,6 +262,22 @@ bool TopSitesDatabaseImpl::GetPageThumbnail(const GURL& url,
   return true;
 }
 
+int TopSitesDatabaseImpl::GetRowCount() {
+  int result = -1;
+  sql::Statement select_statement(db_.GetCachedStatement(
+      SQL_FROM_HERE,
+      "SELECT COUNT (url) FROM thumbnails"));
+  if (!select_statement) {
+    LOG(WARNING) << db_.GetErrorMessage();
+    return result;
+  }
+
+  if (select_statement.Step())
+    result = select_statement.ColumnInt(0);
+
+  return result;
+}
+
 int TopSitesDatabaseImpl::GetURLRank(const MostVisitedURL& url) {
   int result = -1;
   sql::Statement select_statement(db_.GetCachedStatement(
diff --git a/chrome/browser/history/top_sites_database.h b/chrome/browser/history/top_sites_database.h
index dcdc5877..cfb362c 100644
--- a/chrome/browser/history/top_sites_database.h
+++ b/chrome/browser/history/top_sites_database.h
@@ -47,6 +47,9 @@ class TopSitesDatabase {
                                 int url_rank,
                                 const TopSites::Images& thumbnail) = 0;
 
+  // Update rank of a URL that's already in the database.
+  virtual void UpdatePageRank(const MostVisitedURL& url, int new_rank) = 0;
+
   // Convenience wrapper.
   bool GetPageThumbnail(const MostVisitedURL& url,
                         TopSites::Images* thumbnail) {
@@ -76,6 +79,7 @@ class TopSitesDatabaseImpl : public TopSitesDatabase {
   // WARNING: clears both input arguments.
   virtual void GetPageThumbnails(MostVisitedURLList* urls,
                                  std::map<GURL, TopSites::Images>* thumbnails);
+
   // Set a thumbnail for a URL. |url_rank| is the position of the URL
   // in the list of TopURLs, zero-based.
   // If the URL is not in the table, add it. If it is, replace its
@@ -84,6 +88,10 @@ class TopSitesDatabaseImpl : public TopSitesDatabase {
                                 int new_rank,
                                 const TopSites::Images& thumbnail);
 
+  // Sets the rank for a given URL. The URL must be in the database.
+  // Use SetPageThumbnail if it's not.
+  virtual void UpdatePageRank(const MostVisitedURL& url, int new_rank);
+
   // Get a thumbnail for a given page. Returns true iff we have the thumbnail.
   virtual bool GetPageThumbnail(const GURL& url,
                                 TopSites::Images* thumbnail);
@@ -96,14 +104,23 @@ class TopSitesDatabaseImpl : public TopSitesDatabase {
   // or was successfully created.
   bool InitThumbnailTable();
 
+  // Adds a new URL to the database.
+  void AddPageThumbnail(const MostVisitedURL& url,
+                        int new_rank,
+                        const TopSites::Images& thumbnail);
+
+  // Sets the page rank. Should be called within an open transaction.
+  void UpdatePageRankNoTransaction(const MostVisitedURL& url, int new_rank);
+
+  // Updates thumbnail of a URL that's already in the database.
+  void UpdatePageThumbnail(const MostVisitedURL& url,
+                           const TopSites::Images& thumbnail);
+
   // Returns the URL's current rank or -1 if it is not present.
   int GetURLRank(const MostVisitedURL& url);
 
-  // Gets the URL at |rank|. Returns true if the URL is there.
-  bool GetURLAtRank(int rank, MostVisitedURL* url);
-
-  // Sets the rank of a URL. The URL must be present in the DB.
-  void SetURLRank(const MostVisitedURL& url, int rank);
+  // Returns the number of URLs (rows) in the database.
+  int GetRowCount();
 
   // Encodes redirects into a string.
   static std::string GetRedirects(const MostVisitedURL& url);
diff --git a/chrome/browser/history/top_sites_unittest.cc b/chrome/browser/history/top_sites_unittest.cc
index 7dcb1ab..f3406e5 100644
--- a/chrome/browser/history/top_sites_unittest.cc
+++ b/chrome/browser/history/top_sites_unittest.cc
@@ -142,7 +142,25 @@ class MockTopSitesDatabaseImpl : public TopSitesDatabase {
 
   virtual void SetPageThumbnail(const MostVisitedURL& url, int url_rank,
                                 const TopSites::Images& thumbnail) {
-    // Update the list of URLs
+    SetPageRank(url, url_rank);
+    // Update thubmnail
+    thumbnails_map_[url.url] = thumbnail;
+  }
+
+  virtual void UpdatePageRank(const MostVisitedURL& url, int new_rank) {
+    MostVisitedURLList::iterator pos = std::find(top_sites_list_.begin(),
+                                                 top_sites_list_.end(),
+                                                 url);
+    // Is it in the right position?
+    int rank = pos - top_sites_list_.begin();
+    if (rank != new_rank) {
+      // Move the URL to a new position.
+      top_sites_list_.erase(pos);
+      top_sites_list_.insert(top_sites_list_.begin() + new_rank, url);
+    }
+  }
+
+  virtual void SetPageRank(const MostVisitedURL& url, int url_rank) {
     // Check if this url is in the list, and at which position.
     MostVisitedURLList::iterator pos = std::find(top_sites_list_.begin(),
                                                  top_sites_list_.end(),
@@ -151,16 +169,8 @@ class MockTopSitesDatabaseImpl : public TopSitesDatabase {
       // Add it to the list.
       top_sites_list_.insert(top_sites_list_.begin() + url_rank, url);
     } else {
-      // Is it in the right position?
-      int rank = pos - top_sites_list_.begin();
-      if (rank != url_rank) {
-        // Move the URL to a new position.
-        top_sites_list_.erase(pos);
-        top_sites_list_.insert(top_sites_list_.begin() + url_rank, url);
-      }
+      UpdatePageRank(url, url_rank);
     }
-    // Update thubmnail
-    thumbnails_map_[url.url] = thumbnail;
   }
 
   // Get a thumbnail for a given page. Returns true iff we have the thumbnail.
@@ -230,6 +240,7 @@ static void AppendMostVisitedURLWithRedirect(
 }
 
 TEST_F(TopSitesTest, GetCanonicalURL) {
+  ChromeThread db_loop(ChromeThread::DB, MessageLoop::current());
   // Have two chains:
   //   google.com -> www.google.com
   //   news.google.com (no redirects)
@@ -296,6 +307,7 @@ TEST_F(TopSitesTest, DiffMostVisited) {
 }
 
 TEST_F(TopSitesTest, SetPageThumbnail) {
+  ChromeThread db_loop(ChromeThread::DB, MessageLoop::current());
   GURL url1a("http://google.com/");
   GURL url1b("http://www.google.com/");
   GURL url2("http://images.google.com/");
@@ -683,6 +695,10 @@ TEST_F(TopSitesTest, DeleteNotifications) {
 
 TEST_F(TopSitesTest, GetUpdateDelay) {
   top_sites().last_num_urls_changed_ = 0;
+  EXPECT_EQ(30, top_sites().GetUpdateDelay().InSeconds());
+
+  top_sites().top_sites_.resize(20);
+  top_sites().last_num_urls_changed_ = 0;
   EXPECT_EQ(60, top_sites().GetUpdateDelay().InMinutes());
 
   top_sites().last_num_urls_changed_ = 3;