User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a9pre) Gecko/2007110718 Firefox/3.0a9pre Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a9pre) Gecko/2007110718 Firefox/3.0a9pre Currently firefox compiles against system gnome-vfs and it seems to work fine. Under gnome 2.22, a migration to GVFS/gio is going to happen. I'm assuming distributions will ship GVFS instead of gnome-vfs. How will firefox handle the swtch to GVFS? A gvfs release is available here http://ftp.gnome.org/pub/GNOME/sources/gvfs/0.0/ http://ftp.gnome.org/pub/GNOME/sources/gio-standalone/0.1/ Reproducible: Always Steps to Reproduce: 1. 2. 3.

Is this supposed to be a bug report? GVFS/gio don't have a stable API right now and there are discussions of merging gio into glib, so I think all of this is a bit early. If you want to ask about the future plans regarding gnome-vfs, you should ask on the newsgroups or the support forums.

Oh, sorry. I didn't mean to make it look like a question. What I meant to say was. "Is this a realistic enhancement request or is it out of reach?" But yes, GVFS/gio doesn't have a stable api yet. Maybe this should be deferred until it does in the next few months. Sorry for jumping the gun.

I guess, considering that firefox binaries as distributed by mozilla are supposed to have somewhat a broad support, that it would be better for a while to keep gnomevfs and have an additionnal module for gvfs/fio.

Yeah, it'll be a long while before we can even think about getting rid of gnomevfs.

Note that mozilla.org is not shipping Linux binaries for release versions from here on out, only testing/nightly builds.

I believe the gvfs/gio api is stable now. And most of gnome is now ported to it as of 2.24 with plans to deprecate gnomevfs now I believe.

GIO is part of GLib since Version 2.16. I'm not sure what Version of GLib Gecko requires, configure.in says version 1.2, which sounds quite broken ( http://mxr.mozilla.org/mozilla-central/source/configure.in#118 ) But GTK+ 2.10 has a dependency on GLib 2.12. Only GTK+ 2.14 requires a GLib version which includes GIO.

gnomevfs support in nsIconChannel is a bit hacky, see #486925 for an example of its consequences. gvfs and gio should make much easier for non-gnome apps to use that functionality.

I have looked a little at nsIGnomeVFSService and users, namely nsGNOMERegistry, nsMIMEInfoUnix, nsLocalFileUnix and nsGNOMEShellService. I believe a lot of this can be made significantly easier by using GIOs GAppInfo and GContentType. Maybe nsIGnomeVFSService can even go completely when switching to GIO. This looks like a *lot* of work though. I may take a closer look at this at some time, I wanted to get deeper into GLib / GIO anyway.

Assigning to Jan, who has been working on this.

Jan, caillon, any news on this?

I'm still working on it.

Following patch replaces GnomeVFS by GIO in following tasks: - launch associated application for given extension, mime type or uri. - set as default application for specified mime types and file extensions

patch doesn't add the nsIGIOService.idl file

Comment on attachment 383728 [details] [diff] [review] Initial patch for GnomeVFS to GIO replacement >+ * The Original Code is the Mozilla GNOME integration code. >+ * >+ * This code is based on nsGnomeVFSService module. >+ * Portions created by the Initial Developer are Copyright (C) 2009 >+ * the Initial Developer. All Rights Reserved. >+ * >+ * Contributor(s): >+ * Jan Horak <jhorak@redhat.com> This is an invalid license header. Please make sure you use the exact text that the MPL boilerplate uses, as shown below: * The Original Code is __________________________________________. * * The Initial Developer of the Original Code is * ____________________________________________. * Portions created by the Initial Developer are Copyright (C) 2___ * the Initial Developer. All Rights Reserved. * * Contributor(s):

You have to add an email address of the person you want review from if you want to get a review. The problem is I'm not really sure who'd be suitable. bsmedberg? vlad? roc?

None of us are really familiar with this code. How about you, Michael, with me for an additional layer of super-review?

I suppose, but I won't be able to do it for a couple of weeks.

