[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 5212268c67970cca9e0ea5897fd7185a7f1980e6..ec66e679e89a8309c8d0ca34d41995eae61d73ee 100644
--- a/components/history/core/browser/thumbnail_database.cc
+++ b/components/history/core/browser/thumbnail_database.cc
@@ -273,6 +273,22 @@ void DatabaseErrorCallback(sql::Connection* db,
     DLOG(FATAL) << db->GetErrorMessage();
 }
 
+void DeleteOrphanagedFaviconBitmaps(sql::Connection* db) {
+  sql::Statement favicons(db->GetCachedStatement(
+      SQL_FROM_HERE,
+      "DELETE FROM favicon_bitmaps WHERE NOT EXISTS (SELECT id FROM favicons "
+      "WHERE favicon_bitmaps.icon_id = favicons.id)"));
+  favicons.Run();
+}
+
+void DeleteOrphanagedMappings(sql::Connection* db) {
+  sql::Statement mappings(db->GetCachedStatement(
+      SQL_FROM_HERE,
+      "DELETE FROM icon_mapping WHERE NOT EXISTS (SELECT id FROM favicons "
+      "WHERE favicons.id = icon_mapping.icon_id)"));
+  mappings.Run();
+}
+
 void FillStatementForAddFaviconBitmap(
     favicon_base::FaviconID icon_id,
     const scoped_refptr<base::RefCountedMemory>& icon_data,
@@ -434,6 +450,49 @@ void ThumbnailDatabase::TrimMemory(bool aggressively) {
   db_.TrimMemory(aggressively);
 }
 
+void ThumbnailDatabase::ClearOldOnDemandFavicons(
+    base::Time expiration_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).
+  sql::Statement delete_candidates(db_.GetCachedStatement(
+      SQL_FROM_HERE,
+      "SELECT favicon_bitmaps.icon_id, icon_mapping.page_url FROM "
+      "favicon_bitmaps, icon_mapping WHERE favicon_bitmaps.icon_id = "
+      "icon_mapping.icon_id AND favicon_bitmaps.last_requested>0 AND "
+      "favicon_bitmaps.last_requested<?;"));

[pkotwicz, 2017/05/24 17:05:03]

If:
1) SetOnDemandFavicons() is called
2) SetFavicons() with the same page URL (as a result of a user visiting the
page)

Won't this function incorrectly delete the favicon?

Don't you also need to check that favicon_bitmaps.last_updated is expired?

[jkrcal, 2017/06/21 17:19:20]

This has been fixed in the preceding CL: it holds that 
  last_requested > 0 iff last_updated == 0.

On the other hand, I am wondering about the transition to this system: 
 - Before this work is complete, all on demand favicons are stored with both
timestamps equal to 0 and I'd like to tidy up.
 - Thus, I'd like to delete here both old on demand favicons as well as expired
on-visit favicons. 
 - This could be only for a transitional phase and after one milestone, get back
to deleting only old on-demand bitmaps.

Do you see a problem in deleting expired icons?

[jkrcal, 2017/06/22 11:22:11]

Pinging this question.

[pkotwicz, 2017/06/22 20:32:22]

I don't see a problem in expiring the icons

[jkrcal, 2017/07/06 17:01:48]

Done.

+  delete_candidates.BindInt64(0, expiration_threshold.ToInternalValue());
+
+  std::set<FaviconBitmapID> ids_of_bookmarked_favicons;
+  std::set<FaviconBitmapID> ids_to_delete;
+  // Multiple page URLs may map to the same favicon. We omit the favicon_bitmap
+  // from cleaning if at least one of its associated page URLs is bookmarked.
+  while (delete_candidates.Step()) {
+    FaviconBitmapID bitmap_id = delete_candidates.ColumnInt64(0);

[pkotwicz, 2017/05/24 17:05:03]

The "id" column in the favicon_bitmaps table is the
favicon_base::FaviconBitmapID. The "icon_id" column in the favicon_bitmaps table
is the favicon_base::FaviconID 

[jkrcal, 2017/06/21 17:19:20]

Ah, thanks! Done (incl. variable names).

+    if (ids_of_bookmarked_favicons.count(bitmap_id))
+      continue;
+
+    if (backend_client_ && backend_client_->IsBookmarked(
+                               GURL(delete_candidates.ColumnString(1)))) {
+      ids_of_bookmarked_favicons.insert(bitmap_id);
+      ids_to_delete.erase(bitmap_id);

[pkotwicz, 2017/05/24 17:05:03]

Is it possible for |bitmap_id| to have been added to |ids_to_delete|?

[jkrcal, 2017/06/21 17:19:20]

Yes, if first some non-bookmarked page with this icon appeared in the while
loop.

I reworded the variables, I hope it is clearer now.

[pkotwicz, 2017/06/21 19:50:17]

Ahhh. I see now. Thank you for the explanation

+      continue;
+    }
+
+    ids_to_delete.insert(bitmap_id);
+  }
+
+  for (FaviconBitmapID bitmap_id : ids_to_delete) {
+    sql::Statement statement(db_.GetCachedStatement(
+        SQL_FROM_HERE, "DELETE FROM favicons WHERE id=?"));
+    statement.BindInt64(0, bitmap_id);
+    statement.Run();
+  }
+
+  // The bitmaps and mappings for all deleted favicons are deleted at once.
+  DeleteOrphanagedFaviconBitmaps(&db_);
+  DeleteOrphanagedMappings(&db_);
+}
+
 bool ThumbnailDatabase::GetFaviconBitmapIDSizes(
     favicon_base::FaviconID icon_id,
     std::vector<FaviconBitmapIDSize>* bitmap_id_sizes) {