Bug 1422289 - ContentLinkHandler should guess icon type from the extension when type is not defined. r=mak draft
authorEd Lee <edilee@mozilla.com>
Fri, 01 Dec 2017 14:11:08 -0800
changeset 707251 b8bf0ece83340db8d3da328e0234f2c1b7117ee7
parent 707235 e19b017880c8d71385aed453bacbe2b473757e90
child 742888 5f64d9b85583cda95f89c997501b363536800530
push id92055
push userbmo:edilee@mozilla.com
push dateTue, 05 Dec 2017 00:14:39 +0000
reviewersmak
bugs1422289
milestone59.0a1
Bug 1422289 - ContentLinkHandler should guess icon type from the extension when type is not defined. r=mak MozReview-Commit-ID: HfuqudW77jM
browser/base/content/test/favicons/browser_preferred_icons.js
browser/modules/ContentLinkHandler.jsm
--- a/browser/base/content/test/favicons/browser_preferred_icons.js
+++ b/browser/base/content/test/favicons/browser_preferred_icons.js
@@ -1,14 +1,17 @@
 /* Any copyright is dedicated to the Public Domain.
  * http://creativecommons.org/publicdomain/zero/1.0/ */
 
 const ROOT = "http://mochi.test:8888/browser/browser/base/content/test/favicons/";
 
 function waitIcon(url) {
+  // Make sure we don't miss out on an icon if it was previously used in a test
+  PlacesUtils.favicons.removeFailedFavicon(makeURI(url));
+
   // Because there is debounce logic in ContentLinkHandler.jsm to reduce the
   // favicon loads, we have to wait some time before checking that icon was
   // stored properly.
   return BrowserTestUtils.waitForCondition(
     () => {
       let tabIcon = gBrowser.getIcon();
       info("Found icon " + tabIcon);
       return tabIcon == url;
@@ -19,17 +22,18 @@ function waitIcon(url) {
 function createLinks(linkInfos) {
   return ContentTask.spawn(gBrowser.selectedBrowser, linkInfos, links => {
     let doc = content.document;
     let head = doc.getElementById("linkparent");
     for (let l of links) {
       let link = doc.createElement("link");
       link.rel = "icon";
       link.href = l.href;
-      link.type = l.type;
+      if (l.type)
+        link.type = l.type;
       if (l.size)
         link.setAttribute("sizes", `${l.size}x${l.size}`);
       head.appendChild(link);
     }
   });
 }
 
 add_task(async function setup() {
@@ -73,25 +77,90 @@ add_task(async function prefer_sized() {
       type: "image/x-icon"
     },
   ]);
   await promise;
   // Must have at least one test.
   Assert.ok(true, "The expected icon has been set");
 });
 
-add_task(async function prefer_ico() {
+add_task(async function prefer_last_ico() {
   let promise = waitIcon(ROOT + "icon2.ico");
   await createLinks([
     { href: ROOT + "icon.ico",
       type: "image/x-icon"
     },
     { href: ROOT + "icon.png",
       type: "image/png",
     },
     { href: ROOT + "icon2.ico",
-    type: "image/x-icon"
-  },
+      type: "image/x-icon"
+    },
+  ]);
+  await promise;
+  // Must have at least one test.
+  Assert.ok(true, "The expected icon has been set");
+});
+
+add_task(async function fuzzy_ico() {
+  let promise = waitIcon(ROOT + "microsoft.ico");
+  await createLinks([
+    { href: ROOT + "icon.ico",
+      type: "image/x-icon"
+    },
+    { href: ROOT + "icon.png",
+      type: "image/png",
+    },
+    { href: ROOT + "microsoft.ico",
+      type: "image/vnd.microsoft.icon"
+    },
   ]);
   await promise;
   // Must have at least one test.
   Assert.ok(true, "The expected icon has been set");
 });
+
+add_task(async function guess_svg() {
+  let promise = waitIcon(ROOT + "icon.svg");
+  await createLinks([
+    { href: ROOT + "icon.svg" },
+    { href: ROOT + "icon.png",
+      type: "image/png",
+      size: 16 * Math.ceil(window.devicePixelRatio)
+    },
+    { href: ROOT + "icon.ico",
+      type: "image/x-icon"
+    },
+  ]);
+  await promise;
+  // Must have at least one test.
+  Assert.ok(true, "The expected icon has been set");
+});
+
+add_task(async function guess_ico() {
+  let promise = waitIcon(ROOT + "icon.ico");
+  await createLinks([
+    { href: ROOT + "icon.ico" },
+    { href: ROOT + "icon.png",
+      type: "image/png",
+    },
+  ]);
+  await promise;
+  // Must have at least one test.
+  Assert.ok(true, "The expected icon has been set");
+});
+
+add_task(async function guess_invalid() {
+  let promise = waitIcon(ROOT + "icon.svg");
+  // Create strange links to make sure they don't break us
+  await createLinks([
+    { href: ROOT + "icon.svg" },
+    { href: ROOT + "icon" },
+    { href: ROOT + "icon?.svg" },
+    { href: ROOT + "icon#.svg" },
+    { href: "data:text/plain,icon" },
+    { href: "file:///icon" },
+    { href: "about:icon" },
+  ]);
+  await promise;
+  // Must have at least one test.
+  Assert.ok(true, "The expected icon has been set");
+});
--- a/browser/modules/ContentLinkHandler.jsm
+++ b/browser/modules/ContentLinkHandler.jsm
@@ -23,16 +23,19 @@ const SIZES_TELEMETRY_ENUM = {
   ANY: 1,
   DIMENSION: 2,
   INVALID: 3,
 };
 
 const FAVICON_PARSING_TIMEOUT = 100;
 const FAVICON_RICH_ICON_MIN_WIDTH = 96;
 
+const TYPE_ICO = "image/x-icon";
+const TYPE_SVG = "image/svg+xml";
+
 /*
  * Create a nsITimer.
  *
  * @param {function} aCallback A timeout callback function.
  * @param {Number} aDelay A timeout interval in millisecond.
  * @return {nsITimer} A nsITimer object.
  */
 function setTimeout(aCallback, aDelay) {
@@ -115,20 +118,37 @@ function setIconForLink(aIconInfo, aChro
     { url: aIconInfo.iconUri.spec,
       loadingPrincipal: aIconInfo.loadingPrincipal,
       requestContextID: aIconInfo.requestContextID,
       canUseForTab: !aIconInfo.isRichIcon,
     });
 }
 
 /**
- * Checks whether the icon info represents an ICO image.
+ * Guess a type for an icon based on its declared type or file extension.
  */
-function isICO(icon) {
-  return icon.type == "image/x-icon" || icon.type == "image/vnd.microsoft.icon";
+function guessType(icon) {
+  // No type with no icon
+  if (!icon) {
+    return "";
+  }
+
+  // Use the file extension to guess at a type we're interested in
+  if (!icon.type) {
+    let extension = icon.iconUri.filePath.split(".").pop();
+    switch (extension) {
+      case "ico":
+        return TYPE_ICO;
+      case "svg":
+        return TYPE_SVG;
+    }
+  }
+
+  // Fuzzily prefer the type or fall back to the declared type
+  return icon.type == "image/vnd.microsoft.icon" ? TYPE_ICO : icon.type || "";
 }
 
 /*
  * Timeout callback function for loading favicon.
  *
  * @param {Map} aFaviconLoads A map of page URL and FaviconLoad object pairs,
  *   where the FaviconLoad object looks like {
  *     timer: a nsITimer object,
@@ -137,36 +157,34 @@ function isICO(icon) {
  * @param {String} aPageUrl A page URL string for this callback.
  * @param {Object} aChromeGlobal A global chrome object.
  */
 function faviconTimeoutCallback(aFaviconLoads, aPageUrl, aChromeGlobal) {
   let load = aFaviconLoads.get(aPageUrl);
   if (!load)
     return;
 
-  let preferredIcon = {
-    type: null
-  };
+  let preferredIcon;
   let preferredWidth = 16 * Math.ceil(aChromeGlobal.content.devicePixelRatio);
   // Other links with the "icon" tag are the default icons
   let defaultIcon;
   // Rich icons are either apple-touch or fluid icons, or the ones of the
   // dimension 96x96 or greater
   let largestRichIcon;
 
   for (let icon of load.iconInfos) {
     if (!icon.isRichIcon) {
       // First check for svg. If it's not available check for an icon with a
       // size adapt to the current resolution. If both are not available, prefer
       // ico files. When multiple icons are in the same set, the latest wins.
-      if (icon.type == "image/svg+xml") {
+      if (guessType(icon) == TYPE_SVG) {
         preferredIcon = icon;
-      } else if (icon.width == preferredWidth && preferredIcon.type != "image/svg+xml") {
+      } else if (icon.width == preferredWidth && guessType(preferredIcon) != TYPE_SVG) {
         preferredIcon = icon;
-      } else if (isICO(icon) && (preferredIcon.type == null || isICO(preferredIcon))) {
+      } else if (guessType(icon) == TYPE_ICO && (!preferredIcon || guessType(preferredIcon) == TYPE_ICO)) {
         preferredIcon = icon;
       }
     }
 
     // Note that some sites use hi-res icons without specifying them as
     // apple-touch or fluid icons.
     if (icon.isRichIcon || icon.width >= FAVICON_RICH_ICON_MIN_WIDTH) {
       if (!largestRichIcon || largestRichIcon.width < icon.width) {
@@ -180,17 +198,17 @@ function faviconTimeoutCallback(aFavicon
   // Now set the favicons for the page in the following order:
   // 1. Set the best rich icon if any.
   // 2. Set the preferred one if any, otherwise use the default one.
   // This order allows smaller icon frames to eventually override rich icon
   // frames.
   if (largestRichIcon) {
     setIconForLink(largestRichIcon, aChromeGlobal);
   }
-  if (preferredIcon.type) {
+  if (preferredIcon) {
     setIconForLink(preferredIcon, aChromeGlobal);
   } else if (defaultIcon) {
     setIconForLink(defaultIcon, aChromeGlobal);
   }
 
   load.timer = null;
   aFaviconLoads.delete(aPageUrl);
 }