This is a straight port of nsIGnomeVFSService, good so far. Last time I skimmed through the code, it looked quite feasible to get rid of the XPCOM-style interface completely, rather using the gio functions directly instead of wrapping them with an interface/class. I don't believe this functionality is used through scripts, but if it is, it may be a better idea to think up a DOM interface that has implementations for all tier1 platforms.

Thanks for comments, sorry for missing idl file. Headers fixed.

This patch replaces nsGnomeVfsService class by nsGIOService in Mozilla source code.

(In reply to comment #20) > This is a straight port of nsIGnomeVFSService, good so far. > Last time I skimmed through the code, it looked quite feasible to get rid of > the XPCOM-style interface completely, rather using the gio functions directly > instead of wrapping them with an interface/class. This is not a so good idea, especially for people that don't care about gnome and don't want anything related to gnome on their computer (think KDE users, for example). At least, it was true for gnomevfs. I guess it'd be the same with GIO. Keeping it as a XPCOM component allows it to be loaded when gnome libraries are there and not be loaded otherwise. (In reply to comment #22) > Created an attachment (id=383890) [details] > Replace GnomeVFSService by GIOService in rest of the source code > > This patch replaces nsGnomeVfsService class by nsGIOService in Mozilla source > code. Depending on how Mozilla wants to keep backwards compatibility, it might be worth keeping the GnomeVFS layer.

I agree we want to retain the separate dynamically loaded "GNOME module" that we have now. XPCOM generally sucks, but here it seems to be reasonably well-suited so I don't think we need to rip it out. I'm not sure if we're ready to drop gnome-vfs support completely. With which GTK versions is GIO normally available? Would it make sense to abstract out the common stuff from gnome-vfs and GIO into a common superinterface, so that it's easy for code to try GIO and gnome-vfs and use whichever one is available? Thanks!!!

(In reply to comment #24) > With which GTK versions is GIO normally available? Gio has been part of glib since glib 2.16 released in March 2008. Latest release is 2.20.3

(In reply to comment #24) > I'm not sure if we're ready to drop gnome-vfs support completely. I disagree. 3.6 is so far off that a library released in March 2008 seems quite reliable on a constantly-upgrading Linux desktop. If 3.6 comes out one year after 3.5, then I think it'll be the same or longer than the period between GTK+ 2.10 and Firefox 3 that required it at a minimum. Plus, IIRC, doesn't OLPC only ship with GIO and not gnome-vfs? Less code to maintain and less interface complexity is always a good thing.

You disagree that I'm not sure? :-) Karl may have an opinion on this.

(In reply to comment #26) > (In reply to comment #24) > Plus, IIRC, doesn't OLPC only ship with GIO and not gnome-vfs? Yup, we had to branch the fedora packages in order to give up on this dependency. http://koji.fedoraproject.org/koji/buildinfo?buildID=65584 > Less code to maintain and less interface complexity is always a good thing. Not only that, but this also means that any app that embeds xulrunner as currently shipped by linux distros has to be a GNOME App, that meaning that they need to link and use deprecated libraries. For example, the browser in Sugar has to do this workaround: http://git.sugarlabs.org/projects/browse/repos/mainline/commits/c19e24ebe86bb1be38b9ff03fa2557248ebdf6b8 I lost several hours debugging this issue and I hope we can agree in that Gnome-vfs usage in mozilla is very inconvenient for embedders, specially in platforms with low resources.

Please correct me if I'm wrong: The code currently handles file extension <-> mime type <-> application relations, sets up firefox as the default app for certain types and hands out other types to the corresponding default application. For this special use case GIO itself is just a wrapper around xdg-mime, which is designed to be DE-independent. What about using xdg-mime directly? That would make both parties happy as in removing dependency on a deprecated library and not introducing a new dependency on a library that kde people dislike, claiming to be too gnome-specific.

(In reply to comment #29) > What about using xdg-mime directly? That would make both parties happy as in > removing dependency on a deprecated library and not introducing a new > dependency on a library that kde people dislike, claiming to be too > gnome-specific. For one, GIO is part of Glib which we already need. Secondly, xdg-mine is just a spec, not a library, so we'd end up needing to write our own parser for the mime type lookup which seems more work than necessary.

(In reply to comment #30) > For one, GIO is part of Glib which we already need. Though, I'm not sure it will be enough by itself. i.e. will it return interesting things without gvfs?

OK, now I have time to look at this. nsGnomeVFSService.cpp \ ... $(MOZ_GNOMEVFS_LIBS) \ etc... Can't you remove all traces of GnomeVFS? I don't like the idea of us having code for both libraries in the tree especially if only one of them will be used. By the time mozilla-central becomes a release version I expect GIO to be more common than gnome-vfs on GNOME desktops. I think it's now or never to make this hard decision and I really want it to happen. +NS_IMETHODIMP +nsGIOMimeApp::GetCanOpenMultipleFiles(PRBool* aCanOpen) +{ + // GIO/GVFS does not provide such info + aCanOpen = PR_FALSE; + return NS_OK; +} +NS_IMETHODIMP +nsGIOMimeApp::GetRequiresTerminal(PRBool* aRequires) +{ + // GIO/GVFS does not provide such info + *aRequires = PR_FALSE; + return NS_OK; +} ...and any similar functions; are they used anywhere? I can't imagine why Mozilla needs to know these things, so just remove them if you can. Also remove these: + /* boolean keys */ + const long APP_KEY_CAN_OPEN_MULTIPLE = 4; + const long APP_KEY_REQUIRES_TERMINAL = 5; It seems like we can remove a lot of ugly code by making this switch so I'd like to make it happen. Thanks a lot for your work so far!

Thanks for your comments. I've changed patch to replace GnomeVFS completely (excluding configure.in). I also removed unused parts and do some fixes to avoid leaking memory. Could you please do the review again?

Comment on attachment 385767 [details] [diff] [review] Patch v3: removed unused parts Looks good here. Thanks! roc now needs a look at this.

These distribution releases seem to have glib-2.16 available (perhaps through updates): distro release release-date Ubuntu 8.04 2008-04-24 Fedora 9 2008-05-13 openSUSE 11.0 2008-06-19 Debian 5.0 2009-02-15

Does the move to blocking1.9.2- and wanted1.9.2- mean its been dropped from 1.9.2 or just the beta. The next release of gnome will have no dependencies on gnome-vfs at all.

(In reply to comment #26) > I disagree. 3.6 is so far off that a library released in March 2008 seems quite > reliable on a constantly-upgrading Linux desktop. > If 3.6 comes out one year after 3.5, then I think it'll be the same or longer > than the period between GTK+ 2.10 and Firefox 3 that required it at a minimum. > Plus, IIRC, doesn't OLPC only ship with GIO and not gnome-vfs? > Less code to maintain and less interface complexity is always a good thing. It looks like we're going to try to release 3.6 this year, seriously enough that we're nearly feature frozen. If we're successful, is it really OK for a release in November/December 2009 to depend on a library that was first released in March 2008, so that everyone who hasn't upgraded their distro within the last 18 months loses functionality? I'm not convinced. The way I see it we have three options: 1) Put GIO in 1.9.2; people who haven't upgraded their distros will yell at us and say we hate freedom because we're not supporting them 2) Stick with GVFS for 1.9.2; people on the bleeding edge will yell at us and say we hate freedom because we're not supporting the latest and greatest 3) Support both; people will yell at us for being bloated However, given that most distros are already on ridiculously short support cycles, we're probably already going to support most of them longer than the distro vendors themselves, so I vote for option #1.

However, our build machines are still on Centos5, so they can't build this. They'll have to be upgraded before we can land this.

+#if MOZ_GIO_VERSION >= 220 + // For Glib >= 2.20 we can get commandline: + const char *cmd = g_app_info_get_commandline(mApp); So if you build with Glib > 2.20, your build won't run on anything less than 2.20? That seems bad. Seems like we should do the dlsym dance here. +nsGIOService::GetMimeTypeFromExtension(const nsACString &aExtension, + nsACString& aMimeType) Please be consistent, I suggest nsACString&. + char *content_type = g_content_type_guess(fileExtToUse.get(), + NULL, + 0, + &result_uncertain); + if (!content_type) + return NS_ERROR_FAILURE; + + char *mime_type = g_content_type_get_mime_type(content_type); + if (!mime_type) + return NS_ERROR_FAILURE; You leak content_type if mime_type is null. +nsGIOService::GetAppForMimeType(const nsACString &aMimeType, + nsIGIOMimeApp** aApp) Unnecessary whitespace. + if (app_info) { + nsGIOMimeApp *mozApp = new nsGIOMimeApp(app_info); + NS_ENSURE_TRUE(mozApp, NS_ERROR_OUT_OF_MEMORY); + NS_ADDREF(*aApp = mozApp); + } else { + return NS_ERROR_FAILURE; + } + g_free(content_type); You leak content_type if app_info is null. + char *content_type = + g_content_type_from_mime_type(PromiseFlatCString(aMimeType).get()); + if (!content_type) + return NS_ERROR_FAILURE; + + char *desc = g_content_type_get_description(content_type); + if (!desc) + return NS_ERROR_FAILURE; You leak content_type if desc is null. + if (file) + g_object_unref(file); + if (spec) + g_free(spec); Are these functions not null-safe? +nsGIOService::CreateAppFromCommand(nsACString const &cmd, + nsACString const &appName, Unnecessary whitespace +#if MOZ_GIO_VERSION >= 220 + // For Glib >= 2.20 we can compare commandline: + if (strcmp(g_app_info_get_commandline(app_info_from_list), + PromiseFlatCString(cmd).get()) == 0) Again, it doesn't seem good to not run on glib < 2.20 Otherwise the patch looks good, modulo the issues about dependencies. This would be a lot easier to take if we could at least support both GVFS and GIO in our source tree for an interim period.

(In reply to comment #39) > This would be a lot easier to take if we could at least support both GVFS and > GIO in our source tree for an interim period. Oh dear, that's in direct contradiction to my review comment which said to completely strip gnome-vfs from the tree... I still think having both in the tree is nonsensical, and if it has to miss 3.6 then as long as 3.6 really is a fast release I suppose everyone can wait just a little longer. I can :-)

Comment on attachment 385767 [details] [diff] [review] Patch v3: removed unused parts diablo (maemo4.1) uses glib 2.12 We don't want to break our primary public embedding customer :) fremantle (maemo5.0) will have 2.20, but for the time being, breaking our public embedding target is not acceptable. There's nothing wrong with having configure able to try to support both, and dropping the ones it can't, and to runtime dynamically loading whichever ones work/are available.

In addition to timeless' concern about glib versions on Maemo, I am interested in unifying the desktop/maemo code we currently have in the tree. If you look at the trunk nsLocalFileUnix::Reveal, you'll find some Maemo/Hildon code in there as well: http://mxr.mozilla.org/mozilla-central/source/xpcom/io/nsLocalFileUnix.cpp#1664 Same here (and several other places in the file): http://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/unix/nsMIMEInfoUnix.cpp#87 I'm hoping that the new GIO support will "Just Work" on Maemo too. If not, I'd like to move the #ifdef stuff into the service, instead of scattering it around the tree. Thoughts?

Thanks for the review. I've tried to fix leaks and also rewrote code dependent on Glib > 2.20 to use dlopen/dlsym. Can you look into it please?

This resolves all my comments, thanks! Except for the issue about removing GVFS support, I still think we can't do that yet :-(

Are we planning this for 3.6? If not, I don't think gnomevfs will be necessary.

That might depend on what happens with Maemo. IIUC, currently Fennec is aimed at Maemo 4.1, which does not have gio. I don't know when Maemo 5 will be available, and it's not clear to me that 4.1 users will be able to upgrade. What this means is that we'll at least need configure options to choose gnome-vfs and/or gio. If we are going to keep a gnome-vfs module then we might as well make the choice between gio and gnome-vfs run-time selectable for older systems. (IIUC the XPCOM API can be the same and only one module will be loaded, so there is [almost] no bloat.) If the gio/gnome-vfs choice happens at run-time, then this probably could go into 1.9.2. For the free desktop Linux distributions listed in comment 35, this is the projected end of security support for releases with glib < 2.16. distro release end-of-life Ubuntu 6.06 LTS 2009-06-01 Fedora 8 2009-12-25 openSUSE 10.3 2009-10-31 Debian 4.0 2010-02-15

> distro release end-of-life > > Ubuntu 6.06 LTS 2009-06-01 > Fedora 8 2009-12-25 > openSUSE 10.3 2009-10-31 > Debian 4.0 2010-02-15 Fedora 8 was EOL on 2009-01-07. Fedora 9 was EOL on 2009-07-10 and it did has gio http://fedoraproject.org/wiki/LifeCycle/EOL

Thanks for correcting my one-month-after-two-subsequent-releases math which was 1 year out there, sorry.

>+NS_IMETHODIMP >+nsGIOMimeApp::SetAsDefaultForMimeType(nsACString const& aMimeType) >+{ >+ char *content_type = >+ g_content_type_from_mime_type(PromiseFlatCString(aMimeType).get()); >+ if (!content_type) >+ return NS_ERROR_FAILURE; >+NS_IMETHODIMP >+nsGIOService::GetAppForMimeType(const nsACString& aMimeType, >+ nsIGIOMimeApp** aApp) >+{ >+ *aApp = nsnull; >+ >+ char *content_type = >+ g_content_type_from_mime_type(PromiseFlatCString(aMimeType).get()); >+ if (!content_type) >+ return NS_ERROR_FAILURE; >+NS_IMETHODIMP >+nsGIOService::GetDescriptionForMimeType(const nsACString& aMimeType, >+ nsACString& aDescription) >+{ >+ char *content_type = >+ g_content_type_from_mime_type(PromiseFlatCString(aMimeType).get()); >+ if (!content_type) >+ return NS_ERROR_FAILURE; >+ It looks like g_content_type_from_mime_type() is only available from GLib 2.18: http://library.gnome.org/devel/gio/unstable/ix03.html Should we require GLib 2.18 for this XPCOM module or is there a reasonable way of doing this that would work with 2.16?

Okay, I've put back GnomeVFS and did required changes to the source. I also wrote own version of g_content_type_from_mime_type function to be able to compile under 2.16. Could you be so kind and check the last patch?

Seems like the changes to build/wince/tools/Makefile should not be here. Otherwise looks good ... except I'm not sure about defaulting to building with gio yet. It seems to me that if we build with gio then the gnome component will fail to load when gio is not available so the gnomevfs paths won't work either.

But I think this patch would be fine if the default was for gio to be disabled in configure.in.

Sorry for wince makefile diff, I've modified configure.in to not to enable gio by default. Thanks for comments.

Comment on attachment 396184 [details] [diff] [review] Patch v6: do not enable gio by default, removed wince makefile [Checkin: Comment 56] Thanks, let's get this in!

http://hg.mozilla.org/mozilla-central/rev/2572312e17df Thanks!!!

I get this compile error when building with --enable-gio: nsGIOService.o: In function `nsGIOService::CreateAppFromCommand(nsACString const&, nsACString const&, nsIGIOMimeApp**)': nsGIOService.cpp:(.text+0x98): undefined reference to `g_app_info_get_all' nsGIOService.cpp:(.text+0x147): undefined reference to `g_app_info_get_name' nsGIOService.cpp:(.text+0x29b): undefined reference to `g_app_info_create_from_commandline' nsGIOService.o: In function `nsGIOService::ShowURI(nsIURI*)': nsGIOService.cpp:(.text+0x33b): undefined reference to `g_app_info_launch_default_for_uri' nsGIOService.o: In function `nsGIOService::ShowURIForInput(nsACString const&)': nsGIOService.cpp:(.text+0x3a0): undefined reference to `g_file_new_for_commandline_arg' nsGIOService.cpp:(.text+0x3b3): undefined reference to `g_file_get_uri' nsGIOService.cpp:(.text+0x3ce): undefined reference to `g_app_info_launch_default_for_uri' nsGIOService.o: In function `nsGIOMimeApp::GetName(nsACString&)': nsGIOService.cpp:(.text+0x429): undefined reference to `g_app_info_get_name' nsGIOService.o: In function `nsGIOService::GetMimeTypeFromExtension(nsACString const&, nsACString&)': nsGIOService.cpp:(.text+0x53c): undefined reference to `g_content_type_guess' nsGIOService.cpp:(.text+0x54c): undefined reference to `g_content_type_get_mime_type' nsGIOService.o: In function `nsGIOMimeApp::SetAsDefaultForFileExtensions(nsACString const&)': nsGIOService.cpp:(.text+0x694): undefined reference to `g_app_info_set_as_default_for_extension' nsGIOService.o: In function `nsGIOMimeApp::GetSupportedURISchemes(nsIUTF8StringEnumerator**)': nsGIOService.cpp:(.text+0x75d): undefined reference to `g_vfs_get_default' nsGIOService.cpp:(.text+0x76e): undefined reference to `g_vfs_get_supported_uri_schemes' nsGIOService.o: In function `nsGIOMimeApp::Launch(nsACString const&)': nsGIOService.cpp:(.text+0x91c): undefined reference to `g_app_info_launch_uris' nsGIOService.o: In function `nsGIOMimeApp::GetExpectsURIs(int*)': nsGIOService.cpp:(.text+0x98b): undefined reference to `g_app_info_supports_uris' nsGIOService.o: In function `nsGIOMimeApp::GetId(nsACString&)': nsGIOService.cpp:(.text+0x99f): undefined reference to `g_app_info_get_id' nsGIOService.o: In function `get_content_type_from_mime_type(char const*)': nsGIOService.cpp:(.text+0x9c3): undefined reference to `g_content_types_get_registered' nsGIOService.cpp:(.text+0x9d4): undefined reference to `g_content_type_get_mime_type' nsGIOService.o: In function `nsGIOService::GetDescriptionForMimeType(nsACString const&, nsACString&)': nsGIOService.cpp:(.text+0xa72): undefined reference to `g_content_type_get_description' nsGIOService.o: In function `nsGIOService::GetAppForMimeType(nsACString const&, nsIGIOMimeApp**)': nsGIOService.cpp:(.text+0xaf9): undefined reference to `g_app_info_get_default_for_type' nsGIOService.o: In function `nsGIOMimeApp::SetAsDefaultForMimeType(nsACString const&)': nsGIOService.cpp:(.text+0xbad): undefined reference to `g_app_info_set_as_default_for_type' /usr/bin/ld: libmozgnome.so: hidden symbol `g_app_info_get_name' isn't defined /usr/bin/ld: final link failed: Nonrepresentable section on output collect2: ld returned 1 exit status make[4]: *** [libmozgnome.so] Error 1 System is x86_64 with glib 2.20.4.

The GIO header file needs to be added to config/system-headers

Landed this as http://hg.mozilla.org/mozilla-central/rev/a1a1591eee2a

(In reply to comment #59) > Created an attachment (id=396676) [details] > system-headers bustage fix > > Landed this as http://hg.mozilla.org/mozilla-central/rev/a1a1591eee2a I just pushed an additional fix to js/src/config to sync the system-headers files as unit test requires.

(In reply to comment #60) > I just pushed an additional fix to js/src/config to sync the system-headers > files as unit test requires. Somebody should put a big warning in config/system-headers about that. :(

Can this become part of the "Lorentz" release?