While importing favicon, make sure there is an entry in the history db.

This will match with what history db does for regular bookmarked URLs with favicons - when history db is cleaned, we keep an entry in the db with 0 visits as long as that url is bookmarked.

BUG=13338

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

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

3 files changed, 87 insertions, 7 deletions

diff --git a/chrome/browser/history/history_backend.cc b/chrome/browser/history/history_backend.cc
index 0d01496..cedc28b 100644
--- a/chrome/browser/history/history_backend.cc
+++ b/chrome/browser/history/history_backend.cc
@@ -1433,16 +1433,33 @@ void HistoryBackend::SetImportedFavicons(
     }
 
     // Save the mapping from all the URLs to the favicon.
+    BookmarkService* bookmark_service = GetBookmarkService();
     for (std::set<GURL>::const_iterator url = favicon_usage[i].urls.begin();
          url != favicon_usage[i].urls.end(); ++url) {
       URLRow url_row;
-      if (!db_->GetRowForURL(*url, &url_row) ||
-          url_row.favicon_id() == favicon_id)
-        continue;  // Don't set favicons for unknown URLs.
-      url_row.set_favicon_id(favicon_id);
-      db_->UpdateURLRow(url_row.id(), url_row);
-
-      favicons_changed.insert(*url);
+      if (!db_->GetRowForURL(*url, &url_row)) {
+        // If the URL is present as a bookmark, add the url in history to
+        // save the favicon mapping. This will match with what history db does
+        // for regular bookmarked URLs with favicons - when history db is
+        // cleaned, we keep an entry in the db with 0 visits as long as that
+        // url is bookmarked.
+        if (bookmark_service && bookmark_service_->IsBookmarked(*url)) {
+          URLRow url_info(*url);
+          url_info.set_visit_count(0);
+          url_info.set_typed_count(0);
+          url_info.set_last_visit(base::Time());
+          url_info.set_hidden(false);
+          url_info.set_favicon_id(favicon_id);
+          db_->AddURL(url_info);
+          favicons_changed.insert(*url);
+        }
+      } else if (url_row.favicon_id() == 0) {
+        // URL is present in history, update the favicon *only* if it
+        // is not set already.
+        url_row.set_favicon_id(favicon_id);
+        db_->UpdateURLRow(url_row.id(), url_row);
+        favicons_changed.insert(*url);
+      }
     }
   }
 
diff --git a/chrome/browser/history/history_backend.h b/chrome/browser/history/history_backend.h
index 36b5193..4effaca 100644
--- a/chrome/browser/history/history_backend.h
+++ b/chrome/browser/history/history_backend.h
@@ -276,6 +276,7 @@ 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, ImportedFaviconsTest);
   FRIEND_TEST(HistoryBackendTest, URLsNoLongerBookmarked);
   friend class ::TestingProfile;
 
diff --git a/chrome/browser/history/history_backend_unittest.cc b/chrome/browser/history/history_backend_unittest.cc
index 62a518d..9f14f1d 100644
--- a/chrome/browser/history/history_backend_unittest.cc
+++ b/chrome/browser/history/history_backend_unittest.cc
@@ -501,4 +501,66 @@ TEST_F(HistoryBackendTest, ClientRedirect) {
   EXPECT_TRUE(transition2 & PageTransition::CHAIN_END);
 }
 
+TEST_F(HistoryBackendTest, ImportedFaviconsTest) {
+  // Setup test data - two Urls in the history, one with favicon assigned and
+  // one without.
+  GURL favicon_url1("http://www.google.com/favicon.ico");
+  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()));
+  URLRow row1(GURL("http://www.google.com/"));
+  row1.set_favicon_id(favicon1);
+  row1.set_visit_count(1);
+  row1.set_last_visit(Time::Now());
+  URLRow row2(GURL("http://news.google.com/"));
+  row2.set_visit_count(1);
+  row2.set_last_visit(Time::Now());
+  std::vector<URLRow> rows;
+  rows.push_back(row1);
+  rows.push_back(row2);
+  backend_->AddPagesWithDetails(rows);
+  URLRow url_row1, url_row2;
+  EXPECT_FALSE(backend_->db_->GetRowForURL(row1.url(), &url_row1) == 0);
+  EXPECT_FALSE(backend_->db_->GetRowForURL(row2.url(), &url_row2) == 0);
+  EXPECT_FALSE(url_row1.favicon_id() == 0);
+  EXPECT_TRUE(url_row2.favicon_id() == 0);
+
+  // Now provide one imported favicon for both URLs already in the registry.
+  // The new favicon should only be used with the URL that doesn't already have
+  // a favicon.
+  std::vector<history::ImportedFavIconUsage> favicons;
+  history::ImportedFavIconUsage favicon;
+  favicon.favicon_url = GURL("http://news.google.com/favicon.ico");
+  favicon.png_data.push_back('2');
+  favicon.urls.insert(row1.url());
+  favicon.urls.insert(row2.url());
+  favicons.push_back(favicon);
+  backend_->SetImportedFavicons(favicons);
+  EXPECT_FALSE(backend_->db_->GetRowForURL(row1.url(), &url_row1) == 0);
+  EXPECT_FALSE(backend_->db_->GetRowForURL(row2.url(), &url_row2) == 0);
+  EXPECT_FALSE(url_row1.favicon_id() == 0);
+  EXPECT_FALSE(url_row2.favicon_id() == 0);
+  EXPECT_FALSE(url_row1.favicon_id() == url_row2.favicon_id());
+
+  // A URL should not be added to history (to store favicon), if
+  // the URL is not bookmarked.
+  GURL url3("http://mail.google.com");
+  favicons.clear();
+  favicon.favicon_url = GURL("http://mail.google.com/favicon.ico");
+  favicon.png_data.push_back('3');
+  favicon.urls.insert(url3);
+  favicons.push_back(favicon);
+  backend_->SetImportedFavicons(favicons);
+  URLRow url_row3;
+  EXPECT_TRUE(backend_->db_->GetRowForURL(url3, &url_row3) == 0);
+
+  // If the URL is bookmarked, it should get added to history with 0 visits.
+  bookmark_model_.AddURL(bookmark_model_.GetBookmarkBarNode(), 0,
+                         std::wstring(), url3);
+  backend_->SetImportedFavicons(favicons);
+  EXPECT_FALSE(backend_->db_->GetRowForURL(url3, &url_row3) == 0);
+  EXPECT_TRUE(url_row3.visit_count() == 0);
+}
+
 }  // namespace history