Make favicon checks consistent between DOMLinkHandler and tabbrowser.shouldLoadFavIcon

NEW
Unassigned

Status

()

Firefox
Tabbed Browser
9 years ago
6 years ago

People

(Reporter: Wladimir Palant (for Adblock Plus info Cc bugzilla@adblockplus.org), Unassigned)

Tracking

(Blocks: 1 bug)

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

There are two ways for a page to set a favicon. One is explicit, by including a <link> tag in the page source (handled by DOMLinkHandler). The other is implicit, the image /favicon.ico is loaded (handled by tabbrowser.useDefaultIcon/tabbrowser.shouldLoadFavIcon). The checks performed in the two cases are very different:

1. tabbrowser requires both browser.chrome.site_icons and browser.chrome.favicons prefs to be set, DOMLinkHandler only considers browser.chrome.favicons. Supposedly, browser.chrome.site_icons is about all site icons (favicons and image thumbnails) while browser.chrome.favicons is only about favicons. Suggestion: stop using browser.chrome.favicons altogether, having two closely related but slightly different prefs makes little sense.

2. DOMLinkHandler uses security manager to verify that loading the favicon is restricted, tabbrowser simply restricts all favicons to http:// and https://. That probably makes sense given that for tabbrowser all favicons are from same domain so security manager will probably allow (is that always the case?)

3. DOMLinkHandler triggers content policies and allows the favicon to be blocked. tabbrowser doesn't do anything like this. The effect is that for example on heise.de you can "block" the favicon with Adblock Plus (that's the <link> tag) but it will still show up (because favicon.ico fallback still works and it bypasses the content policies). Obviously, tabbrowser should trigger content policies as well.

Ideally, the common parts of the favicon check should all be moved to tabbrowser.shouldLoadFavIcon() so that DOMLinkHandler can call it. The hack from bug 453442 might be a problem however.
Sorry, above it should say "DOMLinkHandler only considers browser.chrome.site_icons"

Links to the code:
http://hg.mozilla.org/mozilla-central/file/73fbb12f48e1/browser/base/content/browser.js#l2824
http://hg.mozilla.org/mozilla-central/file/73fbb12f48e1/browser/base/content/tabbrowser.xml#l678

Comment 2

9 years ago
(In reply to comment #0)
> 1. tabbrowser requires both browser.chrome.site_icons and
> browser.chrome.favicons prefs to be set, DOMLinkHandler only considers
> browser.chrome.site_icons. Supposedly, browser.chrome.site_icons is about all
> site icons (favicons and image thumbnails) while browser.chrome.favicons is
> only about favicons. Suggestion: stop using browser.chrome.favicons altogether,
> having two closely related but slightly different prefs makes little sense.

They are not _slightly_ different: browser.chrome.site_icons controls whether favicons should be displayed at all (although there are some bugs outstanding on history/bookmarks menu/sidebar not following this pref), and browser.chrome.favicons controls whether a /favicon.ico fetch should be attempted if a page does not contain an icon link; This is why you don't need to consider browser.chrome.favicons in DOMLinkHandler.

So, for instance, the polite combination is browser.chrome.site_icons set true and browser.chrome.favicons set false.
Ok, I guess the first point is a misunderstanding due to lack of proper documentation. Judging by a quick Google search this difference isn't properly explained anywhere but there are lots of misconceptions around.
You need to log in before you can comment on or make changes to this bug.