Closed Bug 767661 Opened 12 years ago Closed 1 month ago

Fix for bug 463725 (BBC site icon) breaks NSImage's ability to automatically do the right thing in Retina/HiDPI situations

Categories

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

Product:
Camino Graveyard
Component:
Location Bar & Autocomplete
Platform:
All
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED INCOMPLETE

People

(Reporter: alqahira, Assigned: alqahira)

References

(
URL
)

Details

Attachments

(4 files)

Screenshot of the improved behavior with the patch
41.31 KB, image/png
Details
Patch that fixes everything but location bar
2.23 KB, patch
Details | Diff | Splinter Review
bbc icons - combined screenshot Low- and HiDPI
118.92 KB, image/png
Details
BBC icons on 10.4
35.54 KB, image/png
Details 
As detailed in a number of places (including bug 766015, bug 766418, and bug 463725 comment 21), the fix for the "BBC site icon bug" prevents NSImage from automatically choosing the most appropriately sized representation in Retina/HiDPI environments because we always delete >16px reps when we find a 16x16 rep.

As mentioned in the NSImage docs[1], we can "fix" NSImage's wacky choice for the BBC in the normal DPI cases by setting setPrefersColorMatch to NO and removing hendy's code in SiteIconProvider.

Based on testing, this fixes History and Bookmarks menus and manager collections, Bookmark Bar, the autocomplete window, and the tabs.

The one place where we don't do the right thing is the location bar itself.  It's unclear why; either something bizarre is specified in the nib, or PageProxyIcon.mm is doing something in code that it does not appear to do.  BrowserWrapper's updateSiteIconImage:withURI:status:[2] is the only entry point to setting the proxy icon, and it also sets the tab icon to the exact same image.

So it's unclear how one is wrong and the other is right; it will need more debugging, and it's not immediately clear to me how/where to do that.

The attached patch is a partial fix; it fixes 99% of the problems, except for the BBC (which is a default bookmark) on normal DPI Macs (which are currently the bulk of our users). Specifically,

On normal DPI Macs:
* Regular site icons should display correctly everywhere in the Camino UI (crisp, 16x16 representations used if one is available; otherwise, blurry, scaled-down, 32px reps)
* The BBC site icon and the site icon in the URL should use the crisp/16px representation everywhere in the Camino UI except the location bar.

On HiDPI Macs:
* Any site that has a 32px rep should find that rep used everywhere in the Camino UI, instead of a fuzzy, scaled-up 16px rep.

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

[2] http://mxr.mozilla.org/camino/source/camino/src/browser/BrowserWrapper.mm#1424
Note in the screenshot that the BBC site icon on the bg tab is crisp, and the active tab site icon is a greyscale "16" while the location bar is using the (color) "32", which is a blurry scaled-down 32px rep.

Unrelated to the attachment:

(In reply to comment #0)
> The one place where we don't do the right thing is the location bar itself. 
> It's unclear why; either something bizarre is specified in the nib, or
> PageProxyIcon.mm is doing something in code that it does not appear to do. 
> BrowserWrapper's updateSiteIconImage:withURI:status:[2] is the only entry
> point to setting the proxy icon, and it also sets the tab icon to the exact
> same image.

…and I also don't see anything in the nib that seems like it applies/would influence this situation.
So, apparently, I never attached the patch to comment 0.  Here's the patch that backs out hendy's code and sets setPrefersColorMatch:NO in SiteIconProvider, which fixes tabs (and everywhere *but* the location bar).
Subsequently, I also appear never to have commented on additional debugging I did; starting from BrowserWrapper's updateSiteIconImage:withURI:status: (where we set the tab icon directly and also start the process of setting the PageProxyIcon), I added setPrefersColorMatch:NO (and a setSize) to each subsequent method on the way to PageProxyIcon, in sequence.  None of the places helped.

I then finally implemented setImage: in PageProxyIcon and had that method do setPrefersColorMatch:NO and setSize: before calling [super setImage:] (and some logging to verify that we still have both reps in the image when it gets to PageProxyIcon), also to no avail:

- (void)setImage:(NSImage *)theImage
{
  NSLog(@"site icon: %@", theImage);//theImage, [theImage size].width);
  [theImage setPrefersColorMatch:NO];
  [theImage setSize:NSMakeSize(16,16)];
  [super setImage:theImage];
}

So I don't know if something in the default NSImageView implementation is overriding the things we've specified, or what's going on, but 

Some options here:

1) Wevah suggested rewriting the image to color (the downside being we'd still have to enumerate reps on every image)
2) Special-casing the BBC site icon, the only real-world example we know of--and the whole reason we fixed bug 463725, because the BBC is one of our default bookmarks (I guess we'd just do suggestion 1 on this image?)
3) <del>Scream insanely</del><ins>Ask nicely</ins> for the BBC to fix its site icon, making the 16x16 member color instead of grayscale
4) Ditch PageProxyIcon? (I'm not sure why we need it; aside from making the click select the location bar text, don't we have to do all of the same other things for the tab icons, which don't use PPI?  Still, probably more surgery than we want to do for this bug…)
5) Land the existing patch, and live with a partial regression of bug 463725 for the BBC site icon (only in the location bar) in exchange for making every 16px-32px site icon automatically work correctly on Retina Macs and no more head-bludgeoning for me.

