Bug 1401777 - don't use rich icons for tabs and remove crisp filters making hi res icons look blocky. r=Mardak

MozReview-Commit-ID: DcqxEme7hfx

--HG--
extra : rebase_source : edc6989f8e16a8dced8ed0903f5cbbcfe846a517

browser/base/content/browser.js

@@ -3701,7 +3701,8 @@ const DOMEventHandler = {
         break;
 
       case "Link:SetIcon":
-        this.setIcon(aMsg.target, aMsg.data.url, aMsg.data.loadingPrincipal, aMsg.data.requestContextID);
+        this.setIcon(aMsg.target, aMsg.data.url, aMsg.data.loadingPrincipal,
+                     aMsg.data.requestContextID, aMsg.data.canUseForTab);
         break;
 
       case "Link:AddSearch":
@@ -3720,7 +3721,7 @@ const DOMEventHandler = {
     return true;
   },
 
-  setIcon(aBrowser, aURL, aLoadingPrincipal, aRequestContextID) {
+  setIcon(aBrowser, aURL, aLoadingPrincipal, aRequestContextID = 0, aCanUseForTab = true) {
     if (gBrowser.isFailedIcon(aURL))
       return false;
 
@@ -3728,7 +3729,22 @@ const DOMEventHandler = {
     if (!tab)
       return false;
 
-    gBrowser.setIcon(tab, aURL, aLoadingPrincipal, aRequestContextID);
+    let loadingPrincipal = aLoadingPrincipal ||
+                           Services.scriptSecurityManager.getSystemPrincipal();
+    if (aURL) {
+      try {
+        if (!(aURL instanceof Ci.nsIURI)) {
+          aURL = makeURI(aURL);
+        }
+        PlacesUIUtils.loadFavicon(aBrowser, loadingPrincipal, aURL, aRequestContextID);
+      } catch (ex) {
+        Components.utils.reportError(ex);
+      }
+    }
+
+    if (aCanUseForTab) {
+      gBrowser.setIcon(tab, aURL, loadingPrincipal, aRequestContextID);
+    }
     return true;
   },
 

browser/base/content/tabbrowser.xml

@@ -980,18 +980,9 @@
           <![CDATA[
             let browser = this.getBrowserForTab(aTab);
             browser.mIconURL = aURI instanceof Ci.nsIURI ? aURI.spec : aURI;
-            let loadingPrincipal = aLoadingPrincipal
-              ? aLoadingPrincipal
-              : Services.scriptSecurityManager.getSystemPrincipal();
+            let loadingPrincipal = aLoadingPrincipal ||
+                                   Services.scriptSecurityManager.getSystemPrincipal();
             let requestContextID = aRequestContextID || 0;
-
-            if (aURI) {
-              if (!(aURI instanceof Ci.nsIURI)) {
-                aURI = makeURI(aURI);
-              }
-              PlacesUIUtils.loadFavicon(browser, loadingPrincipal, aURI, requestContextID);
-            }
-
             let sizedIconUrl = browser.mIconURL || "";
             if (sizedIconUrl != aTab.getAttribute("image")) {
               if (sizedIconUrl) {

browser/base/content/test/general/browser_discovery.js

@@ -1,3 +1,4 @@
+/* eslint-disable mozilla/no-arbitrary-setTimeout */
 var browser;
 
 function doc() {
@@ -93,23 +94,28 @@ function runRichIconDiscoveryTest() {
   let testCase = richIconDiscoveryTests[0];
   let head = doc().getElementById("linkparent");
 
-  // 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.
-  BrowserTestUtils.waitForCondition(() => {
-    return gBrowser.getIcon() != null;
-  }, "wait for icon load to finish", 100, 5)
-  .then(() => {
+  // Rich icons are not set as tab icons, so just check for the message.
+  (async function() {
+    let mm = window.messageManager;
+    let deferred = PromiseUtils.defer();
+    testCase.listener = function(msg) {
+      deferred.resolve(msg.data);
+    }
+    mm.addMessageListener("Link:SetIcon", testCase.listener);
+    try {
+      let data = await Promise.race([deferred.promise,
+                                     new Promise((resolve, reject) => setTimeout(reject, 1000))]);
+      is(data.canUseForTab, false, "Rich icons cannot be used for tabs");
       ok(testCase.pass, testCase.text);
-  })
-  .catch(() => {
+    } catch (ex) {
       ok(!testCase.pass, testCase.text);
-  })
-  .then(() => {
+    } finally {
+      mm.removeMessageListener("Link:SetIcon", testCase.listener);
+    }
     head.removeChild(head.getElementsByTagName("link")[0]);
     richIconDiscoveryTests.shift();
     richIconDiscovery(); // Run the next test.
-  });
+  })();
 }
 
 function richIconDiscovery() {

browser/base/content/test/general/browser_favicon_change.js

@@ -17,7 +17,8 @@ add_task(async function() {
         continue;
       }
       let imageVal = extraTab.getAttribute("image").replace(/#.*$/, "");
-      if (!imageVal) {
+      // Ignore chrome favicons set on the tab before the actual page load.
+      if (!imageVal || !imageVal.startsWith("http://example.org/")) {
         // The value gets removed because it doesn't load.
         continue;
       }

browser/components/uitour/test/browser_UITour_availableTargets.js

@@ -116,14 +116,11 @@ add_UITour_task(async function test_availableTargets_addUrlbarPageActionsAll() {
 
 function ok_targets(actualData, expectedTargets) {
   // Depending on how soon after page load this is called, the selected tab icon
-  // may or may not be showing the loading throbber.  Check for its presence and
-  // insert it into expectedTargets if it's visible.
-  let selectedTabIcon =
-    document.getAnonymousElementByAttribute(gBrowser.selectedTab,
-                                            "anonid",
-                                            "tab-icon-image");
-  if (selectedTabIcon && UITour.isElementVisible(selectedTabIcon))
-    expectedTargets.push("selectedTabIcon");
+  // may or may not be showing the loading throbber.  We can't be sure whether
+  // it appears in the list of targets, so remove it.
+  let index = actualData.targets.indexOf("selectedTabIcon");
+  if (index != -1)
+    actualData.targets.splice(index, 1);
 
   ok(Array.isArray(actualData.targets), "data.targets should be an array");
   is(actualData.targets.sort().toString(), expectedTargets.sort().toString(),

browser/modules/ContentLinkHandler.jsm

@@ -114,7 +114,9 @@ function setIconForLink(aIconInfo, aChromeGlobal) {
     "Link:SetIcon",
     { url: aIconInfo.iconUri.spec,
       loadingPrincipal: aIconInfo.loadingPrincipal,
-      requestContextID: aIconInfo.requestContextID });
+      requestContextID: aIconInfo.requestContextID,
+      canUseForTab: !aIconInfo.isRichIcon,
+    });
 }
 
 /*
@@ -149,7 +151,9 @@ function faviconTimeoutCallback(aFaviconLoads, aPageUrl, aChromeGlobal) {
       continue;
     }
 
-    if (icon.isRichIcon) {
+    // 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) {
         largestRichIcon = icon;
       }
@@ -159,17 +163,19 @@ function faviconTimeoutCallback(aFaviconLoads, aPageUrl, aChromeGlobal) {
   }
 
   // Now set the favicons for the page in the following order:
-  // 1. Set the preferred one if any, otherwise use the default one.
-  // 2. Set the best rich icon if any.
+  // 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) {
     setIconForLink(preferredIcon, aChromeGlobal);
   } else if (defaultIcon) {
     setIconForLink(defaultIcon, aChromeGlobal);
   }
 
-  if (largestRichIcon) {
-    setIconForLink(largestRichIcon, aChromeGlobal);
-  }
   load.timer = null;
   aFaviconLoads.delete(aPageUrl);
 }
@@ -204,12 +210,8 @@ function handleFaviconLink(aLink, aIsRichIcon, aChromeGlobal, aFaviconLoads) {
   if (!iconUri)
     return false;
 
-  // Extract the size type and width. Note that some sites use hi-res icons
-  // without specifying them as apple-touch or fluid icons.
+  // Extract the size type and width.
   let width = extractIconSize(aLink.sizes);
-  if (width >= FAVICON_RICH_ICON_MIN_WIDTH)
-    aIsRichIcon = true;
-
   let iconInfo = {
     iconUri,
     width,