[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..ecadbe1ec1d64d2fc475b16fdf49d0baf0a96066 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"
@@ -114,8 +115,23 @@ const int kExpirationDelaySec = 30;
 // iteration, so we want to wait longer before checking to avoid wasting CPU.
 const int kExpirationEmptyDelayMin = 5;
 
+bool IsAnyURLBookmarked(HistoryBackendClient* backend_client,
+                        std::vector<GURL> urls) {

[pkotwicz, 2017/07/10 05:23:28]

Nit: const std::vector<GURL>& urls

[jkrcal, 2017/07/10 16:14:30]

Ah, sure. Thanks!

+  for (const GURL& url : urls) {
+    if (backend_client->IsBookmarked(url))
+      return true;
+  }
+  return false;
+}
+
 }  // namespace
 
+namespace internal {
+
+const base::Feature kClearOldOnDemandFavicons{
+    "ClearOldOnDemandFavicons", base::FEATURE_DISABLED_BY_DEFAULT};
+
+}  // namespace internal
 
 // ExpireHistoryBackend::DeleteEffects ----------------------------------------
 
@@ -477,10 +493,37 @@ 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::map<favicon_base::FaviconID, FaviconURLs> favicons =

[pkotwicz, 2017/07/10 05:23:28]

I would rather that you do one of:
1) Put the FaviconID and the favicon URL in a struct instead of the icon URL and
the page URL. It will likely be harder to return a std::map if you go down this
route.
2) Make use of the already existing IconMapping struct. sky@ will likely have an
opinion as to whether you should fill all of the fields in the IconMapping
struct
3) Rename the FaviconURLs struct to something more intuitive. I don't have any
suggestions but it should likely have the word "mapping" in the name

[jkrcal, 2017/07/10 16:14:30]

I do understand the benefits of 2) or 3) but miss the benefits of 1). If this
was your preferred choice, could you clarify, please?

I tried another shot on 3): IconMappingsForExpiry.
I am also fine with the more generic approach of reusing IconMappings; I still
prefer a custom struct for this task.

[pkotwicz, 2017/07/10 19:50:19]

I don't have a strong preference between the 3 options. The idea with (1) was to
group variables which are related to each other in the same struct.

Currently you have:
std::map<FaviconID, std::map<GURL, std::vector<GURL>>>

where
IconMappingsForExpiry is
std::map<GURL, std::vector<GURL>>

I was suggesting changing the structure to
std::map<std::pair<FaviconID, GURL>, std::vector<GURL>>
where std::pair<FaviconID, GURL> would be a new struct (Though I think you would
need to implement a custom 'less than' operator for the struct)

Returning a std::map of std::maps is kind of weird in my opinion

[jkrcal, 2017/07/11 13:19:34]

Not really. Currently, I have conceptually
std::map<FaviconID, std::pair<GURL, std::vector<GURL>>>

where IconMappingsForExpiry corresponds to std::pair<GURL, std::vector<GURL>.
Thanks for explaining! I still lean towards option 3 (as implemented now).
As argued above, I do not return a map of maps.

+      thumb_db_->GetOldOnDemandFavicons(expiration_threshold);
+  DeleteEffects effects;
+
+  for (auto id_and_urls_pair : favicons) {
+    favicon_base::FaviconID icon_id = id_and_urls_pair.first;
+    const FaviconURLs& urls = id_and_urls_pair.second;
+
+    if (backend_client_ && IsAnyURLBookmarked(backend_client_, urls.page_urls))
+      continue;
+
+    thumb_db_->DeleteFavicon(icon_id);
+    thumb_db_->DeleteIconMappingsForFaviconId(icon_id);
+    effects.deleted_favicons.insert(urls.icon_url);
+  }
+
+  BroadcastNotifications(&effects, DELETION_EXPIRED);
+}
+
 bool ExpireHistoryBackend::ExpireSomeOldHistory(
     base::Time end_time,
     const ExpiringVisitsReader* reader,