402742 - Stock icons will not display if libgnomeui is not present

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect, P3)

Description

[Tim Babych]

Attachment: nightly screenshot

Once bug 294800 landed, FTP sites were showing up pretty and iconful, but that is not so anymore.

With Alpha8 I see icons, with nightlies Oct-Nov I do not.

Ubuntu Linux 7.10, x64

Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.9a9pre) Gecko/2007110604 Minefield/3.0a9pre

Comment 1

[Tim Babych]

Attachment: alpha8 screeshot

Comment 2

[Boris Zbarsky [:bzbarsky]]

Tim, can you hunt down a one-day regression range, by any chance?

Comment 3

[Tim Babych]

has icons:
Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.9a9pre) Gecko/2007092204 Minefield/3.0a9pre


no icons:
Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.9a9pre) Gecko/2007092304 Minefield/3.0a9pre

Comment 4

[[:Aleksej]]

WFM with 32‑bit userland on Debian testing.
Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.9a9pre) Gecko/2007110604 Minefield/3.0a9pre

Comment 5

[Boris Zbarsky [:bzbarsky]]

Checkins in that range:

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-09-22+02&maxdate=2007-09-23+06&cvsroot=%2Fcvsroot

Looks like this is when we switched to stock icons in bug 396992....

Comment 6

[Michael Ventnor]

I wonder if there is a problem with 64-bit GTK. 32-bit works fine so I don't think its us...

Comment 7

[Mike Connor [:mconnor]]

nice to fix, but not a blocker.

Tim, do you have stock icons in other places? (prefwindow, dialogs, etc)

Comment 8

[Tim Babych]

I see winstripe icons in Prefs, Toolbar, Addons and other places such as Bookmark manager.

Comment 9

[Mike Connor [:mconnor]]

What about the GTK-style icons on buttons in dialogs etc?  If stock icons just plain don't work on x64, we should move this to Core and fix that.

Comment 10

[Dão Gottwald [:dao]]

Comment on attachment 287564 [details]
alpha8 screeshot

The "OK" button in the about dialog should have a stock icon. Obviously it's not there.

Comment 11

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Is this a regression?  Or were we not using stock icons in Fx2?

Comment 12

[Boris Zbarsky [:bzbarsky]]

We weren't using them for FTP listings.  Or do you mean in general?

Comment 13

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

I mean in general, as in the OK button in the about dialog.

Comment 14

[Dão Gottwald [:dao]]

There are plans to use stock icons in the main nav toolbar.

Comment 15

[Boris Zbarsky [:bzbarsky]]

renominating based on comment 14.  At that point, the browser UI will be unusable without stock icons...

Comment 16

[Michael Ventnor]

But is this really us and not faulty x64 GTK? I can't find anything in nsIconChannel.cpp that stands out as not 64-bit safe.

Comment 17

[Boris Zbarsky [:bzbarsky]]

If you're asking me, I have no idea.

Do users really care why exactly it is that their browser has no more icons on the main toolbar after an update?  If it's not us, we shouldn't be using stock icons there.

Comment 18

[Michael Ventnor]

http://mxr.mozilla.org/seamonkey/source/modules/libpr0n/decoders/icon/gtk/nsIconChannel.cpp

Can anyone here who has an x64 machine add some debug symbols or printf's to their build and say exactly which path the icon rendering is taking? It might be returning one of the error codes.

This is a typical stock icon URL that can be used for testing:
moz-icon://stock/gtk-go-back?size=toolbar

Comment 19

[Teune van Steeg]

Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9b2pre) Gecko/2007111005 Minefield/3.0b2pre
Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.9b2pre) Gecko/2007110804 Minefield/3.0b2pre

FYI I have no problems with neither my self build (x86_64) nor the nightly build (i686) on Fedora7. 

Comment 20

[Mike Schroepfer]

Plusing and setting to P3. Please adjust if you thing this is wrong

Comment 21

[Michael Ventnor]

Comment #19 said that stock icons worked on their 64 bit build, so I'm not sure if it is our problem. Can anyone else with 64-bit confirm the problem or not?

Comment 22

[Sylvain Pasche]

