Bug 1401777 - don't use rich icons for tabs and remove crisp filters making hi res icons look blocky. r=Mardak
☠☠ backed out by 22e4997cd797 ☠ ☠
authorMarco Bonardo <mbonardo@mozilla.com>
Thu, 21 Sep 2017 10:21:50 +0200
changeset 669081 9c0e1bf558e6ae89d69a2875d5afab381643bec9
parent 669080 6a0e5139dc99751856b446860814dea98a8eaa7a
child 669082 05ad53f12a1bc0128d58fbfa1c61da44774c6a5e
push id81210
push userkgupta@mozilla.com
push dateFri, 22 Sep 2017 14:09:59 +0000
reviewersMardak
bugs1401777
milestone58.0a1
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
browser/base/content/browser.js
browser/base/content/tabbrowser.xml
browser/base/content/test/general/browser_discovery.js
browser/base/content/test/general/browser_favicon_change.js
browser/components/uitour/test/browser_UITour_availableTargets.js
browser/modules/ContentLinkHandler.jsm
--- a/browser/base/content/browser.js
+++ b/browser/base/content/browser.js
@@ -3696,17 +3696,18 @@ const DOMEventHandler = {
   receiveMessage(aMsg) {
     switch (aMsg.name) {
       case "Link:AddFeed":
         let link = {type: aMsg.data.type, href: aMsg.data.href, title: aMsg.data.title};
         FeedHandler.addFeed(link, aMsg.target);
         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":
         this.addSearch(aMsg.target, aMsg.data.engine, aMsg.data.url);
         break;
 
       case "Meta:SetPageInfo":
         this.setPageInfo(aMsg.data);
@@ -3715,25 +3716,40 @@ const DOMEventHandler = {
   },
 
   setPageInfo(aData) {
     const {url, description, previewImageURL} = aData;
     gBrowser.setPageInfo(url, description, previewImageURL);
     return true;
   },
 
-  setIcon(aBrowser, aURL, aLoadingPrincipal, aRequestContextID) {
+  setIcon(aBrowser, aURL, aLoadingPrincipal, aRequestContextID = 0, aCanUseForTab = true) {
     if (gBrowser.isFailedIcon(aURL))
       return false;
 
     let tab = gBrowser.getTabForBrowser(aBrowser);
     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;
   },
 
   addSearch(aBrowser, aEngine, aURL) {
     let tab = gBrowser.getTabForBrowser(aBrowser);
     if (!tab)
       return;
 
--- a/browser/base/content/tabbrowser.xml
+++ b/browser/base/content/tabbrowser.xml
@@ -975,28 +975,19 @@
         <parameter name="aTab"/>
         <parameter name="aURI"/>
         <parameter name="aLoadingPrincipal"/>
         <parameter name="aRequestContextID"/>
         <body>
           <![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) {
                 if (!browser.mIconLoadingPrincipal ||
                     !browser.mIconLoadingPrincipal.equals(loadingPrincipal)) {
                   aTab.setAttribute("iconLoadingPrincipal",
                     this.serializationHelper.serializeToString(loadingPrincipal));
                   aTab.setAttribute("requestcontextid", requestContextID);
--- a/browser/base/content/test/general/browser_discovery.js
+++ b/browser/base/content/test/general/browser_discovery.js
@@ -1,8 +1,9 @@
+/* eslint-disable mozilla/no-arbitrary-setTimeout */
 var browser;
 
 function doc() {
   return browser.contentDocument;
 }
 
 function setHandlerFunc(aResultFunc) {
   gBrowser.addEventListener("DOMLinkAdded", function(event) {
@@ -88,33 +89,38 @@ let richIconDiscoveryTests = [
   { rel: "fluid-icon", text: "fluid-icon discovered" },
   { rel: "unknown-icon", pass: false, text: "unknown icon not discovered" }
 ];
 
 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(() => {
-    ok(testCase.pass, testCase.text);
-  })
-  .catch(() => {
-    ok(!testCase.pass, testCase.text);
-  })
-  .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 (ex) {
+      ok(!testCase.pass, testCase.text);
+    } finally {
+      mm.removeMessageListener("Link:SetIcon", testCase.listener);
+    }
     head.removeChild(head.getElementsByTagName("link")[0]);
     richIconDiscoveryTests.shift();
     richIconDiscovery(); // Run the next test.
-  });
+  })();
 }
 
 function richIconDiscovery() {
   if (richIconDiscoveryTests.length) {
     setHandlerFunc(runRichIconDiscoveryTest);
     gBrowser.setIcon(gBrowser.selectedTab, null,
                      Services.scriptSecurityManager.getSystemPrincipal()
     );
--- a/browser/base/content/test/general/browser_favicon_change.js
+++ b/browser/base/content/test/general/browser_favicon_change.js
@@ -12,17 +12,18 @@ add_task(async function() {
   let expectedFavicon = "http://example.org/one-icon";
   let haveChanged = PromiseUtils.defer();
   let observer = new MutationObserver(function(mutations) {
     for (let mut of mutations) {
       if (mut.attributeName != "image") {
         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;
       }
       is(imageVal, expectedFavicon, "Favicon image should correspond to expected image.");
       haveChanged.resolve();
     }
   });
   observer.observe(extraTab, {attributes: true});
--- a/browser/components/uitour/test/browser_UITour_availableTargets.js
+++ b/browser/components/uitour/test/browser_UITour_availableTargets.js
@@ -111,24 +111,21 @@ add_UITour_task(async function test_avai
   for (let [ targetName, expectedNodeId ] of expectedActions) {
     await assertTargetNode(targetName, expectedNodeId);
   }
   pageActionsHelper.restoreActionsUrlbarState();
 });
 
 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(),
      "Targets should be as expected");
 }
 
 async function assertTargetNode(targetName, expectedNodeId) {
   let target = await UITour.getTarget(window, targetName);
--- a/browser/modules/ContentLinkHandler.jsm
+++ b/browser/modules/ContentLinkHandler.jsm
@@ -109,17 +109,19 @@ function getLinkIconURI(aLink) {
  * }.
  * @param {Object} aChromeGlobal A global chrome object.
  */
 function setIconForLink(aIconInfo, aChromeGlobal) {
   aChromeGlobal.sendAsyncMessage(
     "Link:SetIcon",
     { url: aIconInfo.iconUri.spec,
       loadingPrincipal: aIconInfo.loadingPrincipal,
-      requestContextID: aIconInfo.requestContextID });
+      requestContextID: aIconInfo.requestContextID,
+      canUseForTab: !aIconInfo.isRichIcon,
+    });
 }
 
 /*
  * 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,
@@ -144,37 +146,41 @@ function faviconTimeoutCallback(aFavicon
   for (let icon of load.iconInfos) {
     if (icon.type === "image/svg+xml" ||
       icon.type === "image/x-icon" ||
       icon.type === "image/vnd.microsoft.icon") {
       preferredIcon = icon;
       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;
       }
     } else if (!defaultIcon) {
       defaultIcon = icon;
     }
   }
 
   // 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);
 }
 
 /*
  * Get request context ID of the link dom node's document.
  *
  * @param {DOMNode} aLink A link dom node.
@@ -199,22 +205,18 @@ function getLinkRequestContextID(aLink) 
  * @return {bool} Returns true if the link is successfully handled.
  */
 function handleFaviconLink(aLink, aIsRichIcon, aChromeGlobal, aFaviconLoads) {
   let pageUrl = aLink.ownerDocument.documentURI;
   let iconUri = getLinkIconURI(aLink);
   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,
     isRichIcon: aIsRichIcon,
     type: aLink.type,
     loadingPrincipal: aLink.ownerDocument.nodePrincipal,
     requestContextID: getLinkRequestContextID(aLink)
   };