Fix broken thumbnails for sites with redirects.

BUG=52621
TEST=unit_tests

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

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

4 files changed, 57 insertions, 18 deletions

diff --git a/chrome/browser/dom_ui/most_visited_handler.cc b/chrome/browser/dom_ui/most_visited_handler.cc
index 9a045d8..66097f0 100644
--- a/chrome/browser/dom_ui/most_visited_handler.cc
+++ b/chrome/browser/dom_ui/most_visited_handler.cc
@@ -127,17 +127,6 @@ void MostVisitedHandler::HandleGetMostVisited(const ListValue* args) {
   }
 }
 
-// Set a DictionaryValue |dict| from a MostVisitedURL.
-void SetDictionaryValue(const history::MostVisitedURL& url,
-                        DictionaryValue& dict) {
-  NewTabUI::SetURLTitleAndDirection(&dict, url.title, url.url);
-  dict.SetString("url", url.url.spec());
-  dict.SetString("faviconUrl", url.favicon_url.spec());
-  // TODO(Nik): Need thumbnailUrl?
-  // TODO(Nik): Add pinned and blacklisted URLs.
-  dict.SetBoolean("pinned", false);
-}
-
 void MostVisitedHandler::SendPagesValue() {
   if (pages_value_.get()) {
     FundamentalValue first_run(IsFirstRun());
diff --git a/chrome/browser/history/top_sites.cc b/chrome/browser/history/top_sites.cc
index c0e4580..b6dc226 100644
--- a/chrome/browser/history/top_sites.cc
+++ b/chrome/browser/history/top_sites.cc
@@ -249,7 +249,8 @@ void TopSites::GetMostVisitedURLs(CancelableRequestConsumer* consumer,
 
 bool TopSites::GetPageThumbnail(const GURL& url, RefCountedBytes** data) const {
   AutoLock lock(lock_);
-  std::map<GURL, Images>::const_iterator found = top_images_.find(url);
+  std::map<GURL, Images>::const_iterator found =
+      top_images_.find(GetCanonicalURL(url));
   if (found == top_images_.end()) {
     found = temp_thumbnails_map_.find(url);
     if (found == temp_thumbnails_map_.end())
@@ -379,7 +380,9 @@ std::string TopSites::GetURLString(const GURL& url) {
 
 std::string TopSites::GetURLHash(const GURL& url) {
   lock_.AssertAcquired();
-  return MD5String(GetCanonicalURL(url).spec());
+  // We don't use canonical URLs here to be able to blacklist only one of
+  // the two 'duplicate' sites, e.g. 'gmail.com' and 'mail.google.com'.
+  return MD5String(url.spec());
 }
 
 void TopSites::UpdateMostVisited(MostVisitedURLList most_visited) {
@@ -565,8 +568,11 @@ void TopSites::StoreRedirectChain(const RedirectList& redirects,
   }
 
   // Map all the redirected URLs to the destination.
-  for (size_t i = 0; i < redirects.size(); i++)
-    canonical_urls_[redirects[i]] = destination;
+  for (size_t i = 0; i < redirects.size(); i++) {
+    // If this redirect is already known, don't replace it with a new one.
+    if (canonical_urls_.find(redirects[i]) == canonical_urls_.end())
+      canonical_urls_[redirects[i]] = destination;
+  }
 }
 
 GURL TopSites::GetCanonicalURL(const GURL& url) const {
@@ -634,7 +640,7 @@ void TopSites::StartQueryForMostVisited() {
     // Testing with a mockup.
     // QueryMostVisitedURLs is not virtual, so we have to duplicate the code.
     mock_history_service_->QueryMostVisitedURLs(
-        kTopSitesNumber,
+        kTopSitesNumber + blacklist_->size(),
         kDaysOfHistory,
         &cancelable_consumer_,
         NewCallback(this, &TopSites::OnTopSitesAvailable));
@@ -646,7 +652,7 @@ void TopSites::StartQueryForMostVisited() {
     // |hs| may be null during unit tests.
     if (hs) {
       hs->QueryMostVisitedURLs(
-          kTopSitesNumber,
+          kTopSitesNumber + blacklist_->size(),
           kDaysOfHistory,
           &cancelable_consumer_,
           NewCallback(this, &TopSites::OnTopSitesAvailable));
@@ -764,7 +770,6 @@ base::TimeDelta TopSites::GetUpdateDelay() {
 void TopSites::OnTopSitesAvailable(
     CancelableRequestProvider::Handle handle,
     MostVisitedURLList pages) {
-
   AddPrepopulatedPages(&pages);
   ChromeThread::PostTask(ChromeThread::DB, FROM_HERE, NewRunnableMethod(
       this, &TopSites::UpdateMostVisited, pages));
diff --git a/chrome/browser/history/top_sites.h b/chrome/browser/history/top_sites.h
index b4063ab..81884d7 100644
--- a/chrome/browser/history/top_sites.h
+++ b/chrome/browser/history/top_sites.h
@@ -153,6 +153,7 @@ class TopSites :
   FRIEND_TEST_ALL_PREFIXES(TopSitesTest, PinnedURLs);
   FRIEND_TEST_ALL_PREFIXES(TopSitesTest, BlacklistingAndPinnedURLs);
   FRIEND_TEST_ALL_PREFIXES(TopSitesTest, AddPrepopulatedPages);
+  FRIEND_TEST_ALL_PREFIXES(TopSitesTest, GetPageThumbnail);
 
   ~TopSites();
 
diff --git a/chrome/browser/history/top_sites_unittest.cc b/chrome/browser/history/top_sites_unittest.cc
index d0ab05f..360cd0d 100644
--- a/chrome/browser/history/top_sites_unittest.cc
+++ b/chrome/browser/history/top_sites_unittest.cc
@@ -415,6 +415,50 @@ TEST_F(TopSitesTest, SetPageThumbnail) {
   EXPECT_FALSE(top_sites().SetPageThumbnail(url1a, thumbnail, medium_score));
 }
 
+TEST_F(TopSitesTest, GetPageThumbnail) {
+  ChromeThread db_loop(ChromeThread::DB, MessageLoop::current());
+  MostVisitedURLList url_list;
+  MostVisitedURL url1 = {GURL("http://asdf.com")};
+  url1.redirects.push_back(url1.url);
+  url_list.push_back(url1);
+
+  MostVisitedURL url2 = {GURL("http://gmail.com")};
+  url2.redirects.push_back(url2.url);
+  url2.redirects.push_back(GURL("http://mail.google.com"));
+  url_list.push_back(url2);
+
+  top_sites().UpdateMostVisited(url_list);
+  MessageLoop::current()->RunAllPending();
+
+  // Create a dummy thumbnail.
+  SkBitmap thumbnail;
+  thumbnail.setConfig(SkBitmap::kARGB_8888_Config, 4, 4);
+  thumbnail.allocPixels();
+  thumbnail.eraseRGB(0x00, 0x00, 0x00);
+  ThumbnailScore score(0.5, true, true, base::Time::Now());
+
+  RefCountedBytes* result = NULL;
+  EXPECT_TRUE(top_sites().SetPageThumbnail(url1.url, thumbnail, score));
+  EXPECT_TRUE(top_sites().GetPageThumbnail(url1.url, &result));
+
+  EXPECT_TRUE(top_sites().SetPageThumbnail(GURL("http://gmail.com"),
+                                           thumbnail, score));
+  EXPECT_TRUE(top_sites().GetPageThumbnail(GURL("http://gmail.com"),
+                                           &result));
+  // Get a thumbnail via a redirect.
+  EXPECT_TRUE(top_sites().GetPageThumbnail(GURL("http://mail.google.com"),
+                                           &result));
+
+  EXPECT_TRUE(top_sites().SetPageThumbnail(GURL("http://mail.google.com"),
+                                           thumbnail, score));
+  EXPECT_TRUE(top_sites().GetPageThumbnail(url2.url, &result));
+
+  scoped_ptr<SkBitmap> out_bitmap(gfx::JPEGCodec::Decode(result->front(),
+                                                         result->size()));
+  EXPECT_EQ(0, memcmp(thumbnail.getPixels(), out_bitmap->getPixels(),
+                      thumbnail.getSize()));
+}
+
 TEST_F(TopSitesTest, GetMostVisited) {
   ChromeThread db_loop(ChromeThread::DB, MessageLoop::current());
   GURL news("http://news.google.com/");