382813 - nsFaviconService shouldn't rely on icons in chrome://browser

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect, P3)

Description

[Robert Kaiser]

nsFaviconService.h hardcodes FAVICON_DEFAULT_URL to a chrome://browser URL even though this is toolkit code nowadays.
Either this should point to a toolkit icon, or the app/theme should be able to point it to the icon it wants.

Comment 1

[Asaf Romano (gone)]

Attachment: patch

Consider pinstripe's defaultFavicon its current bookmark-item image.

Comment 2

[(not reading, please use [e-mail] instead)]

Comment on attachment 267350 [details] [diff] [review]
patch

r=sspitzer

Comment 3

[Asaf Romano (gone)]

mozilla/toolkit/components/places/src/nsFaviconService.h 1.7
mozilla/toolkit/themes/pinstripe/mozapps/jar.mn 1.18
mozilla/toolkit/themes/pinstripe/mozapps/places/defaultFavicon.png initial revision: 1.1
mozilla/toolkit/themes/winstripe/mozapps/jar.mn 1.16
mozilla/toolkit/themes/winstripe/mozapps/places/defaultFavicon.png initial revision: 1.1
mozilla/browser/themes/pinstripe/browser/browser.css 1.49
mozilla/browser/themes/pinstripe/browser/jar.mn 1.43
mozilla/browser/themes/winstripe/browser/jar.mn 1.37
mozilla/browser/themes/pinstripe/browser/places/defaultFavicon.png delete
mozilla/browser/themes/winstripe/browser/places/defaultFavicon.png delete

Comment 4

[Alfred Kayser]

Why is favicon suddenly a toolkit thing?
Now themes that want to provide cross-version compatibility have to provide the icon to two locations...

Comment 5

[Asaf Romano (gone)]

Simply because the new favicon service happen to live in toolkit (as all places services).

Cross-version compatibility as in Fx2<->3? I assume you've got all the other differences between the legacy bookmarks & history UI to the places UI resolved...

Comment 6

[Alfred Kayser]

1. Ok.
2. Indeed.

Comment 7

[Justin Dolske [:Dolske]]

Attachment: post-landing fixup

The previous patch removed bookmarks-item.png from the JAR, but missed one use of it. As a result, on startup I get:

Chrome file doesn't exist: /Users/dolske/[...]/bookmark-item.png

This patch wraps the one use with %ifdef MOZ_PLACES, as was previously done elsewhere. I also removed the trailing |(filename.foo)| from a couple other lines in jar.mn,  because as far as I can see it's not needed.

Comment 8

[Asaf Romano (gone)]

Comment on attachment 270271 [details] [diff] [review]
post-landing fixup

r=mano, don't bother %ifdefing this though (bug 386392)

Comment 9

[Justin Dolske [:Dolske]]

Attachment: post-landing fixup (review nits fixed)

Comment 10

[Justin Dolske [:Dolske]]

Checking in browser/themes/pinstripe/browser/searchbar.css;
/cvsroot/mozilla/browser/themes/pinstripe/browser/searchbar.css,v  <--  searchbar.css
new revision: 1.17; previous revision: 1.16
done

Comment 11

[Ed Hume]

OK, you fixed Pinstripe. But the icon is still hard-coded in Winstripe. It needs fixing so we themers can style the thing.

Comment 12

[Asaf Romano (gone)]

I have no idea what that means. Where's the icon hardcoded in winstripe (and in what sense is that wrong, it's the faviconsvc that shouldn't hardcode the icon)? How does winstripe affect your theme? How would a change to the service only affect one theme.

Comment 13

[Alfred Kayser]

The reference is 'hardcoded' (even if using a '#define') in the .cpp code.
But that doesn't mean that a theme cannot change that icon (just change the image itself).

The downside is that themers need to known which references are 'hardcoded', and therefor need to be supported from the theme. It also means that tricks like -moz-image-region are not possible.

It would be good to have kind of a way to make these reference publicly visible (on dev.mozilla.org or such??).

Anyway the purpose of this bug was to move the 'favicon' to mozapps (as some apps do use the favicon service, but not the 'browser' application...) and that happened.

Comment 14

[Alfred Kayser]

oops, lxr told me that there is still a reference to the browser location:
/browser/components/places/content/demos/time.js, line 209 -- imgelt.setAttribute('src', 'chrome://browser/skin/places/defaultFavicon.png');

Whatever the current status of that 'demo', the reference is now plainly wrong and also needs to be fixed.

Comment 15

[Ed Hume]

This is where the image is hardcoded:

http://lxr.mozilla.org/mozilla/source/toolkit/components/places/src/nsFaviconService.h#128

#define FAVICON_DEFAULT_URL "chrome://mozapps/skin/places/defaultFavicon.png"

What this means is that I cannot provide for icon changes to show hover and select states - feedback I provide in all my themes. It is especially important in themes for people with vision impairments - and seven of my themes are adapted for visually impaired users.

By your moving the definition from the toolkit to a css file in the default theme, I can make the changes to the icons in my own themes.

Firefox is supposed to be customizable. Hard-coding images means that themers cannot customize the browser for our users. 

Please do for Winstripe and Gnomestripe what you did for Pinstripe: move the definition from toolkit to a skin css file.

Thank you.

Comment 16

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv