463725 - Choose most appropriately sized site icon when favicon has multiple members

Status: RESOLVED FIXED

(Camino Graveyard :: General, defect)

Description

[Christopher Henderson]

The favicon for BBC News (http://news.bbc.co.uk/favicon.ico) has two members: a 32x32 icon and a 16x16 icon. Current behaviour results in the 32x32 icon being scaled down and used, rather than the 16x16 one. This results in a blurry favicon in all the places it is used. We should examine the icon sizes and use the one closest to 16x16, scaling only if necessary.

Comment 1

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

Attachment: new bbc site icon

Just in case it later vanishes, this is the new site icon they've recently? started using that has 32x32 and 16x16 members.

Comment 2

[Christopher Henderson]

Attachment: Fix v1.0

Fix to make SiteIconProvider examine all the members of a loaded site icon, if there is more than one. It will remove all but one of the members with widths greater than or equal to 16. For example, the attached BBC site icon has 16x16 and 32x32 members; SiteIconProvider will remove the 32x32 member. If a site icon has, say, 32x32, 64x64 and 128x128 members, SiteIconProvider will remove all but the 32x32 member.

This version uses a for loop to enumerate the members; I also have a functionally equivalent version which uses an NSEnumerator. The latter looks better in code, but is probably slower.

Comment 3

[Christopher Henderson]

Attachment: NSEnumerator version

Version of the fix that uses an NSEnumerator, for comparison.

Comment 4

[Christopher Henderson]

Attachment: Fix v1.1

Tighter version of v1.0. Removes a nil check.

Comment 5

[Christopher Henderson]

Attachment: NSEnumerator version

Version of Fix v1.1 that uses an NSEnumerator, for comparison.

Comment 6

[Chris Lawson (gone)]

Nits on both patches: space after the if, and if comments are complete sentences, they should follow standard conventions of English grammar (i.e., captialise them and end them with a period).

As I mentioned on IRC, it seems like we ought to handle the "smaller members" case too. Pseudocode:

if (16x16 exists)
  use it

else if (anything larger-than-16x16 exists)
  scale smallest of those down to 16x16 and use it

else if (only smaller exists)
  use it as-is


The patch does the first two but not the third, though I'm not sure whether we want to use smaller-than-16px icons as-is or scale them up. Scaling them up strikes me as a bad idea in most cases, but how many people are really going to be dumb enough to provide a favicon that's only, say, 8px square?

Comment 7

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

I'm not sure we really care enough about smaller-than-16 sizes, at least not for this patch, though I suppose we should check and see what Safari, Firefox, and WinIE do.

Comment 8

[Chris Lawson (gone)]

Smokey put up an 8x8 test icon here:

http://www.ardisson.org/smokey/moz/8px_site_icon.html

Safari 3.2 on 10.5 ignores the 8x8 entirely (as well as the default site icon on ardisson.org), probably because NSImage doesn't like it at all.

Camino trunk on 10.5 shows the default site icon for ardisson.org and ignores the 8x8 icon.

Firefox 3 (and 2) on 10.5 shows the 8x8 icon blown up to 16x16 with a bad anti-aliasing routine. (On Windows, Firefox 2 doesn't bother with the anti-aliasing and looks much better, at least in these screenshots: http://sc.nitroware.net/i/fx2favicon.png vs http://sc.nitroware.net/i/fx3favicon.png .)

One of our IRC denizens (who also provided the aforementioned screenshots) was kind enough to check IE 6 and 7 on Windows, both of which ignore the 8x8 icon and show the default site icon for ardisson.org instead. (At least, I think that's what I understood him to say. They definitely ignore the 8x8 icon.)

Smokey says Opera basically does what Firefox 2 does.

Based on the above, I think I'm OK with ignoring anything smaller than 16x16 entirely.

Comment 9

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

(In reply to comment #8)

> probably because NSImage doesn't like it at all.

Specifically, Preview complains that the file isn't valid or is corrupt when I try to open it.  According to GraphicConverter, 8x8 is a valid size for .ico files, but NSImage has our hands tied anyway.  Even if it didn't, I'd also be fine with ignoring the oddballs who are trying to use a pinprick for their site icon.

Comment 10

[Chris Lawson (gone)]

Comment on attachment 347115 [details] [diff] [review]
NSEnumerator version

Nits:

>+    // if there is more than one member of the favicon (for example, 16x16 and 32x32 members)
>+    // we want to have only one that is greater than or equal to 16x16.

Please make this comment, which is a complete sentence, follow standard rules of English grammar.

I'd also like this to make it clear that what's going to happen is that we'll discard all but the *smallest* of the members larger than 16x16 if multiple members exist. The code isn't quite as self-documenting as I'd like it to be (or maybe I'm just thick, but it took three readings of the loop for that to sink in when coupled with the comment as worded above).

>+    if([[faviconImage representations] count] > 1) {
>+      NSEnumerator* repEnumerator = [[faviconImage representations] objectEnumerator];

Note that I've reviewed the NSEnumerator version. As Stuart reminded me over on bug 338558, if we don't need the index, we should be using NSEnumerators rather than for loops.

Patch applies and compiles fine, and solves the reported problem, so r=me with those nits addressed.

On a related note, though, I'd like to see testcases with bugs like this if at all possible ("bugs like this" being those where existing testcases are insufficient to cover behaviours addressed by the patch). In this case, a simple HTML page that links to a favicon with 16x16, 32x32, and 64x64 members would work, along with one that has a favicon consisting only of members greater than 16x16 in size. (I'd do it myself but GraphicConverter doesn't seem to want to work with multiple-membered ICO files and Apple's Icon Composer only creates ICNS files.)

Comment 11

[Chris Lawson (gone)]

Oh yeah, and the "space after if" nit from comment 6 still applies, too.

Comment 12

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

(In reply to comment #10)
> greater than 16x16 in size. (I'd do it myself but GraphicConverter doesn't seem
> to want to work with multiple-membered ICO files and Apple's Icon Composer only
> creates ICNS files.)

I wonder if you could make the .icns and have GraphicConverter convert it?  For whatever reason I decided not to try that, and found ICOBundle (http://www.telegraphics.com.au/sw/).  

So here's http://www.ardisson.org/smokey/moz/siteiconsuite/ (which has all the testcases I've made for this bug).

Comment 13

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

(In reply to comment #12)
> So here's http://www.ardisson.org/smokey/moz/siteiconsuite/ (which has all the
> testcases I've made for this bug).

As cl noted last night, none of the icons I made exhibited this bug.  I even made sure that one of them had the largest image as the second icon in the file rather than the first, and that seemed to have no effect.  I'm not sure what is special about the BBC icon that causes this bug.

Comment 14

[Christopher Henderson]

Attachment: Reverse for loop

To ensure data integrity, the for loop now loops backwards. There will be no NSEnumerator version.

Comment 15

[Chris Lawson (gone)]

Comment on attachment 349906 [details] [diff] [review]
Reverse for loop

Looks even better, and avoids the issue of enumerating over something we're changing at the same time.

Comment 16

[Stuart Morgan]

Comment on attachment 349906 [details] [diff] [review]
Reverse for loop

>+      NSArray* reps = [faviconImage representations];
...
>+          [faviconImage removeRepresentation:bestRep];

You aren't changing reps, so this should be done with an enumerator and needn't be in reverse order. 

>+        if ((aWidth > 15.0) && (aWidth < [bestRep size].width)) {

The intent of this comparison would be more clear if you used something like 15.99 instead of 15.0.

Comment 17

[Christopher Henderson]

Attachment: Back to NSEnumerator

This is basically the last NSEnumerator version, with fixes from Stuart's comment. That version got r+, so I'm sending it straight to sr.

Comment 18

[Stuart Morgan]

Comment on attachment 351280 [details] [diff] [review]
Back to NSEnumerator

You lost your pre-paren spaces:

>+    if([[faviconImage representations] count] > 1) {
...
>+      while((aRep = [repEnumerator nextObject])) {
...
>+        if((aWidth > 15.99) && (aWidth < [bestRep size].width)) {

sr=smorgan with those fixed.

Comment 19

[Christopher Henderson]

Attachment: Patch for checkin

Patch for checkin, with Stuart's comments addressed.

Comment 20

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

Landed on cvs trunk; nice sharp BBC icon!

Comment 21

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

(In reply to comment #13)
> As cl noted last night, none of the icons I made exhibited this bug.  I even
> made sure that one of them had the largest image as the second icon in the
> file rather than the first, and that seemed to have no effect.  I'm not sure
> what is special about the BBC icon that causes this bug.

32x32 "Color model: RGB"
16x16 "Color model: Gray"

"How an Image Representation Is Chosen

When you tell an NSImage object to draw itself, it searches its list of image representations for the one that best matches the attributes of the destination device. In determining which image representation to choose, it follows a set of ordered rules that compare the color space, image resolution, bit depth, and image size to the corresponding values in the destination device. The rules are applied in the following order:

1. Choose an image representation whose color space most closely matches the color space of the device. If the device is color, choose a color image representation. If the device is monochrome, choose a gray-scale image representation."

http://developer.apple.com/library/mac/#documentation/Cocoa/Conceptual/CocoaDrawingGuide/Images/Images.html#//apple_ref/doc/uid/TP40003290-CH208-BCIGEIFF

Based on those two pieces of information and the new testcase http://www.ardisson.org/smokey/moz/siteiconsuite/16_32_64-v2.html, I think that was our bug.

The better way to have fixed that was probably to do

"You can change the order in which these rules are applied using the methods of NSImage. For example, if you want to invert the first and second rules, pass NO to the setPrefersColorMatch: method. Doing so causes NSImage to match the resolution before the color space."

though I'm not sure if we'd want to do that all the time (I can sort-of imagine people optimizing some sort of grey/mono member for some specialized use-case, perhaps mobile).

Worse, though, that method doesn't appear to actually solve the problem :-(  Commenting out hendy's code in SiteIconProvider and adding that method still causes us to display the fuzzy BBC icon and the 32 *in the location bar* (the tab bar displays the sharp BBC icon and the grey 16).

(See also bug 310630 comment 8, bug 310630 comment 9, bug 310630 comment 13, and bug 530249 comment 7.)

This bug is sad in 2012, btw, because our fix for it prevents NSImage from doing the right thing automatically in HiDPI mode :(