Closed Bug 694870 Opened 8 years ago Closed 5 years ago

Use Freedesktop standards instead of GnomeVFS and GConf to open files on Linux

Categories

(Firefox :: File Handling, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 38
Tracking Status
firefox38 --- fixed

People

(Reporter: jstpierre, Assigned: alexhenrie24)

References

Details

Attachments

(2 files, 3 obsolete files)

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:7.0.1) Gecko/20100101 Firefox/7.0.1
Build ID: 20110930134708

Steps to reproduce:

When you download a file on Linux and pick the "Open" option, Firefox shouldn't use a file dialog, but instead, just use xdg-open. It's horrendously bad trying to navigate to a program with a file dialog.

Use GtkAppChooser after the GTK+ 3 port is complete.
Confirming based on http://www.reddit.com/r/IAmA/comments/1phhx1/we_are_mozilla_ask_us_anything/cd3kx2s
Status: UNCONFIRMED → NEW
Component: General → File Handling
Ever confirmed: true
Version: 7 Branch → Trunk
Currently, Firefox depends on GnomeVFS and GConf to determine which program should open which file. This has several problems:

1. irc:// URIs have never opened correctly for me, even with the GConf registry keys set correctly.
2. GnomeVFS and GConf are both deprecated.
3. GNOME settings do not work reliably in non-GNOME desktop environments.

For years now there have been Freedesktop standards for determining file associations, and I think we should use those instead. xdg-open and friends implement these standards, but I think the GAppInfo implementation is more convenient for Mozilla.

Related: Bug 739161, bug 713802
Summary: Run... dialog should use xdg-open → Use Freedesktop standards instead of GnomeVFS and GConf to open files on Linux
This patch replaces the deprecated GnomeVFS and GConf lookups with GAppInfo lookups.

Note that I have changed nsGNOMERegistry::GetFromExtension and nsGNOMERegistry::GetFromType to take const char* instead of const nsACString&. This avoids an unnecessary conversion to nsCString when GetFromExtension calls GetFromType.

