[Thumbnails DB] Allow setting last_requested time when accessing favicons.

components/history/core/browser/thumbnail_database.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/history/core/browser/thumbnail_database.h"
 
 #include <stddef.h>
 #include <stdint.h>
 #include <algorithm>
 #include <string>
@@ skipping 46 matching lines @@
 //                    favicons. There is a separate row for every size in a
 //                    multi resolution bitmap. The bitmap data is associated
 //                    to the favicon via the |icon_id| field which matches
 //                    the |id| field in the appropriate row in the |favicons|
 //                    table.
 //
 //   id               Unique ID.
 //   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
+//                    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 {
 
+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
 // older).  Databases containing deprecated versions will be cleared
 // at startup.  Since this database is a cache, losing old data is not
 // fatal (in fact, very old data may be expired immediately at startup
 // anyhow).
 
 // Version 8: 982ef2c1/r323176 by rogerm@chromium.org on 2015-03-31
 // Version 7: 911a634d/r209424 by qsr@chromium.org on 2013-07-01
@@ skipping 403 matching lines @@
   }
 
   if (pixel_size) {
     *pixel_size = gfx::Size(statement.ColumnInt(2),
                             statement.ColumnInt(3));
   }
 
   if (last_requested)
     *last_requested = base::Time::FromInternalValue(statement.ColumnInt64(4));
 
+  // Checking timestamps: last_requested is set only for bitmaps of type
+  // ON_DEMAND, last_updated timestamp is set only for bitmaps of type ON_VISIT.
+  DCHECK(!last_requested || !last_updated || *last_requested == base::Time() ||
+         *last_updated == base::Time())
+      << "the favicon " << bitmap_id << " does not have consistent timestamps";

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

I am not a big fan of DCHECKs in general. The main value of DCHECKs is to check
behavior when running unit tests and browser tests

[jkrcal, 2017/06/09 16:38:38]

Okay, removed (as this is not tested by unit-test, currently).

+
   return true;
 }
 
 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,
+      type == ON_VISIT
+          // 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.
+          ? "INSERT INTO favicon_bitmaps (icon_id, image_data, last_updated, "
+            "width, height) VALUES (?, ?, ?, ?, ?)"
+          // 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.
+          : "INSERT INTO favicon_bitmaps (icon_id, image_data, last_requested, "
+            "width, height) VALUES (?, ?, ?, ?, ?)"));
+

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

Can you do this instead:

INSERT INTO favicon_bitmaps (icon_id, image_data, last_updated, last_requested,
width, height) VALUES (?, ?, ?, ?, ?, ?)

statement.BindInt(2, type == ON_VISIT ? time : 0);
statement.BindInt(3, type == ON_VISIT ? 0 : time);

[jkrcal, 2017/06/09 16:38:38]

Ah, sure, much better.

   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);
   }
+  // Both versions of the statement have a timestamp at index 2.
   statement.BindInt64(2, time.ToInternalValue());
   statement.BindInt(3, pixel_size.width());
   statement.BindInt(4, pixel_size.height());
 
   if (!statement.Run())
     return 0;
   return db_.GetLastInsertRowId();
 }
 
 bool ThumbnailDatabase::SetFaviconBitmap(
     FaviconBitmapID bitmap_id,
     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()));
   } else {
     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();
 }
 
 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=?"));
+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=?"));

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

Isn't it theoretically possible to have multiple entries in the favicons table
(one for each icon type) for |icon_url|. I think that this function should
handle this case

[jkrcal, 2017/06/09 16:38:38]

Done.

+  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);
+
+  // Update the time only for ON_DEMAND bitmaps (i.e. with last_requested > 0).
+  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);
+  // 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).

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

Nit: Merge this comment with the comment on line 611

[jkrcal, 2017/06/09 16:38:38]

Done.

+  base::Time max_time =
+      time - base::TimeDelta::FromDays(kFaviconUpdateLastRequestedAfterDays);
+  statement.BindInt64(2, max_time.ToInternalValue());
   return statement.Run();
 }
 
 bool ThumbnailDatabase::DeleteFaviconBitmap(FaviconBitmapID bitmap_id) {
   sql::Statement statement(db_.GetCachedStatement(SQL_FROM_HERE,
       "DELETE FROM favicon_bitmaps WHERE id=?"));
   statement.BindInt64(0, bitmap_id);
   return statement.Run();
 }
 
@@ skipping 54 matching lines @@
   return db_.GetLastInsertRowId();
 }
 
 favicon_base::FaviconID ThumbnailDatabase::AddFavicon(
     const GURL& icon_url,
     favicon_base::IconType icon_type,
     const scoped_refptr<base::RefCountedMemory>& icon_data,
     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, FaviconBitmapType::ON_VISIT, time,
+                        pixel_size))
     return 0;
 
   return icon_id;
 }
 
 bool ThumbnailDatabase::DeleteFavicon(favicon_base::FaviconID id) {
   sql::Statement statement;
   statement.Assign(db_.GetCachedStatement(SQL_FROM_HERE,
       "DELETE FROM favicons WHERE id = ?"));
   statement.BindInt64(0, id);
@@ skipping 407 matching lines @@
   meta_table_.SetVersionNumber(8);
   meta_table_.SetCompatibleVersionNumber(std::min(8, kCompatibleVersionNumber));
   return true;
 }
 
 bool ThumbnailDatabase::IsFaviconDBStructureIncorrect() {
   return !db_.IsSQLValid("SELECT id, url, icon_type FROM favicons");
 }
 
 }  // namespace history