This bug is a reversal of bug 127349, which removed the partial favicon support.

This is Aleksey's patch from bug 113574.

Adding all CC's from 113574 that were added after 2002-03-04.

To: hewitt@netscape.com, ben@netscape.com, alecf@netscape.com You reviewed removing the favicons from the toolbar for 1.0, can you review putting them back in on the trunk (so that we can hope to make them work properly for 1.1)? Thanks a lot!

ok, I was asked to explain the "official thinking on this" a few things: 1) I am not the official thinker on this. 2) Personally I think the backend impl of favicons should be cleaned up first, to properly cache and so forth. 3) Some smart person (or even not so smart) could easily write up a dyanmic overlay, and an install script + .xpi that would overlay this functionality into mozilla, which would allow the average joe to try out favicon support until the backend implementation. 4) The most recent attachment, or one with a close resemblence was also attached to another bug, by someone else. I don't remember who, or what bug it was. You two should get together and write the .xpi.

oh never mind, now I see the bugmail. in any case, points 1-3 still stand.

> Personally I think the backend impl of favicons should be cleaned up first, > to properly cache and so forth. I already have a patch that adds a preference to always download favicons for personal toolbar, even if they are not in the cache - see attachment 76974 [details] [diff] [review] on bug 116832. I've been running with this patch for over a month now and it works pretty good. Is that sufficient? Can you review it? Thanks!

I don't think the always-download patch should be applied until the underlying cache problem is fixed. Basically, it's a band-aid that masks the real disease. I don't want to have to contact 50-some sites every time I open a new browser window!

> I don't think the always-download patch should be applied until the underlying > cache problem is fixed. Basically, it's a band-aid that masks the real disease. > > I don't want to have to contact 50-some sites every time I open a new browser > window! First, my experience shows that when a new window is opened, most of the icons (all of them, most of the time) are already cached and do not have to be requested again. Second, only the icons that are in the "top level" of the toolbar might get requested when a new window is opened, the submenu icons will get requested only when submenu is opened. Third, even when underlying problem (bug 120466?) is fixed, we'll still probably want the "always load" option - there always be some scenarious that would require some icons to be re-downloaded. Forth, the icons that are requested for the toolbar (as opposed to URL bar) seem to be cached properly, so, again, they end up being re-requested only when they in fact expire. P.S. The RPMs of Mozilla with my favicons patches (and bug 56301 spellchecker) are available at http://nogin.org/RPM/untested/moz/ByDate.html

<i>First, my experience shows that when a new window is opened, most of the icons (all of them, most of the time) are already cached and do not have to be requested again.</i> Not mine. On each of three Linux boxes (with very different software configurations) and on a Win98 system, I get only a few icons that stick in the cache -- and it's different on both systems. I definitely think your patch should go in. I just think the caching problem needs to be solved first.

I don't think "always reload" is the right solution *at all*. There are many permanently- or intermittently-disconnected users of mozilla, and they may *not* want mozilla to attempt network traffic on every startup. What about dial-up users who use mozilla to read pages on their hard disk? They don't want mozilla to dial up their ISP just to get a few dinky icons. That would *suck*. Also, I've got a network with a wireless connection, and many times the wireless is either out of range or I don't wish to use it for $$$ reasons. I would *hate* to have mozilla continually re-requesting the icons. A *much* better solution is to fix things so that these icons never leave the cache. They're small. That's the Right Thing To Do (IMHO). If the user has disk and memory caches set to 0, then the icons should Go Away until they are fetched, as they do now occasionally. We *certainly* should not fetch things willy-nilly, or at least not without waiting until a *legitimate* (i.e. user-requested*) network transactions occurs to open the connection and possibly dial out. I would strongly *oppose* an "always request" patch from *ever* being committed.

> Not mine. On each of three Linux boxes (with very different software > configurations) and on a Win98 system, I get only a few icons that stick in the > cache -- and it's different on both systems. Is that with my "always refetch" patch included and enabled, or without? If without, then yes; but with it, icons are much more likely to be properly cached (see "Fourth" in comment #8)... > I would strongly *oppose* an "always request" patch from *ever* being committed. You do not have to be - the patch adds a *preference* (and the current one has is default to keeping the old behavior), it's up to you to choose whether you want to enable it or not. In any case, please post all further opinions on the "always refertch" patch to bug 116832, where that patch leaves!

