Bug 1409969 - The root domain icon should not be preferred if 2 icons for the same size exist. r=standard8
MozReview-Commit-ID: CnNNp4F6uyh
--- a/toolkit/components/places/FaviconHelpers.cpp
+++ b/toolkit/components/places/FaviconHelpers.cpp
@@ -430,24 +430,31 @@ FetchIconPerSpec(const RefPtr<Database>&
NS_ENSURE_SUCCESS(rv, rv);
int32_t hashIdx = PromiseFlatCString(aPageSpec).RFind("#");
rv = stmt->BindInt32ByName(NS_LITERAL_CSTRING("hash_idx"), hashIdx + 1);
NS_ENSURE_SUCCESS(rv, rv);
// Return the biggest icon close to the preferred width. It may be bigger
// or smaller if the preferred width isn't found.
bool hasResult;
+ int32_t lastWidth = 0;
while (NS_SUCCEEDED(stmt->ExecuteStep(&hasResult)) && hasResult) {
int32_t width;
rv = stmt->GetInt32(0, &width);
+ if (lastWidth == width) {
+ // We already found an icon for this width. We always prefer the first
+ // icon found, because it's a non-root icon, per the root ASC ordering.
+ continue;
+ }
if (!aIconData.spec.IsEmpty() && width < aPreferredWidth) {
// We found the best match, or we already found a match so we don't need
// to fallback to the root domain icon.
break;
}
+ lastWidth = width;
rv = stmt->GetUTF8String(1, aIconData.spec);
NS_ENSURE_SUCCESS(rv, rv);
}
return NS_OK;
}
/**
--- a/toolkit/components/places/tests/favicons/test_root_icons.js
+++ b/toolkit/components/places/tests/favicons/test_root_icons.js
@@ -5,17 +5,17 @@
* This file tests root icons associations and expiration
*/
add_task(async function() {
let pageURI = NetUtil.newURI("http://www.places.test/page/");
await PlacesTestUtils.addVisits(pageURI);
let faviconURI = NetUtil.newURI("http://www.places.test/favicon.ico");
PlacesUtils.favicons.replaceFaviconDataFromDataURL(
- faviconURI, SMALLPNG_DATA_URI.spec, 0, Services.scriptSecurityManager.getSystemPrincipal());
+ faviconURI, SMALLPNG_DATA_URI.spec, 0, systemPrincipal);
await setFaviconForPage(pageURI, faviconURI);
// Sanity checks.
Assert.equal(await getFaviconUrlForPage(pageURI), faviconURI.spec);
Assert.equal(await getFaviconUrlForPage("https://places.test/somethingelse/"),
faviconURI.spec);
// Check database entries.
@@ -46,20 +46,20 @@ add_task(async function() {
add_task(async function test_removePagesByTimeframe() {
// Add a visit in the past with no directly associated icon.
await PlacesTestUtils.addVisits({uri: "http://www.places.test/old/", visitDate: new Date(Date.now() - 86400000)});
let pageURI = NetUtil.newURI("http://www.places.test/page/");
await PlacesTestUtils.addVisits({uri: pageURI, visitDate: new Date(Date.now() - 7200000)});
let faviconURI = NetUtil.newURI("http://www.places.test/page/favicon.ico");
let rootIconURI = NetUtil.newURI("http://www.places.test/favicon.ico");
PlacesUtils.favicons.replaceFaviconDataFromDataURL(
- faviconURI, SMALLSVG_DATA_URI.spec, 0, Services.scriptSecurityManager.getSystemPrincipal());
+ faviconURI, SMALLSVG_DATA_URI.spec, 0, systemPrincipal);
await setFaviconForPage(pageURI, faviconURI);
PlacesUtils.favicons.replaceFaviconDataFromDataURL(
- rootIconURI, SMALLPNG_DATA_URI.spec, 0, Services.scriptSecurityManager.getSystemPrincipal());
+ rootIconURI, SMALLPNG_DATA_URI.spec, 0, systemPrincipal);
await setFaviconForPage(pageURI, rootIconURI);
// Sanity checks.
Assert.equal(await getFaviconUrlForPage(pageURI),
faviconURI.spec, "Should get the biggest icon");
Assert.equal(await getFaviconUrlForPage(pageURI, 1),
rootIconURI.spec, "Should get the smallest icon");
Assert.equal(await getFaviconUrlForPage("http://www.places.test/old/"),
@@ -92,14 +92,32 @@ add_task(async function test_removePages
Assert.equal(rows.length, 0, "There should be no icon entry");
});
add_task(async function test_different_host() {
let pageURI = NetUtil.newURI("http://places.test/page/");
await PlacesTestUtils.addVisits(pageURI);
let faviconURI = NetUtil.newURI("http://mozilla.test/favicon.ico");
PlacesUtils.favicons.replaceFaviconDataFromDataURL(
- faviconURI, SMALLPNG_DATA_URI.spec, 0, Services.scriptSecurityManager.getSystemPrincipal());
+ faviconURI, SMALLPNG_DATA_URI.spec, 0, systemPrincipal);
await setFaviconForPage(pageURI, faviconURI);
Assert.equal(await getFaviconUrlForPage(pageURI),
faviconURI.spec, "Should get the png icon");
});
+
+add_task(async function test_same_size() {
+ // Add two icons with the same size, one is a root icon. Check that the
+ // non-root icon is preferred when a smaller size is requested.
+ let data = readFileData(do_get_file("favicon-normal32.png"));
+ let pageURI = NetUtil.newURI("http://new_places.test/page/");
+ await PlacesTestUtils.addVisits(pageURI);
+
+ let faviconURI = NetUtil.newURI("http://new_places.test/favicon.ico");
+ PlacesUtils.favicons.replaceFaviconData(faviconURI, data, data.length, "image/png");
+ await setFaviconForPage(pageURI, faviconURI);
+ faviconURI = NetUtil.newURI("http://new_places.test/another_icon.ico");
+ PlacesUtils.favicons.replaceFaviconData(faviconURI, data, data.length, "image/png");
+ await setFaviconForPage(pageURI, faviconURI);
+
+ Assert.equal(await getFaviconUrlForPage(pageURI, 20),
+ faviconURI.spec, "Should get the non-root icon");
+});