Closed Bug 379660 Opened 13 years ago Closed 12 years ago

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

Categories

(Core :: Widget: Cocoa, defect)

x86
macOS
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla1.9alpha8

People

(Reporter: cbarrett, Assigned: stanshebs)

Details

Attachments

(1 file, 3 obsolete files)

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?
Flags: blocking1.9+
Target Milestone: --- → mozilla1.9alpha6
Assignee: joshmoz → stanshebs
Target Milestone: mozilla1.9alpha6 → mozilla1.9beta1
Attached patch Proposed patch (obsolete) — Splinter Review
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.
Attachment #272118 - Flags: review?(joshmoz)
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!
Attachment #272118 - Flags: review?(joshmoz) → review-
Attached patch improved favicons (obsolete) — Splinter Review
This version fixes the observed problems, adds some null checks, and correctly parametrizes an image size hardwired in the first version.
Attachment #272118 - Attachment is obsolete: true
Attachment #272710 - Flags: review?(joshmoz)
Also wanted to mention that I tested the improved patch on 10.5 beta and it works OK there too.
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.
Attachment #272710 - Flags: superreview?(pavlov)
Attachment #272710 - Flags: review?(joshmoz)
Attachment #272710 - Flags: review+
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.
Attachment #272710 - Flags: superreview?(pavlov) → superreview-
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.
Attached patch no-interface version (obsolete) — Splinter Review
Tweaked to use static cast and friend class so as to avoid interface hacking, plus minor style fixes as noted previously.
Attachment #272710 - Attachment is obsolete: true
Attachment #273038 - Flags: review?(joshmoz)
Using an accessor instead of direct access.
Attachment #273038 - Attachment is obsolete: true
Attachment #273041 - Flags: review?(joshmoz)
Attachment #273038 - Flags: review?(joshmoz)
Attachment #273041 - Flags: superreview?(pavlov)
Attachment #273041 - Flags: review?(joshmoz)
Attachment #273041 - Flags: review+
Attachment #273041 - Flags: superreview?(pavlov) → superreview+
landed on trunk
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Looking good. Thanks Stan!
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.