264648 - Implement nsOSHelperAppService::GetApplicationDescription for MacOS X

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect, P2)

Description

[Christian :Biesinger (don't email me, ping me on IRC)]

to be able to show an application description in the confirmation dialog for
external protocols, nsOSHelperAppService::GetApplicationDescription should be
implemented.

this bug is about the OS X implementation.

Comment 1

[Asaf Romano (gone)]

Attachment: patch v1

This leaks, releasing the bundle causes a crash on a second call (?) to this
method. I might fix it by checking its retain count, but I really shoudn't have
to...

Comment 2

[Simon Fraser [no longer active]]

Comment on attachment 189769 [details] [diff] [review]
patch v1

>Index: uriloader/exthandler/mac/nsOSHelperAppService.cpp
>===================================================================


>+    if (err == noErr) {
>+      CFBundleRef handlerBundle = ::CFBundleCreate(NULL, handlerBundleURL);

Is this the right way to get the bundle given a URL? Seems a little weird.

>+      if (handlerBundle) {
>+        // Get the human-readable name of the default handler bundle
>+        CFStringRef bundleName =
>+          (CFStringRef)::CFBundleGetValueForInfoDictionaryKey(handlerBundle,
>+                                                              kCFBundleNameKey);
>+        if (bundleName) {
>+          nsAutoBuffer<UniChar, 255> buffer;
>+          CFIndex bundleNameLength = ::CFStringGetLength(bundleName);
>+          buffer.EnsureElemCapacity(bundleNameLength);
>+          ::CFStringGetCharacters(bundleName, CFRangeMake(0, bundleNameLength),
>+                                  buffer.get());
>+          _retval.Assign(buffer.get(), bundleNameLength);
>+          rv = NS_OK;
>+          ::CFRelease(bundleName);

Don't release the result of CFBundleGetValueForInfoDictionaryKey! It't not a
'create' or 'copy' call, so it doesn't return a retained value!

Comment 3

[Asaf Romano (gone)]

Attachment: v1.1

Comment 4

[Asaf Romano (gone)]

(In reply to comment #2)

> >+    if (err == noErr) {
> >+      CFBundleRef handlerBundle = ::CFBundleCreate(NULL, handlerBundleURL);
> 
> Is this the right way to get the bundle given a URL? Seems a little weird.

AFAICT. As for using the bundle dictionary directly, this isn't really an option
-  we should take care of (per-loacle) InfoPlist.strings.

Comment 5

[jhp (no longer active)]

Comment on attachment 189802 [details] [diff] [review]
v1.1

+  nsCAutoString scheme(aScheme);
+  CFStringRef schemeCFString = ::CFStringCreateWithBytes(kCFAllocatorDefault,
+							  (const UInt8
*)scheme.get(),
+							  scheme.Length(),
+							 
kCFStringEncodingUTF8,
+							  false);

It might be better to just use |PromiseFlatString()|:
  CFStringRef schemeCFString =
	    ::CFStringCreateWithBytes(kCFAllocatorDefault,
				(const UInt8*)
PromiseFlatString(aScheme).get(),
				aScheme.Length(), kCFStringEncodingUTF8,
false);

+    // Since the public API (LSGetApplicationForURL) fails every now and then,
+    // we're using undocumented _LSCopyDefaultSchemeHandlerURL

Can you remind me again: in which circumstances did |LSGetApplicationForURL|
fail?

I take it his new patch fixes the leak described earlier?

Nit: Try to keep lines around 80 chars or less.

With those fixes, r=jhpedemonte.

Comment 6

[Simon Fraser [no longer active]]

Comment on attachment 189802 [details] [diff] [review]
v1.1

sr with the review comments addressed.

Comment 7

[Asaf Romano (gone)]

(In reply to comment #5)

> Can you remind me again: in which circumstances did |LSGetApplicationForURL|
> fail?

The only obvious one is the case where your app is the defualt handler (at
least, it happens more often in this case). Sure, that's not an option here, but
there are were other cases it failed for me (in the mac shell service).

Comment 8

[Asaf Romano (gone)]

Checking in mac/nsOSHelperAppService.cpp;
/cvsroot/mozilla/uriloader/exthandler/mac/nsOSHelperAppService.cpp,v  <-- 
nsOSHelperAppService.cpp
new revision: 1.44; previous revision: 1.43
done
Checking in mac/nsOSHelperAppService.h;
/cvsroot/mozilla/uriloader/exthandler/mac/nsOSHelperAppService.h,v  <-- 
nsOSHelperAppService.h
new revision: 1.15; previous revision: 1.14
done