195148 - XPInstall API's getFolder should support Classic and OSX folders under OSX

Status: VERIFIED FIXED

(Core Graveyard :: Installer: XPInstall Engine, enhancement)

Description

[Arun Ranganathan]

Currently, on Mac OS X, the getFolder method (getFolder:
http://devedge.netscape.com/library/manuals/2001/xpinstall/1.0/Chap211.html#1012570
) can be invoked with a few special strings to return the locations of special
folders such as "Plugins", "Components" and a few other platform specific ones
on Mac OS, such as "Mac Preferences".  

But performing installations to the Global Plugins directory (/Library/Internet
Plug-ins), including for XPT files as well as actual plugin files, is becoming
desirable, especially on OS like Mac where there aren't two distinct binary
architectures for plugins (ActiveX and NPAPI).  Thus, one XPInstall could
benefit multiple browsers if the support is there.

So this RFE is asking for some syntactic sugar that returns the global plugins
directory (say something like getFolder("Mac Plugins")), especially if the user
has permission to install there.  If not, they can harness the error condition
and install to the user-specific plugins directory (or whatever
getFolder("Plugins") points to).

Comment 1

[Conrad Carlen (not reading bugmail)]

There is the backend support for this in directory service. See the key
NS_APP_PLUGINS_DIR_LIST. It returns a list of directories. They are are (on OSX
anyway):

Plug-ins in the app bundle
~/Library/Internet Plug-ins
/Library/Internet Plug-ins

That set and its order is not frozen, but could be. Maybe we could say that the
order of the folders returned is ranked in terms of size of domain over which
it's shared (the opposite of the above list). Or, permissions restrictions. 

Comment 2

[Samir Gehani]

installer triage: nsbeta1+ adt2
still need bugs filed for the other target folders that are currently
disabled/#ifdef'ed out on Mac OS X for the getFolder() function.

Comment 3

[Arun Ranganathan]

I suggested above (when I logged this bug) that you could call
getFolder("file:///", subPath) to get to the HD::Library::Internet Plug-ins
location.  

But actually, that's not always true.  In fact, some drives are called Macintosh
HD but what if the drive was called Arun OS X HD ?  Essentially, what I mean is,
you can never tell what the volume name is going to be.

So I use this technique:

var subPath = "../Library/Internet Plug-ins";
var globalPluginsPath = getFolder("Mac System", subPath);

Then to add to /Library/Internet Plug-ins, I use an addFile invocation on
globalPluginsPath.  

This is a bit of a hack, and we ought to have a better way, but I'm posting this
here so that someone can see a workaround. I don't think there's a way to use
Install.gestalt( ) either.

Of course, it's a different matter that I ran into bug 195377 .

Comment 4

[Conrad Carlen (not reading bugmail)]

> var globalPluginsPath = getFolder("Mac System", subPath);

Don't do that. That only works on XP_MAC (CFM) builds, which we no longer make.
"Mac System" means the Mac Classic system folder which, on an OSX system,
doesn't have to exist, or may be on a different drive from "/"

A better way is to ask directory service for NS_APP_PLUGINS_DIR_LIST, which
gives back an nsISimpleEnumerator, and use the last item. That will be
/Library/Internet Plug-ins. Because of permissions of the current user, you
really want to enumerate backwards over the list of returned plugin folders
until success. Oh well, with an nsISimpleEnumerator, that'll take some work :-/

Comment 5

[Arun Ranganathan]

ccarlen,

I suspected that my workaround would only apply to CFM builds (e.g. N7.02) since
the System X directory need not exist.  The purpose of this RFE was to expose a
much better way via XPInstall.  What you propose isn't workable in today's
XPInstall unless I'm mistaken.

Comment 6

[Sean Su]

Attachment: patch v1.0

initial patch that will now support the following folders under OSX:

   Classic folders
     ** Mac System		    : /System Folder/
     ** Mac Desktop		    : null
     ** Mac Trash		    : null
     ** Mac Startup		    : /System Folder/Startup Items/
     ** Mac Shutdown		    : /System Folder/Shutdown Items/
     ** Mac Apple Menu		    : /System Folder/Apple Menu Items/
     ** Mac Control Panel	    : /System Folder/Control Panels/
     ** Mac Extension		    : /System Folder/Extensions/
     ** Mac Fonts		    : /System Folder/Fonts/
     ** Mac Preferences 	    : /System Folder/Preferences/
     ** Mac Documents		    : null

  OSX folders
     ** Temporary		    : /private/tmp/9299/Temporary Items/
     ** MacOSX Home		    : /Users/[user]/ 
     ** MacOSX Default Download     : /Users/[user]/Desktop/
     ** MacOSX User Desktop	    : /Users/[user]/Desktop/
     ** MacOSX Local Desktop	    : null
     ** MacOSX User Applications    : /Users/[user]/Applications/
     ** MacOSX Local Applications   : /Applications/
     ** MacOSX User Documents	    : /Users/[user]/Documents/
     ** MacOSX Local Documents	    : null
     ** MacOSX User Internet PlugIn : /Users/[user]/Library/Internet Plug-Ins/
     ** MacOSX Local Internet PlugIn: /Library/Internet Plug-Ins/
     ** MacOSX User Frameworks	    : null
     ** MacOSX Local Frameworks     : /Library/Frameworks/
     ** MacOSX User Preferences     : /Users/[user]/Library/Preferences/
     ** MacOSX Local Preferences    : /Library/Preferences/
     ** MacOSX Picture Documents    : /Users/[user]/Pictures/
     ** MacOSX Movie Documents	    : /Users/[user]/Movies/
     ** MacOSX Internet Sites	    : /Users/[user]/Sites/

