453221 - Autocomplete popup should have site favicons

Status: RESOLVED FIXED

(Camino Graveyard :: Location Bar & Autocomplete, enhancement)

Description

[Christopher Henderson]

Most websites have distinctive favicons, which makes searching for them in lists easier. The autocomplete popup currently has generic favicons beside each result.

STR: 1. Focus location bar 2. Begin typing in search query with >1 results

EB: List of sites from the history, with their respective favicons

CB: List of sites from the history, with generic site icons

Comment 1

[Chris Lawson (gone)]

Thanks for looking into this. I honestly never really noticed it, but this'll be really nice to have.

Comment 2

[Christopher Henderson]

Attachment: Fix v1.0

Patch to add favicons to the popup list. Grabs favicons from the SiteIconProvider. Provides generic website and file icons if there is no cached favicon.

Main thing to look at would be performance issues.

Comment 3

[Chris Lawson (gone)]

Comment on attachment 336428 [details] [diff] [review]
Fix v1.0

This basically got an r- on IRC, so I'm going to mark it obsolete pending a new patch.

Comment 4

[Christopher Henderson]

Attachment: Fix v1.1

Fixes problem discussed on IRC. We now return an icon every time, if we are in the icon column. Apparently the ternary operator requires the same type on each side of the colon, hence the casts.

Comment 5

[Chris Lawson (gone)]

Comment on attachment 336462 [details] [diff] [review]
Fix v1.1

>Index: camino/src/browser/AutoCompleteDataSource.mm
>===================================================================

>+    NSImage* cachedFavicon =[[SiteIconProvider sharedFavoriteIconProvider] favoriteIconForPage:result];

Nit: space after =. (I assume this was just a typo, as you did it everywhere else.)

General comment on performance: this doesn't seem to noticeably slow things down, but we should probably keep an eye out just the same.

It does seem a little strange to have a drop-down full of a mix of favicons and generic globe icons, though. Presumably, this is because a large number of sites in my history (I keep a year of history) have had their icons expire from the cache (see bug 346061, which will likely solve this problem). Our default for history is nine days, and I sort of doubt we have a lot of users who keep more than about a month of history. There are only 10 users on the cc list for that bug, and only two votes for it, so it's not like there's great demand for that fix.

All of which is a really long-winded way of saying that I think while it's not perfect, the behaviour here is better than without the patch, and we should get better behaviour for free once the other bug is fixed. r=me with the nit fixed.

Tagging Stuart for sr since pink has a bunch of stuff on his plate.

cl

Comment 6

[Stuart Morgan]

Comment on attachment 336462 [details] [diff] [review]
Fix v1.1

Rather than repeat the early-return ternary, and have the return path in the main body be completely different for the icon column vs the others, make result an |id| initialized with the appropriate type via ternary and handle the return for the icon the same way the existing code does.

The votes/cc on the icon bug don't really reflect how often we heard about that issue via other channels, but I'm still fine with landing this now. As for perf, I'm pretty sure that the table view is smart enough not to ask for rows that aren't visible yet, so I wouldn't expect a problem.

Comment 7

[Christopher Henderson]

Attachment: Fix v1.2

New patch with changes per comment 6. Method now gets icons for every local file, not just those in the cache. The SiteIconCache does not cache local file icons to disk; there might be a case for changing its code to achieve what this patch does. Since we now check first if the URL is local, there doesn't seem to be any performance loss.

Requesting another review, as these changes introduce new issues.

Comment 8

[Christopher Henderson]

Attachment: Fix v1.3

Found Bug 414272, which is related to this bug. Removed the local file favicon handling from the previous patch, in favour of fixing the other bug. Sending the patch back to review, but will check on IRC whether it can be sent straight to sr.

Comment 9

[Christopher Henderson]

Attachment: Fix v1.4

Fixed a typo that crept back in from an earlier version. Comments on the previous patch still apply.

Comment 10

[Stuart Morgan]

Comment on attachment 336825 [details] [diff] [review]
Fix v1.4

Sorry, a couple more small things I should have caught before.

>   NSImage*      mIconImage;     // owned
>+  NSImage*      mFileImage;     // owned

mIconImage is now a confusing name; let's change it to mGenericSiteIcon everywhere (and your new one mGenericFileIcon).

>+    nsCAutoString value;
>+    item->GetURL(value);
>+    NSString* valueString = [NSString stringWith_nsACString:value];

This isn't the value, so call the variables something URL related (url and urlString are fine, to be consistent with the other code).

>+    } else {
>+      if ([urlString hasPrefix:@"file://"])
>+        result = mFileImage;
>+    }

else if (...) {

sr=smorgan with those changes, since they are trivial. If you could spin a patch with those changes, just mark it sr+ when you post it.

Comment 11

[Christopher Henderson]

Attachment: Final Fix

Adds fixes from comment 10.

Comment 12

[Stuart Morgan]

Landed on CVS trunk.