467168 - migrate libgnome and libgnomeui to GTK/GIO functions

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[pbrobinson]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.4) Gecko/2008111217 Fedora/3.0.4-1.fc10 Firefox/3.0.4
Build Identifier: trunk

GNOME is mirgating away from libgnome and libgnomeui and deprecating them libraries. Details can be found here http://live.gnome.org/LibgnomeMustDie This will let us drop a lot of other indirect library dependencies too. Its similar to bug 402892 in that the functionality has been replaced with equivalents in gtk+2 and glib2. 

Reproducible: Always

Comment 1

[Arpad Borsos [:Swatinem]]

According to a MXR search for gnome_* symbols the list looks like this:
gnome_init_with_popt_table replaced by gnome_program_init which itself is still a TODO
gnome_icon_theme_new and gnome_icon_theme_lookup_icon replaced by replaced by gtk_icon_theme_new and gtk_icon_theme_lookup_icon both available since GTK+ 2.4
gnome_icon_lookup should be replaced with GIO which requires GLib 2.16. The function however can take a GtkIconTheme.
gnome_program_get seems to be a TODO
gnome_client_request_interaction, gnome_interaction_key_return, gnome_master_client and gnome_client_set_restart_command seems to be a TODO as well
gnome_url_show replaced by gtk_show_uri() which is only available in GTK+ 2.14
libgnomeui_module_info_get is only used as parameter to gnome_program_init

So as far as I can see only the gnome_icon_theme stuff can be replaced with the current dependencies. But I am no expert in this, maybe someone can enlighten me.

Comment 2

[pbrobinson]

Is there any reason the gtk+ 2.4 version is required to be maintained as the minimum. It is after all having been released in March 2004 is positively ancient given that mozilla keeps things like cairo and sqlite requirements to near the latest versions.

Comment 3

[Arpad Borsos [:Swatinem]]

Maybe you misunderstood me. GtkIconTheme is available in GTK+ since 2.4.
Mozilla requires GTK+ 2.10. But yes, 2.10 is also more than 2 years old and a bumped dependency could help with some work.

Comment 4

[Arpad Borsos [:Swatinem]]

Attachment: use GtkIconTheme and GIO instead of GnomeIconTheme

I was playing around with gio and GtkIconTheme a bit. This is what I managed to achieve.
I'm a little confused why Mozillas mime engine returns "application/x-totem-plugin" as mime-type for every file it doesn't know the exact mime-type of.
The patch is not meant for review as it uses recent glib/gio stuff that mozilla doesn't support.

Comment 5

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