Comment 7

[Sean Su]

Attachment: .xpi test file that will test the new folder functions under OSX

Trigger this .xpi file to test the new osx folder functions.  After
installation is complete, look at install.log for output.

Comment 8

[Sean Su]

updating summary to reflect fix.

Comment 9

[Conrad Carlen (not reading bugmail)]

Comment on attachment 119205 [details] [diff] [review]
patch v1.0

>Index: xpcom/io/SpecialSystemDirectory.cpp
>===================================================================
> #if defined(XP_MAC)
> //----------------------------------------------------------------------------------------
> static nsresult nsIFileFromOSType(OSType folderType, nsILocalFile** aFile)
> //----------------------------------------------------------------------------------------
> {
>     CInfoPBRec cinfo;
>     DirInfo *dipb=(DirInfo *)&cinfo;
>     
>     // hack: first check for kDefaultDownloadFolderType
>     // if it is, get Internet Config Download folder, if that's
>     // not availble use desktop folder
>     if (folderType == kDefaultDownloadFolderType)
>     {
>       nsCOMPtr<nsIInternetConfigService> icService (do_GetService(NS_INTERNETCONFIGSERVICE_CONTRACTID));
>       if (icService)
>       {
>         if (NS_SUCCEEDED(icService->GetDownloadFolder(&mSpec)))
>           return;
>         else
>           folderType = kDesktopFolderType;
>@@ -823,20 +847,21 @@
>                 // The "create" parameter, however, is sadly ignored.
>                 // How do we internationalize this?
>                 return nsIFileFromOSType(kVolumeRootFolderType;
>                 *this += "Documents";
>                 CreateDirectory();
>                 break;
> 		} // switch
>     } // for
>     StrFileName filename;
>     filename[0] = '\0';
>     dipb->ioNamePtr = (StringPtr)&filename;
>     dipb->ioFDirIndex = -1;
>     
>     mError = NS_FILE_RESULT(PBGetCatInfoSync(&cinfo));
>     if (NS_SUCCEEDED(mError))
>     {
> 	    mError = NS_FILE_RESULT(FSMakeFSSpec(dipb->ioVRefNum, dipb->ioDrParID, filename, &mSpec));
>     }
> }
> #endif // XP_MAC
>+

This code wouldn't compile if XP_MAC was on. Can you just chop it out while
you're there?
>Index: xpcom/io/SpecialSystemDirectory.h
>===================================================================
>                           nsILocalFile** aFile);
>+NS_IMETHODIMP
>+GetOSXFolderType(short aDomain, OSType aFolderType, nsILocalFile **localFile);
>
Since this isn't an nsI method impl, use an nsresult

>Index: xpcom/io/nsDirectoryServiceDefs.h
>===================================================================
>-    #define NS_MAC_CLASSICPREFS_DIR             "ClassicPrfs"

Make sure to do a full build of the tree - I think this may be used in profile
mgr and that'll need to be changed to NS_MAC_PREFS_DIR.

And then, not from the patch:

** MacOSX User Frameworks	    : null

These few which are null - I suspect it's because they never exist (?) Can you
look into that and, if they never will exist, get rid of them? Symmetry is
nice, but...

Otherwise - excellent!

Comment 10

[Sean Su]

Attachment: patch v1.1

this patch addresses conrad's comments:

* removed obsolete XP_MAC code that doesn't compile
* fixed return type of GetOSXFolderType()
* searched for all other occurrances of NS_MAC_CLASSICPREFS_DIR and replaced it
with NS_MAC_PREFS_DIR
* 'MacOSX User Frameworks' no longer returns 'null'.  'Local Desktop' and
'Local Documents' still return null, but they exist.  Could be an insufficient
access problem.  I'm leaving it in the patch in case the user is able to get
the appropriate permissions for access.

Comment 11

[Conrad Carlen (not reading bugmail)]

Comment on attachment 119222 [details] [diff] [review]
patch v1.1

r=ccarlen

Comment 12

[Arun Ranganathan]

Looks like the following things will happen that I'm interested in(ssu/ccarlen,
correct me if wrong):

1. To install to the app. bundle's plugins directory, getFolder("Plugins") will
work as usual.

2. To install to the user's plugins directory, getFolder("MacOSX User Internet
PlugIn") will now work instead of any hack.

