Closed Bug 274374 Opened 17 years ago Closed 17 years ago

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

Categories

(Firefox :: Shell Integration, defect, P2)

PowerPC
macOS
defect

Tracking

()

VERIFIED FIXED
Firefox1.5

People

(Reporter: bugs, Assigned: mano)

References

Details

Attachments

(1 file, 9 obsolete files)

76.63 KB, patch
jhpedemonte
: review+
Details | Diff | Splinter Review
For default browser, etc.
Priority: -- → P2
Target Milestone: --- → Firefox1.1
Attached patch more or less final version (obsolete) — Splinter Review
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"...
Attachment #168589 - Attachment is obsolete: true
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."
Attached patch patch (obsolete) — Splinter Review
Attachment #168602 - Attachment is obsolete: true
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.
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").
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #186083 - Attachment is obsolete: true
Attachment #186187 - Flags: superreview?(mconnor)
Attachment #186187 - Flags: review?(jhpedemonte)
Comment on attachment 186187 [details] [diff] [review]
patch

some erros in browser.js -> obsolete.
Attachment #186187 - Attachment is obsolete: true
Attachment #186187 - Flags: superreview?(mconnor)
Attachment #186187 - Flags: review?(jhpedemonte)
Attached patch patch (obsolete) — Splinter Review
Attachment #186195 - Flags: superreview?(mconnor)
Attachment #186195 - Flags: review?(jhpedemonte)
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|.
(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.
Attachment #186195 - Attachment is obsolete: true
Attachment #186195 - Flags: superreview?(mconnor)
Attachment #186195 - Flags: review?(jhpedemonte)
Attachment #186195 - Flags: review-
Attached patch patch (obsolete) — Splinter Review
Assignee: bugs → bugs.mano
Status: NEW → ASSIGNED
Attachment #186325 - Flags: review?(jhpedemonte)
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
Attachment #186325 - Attachment is obsolete: true
Attachment #186325 - Flags: review?(jhpedemonte)
(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.
Flags: blocking1.8b4?
Attached patch patch (obsolete) — Splinter Review
Attachment #186372 - Flags: review?(jhpedemonte)
Flags: blocking1.8b4?
Flags: blocking1.8b3?
Flags: blocking-aviary1.1?
Summary: MacOS X Shell Service → Mac OS X Shell Service ("Set default browser" support, etc.)
Comment on attachment 186372 [details] [diff] [review]
patch

there are some errors in the windows shell service changes
Attachment #186372 - Attachment is obsolete: true
Attachment #186372 - Flags: review?(jhpedemonte) → review-
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #186390 - Flags: review?(jhpedemonte)
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).
Attachment #186390 - Flags: review?(jhpedemonte) → review+
Attached patch patch (obsolete) — Splinter Review
Mike, can you review the rest of this patch (js/xul + makefiles).
Attachment #186390 - Attachment is obsolete: true
Attachment #186493 - Flags: review?(mconnor)
Rewrite |IsDefaultBrowser()| to use bundle identifiers. Javier, could you
re-review that part?
Attachment #186493 - Attachment is obsolete: true
Attachment #186667 - Flags: superreview?(mconnor)
Attachment #186667 - Flags: review?(jhpedemonte)
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.
Attachment #186667 - Flags: review?(jhpedemonte) → review+
Blocks: 255560
Comment on attachment 186667 [details] [diff] [review]
[checked in] patch

sort of sr=mconnor/josh on irc. a=shaver. will land later today.
Attachment #186667 - Flags: superreview?(mconnor) → superreview+
Attachment #186667 - Attachment description: patch → [checked in] patch
checked in.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Flags: blocking1.8b3?
Flags: blocking-aviary1.1?
verified with DP trunk build 2005-07-12-07-trunk

though a clean system must exist to have it work (see invalidated bug 299833 )
Status: RESOLVED → VERIFIED
Depends on: 443757
You need to log in before you can comment on or make changes to this bug.