Also, I am a relatively new Mozillan, and I have probably made many mistakes out of ignorance. So please bear with me, it will probably take a few tries to get this right.
Assignee: nobody → alexhenrie24
Attachment #8562551 - Flags: review?(roc)
Attachment #8562551 - Flags: review?(ehsan)
You can test the patch by saving purple-url-handler.desktop (copied from http://askubuntu.com/a/179808/223160) to /usr/share/applications, executing `sudo update-desktop-database`, and opening irc://irc.mozilla.org/firefox in Firefox.
Comment on attachment 8562551 [details] [diff] [review]
[PATCH] Bug 694870 - Use GAppInfo to determine Linux file associations.

Forget about it, after all that trouble I just realized that all I did was duplicate nsGIOService. Just installing purple-url-handler.desktop and executing `sudo update-desktop-database` causes it to appear in the app list.
Attachment #8562551 - Flags: review?(roc)
Attachment #8562551 - Flags: review?(ehsan)
Resolving as "works for me".
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → WORKSFORME
Duplicate of this bug: 739161
Comment on attachment 8562551 [details] [diff] [review]
[PATCH] Bug 694870 - Use GAppInfo to determine Linux file associations.

Review of attachment 8562551 [details] [diff] [review]:
-----------------------------------------------------------------

This basically looks great to me! Karl is a better reviewer for this than Ehsan, especially because Ehsan is on holiday.

I assume these APIs are available in distributions going back for at least a few years?

::: uriloader/exthandler/unix/nsGNOMERegistry.h
@@ +11,5 @@
>  
>  class nsGNOMERegistry
>  {
>   public:
> +  static bool HandlerExists(const char *aSchemeOrType);

This method is ugly ... can we get rid of it and have callers always call SchemeHandlerExists and TypeHandlerExists? Hopefully they know whether they have a scheme or a MIME type...

::: uriloader/exthandler/unix/nsMIMEInfoUnix.cpp
@@ -52,5 @@
> -
> -  if (mClass ==  eProtocolInfo) {
> -    *_retval = nsGNOMERegistry::HandlerExists(mSchemeOrType.get());
> -  } else {
> -    nsRefPtr<nsMIMEInfoBase> mimeInfo = nsGNOMERegistry::GetFromType(mSchemeOrType);

Seems like we can keep checking mClass here and call the appropriate method of nsGNOMERegistry.
It seems like your patch still simplifies the code a lot. Can we still take it?
Flags: needinfo?(alexhenrie24)
(In reply to Robert O'Callahan (:roc) (out of office, slow reviews) (Mozilla Corporation) from comment #9)
> It seems like your patch still simplifies the code a lot. Can we still take
> it?

I'm all for axing the GnomeVFS and GConf support. If you want, I'll submit another patch that uses nsGIOService exclusively (and maybe I'll simplify nsGIOService a bit too). If that's something you're interested in, I'll create a separate bug report for it.
Flags: needinfo?(alexhenrie24)
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
That'd be great. Do you have try-server access to make sure your patch builds on our infrastructure? I don't know that our builders have up-to-date-enough libraries.
Yes, I have try server access. And your build servers have to have support for GAppInfo because nsGIOService.cpp already uses it.
Comment on attachment 8562624 [details] [diff] [review]
[PATCH] Only support GIO in Linux file and protocol handler.

Review of attachment 8562624 [details] [diff] [review]:
-----------------------------------------------------------------

::: uriloader/exthandler/unix/nsGNOMERegistry.cpp
@@ +13,2 @@
>  /* static */ bool
> +nsGNOMERegistry::HandlerExists(const char *aSchemeOrType)

This can only be a scheme now, so call the method SchemeHandlerExists and rename the param to aScheme.
Attachment #8562624 - Flags: review?(roc)
Attachment #8562624 - Flags: review?(karlt)
Attachment #8562624 - Flags: review+
(In reply to Robert O'Callahan (:roc) (out of office, slow reviews) (Mozilla Corporation) from comment #14)
> > +nsGNOMERegistry::HandlerExists(const char *aSchemeOrType)
> 
> This can only be a scheme now, so call the method SchemeHandlerExists and
> rename the param to aScheme.

Let's just leave it as aProtocolScheme.

If this patch is accepted, I'll submit more patches to delete all the other now-vestigial instances of nsIGnomeVFSService and nsIGConfService. Should I make a new bug for that, or reuse bug 713802?
Attachment #8562624 - Attachment is obsolete: true
Attachment #8562624 - Flags: review?(karlt)
Attachment #8563231 - Flags: review?(roc)
Attachment #8563231 - Flags: review?(karlt)
Depends on: 713802
(In reply to Alex Henrie from comment #15)
> If this patch is accepted, I'll submit more patches to delete all the other
> now-vestigial instances of nsIGnomeVFSService and nsIGConfService. Should I
> make a new bug for that, or reuse bug 713802?

New bug please. We don't want to reopen old fixed bugs.
Comment on attachment 8563231 [details] [diff] [review]
[PATCH v2] Only support GIO in Linux file and protocol handler.

Most of this looks great thanks, but the nsOSHelperAppService change would
introduce a change in behavior.

Took me while to understand the reason for removal of the GetFromExtension()
code in nsMIMEInfoUnix::LaunchDefaultWithFile(), but yes that should be
redundant based on the construction from the app in
nsGNOMERegistry::GetFromType().

> nsGNOMERegistry::GetFromExtension(const nsACString& aFileExt)

>   nsRefPtr<nsMIMEInfoBase> mi = GetFromType(mimeType);
>-  if (mi) {
>-    mi->AppendExtension(aFileExt);
>-  }

What is the effect of this change?

>-#ifdef MOZ_WIDGET_GTK
>-  nsRefPtr<nsMIMEInfoBase> gnomeInfo;
>-  if (handler.IsEmpty()) {
>-    // No useful data yet.  Check the GNOME registry.  Unfortunately, newer
>-    // GNOME versions no longer have type-to-extension mappings, so we might
>-    // get back a MIMEInfo without any extensions set.  In that case we'll have
>-    // to look in our mime.types files for the extensions.
>-    LOG(("Looking in GNOME registry\n"));
>-    gnomeInfo = nsGNOMERegistry::GetFromType(aMIMEType);
>-    if (gnomeInfo && gnomeInfo->HasExtensions()) {
>-      LOG(("Got MIMEInfo from GNOME registry, and it has extensions set\n"));
>-      return gnomeInfo.forget();
>-    }
>-  }
>-#endif
>-
>   // Now look up our extensions
>   nsAutoString extensions, mime_types_description;
>   LookUpExtensionsAndDescription(majorType,
>                                  minorType,
>                                  extensions,
>                                  mime_types_description);
> 
> #ifdef MOZ_WIDGET_GTK
>+  nsRefPtr<nsMIMEInfoBase> gnomeInfo = nsGNOMERegistry::GetFromType(aMIMEType);
>   if (gnomeInfo) {

Moving the code looks good based on the fact that HasExtensions() would always
return false, but can we keep the handler.IsEmpty() test, please?
Otherwise system settings would override user .mailcap settings.
.mailcap may be unnecessary these days, but if there is a setting in the user
.mailcap, then I think we should assume that the user has made that setting.
Attachment #8563231 - Flags: review?(karlt) → review-
(In reply to Alex Henrie from comment #15)
> If this patch is accepted, I'll submit more patches to delete all the other
> now-vestigial instances of nsIGnomeVFSService and nsIGConfService.

Removal of nsIGnomeVFSService should always be good - it is not currently compiled by default.

nsUnixSystemProxySettings.cpp will continue to need nsIGConfService as nsGSettingsService needs GIO version 2.28, but we currently aim to support versions 2.22 and newer.
(In reply to Karl Tomlinson (:karlt) from comment #17)
> >   nsRefPtr<nsMIMEInfoBase> mi = GetFromType(mimeType);
> >-  if (mi) {
> >-    mi->AppendExtension(aFileExt);
> >-  }
> 
> What is the effect of this change?

I think I was just being overzealous. I have removed this change from the patch.

> can we keep the handler.IsEmpty() test, please?

Another good catch, thanks.
Attachment #8563231 - Attachment is obsolete: true
Attachment #8565204 - Flags: review?(roc)
Attachment #8565204 - Flags: review?(karlt)
Attachment #8565204 - Flags: review?(karlt) → review+
Requesting checkin of attachment 8565204 [details] [diff] [review]
Keywords: checkin-needed
Is there a Try link handy for this? :)
Keywords: checkin-needed
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #21)
> Is there a Try link handy for this? :)

v1 of the patch had a Try link: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6586df750b28

Since you asked, I just created another Try job for v3: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e8f773781baf
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/797672558e44
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
Blocks: 1134537
Hi... is this bug really fixed?

I'm using Firefox 47.0a2 Developer Edition on Arch Linux. When I download a PDF file in Firefox and open it from the download list, it opens in GIMP. If I open it via xdg-open or Dolphin, it correctly opens in Okular. Furthermore, if I try using "Open Containing Folder", it opens the directory in Gwenview, which doesn't even list the PDF (because it's not a file type it supports).

It seems there is still something seriously broken or mismatching in the way Firefox does file associations on Linux..?
I still have the "PDF opens in Gimp" problem in Firefox 50.0.2.
Interesting, does the patch from bug 1242292 fix the issues from comment 25 and comment 26 ?
I think comment 25 and comment 26 are describing bug 1304650 which is KDE specific and probably has more to do with KDE not conforming with the default application freedesktop spec in some way or another... not entirely sure though yet.
You need to log in before you can comment on or make changes to this bug.