3. To install to the Global Plugins directory, getFolder("MacOSX Local Internet
PlugIn") will now work instead of any hack.

There is a potential fourth installation directory, namely what is specified in
the OS X Un*x shell via an environment variable, but let's not worry about that.

This pretty much meets my requirements when I logged the bug, namely that the
API should offer robust accessibility to other locations on the machine for
plugins to be installed to.  I'm CC'ing peterl for completeness.

Comment 13

[(not reading, please use [e-mail] instead)]

Comment on attachment 119222 [details] [diff] [review]
patch v1.1

sr=sspitzer (moving r= to dveditz, in case you wanted him to see this as well.)

two minor questions / comments:

1)

was nothing needed for Mac OS X?  Or will cavin be back in here, when he trying
to fix 4.x migration for Mac OS X?

 #if defined(XP_OS2)
     NS_GetSpecialDirectory(NS_OS2_DIR, getter_AddRefs(oldRegFile));
 #elif defined(XP_WIN)
     NS_GetSpecialDirectory(NS_WIN_WINDOWS_DIR, getter_AddRefs(oldRegFile));
 #elif defined(XP_MAC)
-    NS_GetSpecialDirectory(NS_MAC_CLASSICPREFS_DIR,
getter_AddRefs(oldRegFile));
+    NS_GetSpecialDirectory(NS_MAC_PREFS_DIR, getter_AddRefs(oldRegFile));
 #endif
 #endif


2)

I haven't seen the surrounding code, but I bet maybe this code be made smaller. 

looks like a lot of copy paste.  I bet it could be written to have this code
once:


+		 nsCOMPtr<nsIProperties> directoryService = 
+			  do_GetService(NS_DIRECTORY_SERVICE_CONTRACTID, &rv);
+		 if (!directoryService) return;
+		 directoryService->Get(XXX, 
+					NS_GET_IID(nsIFile), 
+					getter_AddRefs(mFileSpec));

and use the switch statement, to determine XXX from the switch value.

if you agree, feel free to spin that off to a code cleanup bug.

Comment 14

[Doug Turner (:dougt)]

Comment on attachment 119222 [details] [diff] [review]
patch v1.1

this work will collide with the work alec is doing.  please talk with him
before landing.

I do not think that you want to define this globally - please wrap it in a
#ifdef.

+nsresult
+GetOSXFolderType(short aDomain, OSType aFolderType, nsILocalFile **localFile);

Comment 15

[Conrad Carlen (not reading bugmail)]

> was nothing needed for Mac OS X?  Or will cavin be back in here, when he trying
to fix 4.x migration for Mac OS X?

Yes, something is needed for that code but it's not part of this bug. Once this
is checked in, it will be easy to use what this patch gives to enable 4.x
migration for Mach-O. BTW, I have bug 190336. Didn't know we had decided to fix
that or not.

Comment 16

[Sean Su]

to address seth's comment #2, bug 125106 will take care of optimizations when I
merge its patch to the trunk.

I'll attach a new patch given dougt's comment.

Comment 17

[Sean Su]

Attachment: patch v1.2

this patch has been verified to compile under OSX, Linux, and Win32.

Comment 18

[Sean Su]

Attachment: patch v1.3 (merged with current trunk)

This is simply an updated patch that has been merged with trunk.  I ran my .xpi
test file under OSX and everything is still working.

This is the patch that I just checked in.

Comment 19

[Sean Su]

closing bug as fixed.

Comment 20

[Sean Su]

Attachment: updated test .xpi file

the old .xpi file was missing the test for the User Music folder.  This one has
it.

Comment 21

[Ninoschka Baca]

Trunk build 2003-04-07: Mac 10.1.5
I just noticed with this build that the Import Mail dialog (or Import Settings
dialog) no longer displays Comm 4.x as an option. In fact this dialog is blank.
Could this be a regression due to this fix?  I checked trunk build 2003-04-04
and it atleast lists the Comm 4.x option.

Comment 22

[(not reading, please use [e-mail] instead)]

> I just noticed with this build that the Import Mail dialog (or Import Settings
> dialog) no longer displays Comm 4.x as an option

can you start a new bug, and assign to cavin, who is looking into mac os x issues.

Comment 23

[(not reading, please use [e-mail] instead)]

> can you start a new bug, and assign to cavin, who is looking into mac os x issues.

I spun it off to bug #201157

Comment 24

[Sean Su]

I just installed Monday's build and from mail, I selected Tools|Import.  For
Address Books and Mail, I see 'Communicator 4.x' listed after I click the "Next"
button from the 1st dialog.

Is this what you're referring to that isn't working?  If not, can you tell me
how to get at the dialog that you're seeing the problem with?

Comment 25

[Ninoschka Baca]

There is a problem but not exactly how I described in comment# 21. I updated 
bug# 201157 which explains in more detail. I don't believe that a recent
regression occured. Sorry for the false alarm.

Comment 26

[Grace Bush]

verified fixed build 2003041410
(thanks for test file) also XPInstall getfolder tests showing same results as
test file :)

Comment 27

[(not reading, please use [e-mail] instead)]

Comment on attachment 119222 [details] [diff] [review]
patch v1.1

clearing review request.