274374 - Mac OS X Shell Service ("Set default browser" support, etc.)

Status: VERIFIED FIXED

(Firefox :: Shell Integration, defect, P2)

Description

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

For default browser, etc.

Comment 1

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

Attachment: work in progress

Comment 2

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

Attachment: more or less final version

the "Set as Desktop background" dialog needs a little more love for OS X, and
I'm not quite convinced of its non-modality, but in that case users would need
a way of opening the Desktop preferences before choosing "Set as Desktop
Background"...

Comment 3

[Asaf Romano (gone)]

from
http://developer.apple.com/documentation/Carbon/Reference/Internet_Config/internet_config_ref/Introduction.html:

"Mac OS X applications should employ Launch Services and System Configuration
for managing Internet preferences. In Mac OS X, Internet Config calls through to
these newer APIs. Using them directly increases your application’s efficiency."

Comment 4

[Asaf Romano (gone)]

Attachment: patch

Comment 5

[Asaf Romano (gone)]

So the last patch is an updated version of ben's patch. Chnages:
 * Update to tip.
 * Use Launch Services in IsDefaultBrowser, works.
 * Use Launch Services in SetDefaultBrowser, doesn't-work-yet try... Need to
sort out why it has no affect (FYI, it does work in Camino).
   (It might be that i succeeded to break my LS plist.)
 * Fix leaks in OpenApplication()
 * Working menus in the setDesktopBackground dialog.

Comment 6

[jhp (no longer active)]

Warnings on these two lines:

  CFStringRef cfsFirefoxURL = ::CFRetain(::CFURLGetString(firefoxURL));
  CFStringRef cfsDefaultBrowserURL =
::CFRetain(::CFURLGetString(defaultBrowserURL));

Should just get the URL on one line, and retain on another line.

Get this error on startup (also when pressing "Check Now" in Prefs->General),
but only when Firefox is already set as default web browser:

JavaScript error: , line 0: uncaught exception: [Exception... "Component
returned failure code: 0x80004005 (NS_ERROR_FAILURE)
[nsIShellService.isDefaultBrowser]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)" 
location: "JS frame :: chrome://browser/content/browser.js :: delayedStartup ::
line 758"  data: no]

I got |SetDefaultBrowser| to work by removing the colon from the strings
("http:" -> "http").

Comment 7

[Asaf Romano (gone)]

Attachment: patch

Asking Javier for mac-parts review and Mike for XP stuff.

Changes:
 * Get SetDefaultBrowser working (see comment 6, thanks Javier!)
 * Support aClaimAllTypes in SetDefaultBrowser (ported from Camino).
 * don't remove the shell service check in browser.js

The modal dialog thingy is hard, and I think it would be better to keep it
non-modal on mac (see comment 2), for now.

Comment 8

[Asaf Romano (gone)]

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

some erros in browser.js -> obsolete.

Comment 9

[Asaf Romano (gone)]

Attachment: patch

Comment 10

[jhp (no longer active)]

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

Please try to keep the lines under 80 chars wide.

Why is the "DEFINES += -DHAVE_SHELL_SERVICE=1" code in two places?  Would be
nice to consolidate it in one file.

+#pragma unused (pIndex)
What's this for?

