Backed out changeset 9c0e1bf558e6 (bug 1401777) for failing browser_discovery.js. r=backout
authorSebastian Hengst <archaeopteryx@coole-files.de>
Fri, 22 Sep 2017 17:16:25 +0200
changeset 382459 22e4997cd797715f1ff4221aeff5f5ace94bf9de
parent 382458 7bcda8f31f98a0d8c7be8e42f6a05336225f8401
child 382460 06df6fc905659bc84d14bc3bbf41df2ee75edbf8
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)
reviewersbackout
bugs1401777
milestone58.0a1
backs out9c0e1bf558e6ae89d69a2875d5afab381643bec9
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
Backed out changeset 9c0e1bf558e6 (bug 1401777) for failing browser_discovery.js. r=backout
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,18 +3696,17 @@ 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, aMsg.data.canUseForTab);
+        this.setIcon(aMsg.target, aMsg.data.url, aMsg.data.loadingPrincipal, aMsg.data.requestContextID);
         break;
 
       case "Link:AddSearch":
         this.addSearch(aMsg.target, aMsg.data.engine, aMsg.data.url);
         break;
 
       case "Meta:SetPageInfo":
         this.setPageInfo(aMsg.data);
@@ -3716,40 +3715,25 @@ const DOMEventHandler = {
   },
 
   setPageInfo(aData) {
     const {url, description, previewImageURL} = aData;
     gBrowser.setPageInfo(url, description, previewImageURL);
     return true;
   },
 
-  setIcon(aBrowser, aURL, aLoadingPrincipal, aRequestContextID = 0, aCanUseForTab = true) {
+  setIcon(aBrowser, aURL, aLoadingPrincipal, aRequestContextID) {
     if (gBrowser.isFailedIcon(aURL))
       return false;
 
     let tab = gBrowser.getTabForBrowser(aBrowser);
     if (!tab)
       return false;
 
-    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);
-    }
+    gBrowser.setIcon(tab, aURL, aLoadingPrincipal, 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,19 +975,28 @@
         <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 ||
-                                   Services.scriptSecurityManager.getSystemPrincipal();
+            let loadingPrincipal = aLoadingPrincipal
+              ? 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,9 +1,8 @@
-/* eslint-disable mozilla/no-arbitrary-setTimeout */
 var browser;
 
 function doc() {
   return browser.contentDocument;
 }
 
 function setHandlerFunc(aResultFunc) {
   gBrowser.addEventListener("DOMLinkAdded", function(event) {
@@ -89,38 +88,33 @@ 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");
 
-  // 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);
-    }
+  // 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(() => {
     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,18 +12,17 @@ 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(/#.*$/, "");
-      // Ignore chrome favicons set on the tab before the actual page load.
-      if (!imageVal || !imageVal.startsWith("http://example.org/")) {
+      if (!imageVal) {
         // 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,21 +111,24 @@ 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.  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);
+  // 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");
 
   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,19 +109,17 @@ 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,
-      canUseForTab: !aIconInfo.isRichIcon,
-    });
+      requestContextID: aIconInfo.requestContextID });
 }
 
 /*
  * 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,
@@ -146,41 +144,37 @@ 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;
     }
 
-    // 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 (icon.isRichIcon) {
       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 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);
-  }
+  // 1. Set the preferred one if any, otherwise use the default one.
+  // 2. Set the best rich icon if any.
   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.
@@ -205,18 +199,22 @@ 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.
+  // Extract the size type and width. Note that some sites use hi-res icons
+  // without specifying them as apple-touch or fluid icons.
   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)
   };