223180 - [patch] Load site icons for all bookmarks

Status: VERIFIED WONTFIX

(Camino Graveyard :: Bookmarks, enhancement)

Description

[S Woodside]

It would be nice if the bookmark manager would automatically load the site icons
for all the bookmarks automatically. I mean all of them, not just the ones
already in the cache. I think it would be a big usability boost for the
bookmarks system, to have complete site icons for everything.

Comment 1

[Jasper]

The major problem with this is that it will usage a lot of bandwith and cpu
cycles to do. Which is a no go for me. Keep it lean and mean.

Comment 2

[Ludovic Hirlimann [:Usul]]

The icons could be stored when you visit or bookmark the page. 

Comment 3

[Jasper]

Thats what's being done at the moment in the new bookmarks manager. But I think
we should take it out since in it renders camino inresponsive for the first
couple of seconds. This is no good untill this gone.

Comment 4

[S Woodside]

re comment #3: dave's already got a fix for that:  bug 223205

the icons could be loaded over time, the way the bookmarks are checked (maybe at
the same time?)

removing block on bug 223181, this is more a request, not a cleanup.

Comment 5

[S Woodside]

Attachment: load site icon at the same time as testing a bookmark

Simply added a call where David's code tests a bookmark for validity, also load
the site icon for the bookmark. This occurs every 120 seconds ... eventually it
will update all of the site icons.

Comment 6

[Ludovic Hirlimann [:Usul]]

confirming.

Comment 7

[Mike Pinkerton (not reading bugmail)]

|-refreshIconNetwork| should be renamed something like |-refreshItemUsingNetwork|.

also remove the NSLog. still wondering if this is something we want to do, since
the url checking code will do the right thing if we don't have a network, but
does this? will it cause us to try to go online if we're not?

Comment 8

[Stuart Morgan]

Attachment: Updated patch

I updated this patch to reflect pinkerton's comments, since I'm guessing
sbwoodside is not going to.

Comment 9

[Stuart Morgan]

Comment on attachment 144816 [details] [diff] [review]
Updated patch

I tried to see if this would cause connections, as pinkerton was asking, but it
turns out 10.3 broke auto-connect horribly.

We really need someone with 10.2 and a modem to test this by turning on
auto-connect, running Camino with a fresh profile, then disconnecting and
waiting for a few minutes. To see what happens.

Comment 10

[Jasper]

This patch would be great to have, the one issue that bugs averybody thought is
the fact that site icons get trached when we crash.

Bug 105843 handles the "Cache lost when mozilla crashes" issue. Let's hope they
fix it soon. It's one of those things that make this patch only semi good IMHO.

Comment 11

[Stuart Morgan]

Attachment: The patch again

Sorry, I just discovered that the patch I posted gives malformed patch errors.
This time I'm sure it works.

Comment 12

[Ludovic Hirlimann [:Usul]]

Comment on attachment 145224 [details] [diff] [review]
The patch again

I finnally had the time to test this corectly on a dial-up connection under
10.2.4. Camino aauto-connects to fetch the icons - this is why I giving the
review a -

Comment 13

[Ludovic Hirlimann [:Usul]]

adding sturt to the cc list so he can read my comment to the patch.

Comment 14

[Olivier]

(In reply to comment #0)
> It would be nice if the bookmark manager would automatically load the site icons
> for all the bookmarks automatically. 

Yes, but they DON'T necessary need to be reloaded EACH TIME from the Internet. 
Site icons usually don't change so often as page content, and it doesn't matter
so much if you have an older version in cache. It will be updated the next time
you check the bookmark (manually or automatically). 
This way, no problem if you are offline, travelling with your PowerBook, or
don't want to reconnect your modem!

> I mean all of them, not just the ones already in the cache. 

That's the point! A favorite icon gives a global information about a site, like
it's own URL, and should be treated differently than a common page or background
image. 
It should not be saved temporarily, but PERMANENTLY on the disk (with an
distinctive flag or in a separate profile directory; look for example to
~/Library/Safari/Icons). 
There is no problem of space, because there are usually very small files. 
It could also be included directly in a new XML-tag of the bookmarks.plist. 
So you could backup ALL your bookmarks AND site icons at the same time by
copying a single file. 

> I think it would be a big usability boost for the
> bookmarks system, to have complete site icons for everything.

I agree completely! It's the best way to find an element at the first look in a
very long list, and an important feature for user convenience and navigation
comfort. 
If a good solution is found here, it could apply not only to Camino, but to all
the Mozilla's browsers family. 
So, please vote for this bug or work on it!

Comment 15

[Stuart Morgan]

Olivier: see bug 117895 for the issue of putting favicons somewhere other than
the cache. This bug is specifically about extending the existing automatic
bookmark-checking functionality (currently off it the nightlies) to also get
favicons.

Comment 16

[Jasper]

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

Comment 17

[Mike Pinkerton (not reading bugmail)]

josh removed the bookmark checking code, so doing this will be much harder. I'll
future it rather than closing it, but I'm not a big fan of all the
behind-the-scenes network access.

Comment 18

[Simon Fraser [no longer active]]

I'll take this (though that doesn't mean I'll fix it).

Comment 19

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

The "new" site icon cache saves the favicons separately, so they don't get thrown out on crashes, and they're mapped to bookmarks and history entries.  As long as you visit the sites in your bookmarks at least once, this is now fixed.  The only thing the approach proposed in this bug would do now is a one-off fetch of "legacy" bookmarks' site icons, which is no reason to include it.