Stock icons will not display if libgnomeui is not present

RESOLVED FIXED in mozilla1.9beta2

Status

()

Core
ImageLib
P3
major
RESOLVED FIXED
11 years ago
8 years ago

People

(Reporter: Tim Babych, Assigned: Michael Ventnor)

Tracking

(Blocks: 1 bug)

Trunk
mozilla1.9beta2
x86
Linux
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(6 attachments, 15 obsolete attachments)

132.49 KB, image/png
Details
107.41 KB, image/png
Details
8.35 KB, patch
roc
: review+
Details | Diff | Splinter Review
7.20 KB, patch
Details | Diff | Splinter Review
1.05 KB, patch
Details | Diff | Splinter Review
17.13 KB, patch
vlad
: review+
Mike Schroepfer
: approvalM10+
Mike Schroepfer
: approval1.9+
Details | Diff | Splinter Review
(Reporter)

Description

11 years ago
Created attachment 287563 [details]
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
(Reporter)

Comment 1

11 years ago
Created attachment 287564 [details]
alpha8 screeshot
(Reporter)

Updated

11 years ago
Attachment #287563 - Attachment description: alpha8 screenshot → nightly screenshot
(Reporter)

Updated

11 years ago
Attachment #287564 - Attachment description: nightly screeshot → alpha8 screeshot
Tim, can you hunt down a one-day regression range, by any chance?
Flags: blocking-firefox3?
(Reporter)

Comment 3

11 years ago
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

Updated

11 years ago
Blocks: 396992

Comment 4

11 years ago
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
(Assignee)

Comment 6

11 years ago
I wonder if there is a problem with 64-bit GTK. 32-bit works fine so I don't think its us...
nice to fix, but not a blocker.

Tim, do you have stock icons in other places? (prefwindow, dialogs, etc)
Flags: wanted-firefox3+
Flags: blocking-firefox3?
Flags: blocking-firefox3-
(Reporter)

Comment 8

11 years ago
I see winstripe icons in Prefs, Toolbar, Addons and other places such as Bookmark manager.
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 on attachment 287564 [details]
alpha8 screeshot

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

Updated

11 years ago
Component: General → ImageLib
Flags: wanted-firefox3+
Flags: blocking-firefox3-
Product: Firefox → Core
QA Contact: general → imagelib

Updated

11 years ago
Blocks: 233462
Flags: blocking1.9?
Keywords: regression

Updated

11 years ago
Blocks: 225148
Is this a regression?  Or were we not using stock icons in Fx2?
We weren't using them for FTP listings.  Or do you mean in general?
I mean in general, as in the OK button in the about dialog.
Flags: blocking1.9? → blocking1.9-
There are plans to use stock icons in the main nav toolbar.
renominating based on comment 14.  At that point, the browser UI will be unusable without stock icons...
Flags: blocking1.9- → blocking1.9?
(Assignee)

Comment 16

11 years ago
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.
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.
(Assignee)

Comment 18

11 years ago
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

11 years ago
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

11 years ago
Plusing and setting to P3. Please adjust if you thing this is wrong
Flags: blocking1.9? → blocking1.9+
Priority: -- → P3
(Assignee)

Comment 21

11 years ago
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?

Updated

11 years ago
Keywords: qawanted
Summary: nightlies on Linux x64 do not show icons while browsing FTP sites → nightlies on Ubuntu 7.10 x64 do not show stock icons

Updated

11 years ago
Blocks: 401279

Comment 22

