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

components/history/core/browser/thumbnail_database.cc

Index: components/history/core/browser/thumbnail_database.cc
diff --git a/components/history/core/browser/thumbnail_database.cc b/components/history/core/browser/thumbnail_database.cc
index c8b430f4f71d8dd70f0111995c5ce74297cec7d9..7e2e6a0cbec966c3f4e7e69faded184801755aa1 100644
--- a/components/history/core/browser/thumbnail_database.cc
+++ b/components/history/core/browser/thumbnail_database.cc
@@ -412,6 +412,33 @@ void ThumbnailDatabase::TrimMemory(bool aggressively) {
   db_.TrimMemory(aggressively);
 }
 
+std::map<favicon_base::FaviconID, IconMappingsForExpiry>
+ThumbnailDatabase::GetOldOnDemandFavicons(base::Time threshold) {
+  // Select all bitmaps (and their page URLs) that have not been accessed for a
+  // while. Restrict to on-demand bitmaps (i.e. with last_requested != 0).

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

Nit: Remove the sentence: "Select all bitmaps (and their page URLs) that have
not been accessed for a while." This is clear from the comment in the .h file

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

Done.

+  // TODO(jkrcal): In M63, remove the "(last_requested=0 AND last_updated=0)"
+  // clause which is only transitional - to clean up on demand favicons stored
+  // before this function has been implemented.
+  sql::Statement old_icons(db_.GetCachedStatement(
+      SQL_FROM_HERE,
+      "SELECT favicons.id, favicons.url, icon_mapping.page_url FROM "
+      "favicons, favicon_bitmaps, icon_mapping WHERE favicon_bitmaps.icon_id = "
+      "icon_mapping.icon_id AND favicons.id = favicon_bitmaps.icon_id AND "
+      "((last_requested=0 AND last_updated=0) OR "
+      "(last_requested>0 AND last_requested<?));"));

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

Can

(last_requested=0 AND last_updated=0) OR (last_requested>0 AND last_requested<?)

be simplified to

last_requested<? AND last_updated=0

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

Yes, you are right. Still, I would stick to the current version.
I believe that the current verbose version makes the proposed clean-up (in M63)
easier to understand and thus less error-prone.

[pkotwicz, 2017/07/11 14:41:04]

Fair enough

+  old_icons.BindInt64(0, threshold.ToInternalValue());
+
+  std::map<favicon_base::FaviconID, IconMappingsForExpiry> favicons;

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

Maybe rename this variable to |icon_mappings|

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

Done.

+
+  while (old_icons.Step()) {
+    favicon_base::FaviconID id = old_icons.ColumnInt64(0);
+    favicons[id].icon_url = GURL(old_icons.ColumnString(1));
+    favicons[id].page_urls.push_back(GURL(old_icons.ColumnString(2)));
+  }
+
+  return favicons;
+}
+
 bool ThumbnailDatabase::GetFaviconBitmapIDSizes(
     favicon_base::FaviconID icon_id,
     std::vector<FaviconBitmapIDSize>* bitmap_id_sizes) {
@@ -778,6 +805,14 @@ bool ThumbnailDatabase::DeleteIconMappings(const GURL& page_url) {
   return statement.Run();
 }
 
+bool ThumbnailDatabase::DeleteIconMappingsForFaviconId(
+    favicon_base::FaviconID id) {
+  sql::Statement statement(db_.GetCachedStatement(
+      SQL_FROM_HERE, "DELETE FROM icon_mapping WHERE icon_id=?"));
+  statement.BindInt64(0, id);
+  return statement.Run();
+}
+
 bool ThumbnailDatabase::DeleteIconMapping(IconMappingID mapping_id) {
   sql::Statement statement(db_.GetCachedStatement(SQL_FROM_HERE,
       "DELETE FROM icon_mapping WHERE id=?"));