337474 - Client-Side feed reader applications displayed incorrectly in RSS configuration dialog on Mac

Status: RESOLVED FIXED

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

Description

[Joe Hughes]

See attached screenshot, which shows NetNewsWire incorrectly displayed

Comment 1

[Joe Hughes]

Attachment: Screenshot displaying the problem

Comment 2

[Phil Ringnalda (:philor)]

This would be the same problem as bug 332785, that we don't read /Foo.app/ as a filename, with the same dependency on bug 332784, no?

Comment 3

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

Attachment: trunk patch

1. Make nsILocalFileMac scriptable (trunk only)
2. Add bundleName attribute (trunk only) that gets the name of the application bundle
3. Read this property in the FE places that use it

After this patch is reviewed, I will make a branch one.

Comment 4

[Mark Mentovai]

Comment on attachment 226209 [details] [diff] [review]
trunk patch

>Index: browser/themes/pinstripe/browser/feeds/subscribe.css
>-#chooseClientApp {
>-  font-size: 80%;
>-}
>-

Intentional removal?

>Index: xpcom/io/nsLocalFileMac.cpp

Dead file - best to just make your method implementation here match the implementation in nsLocalFileOSX.  But I also wouldn't mind if you didn't update this file at all.  It's never built and will never be built again.  I just want to avoid introducing code that some future hacker might be tempted to treat as a reference.

>Index: xpcom/io/nsLocalFileOSX.cpp
>+NS_IMETHODIMP
>+nsLocalFile::GetBundleName(nsAString& outBundleName)
>+{
>+  PRBool isPackage = PR_FALSE;
>+  nsresult rv = IsPackage(&isPackage);
>+  if (NS_FAILED(rv) || !isPackage) {
>+    printf("*RET\n");
>+    return rv;

rv will be NS_OK if you got here because !isPackage, but you don't want to return NS_OK in this case.  (And take out the printf.)

>+  }
>+  
>+  nsAutoString name;
>+  rv = GetLeafName(name);
>+  if (NS_FAILED(rv)) {
>+    printf("*GOAT\n");

Heh!

>+    return rv;
>+  }
>+
>+  // 4 characters in ".app"
>+  outBundleName = Substring(name, 0, name.Length() - 4);

But you might have a bundle that doesn't end in ".app".  See the IsPackage implementation: something might be a bundle by virtue of having the kHasBundle bit set.  I think that for our purposes, it's OK to return the name as a whole without any extension stripping if it doesn't end in ".app".

>+  return NS_OK;
>+}

Comment 5

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

Attachment: patch, fix issues

Comment 6

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

ignore content of other patch. oops. will attach final patch for checkin later.

Comment 7

[Mark Mentovai]

Comment on attachment 226253 [details] [diff] [review]
patch, fix issues

(Ignoring portions of this patch belonging to bug 339893.)

>Index: xpcom/io/nsILocalFileMac.idl
-[uuid(614c3010-1dd2-11b2-be04-bcd57a64ffc9)]
+[scriptable, uuid(614c3010-1dd2-11b2-be04-bcd57a64ffc9)]

Change uuid.

>+   readonly attribute AString bundleName;

I think that bundleDisplayName is a better name for this attribute, with the according adjustments to the rest of the patch where it's used.  To me, bundleName would include the ".app" extension, but bundleDisplayName doesn't need to.

>Index: xpcom/io/nsLocalFileOSX.cpp

>+  PRInt32 length = name.Length();
>+  if (Substring(name, length - 4, length).EqualsLiteral(".app")) {
>+    // 4 characters in ".app"
>+    outBundleName = Substring(name, 0, name.Length() - 4);

Reuse the |length| variable here.

Comment 8

[Darin Fisher]

Comment on attachment 226253 [details] [diff] [review]
patch, fix issues

>+      if (macURI.scheme == "http") {

nit: use the schemeIs function instead

>Index: browser/components/shell/public/nsIShellService.idl

> [scriptable, uuid(d6f62053-3769-46f6-bd2b-0a1440d6c394)]
> interface nsIShellService : nsISupports
...
>+  void openApplicationWithURI(in nsILocalFile aApplication, in ACString aURI);
> };

You should change the UUID of this interface.


>Index: browser/components/shell/src/nsGNOMEShellService.cpp

>+  const char* specStr = PromiseFlatCString(aURI).get();

specStr is invalid after this line returns...


>+  PRUint32 pid;
>+  return process->Run(PR_FALSE, &specStr, 1, &pid);

this may crash because specStr points at garbage (sometimes)

the value returned from PromiseFlatCString::get() is only valid
so long as the PromiseFlatCString object is alive.  try this:

  const nsPromiseFlatCString& spec = PromiseFlatCString(aURI);
  const char* specStr = spec.get();


>Index: browser/components/shell/src/nsMacShellService.cpp

>+  const UInt8* uriString = (const UInt8*)PromiseFlatCString(aURI).get();
>+  CFURLRef uri = ::CFURLCreateWithBytes(NULL, uriString, aURI.Length(),
>+                                        kCFStringEncodingUTF8, NULL);

ditto


>Index: browser/components/shell/src/nsWindowsShellService.cpp

>+  const char* specStr = PromiseFlatCString(aURI).get();
>+  PRUint32 pid;
>+  return process->Run(PR_FALSE, &specStr, 1, &pid);

ditto


>Index: xpcom/io/nsILocalFileMac.idl

>-[uuid(614c3010-1dd2-11b2-be04-bcd57a64ffc9)]
>+[scriptable, uuid(614c3010-1dd2-11b2-be04-bcd57a64ffc9)]
> interface nsILocalFileMac : nsILocalFile
...
>+   /**
>+    * bundleName
>+    *
>+    * returns the name of the application bundle (usually the human readable name
>+    * of the application)
>+    */
>+   readonly attribute AString bundleName;
> };

Again, need to change UUID of this interface since a method is being
added.


sr=darin with those problems fixed

Comment 9

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

Attachment: patch for checkin, fixing issues

Comment 10

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

Attachment: branch patch

uses nsILocalFileMac_MOZILLA_1_8_BRANCH

Comment 11

[Asaf Romano (gone)]

Comment on attachment 226825 [details] [diff] [review]
branch patch

nsIShellService_MOZILLA_1_8_BRANCH please.

Comment 12

[Asaf Romano (gone)]

Comment on attachment 226825 [details] [diff] [review]
branch patch

Also,


      
>Index: xpcom/io/nsILocalFileMac.idl
>===================================================================
>+[scriptable, uuid(BB388DF7-F29B-478E-9639-F54D3B8A4A30)]
>+interface nsILocalFileMac_MOZILLA_1_8_BRANCH : nsISupports

This should derive from nsILocalFileMac (then you only need to keep nsILocalFileMac_MOZILLA_1_8_BRANCH in the QueryInterface macros).

Comment 13

[Darin Fisher]

Comment on attachment 226825 [details] [diff] [review]
branch patch

nsIShellService should probably be kept unchanged on the 1.8 branch.  nsIShellService_MOZILLA_1_8_BRANCH! :-/

sr=darin with that change

Comment 14

[Darin Fisher]

Comment on attachment 226825 [details] [diff] [review]
branch patch

a=darin with nsIShellService_MOZILLA_1_8_BRANCH

Comment 15

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

fixed-1.8-branch, fixed-on-trunk