Now that there is a Windows shell service, implementing a Mac one shouldn't be too hard. Indeed, I copied some FF code (probably bad move... but I did tidy it up a bit) and got enough to enable -setDefaultBrowser to work to some extent in about an hour or two this weekend. The patch doesn't really touch Mail or News, it just returns NS_ERROR_NOT_IMPLEMENTED. I also haven't touched or tested the setDefaultBackground code. If someone wants to pick this up, please do as I don't know when I'll have to time work on it again (it does what I needed it to).

(In reply to comment #0) > The patch doesn't really touch Mail or News, it just returns NS_ERROR_NOT_IMPLEMENTED. We still need to figure out what to do on --disable-mailnews builds anyway.

Also see Bug 471346, where parts of the Mac shell service (mainly build stuff) will be implemented.

Attaching a WIP patch. I've been working on this for a couple of days. Lots of testing needs to be done, though.

Frank, might need some help/input from you :-)

Ideally we would use the new NSWorkspace setDesktopImageURL method.

OK, so, generally speaking, this seems to work. There are a few things that needs to be fixed (looking at it), like: 1. We only set the http scheme, not https. 2. We don't handle file types

Ah, right - another thing is that I should probably use the public Launch Services API:s. I now see that Firefox switched to those in 2009.

(In reply to neil@parkwaycc.co.uk from comment #5) > Ideally we would use the new NSWorkspace setDesktopImageURL method. It seems it's only available on 10.6 and higher.

Gecko 17 (comm/mozilla-central) has already dropped support for 10.5, no?

(In reply to neil@parkwaycc.co.uk from comment #9) > Gecko 17 (comm/mozilla-central) has already dropped support for 10.5, no? Ah, you're right (found bug 772682).

(In reply to Stefan [:stefanh] from comment #10) > (In reply to neil@parkwaycc.co.uk from comment #9) > > Gecko 17 (comm/mozilla-central) has already dropped support for 10.5, no? > > Ah, you're right (found bug 772682). That said, I think that's material for another bug.

I had some problems using the public Launch Services API:s. "isDefault = (::CFStringCompare(suiteID, defaultHandlerID, 0) == kCFCompareEqualTo;" always seemed to set isDefault to false. The reason was that the bundleidentifier for some reason contained "SeaMonkey" while the LauchServices file contained "seamonkey". So now I use the kCFCompareCaseInsensitive option.

Minor cleanup + hiding some elements in the setDesktopBackground dialog that Mac doesn't use.

More cleanup after feedback from Neil.

> +#include "nsILocalFile.h" nsILocalFile.h doesn't exist any more. nsILocalFile is an empty interface that just forwards to nsIFile. ditto for: > +#include "nsILocalFileMac.h" > + nsCOMPtr<nsILocalFile> mBackgroundFile;

(In reply to Philip Chee from comment #15) > > +#include "nsILocalFile.h" > nsILocalFile.h doesn't exist any more. nsILocalFile is an empty interface > that just forwards to nsIFile. > ditto for: > > +#include "nsILocalFileMac.h" > > + nsCOMPtr<nsILocalFile> mBackgroundFile; Ah, thanks. nsILocalFileMac is not empty, though.

Interesting. Why can't I find it in MXR? http://mxr.mozilla.org/mozilla-central/find?text=&string=nsILocalFileMac.h There is a nsLocalFile.h though.

Ah it's a generated file

Some cleanup per comment #15 and #16

Frank, I'm aware that you don't have a mac, but since you know the code I'd be really happy if you could look at this. I'll let Karsten look at it as well when you're done.

Comment on attachment 656977 [details] [diff] [review] Some cleanup I've commented on a few numeric datatypes, but I now see most of this has already been fixed in the repository. So you only need to fix your own patch. >diff --git a/suite/shell/src/Makefile.in b/suite/shell/src/Makefile.in >--- a/suite/shell/src/Makefile.in >+++ b/suite/shell/src/Makefile.in >@@ -16,24 +16,25 @@ FORCE_STATIC_LIB=1 > ifeq ($(OS_ARCH),WINNT) > CPPSRCS = nsWindowsShellService.cpp > OS_LIBS += $(call EXPAND_LIBNAME,ole32 version uuid shell32) > > EXTRA_COMPONENTS = nsSetDefault.manifest nsSetDefault.js > else > ifeq ($(MOZ_WIDGET_TOOLKIT), cocoa) > CPPSRCS = nsMacShellService.cpp >+ >+EXTRA_COMPONENTS = nsSetDefault.manifest nsSetDefault.js Is this really required? Ok, then we can also set SeaMonkey as default via commandline. On Windows this was required so that the installer and the OS can set SeaMonkey as default. >diff --git a/suite/shell/src/nsMacShellService.cpp b/suite/shell/src/nsMacShellService.cpp >--- a/suite/shell/src/nsMacShellService.cpp >+++ b/suite/shell/src/nsMacShellService.cpp > NS_IMETHODIMP > nsMacShellService::SetDefaultClient(bool aForAllUsers, > bool aClaimAllTypes, PRUint16 aApps) > { NSPR numeric types have been retired, see Bug 579517. Use uint16_t here instead. >- return NS_ERROR_NOT_IMPLEMENTED; >+ CFStringRef suiteID = ::CFBundleGetIdentifier(::CFBundleGetMainBundle()); >+ if (!suiteID) >+ return NS_ERROR_FAILURE; >+ >+ if (aApps & nsIShellService::BROWSER) >+ { >+ if (::LSSetDefaultHandlerForURLScheme(CFSTR("http"), suiteID) != noErr) >+ return NS_ERROR_FAILURE; >+ if (::LSSetDefaultHandlerForURLScheme(CFSTR("https"), suiteID) != noErr) >+ return NS_ERROR_FAILURE; >+ if (::LSSetDefaultRoleHandlerForContentType(kUTTypeHTML, kLSRolesAll, suiteID) != noErr) >+ return NS_ERROR_FAILURE; >+ if (::LSSetDefaultRoleHandlerForContentType(CFSTR("public.xhtml"), kLSRolesAll, suiteID) != noErr) >+ return NS_ERROR_FAILURE; What is the line with public.xhtml for? Does it register the application/xhtml+xml content type with the OS? Should we also register text/html then? Also add a comment that aForAllUsers is not supported on OS X. > NS_IMETHODIMP > nsMacShellService::GetShouldCheckDefaultClient(bool* aResult) > { >- return NS_ERROR_NOT_IMPLEMENTED; >+ if (mCheckedThisSessionClient) >+ { >+ *aResult = false; >+ return NS_OK; >+ } >+ >+ nsCOMPtr<nsIPrefBranch> prefs(do_GetService(NS_PREFSERVICE_CONTRACTID)); >+ return prefs->GetBoolPref(PREF_CHECKDEFAULTCLIENT, aResult); > } I think you need either to check for success of that do_GetService: nsCOMPtr<nsIPrefBranch> prefs(do_GetService(NS_PREFSERVICE_CONTRACTID, &rv)); NS_ENSURE_SUCCESS(rv, rv); or check with "if (pref)" if that object is non-null. > NS_IMETHODIMP > nsMacShellService::SetShouldCheckDefaultClient(bool aShouldCheck) > { >- return NS_ERROR_NOT_IMPLEMENTED; >+ nsCOMPtr<nsIPrefBranch> prefs(do_GetService(NS_PREFSERVICE_CONTRACTID)); >+ return prefs->SetBoolPref(PREF_CHECKDEFAULTCLIENT, aShouldCheck); >+ Same as above. > NS_IMETHODIMP > nsMacShellService::GetShouldBeDefaultClientFor(PRUint16* aApps) > { NSPR numeric types have been retired, see Bug 579517. Use uint16_t here instead. >- return NS_ERROR_NOT_IMPLEMENTED; >+ nsresult rv; >+ nsCOMPtr<nsIPrefBranch> prefs(do_GetService(NS_PREFSERVICE_CONTRACTID, &rv)); >+ NS_ENSURE_SUCCESS(rv, rv); >+ PRInt32 result; >+ rv = prefs->GetIntPref("shell.checkDefaultApps", &result); >+ *aApps = result; >+ return rv; > } NSPR numeric types have been retired, see Bug 579517. Use int32_t here instead. > NS_IMETHODIMP > nsMacShellService::SetShouldBeDefaultClientFor(PRUint16 aApps) > { See above. > NS_IMETHODIMP > nsMacShellService::SetDesktopBackground(nsIDOMElement* aElement, > PRInt32 aPosition) > { See above. >+NS_IMETHODIMP >+nsMacShellService::OnProgressChange(nsIWebProgress* aWebProgress, >+ nsIRequest* aRequest, >+ PRInt32 aCurSelfProgress, >+ PRInt32 aMaxSelfProgress, >+ PRInt32 aCurTotalProgress, >+ PRInt32 aMaxTotalProgress) >+{ >+ return NS_OK; >+} See above. >+NS_IMETHODIMP >+nsMacShellService::OnLocationChange(nsIWebProgress* aWebProgress, >+ nsIRequest* aRequest, >+ nsIURI* aLocation, >+ PRUint32 aFlags) >+{ >+ return NS_OK; >+} See above. >+NS_IMETHODIMP >+nsMacShellService::OnSecurityChange(nsIWebProgress* aWebProgress, >+ nsIRequest* aRequest, >+ PRUint32 aState) >+{ >+ return NS_OK; >+} See above. >+NS_IMETHODIMP >+nsMacShellService::OnStateChange(nsIWebProgress* aWebProgress, >+ nsIRequest* aRequest, >+ PRUint32 aStateFlags, >+ nsresult aStatus) >+{ See above. I've noticed the Firefox Mac shell service code sends out a observer notification in this function as this code http://mxr.mozilla.org/comm-central/source/mozilla/browser/components/shell/content/setDesktopBackground.js#141 listens to this notification. Looks like the menu item for setting the desktop background image gets disabled on OS X as it's downloading the image again before setting it as background image. Is that correct? Maybe we should a simpler solution like disabling it, too, but not introducing an extra label for it.

Actually I just see Windows installer or OS do not use nsSetDefault.js either (the -setDefaultBrowser, -setDefaultMail and so on command line flags).

Comment on attachment 656977 [details] [diff] [review] Some cleanup > ifeq ($(OS_ARCH),WINNT) > CPPSRCS = nsWindowsShellService.cpp > OS_LIBS += $(call EXPAND_LIBNAME,ole32 version uuid shell32) > > EXTRA_COMPONENTS = nsSetDefault.manifest nsSetDefault.js > else > ifeq ($(MOZ_WIDGET_TOOLKIT), cocoa) > CPPSRCS = nsMacShellService.cpp >+ >+EXTRA_COMPONENTS = nsSetDefault.manifest nsSetDefault.js > else > ifeq ($(MOZ_WIDGET_TOOLKIT), gtk2) > CPPSRCS = nsGNOMEShellService.cpp > endif > endif > endif Ah, I didn't think to change this for the GNOME shell service. Does it make sense to take the components out of the condition altogether?

Fixed most of the comments, see below (In reply to Frank Wein [:mcsmurf] from comment #21) > Comment on attachment 656977 [details] [diff] [review] > Some cleanup > > I've commented on a few numeric datatypes, but I now see most of this has > already been fixed in the repository. So you only need to fix your own patch. Ah, right - seems that the patch bitrottened a bit. I also got hit by the nsCAutoString->nsAutoCString changes in bug 773151. Fixed all datatypes. > > > >diff --git a/suite/shell/src/Makefile.in b/suite/shell/src/Makefile.in > >--- a/suite/shell/src/Makefile.in > >+++ b/suite/shell/src/Makefile.in > >@@ -16,24 +16,25 @@ FORCE_STATIC_LIB=1 > > ifeq ($(OS_ARCH),WINNT) > > CPPSRCS = nsWindowsShellService.cpp > > OS_LIBS += $(call EXPAND_LIBNAME,ole32 version uuid shell32) > > > > EXTRA_COMPONENTS = nsSetDefault.manifest nsSetDefault.js > > else > > ifeq ($(MOZ_WIDGET_TOOLKIT), cocoa) > > CPPSRCS = nsMacShellService.cpp > >+ > >+EXTRA_COMPONENTS = nsSetDefault.manifest nsSetDefault.js > > Is this really required? Ok, then we can also set SeaMonkey as default via > commandline. On Windows this was required so that the installer and the OS > can set SeaMonkey as default. Hmm, it doesn't seem to work, I have to investigate a bit... (also thinking of Neils comment) > >diff --git a/suite/shell/src/nsMacShellService.cpp b/suite/shell/src/nsMacShellService.cpp > >--- a/suite/shell/src/nsMacShellService.cpp > >+++ b/suite/shell/src/nsMacShellService.cpp > > > >- return NS_ERROR_NOT_IMPLEMENTED; > >+ CFStringRef suiteID = ::CFBundleGetIdentifier(::CFBundleGetMainBundle()); > >+ if (!suiteID) > >+ return NS_ERROR_FAILURE; > >+ > >+ if (aApps & nsIShellService::BROWSER) > >+ { > >+ if (::LSSetDefaultHandlerForURLScheme(CFSTR("http"), suiteID) != noErr) > >+ return NS_ERROR_FAILURE; > >+ if (::LSSetDefaultHandlerForURLScheme(CFSTR("https"), suiteID) != noErr) > >+ return NS_ERROR_FAILURE; > >+ if (::LSSetDefaultRoleHandlerForContentType(kUTTypeHTML, kLSRolesAll, suiteID) != noErr) > >+ return NS_ERROR_FAILURE; > >+ if (::LSSetDefaultRoleHandlerForContentType(CFSTR("public.xhtml"), kLSRolesAll, suiteID) != noErr) > >+ return NS_ERROR_FAILURE; > > What is the line with public.xhtml for? Does it register the > application/xhtml+xml content type with the OS? Should we also register > text/html then? I've been testing this by looking at what gets written to the com.apple.LaunchServices.plist file and comparing with what happens when you use the default browser prefs that comes with Safari (you can set SM as the default browser there as well). Using the type identifier kUTTypeHTML sets LSHandlerContentType to "public.html", but there doesn't seem to be a constant for xhtml, so I have to set it explicitly. > Also add a comment that aForAllUsers is not supported on OS > X. Fixed. Hmm, I now see that I don't really care about aClaimAllTypes... It made sense to add html/xhtml as that's what happens when using Safari to set default browser. But maybe I'm wrong here? > > > > NS_IMETHODIMP > > nsMacShellService::GetShouldCheckDefaultClient(bool* aResult) > > { > >- return NS_ERROR_NOT_IMPLEMENTED; > >+ if (mCheckedThisSessionClient) > >+ { > >+ *aResult = false; > >+ return NS_OK; > >+ } > >+ > >+ nsCOMPtr<nsIPrefBranch> prefs(do_GetService(NS_PREFSERVICE_CONTRACTID)); > >+ return prefs->GetBoolPref(PREF_CHECKDEFAULTCLIENT, aResult); > > } > > I think you need either to check for success of that do_GetService: > nsCOMPtr<nsIPrefBranch> prefs(do_GetService(NS_PREFSERVICE_CONTRACTID, &rv)); > NS_ENSURE_SUCCESS(rv, rv); > > or check with "if (pref)" if that object is non-null. Fixed > > NS_IMETHODIMP > > nsMacShellService::SetShouldCheckDefaultClient(bool aShouldCheck) > > { > >- return NS_ERROR_NOT_IMPLEMENTED; > >+ nsCOMPtr<nsIPrefBranch> prefs(do_GetService(NS_PREFSERVICE_CONTRACTID)); > >+ return prefs->SetBoolPref(PREF_CHECKDEFAULTCLIENT, aShouldCheck); > >+ > > Same as above. Fixed > I've noticed the Firefox Mac shell service code sends out a observer > notification in this function as this code > http://mxr.mozilla.org/comm-central/source/mozilla/browser/components/shell/ > content/setDesktopBackground.js#141 listens to this notification. Looks like > the menu item for setting the desktop background image gets disabled on OS X > as it's downloading the image again before setting it as background image. > Is that correct? The notification is used to hide the "Set Desktop Background" button and make the "Open Desktop Preferences" button visible, see http://mxr.mozilla.org/mozilla-central/source/browser/components/shell/content/setDesktopBackground.js#118 :-) The "Set Desktop Background" button always gets disabled and the label also changes to "Saving Picture...", but since I have a quite fast connection I've never seen it - as soon as I've clicked the button I get the "Open Desktop Preferences" button. > Maybe we should a simpler solution like disabling it, too, > but not introducing an extra label for it. I wouldn't mind making us use the observer and also making it possible to get to the Mac OS X desktop prefs from the dialog. But I'd like to do that in another bug :-) (In reply to Frank Wein [:mcsmurf] from comment #22) > Actually I just see Windows installer or OS do not use nsSetDefault.js > either (the -setDefaultBrowser, -setDefaultMail and so on command line > flags). Hm, ok.

Turned out that i missed to include the file in package-manifest.in (also changed removed-files.in). I tested setting default browser and mail from the command line and now it works :-)

Comment on attachment 658206 [details] [diff] [review] Fixed nsSetDefault.js >+++ b/suite/shell/src/nsMacShellService.cpp >+ // if this is the first window, maintain internal state that we've >+ // checked this session (so that subsequent window opens don't show the >+ // default client dialog. Nit: you're missing a closing ) in the comment.

JFTR, I'll file a bug to make the set as bg image dialog look/behave better on mac.

Comment on attachment 658206 [details] [diff] [review] Fixed nsSetDefault.js Giving this to Karsten as well (I'll address comment #26 later)

Comment on attachment 658206 [details] [diff] [review] Fixed nsSetDefault.js Review of attachment 658206 [details] [diff] [review]: ----------------------------------------------------------------- New patch looks fine. ::: suite/shell/src/nsMacShellService.cpp @@ +52,5 @@ > > NS_IMETHODIMP > nsMacShellService::SetDefaultClient(bool aForAllUsers, > bool aClaimAllTypes, uint16_t aApps) > { On aClaimAllTypes: It looks like this param was mostly added for the Mac and the Linux shell service (at least Firefox only uses it there). I'm not sure why, maybe UI guidelines. FF only registers the ftp:// protocol and kUTTypeHTML when aClaimAllTypes is true. But (so far) aClaimAllTypes is always set to true in our code. I would need to look up what the idea behind introducing aClaimAllTypes was.

Comment on attachment 658206 [details] [diff] [review] Fixed nsSetDefault.js I'd still appreciate it if you addressed comment #23 ;-)

(In reply to neil@parkwaycc.co.uk from comment #30) > Comment on attachment 658206 [details] [diff] [review] > Fixed nsSetDefault.js > > I'd still appreciate it if you addressed comment #23 ;-) Sure. How about just putting the components at the end (for all OS)?

(In reply to Stefan [:stefanh] from comment #31) > (In reply to neil@parkwaycc.co.uk from comment #30) > > Comment on attachment 658206 [details] [diff] [review] > > Fixed nsSetDefault.js > > > > I'd still appreciate it if you addressed comment #23 ;-) > > Sure. How about just putting the components at the end (for all OS)? Probably better to put the components inside the 'ifdef CPPSRCS'.

This version: - Fixed comment #26 (also changed to upper-case 'I') - Fixed some other comments in the code (added "." and ":") - Addressed comment #30 (comment #32 solution) - Removed windows ifdefs in removed-files/package-manifest.in. - Removed the <CoreFoundation/CoreFoundation.h> include in nsMacShellService.cpp since it's already included in the header file.

Comment on attachment 661625 [details] [diff] [review] Updated version Looks good, just some nits: > nsMacShellService::IsDefaultClient(bool aStartupCheck, uint16_t aApps, bool *aIsDefaultClient) > { >- return NS_ERROR_NOT_IMPLEMENTED; >+ *aIsDefaultClient = true; >+ if (aApps & nsIShellService::BROWSER) >+ *aIsDefaultClient &= isDefaultHandlerForProtocol(CFSTR("http")); >+ if (aApps & nsIShellService::MAIL) >+ *aIsDefaultClient &= isDefaultHandlerForProtocol(CFSTR("mailto")); >+ if (aApps & nsIShellService::NEWS) >+ *aIsDefaultClient &= isDefaultHandlerForProtocol(CFSTR("news")); >+ if (aApps & nsIShellService::RSS) >+ *aIsDefaultClient &= isDefaultHandlerForProtocol(CFSTR("feed")); >+ >+ // If this is the first window, maintain internal state that we've >+ // checked this session (so that subsequent window opens don't show the >+ // default client dialog). >+ if (aStartupCheck) >+ mCheckedThisSessionClient = true; >+ return NS_OK; > } There's no need to check for mailto: etc. if http already failed, you could reorder the code and exit early, just like you do in SetDefault. >+ if (aApps & nsIShellService::BROWSER) >+ { … >+ if (!suiteID) { Your bracing style is inconsistent. >+ // The handler ID in LaunchServices is in all lower case, but the bundle >+ // identifier could have upper case characters. So we're using >+ // CFStringCompare with the kCFCompareCaseInsensitive option (1) here. >+ isDefault = ::CFStringCompare(suiteID, defaultHandlerID, 1) == kCFCompareEqualTo; If you know which constant it is, why don't you just use it?! > nsMacShellService::SetDesktopBackground(nsIDOMElement* aElement, > int32_t aPosition) … >+ nsCOMPtr<nsIImageLoadingContent> imageContent = do_QueryInterface(aElement, >+ &rv); No need for extra fancy wrapping in these two cases. ;-) r=me with that.

I'll land this on friday or saturday.

https://hg.mozilla.org/comm-central/rev/588792b14f43 (comment #34 fixed)

JFTR (thanks to Neil who pinged me about it), I landed http://hg.mozilla.org/comm-central/rev/ff10a63c4410 as a bustage fix - didn't catch bug 794602 (fixed 2 days ago).

> didn't catch bug 794602 (fixed 2 days ago). Erm, 1 day ago...

Settings flags to make tracking of fixed bugs easier.