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 382419 9c0e1bf558e6ae89d69a2875d5afab381643bec9
parent 382418 6a0e5139dc99751856b446860814dea98a8eaa7a
child 382420 05ad53f12a1bc0128d58fbfa1c61da44774c6a5e
push id32558
push userkwierso@gmail.com
push dateFri, 22 Sep 2017 21:29:46 +0000
treeherdermozilla-central@61e58a7d800b [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersMardak
bugs1401777
milestone58.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 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)
   };