At this point, there are much better things for me to spend my limited time on than beating my head against PageProxyIcon, but any suggestions or words of wisdom are welcome.
I happened upon http://stackoverflow.com/a/11340411 today, so I verified that our PageProxyIcon thingy really is 16x16; it is.

I also updated my mostly-logging implementation of setImage: in PageProxyIcon to:

- (void)setImage:(NSImage *)theImage
{
  NSLog(@"site icon: %@", theImage);//theImage, [theImage size].width);
  NSLog(@"bestRep before: %@", [theImage bestRepresentationForDevice:nil]);
  [theImage setPrefersColorMatch:NO];
  [theImage setSize:NSMakeSize(16,16)];
  NSLog(@"bestRep after: %@", [theImage bestRepresentationForDevice:nil]);
  [super setImage:theImage];
}

Interestingly, for the BBC site icon it logs:

site icon: NSImage 0x12d47f80 Size={16, 16} Reps=(\n    NSBitmapImageRep 0x12d37d30 Size={32, 32} ColorSpace=NSCalibratedRGBColorSpace BPS=8 BPP=32 Pixels=32x32 Alpha=YES Planar=NO Format=2 CGImage=0x12d1daf0,\n    NSBitmapImageRep 0x12d17ad0 Size={16, 16} ColorSpace=NSCalibratedWhiteColorSpace BPS=1 BPP=2 Pixels=16x16 Alpha=YES Planar=NO Format=2 CGImage=0x12d6bb90\n)

bestRep before: NSBitmapImageRep 0x12d17ad0 Size={16, 16} ColorSpace=NSCalibratedWhiteColorSpace BPS=1 BPP=2 Pixels=16x16 Alpha=YES Planar=NO Format=2 CGImage=0x12d6bb90

bestRep after: NSBitmapImageRep 0x12d17ad0 Size={16, 16} ColorSpace=NSCalibratedWhiteColorSpace BPS=1 BPP=2 Pixels=16x16 Alpha=YES Planar=NO Format=2 CGImage=0x12d6bb90

So NSImage really thinks it should be using the 16x16 icon in our PageProxyIcon NSImageView, but for reasons really unknown, that's not what we're seeing.  (Same results with my 3-part test icon; the BBC just generates less rep-logging to wade through.)

So, bug in NSImageView?

Can someone on 10.6+ try the patch in comment 2 and see if you get the right icon member displayed in the location bar there?  I'm less opposed to doing some sort of hack/keeping hendy's code only on older OS versions if it turns out this is an NSImageView bug fixed later on.
(In reply to comment #4)
> Can someone on 10.6+ try the patch in comment 2 and see if you get the right
> icon member displayed in the location bar there?  I'm less opposed to doing
> some sort of hack/keeping hendy's code only on older OS versions if it turns
> out this is an NSImageView bug fixed later on.

The test build in bug 825484 comment 6 includes this patch, for ease-of-testing.
I'm really curious if the right thing does happen in 1x mode in 10.6+ (or even 10.7+, since realistically that's the lower limit for HiDPI Macs), because if it does, either special-casing the BBC icon on ≤10.5 or running hendy's code only on ≤10.5 seems like the best way to fix this bug expediently.
Ok, so with the test build from bug 825484: the BBC icon displays fine without distortion (what  bug 463725 was fixing) – at least it is no worse than the Safari rendering. Most other favicons display OK, as far as I can tell (my ageing eyes are not improving and in @1x mode, I sometimes have trouble seeing details in those things). In @2x rendering, again most favicons are doing fine.

A couple of weird case: both NYT and CBO – in HiDPI mode (*) – Camino first fetches & displays the correctly sized icon, but upon following a link (on CBO, any one link in the navigation bar at the top of the page), the icon revert to a blown up, pixelated version of the small sized image. Follow another link and the dance reverts. Something similar to bug 530249.
I’ve no idea why, up front. On some of my own sites, which have similar 2-image favicons, I can’t repro that.

(*) if you very close attention, the same actually happens in lowDPI mode, it is harder to see, due to less distortion.

The CNN icon still looks crap (but nothing we can do about that…).

Mountain Lion.
If I have some energy & patience next week, I’ll try the Snow Leopard VM (redefinition of slow, that).
> A couple of weird case: both NYT and CBO – in HiDPI mode (*) – Camino first
> fetches & displays the correctly sized icon, but upon following a link (on
> CBO, any one link in the navigation bar at the top of the page), the icon
> revert to a blown up, pixelated version of the small sized image.

Hmm, hold on a moment. Are there multiple favicon files at the root of CBO? E.G a favicon.ico + an eventual .png file ? There appears to be a .htaccess redirect for the png.

