Bug 1403508 - Tabbrowser is no more storing root favicons in Places for pages not defining an icon. r=Mardak
☠☠ backed out by 13dcb7178273 ☠ ☠
authorMarco Bonardo <mbonardo@mozilla.com>
Wed, 27 Sep 2017 14:43:17 +0200
changeset 383433 240bd9bd8b3eb4cda22fb19c3d6d23d2e5c523ce
parent 383432 c050d8574203c0344a9570fef73df51a1a489091
child 383434 f2df51e1bf8a5e58b95b7da60996eb92274e0a7a
push id32594
push userkwierso@gmail.com
push dateThu, 28 Sep 2017 22:49:33 +0000
treeherdermozilla-central@6dea0ee45b66 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersMardak
bugs1403508, 1401777
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 1403508 - Tabbrowser is no more storing root favicons in Places for pages not defining an icon. r=Mardak Setting an icon for the tab and storing that icon in Places are now separate actions. Before bug 1401777 setIcon was doing both, but that was error-prone and more expensive. Due to that change, useDefaultIcon stopped storing root domain favicons in Places. MozReview-Commit-ID: Kt5xEXctsnU
browser/base/content/browser.js
browser/base/content/tabbrowser.xml
browser/base/content/test/general/browser_discovery.js
--- a/browser/base/content/browser.js
+++ b/browser/base/content/browser.js
@@ -3719,26 +3719,18 @@ const DOMEventHandler = {
 
     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);
