460599 - Allow external web feed application to work without gnome libraries

Status: RESOLVED FIXED

(Firefox Graveyard :: RSS Discovery and Preview, enhancement)

Description

[Mike Hommey [:glandium]]

Attachment: patch

Currently, an external web feed application is called with a nsIShellService, which is provided on linux by the libmozgnome component. This component can't be loaded when the system doesn't have gnome installed, so external web feed applications don't work.

The attached patch falls back to nsIProcess to run the web feed application.

Comment 1

[Myk Melez [:myk] [@mykmelez]]

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

>+      try {
          ...
>+        try { ss.openApplicationWithURI(clientApp, spec); } catch(e) {}
>+      } catch(e) {
          ...

I don't have a Linux system without GNOME, so I tested this by replacing the openApplicationWithURI call with a throw statement.

Because the inner catch block catches and discards the exception that openApplicationWithURI presumably throws when GNOME is not on the system, the outer catch block never gets triggered.  For this code to work, the inner try/catch would need to be removed.

Without that extraneous inner try/catch, the outer catch block handles the exception, and nsIProcess seems to work just fine: when my selected feed reader is liferea-add-feed (the script responsible for adding feeds to the Liferea feed reader), and Liferea is running (a prerequisite for the script to work), clicking a feed icon subscribes me to that feed in Liferea.

Note: I'm not a peer for the module in question, so this review is unofficial, and you'll need review of subsequent patches from a module peer/owner before checking in the fix.  But hopefully this review will make that person's job easier.

Comment 2

[Mike Hommey [:glandium]]

When GNOME is not on the system, the failure happens on getting the component, not on the openApplicationWithURI. You can simulate this by removing components/libmozgnome.so.

Comment 3

[Myk Melez [:myk] [@mykmelez]]

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

(In reply to comment #2)
> When GNOME is not on the system, the failure happens on getting the component,
> not on the openApplicationWithURI. You can simulate this by removing
> components/libmozgnome.so.

Erm, sorry, I misunderstood.  I've now simulated the problem by removing components/libmozgnome.so, and indeed, the functionality fails without the patch and works with it.  r=myk

Comment 4

[:Gavin Sharp [email: [e-mail]]]

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

>diff --git a/browser/components/feeds/src/FeedConverter.js b/browser/components/feeds/src/FeedConverter.js

>+      try {
>+        var ss =
>+            Cc["@mozilla.org/browser/shell-service;1"].
>+            getService(Ci.nsIShellService);
>+        try { ss.openApplicationWithURI(clientApp, spec); } catch(e) {}
>+      } catch(e) {

Add a "retrieving the shell service might fail on some systems" comment here?

Why are you adding a try/catch around openApplicationWithURI (and p.run for that matter)? Either remove them or have their catch blocks use Components.utils.reportError to report the exception.

Comment 5

[Mike Hommey [:glandium]]

(In reply to comment #4)
> Why are you adding a try/catch around openApplicationWithURI (and p.run for
> that matter)?

I thought it wasn't expected to have the nsIProcess fallback if only openApplicationWithURI fails, which means we do have the gnome shell service.
Maybe an approach with only getService in a try/catch block and the rest in a if statement would be better?

Comment 6

[Mike Hommey [:glandium]]

Attachment: alternative patch

I was referring to something like this in comment #5. How about this approach?

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 344150 [details] [diff] [review]
alternative patch

This patch is effectively the same, since it also adds a try/catch around the openApplicationWithURI call that wasn't there before.

You're right that it's likely to be useless to fall back to nsIProcess if openApplicationWithURI somehow fails (especially since the windows and linux openApplicationWithURI implementations just use nsIProcess anyways), but I don't think it really hurts (and shouldn't really ever happen).

Attachment 343737 [details] [diff] with the inner try/catch removed is fine.

Comment 8

[Mike Hommey [:glandium]]

(In reply to comment #7)
> (From update of attachment 344150 [details] [diff] [review])
> This patch is effectively the same, since it also adds a try/catch around the
> openApplicationWithURI call that wasn't there before.

Arg. I meant to remove it, actually.

Comment 9

[Mike Hommey [:glandium]]

Attachment: first patch, v2 [Checkin: Comment 10]

Same as the first patch, without nested try/catch, and with comments.

Comment 10

[Serge Gautherie (:sgautherie)]

Comment on attachment 344241 [details] [diff] [review]
first patch, v2
[Checkin: Comment 10]

http://hg.mozilla.org/mozilla-central/rev/3917edfc1506

Comment 11

[echoes]

with this patch merged it means i'll be able to subscribe to feeds with akregator from firefox correct?