Closed Bug 363574 Opened 15 years ago Closed 15 years ago

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


(Core :: Widget: Cocoa, defect)

Not set





(Reporter: waynegwoods, Assigned: jaas)




(1 file, 1 obsolete file)

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.
Blocks: cocoa
Flags: blocking1.9? → blocking1.9+
Attached patch fix v1.0Splinter Review
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.
Attachment #253989 - Attachment is obsolete: true
Attachment #260400 - Flags: review?(mark)
> 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 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.
Whoops, I'll get back to this soon.
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?
Attachment #260400 - Flags: review?(mark) → review+
Attachment #260400 - Flags: superreview?(pavlov)
Attachment #260400 - Flags: superreview?(pavlov) → superreview+
landed on trunk
Closed: 15 years ago
Resolution: --- → FIXED
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.
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.
Resolution: FIXED → ---
Target Milestone: --- → mozilla1.9alpha6
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.

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.
Target Milestone: mozilla1.9alpha6 → mozilla1.9beta2
Assignee: joshmoz → stanshebs
Assignee: stanshebs → joshmoz
marking this fixed again since we have an actual implementation, bug 379660 is for further issues
Closed: 15 years ago15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.