[Thumbnails DB] Allow setting last_requested time when accessing 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 60445eb48f0dc66b6f7bc4dd12dedc3898a0327a..d189d5b11f47e05bfa4d0f1e2fc86c857170e80a 100644
--- a/components/history/core/browser/thumbnail_database.cc
+++ b/components/history/core/browser/thumbnail_database.cc
@@ -64,13 +64,17 @@ namespace history {
 //   icon_id          The ID of the favicon that the bitmap is associated to.
 //   last_updated     The time at which this favicon was inserted into the
 //                    table. This is used to determine if it needs to be
-//                    redownloaded from the web.
+//                    redownloaded from the web. Value 0 denotes that the bitmap
+//                    has been explicitly expired.
 //   image_data       PNG encoded data of the favicon.
 //   width            Pixel width of |image_data|.
 //   height           Pixel height of |image_data|.
 //   last_requested   The time at which this bitmap was last requested. This is
-//                    used to determine the priority with which the bitmap
-//                    should be retained on cleanup.
+//                    only used for bitmaps of type ON_DEMAND, for clearing old

[brettw, 2017/06/19 19:31:07]

Can you reword this comment a bit? It reads to me a bit circular where the type
== ON_DEMAND tells you that this is used, but this existing tells you that type
== ON_DEMAND.

[jkrcal, 2017/06/21 16:05:50]

Done.

+//                    entries. (On-demand bitmaps cannot get cleared along with
+//                    expired visits in history DB.) On-demand bitmaps are
+//                    defined by last_requested>0 and thus this field should
+//                    never get updated for bitmaps of type ON_VISIT.
 
 namespace {
 
@@ -503,12 +507,16 @@ bool ThumbnailDatabase::GetFaviconBitmap(
 FaviconBitmapID ThumbnailDatabase::AddFaviconBitmap(
     favicon_base::FaviconID icon_id,
     const scoped_refptr<base::RefCountedMemory>& icon_data,
+    FaviconBitmapType type,
     base::Time time,
     const gfx::Size& pixel_size) {
   DCHECK(icon_id);
-  sql::Statement statement(db_.GetCachedStatement(SQL_FROM_HERE,
-      "INSERT INTO favicon_bitmaps (icon_id, image_data, last_updated, width, "
-      "height) VALUES (?, ?, ?, ?, ?)"));
+
+  sql::Statement statement(db_.GetCachedStatement(
+      SQL_FROM_HERE,
+      "INSERT INTO favicon_bitmaps (icon_id, image_data, last_updated, "
+      "last_requested, width, height) VALUES (?, ?, ?, ?, ?, ?)"));
+
   statement.BindInt64(0, icon_id);
   if (icon_data.get() && icon_data->size()) {
     statement.BindBlob(1, icon_data->front(),
@@ -516,9 +524,19 @@ FaviconBitmapID ThumbnailDatabase::AddFaviconBitmap(
   } else {
     statement.BindNull(1);
   }
-  statement.BindInt64(2, time.ToInternalValue());
-  statement.BindInt(3, pixel_size.width());
-  statement.BindInt(4, pixel_size.height());
+
+  // On-visit bitmaps:
+  //  - keep track of last_updated: last write time is used for expiration;
+  //  - always have last_requested==0: no need to keep track of last read time.
+  statement.BindInt64(2, type == ON_VISIT ? time.ToInternalValue() : 0);
+  // On-demand bitmaps:
+  //  - always have last_updated==0: last write time is not stored as they are
+  //    always expired and thus ready to be replaced by ON_VISIT icons;
+  //  - keep track of last_requested: last read time is used for cache eviction.
+  statement.BindInt64(3, type == ON_DEMAND ? time.ToInternalValue() : 0);
+
+  statement.BindInt(4, pixel_size.width());
+  statement.BindInt(5, pixel_size.height());
 
   if (!statement.Run())
     return 0;
@@ -530,8 +548,13 @@ bool ThumbnailDatabase::SetFaviconBitmap(
     scoped_refptr<base::RefCountedMemory> bitmap_data,
     base::Time time) {
   DCHECK(bitmap_id);
-  sql::Statement statement(db_.GetCachedStatement(SQL_FROM_HERE,
-      "UPDATE favicon_bitmaps SET image_data=?, last_updated=? WHERE id=?"));
+  // By updating last_updated timestamp, we assume the icon is of type ON_VISIT.
+  // If it is ON_DEMAND, reset last_requested to 0 and thus silently change the
+  // type to ON_VISIT.
+  sql::Statement statement(
+      db_.GetCachedStatement(SQL_FROM_HERE,
+                             "UPDATE favicon_bitmaps SET image_data=?, "
+                             "last_updated=?, last_requested=? WHERE id=?"));
   if (bitmap_data.get() && bitmap_data->size()) {
     statement.BindBlob(0, bitmap_data->front(),
                        static_cast<int>(bitmap_data->size()));
@@ -539,7 +562,8 @@ bool ThumbnailDatabase::SetFaviconBitmap(
     statement.BindNull(0);
   }
   statement.BindInt64(1, time.ToInternalValue());
-  statement.BindInt64(2, bitmap_id);
+  statement.BindInt64(2, 0);
+  statement.BindInt64(3, bitmap_id);
 
   return statement.Run();
 }
@@ -548,22 +572,46 @@ bool ThumbnailDatabase::SetFaviconBitmapLastUpdateTime(
     FaviconBitmapID bitmap_id,
     base::Time time) {
   DCHECK(bitmap_id);
-  sql::Statement statement(db_.GetCachedStatement(SQL_FROM_HERE,
-      "UPDATE favicon_bitmaps SET last_updated=? WHERE id=?"));
+  // By updating last_updated timestamp, we assume the icon is of type ON_VISIT.
+  // If it is ON_DEMAND, reset last_requested to 0 and thus silently change the
+  // type to ON_VISIT.
+  sql::Statement statement(
+      db_.GetCachedStatement(SQL_FROM_HERE,
+                             "UPDATE favicon_bitmaps SET last_updated=?, "
+                             "last_requested=? WHERE id=?"));
   statement.BindInt64(0, time.ToInternalValue());
-  statement.BindInt64(1, bitmap_id);
+  statement.BindInt64(1, 0);
+  statement.BindInt64(2, bitmap_id);
   return statement.Run();
 }
 
-bool ThumbnailDatabase::SetFaviconBitmapLastRequestedTime(
-    FaviconBitmapID bitmap_id,
-    base::Time time) {
-  DCHECK(bitmap_id);
-  sql::Statement statement(db_.GetCachedStatement(SQL_FROM_HERE,
-      "UPDATE favicon_bitmaps SET last_requested=? WHERE id=?"));
-  statement.BindInt64(0, time.ToInternalValue());
-  statement.BindInt64(1, bitmap_id);
-  return statement.Run();
+bool ThumbnailDatabase::TouchOnDemandFavicon(const GURL& icon_url,
+                                             base::Time time) {
+  // Look up the icon ids for the url.
+  sql::Statement id_statement(db_.GetCachedStatement(
+      SQL_FROM_HERE, "SELECT id FROM favicons WHERE url=?"));
+  id_statement.BindString(0, URLDatabase::GURLToDatabaseURL(icon_url));
+
+  while (id_statement.Step()) {
+    favicon_base::FaviconID icon_id = id_statement.ColumnInt64(0);
+
+    // Update the time only for ON_DEMAND bitmaps (i.e. with last_requested >
+    // 0). For performance reasons, update the time only if the currently stored
+    // time is old enough (UPDATEs where the WHERE condition does not match any
+    // entries are way faster than UPDATEs that really change some data).
+    sql::Statement statement(db_.GetCachedStatement(
+        SQL_FROM_HERE,
+        "UPDATE favicon_bitmaps SET last_requested=? WHERE icon_id=? AND "
+        "last_requested>0 AND last_requested<=?"));
+    statement.BindInt64(0, time.ToInternalValue());
+    statement.BindInt64(1, icon_id);
+    base::Time max_time =
+        time - base::TimeDelta::FromDays(kFaviconUpdateLastRequestedAfterDays);

[pkotwicz, 2017/06/09 23:47:23]

Nit: You can move the initialization of |max_time| outside the loop

[jkrcal, 2017/06/21 16:05:50]

Done.

+    statement.BindInt64(2, max_time.ToInternalValue());
+    if (!statement.Run())
+      return false;
+  }
+  return true;
 }
 
 bool ThumbnailDatabase::DeleteFaviconBitmap(FaviconBitmapID bitmap_id) {
@@ -634,10 +682,11 @@ favicon_base::FaviconID ThumbnailDatabase::AddFavicon(
     const GURL& icon_url,
     favicon_base::IconType icon_type,
     const scoped_refptr<base::RefCountedMemory>& icon_data,
+    FaviconBitmapType type,
     base::Time time,
     const gfx::Size& pixel_size) {
   favicon_base::FaviconID icon_id = AddFavicon(icon_url, icon_type);
-  if (!icon_id || !AddFaviconBitmap(icon_id, icon_data, time, pixel_size))
+  if (!icon_id || !AddFaviconBitmap(icon_id, icon_data, type, time, pixel_size))
     return 0;
 
   return icon_id;