[Thumbnails DB] Add functionality to clear unused on-demand favicons.

components/history/core/browser/expire_history_backend.cc

Index: components/history/core/browser/expire_history_backend.cc
diff --git a/components/history/core/browser/expire_history_backend.cc b/components/history/core/browser/expire_history_backend.cc
index d57a60432afb85c3f74790c13d50d8e7865dd7b9..ee858d4e4883824a50c0730980f9f434547f4841 100644
--- a/components/history/core/browser/expire_history_backend.cc
+++ b/components/history/core/browser/expire_history_backend.cc
@@ -12,6 +12,7 @@
 
 #include "base/bind.h"
 #include "base/compiler_specific.h"
+#include "base/feature_list.h"
 #include "base/files/file_enumerator.h"
 #include "base/files/file_util.h"
 #include "base/location.h"
@@ -116,6 +117,12 @@ const int kExpirationEmptyDelayMin = 5;
 
 }  // namespace
 
+namespace internal {
+
+const base::Feature kClearOldOnDemandFavicons{
+    "ClearOldOnDemandFavicons", base::FEATURE_DISABLED_BY_DEFAULT};
+
+}  // namespace internal
 
 // ExpireHistoryBackend::DeleteEffects ----------------------------------------
 
@@ -477,10 +484,47 @@ void ExpireHistoryBackend::DoExpireIteration() {
   // creating a new task for future iterations.
   if (more_to_expire)
     work_queue_.push(reader);
+  // Otherwise do a final clean-up - remove old favicons not bound to visits.
+  else
+    ClearOldOnDemandFavicons(GetCurrentExpirationTime());
 
   ScheduleExpire();
 }
 
+void ExpireHistoryBackend::ClearOldOnDemandFavicons(
+    base::Time expiration_threshold) {
+  if (!base::FeatureList::IsEnabled(internal::kClearOldOnDemandFavicons))
+    return;
+
+  std::vector<std::pair<favicon_base::FaviconID, GURL>> favicons =
+      thumb_db_->GetOldOnDemandFavicons(expiration_threshold);
+
+  // Multiple page URLs may map to the same favicon. We omit the favicon from
+  // cleaning if at least one of its associated page URLs is bookmarked.
+  std::set<favicon_base::FaviconID> ids_of_icons_with_some_bookmarked_page;
+  std::set<favicon_base::FaviconID> ids_of_icons_with_no_bookmarked_page;
+  for (auto id_and_url_pair : favicons) {
+    favicon_base::FaviconID icon_id = id_and_url_pair.first;
+    if (ids_of_icons_with_some_bookmarked_page.count(icon_id))
+      continue;
+
+    const GURL& page_url = id_and_url_pair.second;
+    if (backend_client_ && backend_client_->IsBookmarked(page_url)) {
+      ids_of_icons_with_some_bookmarked_page.insert(icon_id);
+      ids_of_icons_with_no_bookmarked_page.erase(icon_id);
+      continue;
+    }
+
+    ids_of_icons_with_no_bookmarked_page.insert(icon_id);
+  }
+
+  for (favicon_base::FaviconID icon_id : ids_of_icons_with_no_bookmarked_page) {
+    if (!thumb_db_->DeleteFavicon(icon_id) ||
+        !thumb_db_->DeleteIconMappingsForFaviconId(icon_id))

[pkotwicz, 2017/07/07 00:42:38]

Don't you need to update DeleteEffects::deleted_favicons ?

[jkrcal, 2017/07/07 08:45:16]

Done. 

This meant pulling also the icon_url. This was strong enough reason to create a
struct for the result and move some of the logic back to thumbnail_database.cc.
I like it much better now.

+      return;

[pkotwicz, 2017/07/07 00:42:38]

For the sake of consistency with DeleteFaviconsIfPossible() don't return if a
given deletion fails

[jkrcal, 2017/07/07 08:45:16]

Done.

+  }
+}
+
 bool ExpireHistoryBackend::ExpireSomeOldHistory(
     base::Time end_time,
     const ExpiringVisitsReader* reader,