11 years ago
(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.
(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?
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.
Severity: normal → major

Comment 25

11 years ago
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

11 years ago
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.
Sounds like Ubuntu ships a broken 32-bit gnome environment; this bug should get
filed upstream with them.

Comment 28

11 years ago
(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.
> 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!
I just filed Ubuntu launchpad #162993 (https://launchpad.net/bugs/162993) on this issue.

Comment 31

11 years ago
(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

11 years ago
> 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 34

11 years ago
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.

Updated

11 years ago
Duplicate of this bug: 405619

Updated

11 years ago
Duplicate of this bug: 406037

Comment 37

11 years ago
(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

11 years ago
Created attachment 291129 [details] [diff] [review]
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 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.
Attachment #291129 - Flags: review?(roc)
Attachment #291129 - Flags: review?(mconnor)
Attachment #291129 - Flags: review?(gavin.sharp)
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.
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.
I guess that's simpler than implementing fallback in all the places involved, right?
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.
bz: 'content' can fall back, but how can you specify fallback content for 'list-style-image'?
(Assignee)

Comment 45

11 years ago
Created attachment 291153 [details] [diff] [review]
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.
Assignee: nobody → ventnor.bugzilla
Status: NEW → ASSIGNED
Attachment #291153 - Flags: superreview?(roc)
Attachment #291153 - Flags: review?(roc)
Assignee: ventnor.bugzilla → nobody
Status: ASSIGNED → NEW
Component: ImageLib → Widget: Gtk
QA Contact: imagelib → gtk
Assignee: nobody → ventnor.bugzilla
Status: NEW → ASSIGNED
Summary: nightlies on Ubuntu 7.10 x64 do not show stock icons → Stock icons will not display if libgnomeui is not present
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

11 years ago
To clear a misconception, GTK+ itself does not depend on libgnomeui. libgnomeui is even deprecated by Gnome now, please see comment #33 .
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.
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)
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...
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.
That sounds pretty good to me.  I think I like it more than introducing extra selectors, etc...
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.
(Assignee)

Comment 54

11 years ago
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.
(Assignee)

Updated

11 years ago
Attachment #291153 - Attachment is obsolete: true
Attachment #291153 - Flags: superreview?(roc)
Attachment #291153 - Flags: review?(roc)
(Assignee)

Comment 55

11 years ago
Created attachment 291313 [details] [diff] [review]
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... :-(
Attachment #291313 - Flags: superreview?(roc)
Attachment #291313 - Flags: review?(roc)
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?
(Assignee)

Comment 57

11 years ago
Created attachment 291327 [details] [diff] [review]
...this time without the leaks!

Right, I probably should do that.
Attachment #291313 - Attachment is obsolete: true
Attachment #291327 - Flags: superreview?(roc)
Attachment #291327 - Flags: review?(roc)
Attachment #291313 - Flags: superreview?(roc)
Attachment #291313 - Flags: review?(roc)
(Assignee)

Comment 58

11 years ago
Created attachment 291374 [details] [diff] [review]
Global scope

Uses global scope to prevent the overhead of constantly loading and unloading libraries.
Attachment #291327 - Attachment is obsolete: true
Attachment #291374 - Flags: superreview?(roc)
Attachment #291374 - Flags: review?(roc)
Attachment #291327 - Flags: superreview?(roc)
Attachment #291327 - Flags: review?(roc)
> 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.
(Assignee)

Comment 60

11 years ago
Created attachment 291377 [details] [diff] [review]
Global scope 2

Fix review comments. gnome_init was a #define in a header, not a symbol, hence the change.
Attachment #291374 - Attachment is obsolete: true
Attachment #291377 - Flags: superreview?(roc)
Attachment #291377 - Flags: review?(roc)
Attachment #291374 - Flags: superreview?(roc)
Attachment #291374 - Flags: review?(roc)
(Assignee)

Comment 61

11 years ago
Created attachment 291484 [details] [diff] [review]
Global scope 3

Good news roc, there's a shutdown function so we aren't forced to leak the library after all.
Attachment #291377 - Attachment is obsolete: true
Attachment #291484 - Flags: superreview?(roc)
Attachment #291484 - Flags: review?(roc)
Attachment #291377 - Flags: superreview?(roc)
Attachment #291377 - Flags: review?(roc)
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!
(Assignee)

Comment 63

11 years ago
Created attachment 291504 [details] [diff] [review]
Patch 4
Attachment #291484 - Attachment is obsolete: true
Attachment #291504 - Flags: superreview?(roc)
Attachment #291504 - Flags: review?(roc)
Attachment #291484 - Flags: superreview?(roc)
Attachment #291484 - Flags: review?(roc)
(Assignee)

Comment 64

11 years ago
Created attachment 291508 [details] [diff] [review]
Patch 4.01

Er, this is the right patch.
Attachment #291504 - Attachment is obsolete: true
Attachment #291508 - Flags: superreview?(roc)
Attachment #291508 - Flags: review?(roc)
Attachment #291504 - Flags: superreview?(roc)
Attachment #291504 - Flags: review?(roc)
Attachment #291508 - Flags: superreview?(roc)
Attachment #291508 - Flags: superreview+
Attachment #291508 - Flags: review?(roc)
Attachment #291508 - Flags: review+
So, will this patch completely fix this bug, or is there more work to do after this patch lands?
Keywords: qawanted → checkin-needed
(Assignee)

Comment 66

11 years ago
(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.

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.
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
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9 M10

Updated

11 years ago
Attachment #291129 - Attachment is obsolete: true
Attachment #291129 - Flags: review?(roc)
Attachment #291129 - Flags: review?(mconnor)
Attachment #291129 - Flags: review?(gavin.sharp)
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.
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.
Filed as Bug 406868.
Component: Widget: Gtk → ImageLib
QA Contact: gtk → imagelib

Comment 72

11 years ago
Created attachment 291614 [details]
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?
(Assignee)

Comment 73

11 years ago
(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

(Assignee)

Comment 74

11 years ago
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.
Keywords: relnote

Comment 75

11 years ago
(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.

Updated

11 years ago
Attachment #291614 - Attachment is obsolete: true

Comment 76

11 years ago
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

11 years ago
s/they/there
(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
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

11 years ago
(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

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

Comment 82

11 years ago
Created attachment 291702 [details] [diff] [review]
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 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

11 years ago
Created attachment 291734 [details] [diff] [review]
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

11 years ago
Created attachment 291776 [details] [diff] [review]
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

11 years ago
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

11 years ago
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.
(Assignee)

Comment 88

11 years ago
Created attachment 291998 [details] [diff] [review]
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.
Attachment #291998 - Flags: superreview?(roc)
Attachment #291998 - Flags: review?(roc)
(Assignee)

Comment 89

11 years ago
Created attachment 292000 [details] [diff] [review]
...with the configure patch

This includes Walter's configure patch.
Attachment #291998 - Attachment is obsolete: true
Attachment #292000 - Flags: superreview?(roc)
Attachment #292000 - Flags: review?(roc)
Attachment #291998 - Flags: superreview?(roc)
Attachment #291998 - Flags: review?(roc)
(Assignee)

Comment 90

11 years ago
Created attachment 292010 [details] [diff] [review]
...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 :-)
Attachment #292000 - Attachment is obsolete: true
Attachment #292010 - Flags: superreview?(roc)
Attachment #292010 - Flags: review?(roc)
Attachment #292000 - Flags: superreview?(roc)
Attachment #292000 - Flags: review?(roc)
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?
(Assignee)

Comment 92

11 years ago
Created attachment 292013 [details] [diff] [review]
...and consistent configure checks
Attachment #292010 - Attachment is obsolete: true
Attachment #292013 - Flags: superreview?(roc)
Attachment #292013 - Flags: review?(roc)
Attachment #292010 - Flags: superreview?(roc)
Attachment #292010 - Flags: review?(roc)

Updated

11 years ago
Attachment #291734 - Attachment is obsolete: true

Comment 93

11 years ago
Created attachment 292025 [details] [diff] [review]
...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

11 years ago
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?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 95

11 years ago
(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).
(Assignee)

Comment 96

11 years ago
Created attachment 292112 [details] [diff] [review]
Complete merged patch

This should be it. This should finally be it. Oh God I hope this is correct.
Attachment #292013 - Attachment is obsolete: true
Attachment #292025 - Attachment is obsolete: true
Attachment #292112 - Flags: superreview?(roc)
Attachment #292112 - Flags: review?(roc)
Attachment #292013 - Flags: superreview?(roc)
Attachment #292013 - Flags: review?(roc)
(Assignee)

Comment 97

11 years ago
(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 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.
Attachment #292112 - Flags: review?(roc) → review+
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.
Attachment #292112 - Flags: superreview?(roc) → superreview+
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.
Attachment #292112 - Flags: approvalM10?

Comment 102

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

Sure
Attachment #292112 - Flags: approvalM10?
Attachment #292112 - Flags: approvalM10+
Attachment #292112 - Flags: approval1.9+
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
Status: REOPENED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED
(Assignee)

Comment 104

11 years ago
If more people can confirm that this works now without libgnomeui, libgnome and libgnomevfs then the relnote is no longer needed.
Keywords: relnote
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

11 years ago
(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

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

Comment 108

11 years ago
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

11 years ago
The latest nightly looks good for me as well, no warnings on the console.
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.
(Reporter)

Comment 111

11 years ago
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

Updated

8 years ago
You need to log in before you can comment on or make changes to this bug.