Bug 1409969 - The root domain icon should not be preferred if 2 icons for the same size exist. r=standard8 draft
authorMarco Bonardo <mbonardo@mozilla.com>
Sat, 21 Oct 2017 17:45:24 +0200
changeset 684387 c3626cf3e9ced826a74a67f2ab59714029f84a23
parent 683863 d1e995c8640a191cd127e87273ec96cb2fabffa9
child 736852 56dcfe441741f550fe5cdbccf040bacd04e46bae
push id85603
push usermak77@bonardo.net
push dateSat, 21 Oct 2017 15:46:51 +0000
reviewersstandard8
bugs1409969
milestone58.0a1
Bug 1409969 - The root domain icon should not be preferred if 2 icons for the same size exist. r=standard8 MozReview-Commit-ID: CnNNp4F6uyh
toolkit/components/places/FaviconHelpers.cpp
toolkit/components/places/tests/favicons/test_root_icons.js
--- 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");
+});