*** Bug 143341 has been marked as a duplicate of this bug. ***

*** Bug 145032 has been marked as a duplicate of this bug. ***

Wouldn't the intelligent thing, since they are stored in cache, and the cache is cleared fairly often (on crash etc), be to have Anther folder for favicon cache, seperate from the cache dir? ficache or something of the like. This way, when the cache is wiped out, the favicons stay.

Favicons also dont show in the top left hand corner by the pages title IS that bug filed?

Adam, see bug 117895. Alex, see bug 82130, bug 109037 and bug 112383

Updated to apply cleanly to the trunk.

*** Bug 149057 has been marked as a duplicate of this bug. ***

*** Bug 153000 has been marked as a duplicate of this bug. ***

------- Additional Comment #4 From Alec Flett 2002-05-12 16:48 ------- > Some smart person (or even not so smart) could easily write up a dynamic > overlay, and an install script + .xpi that would overlay this functionality > into mozilla, which would allow the average joe to try out favicon support > until the backend implementation. It would have to be a very smart person to dynamically overlay a template. Overlays work off ids which aren't allowed in templates...

*** Bug 161632 has been marked as a duplicate of this bug. ***

What I don't understand on this bug is: 1. Why to have yet another prefs which specifies wheter to download the Icons everytime or not. 2. Why not simple create a folder "siteicons" in the Cache folder, where the favicons are stored. 3. As the favicons are stored as a part or let's say together with the bookmark, why not simply use the same policy as for the bookmarks when you do: Bookmarks-> Manage Bookmarks -> (Select Bookmark) Properties -> Tab Schedule. So if the user sets a date under the Schedule tab to check for updates on this bookmark, the favicon gets updated at this date as well. Or if it is set to never the favicon gets also never updated. What's the deal ? We are just talking about an eye candy icon. And even IE has bugs on this issue, so if you delete Temporary Files and as well all Offline Contents the Favicons are gone also. So what could be worse than that ? Personally I really liked it when they were in the Mozilla trunk for the short while being, and I think even a not-so-perfect implementation on this, is better than no favicon support at all, because we're dealing with nothing serious here.

What I don't understand on this bug is: 1. Why to have yet another prefs which specifies wheter to download the Icons everytime or not. 2. Why not simple create a folder "siteicons" in the Cache folder, where the favicons are stored. 3. As the favicons are stored as a part or let's say together with the bookmark, why not simply use the same policy as for the bookmarks when you do: Bookmarks-> Manage Bookmarks -> (Select Bookmark) Properties -> Tab Schedule. So if the user sets a date under the Schedule tab to check for updates on this bookmark, the favicon gets updated at this date as well. Or if it is set to never the favicon gets also never updated. What's the deal ? We are just talking about an eye candy icon. And even IE has bugs on this issue, so if you delete Temporary Files and as well all Offline Contents the Favicons are gone also. So what could be worse than that ? Personally I really liked it when they were in the Mozilla trunk for the short while being, and I think even a not-so-perfect implementation on this, is better than no favicon support at all, because we're dealing with nothing serious here.

> Why to have yet another prefs which specifies wheter to download the Icons > everytime or not. Main reason - I wanted a temporary workaround until a "real" solution is implemented. Also, no matter what implementation we choose, (unless we store the icons themselves in the bookmarks.html file), there will be some scenarious (however unlikely) where the icon URL is in bookmark, but the icon is not available locally. It's nice to have an option to have the icons force-loaded in this case. > Why not simple create a folder "siteicons" in the Cache folder, where the > favicons are stored. OK, please code it up and attach your patch to bug 117895. > As the favicons are stored as a part or let's say together with the bookmark, > why not simply use the same policy as for the bookmarks when you do: > > Bookmarks-> Manage Bookmarks -> (Select Bookmark) Properties -> Tab Schedule. > > So if the user sets a date under the Schedule tab to check for updates on this > bookmark, the favicon gets updated at this date as well. Or if it is set to > never the favicon gets also never updated. Great. Please code it up and attach your patch to bug 117895.

Great, could I do it in Perl ? That's the main reason why I'm mostly just appearing as a bug reporter/commenter. I only know Perl, but I see that in the longterm I really need to learn C. I would also really like to hack on Mozilla, preventing me from doing so are 1. Lack of time in the moment 2. Lack of knowledge in C 3. I have not quite yet a full overview where what files are related to what. P.S. Sorry for the doulbe post, the second happened when I added myself to the CC list.

