Bug 1357664 - Don't delete all relations to expired icons when updating icons for a specific page. r=adw draft
authorMarco Bonardo <mbonardo@mozilla.com>
Wed, 19 Apr 2017 10:19:48 +0200
changeset 564985 d1ba003454b12a3fd688545a995c8f0c56bc95c7
parent 564706 3f9f6d6086b2d247831d1d03d530095bebd5a6b2
child 624875 932523d12d7e70bd8ef40d09ff2dd336564f0a45
push id54741
push usermak77@bonardo.net
push dateWed, 19 Apr 2017 08:42:47 +0000
reviewersadw
bugs1357664
milestone55.0a1
Bug 1357664 - Don't delete all relations to expired icons when updating icons for a specific page. r=adw MozReview-Commit-ID: JYHeuyvrJYp
toolkit/components/places/FaviconHelpers.cpp
toolkit/components/places/tests/favicons/test_expire_on_new_icons.js
toolkit/components/places/tests/head_common.js
--- 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 => {