339893 - Unable to get Client Side Feed Reader to work on the Mac

Status: RESOLVED FIXED

(Firefox Graveyard :: RSS Discovery and Preview, defect, P2)

Description

[Marcia Knous [:marcia]]

Seen using Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.1a3) Gecko/20060531 BonEcho/2.0a3.

STR:

1. Change RSS feed reader preference and select application to use (I tried NewsFire, Vienna and NetNewsWire)
2. Click on an XML icon or the feed location in the location bar.
3. I get the feedview page and click on "Subscribe Now". The icon of the reader I have selected does show.

Result:
I get the following error in the Error console:
Error: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIProcess.init]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: file:///Users/marcia/Desktop/Bon%20Echo%20Nightly/BonEcho.app/Contents/MacOS/components/FeedConverter.js :: FRS_addToClientReader :: line 320"  data: no]
Source File: file:///Users/marcia/Desktop/Bon%20Echo%20Nightly/BonEcho.app/Contents/MacOS/components/FeedConverter.js
Line: 320

I also tested this on the Intel Mac in the QA lab and saw the same thing. Tested with a fresh profile and an existing one.

Comment 1

[Phil Ringnalda (:philor)]

I knew I'd seen that before: bug 307463

Comment 2

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: patch

Implement openApplicationWithURI on nsIMacShellService to do this properly with LaunchServices

Comment 3

[Mark Mentovai]

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

+#ifndef XP_MACOSX
       var process = 
           Cc["@mozilla.org/process/util;1"].
           createInstance(Ci.nsIProcess);
       process.init(clientApp);
       process.run(false, [uri], 1);
+#else
+      var ios = 
+        Cc["@mozilla.org/network/io-service;1"].
+        getService(Ci.nsIIOService);
+      uri = ios.newURI(uri, null, null);
+      var ss = 
+          Cc["@mozilla.org/browser/shell-service;1"].
+          getService(Ci.nsIMacShellService);
+      ss.openApplicationWithURI(clientApp, uri);
+#endif

Nit: inconsistent indentation on the Cc.

I'd rather see openApplicationWithURI moved into nsIShellService (perhaps generalized into openApplicationWithURIs?).  It should be implemented as an nsIProcess wrapper on all platforms except the Mac, which should use the LaunchServices code you've written.  The standard way to open a URI in a specific application should be to make this call through the shell service abstraction on all platforms.  This will save us from having to go through this rigamarole again in the future.

In this specific case, you do need to account for one Mac-ism in FeedConverter.  By convention, the URI that you pass to the feed reader must use the "feed" scheme.  This convention is brought to us by Safari, and I didn't even know about it until I tested this patch as-is and found it dysfunctional with some readers.  But we can't just stop there.  We need to transform http:// URIs to feed://, and non-http:// URIs to feed:scheme://URI.  It's clearer in an example:

http://www.mozilla.org/news.rdf becomes feed://www.mozilla.org/news.rdf
https://www.mozilla.org/news.rdf becomes feed:https://www.mozilla.org/news.rdf

Unfortunately, but for good reason, there's no good way to make nsIURI abuse URIs like this in the non-"http" case.  You can't set uri.scheme to anything that contains a colon, and when you set uri.spec to feed:https://whatever, the second colon will be stripped.  So it seems like this might need to be handled directly in the Mac shell service, which might make this too single-purpose, which might mean that it doesn't belong in the cross-platform shell service.

+  lfm->GetCFURL(&appURL);

Check rv?

+  CFStringRef specRef = 
+    ::CFStringCreateWithCString(NULL, spec.get(), kCFStringEncodingUTF8);
+  if (!specRef)
+    return NS_ERROR_OUT_OF_MEMORY;
+    
+  CFURLRef uri = ::CFURLCreateWithString(NULL, specRef, NULL);
+  if (!uri) {
+    ::CFRelease(specRef);
+    return NS_ERROR_OUT_OF_MEMORY;
+  }

You can simplify this by getting rid of specRef and doing:

  CFURLRef uri = ::CFURLCreateWithBytes(NULL, (const UInt8*)spec.get(),
                                        spec.Length(), kCFStringEncodingUTF8,
                                        NULL);
  if (!uri) 
    return NS_ERROR_OUT_OF_MEMORY;

+  CFArrayRef uris = ::CFArrayCreate(NULL, (void**)&uri, 1, NULL);

Cast should be to |const void|, this is a hard error in gcc 4.

Comment 4

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: patch

Comment 5

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: better patch (generic fn on shell service)

Comment 6

[Mark Mentovai]

Comment on attachment 225790 [details] [diff] [review]
better patch (generic fn on shell service)

Few minor comments.

>Index: browser/components/feeds/content/options.js
>-    fp.init(window, title, Ci.nsIFilePicker.modeOpen);
>+    fp.init(window, "Choose Client App", Ci.nsIFilePicker.modeOpen);

Go back to |title| for now and fix up the titlelessness properly later.

>Index: browser/components/feeds/src/FeedConverter.js
>         var clientApp = 
>-            prefs.getComplexValue(PREF_SELECTED_APP, Ci.nsILocalFile);
>+        prefs.getComplexValue(PREF_SELECTED_APP, Ci.nsILocalFile);

Indentation change intentional?

>+#ifdef XP_MACOSX

Put a comment here explaining that we've got to adhere to the convention learned by observing what Safari does.

>+      var ios = 
>+          Cc["@mozilla.org/network/io-service;1"].
>+          getService(Ci.nsIIOService);
>+      var macURI = ios.newURI(uri, null, null);
>+      if (macURI.scheme == "http") {
>+        macURI.scheme = "feed";
>+        uri = macURI.spec;

uri starts out as an nsIURI but turns into a string.  That's got typed-language guys like me all confused.  You're more of a js guy than me, so if you usually do this kind of thing in js, it's fine.  Otherwise, consider using a different variable when you shift types?

>Index: browser/components/shell/public/nsIShellService.idl
>+  /**
>+   * Opens an application bundle with a specific URI.
>+   * @param   application
>+   *          The bundle file

This comment is Mac-specific, but the method is in the base interface now - genericize.

>Index: browser/components/shell/src/nsWindowsShellService.cpp
>+  const char* specStr = aURI.get();

PromiseFlatCString(aURI).get() is safer.  (Same comment applies to GNOME version.)

Comment 7

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: patch for check in

Comment 8

[Darin Fisher]

Comment on attachment 226203 [details] [diff] [review]
patch for check in

a=darin (with nsIShellService_MOZILLA_1_8_BRANCH)

Comment 9

[Ben Goodger (use ben at mozilla dot org for email)]

fixed-1.8-branch, fixed-on-trunk