Bug 1453580 - Remove promiseFaviconLinkUrl and fix its consumers. r=Gijs
authorMarco Bonardo <mbonardo@mozilla.com>
Mon, 16 Apr 2018 18:24:06 +0200
changeset 414351 9923d93eddd88deb8a071b6cdf9e506b5a3152f2
parent 414350 081b471aa819c20892167f68e690f718c7e89604
child 414352 13f330a9dba13b5aad22721adae31023693b483a
push id62853
push usermak77@bonardo.net
push dateWed, 18 Apr 2018 19:58:23 +0000
treeherderautoland@9923d93eddd8 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersGijs
bugs1453580
milestone61.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1453580 - Remove promiseFaviconLinkUrl and fix its consumers. r=Gijs MozReview-Commit-ID: GrY8s3l71Mp
browser/modules/ReaderParent.jsm
services/sync/modules/SyncedTabs.jsm
services/sync/tests/unit/test_syncedtabs.js
toolkit/components/passwordmgr/content/passwordManager.js
toolkit/components/places/PlacesUtils.jsm
toolkit/components/reader/AboutReader.jsm
--- a/browser/modules/ReaderParent.jsm
+++ b/browser/modules/ReaderParent.jsm
@@ -17,26 +17,31 @@ ChromeUtils.defineModuleGetter(this, "UI
 const gStringBundle = Services.strings.createBundle("chrome://global/locale/aboutReader.properties");
 
 var ReaderParent = {
   // Listeners are added in nsBrowserGlue.js
   receiveMessage(message) {
     switch (message.name) {
       case "Reader:FaviconRequest": {
         if (message.target.messageManager) {
-          let faviconUrl = PlacesUtils.promiseFaviconLinkUrl(message.data.url);
-          faviconUrl.then(function onResolution(favicon) {
-            message.target.messageManager.sendAsyncMessage("Reader:FaviconReturn", {
-              url: message.data.url,
-              faviconUrl: favicon.pathQueryRef.replace(/^favicon:/, "")
-            });
-          },
-          function onRejection(reason) {
-            Cu.reportError("Error requesting favicon URL for about:reader content: " + reason);
-          }).catch(Cu.reportError);
+          try {
+            let preferredWidth = message.data.preferredWidth || 0;
+            let uri = Services.io.newURI(message.data.url);
+            PlacesUtils.favicons.getFaviconURLForPage(uri, iconUri => {
+              if (iconUri) {
+                iconUri = PlacesUtils.favicons.getFaviconLinkForIcon(iconUri);
+                message.target.messageManager.sendAsyncMessage("Reader:FaviconReturn", {
+                  url: message.data.url,
+                  faviconUrl: iconUri.pathQueryRef.replace(/^favicon:/, "")
+                });
+              }
+            }, preferredWidth);
+          } catch (ex) {
+            Cu.reportError("Error requesting favicon URL for about:reader content: " + ex);
+          }
         }
         break;
       }
 
       case "Reader:UpdateReaderButton": {
         let browser = message.target;
         if (message.data && message.data.isArticle !== undefined) {
           browser.isArticle = message.data.isArticle;
--- a/services/sync/modules/SyncedTabs.jsm
+++ b/services/sync/modules/SyncedTabs.jsm
@@ -48,22 +48,20 @@ XPCOMUtils.defineLazyGetter(this, "log",
 let SyncedTabsInternal = {
   /* Make a "tab" record. Returns a promise */
   async _makeTab(client, tab, url, showRemoteIcons) {
     let icon;
     if (showRemoteIcons) {
       icon = tab.icon;
     }
     if (!icon) {
-      try {
-        icon = (await PlacesUtils.promiseFaviconLinkUrl(url)).spec;
-      } catch (ex) { /* no favicon avaiable */ }
-    }
-    if (!icon) {
-      icon = "";
+      // By not specifying a size the favicon service will pick the default,
+      // that is usually set through setDefaultIconURIPreferredSize by the
+      // first browser window. Commonly it's 16px at current dpi.
+      icon = "page-icon:" + url;
     }
     return {
       type:  "tab",
       title: tab.title || url,
       url,
       icon,
       client: client.id,
       lastUsed: tab.lastUsed,
--- a/services/sync/tests/unit/test_syncedtabs.js
+++ b/services/sync/tests/unit/test_syncedtabs.js
@@ -187,18 +187,18 @@ add_task(async function test_clientWithT
     },
   });
 
   let clients = await SyncedTabs.getTabClients();
   equal(clients.length, 1);
   clients.sort((a, b) => { return a.name.localeCompare(b.name); });
   equal(clients[0].tabs.length, 1);
   equal(clients[0].tabs[0].url, "http://foo.com/");
-  // expect the default favicon (empty string) due to the pref being false.
-  equal(clients[0].tabs[0].icon, "");
+  // Expect the default favicon due to the pref being false.
+  equal(clients[0].tabs[0].icon, "page-icon:http://foo.com/");
   Services.prefs.clearUserPref("services.sync.syncedTabs.showRemoteIcons");
 });
 
 add_task(async function test_filter() {
   // Nothing matches.
   await configureClients({
     guid_desktop: {
       clientName: "My Desktop",
--- a/toolkit/components/passwordmgr/content/passwordManager.js
+++ b/toolkit/components/passwordmgr/content/passwordManager.js
@@ -113,52 +113,30 @@ function Shutdown() {
 }
 
 function setFilter(aFilterString) {
   filterField.value = aFilterString;
   FilterPasswords();
 }
 
 let signonsTreeView = {
-  // Keep track of which favicons we've fetched or started fetching.
-  // Maps a login origin to a favicon URL.
-  _faviconMap: new Map(),
   _filterSet: [],
-  // Coalesce invalidations to avoid repeated flickering.
-  _invalidateTask: new DeferredTask(() => {
-    signonsTree.treeBoxObject.invalidateColumn(signonsTree.columns.siteCol);
-  }, 10, 0),
   _lastSelectedRanges: [],
   selection: null,
 
   rowCount: 0,
   setTree(tree) {},
   getImageSrc(row, column) {
     if (column.element.getAttribute("id") !== "siteCol") {
       return "";
     }
 
     const signon = GetVisibleLogins()[row];
 
-    // We already have the favicon URL or we started to fetch (value is null).
-    if (this._faviconMap.has(signon.hostname)) {
-      return this._faviconMap.get(signon.hostname);
-    }
-
-    // Record the fact that we already starting fetching a favicon for this
-    // origin in order to avoid multiple requests for the same origin.
-    this._faviconMap.set(signon.hostname, null);
-
-    PlacesUtils.promiseFaviconLinkUrl(signon.hostname)
-      .then(faviconURI => {
-        this._faviconMap.set(signon.hostname, faviconURI.spec);
-        this._invalidateTask.arm();
-      }).catch(Cu.reportError);
-
-    return "";
+    return PlacesUtils.urlWithSizeRef(window, "page-icon:" + signon.hostname, 16);
   },
   getCellValue(row, column) {},
   getCellText(row, column) {
     let time;
     let signon = GetVisibleLogins()[row];
     switch (column.id) {
       case "siteCol":
         return signon.httpRealm ?
--- a/toolkit/components/places/PlacesUtils.jsm
+++ b/toolkit/components/places/PlacesUtils.jsm
@@ -1523,39 +1523,16 @@ var PlacesUtils = {
             resolve({ uri, dataLen, data, mimeType });
           } else {
             reject();
           }
         });
     });
   },
 
-  /**
-   * Gets the favicon link url (moz-anno:) for a given page url.
-   *
-   * @param aPageURL url of the page to lookup the favicon for.
-   * @resolves to the nsIURL of the favicon link
-   * @rejects if the given url has no associated favicon.
-   */
-  promiseFaviconLinkUrl(aPageUrl) {
-    return new Promise((resolve, reject) => {
-      if (!(aPageUrl instanceof Ci.nsIURI))
-        aPageUrl = NetUtil.newURI(aPageUrl);
-
-      PlacesUtils.favicons.getFaviconURLForPage(aPageUrl, uri => {
-        if (uri) {
-          uri = PlacesUtils.favicons.getFaviconLinkForIcon(uri);
-          resolve(uri);
-        } else {
-          reject("favicon not found for uri");
-        }
-      });
-    });
-  },
-
    /**
    * Returns the passed URL with a #size ref for the specified size and
    * devicePixelRatio.
    *
    * @param window
    *        The window where the icon will appear.
    * @param href
    *        The string href we should add the ref to.
--- a/toolkit/components/reader/AboutReader.jsm
+++ b/toolkit/components/reader/AboutReader.jsm
@@ -698,17 +698,20 @@ AboutReader.prototype = {
 
   _requestFavicon() {
     let handleFaviconReturn = (message) => {
       this._mm.removeMessageListener("Reader:FaviconReturn", handleFaviconReturn);
       this._loadFavicon(message.data.url, message.data.faviconUrl);
     };
 
     this._mm.addMessageListener("Reader:FaviconReturn", handleFaviconReturn);
-    this._mm.sendAsyncMessage("Reader:FaviconRequest", { url: this._article.url });
+    this._mm.sendAsyncMessage("Reader:FaviconRequest", {
+      url: this._article.url,
+      preferredWidth: 16 * this._win.devicePixelRatio
+    });
   },
 
   _loadFavicon(url, faviconUrl) {
     if (this._article.url !== url)
       return;
 
     let doc = this._doc;