I wonder if that might cause that weird dance.
1. First fetch of the site --> correct icon
2. after following a link, Camino appears to find a low res image from the root of the server, then try to correct with the real image but still takes the low-res image. Something like that ?
(In reply to philippe (part-time) from comment #7)
> Ok, so with the test build from bug 825484: the BBC icon displays fine
> without distortion (what  bug 463725 was fixing) – at least it is no worse
> than the Safari rendering. 

So BBC is equally crisp in 1x mode in both the tab and in the location bar?  Sorry, I should have been clear that the state of the BBC in the location bar in 1x was the data point I was most interested in, since that was the one place where it was broken here.
(In reply to philippe (part-time) from comment #8)
> Hmm, hold on a moment. Are there multiple favicon files at the root of CBO?
> E.G a favicon.ico + an eventual .png file ? There appears to be a .htaccess
> redirect for the png.

Yeah, we always used to redirect favicon.ico -> the png, I think because at the time NSImage couldn't handle transparency in ICO (it can't on 10.3 but can on 10.5; I'd have to check cb-x1 to see what the status on 10.4 is, but our hands are tied if we want a HiDPI-compatible site icon).

> I wonder if that might cause that weird dance.
> 1. First fetch of the site --> correct icon
> 2. after following a link, Camino appears to find a low res image from the
> root of the server, then try to correct with the real image but still takes
> the low-res image. Something like that ?

The Camino code always fetches favicon.ico first and displays it while it waits to see if there are any <link> elements.  It's then supposed to update all images to the <link>ed one (munged by bug 463725's code), but this turns out to be both buggy, bug 299270, and indeterminate, bug 530249.  Finally, the code saves the page URL <-> preferred site icon URL mapping and icon in the icon cache if there's a <link>ed site icon, so that on subsequent pageloads (and other uses in Camino, i.e. history and bookmarks) we always show the correct, <link>ed site icon immediately (until expiration).

In theory this sounds great, but as a practical result, for sites with <link>s, we re-run this dance on the first pageload of every page (and then again the first pageload if the icon/mapping ever expires from icon cache).  (We also don't deal with changes in site icon on the server while the icon is still in the icon cache, etc.)

The situation on cbo is this:
* /favicon.ico is currently redirected to /img/favicon.ico
* header.inc still has '<link rel="icon" href="/img/favicon.png" type="image/png" />'

The first one got changed with bug 766418 attachment 636026 [details] [diff] [review], but it looks like I missed the second one somehow :/

So what you've described seems consistent with how I'd expect the Camino code to act given the current broken state of things on cbo.  I'll fix header.inc later today and post back; I expect when you can test again after that, you'll see sane behavior from cbo.
(In reply to comment #10)
> header.inc later today and post back; I expect when you can test again after
> that, you'll see sane behavior from cbo.

header.inc is now fixed; either I somehow only missed that file in all of those changes, or this is another case of Sam not updating his tree before doing Amasis and blowing away that change, too :-P
(In reply to Smokey Ardisson (offline for a while; not following bugs - do not email) from comment #9) 
> So BBC is equally crisp in 1x mode in both the tab and in the location bar? 
> Sorry, I should have been clear that the state of the BBC in the location
> bar in 1x was the data point I was most interested in, since that was the
> one place where it was broken here.

Yes, it displays fine in both modes. In HiDP mode it is a bit weird (type esp), but no different from the Safari rendering.
Bonus in the screenshot: the CBO icon.
(In reply to Smokey Ardisson (offline for a while; not following bugs - do not email) from comment #10)
> So what you've described seems consistent with how I'd expect the Camino
> code to act given the current broken state of things on cbo.  I'll fix
> header.inc later today and post back; I expect when you can test again after
> that, you'll see sane behavior from cbo.

Despite trying hard, I can't reproduce the funky behaviour anymore. Yay!

I found the explanation for the NYT funkiness: on the front page, it links to http://css.nyt.com/images/icons/nyt.ico (multi-member 48/32/16 .ico file), but following a link there is no reference to any link rel="icon" in the source, and it fetches http://www.nytimes.com/favicon.ico (single member file). Nothing to see there.

---
As far as I'm concerned, this patch is good to go, at least for 10.8; I don't think 10.7 is any different.
Attached image BBC icons on 10.4 — 
Since I was on the tinderboxen already tonight to fix hostnames and reboot and whatnot, I checked the rendering of the BBC site icon on 10.4.

The rendering is identical with and without this patch; we choose the 1-bit (16px) image both times, in both the tab and location bar.  Unfortunately, 10.4's NSImage can't render it worth squat :P

So that definitely lends more evidence to some sort of NSImageView bug on 10.5 that causes the location bar to use the wrong member.

(FWIW, Safari on both 10.4 and 10.5 appears to use the scaled-down 4-bit 32px member.)
Status: ASSIGNED → RESOLVED
Closed: 1 month ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: