Bug 1357664 - Don't delete all relations to expired icons when updating icons for a specific page. r=adw

MozReview-Commit-ID: JYHeuyvrJYp

--HG--
extra : rebase_source : d1ba003454b12a3fd688545a995c8f0c56bc95c7

toolkit/components/places/FaviconHelpers.cpp

@@ -856,12 +856,13 @@ AsyncAssociateIconToPage::Run()
     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);

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

@@ -5,6 +5,8 @@
 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 = [
     {
@@ -31,6 +33,13 @@ add_task(function* test_replaceFaviconData_validHistoryURI() {
                                             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.
@@ -40,4 +49,9 @@ add_task(function* test_replaceFaviconData_validHistoryURI() {
   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");
 });

toolkit/components/places/tests/head_common.js

@@ -884,9 +884,12 @@ function setFaviconForPage(page, icon, forceReload = true) {
 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 => {
+      if (iconURI)
         resolve(iconURI.spec);
+      else
+        reject("Unable to find an icon for " + pageURI.spec);
     }, width);
   });
 }