+nsMacShellService::OnStateChange(nsIWebProgress* aWebProgress,
Add some comments to this file to explain what the native code is doing?

+      nsCOMPtr<nsILocalFile>
lf(do_CreateInstance("@mozilla.org/file/local;1"));
+      lf->InitWithNativePath(DESKTOP_PREFPANE);
You can replace these with a call to |NS_NewNativeLocalFile()|.

+nsMacShellService::GetDesktopBackgroundColor(PRUint32 *aColor)
Will you get to this later, or worry about it in a different bug?

In |SetDesktopBackground()|, the param |aPosition| is saved to
|mBackgroundPosition|, but it is never actually used.

+// these functions are undocumented - even if we're not using them all
+// its best to leave them here since they are not documented anywhere else
Please add some comments about which ones need |CFRetain| and |CFRelease|.

Comment 11

[Asaf Romano (gone)]

(In reply to comment #10)

> (From update of attachment 186195 [details] [diff] [review] [edit])
> Please try to keep the lines under 80 chars wide.

done.

> Why is the "DEFINES += -DHAVE_SHELL_SERVICE=1" code in two places?  Would be
> nice to consolidate it in one file.

Well, they're built separatley, so i'm not sure we can fix it easily.

> +#pragma unused (pIndex)
> What's this for?

This was left from Ben's patch, but as pIndex isn't declared even, I've removed it.

> +nsMacShellService::OnStateChange(nsIWebProgress* aWebProgress,
> Add some comments to this file to explain what the native code is doing?

Done, there's still a room for improvement, I think.
 
> +      nsCOMPtr<nsILocalFile>
> lf(do_CreateInstance("@mozilla.org/file/local;1"));
> +      lf->InitWithNativePath(DESKTOP_PREFPANE);
> You can replace these with a call to |NS_NewNativeLocalFile()|.
> 
> +nsMacShellService::GetDesktopBackgroundColor(PRUint32 *aColor)
> Will you get to this later, or worry about it in a different bug?

Well, this has no real meaning on OS X. Apple is using pictures for the few
colors the desktop prefs ui does support (see /Library/Desktop Pictures/Solid
Colors). I've changed the comments to clarify that.

> In |SetDesktopBackground()|, the param |aPosition| is saved to
> |mBackgroundPosition|, but it is never actually used.

I've removed the private class memeber altogether and added a comment in
|SetDesktopBackground| to clarify that it's not supported.

> +// these functions are undocumented - even if we're not using them all
> +// its best to leave them here since they are not documented anywhere else
> Please add some comments about which ones need |CFRetain| and |CFRelease|.
 
This was only relevant for |_LSCopyDefaultSchemeHandlerURL|, wasn't it? Anyway,
I've extended the pseudo private API documentation, a little bit.

Comment 12

[Asaf Romano (gone)]

Attachment: patch

Comment 13

[timeless]

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

>Index: browser/components/shell/src/nsMacShellService.cpp
>@@ -9,53 +9,138 @@
>- * The Initial Developer of the Original Code is mozilla.org.
>- * Portions created by the Initial Developer are Copyright (C) 2004
>+ * The Initial Developer of the Original Code is Ben Goodger.
>+ * Portions created by the Initial Developer are Copyright (C) 2005
>  * the Initial Developer. All Rights Reserved.
This is almost certainly wrong.

>@@ -86,28 +171,234 @@ nsMacShellService::SetShouldCheckDefault
> nsMacShellService::SetDesktopBackground(nsIDOMElement* aElement, 
>+  nsCOMPtr<nsIDOMHTMLImageElement> image(do_QueryInterface(aElement));
>+  if (!image)
>+    return NS_ERROR_FAILURE;
you can't provide a more specific error like invalid_arg?

>+  nsCOMPtr<nsIURI> imageURI, docURI;
>+  NS_NewURI(getter_AddRefs(imageURI), src);
>+  NS_NewURI(getter_AddRefs(docURI), baseURI);
you didn't check docURI for failure, is this really ok?

>+  if (!imageURI)
>+    return NS_ERROR_OUT_OF_MEMORY;

>+  nsCOMPtr<nsIProperties> fileLocator(do_GetService("@mozilla.org/file/directory_service;1"));
you didn't check for failure, you crashed:
>+  fileLocator->Get(NS_OSX_PICTURE_DOCUMENTS_DIR, NS_GET_IID(nsILocalFile),
>+                   getter_AddRefs(mBackgroundFile));
you didn't check for failure

use CopyUTF8toUTF16 instead.
>+  nsAutoString fileNameUnicode = NS_ConvertUTF8toUCS2(fileName);
you crashed:
>+  mBackgroundFile->Append(fileNameUnicode);

>+  return wbp->SaveURI(imageURI, nsnull, docURI, nsnull, nsnull, mBackgroundFile);

>+    mBackgroundFile->Exists(&exists);
technically you're expected to check rv from Exists.
>+    if (!exists)
>+      return NS_OK;

this line is really long :(
>+                                  "'----':'obj '{want:type(prop),form:prop,seld:type('dpic'),from:'null'()},data:(@)",

>Index: browser/components/shell/src/nsWindowsShellService.cpp
>+  nsCOMPtr<nsIStringBundle> brandBundle, shellBundle;
>+  bundleService->CreateBundle("chrome://global/locale/brand.properties",
>+                              getter_AddRefs(brandBundle));
you didn't check for failure
>+  bundleService->CreateBundle("chrome://browser/locale/shellservice.properties",
>+                              getter_AddRefs(brandBundle));
you didn't check for failure

>+  nsXPIDLString brandShortName;
you crash:
>+  brandBundle->GetStringFromName(NS_LITERAL_STRING("brandShortName").get(),
>+                                 getter_Copies(brandShortName));
>+  const PRUnichar* brandNameStrings[] = { brandShortName.get() };
>+  nsXPIDLString backgroundFileName;
you crash:
>+  bundle->FormatStringFromName(NS_LITERAL_STRING("desktopBackgroundFileNameWin").get(),
>+                               brandNameStrings, 1, getter_Copies(backgroundFileName));
>+  winPath.Append(backgroundFileName);

>Index: browser/locales/en-US/chrome/browser/shellservice.properties
>-alreadyDefaultBrowser=%S is already set as your default browser.
>\ No newline at end of file
...
>+

one too many newlines at end of file

Comment 14

[Asaf Romano (gone)]

(In reply to comment #13)

fixed all of those but:

> technically you're expected to check rv from Exists.
> >+    if (!exists)
> >+      return NS_OK;

|Exists()| will always return NS_OK from the points it's used here.

Comment 15

[Asaf Romano (gone)]

Attachment: patch

Comment 16

[Asaf Romano (gone)]

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

there are some errors in the windows shell service changes

Comment 17

[Asaf Romano (gone)]

Attachment: patch

Thanks for testing, Gavin.

(I should also menton that i've not changed that:

this line is really long :(
>+				    "'----':'obj
'{want:type(prop),form:prop,seld:type('dpic'),from:'null'()},data:(@)",
 
as i'm not sure it's valid to crop it.

Comment 18

[jhp (no longer active)]

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

There are still several lines that exceed 80 chars.

+  CFURLRef firefoxURL = ::CFBundleCopyBundleURL(CFBundleGetMainBundle());
You need to release this variable, in |IsDefaultBrowser()| and
|SetDefaultBrowser()|.

+  // Get the defualt http handler
"default" misspelled.

Please add some more comments to the code in |SetDesktopBackground()|.

With those nits fixed, r=jhpedemonte for Mac specific parts (non-js/xul).

Comment 19

[Asaf Romano (gone)]

Attachment: patch

Mike, can you review the rest of this patch (js/xul + makefiles).

Comment 20

[Asaf Romano (gone)]

Attachment: [checked in] patch

Rewrite |IsDefaultBrowser()| to use bundle identifiers. Javier, could you
re-review that part?

Comment 21

[jhp (no longer active)]

Comment on attachment 186667 [details] [diff] [review]
[checked in] patch

+  // Since nither Launch Services nor Internet Config actually differ between 
s/nither/neither

+  // bundles which have the same bundle identifier (That's, if we set our
s/That's/That is

You are leaking some objects due to early returns in |IsDefaultBrowser()|. 
Might I suggest:

------	::CFRetain(firefoxID);

  // Get the default http handler URL
  CFURLRef defaultBrowserURL;
  OSStatus err = ::_LSCopyDefaultSchemeHandlerURL(CFSTR("http"),
						  &defaultBrowserURL);

  nsresult rv = NS_ERROR_FAILURE;
  if (err == noErr) {
    // Get a reference to the bundle (based on its URL)
    CFBundleRef defaultBrowserBundle = ::CFBundleCreate(NULL,
defaultBrowserURL);
    if (defaultBrowserBundle) {
      // Get its identifier
      CFStringRef defaultBrowserID =
			 ::CFBundleGetIdentifier(defaultBrowserBundle);
      if (defaultBrowserID) {
	// and compare it to our bundle identifier
	::CFRetain(defaultBrowserID);
	*aIsDefaultBrowser = ::CFStringCompare(firefoxID, defaultBrowserID, 0)
			     == kCFCompareEqualTo;
	::CFRelease(defaultBrowserID);
      } else {
	// If the default browser bundle doesn't have an identifier in its
	// plist, it's not our bundle
	*aIsDefaultBrowser = PR_FALSE;
      }

      ::CFRelease(defaultBrowserBundle);
      rv = NS_OK;
    }

    ::CFRelease(defaultBrowserURL);
  }

  ::CFRelease(firefoxID);

  // If this is the first browser window, maintain internal state that we've
  // checked this session (so that subsequent window opens don't show the 
  // default browser dialog).
  if (aStartupCheck)
    mCheckedThisSession = PR_TRUE;

  return rv;
-------

With that, r=jhpedemonte.

Comment 22

[Asaf Romano (gone)]

Comment on attachment 186667 [details] [diff] [review]
[checked in] patch

sort of sr=mconnor/josh on irc. a=shaver. will land later today.

Comment 23

[Asaf Romano (gone)]

checked in.

Comment 24

[Tracy Walker [:tracy]]

verified with DP trunk build 2005-07-12-07-trunk

though a clean system must exist to have it work (see invalidated bug 299833 )