(In reply to comment #21)
> Comment #19 said that stock icons worked on their 64 bit build, so I'm not sure
> if it is our problem. Can anyone else with 64-bit confirm the problem or not?

I see the same issue as the reporter. With my own 64bit builds, I have stock icons but I don't see them with the nightly builds (i686).

I'm using Ubuntu 7.10 x86_64. Ubuntu has a different way of packaging things for 32bit, like having one big ia32-libs package containing all 32bits libraries. There may be missing things in the 32bit packages.

If I try to open the moz-icon://stock/gtk-go-back?size=toolbar image in a nightly build, I'm getting a "Firefox doesn't know how to open this address, because the protocol (moz-icon) isn't associated with any program." message.

Comment 23

[Dão Gottwald [:dao]]

(In reply to comment #22)
> With my own 64bit builds, I have stock
> icons but I don't see them with the nightly builds (i686).

What about release builds? Firefox 2?

Comment 24

[Dão Gottwald [:dao]]

Note that we're now using stock icons for the navigation toolbar, which means that we should get much more feedback if this is a widespread problem.

Comment 25

[Sylvain Pasche]

here's the issue:

ldd components/libimgicon.so |grep "not found"
        libgnomeui-2.so.0 => not found
        libbonoboui-2.so.0 => not found
        libgnome-keyring.so.0 => not found
        libgnome-2.so.0 => not found
        libgnomevfs-2.so.0 => not found

The ia32-libs Ubuntu package does not contains all of the needed 32bit gnome libraries for accessing the stock icons. I can also confirm that I see no icons in the toolbar with the latest nightly.

Comment 26

[İsmail Dönmez]

GTK+ stock icon support should not depend on Gnome libraries anyway. 32bit nightly build with cairo-gtk2 enabled build with no Gnome support doesn't show toolbar icons here.

Comment 27

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Sounds like Ubuntu ships a broken 32-bit gnome environment; this bug should get
filed upstream with them.

Comment 28

[İsmail Dönmez]

(In reply to comment #27)
> Sounds like Ubuntu ships a broken 32-bit gnome environment; this bug should get
> filed upstream with them.
> 

Does that mean GTK+ stock icon support depends on Gnome libraries? Thats not really a wise decision. Its just lots of extra dependencies just to display icons.

Comment 29

[Boris Zbarsky [:bzbarsky]]

> Does that mean GTK+ stock icon support depends on Gnome libraries? 

Yes.  On gnome_icon_lookup and gnome_icon_theme_lookup_icon to be exact.

If you know a way to achieve equivalent functionality without the GNOME dependency, please do tell!

Comment 30

[Reed Loden [:reed]]

I just filed Ubuntu launchpad #162993 (https://launchpad.net/bugs/162993) on this issue.

Comment 31

[İsmail Dönmez]

(In reply to comment #29)
> > Does that mean GTK+ stock icon support depends on Gnome libraries? 
> 
> Yes.  On gnome_icon_lookup and gnome_icon_theme_lookup_icon to be exact.
> 
> If you know a way to achieve equivalent functionality without the GNOME
> dependency, please do tell!

Well thats gonna be fun for distributions, I'll check with GTK+ guys if there is a GTK+ way to do this.

Comment 32

[Sylvain Fourmanoit]

> If you know a way to achieve equivalent functionality without the GNOME
> dependency, please do tell!

Wouldn't it be possible to steal it from libgnomeui without too much work? I
had a look at the GTK+ file chooser, and they pretty much re-implement it
internally[1]. Apparently, other applications such as gedit are planning taking
this road as well[2].

If it's too much trouble, now that stock icons are used in the navigation bar
by default (i.e. now that gnomestripe has been committed to the trunk), I think
it would be nicer if a stub moz-icon:// handler showing some default "missing"
icon was implemented for the one of us not having gnome installed: the
interface would feel less broken this way, and users could quickly spot the
problem.

--
[1]http://svn.gnome.org/viewvc/gtk%2B/trun/gtk/gtkfilechooserdefault.c?view=mar$
[2]http://blogs.gnome.org/pbor/2007/09/24/delivering-the-killing-blow-to-libgno$

Comment 33

[Reed Loden [:reed]]

(In reply to comment #32)
> [1]http://svn.gnome.org/viewvc/gtk%2B/trun/gtk/gtkfilechooserdefault.c?view=mar$

http://svn.gnome.org/viewvc/gtk%2B/trunk/gtk/gtkfilechooserdefault.c?view=markup

> [2]http://blogs.gnome.org/pbor/2007/09/24/delivering-the-killing-blow-to-libgno$

http://blogs.gnome.org/pbor/2007/09/24/delivering-the-killing-blow-to-libgnome/

Comment 34

[Jon Burgess]

I saw this on Fedora 8 x86_64. The previous/next/home icons were all missing. syp on irc.mozilla.org pointed me to this bug.

I found the libraries were missing as per comment 25. It was fixed by doing:
 
# yum install libgnomeui.i386

Then I needed to restart minefield and create a new profile before the icons returned.

Comment 37

[Walter Meinl]

(In reply to comment #23)
> What about Firefox 2?
> 
Firefox 2 uses also some few stock icons on buttons in the pref panel (toolkit/themes/gnomestripe/global/button.css. These are also missig when MOZ_ENABLE_GNOMUI is not defined or libgnomui.so is not present. However, as you have still the text on the buttons, it is not so obvious.
In principle the usage of GNOME stock icons pulls in an unconditional dependency on libgnomeui, thus building with disable-gnomeui should be removed from configure.in. But as a workaround we could also build winstripe for those who don't want to pull in libgnomeui. This would however not be a solution for distros who don't pack libgnomeui.so in their libs.

Comment 38

[Walter Meinl]

Attachment: This would build winstripe if libgnomeui isn't there

As a workaround use winstripe theme if libgnomeui isn't present. This patch doesn't touch toolkit/themes/gnomestripe/global/global.css and button.css cause those look broken with winstripe and the corresponding text is still displayed. This helps only when building yourself - not helpful for distros omitting libgnomeui.

Comment 39

[Reed Loden [:reed]]

Comment on attachment 291129 [details] [diff] [review]
This would build winstripe if libgnomeui isn't there

If we can't come up with a better fix by b2, we might should take a wallpaper like this. :/

Requesting r?gavin/mconnor for normal browser/toolkit review (whoever gets to it first) and r?roc to see if he thinks this is the best fix for now until we figure something else out to deal with this.

Comment 40

[Reed Loden [:reed]]

Comment on attachment 291129 [details] [diff] [review]
This would build winstripe if libgnomeui isn't there

You probably also need to add some ifdefs around the moz-icon:// stuff in toolkit/themes/gnomestripe/global/button.css and toolkit/themes/gnomestripe/global/global.css so that winstripe's version is used instead.

Comment 41

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We have to have a solution so that the same release build will display stock icons when libgnomeui is present and will display some other icons when libgnomeui is not present (e.g. for KDE users). So a build-time only solution is a short-term hack but I think the real solution really does block Firefox 3 so we'd better work on that now anyway.

I think we have to ship the winstripe icons as part of gnomestripe and find some way of making the stock icon CSS rules conditional on libgnomeui being present. One way that I think would actually be quite nice is to create a new nsILookAndFeel value to signal whether stock icons are available and use the :-moz-system-metric CSS pseudo-class in theme stylesheets to select the icon. In gtk2/nsLookAndFeel.cpp we would check whether stock icons are available. We'd add an atom here ("stock-icon-available")?
http://mxr.mozilla.org/seamonkey/source/layout/style/nsCSSRuleProcessor.cpp#749
by querying the new nsILookAndFeel value.

Comment 42

[Boris Zbarsky [:bzbarsky]]

I guess that's simpler than implementing fallback in all the places involved, right?

Comment 43

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The only problem I see with that approach is that it doesn't tell us *which* icons are available. We should define the nsILookAndFeel value to mean "GTK 2.6 stock icons are available" or something.

This is incredibly lame, GTK+ should provide at least a fallback icon instead of having these APIs just fail when GNOME libraries are missing.

Comment 44

[Robert O'Callahan (:roc) (email my personal email if necessary)]

bz: 'content' can fall back, but how can you specify fallback content for 'list-style-image'?

Comment 45

[Michael Ventnor]

Attachment: Somewhat of a runtime check

This adds a system metric, it does a test-the-waters icon rendering and if nobody's home then it will cause the check to fail and thus not attach the system metric. I haven't made anything to use it yet so this patch by itself won't fix the bug but it brings us very close to CSS-only fallbacks.

Comment 46

[Boris Zbarsky [:bzbarsky]]

We could invent some -moz syntax for it, but yeah.  It's a bit of a pain.

I kinda wish we weren't abusing that property through all of XUL.  :(

Comment 47

[İsmail Dönmez]

To clear a misconception, GTK+ itself does not depend on libgnomeui. libgnomeui is even deprecated by Gnome now, please see comment #33 .

Comment 48

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think there are actually two bugs here.

1) failure to display icons for file types. In our code this depends on libgnomeui. Maybe it doesn't need to. That is what this bug is about.

2) failure to display generic GTK+ stock icons. This is what, say, bug 406037 is about. This does not need to depend on libgnomeui, as far as I can tell, although maybe it does in our code since it's in a file that links directly to libgnomeui.

I think we should reopen bug 406037 and fix issue #2 there, and change the summary on this bug to focus on issue #1.

Either way it seems to me the patches here are not actually needed since they're trying to fix issue #2.

Comment 49

[Michael Monreal [:micmon]]

Problem 1): there is no need to depend on libgnomeui for this. The icons are pulled from the icon theme... gnome-icon-theme for example, on which Firefox itself does not depend. Using moz-icon, we CAN display not only the icons GTK ships, but also the ones that are only in the fdo icon theme:

moz-icon://stock/package-x-generic?size=menu

This works, but only displays the icon if it is available in the active theme.

So what could be done here is to test if a list of theme icons are available when FF starts and else fall back on icons that are shipped with firefox, perhaps? If this would be possible, we could use the same for some other icons like window-new and tab-new (which have icons in fdo themes but not GTK, so currently we only use the ones shipping with FF)

Comment 50

[Boris Zbarsky [:bzbarsky]]

One other option is that the moz-icon impl could do the fallback itself.  That is, define skin URIs that particular stock icons would fall back on, and the newChannel call or whatever could just create a channel for that URI if there is no such stock icon.

That won't help with the file type icons, of course, which are what this bug is about initially...

Comment 51

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think comment #49 is really addressing issue #2, right?

bz: I thought of that, but it doesn't really extend across apps unless we create new infrastructure for apps to install their own fallback mappings. How about this alternative: allow moz-icon URLs of the form

moz-icon://stock/package-x-generic?size=menu#chrome://....
or
moz-icon://stock/package-x-generic?size=menu&fallback=chrome://...

where the fallback is encoded in the rest of the URL somehow.

Comment 52

[Boris Zbarsky [:bzbarsky]]

That sounds pretty good to me.  I think I like it more than introducing extra selectors, etc...

Comment 53

[Michael Monreal [:micmon]]

Robert, I was talking about issue #1... but I don't really understand how gnomeui is used, so perhaps what I say does not make any sense. I think GTK has access to the fdo theme somehow, as pure GTK apps (not using gnomeui) can also make use of themed icons to override the GTK stock ones.

Anyway, the new moz-icon scheme you proposed sounds great and it would allow the use of fdo-icon-spec-only icons in the toolbar, too.

Comment 54

[Michael Ventnor]

Hm, I'm not sure about the idea of an extra parameter. Its a good idea but it leaves us at the mercy of the image decoder since there is no way in CSS for us to tell which image is being returned.

Because of this, we cannot make new rule declarations to do things like -moz-image-region. Whereas the previous idea with system metrics would allow us to do that and thus not force us to create separate images for every single fallback icon.

Comment 55

[Michael Ventnor]

Attachment: Soft dependency on libgnomeui

This will remove the hard dependency that the icon decoder has on libgnomeui, by changing the libgnomeui function calls to soft library symbol lookups. Since the dependency is now soft this should mean that you no longer need libgnomeui to get the stock icons, which actually use a GTK function.

Libgnomeui was necessary for filetype icons so no libgnomeui means no download manager icons, but this was always the behaviour when libgnomeui was not found.

I'll want a review quickly since B2 freeze is too close... :-(

Comment 56

[Reed Loden [:reed]]

Comment on attachment 291313 [details] [diff] [review]
Soft dependency on libgnomeui

Are you not going to do PR_UnloadLibrary somewhere so you don't leak the original load?

Comment 57

[Michael Ventnor]

Attachment: ...this time without the leaks!

Right, I probably should do that.

Comment 58

[Michael Ventnor]

Attachment: Global scope

Uses global scope to prevent the overhead of constantly loading and unloading libraries.

Comment 59

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> gLibGnomeUi 

gLibGnomeUI

move all the FindFunctionSymbol calls under "if (!gTriedToLoadLibGnomeUI)", and if any of the functions are missing, PR_FreeLibrary the library and set gLibGnomeUI to null.

Comment 60

[Michael Ventnor]

Attachment: Global scope 2

Fix review comments. gnome_init was a #define in a header, not a symbol, hence the change.

Comment 61

[Michael Ventnor]

Attachment: Global scope 3

Good news roc, there's a shutdown function so we aren't forced to leak the library after all.

Comment 62

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Move the "if (!gTriedToLoadLibGnomeUI)" code to its own static helper function, say "EnsureLibGnomeLoaded()". Put all the PR_FindFunctionSymbol calls together and then just have one "if (!_gnome_init || !_gnome_icon_theme_new || ...)" statement.

+  if (gLibGnomeUI) {
+    PR_UnloadLibrary(gLibGnomeUI);
+    gLibGnomeUI = nsnull;
+  }

Also set gTriedToLoadLibGnomeUI to false so we can start up again if we need to.

Otherwise looks just right!

Comment 63

[Michael Ventnor]

Attachment: Patch 4

Comment 64

[Michael Ventnor]

Attachment: Patch 4.01

Er, this is the right patch.

Comment 65

[Reed Loden [:reed]]

So, will this patch completely fix this bug, or is there more work to do after this patch lands?

Comment 66

[Michael Ventnor]

(In reply to comment #65)
> So, will this patch completely fix this bug, or is there more work to do after
> this patch lands?

This patch should completely fix the bug, but since I don't have a machine that doesn't have libgnomeui I can't really tell.

Comment 67

[Dão Gottwald [:dao]]

If you're going to close this bug, a new one should be filed for this:

(In reply to comment #53)
> Anyway, the new moz-icon scheme you proposed sounds great and it would allow
> the use of fdo-icon-spec-only icons in the toolbar, too.

Comment 68

[Reed Loden [:reed]]

Checking in modules/libpr0n/decoders/icon/Makefile.in;
/cvsroot/mozilla/modules/libpr0n/decoders/icon/Makefile.in,v  <--  Makefile.in
new revision: 1.25; previous revision: 1.24
done
Checking in modules/libpr0n/decoders/icon/gtk/nsIconChannel.cpp;
/cvsroot/mozilla/modules/libpr0n/decoders/icon/gtk/nsIconChannel.cpp,v  <--  nsIconChannel.cpp
new revision: 1.14; previous revision: 1.13
done

Comment 69

[Robert O'Callahan (:roc) (email my personal email if necessary)]

None of the moz-icon fallback schemes we've worked out so far seem all that appealing, so I'm not that excited about using fdo-icon-spec-only icons. Someone can file a bug for it I guess.

This patch does not fix my issue #1: icons for file types not appearing when lignomeui is unavailable. Someone should definitely file a bug on that. It's not very high priority though.

Comment 70

[Michael Monreal [:micmon]]

I'll file the follow-up bug. Having no mime icons in 3 places (ftp listings, applications list, download manager) doesn't seem to be the way to go.

Comment 71

[Michael Monreal [:micmon]]

Filed as Bug 406868.

Comment 72

[Sylvain Fourmanoit]

Attachment: post-commit minefield screenshot

(In reply to comment #66)
> This patch should completely fix the bug, but since I don't have
> a machine that doesn't have libgnomeui I can't really tell.

Enclosed screenshot was taken on a linux x86_64 / gtk2+ build including the newly commited fix, on a system without libgnomeui installed: unfortunately, it doesn't seem to work yet. Can anyone confirm or infirm this?

Comment 73

[Michael Ventnor]

(In reply to comment #72)
> Enclosed screenshot was taken on a linux x86_64 / gtk2+ build including the
> newly commited fix, on a system without libgnomeui installed: unfortunately, it
> doesn't seem to work yet. Can anyone confirm or infirm this?
> 

Did you build this yourself? Currently you still need libgnomeui-dev for the icon code to be linked at compile-time. Try a build from the official server which has libgnomeui headers.

/me makes a note to fix this for self-builders

Comment 74

[Michael Ventnor]

Drivers, this is what I want you to relnote:

If you wish to build Firefox from source then the libgnomeui headers must be installed and the configure option --enable-gnomeui must be set, otherwise icons may not appear in the navigation toolbar and other places where native GTK icons are used.

Comment 75

[Sylvain Fourmanoit]

(In reply to comment #73)
> Did you build this yourself?

Yes.

> Currently you still need libgnomeui-dev for the icon code to be linked
> at compile-time.

I can see that now... I should have looked at your code: thanks for the
explanation.

> Try a build from the official server which has libgnomeui headers.

Just installed libgnomeui with headers, rebuilt mozilla, removed
libgnomeui, and it does work.

Comment 76

[İsmail Dönmez]

IMHO This fix is not good enough they are distributions which don't distribute libgnomeui and this is a regression from Firefox2. Toolbar icons should be shown even if libgnomeui headers not found.

Comment 77

[İsmail Dönmez]

s/they/there

Comment 78

[Dão Gottwald [:dao]]

(In reply to comment #76)
> IMHO This fix is not good enough they are distributions which don't distribute
> libgnomeui and this is a regression from Firefox2. Toolbar icons should be
> shown even if libgnomeui headers not found.

-> bug 406868

Comment 79

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Actually, we could have this code build when MOZ_ENABLE_GNOMEUI is not set, and use #ifdef around the libgnomeui headers, and #ifdef out the file type icon code. Then you'd still get icons in the Firefox UI even if libgnomeui is not available at build time.

Comment 80

[İsmail Dönmez]

(In reply to comment #79)
> Actually, we could have this code build when MOZ_ENABLE_GNOMEUI is not set, and
> use #ifdef around the libgnomeui headers, and #ifdef out the file type icon
> code. Then you'd still get icons in the Firefox UI even if libgnomeui is not
> available at build time.

That would be very nice indeed.

Comment 81

[ojab]

Yeah. I don't want to have libgnomeui installed, but having icons will be very good :)

Comment 82

[Michael Ventnor]

Attachment: Patch using ifdefs

I don't know if this compiles w/o libgnomeui, but knowing the recent quality of my code lately I doubt it :P

This needs testers before I ask for review, but it will need to happen quickly because the floodgates are starting to open for Beta 2.

Comment 83

[Reed Loden [:reed]]

Comment on attachment 291702 [details] [diff] [review]
Patch using ifdefs

Can you file a new bug for this? Also, http://mxr.mozilla.org/firefox/source/toolkit/library/libxul-config.mk#279 needs to be fixed, too.

Comment 84

[Sylvain Fourmanoit]

Attachment: Patch 2 using ifdefs

(In reply to comment #82)
> I don't know if this compiles w/o libgnomeui
Here is a revision of your patch: it now compiles here on a gtk2+ system without libgnomeui (no headers nor lib), and the moz-icon:// protocol is present in the resulting built.

It's hard for me to tell if it works as expected though: I seems to only get 
"missing icons" instead of the expected stock icons (such as gtk-go-back) though the moz-icon handler, while I get a few "Gtk-WARNING **: Error loading theme icon '' for stock: Icon '' not present in theme" on the console.
Looking at comment #83, I wonder if it's not related to me screwing the xul.

Comment 85

[Walter Meinl]

Attachment: enable icon decoder in non gnomui builds would belong to patch2

(In reply to comment #84)
> Created an attachment (id=291734) [details]
> Patch 2 using ifdefs

I need in addition a patch for configure, otherwise icon decoder gets stripped and not built at all with disable-gnomeui and the icons are missing.

> while I get a few "Gtk-WARNING **: Error loading
> theme icon '' for stock: Icon '' not present in theme" on the console.
Unfortunately, I end up then with same result

Comment 86

[Sylvain Pasche]

By the way, there is still no icons displayed on Ubuntu 7.10 x86_64 (which was the initial issue of this bug).

 ldd libimgicon.so|grep not
        libgnomevfs-2.so.0 => not found
        libgnome-2.so.0 => not found

Ubuntu misses these 32bit libraries so that the component is not loaded.

Comment 87

[logan]

I just tried a recent hourly for Linux (2007120604) on unstable Debian and I'm also not seeing any moz-icon's without first installing libgnomeui-0.

Comment 88

[Michael Ventnor]

Attachment: Follow up: more soft runtime dependencies

This makes both libgnome and libgnomevfs into soft dependencies, leaving only GTK2 as the hard library dependency and it should allow anyone to get stock icons after this has been checked in.

Comment 89

[Michael Ventnor]

Attachment: ...with the configure patch

This includes Walter's configure patch.

Comment 90

[Michael Ventnor]

Attachment: ...and the ifdefs

Since we are using types from those libraries, we need to ifdef out the code so that it will compile on machines without those libraries. If my coding is correct, this patch should fix every problem in this bug. Of course I'm frequently wrong so testers, you know what to do :-)

Comment 91

[Reed Loden [:reed]]

Comment on attachment 292010 [details] [diff] [review]
...and the ifdefs

>+ifeq (gtk2,$(MOZ_WIDGET_TOOLKIT))
...
>+ifneq (,$(filter gtk2,$(MOZ_WIDGET_TOOLKIT)))

Why not use the same type of check for both of these?

Comment 92

[Michael Ventnor]

Attachment: ...and consistent configure checks

Comment 93

[Sylvain Fourmanoit]

Attachment: ...and gtk2+ CFLAGS

(In reply to comment #90, comment #92)

> If my coding is correct, this patch should fix every problem in this bug

just tested it on a "libgnomeui-less" gtk2+ linux box (with the small fix above); it does build and run fine: all the icons in the navigation bar, menus, tabs, etc. are back at last... Michael, thanks for your work!

Comment 94

[Ginn Chen]

For comment #86, comment #87
Icons are missing on Solaris, too

There's a typo in
http://lxr.mozilla.org/seamonkey/source/modules/libpr0n/decoders/icon/gtk/nsIconChannel.cpp#206

should be
!_gnome_icon_lookup
not
!gnome_icon_lookup

Do we have a bug open for this?

Comment 95

[Walter Meinl]

(In reply to comment #93)
> Created an attachment (id=292025) [details]
> ...and gtk2+ CFLAGS
> 
Yes, the gtk2+ CFLAGS are needed, then it works. I tested also building with
enable-gnomeui and running on a system lacking libgnome{ui} and gnome-vfs to simulate the Ubuntu situation - and it works, too. Icons are displayed.

Finally, searching for the term "gnomeui" at mxr.mozilla.org I got aware that the build time dependency on libgnomeui up to now seemed to be only necessary to get the icondecoder built. Thus, it appears that after Micheal's patch the libgnomeui dependency could be completely removed from the code (if building the icondecoder with libgnomeui in favor over without libgnomeui doesn't have additional advantages I don't see).

Comment 96

[Michael Ventnor]

Attachment: Complete merged patch

This should be it. This should finally be it. Oh God I hope this is correct.

Comment 97

[Michael Ventnor]

(In reply to comment #95)
> (In reply to comment #93)
> Finally, searching for the term "gnomeui" at mxr.mozilla.org I got aware that
> the build time dependency on libgnomeui up to now seemed to be only necessary
> to get the icondecoder built. Thus, it appears that after Micheal's patch the
> libgnomeui dependency could be completely removed from the code (if building
> the icondecoder with libgnomeui in favor over without libgnomeui doesn't have
> additional advantages I don't see).

libgnomeui is needed for file type icons, for example the icons in the download manager. We also need it for the object types that are used in the icon code. Its just that after this patch, they're completely optional like in Firefox 2.

Comment 98

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 292112 [details] [diff] [review]
Complete merged patch


I guess this looks fine, one change:

>+#ifdef MOZ_ENABLE_GNOMEUI
> nsresult
> nsIconChannel::InitWithGnome(nsIMozIconURI *aIconURI)
> {
>   nsresult rv;
> 
>-  if (NS_FAILED(ensure_libgnomeui()))
>+  if (NS_FAILED(ensure_libgnomeui()) || NS_FAILED(ensure_libgnome()) || NS_FAILED(ensure_libgnomevfs())) {
>+    gTriedToLoadGnomeLibs = PR_TRUE;
>     return NS_ERROR_NOT_AVAILABLE;
>-  
>-  if (!gnome_program_get()) {
>+  }
>+
>+  gTriedToLoadGnomeLibs = PR_TRUE;

Just set gTriedToLoadGnomeLibs above the if(), don't do it in both the if clause and after it.

Comment 99

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 292112 [details] [diff] [review]
Complete merged patch

I think Vlad's comment is wrong, if you do that, ensure_lib* will skip loading the libraries.

I think the code as written is correct, but confusing. ensure_lib* should not check gTriedToLoadGnomeLibs internally. Instead InitWithGnome should just check it and only call ensure_lib* if it's not set.

Having said that, I think we could land the patch as-is for beta2.

Comment 100

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Er yes, roc's right.

Comment 101

[Reed Loden [:reed]]

Comment on attachment 292112 [details] [diff] [review]
Complete merged patch

Want to land this for b2, as it fixes missing icons on Solaris, 64-bit machines, and people without the proper gtk2/gnome libs.

Comment 102

[Mike Schroepfer]

Comment on attachment 292112 [details] [diff] [review]
Complete merged patch

Sure

Comment 103

[Reed Loden [:reed]]

Checking in modules/libpr0n/decoders/icon/Makefile.in;
/cvsroot/mozilla/modules/libpr0n/decoders/icon/Makefile.in,v  <--  Makefile.in
new revision: 1.26; previous revision: 1.25
done
Checking in modules/libpr0n/decoders/icon/nsIconModule.cpp;
/cvsroot/mozilla/modules/libpr0n/decoders/icon/nsIconModule.cpp,v  <--  nsIconModule.cpp
new revision: 1.12; previous revision: 1.11
done
Checking in modules/libpr0n/decoders/icon/gtk/nsIconChannel.cpp;
/cvsroot/mozilla/modules/libpr0n/decoders/icon/gtk/nsIconChannel.cpp,v  <--  nsIconChannel.cpp
new revision: 1.16; previous revision: 1.15
done
Checking in modules/libpr0n/decoders/icon/gtk/Makefile.in;
/cvsroot/mozilla/modules/libpr0n/decoders/icon/gtk/Makefile.in,v  <--  Makefile.in
new revision: 1.5; previous revision: 1.4
done
Checking in toolkit/library/libxul-config.mk;
/cvsroot/mozilla/toolkit/library/libxul-config.mk,v  <--  libxul-config.mk
new revision: 1.55; previous revision: 1.54
done
Checking in modules/libpr0n/decoders/Makefile.in;
/cvsroot/mozilla/modules/libpr0n/decoders/Makefile.in,v  <--  Makefile.in
new revision: 1.15; previous revision: 1.14
done
Checking in configure.in;
/cvsroot/mozilla/configure.in,v  <--  configure.in
new revision: 1.1895; previous revision: 1.1894
done

Comment 104

[Michael Ventnor]

If more people can confirm that this works now without libgnomeui, libgnome and libgnomevfs then the relnote is no longer needed.

Comment 105

[Reed Loden [:reed]]

Comment on attachment 292112 [details] [diff] [review]
Complete merged patch

> PR_STATIC_CALLBACK(void)
> IconDecoderModuleDtor(nsIModule* aSelf)
> {
>-#ifdef MOZ_ENABLE_GNOMEUI
>   nsIconChannel::Shutdown();
>-#endif
> }

This removal was wrong, as it caused all non-Linux machines to burn due there only being a nsIconChannel::Shutdown() implementation for Linux and not all platforms. I readded the #ifdef but made it check for MOZ_WIDGET_GTK2 instead of MOZ_ENABLE_GNOMEUI.

Comment 106

[Sylvain Fourmanoit]

(In reply to comment #104)
> If more people can confirm that this works now without libgnomeui, libgnome and
> libgnomevfs then the relnote is no longer needed.

Just tried: it does work fine under those conditions, at least on Linux / GTK2+.

Comment 107

[Walter Meinl]

builds fine w/o libgnomeui runs w/o libgnomeui, libgnome and libgnomevfs. Thanks for making it work

Comment 108

[Brian J. Conway]

The latest Linux hourly is working well with a non-libgnomeui install, just a bunch of gtk warnings in my console to look into (currently using Clearlooks, haven't look at Debian's default Raleigh theme). My default icon set is ugly as sin, but that's more of a Debian thing to work out. Thanks for all your work, b2pre is usable for me again.

Comment 109

[logan]

The latest nightly looks good for me as well, no warnings on the console.

Comment 110

[Ori Avtalion (salty-horse)]

I'm using 64-bit Ubuntu.
With the nightly build, most of the icons are displayed except gtk-cancel and gtk-stop.

The error:
(firefox-bin:8381): Gtk-WARNING **: Error loading theme icon 'gtk-cancel' for stock: Unable to load image-loading module: /usr/lib32/gtk-2.0/2.10.0/loaders/svg_loader.so: /usr/lib32/gtk-2.0/2.10.0/loaders/svg_loader.so: cannot open shared object file: No such file or directory

Indeed, I don't have a 32-bit version of the svg_loader.

Comment 111

[Tim Babych]

if I understood right this bug is no longer about ubuntu x64 and only about libgnomeui

For missing ubuntu libs see comment #30, you will need 32bit versions of 
librsvg2
librsvg2-common
libgsf
libcroco3