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
--- 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");
+ // First 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";