(In reply to comment #2)
> Is there any reason the gtk+ 2.4 version is required to be maintained as the
> minimum. It is after all having been released in March 2004 is positively
> ancient given that mozilla keeps things like cairo and sqlite requirements to
> near the latest versions.

We ship our own cairo and SQLite versions. We don't want to ship our own glib and gtk libraries.

Comment 6

[Reed Loden [:reed]]

(In reply to comment #4)
> The patch is not meant for review as it uses recent glib/gio stuff that mozilla
> doesn't support.

Now that bug 402892 has landed, can you update your patch and request review?

Comment 7

[pbrobinson]

Any updates on this. With GNOME 3 being not far off it would be nice to see this in the Firefox "Lorentz" release

Comment 8

[pbrobinson]

Adding a request for 1.9.3 as its minor and contained.

Comment 9

[pbrobinson]

Any chance of this for Firefox 4. Gnome 3 drops support for libgnome/libgnoneui

Comment 10

[Benjamin Smedberg]

This is not a blocker.

Comment 11

[Jan Horak [:jhorak]]

Attachment: gio icons channel 1

I've created a patch which use gio for creating icon channel heavily based on 
https://bugzilla.mozilla.org/attachment.cgi?id=362929
Thanks Arpad for your patch.
GnomeUI code is still kept.

Comment 12

[Arpad Borsos [:Swatinem]]

Nice to see some progress on this again :-)
I guess I am missing something, MOZ_ENABLE_GIO is defined by the build system?

Comment 13

[Jan Horak [:jhorak]]

(In reply to comment #12)
> Nice to see some progress on this again :-)
> I guess I am missing something, MOZ_ENABLE_GIO is defined by the build system?

Yes, it is set when --enable-gio is added to .mozconfig.

Comment 14

[Hussam Al-Tayeb]

I rediffed the patch against lastest trunk from mercurial
libgnome(ui) aren't installed and neither is gnome-vfs
I built minefield with --enable-gio
it built corrently but I'm not sure the icon lookup functionality is working.
Under preferences -> applications , the icons for file types for example 'Bzip archive' don't show (this used to work in builds using libgnomeui).
After I download a new file and a new file type is added to the Applications pane, I get the following warning.

(firefox-bin:28218): Gtk-CRITICAL **: IA__gtk_icon_theme_lookup_by_gicon: assertion `G_IS_ICON (icon)' failed

(firefox-bin:28218): GLib-GObject-CRITICAL **: g_object_unref: assertion `G_IS_OBJECT (object)' failed

(firefox-bin:28218): Gtk-CRITICAL **: IA__gtk_icon_theme_lookup_by_gicon: assertion `G_IS_ICON (icon)' failed

(firefox-bin:28218): GLib-GObject-CRITICAL **: g_object_unref: assertion `G_IS_OBJECT (object)' failed

This error shows only when I download a new file type for the first time. 

Rediffed patch: http://pastebin.com/ACNFEsJC

Comment 15

[Hussam Al-Tayeb]

I was mistaken. the error messages shows everytime I download a file even if I've downloaded a file of this type before.

Comment 16

[Joe Drew (not getting mail)]

Comment on attachment 510266 [details] [diff] [review]
gio icons channel 1


>diff --git a/modules/libpr0n/decoders/icon/gtk/nsIconChannel.cpp b/modules/libpr0n/decoders/icon/gtk/nsIconChannel.cpp

>+  GtkIconInfo *iconInfo = gtk_icon_theme_lookup_by_gicon(iconTheme,
>+                          icon, iconSize,
>+                          (GtkIconLookupFlags)0);

Line up your extra arguments under the "i" in iconTheme rather than at the "g" in gtk.

I haven't done an exhaustive review of function return values for whether they need to be explicitly unreffed or explicitly freed, because I'd need to look up every function call manually. Karl, can you take a look at this? Maybe you'll be a bit quicker thanme.

>diff --git a/modules/libpr0n/decoders/icon/gtk/nsIconChannel.h b/modules/libpr0n/decoders/icon/gtk/nsIconChannel.h
>--- a/modules/libpr0n/decoders/icon/gtk/nsIconChannel.h
>+++ b/modules/libpr0n/decoders/icon/gtk/nsIconChannel.h
>@@ -71,11 +71,12 @@ class nsIconChannel : public nsIChannel 
>      * Will always be non-null after a successful Init.
>      */
>     nsCOMPtr<nsIChannel> mRealChannel;
> 
>     /**
>      * Called by Init if we need to use the gnomeui library.
>      */
>     nsresult InitWithGnome(nsIMozIconURI *aURI);
>+    nsresult InitWithGIO(nsIMozIconURI *aIconURI);

Is there any reason this needs to be a member function? (A quick reading of InitWithGIO doesn't show any use of member variables.)

Comment 17

[Jan Horak [:jhorak]]

(In reply to comment #16)
> Comment on attachment 510266 [details] [diff] [review]
> >diff --git a/modules/libpr0n/decoders/icon/gtk/nsIconChannel.h b/modules/libpr0n/decoders/icon/gtk/nsIconChannel.h
> >--- a/modules/libpr0n/decoders/icon/gtk/nsIconChannel.h
> >+++ b/modules/libpr0n/decoders/icon/gtk/nsIconChannel.h
> >@@ -71,11 +71,12 @@ class nsIconChannel : public nsIChannel 
> >      * Will always be non-null after a successful Init.
> >      */
> >     nsCOMPtr<nsIChannel> mRealChannel;
> > 
> >     /**
> >      * Called by Init if we need to use the gnomeui library.
> >      */
> >     nsresult InitWithGnome(nsIMozIconURI *aURI);
> >+    nsresult InitWithGIO(nsIMozIconURI *aIconURI);
> 
> Is there any reason this needs to be a member function? (A quick reading of
> InitWithGIO doesn't show any use of member variables.)
It use only mRealChannel. I tried to keep to same pattern as InitWithGnome.

Comment 18

[Jan Horak [:jhorak]]

Attachment: gio icons channel 2

-Unbitrotten
-Small refactoring
-Checking for correct gio object unref

Things are getting a little bit urgent now. Fedora 15 with Gnome 3, which drops gnomeui, is going to be released in late May. If we want to keep icons in Downloads and Preferences/Application we should try to push it into codebase.

Comment 19

[Jan Horak [:jhorak]]

(In reply to comment #14)
> I rediffed the patch against lastest trunk from mercurial
> libgnome(ui) aren't installed and neither is gnome-vfs
> (firefox-bin:28218): Gtk-CRITICAL **: IA__gtk_icon_theme_lookup_by_gicon:
> assertion `G_IS_ICON (icon)' failed
> 
> (firefox-bin:28218): GLib-GObject-CRITICAL **: g_object_unref: assertion
> `G_IS_OBJECT (object)' failed
> 
> (firefox-bin:28218): Gtk-CRITICAL **: IA__gtk_icon_theme_lookup_by_gicon:
> assertion `G_IS_ICON (icon)' failed
> 
> (firefox-bin:28218): GLib-GObject-CRITICAL **: g_object_unref: assertion
> `G_IS_OBJECT (object)' failed

This is also fixed in gio icons channel 2 patch, please have a look if you can spare some time with testing and report back any problems.

Comment 20

[Karl Tomlinson (:karlt)]

Comment on attachment 524059 [details] [diff] [review]
gio icons channel 2

>+  nsresult rv;
>+  PRInt32 iconSize;
>+  nsCAutoString iconSizeString;
>+  
>+  // Get icon size
>+  aIconURI->GetIconSize(iconSizeString);
>+  if (iconSizeString.IsEmpty()) {
>+    PRUint32 size;
>+    rv = aIconURI->GetImageSize(&size);
>+    iconSize = size;
>+    NS_ASSERTION(NS_SUCCEEDED(rv), "GetImageSize failed");
>+  } else {
>+    int size;
>+
>+    GtkIconSize icon_size = moz_gtk_icon_size(iconSizeString.get());
>+    gtk_icon_size_lookup(icon_size, &size, NULL);
>+    iconSize = size;
>+  }

Can you make this into a helper function to share with InitWithGnome please? 

>+  nsCAutoString type;
>+  aIconURI->GetContentType(type);

Move this to the block that uses |type|.

>+      if (!fileInfo) {
>+        g_warning("Cannot get fileinfo from %s", spec.get());

Save g_warning() for programmer errors.
If the file no longer exists, that's not a programmer error.
NS_WARNING would be fine here, if you like.

>+    if (type.IsEmpty()) {
>+      // content type cannot be found
>+      ctype = g_content_type_from_mime_type("unknown");
>+    } else {
>+      ctype = g_content_type_from_mime_type(type.get());

>+    icon = g_content_type_get_icon(ctype);

g_content_type_from_mime_type requires gio-2.0 version 2.18
so the version required in configure needs to be bumped.

g_content_type_from_mime_type() may return NULL, but g_content_type_get_icon()
is not null-safe.

Is there a reason to use "unknown" rather than the empty string?
Is "unknown" defined to have special meaning somewhere?
I don't see it in GContentType, nor
http://standards.freedesktop.org/icon-naming-spec/icon-naming-spec-latest.html,
but I do see it in some icon themes.

If it is the icon name that you are trying to match in the theme, then it
would be better to lookup by icon name rather than by GIcon.
The unknown type case could be handled in a single !iconInfo block as you have
below.

>+    free(ctype);

g_free.

>+  // Unref fileInfo while it is no longer needed by icon
>+  if (fileInfo)
>+    g_object_unref(fileInfo);

This doesn't work with the early return above.
Instead, move the fileInfo declaration to where g_file_query_info is called,
g_object_ref(icon) if g_file_info_get_icon(fileInfo) is non-NULL, and
g_object_unref(fileInfo) immediately.

>+  GtkIconInfo *iconInfo = gtk_icon_theme_lookup_by_gicon(iconTheme,
>+                          icon, iconSize,
>+                          (GtkIconLookupFlags)0);

gtk_icon_theme_lookup_by_gicon needs gtk+-2.0 version 2.14.  The requirement
can be bumped in configure.in but it needs to be conditional on --enable-gio
because our (implicitly) --disable-gio builds are made against version 2.10.

>+    // Mozillas mimetype lookup sometimes returns bogus types that don't have
>+    // icons. Try again with a unknown type.

The lack of an icon for the mimetype doesn't necessarily mean that the
mimetype is "bogus".

As mentioned above, I'm guessing you want gtk_icon_theme_lookup_icon here.

>+  // Create a GdkPixbuf buffer containing icon and scale it
>+  GError *err = nsnull;
>+  GdkPixbuf* buf = gtk_icon_info_load_icon(iconInfo, &err);
>+  gtk_icon_info_free(iconInfo);
>+  if (!buf) {
>+    if (err)
>+      g_error_free(err);
>+    return NS_ERROR_UNEXPECTED;
>+  }
>+
>+  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);
>+    g_object_unref(buf);
>+    if (!scaled)
>+      return NS_ERROR_OUT_OF_MEMORY;
>+  }
>+  // XXX Respect icon state
>+
>+  rv = moz_gdk_pixbuf_to_channel(scaled, aIconURI,
>+                                 getter_AddRefs(mRealChannel));
>+  g_object_unref(scaled);
>+  return rv;

Can you use a helper function to share this with InitWithGnome, please?

> #ifdef MOZ_ENABLE_GNOMEUI
>     return InitWithGnome(iconURI);
>+#endif
>+#ifdef MOZ_ENABLE_GIO
>+    return InitWithGIO(iconURI);
> #else
>     return NS_ERROR_NOT_AVAILABLE;

"#else if defined(MOZ_ENABLE_GIO)" would be clearer here and avoid possible
unreached-statement compiler warnings.

Comment 21

[pbrobinson]

is this going to be able to make firefox 5?

Comment 22

[Michael Ventnor]

Perhaps for mozilla-central we can drop and remove the code for GnomeUI completely since it's been deprecated for some time?

Comment 23

[Karl Tomlinson (:karlt)]

We can do that when Mozilla has build machines that can handle --enable-gio.
We'll need new builders for GTK+3 too so I expect we'll do the two changes together in mozilla.org builds.

Comment 24

[Karl Tomlinson (:karlt)]

(In reply to comment #21)
> is this going to be able to make firefox 5?

No, Firefox 5 has already branched.

Comment 25

[Karl Tomlinson (:karlt)]

(In reply to comment #20)
> g_content_type_from_mime_type requires gio-2.0 version 2.18
> so the version required in configure needs to be bumped.

> gtk_icon_theme_lookup_by_gicon needs gtk+-2.0 version 2.14.  The requirement
> can be bumped in configure.in but it needs to be conditional on --enable-gio
> because our (implicitly) --disable-gio builds are made against version 2.10.

We may need to think about this if we are going to --enable-gio for bug 147659.

The one notable distribution I see is Debian lenny, which has gtk+2.12.12 and glib 2.16.6.

AIUI lenny may have security support until about 2012-02-06?

Comment 26

[Mike Hommey [:glandium]]

(In reply to comment #25)
> AIUI lenny may have security support until about 2012-02-06?

Old stable is usually supported until one year after a new stable release, so that'd be until around that date.

Comment 27

[Jan Horak [:jhorak]]

Attachment: gio icons channel 3

- helper functions created
- gwarning removed
- In:
  >+    if (type.IsEmpty()) {
  >+      // content type cannot be found
  >+      ctype = g_content_type_from_mime_type("unknown");
  removed, same job is done in handling "if (!iconInfo)"
- using gtk_icon_theme_lookup_icon
- better #else when deciding which service to use (MOZ_ENABLE_GNOMEUI or MOZ_ENABLE_GIO or returning NS_ERROR_NOT_AVAILABLE).
Please have a look.

Comment 28

[Hussam Al-Tayeb]

(In reply to comment #27)
> Created attachment 531947 [details] [diff] [review] [review]
> gio icons channel 3
> 
> - helper functions created
> - gwarning removed
> - In:
>   >+    if (type.IsEmpty()) {
>   >+      // content type cannot be found
>   >+      ctype = g_content_type_from_mime_type("unknown");
>   removed, same job is done in handling "if (!iconInfo)"
> - using gtk_icon_theme_lookup_icon
> - better #else when deciding which service to use (MOZ_ENABLE_GNOMEUI or
> MOZ_ENABLE_GIO or returning NS_ERROR_NOT_AVAILABLE).
> Please have a look.
This patch works perfectly. It fixes application/file type icons appearing in the Applications panel.

Comment 29

[Karl Tomlinson (:karlt)]

Comment on attachment 531947 [details] [diff] [review]
gio icons channel 3

>-        PKG_CHECK_MODULES(MOZ_GIO, gio-2.0 >= $GIO_VERSION,[
>+        PKG_CHECK_MODULES(MOZ_GIO, gio-2.0 >= $GIO_VERSION gtk+-2.0 >= 2.14,[

The side-effects of PKG_CHECK_MODULES set MOZ_GIO_CFLAGS and MOZ_GIO_LIBS.
It's not really right to add gtk flags to these.

Given the way configure.in is currently ordered, probably the best thing to do
here is use a temporary veriable in a separate check.  e.g
PKG_CHECK_MODULES(_GTKCHECK, gtk+-2.0 >= 2.14)

>+nsresult 
>+GetIconSize(nsIMozIconURI *aIconURI, PRInt32 &aIconSize)

"static" for internal linkage.

Return PRInt32 or gint rather than using an out-parameter, as this is not an
XPCOM method.

>+  nsresult rv;

This is C++ code, so declare rv at the point where it is initialized by
aIconURI->GetImageSize(&size).

>+nsresult
>+ScaleIconBuf(GdkPixbuf **aBuf, PRInt32 iconSize)

"static"

>+  if (!*aBuf) {
>+    return NS_ERROR_UNEXPECTED;
>+  }

Don't bother with this check.  This is a file-local function and the callers
will pass a sane argument.

>+    GdkPixbuf *scaled = gdk_pixbuf_scale_simple(*aBuf, iconSize, iconSize,
>+                                     GDK_INTERP_BILINEAR);

Alignment issue.

>+  rv = ScaleIconBuf(&buf, iconSize);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  rv = moz_gdk_pixbuf_to_channel(scaled, aIconURI,
>+                                 getter_AddRefs(mRealChannel));

s/scaled/buf/

>+      if (fileInfo) {
>+        // icon from g_content_type_get_icon doesn't need unref
>+        icon = g_file_info_get_icon(fileInfo);

g_object_ref(icon) here if it is non-NULL.
That way icon_need_unref is unnecessary, g_object_unref(fileInfo) can be
called immediately and the fileInfo declaration can be moved to its
initialization with g_file_query_info().

>+    // Mozilla's mimetype lookup failed. Try unknown type.

'Try the "unknown" icon.' 

>+  GError *err = nsnull;
>+  GdkPixbuf* buf = gtk_icon_info_load_icon(iconInfo, &err);
>+  gtk_icon_info_free(iconInfo);
>+  if (!buf) {
>+    if (err)
>+      g_error_free(err);

Pass NULL for the GError** argument so there's nothing to free.

Comment 30

[Karl Tomlinson (:karlt)]

(In reply to comment #25)
Note for future reference:

If we do decide to --enable-gio for mozilla builds (I'd like to but no plan has been effected yet), we can replace gtk_icon_theme_lookup_by_gicon() with g_themed_icon_get_names() and gtk_icon_theme_choose_icon (), which needs only gtk+-2.12, provided by lenny.

Comment 31

[Jan Horak [:jhorak]]

Attachment: gio icons channel 4

Thanks for comments, I tried to fix them in attached patch.

Comment 32

[Karl Tomlinson (:karlt)]

Comment on attachment 539805 [details] [diff] [review]
gio icons channel 4

>+++ b/toolkit/system/gnome/Makefile.in
>@@ -60,16 +60,17 @@ ifdef MOZ_ENABLE_GNOMEVFS
> CPPSRCS += \
> 	nsGnomeVFSService.cpp \
> 	$(NULL)
> endif
> 
> ifdef MOZ_ENABLE_GIO
> CPPSRCS += \
> 	nsGIOService.cpp \
>+	nsGSettingsService.cpp \

This must be in here by mistake.

>+      GFileInfo *fileInfo = g_file_query_info(file,
>+                                   G_FILE_ATTRIBUTE_STANDARD_ICON,
>+                                   G_FILE_QUERY_INFO_NONE, NULL, NULL);

Can you touch up the indentation here, please?

> 
>-  // XXX Respect icon state
>   
>-  rv = moz_gdk_pixbuf_to_channel(scaled, aIconURI,
>+  // Try to get icon by using MIME type

>+  
>+
>+  if (!iconInfo) {

And there's a couple of double blank lines.  Can you reduce each of them to a single blank line, please?

Comment 33

[Jan Horak [:jhorak]]

Attachment: gio icons channel 5

Ouch, sorry about wrong part of code which sneak from different patch. Fixed indentation and new lines. Checkin-needed or super-rewiew?

Comment 34

[Karl Tomlinson (:karlt)]

Comment on attachment 540021 [details] [diff] [review]
gio icons channel 5

Thanks.  No interface changes so no super-review required.

Comment 35

[Phil Ringnalda (:philor)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/78ed4ddb5fbd

Comment 36

[Mounir Lamouri (:mounir)]

Merged:
http://hg.mozilla.org/mozilla-central/rev/78ed4ddb5fbd