46177 - Bookmarks menu in menubar does not have icons

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Mac, enhancement, P3)

Description

[Andrew Thompson]

Moz buil 2000072113

On the Mac, the bookmarks menu in the menubar has no icons in it.
(This was also true in Nav 4)
The bookmarks menu in the Mac Classic Skin and the Bookmarks Sidebar panel have
icons. The bookmarks menu in the menubar should match.
(I'd check the modern skin, which is what I usually use, but skin switching
seems to be bust in this build, so I'm stuck in Classic for now)

(Oh, and I've heard in the past, not from netscape though, that it can't be done
on the Mac. It can. Checkout IE5 for Mac)

Comment 1

[Matthew T (active 1999-2002)]

Netscape 4.x also did it in the `Move to' folder in Messenger.

Comment 2

[Mike Pinkerton (not reading bugmail)]

no one ever said it can't be done, it's just complicated w/out special case 
code. the menu code doesn't know anything about which menu is the "bookmarks" 
menu, and special casing it would be ugly ugly ugly.

Pushing this off to future, since it is technically possible, but quite 
involved the way we do menus. cc'ing saari.

Comment 3

[Matthew T (active 1999-2002)]

Implementation: *If* a menu item handled by the Mac native menu bar contains one 
or more graphics, *then* take the first graphic in the item, resize it (if this 
is technically necessary) to be the right size for menu item icons on Mac, and 
then plunk it in the menu as described in
<http://developer.apple.com/techpubs/macos8/HumanInterfaceToolbox/MenuManager/﷒0﷓>.

Comment 4

[Mike Pinkerton (not reading bugmail)]

the tricky part is converting a gif to a mac native icon, not determining what 
the graphic should be. i mis-spoke earlier. :P

Comment 5

[Andrew Thompson]

Can't we just ask Quicktime to do it or would that muddy the architecture too 
much?
This is a Mac only feature, so Quicktime can be 99.999999% assumed.

Comment 6

[Matthew T (active 1999-2002)]

If we're going to (eventually) add a preview of a saved document to that 
document's icon (like Photoshop does), we're going to need a picture-->icon 
conversion routine anyway.

Surely someone must have written such a routine already, that they're willing to 
give away?

Comment 7

[Nikhil Bhatla (gone)]

I know that the open-source Nautilus GUI project must have some routines to do 
this.   Each icon on the desktop is dynamically created to be a snapshot of the 
file, image files included.  Since this is open-source and probably GPL'ed, we 
should be able to use it in Mozilla. Someone should look into this: http://
nautilus.eazel.com/

Nautilus is a project started by Eazel (www.eazel.com) working in collaboration 
with GNOME and HelixCode to design a usable GUI for Linux.  The cool part about 
Eazel is that they're a bunch of the original Macintosh folks.

Comment 8

[saari (gone)]

I doubt Nautilus is using Mac graphic file formats (PICT, icon, etc). I could
always be wrong...

Comment 9

[timeless]

Actually I'm not sure if there are any tricky parts. Converting from image to 
icon shouldn't be too complicated.

Step one is just having 2-3 generic icons in the menu if someone really wants 
to do it.

<asside in-reply-to=eazel>Even if it does, GPL is generally incompatible w/ 
Mozilla (MPL/NPL).</asside>

Generating previews as a bug would probably be futured. As for quicktime, I 
don't remember if mozilla requires it. But I prefer to not increase system 
requirements.

wrt pinkerton's comments. I'd think being able to put icons into any menu would 
be part of the XP wrappings.  Although I'm not sure how do any of this [I'd 
need to turn on my mac, plug the monitor in and pop in the MPW cd].

Comment 10

[John Bafford]

Timeless suggests not using QuickTime as that would "increase" system 

requirements.



As Mozilla already requires Mac OS 8(.5?) requiring QuickTime means nothing, as 

QuickTime has come shipped with Mac OS since probably Mac OS 7.5. And there's 

probably something else that Mozilla does that requires QuickTime. (If the 

graphics engine were designed right, it would be passing all file formats that 

QuickTime supports to QuickTime, rather than doing it itself. Using Operating 

System services when possible is a Good Thing.)

Comment 11

[Adam]

If anything ever comes out of this (I pray not) be sure to make this an option.
Icons in the menubar is one of the most annoying
we-have-too-much-time-on-our-hands "feature" that can ever be added to an app. I
believe there is an app that provides much of the same functionality (menubar
icons that can be turned off), but I'm at a loss for exactly which one.

Comment 12

[Andrew Thompson]

Um, this bug is not about putting a menu in the menubar with a Mozilla icon, its 
about putting icons in the *Bookmarks* menu to the left of all of the Bookmarks, 
as is already done in the Bookmarks sidebar panel, in the Bookmarks popup menu on 
the personal toolbar, and in the Bookmarks menu on other platforms. For a Mac 
example, see the Favourites menu in IE5 (or indeed the contents of the Apple 
menu).

Aside from being more visually consistent, if you bookmark lots of different 
kinds of resources (i.e. not just web pages) the icons can help you recognise 
what's what.

Comment 13

[saari (gone)]

Problem here is needing the code to go from the XP image format (gif, jpeg, png,
etc.) to the Mac friendly icon that gets feed to the menu manager.

Comment 14

[Andrew Thompson]

I found some sample Apple code which can make cicn and Icon Suite resources from 
arbitrary pixel data in a GWorld. I'm going to try to write a function that'll go 
from an nsIImage (with a GIF/PNG or whatever in it) to a Mac icon. No Quicktime 
required. 

However, linking it into the menu system looks extremely hairy to me. However 
saari and pinkerton seem to think that's easy, judging by the above comments. In 
any case the code is here:

http://developer.apple.com/samplecode/Sample_Code/Graphics_2D/MakeIcon.htm

Comment 15

[Andrew Thompson]

Now we have icon creation code, back to looking at this. This comment is an ideas 
scratchpad. I'm tired and don't want to lose anything I've worked out so far.

Funamentally menus have icons on them because they're styled that way in the 
skin, using the ubiquitous list-style-image: url('chrome://') gubbins.

Somehow the icon being set by CSS has to be communicate to the Mac menu code in 
the form of an nsIImage, which we can QueryInterface into an nsImageMac.

Then all we have to do is call ConvertToIcon on that image and set it into the 
menu using ::SetMenuItemIconHandle ... see
http://developer.apple.com/techpubs/macos8/HumanInterfaceToolbox/MenuManager/﷒0﷓

The place this code needs to go (I think) is:
NS_METHOD nsMenu::AddMenuItem(nsIMenuItem * aMenuItem)
We probably want to make the icon (or the nsImage...?) itself a property of 
nsMenuItem, with get/set accessors like the menu label and shortcut key etc.

So the final piece is how (and where) to work out if a nsMenuItem should have an 
icon ... an call setIcon on it. Need access to the CSS style of the <menuitem> to 
find out if list-style-image is set? If so get an nsImage from the chrome URL and 
call the hypothetical nsMenuItem::SetIcon (or add it to nsMenuItem::Create). 

Can't help but think that since we usually use the same icon over and over again 
in a menu, especially in this bug, (bookmark, bookmark folder), there must be 
some point where we can convert the nsImage into an icon once and set it (or a 
simply HandToHand copy of it) into every menu item.

If anyone has any insight... fire away

Comment 16

[Andrew Thompson]

Apologies for more spam, but I thought of something that'll make making this 
performant easier... 

::SetMenuItemIconHandle supporting using IconRefs to set the icon. IconRefs are 
loaded into a central cache by the system and are reference counted by nature. 
They're keyed by a  pair - a 4 char creator and type code. 

If we can come up with a good nsImage->key scheme we can simply let the system 
cache any icons for us, if we are asked to set an icon which is not in the cache 
we simply add it. Combined with using the built in IconRef reference counting it 
should be relatively easy to avoid converting the same image over and over again, 
and squash any leaks....

Comment 17

[Andrew Thompson]

Attachment: Some work towards supporting icons in Mac menus

Comment 18

[Andrew Thompson]

Here's where I'm at so far on this. (attached)
I have some infrastructure set up to handle loading each icon only once and 
caching them. I also have managed to get hold of the chrome: URI which tells me 
what .gif file to use.

I'm stimied because I don't know how to actually load that .gif file and come up 
with an nsImage. Strikes me this should be relatively easy, but no one seems to 
be able to tell me how.

Any help or comments apprechiated...

Comment 19

[Andrew Thompson]

Attachment: Partial patch... where I'm going with this

Comment 20

[Andrew Thompson]

Attachment: oops, that was an old patch

Comment 21

[Andrew Thompson]

I'd *love* some comments on this. Its basically complete, but naturally it 
doesn't work, and it probably leaks like a sieve. Any advice would be great.

Comment 22

[Andrew Thompson]

Attachment: Picture of first working version. Rough, but getting there

Comment 23

[Mike Pinkerton (not reading bugmail)]

i don't like that we're computing style to get the icon. that will slow us way 
down, and we can't afford to get any slower for something so trivial.

Comment 24

[Andrew Thompson]

I had this converstation with hyatt... Note its only done in nsMenuItem::create() 
so its a one time hit for non-dynamic menus at least. Of course, very many of our 
menus are dynamic. The tradeoff here is that the icons aren't "live" they don't 
animate like they do in XPMenus.

Casual testing (commenting out my code) with a bookmarks folder I have with 47 
items in it (which I'd say is very long, for any menu other than the bookmarks 
menu, cause some people make them stupidly long) indicates its barely 
perceptible, if its noticable at all. There's a delay for the 47 items menu even 
without my code.

Presumably though, could I add an id="" or more properly a class="" to all of the 
bookmarks menus items as they're generated, then use this as the key to my icon 
map? Then, I'd only have to resolve style once for each class of menu item. I'd 
then have to register for skin switch events though...

Thanks for the comments. What do you think of the class idea?

Comment 25

[Andrew Thompson]

Hmmm, so looking at this from navigatorOverlay.xul,

      <template id="bookmarksMenuTemplate">
        <rule iscontainer="true" isempty="true"
              rdf:type="http://home.netscape.com/NC-rdf#Folder">
          <menupopup>
            <menu class="menu-iconic bookmark-item" uri="rdf:*" 
                  type="rdf:http://www.w3.org/1999/02/22-rdf-syntax-ns#type"
                  label="rdf:http://home.netscape.com/NC-rdf#Name">

etc. etc.

I should be able to make a special case for those nsIContent nodes who's class 
attribute includes the string "bookmark-item" and bypass style resolution after 
the first one is encountered... Sound reasonable? Other menus should have much 
saner number of items in them, but users could stuff literally hundreds into the 
bookmarks menus.

AndyT

Comment 26

[Mike Pinkerton (not reading bugmail)]

i like the class idea, i'll talk to hyatt today at lunch about it. I just worry 
about the class name changing out from under us. you know how crazy those skin 
guys are! ;) maybe a comment in the xul to tell people that if they change that 
class they have to also update the mac menu code, but that's like spitting in the 
wind: you'll get hit eventually.

saari would probably be a good person to r= as well, for the imaging stuffs. i'll 
get him to look at it as well.

Comment 27

[Andrew Thompson]

-> Mine.

I've been thinking about this, the more I look at my example, the less convinced 
I am I can tell any speed difference, but it irks me to re-resolve the style for 
every bookmark menu item when I know they're the same. I know what you mean about 
depending on the class names though.

How about this:

Extend my Map to map all class attributes encountered to the icon in question. 
(ie, we don't care what the class attribute is, we just use it as a key)

When getting the icon, first try looking it up by class attribute, and only if 
that fails bother to resolve style and use the url as a secondary key.

To avoid making bad assumptions that items with the same class have the same 
style, we could introduce a rule whereby if an item has an id attribute, we 
assume its unique in some way and always resolve style on it (ie, never look up 
the icon for the class). This should cover 90%+ of probable cases.... all of the 
bookmarks items should resolve quickly, and anything unique should be recognised 
as such. Oh, one day someone will come along and write a style rule that'll break 
this, but it'll be quite tricky for them ;)

Comment 28

[Andrew Thompson]

Attachment: "Working" patch - putting up version that works. Worst klangers removed :)

Comment 29

[Andrew Thompson]

Attachment: Patch rewritten to handle sub menus, working picture...

Comment 30

[Andrew Thompson]

Attachment: patch should be complete enough that you could apply it. Still in need of some fixes/restructuring

Comment 31

[Andrew Thompson]

Attachment: New version of patch, works, seems complete to me, I want reviews! (still has some debug code)

Comment 32

[Andrew Thompson]

Latest patch: if you need to apply, send me mail and I'll point out all the 
little things you're gonna have to do. One of these is to apply all of the 
patches from the bugs this depends on first!

I's like reviews please! Read it over and see what you make of it, we can deal 
with the housekeeping later once the code is right.

Pinkerton: re: your performance concerns. I did some PrIntervalTimer based 
profiling and on my single CPU 400mhz G4, resolving style on menu creation is 
some 2-5 milliseconds per item ... modal average seems to be 3. So even a 100 
items menu only takes around 300 milliseconds to resolve.

Comment 33

[Andrew Thompson]

Attachment: Port to Mac OS X. Woot! Mega patch now does Classic & Carbon

Comment 34

[Andrew Thompson]

Eyecandy: http://pixel.recoil.org/BookMarksMenuX.gif

Comment 35

[Simon Fraser [no longer active]]

Ooh, nice.

Comment 36

[Robert John Churchill]

No activity for a few months... what's the current state of this?

Now that we have moz-icon: support for menus (which means favicons for
bookmark'ed websites as well as native system icons for bookmark'ed file:///
URLs), let's get this all fully functional and checked in.  :)  

Comment 37

[Andrew Thompson]

0/ Well, it basically still works (and on OS X too)
1/ I just got r and sr on one of the depends on bugs, so I'll check that in soon.
2/ I have a list of half a dozen or so outstanding issues I should get to
3/ I'm aware of moz-icon. Depending on how hard that looks to support, I'll 
either do that here or open a seperate bug once this is checked in.

Comment 38

[Greg K.]

Retarget at OS X only?

Comment 39

[Jo Hermans]

Any progress, LordPixel ? Shouldn't we just check it in for Moz 1.6a, and try to
optimize it later ?

Comment 40

[Andrew Thompson]

Oh, well. Things have changed a lot since this patch last worked at all.
I do intend to come back to it someday, and the underlying icon code is all
still there, but right now it doesn't really do much. Sorry.

Comment 41

[Wayne Woods]

Anyone willing to revisit this now? :) It'd be nice to get this done, as it's
still an obvious Mac interface deficiency compared to the Windows version.

Comment 42

[Takeshi Nishimura]

Just for anyone who revisits this:
+  if (status && imgIRequest::STATUS_LOAD_COMPLETE) {
may be
+  if (status & imgIRequest::STATUS_LOAD_COMPLETE) {

Comment 43

[Wayne Woods]

*** Bug 219846 has been marked as a duplicate of this bug. ***

Comment 44

[Wayne Woods]

I'm not sure how Josh et al. are going with implementing native widget support in Mozilla, but maybe this will become much easier when they're done? :)

Comment 45

[Phil Ringnalda (:philor)]

*** Bug 340774 has been marked as a duplicate of this bug. ***

Comment 46

[Mark Mentovai]

Attachment: Fresh

I took a look at this after doing bug 345739.  I began with the vestiges of the five-year-old patch, since I didn't know how to get to the style sheet from an arbitrary menu content node myself.  Thanks, lordpixel!  Most of everything else in this patch is entirely original.

The difference from the old patch is that this one supports bookmark favicons by preferring the menu content's "image" attribute, if it exists, to the CSS "list-style-image" property.  (There are also a couple of fixes to the pinstripe theme to get the right values set on "list-style-image" on the Mac, and to put back the globe as the default favicon.)

This patch doesn't include the cache that the old patches did.  Caching is necko's job, and since I'm using CGImages (retained by the menu manager) instead of old-world icons, there's no need to keep them around.

One aspect of this patch is something of a hack, though.  MenuSelect is a blocking API.  Once the user starts mousing around in native menus, the main thread blocks completely.  Gecko's own main thread is entirely unable to make progress.  Because this patch depends on imgILoader, which may not complete before LoadImage is returned, processing Gecko events is crucial.  (There are other areas of suckage where we have trouble because we're not processing Gecko events while the system is tracking the menu bar or a scrollbar.)  My solution to this is to process Gecko events when a few hookable things happen inside of MenuSelect, such as opening menus and targeting menu items.

This was fun, and it works surprisingly well.

Comment 47

[Mark Mentovai]

Attachment: Screen shot

Really, it's much more fun to use a patched build than to stare at a picture, but for those without...

Comment 48

[Mark Mentovai]

Re the hackish solution to get the event loop to run: running the event loop like this shouldn't be a problem for Gecko, because Gecko's expecting its thread to keep running while the user is using the menus anyway.  I was concerned that we'd crash if the user was mousing around in the menus when a script decided to close the window.  Fortunately, that's not the case.  When a window closes, menu tracking ends and the menus disappear "gracefully."  This is the same behavior that we experience on Windows, and it's about the best we can do.  If a tab closes within a window, menu tracking continues.  Again, this is the same as Windows.

(Actually, it seems that we do a little bit better on the Mac than on Windows when it comes to updating menu items during tracking.  If you select text in tab A and have no text selected in tab B, activate tab B, and go into the Edit menu, the Copy menu item will be disabled.  When tab B closes and tab A activates, the Copy menu item should enable itself.  It does on the Mac, but on Windows, its appearance remains dimmed until you go to another menu and then come back to the Edit menu.)

Having performed these tests, I have more confidence in my approach.

Comment 49

[Stefan [:stefanh]]

Comment on attachment 230781 [details] [diff] [review]
Fresh

Woaw - this is really nice. Thanks Mark!

Some issues I see on my seamonkey debug build (on 10.3.9). I don't know wether this should work when building with the 10.2.8 sdk (that I do), so bear with me if this is bogus:

1) Selected menuitems "stays" selected when you move the mouse pointer away. If I open a menu and move the mouse pointer downwards over menuitems all menuitems gets the blue selection color.

2) Menuitems doen't turn blue until you have the mouse pointer at the very bottom of the menuitem. It's like the opposite of the real native ones where they turn blue when you have your mouse over the top of the menuitem.

3) I get this assertion (that looks related) a lot of times:

###!!! ASSERTION: Someone forgot to properly cancel this request!: '!mListener', file /Users/Stefan/mozilla-debug/mozilla/modules/libpr0n/src/imgRequestProxy.cpp, line 75

Comment 50

[Stefan [:stefanh]]

Attachment: Screenshot describing issue 1 in comment #49

Comment 51

[Mark Mentovai]

Re issues 1 and 2, there may be limitations in 10.3.9's menu implementation that prevent this from working properly.  Hmm.  This would be a factor of the runtime OS, not the SDK.

Re issue 3, that's me - I was running in to some trouble when I was canceling the request.  I should probably just not cancel the request and not null out the request object until OnStopRequest is called.

I've also come up with a different implementation to keep Gecko events firing while in menu tracking, but I don't think that would have any impact on what you're seeing.

Comment 52

[Mark Mentovai]

The different implementation I mentioned in comment 51 is attachment 230781 [details] [diff] [review] (bug 346108).  You can use that INSTEAD of the hackodiff to nsMenuBarX.cpp in attachment 230781 [details] [diff] [review].

Comment 53

[Mark Mentovai]

Panther doesn't behave particularly well when you change the icons for a menu item on a menu that's already open.  I haven't seen what attachment 230899 [details] depicts, but I'm not working with menu items that have key equivalents.

Comment 54

[Stefan [:stefanh]]

(In reply to comment #52)
> The different implementation I mentioned in comment 51 is attachment 230781 [details] [diff] [review] [edit]
> (bug 346108).  You can use that INSTEAD of the hackodiff to nsMenuBarX.cpp in
> attachment 230781 [details] [diff] [review] [edit].
>

Nice. Applying attachment 230781 [details] [diff] [review] in bug 230781 and backing out the changes in nsMenuBarX.cpp actually seem to made issue 1 & 2 disappear.

(In reply to comment #53)
> Panther doesn't behave particularly well when you change the icons for a menu
> item on a menu that's already open.  I haven't seen what attachment 230899 [details] [edit]
> depicts, but I'm not working with menu items that have key equivalents.

You mean that we can't change an icon in a menuitem if we ,for instance, hover the menuitem? Yeah, I've seen that - in seamonkey, hovering over a menu with a bookmark folder should change the bm folder icon to an "open" folder (this happens in the personal toolbar bookmarks menu). This doesn't seem to be the case with native menu icons. I don't really know if this would be an issue, though.

Comment 55

[Mark Mentovai]

That's 'cuz I'm not tracking CSS properties the same way that the XUL interface does, and I probably won't.  What I mean is a little more disgusting, though: you'll open a bookmarks menu, it'll populate with some icons but not all, and the rest won't show up until after you've left menu tracking and then reopened the menu.

But that's not the worst: I'm also seeing the highlight for menu items not extend all the way across to the right edge of the menu (instead, it's only about as wide as the menu item) on Panther if the new implementation tries add an icon to a menu item while the menu is open.  (But I don't really trust this Panther box.)  You're not seeing anything like that?

Comment 56

[Matthew T (active 1999-2002)]

Yes, I reported that exact problem as a bug in Adium and it was WONTFIXed <http://trac.adiumx.com/ticket/2102>. But I can't think of any good reason to change the icon for a Bookmarks menu item after the menu is open. (Changing a closed folder to an open folder is not a good reason.:-)

Comment 57

[Mark Mentovai]

Yeah.  I'm thinking that if we do this, it should be a Tiger-and-up feature only.

I ported this patch to the 1.8[.1] branch yesterday.  Normally, I wouldn't have done it so prematurely, but the 1.8.1 branch has a different events implementation that allows this patch to behave properly without any of nsMenuBarX/bug 346108 hackaround business.  I've been running with that yesterday afternoon and evening and it looks great.

Beltzner, what do you think?  Is this something we'd want as a 10.4-only feature for 1.8.1?  Screen shot in attachment 230782 [details].

Comment 58

[Matthew T (active 1999-2002)]

Icons in Bookmarks/equivalent menus work fine in 10.3 in Safari, Opera, and OmniWeb. I find it very strange that you're proposing making the Mozilla implementation 10.4-only just so that icons can change while a menu is open.

Comment 59

[Stefan [:stefanh]]

(In reply to comment #56)
> Yes, I reported that exact problem as a bug in Adium and it was WONTFIXed
> <http://trac.adiumx.com/ticket/2102>. But I can't think of any good reason to
> change the icon for a Bookmarks menu item after the menu is open. (Changing a
> closed folder to an open folder is not a good reason.:-)
>

We do change bookmark item icons if you have bm favicons enabled. It's pretty easy to reproduce the problem anyway, at least for me with an old and slow machine: Upon launch, open the bm menu quickly - sometimes the icons haven't loaded yet - and hover over the menuitems.

Comment 60

[Mark Mentovai]

I had written a response, clicked "commit", and then gone to work on other things.  Then I came back and closed the tab without looking - but it was a mid-air collision!

Oh well.  I explained roughly the same thing I had written in the comment to this code:

// static
PRBool
nsMenuItemIcon::IconsEnabled()
{
  // Only enable native menu item icons on Mac OS X 10.4 ("Tiger") or later.
  // The menu manager in 10.3 ("Panther") is unable to cope with menu item
  // icons changing while a menu is open in tracking.  Because menu item
  // icons are set in an imgIDecoderObserver notification, it's possible
  // and even likely that some icons will not be set until after the menu
  // is open.  On Panther, when this happens, the updated icon will not be
  // displayed and highlighting of menu items in the affected menu will be
  // incorrect.  Tiger has no trouble updating icons while a menu is open.

Addendum: for the purposes of Moz apps, which have icons in chrome or data URIs, this has nothing to do with oldness or slowness.  We could load icons synchronously, but that could introduce a delay in opening menus, especially in apps that specify uncached network URIs.  We should be app-agnostic in widget.  The sleazier architecture isn't worth it to make this work in Panther.

Comment 61

[Mark Mentovai]

Attachment: v2 (trunk)

This is the trunk version, with all event hacks removed, so it's safe.  It looks bad due to bug 338225: you'll open a menu and some icons won't be present, and because Gecko events won't be firing while the menu is open, they won't show up.  If this does land on the trunk, it may be worth keeping disabled until that bug is fixed.  Or maybe it could be kept enabled as a reminder that bug 338225 needs to be fixed.  (It is a serious regression.)

If you wish to test this, please test the 1.8[.1] branch version - on the branch, of course.  PLEvents are able to fire during menu tracking, so this patch works like a charm there.  No hacky or disgusting or unsafe tricks were involved in the creation of this patch.

Comment 62

[Mark Mentovai]

Attachment: v2 (1.8[.1])

I will post a Bon Echo test build later for those who would like to see this in action.

Comment 63

[Mark Mentovai]

A 1.8[.1] test build with this (v2) patch is available at:

http://jackassofalltrades.com/tmp/bonecho-18-20060728-46177v2.dmg

This enables native menu item icons on Mac running Tiger.  Interested parties, please test it and report any problems (or successes).  Source pull time was 1900 ET.

Comment 64

[Wayne Woods]

This looks nice, Mark! I notice it makes submenus open to the left, instead of the right, so I assume the menus are a bit wider as a result. Maybe it'll correct itself when the component name is switched back to Firefox anyway, but otherwise, is there any way you can make the menu a tiny bit narrower?

Comment 65

[Jo Hermans]

(In reply to comment #63)
> A 1.8[.1] test build with this (v2) patch is available at:
> 
> http://jackassofalltrades.com/tmp/bonecho-18-20060728-46177v2.dmg
> 
> This enables native menu item icons on Mac running Tiger.  Interested parties,
> please test it and report any problems (or successes).  Source pull time was
> 1900 ET.
> 

Out of curiosity, I tried it on Mac OS X 10.2.8. No problems that I could see. Ofcourse, no icons either.

Comment 66

[Mark Mentovai]

(In reply to comment #64)

Which side the submenu opens on is a factor of the menu position, width of the parent and child menus and the width of your screen.  I can't do anything about this (except buy you a new display! - or make the icons smaller, but 16px^2 is generally accepted as the correct size of a favicon).  You can, though.  A 16px wide icon only makes the menu item 19px wider.  Menu width is calculated to accomodate the widest item in a menu, so if you trim your longest titles, you'll narrow things back down a bit.  As you point out, we'll get 12px back in a brand with official "Firefox" branding.  You probably already know all of this, I'm just pointing out options for a legitimate gripe.

For what it's worth, with my bookmarks, I can get menus as deep as far as the grandchild menu of the bookmarks menu to open on the right.  This is on displays that are 1280px and 1440px wide.  I'm not even going to tell you how many submenus I can fit on 1920px, but I will say that at 1024px, I can at least get children of the bookmarks menu to open on the right.  Grandchildren open on the left, but that's no different from when the icons are off.

(In reply to comment #65)

I haven't tested on 10.2 yet because I assumed menus would be as broken there as they are on 10.3, but I'll check this out later and reenable the feature on 10.2 if they're working there.

Comment 67

[Mark Mentovai]

10.2 behaves better than 10.3, but still not good enough to turn the feature on there.  On 10.2, setting or changing a menu item's icon after the menu is open has no effect until the item in question is selected (or deselected).  At that time, the new icon paints properly.  Unlike 10.3, the selection width isn't altered.  However, due to the issue I explained in comment 60, many menu items will still show up with no icons when the menu is first opened on 10.2, which looks unprofessional and bad.

Adding my own InvalidateMenuItems and UpdateInvalidMenuItems calls makes no difference on 10.2 or 10.3.

Comment 68

[Andrew Thompson]

Mark, it's great to see you pick up this 5 year old code and do something with it, even if you're just using a tiny % of what I wrote back then.

Given it used to work on Mac OS 9 ;-) I think it would be appropriate to enable static menu icons on 10.X and dynamic changing ones on 10.4+. Is this the current plan?

Comment 69

[Mark Mentovai]

We can't really do that.  Dynamic icons aren't the problem.  Pretend for a moment that we won't support dynamic menu item icons in the native menus.  That's not such a stretch because really, we won't - I included code to change the icon dynamically if the "image" attribute of the <menuitem> changes, but that never happens in practice.  The only time XUL menus change icons is when you target a specific menu item and style sheet directs a new icon when the CSS "open" property is "true", and I'm not including support for that here.  I'm also not including support for the CSS "-moz-image-region" property, because the Mac themes never use it.

Anyway, dynamic icons aren't the problem.  Comment 60, from the code:

  // Because menu item
  // icons are set in an imgIDecoderObserver notification, it's possible
  // and even likely that some icons will not be set until after the menu
  // is open.

Specifically, whether or not imgIDecoderObserver::OnStopFrame is called synchronously from within imgILoader::LoadImage depends on whether or not an imgIRequest for the URI has been launched before (and its first frame already decoded).  Other LoadImage requests will return before OnStopFrame can be called, allowing the native menu to open.  Comment 60 also says why I don't want to load images synchronously.  I suppose I could special-case chrome and data URIs to load synchronously, but I'd be concerned about the response time to open the bookmarks menu for a large bookmarks list.  Then again, we'd suffer that same delay if there had alrady been imgIRequests for those URIs...

Comment 70

[Andrew Thompson]

So let me see if I understand what you're saying:

* You're passing LOADNORMAL to LoadImage, therefore...
* The first time a user opens the menu, reasonably none of the images will already have been loaded, so LoadImage will return immediately, and later, asynchronously OnStopFrame will be called when the first frame is decoded.
* The second time the user opens the menu, reasonably most or all of the images will already be loaded, thus LoadImage will actually call OnStopFrame synchronously. 

Thus the first time the user opens the menu in a run of the program, he or she might see the icons loading one by one. The second time they should more or less all be there? Or am I ascribing too much caching behaviour to the image loader?

It's the first pass that's troublesome on pre-Tiger, because the OS doesn't like the icons changing once the menu is open.

Two questions:

Begin as I'm more familiar with Cocoa than Carbon these days, which thread is trying to update the menu items asychronously? Is there a similar rule to Cocoa that all UI changes have to be done from a specific thread? Could it be the thread that is calling back OnStopFrame is wrong, and that's why 10.3 and below don't like updating the display, but 10.4 is more tolerant?

If I have the details above correct, it sounds like the code actually works OK the second time the menu is opened on 10.3, because the loading goes synchronous, generally? So would it be possible to just block the first load of the icons on 10.3-. Can you tell in OnStopFrame whether you're being called synchronously from LoadImage or not?

I'm probably missing a lot of important details...

Comment 71

[Mark Mentovai]

(In reply to comment #70)
> Thus the first time the user opens the menu in a run of the program, he or
> she might see the icons loading one by one. The second time they should more
> or less all be there?

Yes.  The "one by one" behavior doesn't bother me so much.  As long as the icons display as soon as they're loaded and decoded, I feel we've done our duty.  The problem is that we don't get "one by one" behavior pre-Tiger, and that the Menu Manager gets all confused.

> Or am I ascribing too much caching behaviour to the image loader?

It's got nothing to do with LOAD_NORMAL.  Other than that, you've got it.

> It's the first pass that's troublesome on pre-Tiger, because the OS doesn't
> like the icons changing once the menu is open.

Right.

> Begin as I'm more familiar with Cocoa than Carbon these days, which thread is
> trying to update the menu items asychronously?

Always the main thread.  The load can happen on an auxiliary thread, but observer notifications always go to the main thread via PLEvents (1.8) or Gecko events (trunk) when necessary.

> Is there a similar rule to Cocoa that all UI changes have to be done from a
> specific thread?

Absolutely.

> Could it be the thread that is calling back OnStopFrame is wrong, and that's
> why 10.3 and below don't like updating the display, but 10.4 is more
> tolerant?

Nope.  We're always on the right thread, but older menu managers aren't expecting menu items to change while menus are open.  Remember, when you consider the entire Mac universe since 1984, it's only relatively recently that MenuSelect stopped being totally blocking.

> If I have the details above correct, it sounds like the code actually works
> OK the second time the menu is opened on 10.3, because the loading goes
> synchronous, generally?

The more common case on the 1.8 branch is that the menu and its items have already been built, and the icons have already been set and don't need to change.  But you're correct: even if the menu needed to be rebuilt, many more of the icons would be loaded synchronously the second time around, because they'd already have been loaded previously.  This is actually what happens in Firefox on the trunk: Places rebuilds the bookmarks menu each time it's opened.  I find this to be a little sucky.

> So would it be possible to just block the first load of
> the icons on 10.3-. Can you tell in OnStopFrame whether you're being called
> synchronously from LoadImage or not?

Sure, I can set a member variable around the LoadImage call and check it in OnStopFrame, but that won't do much other than tell me that it's already too late to set the icon.  Not to worry, though.  I've still got a few tricks up my sleeve.

> I'm probably missing a lot of important details...

New patch coming...

(And thanks again for that style-resolution code from the five-year-old patches.)

Comment 72

[Mark Mentovai]

Attachment: v3 (trunk) works on Panther

In this patch, if the icon is coming from chrome:, data:, or moz-anno:, it is loaded synchronously.  Because of bug 338225, this is done regardless of OS release in the trunk version of the patch.

Comment 73

[Mark Mentovai]

Attachment: v3 (1.8[.1]) works on Panther and Jaguar

(I don't claim that the trunk version works on Jaguar because it may not - we've already decided that the trunk will not be Jaguar-compatible.)

This version also contains the theme fix for bug 346311.

The 1.8[.1] branch version allows the load to remain async on Tiger, even for chrome:, data:, and moz-anno: URIs.

A test build equivalent to yesterday's with this v3 patch is here:

http://jackassofalltrades.com/tmp/bonecho-18-20060728-46177v3.dmg

Thanks to all of you who pressed the issue to get this going on earlier OS releases.

Comment 74

[Mark Mentovai]

Comment on attachment 231264 [details] [diff] [review]
v3 (1.8[.1]) works on Panther and Jaguar

I'm especially interested in making sure that we don't block forever waiting for a bogus image if a bad chrome: or data: URI is given.  If anyone wishes to test this on 10.2 or 10.3, please let me know your results.  An easy way to test this would be to play around with the icon attributes in bookmarks.html.

Comment 75

[Jo Hermans]

(In reply to comment #74)
> (From update of attachment 231264 [details] [diff] [review] [edit])
> I'm especially interested in making sure that we don't block forever waiting
> for a bogus image if a bad chrome: or data: URI is given.  If anyone wishes to
> test this on 10.2 or 10.3, please let me know your results.  An easy way to
> test this would be to play around with the icon attributes in bookmarks.html.
> 

The build works for me on Mac OS X 10.2.8. I tried damaging the icon-attribute (different uri, damaged contents), but that just resulted in an empty icon. When I removed the icon attribute, I got the default 'world' icon. Any special tests ?

Comment 76

[Josh Aas]

Comment on attachment 231262 [details] [diff] [review]
v3 (trunk) works on Panther

+  // and NS_NewURI wil fail.

"wil" should be "will"

I haven't finished reviewing.

Comment 77

[Mark Mentovai]

> The build works for me on Mac OS X 10.2.8. I tried damaging the icon-
> attribute (different uri, damaged contents), but that just resulted in an
> empty icon. When I removed the icon attribute, I got the default 'world'
> icon. Any special tests ?

If you can replace image files we use in the chrome archive like bookmark-item.png with corrupt garbage files and the app leaves an empty icon in the native menu without hanging, then I'm satisfied.

> "wil" should be "will"

Shipley would disagree.

Comment 78

[Stefan [:stefanh]]

Comment on attachment 231262 [details] [diff] [review]
v3 (trunk) works on Panther

Hmm, I tried this on 10.3.9 with seamonkey. I noticed that I only get a generic bookmark icon instead of a favicon. Even though I can see bm favicons in the (xul) personal toolbar bookmarks menu.

Comment 79

[Mark Mentovai]

I tried this on the SeaMonkey trunk and don't see favicons for any bookmark.  The "Bookmarks" drop-down in the personal toolbar doesn't have them and the bookmark manager (command-B) doesn't have them either.  The bookmarks menu in the native menu bar matches this behavior with the patch here.

In order to see bookmark favicons, the "image" attribute must be set.  For minimal suck, it should be set to a URI that doesn't require hitting the network, such as a data: URI (used by Firefox without Placs), or a moz-anno: URI (used by Places).  Pre-Places Firefox stores its data: URIs in bookmarks.html as an "icon" attribute on the anchor element.  I don't see anything like this in SeaMonkey's bookmarks.html.

Comment 80

[Stefan [:stefanh]]

(In reply to comment #79)
> I tried this on the SeaMonkey trunk and don't see favicons for any bookmark. 
> The "Bookmarks" drop-down in the personal toolbar doesn't have them and the
> bookmark manager (command-B) doesn't have them either.  The bookmarks menu in
> the native menu bar matches this behavior with the patch here.

Probably because "browser.chrome.favicons" is set to "false" by default. That is, most users won't see the difference. I was mainly curious because this seemed to work with the previous patch.
> 
> In order to see bookmark favicons, the "image" attribute must be set.  For
> minimal suck, it should be set to a URI that doesn't require hitting the
> network, such as a data: URI (used by Firefox without Placs), or a moz-anno:
> URI (used by Places).  Pre-Places Firefox stores its data: URIs in
> bookmarks.html as an "icon" attribute on the anchor element.  I don't see
> anything like this in SeaMonkey's bookmarks.html.
>

Ah, ok. I've just been told that seamonkey takes the favicons from the cache (somehow). 

Comment 81

[Mark Mentovai]

OK.  I set that preference to true and set browser.chrome.load_toolbar_icons to 2.  Looks like SeaMonkey is stashing the URI of the favicon in a "src" attribute instead of an "image" attribute.  I have no idea how it was possible that you saw favicons in the menu with the previous patch.

I could make GetIconURI look at the "src" attribute, but I don't want to for a couple of reasons: first, it's not at all clear that "src" means image in all contexts, and second, SeaMonkey stashes network URIs in the "src" attribute.  I don't want to make network URIs load synchronously.  On the trunk, if they haven't loaded before the menu is opened, they won't load on any OS (bug 338225).  On the branch, if the asynchronously-loaded icon is set on the menu item while the menu is open, bad and unexpected things will happen pre-Tiger (comment 60 et seq.)  I do think it would be a better design for SeaMonkey to not maintain the URI of favicons but instead a cached representation of the last known good favicon for a bookmark.  Its current approach has problems beyond needing to hit the network or cache, but this strays a bit and is really an entirely separate issue.

As you point out, this is a non-issue with the SeaMonkey defaults.  And I'll add: SeaMonkey's Window menu now looks hot.

Comment 82

[Stefan [:stefanh]]

> 
> As you point out, this is a non-issue with the SeaMonkey defaults.  And I'll
> add: SeaMonkey's Window menu now looks hot.
>

It really does :-)

Comment 83

[Asaf Romano (gone)]

FYI http://lxr.mozilla.org/seamonkey/source/toolkit/content/widgets/menu.xml#84

Now, what this means:
  1. For normal menuitems (not menuitem-iconic), the src attribute is either nothing or something you shouldn't mess with.
  2. For menuitem-iconic, src is always mapped to the image attribute of the actual <xul:image> element (in XUL-generated menus of course).

We really should take care of the second issue, the only toolkit-agnostic way to do this is by implementing a new interface (see "implementation implements") for menuitem-iconic which would return the menuitem image uri.

Comment 84

[Christian :Biesinger (don't email me, ping me on IRC)]

I thought seamonkey, for favicons in bookmarks, never hit the network due to use of validate="never".

Comment 85

[Mark Mentovai]

Attachment: v4 (trunk) uses nsRefPtr instead of nsAutoPtr

Per bryner's request

Comment 86

[Brian Ryner (not reading)]

Comment on attachment 231475 [details] [diff] [review]
v4 (trunk) uses nsRefPtr instead of nsAutoPtr

>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ mozilla/widget/src/mac/nsMenuItemIcon.h	31 Jul 2006 20:26:14 -0000
>+class nsMenuItemIcon : public imgIDecoderObserver
>+{
>+public:
>+  nsMenuItemIcon(nsISupports* aMenuItem,
>+                 nsIMenu*     aMenu,
>+                 nsIContent*  aContent);
>+  virtual ~nsMenuItemIcon();

I don't think this class needs a virtual or public destructor.

>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ mozilla/widget/src/mac/nsMenuItemIcon.cpp	31 Jul 2006 20:26:14 -0000
>+NS_IMETHODIMP
>+nsMenuItemIcon::FrameChanged(imgIContainer*  aContainer,
>+                             gfxIImageFrame* aFrame,
>+                             nsIntRect*      aDirtyRect)
>+{
>+  return NS_ERROR_NOT_IMPLEMENTED;

For these and other notifications, it seems like you should just return NS_OK if you don't need to do anything.

>+NS_IMETHODIMP
>+nsMenuItemIcon::OnStopFrame(imgIRequest*    aRequest,
>+                            gfxIImageFrame* aFrame)
>+{
>+  nsresult rv = NS_ERROR_FAILURE;

Move the declaration of rv down to where it's first used.

>--- mozilla/widget/src/mac/nsMenuItemX.cpp	10 Jul 2006 17:50:30 -0000	1.38
>+++ mozilla/widget/src/mac/nsMenuItemX.cpp	31 Jul 2006 20:26:15 -0000
>+NS_IMETHODIMP
>+nsMenuItemX::SetupIcon()
>+{
>+  if (!mIcon.get()) return NS_ERROR_OUT_OF_MEMORY;

Any particular reason the .get() is needed here?

Can you summarize what's going on with the css changes in this patch?

Looks good otherwise.

Comment 87

[Mark Mentovai]

> I don't think this class needs a virtual or public destructor.

You're right, it won't be the superclass for anything else.  I'll make it private and nonvirtual.

> Any particular reason the .get() is needed here?

.get() is not needed, it's not ambiguous without .get().  I'll .get() rid of it.

The CSS changes prevent the folder icon from showing up for the open-in-tabs and open-livemark-items-in-tabs menu items.  In places builds, it puts back the globe icon as the default bookmark icon and favicon.  The patch also makes sure to get the right icon for livemark entries in menus.

Comment 88

[Mark Mentovai]

Attachment: v5 (trunk) for checkin

Tree's burning, can't check in now.

Comment 89

[Mark Mentovai]

Attachment: v5 (1.8[.1])

Comment 90

[Mark Mentovai]

Checked in on the trunk.

During some last-minute tests, I noticed a severe leak.  The problem was in existing code nsMenuX which I replicated in my patch.  I fixed the leak for checkin and made the existing code depend on the new fixed version.  I verified that all objects of classes nsMenuX, nsMenuItemX, and nsMenuItemIcon are properly released.  The fixed code is in nsMenuX::ChangeNativeEnabledStatusForMenuItem and nsMenuX::GetMenuRefAndItemIndexForMenuItem.  Josh, when you get a sec, I'd appreciate it if you could take a look at the changes I made to those methods.

Comment 91

[Mark Mentovai]

Comment on attachment 231495 [details] [diff] [review]
v5 (1.8[.1])

This is identical to the trunk version, except it uses 1.8.1 APIs where different from the trunk.  It doesn't modify any interfaces that shipped with 1.8.0, instead extending _MOZILLA_1_8_BRANCH interfaces and using those interfaces where appropriate.

This is high-profile polish, and I believe we've already worked out the kinks through a series of Bone Cho test builds (comments 62-82).

Comment 92

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 231495 [details] [diff] [review]
v5 (1.8[.1])

>+NS_IMETHODIMP
>+nsMenuItemIcon::OnStopRequest(imgIRequest* aRequest,
>+                              PRBool       aIsLastPart)
>+{
>+  mIconRequest->Cancel(NS_BINDING_ABORTED);
>+  mIconRequest = nsnull;
>+  return NS_OK;
>+}

I'm having trouble understanding this -- in the notification that the request is done, you cancel the request?  Then again, maybe you do want to do this for the multipart/x-mixed-replace case...

Comment 93

[Mike Schroepfer]

Mark - can you comment on risk level for the branch for this?   Would absolutely love this for the branch - but concerned at the size/complexity of the patch so close to beta2.

Comment 94

[Mark Mentovai]

David, if I don't cancel the request, I can't free it without triggering this assertion:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/modules/libpr0n/src/imgRequestProxy.cpp&rev=1.56&mark=75#72

I don't want to hold the request object longer than necessary.  If there's a better place to free it that doesn't require a call to Cancel, I'm all ears.

Schrep, the riskiest part of this patch is spinning the loop in LoadIcon to complete the load synchronously.  It's not an unsafe time for the loop to run - we process events while the menu is being tracked, and the LoadIcon call comes right before that.  There is a potential to block the main thread if the icon load doesn't complete or if the request doesn't end.  But because I'm only doing a sync load for images coming from "local" URIs (chrome:, data:, and moz-anno:), and on the 1.8 branch, only on 10.2 and 10.3, there's minimal potential for an evil block.  Testing hasn't shown that this will be a problem.  On the trunk, which currently requires sync loading even on 10.4, there's no perceptible delay in opening reasonably-sized bookmarks menus.  I wouldn't have recommended the earlier (v1, v2-trunk) versions of this patch, but I think that this implementation is workable.  I'd give it a couple of days of bake time anyway.

(And it comes with an important leak fix, but of course we'd do that on the branch in any case.)

Comment 95

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 231495 [details] [diff] [review]
v5 (1.8[.1])

Mark, after some debate at today's triage, drivers felt that this polish patch was too risky to take for the benefit it could yeild for pp in Fx2 at this stage, so it will have to wait for Fx3.

Could we get a separate bug filed on the leak, though?

Comment 96

[Wayne Woods]

I can understand the concern, though I think it's a shame that this patch missed the deadline... it would have been one of the most immediately visible improvements of Firefox 2 for Mac users. Is there still a chance it could be landed on the 1.8 branch after 1.8.1 forks? So maybe make it in by Fx 2.1 or something...

Comment 97

[Mark Mentovai]

Comment on attachment 231495 [details] [diff] [review]
v5 (1.8[.1])

The leak is bug 346878.

Beltzner suggested rerequesting approval after this has baked for a few days on the trunk without any reported problems.

Comment 98

[Mike Connor [:mconnor]]

Comment on attachment 231495 [details] [diff] [review]
v5 (1.8[.1])

a=mconnor on behalf of drivers for checkin to the 1.8 branch.

While there is risk involved, we are now willing to take the chance before b2 to take this.

Comment 99

[Mark Mentovai]

Thanks!  Checked in on MOZILLA_1_8_BRANCH before 1.8.1b2.