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..08c243302654831b017c1e1be25210b8ed1d9bfe 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 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 |
@@ -497,18 +503,42 @@ bool ThumbnailDatabase::GetFaviconBitmap( |
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
jkrcal
2017/06/09 16:38:38
Okay, removed (as this is not tested by unit-test,
|
+ |
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_bitm
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(), |
@@ -516,6 +546,7 @@ FaviconBitmapID ThumbnailDatabase::AddFaviconBitmap( |
} 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()); |
@@ -530,8 +561,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 +575,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,21 +585,42 @@ 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 e
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 6
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(); |
} |
@@ -637,7 +695,9 @@ favicon_base::FaviconID ThumbnailDatabase::AddFavicon( |
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; |