Closed Bug 571193 Opened 9 years ago Closed 9 years ago

make Mac OS X share UNIX filesystem code

Categories

(Core :: XPCOM, defect)

All
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0b3

People

(Reporter: jaas, Assigned: jaas)

References

(Depends on 1 open bug)

Details

Attachments

(1 file, 8 obsolete files)

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.
Attached patch fix v0.9 (obsolete) — Splinter Review
This patch is far enough along that xpcshell and Firefox will run. There are some blanks to fill in and I doubt tests pass.
Attached patch fix v0.95 (obsolete) — Splinter Review
Filesystem xpcshell tests pass, fixed a nasty apprunner bug, other fixes. Fixes bugs 506812, 528447, and 530188.
Attachment #450329 - Attachment is obsolete: true
Attached patch fix v0.96 (obsolete) — Splinter Review
Attachment #450425 - Attachment is obsolete: true
Attached patch fix v1.0 (obsolete) — Splinter Review
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.
Attachment #450592 - Attachment is obsolete: true
Attachment #450613 - Flags: review?(mstange)
Attached patch fix v1.1 (obsolete) — Splinter Review
Updated to current trunk.
Attachment #450613 - Attachment is obsolete: true
Attachment #450613 - Flags: review?(mstange)
Attachment #451199 - Flags: review?(mstange)
Attachment #451199 - Flags: superreview?(benjamin)
Attachment #451199 - Flags: review?(mstange) → review?(b56girard)
See bug 57995 for more information on SetFollowLinks/GetFollowLinks.
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.
(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.
Attached patch fix v1.2 (obsolete) — Splinter Review
Attachment #451199 - Attachment is obsolete: true
Attachment #453507 - Flags: review?(b56girard)
Attachment #451199 - Flags: superreview?(benjamin)
Attachment #451199 - Flags: review?(b56girard)
Attachment #453507 - Flags: superreview?(benjamin)
Attachment #453507 - Flags: review?(b56girard) → review+
Blocks: 506812, 528447, 530188
Attached patch fix v1.3 (obsolete) — Splinter Review
updated to current trunk
Attachment #453507 - Attachment is obsolete: true
Attachment #456286 - Flags: superreview?(benjamin)
Attachment #453507 - Flags: superreview?(benjamin)
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
Attachment #456286 - Flags: superreview?(benjamin) → superreview+
Attached patch fix v1.4 (obsolete) — Splinter Review
indentation and comment fixes

pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/f6c93f02146c
Attachment #456286 - Attachment is obsolete: true
Attached patch fix v1.5Splinter Review
Includes fixes for a test failure and a memory leak.
Attachment #458001 - Attachment is obsolete: true
Attachment #459099 - Flags: review?(b56girard)
Attachment #459099 - Flags: review?(b56girard) → review+
pushed to mozilla-central again

http://hg.mozilla.org/mozilla-central/rev/9c005e5d9719
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Depends on: 581182
Target Milestone: --- → mozilla2.0b3
Depends on: 703161
You need to log in before you can comment on or make changes to this bug.