Bug 1409969 - The root domain icon should not be preferred if 2 icons for the same size exist. r=standard8

MozReview-Commit-ID: CnNNp4F6uyh

--HG--
extra : rebase_source : c3626cf3e9ced826a74a67f2ab59714029f84a23

toolkit/components/places/FaviconHelpers.cpp

@@ -435,14 +435,21 @@ FetchIconPerSpec(const RefPtr<Database>& aDB,
   // 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);
   }

toolkit/components/places/tests/favicons/test_root_icons.js

@@ -10,7 +10,7 @@ add_task(async function() {
   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.
@@ -51,10 +51,10 @@ add_task(async function test_removePagesByTimeframe() {
   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.
@@ -97,9 +97,27 @@ add_task(async function test_different_host() {
   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");
+});