[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..5212268c67970cca9e0ea5897fd7185a7f1980e6 100644
--- a/components/history/core/browser/thumbnail_database.cc
+++ b/components/history/core/browser/thumbnail_database.cc
@@ -64,16 +64,22 @@ 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 on-demand bitmaps, for clearing old 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 standard (not on-demand) bitmaps.
 
 namespace {
 
+const int kFaviconUpdateLastRequestedAfterDays = 14;
+
 // For this database, schema migrations are deprecated after two
 // years.  This means that the oldest non-deprecated version should be
 // two years old or greater (thus the migrations to get there are
@@ -267,6 +273,26 @@ void DatabaseErrorCallback(sql::Connection* db,
     DLOG(FATAL) << db->GetErrorMessage();
 }
 
+void FillStatementForAddFaviconBitmap(
+    favicon_base::FaviconID icon_id,
+    const scoped_refptr<base::RefCountedMemory>& icon_data,
+    base::Time time,
+    const gfx::Size& pixel_size,
+    sql::Statement* statement) {
+  DCHECK(statement);
+  DCHECK(icon_id);
+  statement->BindInt64(0, icon_id);
+  if (icon_data.get() && icon_data->size()) {
+    statement->BindBlob(1, icon_data->front(),
+                        static_cast<int>(icon_data->size()));
+  } else {
+    statement->BindNull(1);
+  }
+  statement->BindInt64(2, time.ToInternalValue());
+  statement->BindInt(3, pixel_size.width());
+  statement->BindInt(4, pixel_size.height());
+}
+
 }  // namespace
 
 ThumbnailDatabase::IconMappingEnumerator::IconMappingEnumerator() {
@@ -506,19 +532,36 @@ FaviconBitmapID ThumbnailDatabase::AddFaviconBitmap(
     base::Time time,
     const gfx::Size& pixel_size) {

[pkotwicz, 2017/05/24 15:43:31]

Can you either:
- add an enum parameter with whether this is an "on demand bitmap"?
- add a parameter for the requested time

I am personally not a fan of FillStatementForAddFaviconBitmap()

sky@ might have a different opinion though.

[sky, 2017/05/24 18:20:15]

+1 to what Peter says.

[jkrcal, 2017/06/06 15:41:55]

Done.

   DCHECK(icon_id);
-  sql::Statement statement(db_.GetCachedStatement(SQL_FROM_HERE,
-      "INSERT INTO favicon_bitmaps (icon_id, image_data, last_updated, width, "
-      "height) VALUES (?, ?, ?, ?, ?)"));
-  statement.BindInt64(0, icon_id);
-  if (icon_data.get() && icon_data->size()) {
-    statement.BindBlob(1, icon_data->front(),
-                       static_cast<int>(icon_data->size()));
-  } else {
-    statement.BindNull(1);
-  }
-  statement.BindInt64(2, time.ToInternalValue());
-  statement.BindInt(3, pixel_size.width());
-  statement.BindInt(4, pixel_size.height());
+  // Standard bitmaps (in contrast to on-demand bitmaps below):
+  //  - 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.
+  sql::Statement statement(db_.GetCachedStatement(
+      SQL_FROM_HERE,
+      "INSERT INTO favicon_bitmaps (icon_id, image_data, last_updated, "
+      "width, height) VALUES (?, ?, ?, ?, ?)"));
+  FillStatementForAddFaviconBitmap(icon_id, icon_data, time, pixel_size,
+                                   &statement);
+
+  if (!statement.Run())
+    return 0;
+  return db_.GetLastInsertRowId();
+}
+
+FaviconBitmapID ThumbnailDatabase::AddOnDemandFaviconBitmap(
+    favicon_base::FaviconID icon_id,
+    const scoped_refptr<base::RefCountedMemory>& icon_data,
+    base::Time time,
+    const gfx::Size& pixel_size) {
+  // On-demand bitmaps (in contrast to standard bitmaps above):
+  //  - always have last_updated==0: last write time is not stored as they are
+  //    always expired and thus ready to be replaced by standard icons;
+  //  - keep track of last_requested: last read time is used for cache eviction.

[pkotwicz, 2017/05/24 15:43:31]

If a row has both non-zero |last_updated| and non-zero |last_requested| is it a
"standard bitmap" or an "on-demand bitmap"?

[jkrcal, 2017/06/06 15:41:55]

Oh, very good point. I changed other functions that update last_updated so that
they now reset last_requested to 0. Your situation should never happen. I also
added a DCHECK in GetFaviconBitmap().

+  sql::Statement statement(db_.GetCachedStatement(
+      SQL_FROM_HERE,
+      "INSERT INTO favicon_bitmaps (icon_id, image_data, last_requested, "
+      "width, height) VALUES (?, ?, ?, ?, ?)"));
+  FillStatementForAddFaviconBitmap(icon_id, icon_data, time, pixel_size,
+                                   &statement);
 
   if (!statement.Run())
     return 0;
@@ -555,14 +598,25 @@ bool ThumbnailDatabase::SetFaviconBitmapLastUpdateTime(
   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=?"));
+bool ThumbnailDatabase::TouchOnDemandFavicon(const GURL& icon_url,
+                                             base::Time time) {
+  // Look up the id of the icon.
+  sql::Statement id_statement(db_.GetCachedStatement(
+      SQL_FROM_HERE, "SELECT id FROM favicons WHERE url=?"));
+  id_statement.BindString(0, URLDatabase::GURLToDatabaseURL(icon_url));
+  if (!id_statement.Step())
+    return false;  // not cached
+  favicon_base::FaviconID icon_id = id_statement.ColumnInt64(0);
+

[pkotwicz, 2017/05/24 15:43:31]

Can you skip the SELECT and go straight to the UPDATE statement?

"WHERE url=?"

[jkrcal, 2017/06/06 15:41:55]

Not really because "url" is in the favicons table whereas I am updating the
favicon_bitmaps table below (and I cannot join two tables in an UPDATE
statement).

[pkotwicz, 2017/06/07 17:40:52]

I see now.

+  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, bitmap_id);
+  statement.BindInt64(1, icon_id);
+  base::Time max_time =
+      time - base::TimeDelta::FromDays(kFaviconUpdateLastRequestedAfterDays);

[pkotwicz, 2017/05/24 15:43:31]

Why the upper bound on |last_requested|? Is this to make the SQL query faster?

[jkrcal, 2017/06/06 15:41:55]

Yes, it is because of performance.

I did a small experiment. I run repeated transactions with one call to
TouchOnDemandFavicon each:
 - with this where clause: ~ 10000 transations / second
 - without this clause: ~ 100 transactions / second

[pkotwicz, 2017/06/07 17:40:52]

Thank you for adding the explanation

+  statement.BindInt64(2, max_time.ToInternalValue());
   return statement.Run();
 }