379660 - Favicons not showing up in history menu (most of the time)

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Colin Barrett [:cbarrett]]

The only way I can reliably get them to show up is:

1. Select History menu
2. Move mouse over to show Bookmarks menu
3. Mouse back over to show History menu

Related to bug 363574?

Comment 1

[Stan Shebs]

Attachment: Proposed patch

This patch rejiggers the menu item icon code to use the usual Cocoa methods and images instead of old flaky Carbon bits. It's conceptually straightforward, the trickiest bit is that the menu item for a submenu needs to be kept around now, and accessed with an interface method just before the icon is set up, otherwise the depth-first menu recursing won't have filled in content sufficiently. I cut corners in icon creation by converting the CG version to NSImage, when it would be more efficient to create the NSImage directly, but I wanted to get the overall patch up for review in the meantime.

Comment 2

[Josh Aas]

Comment on attachment 272118 [details] [diff] [review]
Proposed patch

>+  NSMenuItem*               mNativeMenuItem;       // strong ref, we own

This does not appear to be a strong ref. Either actually strong ref it or fix the comment. If you aren't going to strong ref it then we need to be very careful about making sure that pointer is valid at all times.

+      sPlaceholderIconImage = [[NSImage alloc] initWithSize:NSMakeSize(kIconWidth, kIconHeight)];

Make a quick note here at the site of "alloc" that it is OK to "leak" this (we never release it).

>+    [mNativeMenuItem setImage:sPlaceholderIconImage];
>+
>   }

Kill off the extra blank line here.

>+  NSImage* newImage;

Why not just declare this down where you allocate the data?

>+  newImage = [[NSImage alloc] initWithSize:imageRect.size];

This leaks!

Comment 3

[Stan Shebs]

Attachment: improved favicons

This version fixes the observed problems, adds some null checks, and correctly parametrizes an image size hardwired in the first version.

Comment 4

[Stan Shebs]

Also wanted to mention that I tested the improved patch on 10.5 beta and it works OK there too.

Comment 5

[Josh Aas]

Comment on attachment 272710 [details] [diff] [review]
improved favicons

+  if (!mNativeMenuItem)
+    {

This bracing style is not normal, please put the opening brace on the same line as the test and don't indent the end brace. Can fix on checkin.

 #import <Carbon/Carbon.h>
 
+#import <Cocoa/Cocoa.h>

Still need to kill off the empty line between these. Can fix on checkin.

Comment 6

[Stuart Parmenter]

Comment on attachment 272710 [details] [diff] [review]
improved favicons

Why do we need to add this to the interface?  You know what kind of menu you are and should be able to cast.

Comment 7

[Stan Shebs]

This is the icon object using a menu object that it's associated with, it has no special status. Casting doesn't work, the field is still protected.

Comment 8

[Stan Shebs]

Attachment: no-interface version

Tweaked to use static cast and friend class so as to avoid interface hacking, plus minor style fixes as noted previously.

Comment 9

[Stan Shebs]

Attachment: using accessor this time

Using an accessor instead of direct access.

Comment 10

[Josh Aas]

landed on trunk

Comment 11

[Wayne Woods]

Looking good. Thanks Stan!