225148 - Implement moz-icon channel for linux/gtk2

Status: RESOLVED FIXED

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

Description

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

per the above url, this should be possible

Comment 1

[Boris Zbarsky [:bzbarsky]]

Am I missing something?  How is one supposed to use this "filename without path
information" that this function returns??

Comment 2

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

indeed a good question
looking further, my guess is
http://developer.gnome.org/doc/API/2.0/gtk/gtk-Themeable-Stock-Images.html#gtk-icon-size-from-name
or
http://developer.gnome.org/doc/API/2.0/gtk/GtkStyle.html#gtk-style-lookup-icon-set
or something like that

Comment 3

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

ok that function doesn't really work.

I was pointed to
http://cvs.gnome.org/bonsai/cvsblame.cgi?file=libgnomeui%2Flibgnomeui/gnome-icon-lookup.c&rev=&root=/cvs/gnome#212

which seems to be what we want, together with the functions from gnome-icon-theme.h.

taking bug.

Comment 4

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

or maybe GtkIconTheme, http://developer.gnome.org/doc/API/2.0/gtk/GtkIconTheme.html

Comment 5

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

Attachment: patch

GtkIconTheme requires Gtk 2.3, so that's out :)

Comment 6

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

note that this patch does have one problem: not all icons are available in all
sizes, so gnome may give me an icon that is larger than I want and expect me to
scale it... however, from this code here is it not really possible to do scaling.

there's not much I can do about this...

Comment 7

[Brian Ryner (not reading)]

Comment on attachment 141784 [details] [diff] [review]
patch

>+  if (!gnome_program_get()) {
>+    char* empty[] = { "" };
>+    gnome_init("Gecko", "1.0", 1, empty);
>+  }
>+

This will have to change at some point; see bug 230144.  I have a work in
progress patch to use brandShortName from brand.properties instead.

>+  // XXX should we pass a GnomeVFSFileInfo here?
>+  char* name = gnome_icon_lookup(t, NULL, spec.get(), NULL, NULL, type.get(), GNOME_ICON_LOOKUP_FLAGS_NONE, NULL);

It looks like you probably should, yes.  I don't think it's an issue with the
current implementation in libgnomeui, but it's not documented as optional.  You
may want to use gnome_icon_lookup_sync() instead, which does not require this
parameter (it fetches the needed info itself).

Looks good other than that, thanks for doing this.

Comment 8

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

hmm... if I use the _sync function, I can't specify a mime type. I do want to
specify one...

I guess I'll just get the GnomeVFSFileInfo myself. hm... if I just call
gnome_vfs_get_file_info that will probably make an http request for an http url...

I guess I'll only call it for file uris, and create an GnomeVFSFileInfo myself
for other types

Comment 9

[Darin Fisher]

Comment on attachment 141784 [details] [diff] [review]
patch

I mentioned over IRC that this:

> case "$MOZ_WIDGET_TOOLKIT" in
> gtk)
> 	MOZ_ENABLE_GTK=1
>     MOZ_ENABLE_XREMOTE=1
>+    MOZ_ENABLE_GNOMEUI=1
>     TK_CFLAGS='$(MOZ_GTK_CFLAGS)'
>     TK_LIBS='$(MOZ_GTK_LDFLAGS)'
> 	AC_DEFINE(MOZ_WIDGET_GTK)
>     ;;
> 
> gtk2)
>     MOZ_ENABLE_GTK2=1
>     MOZ_ENABLE_XREMOTE=1
>+    MOZ_ENABLE_GNOMEUI=1
>     TK_CFLAGS='$(MOZ_GTK2_CFLAGS)'
>     TK_LIBS='$(MOZ_GTK2_LIBS)'
>     AC_DEFINE(MOZ_WIDGET_GTK2)
>     ;;
> 
> xlib)
> 	MOZ_ENABLE_XLIB=1
>     MOZ_ENABLE_XREMOTE=1
>+    MOZ_ENABLE_GNOMEUI=1
> 	TK_CFLAGS='$(MOZ_XLIB_CFLAGS)'
> 	TK_LIBS='$(MOZ_XLIB_LDFLAGS)'
> 	AC_DEFINE(MOZ_WIDGET_XLIB)
>     ;;

Can be replaced with this:

