571193 - make Mac OS X share UNIX filesystem code

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Josh Aas]

We should consider getting rid of our Mac OS X filesystem code and using the general UNIX code. We'll have to implement nsILocalFileMac there, but that shouldn't be too bad. We'll also have to watch out for Darwin's special implementation of 'realpath' (see the Mac OS X 10.6 man page).

This should make future maintenance much easier and fix a bunch of bugs in the Mac OS X implementation, such as bug 530188.

Comment 1

[Josh Aas]

Attachment: fix v0.9

This patch is far enough along that xpcshell and Firefox will run. There are some blanks to fill in and I doubt tests pass.

Comment 2

[Josh Aas]

Attachment: fix v0.95

Filesystem xpcshell tests pass, fixed a nasty apprunner bug, other fixes. Fixes bugs 506812, 528447, and 530188.

Comment 3

[Josh Aas]

Attachment: fix v0.96

Comment 4

[Josh Aas]

Attachment: fix v1.0

I'm going to do some more testing before review but I think this might be ready to go. It unifies Mac OS X and UNIX behavior, fixes bugs 506812, 528447, and 530188, and we drop about 1800 lines of Mac-specific code.

The only potential downside is that we lose support for SetFollowLinks/GetFollowLinks for now. UNIX never implemented it. Adding it will be a pretty major undertaking and before we do that we should really make it more clear what exactly that API is supposed to do. Everything works fine so far with it unimplemented, so I think we're fine moving ahead without it for now.

Comment 5

[Josh Aas]

Attachment: fix v1.1

Updated to current trunk.

Comment 6

[Josh Aas]

See bug 57995 for more information on SetFollowLinks/GetFollowLinks.

Comment 7

[Benoit Girard (:BenWa)]

Comment on attachment 451199 [details] [diff] [review]
fix v1.1

