Bug 1357664 - Don't delete all relations to expired icons when updating icons for a specific page. r=adw
MozReview-Commit-ID: JYHeuyvrJYp
--- a/toolkit/components/places/FaviconHelpers.cpp
+++ b/toolkit/components/places/FaviconHelpers.cpp
@@ -851,22 +851,23 @@ AsyncAssociateIconToPage::Run()
// to this page, and we should not expire the payloads we just added.
// For this, we use the expiration field as an indicator and remove relations
// based on it being elapsed. We don't remove orphan icons at this time since
// it would have a cost. The privacy hit is limited since history removal
// methods already expire orphan icons.
if (mPage.id != 0) {
nsCOMPtr<mozIStorageStatement> stmt;
stmt = DB->GetStatement(
- "DELETE FROM moz_icons_to_pages WHERE icon_id IN ( "
+ "DELETE FROM moz_icons_to_pages "
+ "WHERE icon_id IN ( "
"SELECT icon_id FROM moz_icons_to_pages "
"JOIN moz_icons i ON icon_id = i.id "
"WHERE page_id = :page_id "
"AND expire_ms < strftime('%s','now','localtime','start of day','-7 days','utc') * 1000 "
- ") "
+ ") AND page_id = :page_id "
);
NS_ENSURE_STATE(stmt);
mozStorageStatementScoper scoper(stmt);
rv = stmt->BindInt64ByName(NS_LITERAL_CSTRING("page_id"), mPage.id);
NS_ENSURE_SUCCESS(rv, rv);
rv = stmt->Execute();
NS_ENSURE_SUCCESS(rv, rv);
} else {
--- a/toolkit/components/places/tests/favicons/test_expire_on_new_icons.js
+++ b/toolkit/components/places/tests/favicons/test_expire_on_new_icons.js
@@ -1,15 +1,17 @@
/*
* Tests that adding new icons for a page expired old ones.
*/
add_task(function* test_replaceFaviconData_validHistoryURI() {
const TEST_URL = "http://mozilla.com/"
yield PlacesTestUtils.addVisits(TEST_URL);
+ const TEST_URL2 = "http://test.mozilla.com/"
+ yield PlacesTestUtils.addVisits(TEST_URL2);
let favicons = [
{
name: "favicon-normal16.png",
mimeType: "image/png",
expire: PlacesUtils.toPRTime(new Date(1)) // Very old.
},
{
@@ -26,18 +28,30 @@ add_task(function* test_replaceFaviconDa
for (let icon of favicons) {
let data = readFileData(do_get_file(icon.name));
PlacesUtils.favicons.replaceFaviconData(NetUtil.newURI(TEST_URL + icon.name),
data, data.length,
icon.mimeType,
icon.expire);
yield setFaviconForPage(TEST_URL, TEST_URL + icon.name);
+ if (icon.expire != 0) {
+ PlacesUtils.favicons.replaceFaviconData(NetUtil.newURI(TEST_URL + icon.name),
+ data, data.length,
+ icon.mimeType,
+ icon.expire);
+ yield setFaviconForPage(TEST_URL2, TEST_URL + icon.name);
+ }
}
// Only the second and the third icons should have survived.
Assert.equal(yield getFaviconUrlForPage(TEST_URL, 16),
TEST_URL + favicons[1].name,
"Should retrieve the 32px icon, not the 16px one.");
Assert.equal(yield getFaviconUrlForPage(TEST_URL, 64),
TEST_URL + favicons[2].name,
"Should retrieve the 64px icon");
+
+ // The expired icon for page 2 should have survived.
+ Assert.equal(yield getFaviconUrlForPage(TEST_URL2, 16),
+ TEST_URL + favicons[0].name,
+ "Should retrieve the expired 16px icon");
});
--- a/toolkit/components/places/tests/head_common.js
+++ b/toolkit/components/places/tests/head_common.js
@@ -877,19 +877,22 @@ function setFaviconForPage(page, icon) {
Services.scriptSecurityManager.getSystemPrincipal()
);
});
}
function getFaviconUrlForPage(page, width = 0) {
let pageURI = page instanceof Ci.nsIURI ? page
: NetUtil.newURI(new URL(page).href);
- return new Promise(resolve => {
+ return new Promise((resolve, reject) => {
PlacesUtils.favicons.getFaviconURLForPage(pageURI, iconURI => {
- resolve(iconURI.spec);
+ if (iconURI)
+ resolve(iconURI.spec);
+ else
+ reject("Unable to find an icon for " + pageURI.spec);
}, width);
});
}
function getFaviconDataForPage(page, width = 0) {
let pageURI = page instanceof Ci.nsIURI ? page
: NetUtil.newURI(new URL(page).href);
return new Promise(resolve => {