Last Comment Bug 433254 - Implement Mac shell service for SeaMonkey.
: Implement Mac shell service for SeaMonkey.
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: OS Integration (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.15
Assigned To: Stefan [:stefanh] (away until December 6)
:
:
Mentors:
: 44658 127206 (view as bug list)
Depends on: 380347
Blocks: 791786
  Show dependency treegraph
 
Reported: 2008-05-11 11:33 PDT by Mark Banner (:standard8, afk until Dec)
Modified: 2012-10-11 02:28 PDT (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed


Attachments
WIP (24.21 KB, patch)
2008-05-11 11:33 PDT, Mark Banner (:standard8, afk until Dec)
no flags Details | Diff | Splinter Review
wip1 (16.37 KB, patch)
2012-08-15 17:44 PDT, Stefan [:stefanh] (away until December 6)
no flags Details | Diff | Splinter Review
WIP2 (16.27 KB, patch)
2012-08-16 08:01 PDT, Stefan [:stefanh] (away until December 6)
no flags Details | Diff | Splinter Review
WIP3 (15.25 KB, patch)
2012-08-16 19:23 PDT, Stefan [:stefanh] (away until December 6)
no flags Details | Diff | Splinter Review
WIP4 (17.03 KB, patch)
2012-08-17 07:54 PDT, Stefan [:stefanh] (away until December 6)
no flags Details | Diff | Splinter Review
WIP5 (16.64 KB, patch)
2012-08-17 10:28 PDT, Stefan [:stefanh] (away until December 6)
no flags Details | Diff | Splinter Review
WIP6 (16.63 KB, patch)
2012-08-18 14:44 PDT, Stefan [:stefanh] (away until December 6)
no flags Details | Diff | Splinter Review
Some cleanup (15.97 KB, patch)
2012-08-30 12:03 PDT, Stefan [:stefanh] (away until December 6)
no flags Details | Diff | Splinter Review
New version (16.13 KB, patch)
2012-09-04 12:01 PDT, Stefan [:stefanh] (away until December 6)
no flags Details | Diff | Splinter Review
Fixed nsSetDefault.js (17.70 KB, patch)
2012-09-04 13:33 PDT, Stefan [:stefanh] (away until December 6)
bugzilla: review+
Details | Diff | Splinter Review
Updated version (17.95 KB, patch)
2012-09-16 12:21 PDT, Stefan [:stefanh] (away until December 6)
mnyromyr: review+
Details | Diff | Splinter Review

Description Mark Banner (:standard8, afk until Dec) 2008-05-11 11:33:35 PDT
Created attachment 320452 [details] [diff] [review]
WIP

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).
Comment 1 neil@parkwaycc.co.uk 2008-05-11 15:46:51 PDT
(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.
Comment 2 Frank Wein [:mcsmurf] 2009-02-18 12:00:29 PST
Also see Bug 471346, where parts of the Mac shell service (mainly build stuff) will be implemented.
Comment 3 Stefan [:stefanh] (away until December 6) 2012-08-15 17:44:34 PDT
Created attachment 652287 [details] [diff] [review]
wip1

Attaching a WIP patch. I've been working on this for a couple of days. Lots of testing needs to be done, though.
Comment 4 Stefan [:stefanh] (away until December 6) 2012-08-15 17:45:08 PDT
Frank, might need some help/input from you :-)
Comment 5 neil@parkwaycc.co.uk 2012-08-16 06:09:41 PDT
Ideally we would use the new NSWorkspace setDesktopImageURL method.
Comment 6 Stefan [:stefanh] (away until December 6) 2012-08-16 08:01:01 PDT
Created attachment 652456 [details] [diff] [review]
WIP2

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
Comment 7 Stefan [:stefanh] (away until December 6) 2012-08-16 08:24:37 PDT
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.
Comment 8 Stefan [:stefanh] (away until December 6) 2012-08-16 08:29:17 PDT
(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.
Comment 9 neil@parkwaycc.co.uk 2012-08-16 09:18:11 PDT
Gecko 17 (comm/mozilla-central) has already dropped support for 10.5, no?
Comment 10 Stefan [:stefanh] (away until December 6) 2012-08-16 17:30:10 PDT
(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).
Comment 11 Stefan [:stefanh] (away until December 6) 2012-08-16 18:55:56 PDT
(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.
Comment 12 Stefan [:stefanh] (away until December 6) 2012-08-16 19:23:49 PDT
Created attachment 652653 [details] [diff] [review]
WIP3

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.
Comment 13 Stefan [:stefanh] (away until December 6) 2012-08-17 07:54:27 PDT
Created attachment 652766 [details] [diff] [review]
WIP4

Minor cleanup + hiding some elements in the setDesktopBackground dialog that Mac doesn't use.
Comment 14 Stefan [:stefanh] (away until December 6) 2012-08-17 10:28:46 PDT
Created attachment 652822 [details] [diff] [review]
WIP5

More cleanup after feedback from Neil.
Comment 15 Philip Chee 2012-08-18 04:34:30 PDT
> +#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;
Comment 16 Stefan [:stefanh] (away until December 6) 2012-08-18 06:54:40 PDT
(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.
Comment 17 Philip Chee 2012-08-18 08:39:46 PDT
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.
Comment 18 Philip Chee 2012-08-18 08:45:12 PDT
Ah it's a generated file
Comment 19 Stefan [:stefanh] (away until December 6) 2012-08-18 14:44:53 PDT
Created attachment 653106 [details] [diff] [review]
WIP6

Some cleanup per comment #15 and #16
Comment 20 Stefan [:stefanh] (away until December 6) 2012-08-30 12:03:55 PDT
Created attachment 656977 [details] [diff] [review]
Some cleanup

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 21 Frank Wein [:mcsmurf] 2012-09-04 03:27:35 PDT
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.
Comment 22 Frank Wein [:mcsmurf] 2012-09-04 03:36:46 PDT
Actually I just see Windows installer or OS do not use nsSetDefault.js either (the -setDefaultBrowser, -setDefaultMail and so on command line flags).
Comment 23 neil@parkwaycc.co.uk 2012-09-04 04:12:49 PDT
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?
Comment 24 Stefan [:stefanh] (away until December 6) 2012-09-04 12:01:13 PDT
Created attachment 658175 [details] [diff] [review]
New version

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.
Comment 25 Stefan [:stefanh] (away until December 6) 2012-09-04 13:33:03 PDT
Created attachment 658206 [details] [diff] [review]
Fixed nsSetDefault.js

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 26 Ian Neal 2012-09-05 15:49:46 PDT
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.
Comment 27 Stefan [:stefanh] (away until December 6) 2012-09-14 07:36:29 PDT
JFTR, I'll file a bug to make the set as bg image dialog look/behave better on mac.
Comment 28 Stefan [:stefanh] (away until December 6) 2012-09-15 00:29:21 PDT
Comment on attachment 658206 [details] [diff] [review]
Fixed nsSetDefault.js

Giving this to Karsten as well (I'll address comment #26 later)
Comment 29 Frank Wein [:mcsmurf] 2012-09-15 06:59:29 PDT
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 30 neil@parkwaycc.co.uk 2012-09-15 12:31:55 PDT
Comment on attachment 658206 [details] [diff] [review]
Fixed nsSetDefault.js

I'd still appreciate it if you addressed comment #23 ;-)
Comment 31 Stefan [:stefanh] (away until December 6) 2012-09-16 04:18:05 PDT
(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)?
Comment 32 Stefan [:stefanh] (away until December 6) 2012-09-16 11:01:57 PDT
(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'.
Comment 33 Stefan [:stefanh] (away until December 6) 2012-09-16 12:21:49 PDT
Created attachment 661625 [details] [diff] [review]
Updated version

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 34 Karsten Düsterloh 2012-10-03 08:25:14 PDT
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.
Comment 35 Stefan [:stefanh] (away until December 6) 2012-10-03 13:50:51 PDT
I'll land this on friday or saturday.
Comment 36 Stefan [:stefanh] (away until December 6) 2012-10-05 11:01:02 PDT
https://hg.mozilla.org/comm-central/rev/588792b14f43 (comment #34 fixed)
Comment 37 Stefan [:stefanh] (away until December 6) 2012-10-05 11:06:24 PDT
*** Bug 44658 has been marked as a duplicate of this bug. ***
Comment 38 Stefan [:stefanh] (away until December 6) 2012-10-05 11:10:07 PDT
*** Bug 127206 has been marked as a duplicate of this bug. ***
Comment 39 Stefan [:stefanh] (away until December 6) 2012-10-05 16:13:44 PDT
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).
Comment 40 Stefan [:stefanh] (away until December 6) 2012-10-05 16:22:33 PDT
> didn't catch bug 794602 (fixed 2 days ago).

Erm, 1 day ago...
Comment 41 Frank Wein [:mcsmurf] 2012-10-11 02:28:09 PDT
Settings flags to make tracking of fixed bugs easier.

Note You need to log in before you can comment on or make changes to this bug.