> Main reason - I wanted a temporary workaround until a "real" solution is > implemented. OK, understood > It's nice to have an option to have the icons force-loaded in this case. Can agree from this standpoint. Excuse me if I sounded rough, but I was a litte bit annoyed because favicon support was backed out because it could seem broken to the user by bug: http://bugzilla.mozilla.org/show_bug.cgi?id=127349 but then "Print Preview" was not backed out on Linux, which is horribly broken and even Netscape 7.0 ships with this really ugly bug. http://bugzilla.mozilla.org/show_bug.cgi?id=109970 2. I read http://bugzilla.mozilla.org/show_bug.cgi?id=117895 there someone was complaining about the problem when favicons are stored in a separate cache to sync his bookmarks between two pc's. How about, to name the favicons according to the URL, for example the favicon for www.google.com would be www-google-com.ico so in this way he still could easily sync between two PC's and favicons could reside in a seperate cache folder. However I'm aware that this has the drawback thate the same icon could be stored twice, once as www-google-com.ico and once as google-com.ico.

You misuderstand the issue. The issue is not figuring out *how* to implement it, the issue is to figure out *who* has time to work on it... Still, IMHO the only way to get it fixed would be to re-enable the favicons (however broken) early in the alpha cycle and hope that enough people will be annoyed by the brokenness, so that somebody actually does something. ;-)

How about making a section in "Preferences" where broken or not fully supported features can be enabled? A good candidate for this would be also RFE #33966. A big warning in this section and everything would be OK (for me and 50% of the users). People like me just want to have nice environments (icons are better than text in most situations). Sorry, if I appear impatient to You. :)

> How about making a section in "Preferences" where > broken or not fully supported features can be enabled? That's what the Debug section is for.

My current plan for this is to take the "always refresh" patch from bug 116832 and update it to create a preference browser.chrome.toolbar_icons with values 0 - never load (current behavior) 1 - when in cache (the way it worked before they were dropped) 2 - always load This way I hope to get the patch accepted with the default setting of "0" which would mean that by default the icons would not be in toolbar, which should remove the requirement of making sure they work properly before bringing them back. However those who want them and/or willing to work on favicons could set the pref to 1 or 2 and get their favicons back. Alec F, would that sufficiantly address your points in comment #4?

Um, since this is a "real" solution, could we hope, pretty please, for Preferences entry for these "0", "1", and "2" options so that *anyone* can set these options, not just those who will hack .js files? Thank you. Faviconless in Chicago. :) -- Marek P.s. This pertains to comment #30.

I strongly support making it in a optional way, otherwise I don't see favicons coming back in bookmakrs never.

This patch does not change the default behavior in any way (bu defaukt, there will be no site icons in toolbar), but adds a "hidden" pref "browser.chrome.load_toolbar_icons" that would alow people to turn on the icons in toolbar as described in comment #30. To enable the site/fav icons in toolbar, make sure you have "Show Web Site Icons" turned on in "Preferences -> Appearance" and add user_pref("browser.chrome.favicons", true); user_pref("browser.chrome.load_toolbar_icons",2); line to your prefs.js (you could also use 1 instead of 2 if you want the icons to appear only when they are already cached - the way it used to work when icons wre in Mozilla). P.S. Re: comment 31: > Um, since this is a "real" solution, I am hoping this would be a temporary solution. Once icons in toolbar work satisfactory, I would imagine that we'll just keep the "Show Web Site Icons" for turing the icons on/off everywhere. > could we hope, pretty please, for Preferences > entry for these "0", "1", and "2" options Feel free to file a *separate* bug once this patch goes in. > so that *anyone* can set these options, > not just those who will hack .js files? It's really easy: 0) Exit Mozilla. 1) go to the profile directory on your disk. If you do not know where it is, look for the "prefs.js" file on disk. If you find several, take the most recently used/accessed. 2) Open the file in your favorite text editor (Notepad, vi, emacs, etc). 3) Add the two lines as described above (anywhere in the file). 4) Start Mozilla.

P.P.S. Please review.