>+  nsresult RevealFileInFinder(CFURLRef url)
>+  {
>+    NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NSRESULT;
>+
>+    NSAutoreleasePool* ap = [[NSAutoreleasePool alloc] init];
>+    BOOL success = [[NSWorkspace sharedWorkspace] selectFile:[(NSURL*)url path] inFileViewerRootedAtPath:@""];
>+    [ap release];

Is a release pool needed here and a few other following methods? The documentation doesn't seem to indicate that an object is created.

>+    NSAutoreleasePool* ap = [[NSAutoreleasePool alloc] init];
>+    NSDictionary* dict = [[NSFileManager defaultManager] fileAttributesAtPath:[(NSURL*)url path] traverseLink:YES];
>+    id creatorNum = (NSNumber*)[dict objectForKey:NSFileHFSCreatorCode];

id should be NSNumber*.

>+  nsresult GetFileTypeCode(CFURLRef url, OSType *typeCode)
>+  {
>+    NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NSRESULT;
>+
>+    nsresult rv = NS_ERROR_FAILURE;
>+

Should this, and other method that take in pointers, also use 'NS_ENSURE_ARG'? 

>+    NSDictionary* dict = [NSDictionary dictionaryWithObject:[NSNumber numberWithUnsignedLong:typeCode] forKey:NSFileHFSTypeCode];
>+    BOOL success = [[NSFileManager defaultManager] changeFileAttributes:dict atPath:[(NSURL*)url path]];

A few of the library functions you used are deprecated in 10.5, such as 'changeFileAttributes:atPath:', and have other alternative suggested in the API. But presumably you have a reason to use these instead?

>+#elif defined(XP_MACOSX)
>+    CFURLRef url;
>+    if (NS_SUCCEEDED(GetCFURL(&url)) && url) {
>+      nsresult rv = CocoaFileUtils::RevealFileInFinder(url);

'&& url' isn't needed, this is done in a few places.

>+
>+NS_IMETHODIMP
>+nsLocalFile::InitWithCFURL(CFURLRef aCFURL)
>+{
>+  UInt8 path[PATH_MAX];
>+  if (::CFURLGetFileSystemRepresentation(aCFURL, false, path, PATH_MAX)) {
>+    nsDependentCString nativePath((char*)path);

Don't HFS+ path use UTF-16 encoding? 'CFURLGetFileSystemRepresentation' is said to return 'the file system's native string representation'.
Also is PATH_MAX defined in terms of bytes or characters since UTF-8/16 are variable length?

>+NS_IMETHODIMP
>+nsLocalFile::OpenDocWithApp(nsILocalFile *aAppToOpenWith, PRBool aLaunchInBackground)
>+{
>+  FSRef docFSRef;
>+  nsresult rv = GetFSRef(&docFSRef);
>+  if (NS_FAILED(rv))
>+    return rv;
>+
>+  if (!aAppToOpenWith) {
>+    OSErr err = ::LSOpenFSRef(&docFSRef, NULL);
>+    return MacErrorMapper(err);
>+  }
>+
>+  nsCOMPtr<nsILocalFileMac> appFileMac = do_QueryInterface(aAppToOpenWith, &rv);
>+  if (!appFileMac)
>+    return rv;

'rv' should not be an error at the last return, an error should be returned instead.

Comment 8

[Josh Aas]

(In reply to comment #7)

> (From update of attachment 451199 [details] [diff] [review])
> >+    NSAutoreleasePool* ap = [[NSAutoreleasePool alloc] init];
> >+    BOOL success = [[NSWorkspace sharedWorkspace] selectFile:[(NSURL*)url path] inFileViewerRootedAtPath:@""];
> >+    [ap release];
> 
> Is a release pool needed here and a few other following methods? The
> documentation doesn't seem to indicate that an object is created.

I typically put an autorelease pool around any Obj-C code that might run outside of the event loop in case the underlying code creates any autoreleased objects. Pools are cheap.

> >+    NSDictionary* dict = [NSDictionary dictionaryWithObject:[NSNumber numberWithUnsignedLong:typeCode] forKey:NSFileHFSTypeCode];
> >+    BOOL success = [[NSFileManager defaultManager] changeFileAttributes:dict atPath:[(NSURL*)url path]];
> 
> A few of the library functions you used are deprecated in 10.5, such as
> 'changeFileAttributes:atPath:', and have other alternative suggested in the
> API. But presumably you have a reason to use these instead?

Good point, I'll change to "setAttributes:ofItemAtPath:error:" but I'll leave "fileAttributesAtPath:traverseLink:" because the replacement's symlink behavior is potentially different and undefined. We can deal with that later if we want.

> >+NS_IMETHODIMP
> >+nsLocalFile::InitWithCFURL(CFURLRef aCFURL)
> >+{
> >+  UInt8 path[PATH_MAX];
> >+  if (::CFURLGetFileSystemRepresentation(aCFURL, false, path, PATH_MAX)) {
> >+    nsDependentCString nativePath((char*)path);
> 
> Don't HFS+ path use UTF-16 encoding? 'CFURLGetFileSystemRepresentation' is said
> to return 'the file system's native string representation'.
> Also is PATH_MAX defined in terms of bytes or characters since UTF-8/16 are
> variable length?

The code is written in my patch the way we have been doing it in the existing mac-specific FS impl.

The documentation is confusing here. CFURLCopyPath mentions "If the path is to be passed to file system calls, you may also want to use the function CFURLGetFileSystemRepresentation, which returns a C string." If this is true then the code is correct.

This set of threads has more good conversation on the subject:

http://lists.apple.com/archives/carbon-dev/2005/Feb/thrd12.html

> >+  if (!appFileMac)
> >+    return rv;
> 
> 'rv' should not be an error at the last return, an error should be returned
> instead.

I don't understand this comment.

Comment 9

[Josh Aas]

Attachment: fix v1.2

Comment 10

[Josh Aas]

Attachment: fix v1.3

updated to current trunk

Comment 11

[Benjamin Smedberg]

Comment on attachment 456286 [details] [diff] [review]
fix v1.3

In CocoalFileUtils.h, the methods are indented. Standard style is not to indent things in a namespace block, and do add a // namespace comment at the namespace-close, i.e.

namespace CocoaFileUtils {

nsresult methodfoo();
...

} // namespace CocoaFileUtils

It's also a little strange that you're not using mozilla::cocoafileutils, since that is the pattern for all C++ namespace users in our codebase.

Definitely the methods in CocoaFileUtils.mm should not be indented.

r=me with that fixed

Comment 12

[Josh Aas]

Attachment: fix v1.4

indentation and comment fixes

pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/f6c93f02146c

Comment 13

[Josh Aas]

backed out due to test failure, bug 579584

http://hg.mozilla.org/mozilla-central/rev/d0d098061840
http://hg.mozilla.org/mozilla-central/rev/c1c5cc1da054

Comment 14

[Josh Aas]

Attachment: fix v1.5

Includes fixes for a test failure and a memory leak.

Comment 15

[Josh Aas]

pushed to mozilla-central again

http://hg.mozilla.org/mozilla-central/rev/9c005e5d9719