Last Comment Bug 323508 - Favicon (error icon) in Bookmark not updated
: Favicon (error icon) in Bookmark not updated
Status: RESOLVED FIXED
: fixed-seamonkey1.1.10
Product: SeaMonkey
Classification: Client Software
Component: Bookmarks & History (show other bugs)
: 1.8 Branch
: x86 Windows 98
: -- normal with 1 vote (vote)
: ---
Assigned To: neil@parkwaycc.co.uk
:
Mentors:
Depends on:
Blocks: 108809
  Show dependency treegraph
 
Reported: 2006-01-15 03:19 PST by Boris 'pi' Piwinger
Modified: 2008-04-07 06:51 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Proposed patch (1.20 KB, patch)
2008-04-04 02:38 PDT, neil@parkwaycc.co.uk
jag-mozilla: review+
jag-mozilla: superreview+
ajschult: approval‑seamonkey1.1.10+
Details | Diff | Splinter Review

Description Boris 'pi' Piwinger 2006-01-15 03:19:42 PST
Hi!

Most likely a dupe, but I don't find it. It appears in Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8) Gecko/20051219 SeaMonkey/1.0b and previous versions.

I have a bookmark for a group of tabs. When I created it, some icons where available which changed in the meantime. In particular, one page did not load and had the warning symbol (exclamation point in yellow triangle). Those icons were saved and are displayed in the bookmark manager.

Now icons changed, in particular the one with the warning. They are always displayed correctly in the tabs when I call the bookmark. The icons shown in the bookmark manager are never updated, though. There also seems to be no way of manually updating.

pi
Comment 1 Boris 'pi' Piwinger 2006-01-24 03:28:22 PST
To add to this bug, I now observe the following (using WinXP):

There seem to be (at least) three different (!) icon caches:

1) for icons in "Bookmars" drop-down menu

2) for bookmark manager

3) for personal toolbar

I actually obeserved, that icons in the toolbar don't show (when not visiting the links). If I go in the drop-down menu and "Personal Toolbar" gets expanded, there the links are populated with icons, but they don't show up in the toolbar itself.

Actually, all three different caches should only be one since they refer to the exact same set of bookmarks.

pi
Comment 2 Ray Booysen 2006-02-13 07:44:21 PST
Reassigning as per Bug #32644
Comment 3 Boris 'pi' Piwinger 2006-03-20 23:27:01 PST
I think the real problem is that the error icon is actually written in form of data: to bookmarks.html. The icon attribute is then not updated.

pi
Comment 4 Boris 'pi' Piwinger 2006-12-03 03:58:44 PST
In 1.1b this problem just went away. No clue which bug fixed it.

pi
Comment 5 Boris 'pi' Piwinger 2007-06-08 12:21:18 PDT
The bug is back in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.4) Gecko/20070509 SeaMonkey/1.1.2 (also in 1.1.1 already). The warning sign is never updated. To test simply make a page unavailable (e.g. use invalid proxy setting), see icon in bookmarks. Regularly load the page, icon is shown in address bar, bookmark icon is not updated.

Resolution would be not to save the icon explicitly in the bookmark file, I believe.

pi
Comment 6 neil@parkwaycc.co.uk 2008-04-04 02:35:16 PDT
I don't think that error icons should ever update bookmarks. The fact that they do can be traced to a combination of two bugs in the patch for bug 108809. The first is that the code in the onLinkIconAvailable handler always submits the icon to the bookmarks, even when the icon isn't actually available, although navigator.js already waits for the icon to load before submitting it to bookmarks. The second is that it determines the document's URI incorrectly.
Comment 7 neil@parkwaycc.co.uk 2008-04-04 02:38:48 PDT
Created attachment 313562 [details] [diff] [review]
Proposed patch
Comment 8 jag (Peter Annema) 2008-04-05 02:08:37 PDT
Comment on attachment 313562 [details] [diff] [review]
Proposed patch

So this code is redundant because |onload| and |onerror| on the page-proxy-favicon element (updated right above the code you're removing) already do the right thing through |HandleBookmarkIcon(...)|.

r+sr=jag

That mehod, btw, has a rather ugly if/else. Feel free to fix that with rs=jag
Comment 9 neil@parkwaycc.co.uk 2008-04-05 09:44:25 PDT
Comment on attachment 313562 [details] [diff] [review]
Proposed patch

This patch should be simple enough for the branch too (obviously when applied against its old location).
Comment 10 Andrew Schultz 2008-04-05 16:00:37 PDT
Comment on attachment 313562 [details] [diff] [review]
Proposed patch

a=ajschult
Comment 11 neil@parkwaycc.co.uk 2008-04-07 06:51:42 PDT
Fix checked in.

Note You need to log in before you can comment on or make changes to this bug.