Comment on attachment 101582 [details] [diff] [review] Bring back the icons on a pref-controlled basis. I'm not a C++ hacker, but... >+ PRInt32 toolbarIcons=0; >+ prefServ->GetIntPref("browser.chrome.load_toolbar_icons", &toolbarIcons); >+ if (toolbarIcons>0) { >+ prefServ->GetBoolPref("browser.chrome.site_icons", &mBrowserIcons); Indentation looks wrong here. Also you should have spaces around = and > signs. >+ mAlwaysLoadIcons=(toolbarIcons>1); >+ } else >+ mAlwaysLoadIcons=mBrowserIcons=PR_FALSE; I don't think folders can have favicons: > <toolbarbutton type="menu" editable="true" class="bookmark-item" >+ image="rdf:http://home.netscape.com/NC-rdf#Icon" >+ validate="never" > <toolbarbutton type="menu" class="bookmark-item" uri="rdf:*" editable="true" > rdf:type="http://home.netscape.com/NC-rdf#Folder" >+ image="rdf:http://home.netscape.com/NC-rdf#Icon" >+ validate="never" > <menu class="menu-iconic bookmark-item" uri="rdf:*" > type="rdf:http://www.w3.org/1999/02/22-rdf-syntax-ns#type" >+ image="rdf:http://home.netscape.com/NC-rdf#Icon" >+ validate="never" > <menu class="menu-iconic bookmark-item" uri="rdf:*" > type="rdf:http://www.w3.org/1999/02/22-rdf-syntax-ns#type" >+ image="rdf:http://home.netscape.com/NC-rdf#Icon" >+ validate="never" > <menu class="menu-iconic bookmark-item" uri="rdf:*" >+ image="rdf:http://home.netscape.com/NC-rdf#Icon" >+ validate="never" > <menu class="menu-iconic bookmark-item" uri="rdf:*" >+ image="rdf:http://home.netscape.com/NC-rdf#Icon" >+ validate="never"

This patch updates for bitrot and fixes a few issues: > Indentation looks wrong here. Also you should have spaces around = and > signs. The original file already has highly inconsistent spaces/tabs indentation styles. However I tried making my patch more "locally consistent". I also fixed the spacing as requested. > I don't think folders can have favicons Well, here I am just restoring what used to be there. See http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/xpfe/browser/resources/content/navigator.xul&rev=1.366&mark=302,303,315,318,338,340,355,356#302 and http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/xpfe/browser/resources/content/navigatorOverlay.xul&rev=1.247&mark=303,304,316,317#302 if interested. Also, bug 114692 requests it anyway, so I do not see a reason for not keeping it there.

Can somebody please review the attachment 102550 [details] [diff] [review] (bring back the icons making them controlled by a hiddent pref with default behavior set to off for now, so that unexperienced users do not see any of the related bugs)? It would be really great if we could check this in early in 1.3 cycle. Thanks a lot!

You should really ask hyatt. He seems to have solved this problem for Phoenix (or is that "hacked around"?).

Comment on attachment 102550 [details] [diff] [review] Bring back the icons on a pref-controlled basis. v2 Hyatt, could you please consider reviewing this patch? Thanks a lot!