>+if test "$_X11_TOOLKIT"
>+then

      MOZ_ENABLE_GNOMEUI=1

>+    MOZ_ARG_DISABLE_BOOL(gnomeui,
>+    [  --disable-gnomeui       Disable libgnomeui support (default: auto, optional at runtime) ],
>+        MOZ_ENABLE_GNOMEUI=,
>+        MOZ_ENABLE_GNOMEUI=force)

Comment 10

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

Attachment: patch v2

ok, changes: what darin said, plus always pass a GnomeVFSFileInfo to
gnome_icon_lookup

Comment 11

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

Comment on attachment 142196 [details] [diff] [review]
patch v2

contains a few mistakes...

Comment 12

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

Attachment: patch v3

Comment 13

[Darin Fisher]

Comment on attachment 142386 [details] [diff] [review]
patch v3

>Index: modules/libpr0n/decoders/icon/gnome/Makefile.in

>+# The contents of this file are subject to the Mozilla Public
>+# License Version 1.1 (the "License"); you may not use this file
>+# except in compliance with the License. You may obtain a copy of
>+# the License at http://www.mozilla.org/MPL/

tri-license?


>Index: modules/libpr0n/decoders/icon/gnome/nsIconChannel.cpp


>+  GnomeVFSFileInfo* fileInfo = gnome_vfs_file_info_new();
>+  nsCAutoString spec;
>+  nsCOMPtr<nsIURI> fileURI;
>+  rv = mURI->GetIconFile(getter_AddRefs(fileURI));
>+  if (NS_SUCCEEDED(rv) && fileURI) {
>+    fileURI->GetSpec(spec);

use GetAsciiSpec since we don't know for certain the GnomeVFS will be ok
with UTF-8, and ASCII-only URIs should always work.


>+    if (fileInfo && NS_SUCCEEDED(fileURI->SchemeIs("file", &isFile)) && isFile) {
>+      gnome_vfs_get_file_info(spec.get(), fileInfo, GNOME_VFS_FILE_INFO_DEFAULT);
>+    }
>+    else if (fileInfo) {

nit: these two branches could just be wrapped in one |if (fileInfo) { ... }|
block, but see below.  i don't think you need to heap allocate fileInfo.


>+      // We have to get a leaf name from our uri...
>+      nsCOMPtr<nsIURL> url(do_QueryInterface(fileURI));
>+      if (url) {
>+        nsCAutoString name;
>+        url->GetFileName(name);
>+        fileInfo->name = g_strdup(name.get());

GetFileName returns UTF-8, are you sure that is valid here?  do
you need to convert to the filesystem charset since this string
is going to be passed down to gnome?  also, what about escaped
characters in the |name| value?  it seems like you might need to
unescape name since gnome probably doesn't handle %-escaped
file names.


>+    if (fileInfo)
>+      gnome_vfs_file_info_unref(fileInfo);

nit: 

you could allocate your GnomeVFSFileInfo on the stack.	there's
no reason to allocate it on the heap.  use gnome_vfs_file_info_clear
to release any memory owned by the fileInfo object.


>+  if (!name) {
>+    g_object_unref(G_OBJECT(t));

so, this code requires GLIB-2?	do we ensure that in the autoconf
stuff?


>+  char* file = gnome_icon_theme_lookup_icon(t, name, iconSize, NULL, NULL);

what charset does this return?	necko assumes file pathes will be in the native
charset, so i think passing this string to NS_NewURI may be incorrect.	you
should
construct a local file from this path, and then call NS_NewFileURI instead. 
or, 
if you know that |file| is in UTF-8, then you need to manually convert that to
either UTF-16 or the "native" charset and still construct a nsILocalFile. 
appending
this string to "file://" is potentially very broken.

interesting detail about file:// URLs... at present they are always ASCII. 
they 
are in fact %-escaped native pathes.  you probably don't want to have to know
that
here, so that's why i recommend creating a nsILocalFile instead.

Comment 14

[Darin Fisher]

>use GetAsciiSpec since we don't know for certain the GnomeVFS will be ok
>with UTF-8, and ASCII-only URIs should always work.

of course there's the issue that we don't know what charset GnomeVFS uses for
file:// URLs.  does it use the %-escaped filesystem charset like gecko, or does
it use %-escaped UTF-8?  if the latter, then we'd need to do a messy charset
conversion.

Comment 15

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

(In reply to comment #13)
> so, this code requires GLIB-2?	do we ensure that in the autoconf
> stuff?

We do, implicitly... gnome-vfs 2 requires glib 2.

(still have to look at that charset stuff)

Comment 16

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

(In reply to comment #14)
> of course there's the issue that we don't know what charset GnomeVFS uses for
> file:// URLs.  does it use the %-escaped filesystem charset like gecko, or does
> it use %-escaped UTF-8?  if the latter, then we'd need to do a messy charset
> conversion.

The following url clarifies that gnomevfs uses the escaped native charset for
file: urls.
http://lists.gnome.org/archives/gnome-vfs-list/2004-March/msg00049.html

however, this patch requires updating:
o) allowing to enable this when the toolkit is gtk1 will lead to problems
o) some themes apparently don't provide 16x16 icons for everything, that means
e.g. download manager shows like the top 16 pixels of an 48x48 icon, that's
really ugly. I guess I'll change this to use GdkPixbuf to scale the file and
convert it to a PNG file. That may also bring support for SVG themes.
o) +    // XXX "Gecko" will have to change, see bug 230144
As that bug is basically fixed, I should change this here too...

Comment 17

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

(In reply to comment #16)
> really ugly. I guess I'll change this to use GdkPixbuf to scale the file and

hmm, I was thinking of
http://developer.gnome.org/doc/API/2.0/gtk/GtkIconTheme.html#gtk-icon-theme-load-icon
there, but that's only in gtk 2.4...

Comment 18

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

Attachment: patch v4 variant 1

OK; all changes made, except that this doesn't do any scaling

Comment 19

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

Attachment: patch v4 variant 2

variant 2: Scales using GdkPixbuf. Does not require Gtk 2.4. Looks nice in the
dl manager :)

scrolling performance is not quite ideal in the dl manager...

variant 3 requires gtk 2.4, I'll do that tomorrow.

Comment 20

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

Attachment: patch v4 variant 3

OK; this is the version that requires gtk 2.4. the code looks much nicer :) and
doesn't require creating temporary files. it's also faster.

Comment 21

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

Comment on attachment 145522 [details] [diff] [review]
patch v4 variant 2

ok... I think I prefer this patch - the unscaled icons do look ugly; but I
don't think requiring gtk 2.4 is a good idea...

re-requesting review, this added quite a bit of code

Comment 22

[Brian Ryner (not reading)]

Comment on attachment 145522 [details] [diff] [review]
patch v4 variant 2

>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ modules/libpr0n/decoders/icon/gnome/nsIconChannel.cpp	6 Apr 2004 00:38:56 -0000
>+  // scale...
>+  GdkPixbuf* scaled = gdk_pixbuf_scale_simple(buf, iconSize, iconSize, GDK_INTERP_BILINEAR);

Should we avoid this call if the starting icon size matches iconSize?

>+  // Now we have to create an uri for the file...
"a uri"

Looks good other than that.  You're right, gtk 2.4 is _much_ nicer for this.  I
wonder if it would be worth checking for the needed symbols at runtime and
using the new API if it's available.  Something to think about, but it
shouldn't hold this patch up.

Comment 23

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

Comment on attachment 145522 [details] [diff] [review]
patch v4 variant 2

wow, gdk-pixbuf itself does not seem to do that optimization...

ok; I'll change:
+  // scale...
+  GdkPixbuf* scaled = gdk_pixbuf_scale_simple(buf, iconSize, iconSize,
GDK_INTERP_BILINEAR);
+  gdk_pixbuf_unref(buf);
+  if (!scaled)
+    return NS_ERROR_OUT_OF_MEMORY;

to:
+ GdkPixbuf* scaled = buf;
+  if (gdk_pixbuf_get_width(buf)  == iconSize &&
    gdk_pixbuf_get_height(buf) == iconSize) {
+    // scale...
+    scaled = gdk_pixbuf_scale_simple(buf, iconSize, iconSize,
GDK_INTERP_BILINEAR);
+    gdk_pixbuf_unref(buf);
+    if (!scaled)
+      return NS_ERROR_OUT_OF_MEMORY;
+  }

(correctly indented, this textbox is a bit small to tell whether it is now...)

Comment 24

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

er, oops, make that
+  if (gdk_pixbuf_get_width(buf) != iconSize ||
+      gdk_pixbuf_get_height(buf) != iconSize) {

Comment 25

[Darin Fisher]

Comment on attachment 145522 [details] [diff] [review]
patch v4 variant 2

>Index: configure.in

>+if test "$_X11_TOOLKIT" && test -z "$MOZ_ENABLE_GTK"

shouldn't this test for MOZ_ENABLE_GTK2 instead?  i mean, i suppose
with this test you could theoretically use this code in the XLIB
build, but if we extend that approach couldn't we end up with the
XLIB build including some optional GTK1 stuff here and some optional
GTK2 stuff there.  might be better to make this specific to the GTK2
builds.


>Index: modules/libpr0n/decoders/icon/nsIconProtocolHandler.cpp

>+  nsIconChannel* channel = new nsIconChannel;
>+  if (!channel)
>+    return NS_ERROR_OUT_OF_MEMORY;
> 
>+  nsresult rv = channel->Init(url);
>+  if (NS_FAILED(rv)) {
>+    delete channel;
>+    return rv;
>+  }

BTW, just a heads up: this is in general a risky code pattern.
if Init() ever happened to AddRef |this| as a side-effect, and
then failed for some reason, you'd end up with a crash... 
having called delete on an object that is still referenced.  I
looked over Init and it doesn't look like a problem in this case,
but you never know what someone down the road might do.


>Index: modules/libpr0n/decoders/icon/gnome/Makefile.in

>+REQUIRES	= xpcom \
>+		  string \
>+		  necko \
>+			intl \
>+			mimetype \
>+			exthandler \
>+		  $(NULL)

indentation in REQUIRES line is inconsistent.


>Index: modules/libpr0n/decoders/icon/gnome/nsIconChannel.cpp

>+nsIconChannel::Init(nsIURI* aURI) {

>+  if (!gnome_program_get()) {
>+    // Get the brandShortName from the string bundle to pass to GNOME
>+    // as the application name.  This may be used for things such as
>+    // the title of grouped windows in the panel.
>+    nsCOMPtr<nsIStringBundleService> bundleService = 
>+      do_GetService(NS_STRINGBUNDLE_CONTRACTID);
...
>+    gnome_init(NS_ConvertUTF16toUTF8(appName).get(), "1.0", 1, empty);
>+  }

ouch!  you're adding a new libpr0n dependency on strres/intl.  this sounds like
something that requires MOA.  perhaps you could create a nsIGnomeSession type
service or something.  then you could hide all the Gnome initialization details
in there.

or, maybe we should just always initialize Gnome in the apprunner code.
why bother doing this lazily?

it seems to me that as we increase our integration with Gnome, we are going
to have more places that are going to need to perform this Gnome init logic,
which suggests that we should put it someplace central or that we should not
do it lazily.


>+  PRUint32 iconSize;
>+  nsresult rv = mURI->GetImageSize(&iconSize);
>+  if (NS_FAILED(rv))
>+    iconSize = 16;

is this really non-fatal?  what does it mean for the image to not know its
size?  in fact, it looks like nsIconURI already never returns failure here,
and it also seems to return 16 as its default value if all else fails.
how about simply asserting that this function doesn't fail since we know
that it never will?


>+  GnomeVFSFileInfo fileInfo;
>+  memset(&fileInfo, 0, sizeof(fileInfo));

maybe just do this instead ot initialize to zero:

  GnomeVFSFileInfo fileInfo = {0};


>+  fileInfo.refcount = 1; // In case some GnomeVFS function addrefs and releases it

do you think we need the same safety check in extensions/gnomevfs?

hmm, so i guess this patch makes it so that our GTK2 builds will require
GnomeVFS2.
so, there should be no reason not to allow static linking of
extensions/gnomevfs,
right?


>+  nsCAutoString spec;
>+  nsCOMPtr<nsIURI> fileURI;
>+  rv = mURI->GetIconFile(getter_AddRefs(fileURI));
>+  if (NS_SUCCEEDED(rv) && fileURI) {

GetIconFile also never fails.  maybe it is okay to count on the behavior of
code
in your own module?


>+      nsCOMPtr<nsIURL> url(do_QueryInterface(fileURI));
>+      if (url) {
>+        nsCAutoString name;
>+        // The filename we get is UTF-8, which matches gnome expectations.

no, |name| might end up containing %-escaped binary junk.  you can only 
assume that the string you get back is UTF-8 encoded, not that the
filename is UTF-8 encoded! ;-)


>+        // See also: http://lists.gnome.org/archives/gnome-vfs-list/2004-March/msg00049.html
>+        // "Whenever we can detect the charset used for the URI type we try to
>+        //  convert it to/from utf8 automatically inside gnome-vfs."
>+        url->GetFileName(name);
>+        fileInfo.name = g_strdup(name.get());
>+        nsUnescape(fileInfo.name);

now, fileInfo.name might be ISO-8859-1 or Shift-JIS.  necko only guarantees
that the string you got back from GetFileName is itself UTF-8.	once you
unescape all bets are off.  i suggest that you don't unescape since 
presumably this filename corresponds to something on a remote server, right?
or is there a reason why unescaping this string is required?



>+  // Now we have to create an uri for the file...
>+  // We know the name is ASCII
>+  nsCOMPtr<nsIURI> realURI;
>+  rv = NS_NewURI(getter_AddRefs(realURI), NS_LITERAL_CSTRING("file://") + nsDependentCString(tmpfile));
>+  if (NS_FAILED(rv)) {
>+    close(fd);
>+    remove(tmpfile);
>+    return rv;
>+  }
>+
>+  // Create a channel to file
>+  rv = NS_NewChannel(getter_AddRefs(mRealChannel), realURI);
>+  close(fd);
>+
>+  // We can't delete the file right now, the channel needs to be able to access
>+  // it

have you considered using nsIFileInputStream in conjunction with
nsIInputStreamChannel?	you can configure nsIFileInputStream to 
delete the file on close.

you could: 1) create a localfile from tmpfile, 2) create a local
file input stream from that, 3) create a nsIURI from the nsIFile
using NS_NewFileURI, and finally call NS_NewInputStreamChannel.

or do you truly need this tmpfile to hang around until the end
of the application?

Comment 26

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

+FORCE_SHARED_LIB = 1

(In reply to comment #25)
> shouldn't this test for MOZ_ENABLE_GTK2 instead?  i mean, i suppose
> with this test you could theoretically use this code in the XLIB
> build,

yep, that's what I was thinking

> but if we extend that approach couldn't we end up with the
> XLIB build including some optional GTK1 stuff here and some optional
> GTK2 stuff there.  might be better to make this specific to the GTK2
> builds.

but this is a good point. I'll change it to test for gtk2.

> BTW, just a heads up: this is in general a risky code pattern.
> if Init() ever happened to AddRef |this| as a side-effect, and
> then failed for some reason, you'd end up with a crash... 

oh, hm, good point - I'll move the NS_ADDREF below the OOM check
(I don't need a NS_RELEASE in failure case, as I use |delete| here)

> or, maybe we should just always initialize Gnome in the apprunner code.
> why bother doing this lazily?

if I were to do that, mozilla would require gnome, no?

I'll need to think about this some more...

> how about simply asserting that this function doesn't fail since we know
> that it never will?

sounds good, will do.

> >+  fileInfo.refcount = 1; // In case some GnomeVFS function addrefs and
releases it
> 
> do you think we need the same safety check in extensions/gnomevfs?

I do think that would be a good idea.

> hmm, so i guess this patch makes it so that our GTK2 builds will require
> GnomeVFS2.

eh? no, that's an optional dependency. I'm using FORCE_SHARED_LIB; so if gnome
or gnomevfs aren't present, the component will simply fail to load.

I don't think I'd want to add such a dependency.

> GetIconFile also never fails.  maybe it is okay to count on the behavior of
> code
> in your own module?

hm, true. but it certainly can return a null uri. (e.g. for moz-icon://.txt)

I guess I could remove the NS_SUCCEEDED part of the if.

> no, |name| might end up containing %-escaped binary junk.  you can only 
> assume that the string you get back is UTF-8 encoded, not that the
> filename is UTF-8 encoded! ;-)

ok :)

> now, fileInfo.name might be ISO-8859-1 or Shift-JIS.  necko only guarantees
> that the string you got back from GetFileName is itself UTF-8.	once you
> unescape all bets are off.  i suggest that you don't unescape since 
> presumably this filename corresponds to something on a remote server, right?
> or is there a reason why unescaping this string is required?

good point. I think we can avoid unescaping here, then.

> have you considered using nsIFileInputStream in conjunction with
> nsIInputStreamChannel?	you can configure nsIFileInputStream to 
> delete the file on close.

ah, excellent - I was not aware of that. I like that much more, I'll use that.

Comment 27

[Darin Fisher]

Ah.. I didn't realize that you were developing the icon decoder as a standalone
DSO.  I think this means that you will also need to go through and update all
the different installer packaging stuff (for seamonkey and firefox).

Should we maybe consolidate the gnomevfs stuff and this stuff into one DSO then?
 Unfortunately, it doesn't look like it is easy to keep parts of the icon
decoder code where it is and move other parts into extensions/gnome-support/
(<-- or whatever we choose to call it)

Comment 28

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

(In reply to comment #27)
> Ah.. I didn't realize that you were developing the icon decoder as a standalone
> DSO.  I think this means that you will also need to go through and update all
> the different installer packaging stuff (for seamonkey and firefox).

ah... right...

> Should we maybe consolidate the gnomevfs stuff and this stuff into one DSO then?
>  Unfortunately, it doesn't look like it is easy to keep parts of the icon
> decoder code where it is and move other parts into extensions/gnome-support/
> (<-- or whatever we choose to call it)

I hear caillon wants to extend this code to also support gtk-only environments.
hence, I wouldn't want this to live in a gnome-specific dir.

(this is why I renamed the directory from "gnome" to "gtk" in the next version
of the patch)

Comment 29

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

Attachment: patch v5 variant 2

Comment 30

[Darin Fisher]

Comment on attachment 148530 [details] [diff] [review]
patch v5 variant 2

>Index: modules/libpr0n/decoders/icon/nsIconProtocolHandler.cpp

>+  nsIconChannel* channel = new nsIconChannel;
>+  if (!channel)
>+    return NS_ERROR_OUT_OF_MEMORY;
>+  NS_ADDREF(channel);
> 
>+  nsresult rv = channel->Init(url);
>+  if (NS_FAILED(rv)) {
>+    delete channel;

oops, i'm sure you meant "NS_RELEASE(channel)"


>Index: modules/libpr0n/decoders/icon/gtk/nsIconChannel.cpp

>+NS_IMPL_ISUPPORTS2(nsIconChannel,
>+                   nsIRequest,
>+                   nsIChannel);

bogus semicolon.


>+static nsresult pngfile_to_channel(const char* aFilename, nsIChannel** aChannel) {

nit: add a line break after return type?  i.e.:

  static nsresult
  pngfile_to_channel(const char* aFilename, nsIChannel** aChannel) {


>Index: modules/libpr0n/decoders/icon/gtk/nsIconChannel.h

>+    nsIconChannel() {}
>+    ~nsIconChannel() {}

maybe remove these altogether?


sr=darin

Comment 31

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

ok, checked in, with comments addressed.

note that I did mean delete there (I have a concrete pointer to this class here,
and wanted to save the indirection...) but I guess NS_RELEASE is better, in case
Init passes |this| somewhere else which addrefs...

Comment 32

[Darin Fisher]

> note that I did mean delete there (I have a concrete pointer to this class here,
> and wanted to save the indirection...) but I guess NS_RELEASE is better, in case
> Init passes |this| somewhere else which addrefs...

exactly!  :)

Comment 33

[tor]

On fedora core1 this checkin causes a crash when the component is used,

./mozilla-bin: relocation error:
/home/tor/mopt/dist/bin/components/libimgicon.so: undefined symbol:
_Z20gnome_icon_theme_newv

gnome-icon-theme.h is missing its G_BEGIN_DECLS.  Adding a extern "C" {}
block around the include of this file fixes the problem.

It doesn't appear as though the chrome is using the icons properly.  With
the classic theme the download manager looks pretty messed up (image
attached).

Comment 34

[tor]

Attachment: download manager post patch

Comment 35

[Scott MacGregor]

Christian, this patch has broken the Thunderbird trunk Linux build:

http://tinderbox.mozilla.org/showbuilds.cgi?tree=Thunderbird

An excerpt of the errors:

nsIconChannel.cpp:179: error: `GnomeIconTheme' undeclared (first use this 
   function)
nsIconChannel.cpp:179: error: (Each undeclared identifier is reported only once 
   for each function it appears in.)
nsIconChannel.cpp:179: error: `t' undeclared (first use this function)
nsIconChannel.cpp:179: error: `gnome_icon_theme_new' undeclared (first use this 
   function)
nsIconChannel.cpp:186: error: `GNOME_ICON_LOOKUP_FLAGS_NONE' undeclared (first 
   use this function)
nsIconChannel.cpp:186: error: `gnome_icon_lookup' undeclared (first use this 

Can you take a look? Thanks!

Comment 36

[Darin Fisher]

perhaps christian was compiling against a new version of the gnome libs that
didn't have those problems?  older versions of the gnomevfs headers are known to
be missing extern "C" wrappers for C++.

Comment 37

[Jan Varga [:janv]]

Man, you really need scaling support in the tree widget. I just checked with
Gnome 2.6, icons in the download manager are just too large.

Comment 38

[Jan Varga [:janv]]

I forgot to mention that the scrolling was very slow due to the icon loading.

Comment 39

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

(In reply to comment #33)
> On fedora core1 this checkin causes a crash when the component is used,
> 
> ./mozilla-bin: relocation error:
> /home/tor/mopt/dist/bin/components/libimgicon.so: undefined symbol:
> _Z20gnome_icon_theme_newv
> 
> gnome-icon-theme.h is missing its G_BEGIN_DECLS.  Adding a extern "C" {}
> block around the include of this file fixes the problem.

*sigh*, gnome starts to annoy me...

> It doesn't appear as though the chrome is using the icons properly.  With
> the classic theme the download manager looks pretty messed up (image
> attached).

sorry... I made a stupid typo. should be fixed now.


(In reply to comment #37)
> Man, you really need scaling support in the tree widget. I just checked with
> Gnome 2.6, icons in the download manager are just too large.

yeah, unfortunately the checked in version only took the "scale" codepath when
requested width == real width, not !=. I checked in a fix.

I'll take a look at the thunderbird bustage in a seocnd.

Comment 40

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

OK... so http://search.cpan.org/src/MLEHMANN/Gnome2-0.49/xs/GnomeIconLookup.xs
indicates GnomeIconTheme is new in libgnomui 2.0.6
does anyone know which version of that is installed on egg?

Comment 41

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

Attachment: fix bustages (checked in)

This should fix:
o) bustage with some test versions (have to use = not ==)
o) the problem with missing extern "C" in some libgnomeui versions
o) lack of GnomeIconTheme in older libgnomeui versions

Comment 42

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

Comment on attachment 148855 [details] [diff] [review]
fix bustages (checked in)

I checked this bustage fix in; but with 2.1.0 as version, since that's what
seems to first contain the required functions

Comment 43

[Christopher Aillon (sabbatical, not receiving bugmail)]

I would recommend not using 2.1.0 as the minimum required version, as that was
an unstable development snapshot.

Comment 44

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

(In reply to comment #43)
> I would recommend not using 2.1.0 as the minimum required version, as that was
> an unstable development snapshot.

so you'd prefer it if I used 2.2.0? why should I not use the first version this
appeared in?

Comment 45

[Christopher Aillon (sabbatical, not receiving bugmail)]

(In reply to comment #44)
> so you'd prefer it if I used 2.2.0? why should I not use the first version this
> appeared in?

Yes, use 2.2.x.   2.1.x is unsupported, undocumented, not guaranteed to work,
etc.  Additionally, much like the Mozilla Foundation probably wouldn't like
random Gecko based projects to ship with random development snapshots (say of
this past weekend with the 3 major blockers that closed the entire tree) just
because they contain X feature, I don't think the GNOME Foundation would like to
have external projects relying on unstable, development versions of its software
in return.  Not only that, but I don't think the Mozilla Foundation wants to be
in a position to have to field bugs against potential crashes, or other bugs
caused by the "support" of development versions.

Comment 46

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

ok, done