363574 - [Cocoa] Favicons/site icons missing from Bookmarks menu in Cocoa builds

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Wayne Woods]

Since Cocoa was enabled, site icons have vanished from the Bookmarks menu (which were implemented under bug 46177).

This is another bug I'm surprised I can't find a report for. Perhaps I just didn't recognize it.

Comment 1

[Asaf Romano (gone)]

Attachment: checkpoint

FWIW

Comment 2

[Josh Aas]

Attachment: fix v1.0

This patch works pretty well. Two things about it:

1. We should factor some of the cairo image format translation code (gfxIImage -> CGImage, NSImage) into helper routines for clarity. I didn't do it in this patch because right how this particular translation isn't done anywhere else (gfx image -> CGImage), and I'd rather sit down and look into a good helper routine strategy later.

2. When opening the history menu in Firefox, menu item icons don't appear unless you keep the mouse down, move to another menu, then mouse back over the history menu. I believe this bug existed in the carbon menu item icon implementation but it didn't show up because the history menu didn't have icons in Firefox 2. We need to fix this for sure, but I'd like to start review of this patch now and maybe fix the history menu in a followup patch.

Comment 3

[Mark Mentovai]

> I believe this bug existed in the carbon menu item icon implementation but it
> didn't show up because the history menu didn't have icons in Firefox 2.

Yes, you're right, we would have had this bug with the Carbon icons too if we had ever used them with a History menu that included icons.

Comment 4

[Mark Mentovai]

Comment on attachment 260400 [details] [diff] [review]
fix v1.0

The history menu thing can be worked around by returning PR_TRUE from nsMenuItemIconX::ShouldLoadSync.  Don't do that, though.  It would suck.

Did you test this patch with 10.3?  There were enough differences between 10.3 and 10.4 when I wrote the initial Carbon icon support that both should be tested.  If you can't test 10.3 yourself, just make a point to ask someone to do it once this lands.

Comment 5

[Mark Mentovai]

Whoops, I'll get back to this soon.

Comment 6

[Mark Mentovai]

Comment on attachment 260400 [details] [diff] [review]
fix v1.0

The conversion stuff really does belong elsewhere.

What's with all of the *X class- and file-naming?  That's a throwback to when Carbon and Classic had different implementations, why are we using it for new classes?

Comment 7

[Josh Aas]

landed on trunk

Comment 8

[Wayne Woods]

20070424 trunk Bookmarks Menu... I'm only seeing icons for folders (including livemark folders). I loaded a few individual pages from the menu, in case it had simply lost the icons over time and needed to cache them again, but still no luck.

Comment 9

[Wayne Woods]

This is definitely still broken -> reopening. Tested using a fresh profile, 20070426 trunk build on OS X 10.4.9 (PPC).

If you add a page to the bookmarks menu for the first time, no favicon appears, and the icon doesn't appear to be cached, either (doesn't even appear in the bookmarks manager where it normally would)
If you then use the bookmarks menu to go to the page for the second time, the favicon *is* cached, and appears on the menu.
If you then quit and restart the browser, or sometimes even just go to a new tab, the icon vanishes from the menu again, and this time it seems to be gone for good... attempting to reload it from the menu doesn't seem to bring back the icon once it's been cached.

Comment 10

[Justin Dolske [:Dolske]]

Yeah, favicon have been and continue to be missing from the Bookmarks menu for me. Actually, to be precise, they only seem to show up the second time the menu is drawn after the menubar gets focus.

Eg.
1. Click Bookmarks menu... no icons.
2. Hover over to Tools menu
3. Hover back to Bookmarks menu... icons.
4. Click on content area
5. Lather, rinse, repeat symptoms.

Comment 12

[Josh Aas]

marking this fixed again since we have an actual implementation, bug 379660 is for further issues