You are keying mBrowserIcons off this toolbar pref, which is incorrect (i.e., you're setting it to false if the toolbar icon pref is 0). Phoenix displays favicons in the bookmarks tree view, and your toolbar pref should not disable the display of those icons. I'm unclear why you need a separate pref for the display of the toolbar icons. Can't you just key off the pref that's already there (and stored in mBrowserIcons). Basically all you should have to do to make this work is patch the XUL. YOu shouldn't need to touch any .cpp files.

> Phoenix displays favicons in the bookmarks tree view, > and your toolbar pref should not disable > the display of those icons. Wouldn't this mean that Phoenix should just have a different default value for this pref? > I'm unclear why you need a separate pref for the display of the > toolbar icons. Can't you just key off the pref that's already > there (and stored in mBrowserIcons). It was not my choice - please see the history of this bug (especially comments 4-11). As far as I understand, "official thinking" on this was that until the backend bugs are fixed, for the "oridinary" user (e.g. without manually editing prefs.js) icons should not appear in the toolbar, even if they are enabled in all the other places. My patch accomplishes exactly this - it makes sure they will only appear in toolbar if the general favicon support is enabled in the preferences *and* if the toolbar icons are specifically requested via a hidden pref.

We're not going to hold 1.3a for this. That doesn't mean that the fix won't be included in 1.3a. (The "wanted1.3a" flag has been changed to "blocking1.3a" to more accurately reflect how the flag is used by drivers@mozilla.org).

This does not seem to be making 1.3a :-( Can somebody please help me get this into 1.3b? This bug currently has 71 votes, the patch is there, the only thing that's missing is for somebody to decide how mozilla.org wants toolbar icons: enabled by default, disabled by default (even when enabled in URL bar and similar places), controlled by a hidden pref, or something else (and then to follow through on that decision with a review on an appropriate patch). CCing XP Apps people from the http://www.mozilla.org/owners.html page.

I'll all for bringing these back, but the caching problems need to be solved. Just enabling toolbar favicons but not having anyone working on that would be worse than leaving the whole thing disabled.

> I'll all for bringing these back, but the caching problems need to be solved. Some of those were solved (for Phoenix) and the patch also solves (or rather provides a pref) addresses some. > Just enabling toolbar favicons but not having anyone working on that would be > worse than leaving the whole thing disabled. The proposed patch would in fact "leaving the whole thing disabled". If will only add a hidden pref so that those people who really care and are willing to live with consequences can enable them. Let me re-iterate what my patch is doing. It has two parts - a FE part and a BE part. The FE part just adds the icons back. The BE part establishes a hidden pref that can take one of the three values: 0 (default) - disables the toolbar icons (even if the favicons are otherwise enabled and appear in URL bar, etc - as if the FE part of the patch just was not there). Setting the pref to 0 should be equivalent to not having this patch at all. 1 - FE part would work as if there is no BE part - eg. the icons would work exactly as they do in Phoenix. In other words, icons would be displayed in toolbar if an only if they are already cached. As soon as the icon is displayed, the cache entry will be marked "never expire" (this was added for Phoenix - not necessary the best possible solution, much much better than most alternatives), so any icon once displayed in toolbar would stay there. >=2 - "always download icons" - whenever an icon URI is known (e.g. in bookmarks file), the icon would be downloaded and displayed (if it's not known, but /favicon.ico happens to be in the cache, it will behave as in (1)). This is somewhat network-unfriendly, but great visually (unless there are some network problems you always have all your icons shown, and if some icon is missing, it tells you that the site is down). IOW, if you believe that toolbar icons are not ready to be enabled by default (in mozilla.org official builds), then please review attachment 102550 [details] [diff] [review] - it only adds a hidden pref and does not change Mozilla's default behavior. If you believe they are ready, then please review the FE (XUL) part of the attachment 102550 [review] and I'll be happy to attach a new patch that does not contain the BE part.

Comment on attachment 102550 [details] [diff] [review] Bring back the icons on a pref-controlled basis. v2 Hmm... which is correct, src= or image=?

> Hmm... which is correct, src= or image=? scr= for menuitem and image= for everything else? I do not know - for the FE part of the patch I've just put back exactly what was there before and the only thing that I know is that it works.

When a bookmarked page changes its title, do you want or expect your bookmark's title to change, too. Do you want or expect the bookmark's title to be tied to the page's title without you being able to change it? How about the description? Do you want it to be tied to the description meta-tag? Of course, in both cases the answer is "no". Why should icons be different? I don't think they should be. I think the icon is a static property of the bookmark, very much like the title (it's a kind of graphical title, after all). Once the bookmark has been added, its icon should not disappear, change or be redownloaded from the net unless explicitly requested by the user (and AFAIK it works like this in Internet Explorer). So IMHO, attachment 102550 [details] [diff] [review] is not a solution, nor is the dedicated favicon cache suggested elsewhere a solution. I think nothing that uses a cache can be a solution because a cache by definition is a temporary data store invisible to and inaccessible by the user. I think favicons should not be cached, they should be *stored*, stored in a user-visible and accessible way (see also bug 121348 which requests this, too). My patch is a first step in this direction. This is what it does: It splits up the currently existing Icon property from nsBookmarksService.cpp (which is reflected by the ICON attribute of a bookmark entry) into 2 properties. The URL stored in the ICON attribute becomes the SiteIcon property. It tracks the site's idea of what its icon should be just as it does now but it will not be used for displaying purposes. The Icon property is matched by a new attribute USERICON. This is the icon that is used for displaying and it never changes unless the user requests it. Because no one has USERICON attributes in his bookmarks.html, my patch has no user-visible effects. You won't get any icons, just as you don't get any icons from a Mozilla without a patch. In order to see something you will have to manually edit your bookmarks.html and add USERICON attributes. For an entry with a USERICON attribute you will get an icon - always. Of course, you should not put an http:// URL here, because then you'd cause Mozilla to download the icon from the net and (unless its on your own server) it could just change under your nose, so you'd have the same problems as with attachment 102550 [details] [diff] [review]. The USERICON attribute is meant to carry a file:// URL to a local file. I've written a bash script (see upcoming attachment) that will go through your bookmarks.html, download the favicons, store them and add an appropriate USERICON. TODO: 1. In the bookmark properties dialog, add an input field that shows the user icon's URL and allows changing it. The input field needs a button that will open a file picker for easy file selection. 2. The bookmark properties dialog needs another button, called "Download site icon" with the following functionality: if (bookmark has SiteIcon property (i.e. ICON attribute)) download favicon from the SiteIcon URL if (previous download failed || no SiteIcon property) download favicon.ico from same directory that bookmark's target URL points at (e.g. http://example.com/dir1/favicon.ico for a bookmark of http://example.com/dir1/index.html) if (previous download failed) download favicon.ico from top-level directory of the server the target URL points at (e.g. http://example.com/favicon.ico) if (download successful) update user icon input field with a path that points to a generated filename in the <profiledir>/favicons directory, preferably a descriptive name (my bash script currently uses "example-com!dir1!index-html.ico" for the above example); ---------------- if (user exits dialog with okay && download successful) { move the downloaded file to <profiledir>/favicons directory and store under the generated name (see above); update Icon property (i.e. USERICON attribute) with file:// URL that points to the icon file; } else if (download successful) remove downloaded temporary file 3. The Add Bookmark dialog must at least call the Re/Download function from 2. after the new bookmark has been added. It would be nice if it also offered a file picker to choose an icon directly when adding without having to go to the properties. 4. The Manage Bookmarks UI needs a menu function "Download missing icons" that goes through all bookmarks and for each bookmark that has no user icon (or one that points to a non-existent file), calls the Re/Download function from 2. NOTES: I've not mentioned deleting of icons above, because I'm not sure how it should be handled. Under no circumstances must Mozilla touch icons outside of the <profiledir>/favicons directory, so if automatic deleting of the old icon whenever the icon URL is changed is implemented it must check if the old URL is inside <profiledir>/favicons or not. However, it is possible that users want to store their own icons in <profiledir>/favicons, too. As I've said in the beginning, icons are stored, not cached, so unlike putting stuff into Mozilla's cache directory, which no one will expect to be save, putting your own icons into <profiledir>/favicons is reasonable. So while I think that it makes sense (and is expected by the user) to reuse the same generated name in <profiledir>/favicons when redownloading an icon (thereby overwriting the old one), I'm not sure if icons with non-generated names such as <profiledir>/favicons/myprettyicon.png which were put into <profiledir>/favicons by the user should be touched. For the time being it is probably the best to not ever delete icons automatically. Maybe the Manage Bookmarks UI can have a function "Remove unused icons" which pops up a confirm box asking "Remove icons from <profiledir>/favicons that are not assigned to any bookmark? Yes No" This function could be further restricted to touch only files with generated names. Okay, after this longish proposal, here's the bad news: I don't know Mozilla well enough to implement it :-(

After the bad news at the end of my previous comment, here's the good news: If you're a Unix user you can use my bash script to automatically download all favicons (with wget) and add USERICON attributes to bookmarks.html. Together with my Mozilla patch, this will give you nice off-line icons that don't disappear or change unless you want them to. I am certainly very happy with this. I don't add bookmarks very often and if I do I can just rerun the script (it will not redownload existing icons and won't try to download icons that have previously failed, so it doesn't take long to run). More information is in the comments at the beginning of the script.

OK, I would agree pretty much with this, yes I think too, though I comment to have a specific folder under <profile>/cache that this solution of having <profile>/siteicons is even better and much more as I would like it. Despite of that, I think you're handling the deleting of icons under <profile>/siteicons (I use siteicons because I think it reflects better the meaning as favicons and I don't like favicons because the name is just copied from Microsoft nothing own) to serious, it's really ok to not delete any icons above <profileDir>, but I think it's ok to delete the icons under siteicons. Because, hey we're just talking about a cute goodie, nothing really vital to the usage of Mozilla. And 3rd why I think it is better to delete the icons under <profile>/siteicons, is that better to delete an icon accidently (however being very small) than to pollute User's harddisk unnecessary with unused files. OK, you can say I'm to tidy with cleaning up my hardisk :-)

in response to comment 48, regarding storing the icon: i think mozilla's current self-contained bookmark file is ideal. rather than store the icon in a seperate file, we can use the xml data: protocol (already implemented) as shown in bug 121793. there is an example of an html document containing a image in attachment 66434 [details]. i suppose the only problem with this recommendation is that saving into the base64 format has not yet been done. ...that would be the first step towards solving bug 121793.

re comment 51: I don't like the idea of icons stored in the bookmark file. This makes matters more complicated for users as they have to fiddle with import and export functions instead of being able to just open an icon with Gimp to edit it. It also adds a lot of clutter to the bookmark file and probably makes it more difficult to process with home-grown scripts (as I assume the quite long icons would not be put into an attribute on a single line) not to speak of hand-editing bookmarks.html. I also like the ability to just use an image viewer in the favicons/ (or siteicons/, I don't care about the name) directory, especially since you can't see the higher resolutions often contained in icon files without accessing the file directly, since Mozilla always displays 16x16. Finally, I believe that someone with a little knowledge of Mozilla could implement my proposal over a weekend, whereas embedded icons seem more complicated.