-      }
-    }
-
+      gBrowser.storeIcon(aBrowser, aURL, loadingPrincipal, aRequestContextID);
+    }
     if (aCanUseForTab) {
       gBrowser.setIcon(tab, aURL, loadingPrincipal, aRequestContextID);
     }
     return true;
   },
 
   addSearch(aBrowser, aEngine, aURL) {
     let tab = gBrowser.getTabForBrowser(aBrowser);
--- a/browser/base/content/tabbrowser.xml
+++ b/browser/base/content/tabbrowser.xml
@@ -966,16 +966,35 @@
         Cc["@mozilla.org/network/serialization-helper;1"]
           .getService(Ci.nsISerializationHelper);
       </field>
 
       <field name="mIconLoadingPrincipal">
         null
       </field>
 
+      <method name="storeIcon">
+        <parameter name="aBrowser"/>
+        <parameter name="aURI"/>
+        <parameter name="aLoadingPrincipal"/>
+        <parameter name="aRequestContextID"/>
+        <body>
+          <![CDATA[
+          try {
+            if (!(aURI instanceof Ci.nsIURI)) {
+              aURI = makeURI(aURI);
+            }
+            PlacesUIUtils.loadFavicon(aBrowser, aLoadingPrincipal, aURI, aRequestContextID);
+          } catch (ex) {
+            Components.utils.reportError(ex);
+          }
+        ]]>
+        </body>
+      </method>
+
       <method name="setIcon">
         <parameter name="aTab"/>
         <parameter name="aURI"/>
         <parameter name="aLoadingPrincipal"/>
         <parameter name="aRequestContextID"/>
         <body>
           <![CDATA[
             let browser = this.getBrowserForTab(aTab);
@@ -1042,38 +1061,45 @@
           ]]>
         </body>
       </method>
 
       <method name="useDefaultIcon">
         <parameter name="aTab"/>
         <body>
           <![CDATA[
-            var browser = this.getBrowserForTab(aTab);
-            var documentURI = browser.documentURI;
-            var icon = null;
+            let browser = this.getBrowserForTab(aTab);
+            let documentURI = browser.documentURI;
+            let requestContextID = browser.contentRequestContextID;
+            let loadingPrincipal = browser.contentPrincipal;
+            let icon = null;
 
             if (browser.imageDocument) {
               if (Services.prefs.getBoolPref("browser.chrome.site_icons")) {
                 let sz = Services.prefs.getIntPref("browser.chrome.image_icons.max_size");
                 if (browser.imageDocument.width <= sz &&
                     browser.imageDocument.height <= sz) {
+                  // Don't try to store the icon in Places, regardless it would
+                  // be skipped (see Bug 403651).
                   icon = browser.currentURI;
                 }
               }
             }
 
             // Use documentURIObject in the check for shouldLoadFavIcon so that we
             // do the right thing with about:-style error pages.  Bug 453442
             if (!icon && this.shouldLoadFavIcon(documentURI)) {
               let url = documentURI.prePath + "/favicon.ico";
-              if (!this.isFailedIcon(url))
+              if (!this.isFailedIcon(url)) {
                 icon = url;
-            }
-            this.setIcon(aTab, icon, browser.contentPrincipal, browser.contentRequestContextID);
+                this.storeIcon(browser, icon, loadingPrincipal, requestContextID);
+              }
+            }
+
+            this.setIcon(aTab, icon, loadingPrincipal, requestContextID);
           ]]>
         </body>
       </method>
 
       <method name="isFailedIcon">
         <parameter name="aURI"/>
         <body>
           <![CDATA[
--- a/browser/base/content/test/general/browser_discovery.js
+++ b/browser/base/content/test/general/browser_discovery.js
@@ -1,14 +1,16 @@
 /* eslint-disable mozilla/no-arbitrary-setTimeout */
 
 add_task(async function() {
-  let rootDir = getRootDirectory(gTestPath);
-  let url = rootDir + "discovery.html";
+  let url = "http://mochi.test:8888/browser/browser/base/content/test/general/discovery.html";
   info("Test icons discovery");
+  // Tirst we need to clear the failed favicons cache, since previous tests
+  // likely added this non-existing icon, and useDefaultIcon would skip it.
+  PlacesUtils.favicons.removeFailedFavicon(makeURI("http://mochi.test:8888/favicon.ico"));
   await BrowserTestUtils.withNewTab(url, iconDiscovery);
   info("Test search discovery");
   await BrowserTestUtils.withNewTab(url, searchDiscovery);
 });
 
 let iconDiscoveryTests = [
   { text: "rel icon discovered" },
   { rel: "abcdefg icon qwerty", text: "rel may contain additional rels separated by spaces" },
@@ -23,27 +25,33 @@ let iconDiscoveryTests = [
   { richIcon: true, rel: "apple-touch-icon", text: "apple-touch-icon discovered" },
   { richIcon: true, rel: "apple-touch-icon-precomposed", text: "apple-touch-icon-precomposed discovered" },
   { richIcon: true, rel: "fluid-icon", text: "fluid-icon discovered" },
   */
   { richIcon: true, rel: "unknown-icon", pass: false, text: "unknown icon not discovered" }
 ];
 
 async function iconDiscovery() {
+  // Since the page doesn't have an icon, we should try using the root domain
+  // icon.
+  await BrowserTestUtils.waitForCondition(() => {
+    return gBrowser.getIcon() == "http://mochi.test:8888/favicon.ico";
+  }, "wait for default icon load to finish");
+
   for (let testCase of iconDiscoveryTests) {
     if (testCase.pass == undefined)
       testCase.pass = true;
     testCase.rootDir = getRootDirectory(gTestPath);
 
     // Clear the current icon.
     gBrowser.setIcon(gBrowser.selectedTab, null);
 
     let promiseLinkAdded =
-      BrowserTestUtils.waitForEvent(gBrowser.selectedBrowser, "DOMLinkAdded",
-                                    false, null, true);
+      BrowserTestUtils.waitForContentEvent(gBrowser.selectedBrowser, "DOMLinkAdded",
+                                           false, null, true);
     let promiseMessage = new Promise(resolve => {
       let mm = window.messageManager;
       mm.addMessageListener("Link:SetIcon", function listenForIcon(msg) {
         mm.removeMessageListener("Link:SetIcon", listenForIcon);
         resolve(msg.data);
       });
     });
 
@@ -61,17 +69,17 @@ async function iconDiscovery() {
 
     if (!testCase.richIcon) {
       // 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.
       try {
         await BrowserTestUtils.waitForCondition(() => {
           return gBrowser.getIcon() != null;
-        }, "wait for icon load to finish", 100, 5);
+        }, "wait for icon load to finish", 100, 20);
         ok(testCase.pass, testCase.text);
       } catch (ex) {
         ok(!testCase.pass, testCase.text);
       }
     } else {
       // Rich icons are not set as tab icons, so just check for the SetIcon message.
       try {
         let data = await Promise.race([promiseMessage,
@@ -110,18 +118,18 @@ async function searchDiscovery() {
   let browser = gBrowser.selectedBrowser;
 
   for (let testCase of searchDiscoveryTests) {
     if (testCase.pass == undefined)
       testCase.pass = true;
     testCase.title = testCase.title || searchDiscoveryTests.indexOf(testCase);
 
     let promiseLinkAdded =
-      BrowserTestUtils.waitForEvent(gBrowser.selectedBrowser, "DOMLinkAdded",
-                                    false, null, true);
+      BrowserTestUtils.waitForContentEvent(gBrowser.selectedBrowser, "DOMLinkAdded",
+                                           false, null, true);
 
     await ContentTask.spawn(gBrowser.selectedBrowser, testCase, test => {
       let doc = content.document;
       let head = doc.getElementById("linkparent");
       let link = doc.createElement("link");
       link.rel = test.rel || "search";
       link.href = test.href || "http://so.not.here.mozilla.com/search.xml";
       link.type = test.type || "application/opensearchdescription+xml";
@@ -141,20 +149,19 @@ async function searchDiscovery() {
       ok(hasEngine, testCase.text);
       browser.engines = null;
     } else {
       ok(!testCase.pass, testCase.text);
     }
   }
 
   info("Test multiple engines with the same title");
-  let count = 0;
   let promiseLinkAdded =
-    BrowserTestUtils.waitForEvent(gBrowser.selectedBrowser, "DOMLinkAdded",
-                                  false, () => ++count == 2, true);
+    BrowserTestUtils.waitForContentEvent(gBrowser.selectedBrowser, "DOMLinkAdded",
+      false, e => e.target.href == "http://second.mozilla.com/search.xml", true);
   await ContentTask.spawn(gBrowser.selectedBrowser, null, () => {
     let doc = content.document;
     let head = doc.getElementById("linkparent");
     let link = doc.createElement("link");
     link.rel = "search";
     link.href = "http://first.mozilla.com/search.xml";
     link.type = "application/opensearchdescription+xml";
     link.title = "Test Engine";