Besides, before the XML data: protocol can be used, the bookmarks have to be made proper XHTML, which makes them incompatible with all other software that currently works with/imports Mozilla bookmarks.

Ooglyoog

Updated to apply to 2003-03-25 (and hopefully later; seems like nsBookmarksService.cpp gets reindented all the time)

Updated to apply cleanly to 1.4rc1

I hate to be a terrible party-pooper, especially with 96 (!) votes, but is the Mozilla-Suite UI actually being worked on anymore? My understanding (from the new roadmap) was that UI work was going mostly into Mozilla Firebird. If this works in Mozilla Firebird, do we need it in Moz-Suite? It could even be a good reason to use Firebird -- "this popular broken thing in Moz works right in Firebird." Although, I suppose the code is all volunteer stuff. Still, it's a point to consider. -M

I've read in the 1.4 release notes that mozilla now supports favicons in the bookmarks window. That seems to work, but they disappear when I restar mozilla. And what about the personal toolbar now? I still have no site icons there. Does this mean this bug is still unsolved?

10 out of 10 for observation!!

Comment on attachment 125548 [details] [diff] [review] Bring back the icons on a pref-controlled basis, v3 looks good, r=varga

Comment on attachment 125548 [details] [diff] [review] Bring back the icons on a pref-controlled basis, v3 PRBool mDirty; PRBool mBrowserIcons; + PRBool mAlwaysLoadIcons; PRBool busySchedule; ugh, looks like its time to start using PRPackedBool sr=alecf assuming this highly political issue of site icons is more or less fine with everyone.. and bonus points if you do some PRPackedBool conversion..

alecf: those PRBools are passed by reference, changing them means changing other stuff. We can file a new bug to do it.

Why isn't busySchedule named like a member (mBusySchedule)? /be

Comment on attachment 125548 [details] [diff] [review] Bring back the icons on a pref-controlled basis, v3 I do not have CVS access - can somebody check it in? Thanks a lot!

Fix was checked in by timeless.

Note that people who want to actually see the icons in toolbar, need to set user_pref("browser.chrome.load_toolbar_icons", 1); or user_pref("browser.chrome.load_toolbar_icons", 2); (1 means only show icons when they are cached, 2 means always load them).

I installed mozilla 1.6b with a new profile and found out that the pref setting for this is still a hidden one. Is there a specific reason for not having a pref UI for "browser.chrome.load_toolbar_icons"? We already have 'Show Web Site Icons' on the 'Appearance' pref panel. People might think that favicons for bookmarks are unsupported in mozilla, because of this hidden pref setting.

How about if we set browser.chrome.load_toolbar_icons to 1 by default?

That would be a good starter, but I would still prefer a pref UI for it.

Since this bug is marked fixed, I just opened up bug 228307 requesting the pref UI.

*** Bug 331219 has been marked as a duplicate of this bug. ***