Closed
Bug 118203
Opened 23 years ago
Closed 22 years ago
[mach] Need an nsLocalFile implementation
Categories
(Core :: XPCOM, defect)
Tracking
()
RESOLVED
FIXED
mozilla1.0
People
(Reporter: ccarlen, Assigned: ccarlen)
References
Details
(Keywords: topembed+)
Attachments
(2 files, 7 obsolete files)
49.38 KB,
patch
|
sfraser_bugs
:
review+
sfraser_bugs
:
superreview+
|
Details | Diff | Splinter Review |
280.78 KB,
patch
|
sfraser_bugs
:
review+
sfraser_bugs
:
superreview+
|
Details | Diff | Splinter Review |
This came from bug 110372. Summary and that bug say it all.
Updated•23 years ago
|
Summary: Need an nsLocalFile implementation for Mach-0 → [mach] Need an nsLocalFile implementation
Comment 3•23 years ago
|
||
Will fixing this bug fix the following two issues, or should I file seperate bugs on them? 1) When downloading something, files end up in the /tmp directory when the download is finished, rather than beign moved to where I told it to save it 2) when trying to upload a file, (like adding an attachment to bugzilla), after selecting the file through the file picker, the textfield that normall contains a file name isnt' populated.
Comment 4•23 years ago
|
||
Marcus, yes, the problems you mention are due to nsLocalFile. No need to file additional bugs.
Updated•23 years ago
|
Target Milestone: mozilla0.9.9 → mozilla1.0
Comment 5•23 years ago
|
||
scc, I have been taking qa on mach-o bugs since most qa contacts do not have access to mach-o builds. Do you want to keep this one?
Comment 6•23 years ago
|
||
zach, scc is on sabbatical for a few more weeks. And I doubt he'd mind you taking the QA contact on this :-)
Updated•22 years ago
|
Comment 10•22 years ago
|
||
*** Bug 132841 has been marked as a duplicate of this bug. ***
Comment 11•22 years ago
|
||
I will gladly test a fix for this bug *IF* it indeed fixes 86538 as well.
Updated•22 years ago
|
Keywords: mozilla1.0.1,
nsbeta1
Assignee | ||
Comment 12•22 years ago
|
||
The patch does this: 1) Adds nsLocalFileOSX 2) Adds new OSX files to our build of MoreFiles, removes the old HFS impl files from MoreFiles. The new files added to MoreFiles are from Apple DTS and (IMO) don't needs review. 3) Changes the registry lib code to use MoreFilesX instead of old MoreFiles. It was the only consumer of MoreFiles in the tree. 4) Changes to chimera and directory service to make use of the new file impl. Argh. With the new files, the patch exceeded the 300K attachment limit. So, I had to ZIP it.
Assignee | ||
Comment 13•22 years ago
|
||
The patch does this: 1) Adds nsLocalFileOSX 2) Adds new OSX files to our build of MoreFiles, removes the old HFS impl files from MoreFiles. The new files added to MoreFiles are from Apple DTS and (IMO) don't needs review. 3) Changes the registry lib code to use MoreFilesX instead of old MoreFiles. It was the only consumer of MoreFiles in the tree. 4) Changes to chimera and directory service to make use of the new file impl. Argh. With the new files, the patch exceeded the 300K attachment limit. So, I had to ZIP it.
Assignee | ||
Comment 14•22 years ago
|
||
Comment on attachment 88790 [details] [diff] [review] patch nevermind this
Attachment #88790 -
Attachment is obsolete: true
Assignee | ||
Comment 15•22 years ago
|
||
Attachment #88791 -
Attachment is obsolete: true
Assignee | ||
Comment 16•22 years ago
|
||
The two patches here should make for easier review. The Apple DTS code is split off into it's own patch and some chimera code to make use of the new file code has been removed - that should be its own bug.
Comment 17•22 years ago
|
||
are there any possible performance effects w/ this bug?
Comment 18•22 years ago
|
||
Comment on attachment 88792 [details] [diff] [review] patch with only AppleDTS code to be added to MoreFiles r=sdagley (being is this _is_ the Apple MoreFiles code)
Attachment #88792 -
Flags: review+
Assignee | ||
Comment 19•22 years ago
|
||
Attachment #88793 -
Attachment is obsolete: true
Comment 20•22 years ago
|
||
comments: - you set an arbitrary 512 max length on paths, is there any way to remove that or figure out how long the path is for an FSRef before you convert it to a path? - how does that 512 relate to PATH_MAX on other platforms, or on darwin? + FileInfo fInfo; // Finder flags are in the same place whether we use FileInfo or FolderInfo + ::BlockMoveData(&catalogInfo.finderInfo, &fInfo, sizeof(FileInfo)); + if ((fInfo.finderFlags & kIsInvisible) != 0) { + *_retval = PR_TRUE; + } don't need to copy data just to check a flag + // XXX - Because we define DARWIN, we can't use CFURLGetFSRef. This blows. file a bug for us to turn that off. we really should not be defining DARWIN when building mozilla. as a workaround, can't you just #undef DARWIN at the top of the file before including anything else? we do that in widget/src/cocoa all over the place. + // XXX - This should be cut from the API. Would create an evil dependency. how do the other platforms handle it? + // *outIsPackage = ((mCachedCatInfo.dirInfo.ioFlAttrib & kioFlAttribDirMask) && + // (mCachedCatInfo.dirInfo.ioDrUsrWds.frFlags & kHasBundle)); if we're not going to check the bundle flag, we need to check for more than just .app (.plugin? or any cfm app that's a package that doesn't end in .app, like mozilla) with these fixed, r=pink. conrad and i have already discussed them and he's working on them. just wanted to include them here so any sr (read: smfr) would know i'd already brought them up.
Comment 21•22 years ago
|
||
Comment on attachment 89068 [details] [diff] [review] updated patch r=pink with above comments fixed
Attachment #89068 -
Flags: review+
Comment 22•22 years ago
|
||
Comments on libreg part of the patch: Index: modules/libreg/src/reg.c =================================================================== +static OSErr isFileInTrash(FSSpec *fileSpec, PRBool *inTrash) +{ + OSErr err; + short vRefNum; + long dirID; + + if (fileSpec == NULL || inTrash == NULL) + return paramErr; + *inTrash = PR_FALSE; + + err = FindFolder(fileSpec->vRefNum, kTrashFolderType, false, &vRefNum, &dirID); + if (err == noErr) + if (dirID == fileSpec->parID) /* File is inside the trash */ + *inTrash = PR_TRUE; + + return err; +} +#endif Can you add a comment here that this won't always work, because the file may be inside another folder in the trash (but don't bother fixing this time around). It needs logic like your Mac OS X version above. + err = FSResolveAlias(NULL, h, &fsRef, &wasChanged); + if (err != noErr) + goto fail; + + /* if the alias has changed and the file is now in the trash, + assume that user has deleted it and that we do not want to look at it */ + if (wasChanged && (isFileInTrash(&fsRef, &inTrash) == noErr) && inTrash) I don't think the 'wasChanged' param means that the file is an alias. 'wasChanged' means that the alias had to be updated, because the target of the alias moved. To tell if it is actually an alias, test to see if FSResolveAlias() changed the fsRef (FSCompareFSRefs on the before and after). Index: modules/libreg/src/vr_stubs.c =================================================================== +static const UniChar kOSXRegName[] = { 'g', 'l', 'o', 'b', 'a', 'l', 'r', 'e', 'g', '.', 'd', 'a', 't'}; This is our chance to have a nice human-readable name for Mac OS X. Can we choose something better? OS 9 has "Application Registry". How about "Profiles Registry.dat" or somesuch? It would also be nice to have a unique extension for our registry files, so that they don't show up as Excel documents in the Finder. + if (!err) if (err == noErr) please (several places). + err = FSMakeFSRefUnicode(&foundRef, UNICHAR_ARRAY_LEN(kOSXRegParentName), kOSXRegParentName, + kTextEncodingUnknown, &parentRef); kTextEncodingUnknown? Some source examples use kTextEncodingUnicodeDefault. + if (err == fnfErr) Are you always going to get fnfErr here? Did you test this code path? + FSCatalogInfo catalogInfo; + ((FileInfo *) &(catalogInfo.finderInfo))->fileType = 'REGS'; + ((FileInfo *) &(catalogInfo.finderInfo))->fileCreator = 'MOSS'; + ((FileInfo *) &(catalogInfo.finderInfo))->finderFlags = 0; + ((FileInfo *) &(catalogInfo.finderInfo))->location.h = 0; + ((FileInfo *) &(catalogInfo.finderInfo))->location.v = 0; + ((FileInfo *) &(catalogInfo.finderInfo))->reservedField = 0; Maybe more nicely written as: FileInfo fInfo = { 'REGS', 'MOSS' }; // rest of fields filled with zero BlockMoveData(&fInfo, &catalogInfo.finderInfo, sizeof(FileInfo)); [above comments apply to vr_findVerRegName() too]
Comment 23•22 years ago
|
||
Comments on the rest: Index: xpcom/io/nsLocalFileOSX.cpp =================================================================== + nsLocalFile *newFile = new nsLocalFile(mFSRefsArray[mArrayIndex], nsString()); NS_LITERAL_STRING("") might be slightly more efficient than nsString(). + err = ::FSMakeFSRefUnicode(&mFSRef, aNode.Length(), + (UniChar *)PromiseFlatString(aNode).get(), + kTextEncodingUnknown, &newLeafRef); Maybe use kTextEncodingUnicodeDefault? (Several other places too.) +NS_IMETHODIMP nsLocalFile::SetLastModifiedTime(PRInt64 aLastModifiedTime) ... +#if XP_MACOSX + FSRef parentRef; + PRBool notifyParent; Why the #ifdefs here? +NS_IMETHODIMP nsLocalFile::IsSymlink(PRBool *_retval) +{ + NS_ERROR("NS_ERROR_NOT_IMPLEMENTED"); + return NS_ERROR_NOT_IMPLEMENTED; +} Should we implement this for OS X? Should we also consider an alias to be a symlink? +NS_IMETHODIMP nsLocalFile::Contains(nsIFile *inFile, PRBool recur, PRBool *_retval) ... + while ((::FSGetParentRef(&inFSRef, &parentFSRef) == noErr && + memcmp(&parentFSRef, &kInvalidFSRef, sizeof(FSRef)) != 0)) { Use the MoreFilesX routine to test for a valid FSRef here. + *_retval = (::strncmp(PromiseFlatCString(thisPath).get(), + PromiseFlatCString(inPath).get(), thisPath.Length()) == 0); + Do paths to directories always end in a separator? If not, then this comparison might erroneously say that "foo/bar/baz" contains "foo/bar/bazooka/quux". +NS_IMETHODIMP nsLocalFile::GetParent(nsIFile * *aParent) ... +#if DEBUG + // In a debug build, where components are symlinks, we need to do it this way. + // If a file object which is a symlink is asked for its parent, its parent + // will be that of the resolved link which isn't what the component mgr + // expects. Oh lordy. It seems very wrong to have code here that is i) Debug only ii) Pandering to some idiosyncracy of the component manager. It seems quite normal for release builds to have symlinks to other libs in the component directory. We should either state that this is not supported, or fix the component manager. + if (parentRef == kInvalidFSRef) + return NS_OK; Another place for the proper invalidity check. +NS_IMETHODIMP nsLocalFile::InitWithPath(const nsAString& filePath) ... + // First attempt to convert directly to an FSRef Say in your comment why this might legally fail. + // Argh - since no rtti, we have to use a static cast. + // But, we can be sure that this is the only object implementing nsILocalFile. + nsLocalFile *asLocalFile = NS_STATIC_CAST(nsLocalFile*, aFile); You could QI to nsILocalFileMac as a further check. + mFSRef = asLocalFile->mFSRef; + mNonExtantNodes = asLocalFile->mNonExtantNodes; + mTargetFSRef = asLocalFile->mTargetFSRef; + mIdentityDirty = PR_TRUE; Couldn't you factor this code and the copy ctor code using a shared method? + *_retval = PR_Open(path.get(), flags, mode); ... + *_retval = fopen(path.get(), mode); Do NSPR and MSL handle UTF-8 paths OK? + // XXX - Because we define DARWIN, we can't use CFURLGetFSRef. This blows. Why not? +/* void setFileTypeAndCreatorFromMIMEType (in string aMIMEType); */ +NS_IMETHODIMP nsLocalFile::SetFileTypeAndCreatorFromMIMEType(const char *aMIMEType) +{ + // XXX - This should be cut from the API. Would create an evil dependency. + NS_ERROR("NS_ERROR_NOT_IMPLEMENTED"); + return NS_ERROR_NOT_IMPLEMENTED; +} + +/* void setFileTypeAndCreatorFromExtension (in string aExtension); */ +NS_IMETHODIMP nsLocalFile::SetFileTypeAndCreatorFromExtension(const char *aExtension) +{ + // XXX - This should be cut from the API. Would create an evil dependency. + NS_ERROR("NS_ERROR_NOT_IMPLEMENTED"); + return NS_ERROR_NOT_IMPLEMENTED; +} Is returning errors here going to cause stuff (like file downloading) to fail? Maybe LXR for the callers to see, and return NS_OK.
Comment 24•22 years ago
|
||
Okay, lots of comments from Simon. We're blocking the major QA certification push waiting for this as it is the biggest change we're looking at by far. Given the feedback so far, is the ETA for this still today (Tues.)?
Assignee | ||
Comment 25•22 years ago
|
||
New patch coming up to address comments: First Pink's: 1. you set an arbitrary 512 max length on paths, is there any way to remove that Changed to PATH_MAX which on OSX is 1024. While HFS+ may be capable of unlimited paths lengths, we do have to feed paths to BSD routines, to which I think that limit would apply. Anyway, 1024 is pretty darn long. 2. + // XXX - Because we define DARWIN, we can't use CFURLGetFSRef. This blows. file a bug for us to turn that off Filed bug 154232. I was able to do what I needed to without them, though. 3. + // XXX - This should be cut from the API. Would create an evil dependency. how do the other platforms handle it? Suffix alone is all they need. I think that nsLocalFile should not depend on InternetConfig to make those mappings. Higher level code which saves files can depend on that, but not xpcom. 4. + // *outIsPackage = ((mCachedCatInfo.dirInfo.ioFlAttrib & kioFlAttribDirMask) && + // (mCachedCatInfo.dirInfo.ioDrUsrWds.frFlags & kHasBundle)); if we're not going to check the bundle flag Fixed it to check the bundle flag. And Simon's: 1. Can you add a comment here that this won't always work, Did it. 2. I don't think the 'wasChanged' param means that the file is an alias. Discussed on AIM. The code is OK afterall. 3. This is our chance to have a nice human-readable name for Mac OS X. The file names are nicer now and use .regs as suffix (IMO) 4. kTextEncodingUnknown? Some source examples use kTextEncodingUnicodeDefault. MoreFilesX uses kTextEncodingUnknown. It doesn't matter to HFS+, only used as a hint for the Finder. Besides, these strings only contain ASCII chars. 5. Maybe more nicely written as: Did that. 6. NS_LITERAL_STRING("") might be slightly more efficient than nsString() I don't think so. Especially since the Mach-0 build doesn't have L"" string constants and the NS_LITERAL_STRING macro has to create a temp object to convert. 7. Why the #ifdefs here? Only OSX supports that. Initially, I wanted to make this patch for OSX Mach-0 and CFM. Eventually it will be so the #ifdefs will be useful then. There's also comments: // XXX - Needs work for CFM 8. Should we implement this for OS X? Should we also consider an alias to be a symlink? Implemeted it. Like nsLocalFileMac, I considered aliases to count as symlinks. symlinks seem to be implicitly resolved by HFS+ 9. Use the MoreFilesX routine to test for a valid FSRef here. Did that. 10. Do paths to directories always end in a separator? If not, then this comparison might erroneously say that "foo/bar/baz" contains "foo/bar/bazooka/quux". You're right. Fixed that. 11. Oh lordy. It seems very wrong to have code here that is i) Debug only ii) Pandering to some idiosyncracy of the component manager. Yeah, I agree. But it works and changing the component mgr in order to avoid this at this point, I don't think is a good idea. 12. + if (parentRef == kInvalidFSRef) + return NS_OK; Another place for the proper invalidity check. Did that. 13. +NS_IMETHODIMP nsLocalFile::InitWithPath(const nsAString& filePath) ... + // First attempt to convert directly to an FSRef Say in your comment why this might legally fail. Did that. 14. + // Argh - since no rtti, we have to use a static cast. + // But, we can be sure that this is the only object implementing nsILocalFile. + nsLocalFile *asLocalFile = NS_STATIC_CAST(nsLocalFile*, aFile); You could QI to nsILocalFileMac as a further check. Did that. 15. Couldn't you factor this code and the copy ctor code using a shared method? I ended up changing this routine a little (in order to get around warning from gcc - see the comment) Because of that, it's less similar to the copy constructor. 16. Do NSPR and MSL handle UTF-8 paths OK? For Mach-0, there is no MSL. But, under CFM it doesn't support them and was the main reason I decided to make this Mach-0 only. On Mach-0, NSPR (because Apple's BSD is underneath) handles it well. All char* mathods on the BSD layer take *only* UTF-8. I tested this by putting the app in a path with Japanese chars when the system locale was not set to Japanese (the tough case) and it worked 17. + // XXX - Because we define DARWIN, we can't use CFURLGetFSRef. This blows. Why not? Those routines are inside #ifndef DARWIN in the CoreFoundation headers. +/* void setFileTypeAndCreatorFromExtension (in string aExtension); */ +NS_IMETHODIMP nsLocalFile::SetFileTypeAndCreatorFromExtension(const char *aExtension) +{ + // XXX - This should be cut from the API. Would create an evil dependency. + NS_ERROR("NS_ERROR_NOT_IMPLEMENTED"); + return NS_ERROR_NOT_IMPLEMENTED; +} 18. Is returning errors here going to cause stuff (like file downloading) to fail? Maybe LXR for the callers to see, and return NS_OK. I did. Only one of those methods is called by anything and the one caller ignores the error.
Assignee | ||
Comment 26•22 years ago
|
||
The patch is a concatenation of 2 separate patches: 1 from modules/libreg and the other from xpcom/io. The rest was not changed.
Comment 27•22 years ago
|
||
Comment on attachment 89217 [details] [diff] [review] revised to address comments sr=sfraser
Attachment #89217 -
Flags: superreview+
Comment 28•22 years ago
|
||
Comment on attachment 89217 [details] [diff] [review] revised to address comments r=pink
Attachment #89217 -
Flags: review+
Assignee | ||
Comment 29•22 years ago
|
||
Checked into CHIMERA_M1_0_BRANCH. Leaving this open until I get it into the trunk for Mach-0 Seamonkey.
Comment 30•22 years ago
|
||
Today I tried the latest nightly build (Gecko/20020730) of URL http://ftp.mozilla.org/pub/mozilla/nightly/latest/MacMozilla-MachO.dmg.gz and discovered, that the meanwhile applied patches seem to be at least sufficient, to solve the (see bug 86538) problem of the Aqua build of Mozilla to run from an UFS partition. On the other hand the trunk nightly build with the usual name still didn't work at my last trial --- so the difference is probably at least also these patches were applied to above, but not to the regular pre-release build yet? So far I had no crashes and most preferences were used from the 1.0 release running in UNIX manner on top of XDarwin, when I checked them. Well done... My test is insofar conclusive, because I have absolutely no HFS(+) partition around, but only UFS (and via NFS other case sensitive VFS implementations, especially ReiserFS and XFS). Of course I leave it to you active Mozilla developers to close this case as resolved bug, when you think it is right...
Assignee | ||
Comment 31•22 years ago
|
||
> Today I tried the latest nightly build (Gecko/20020730) of URL http://ftp.mozilla.org/pub/mozilla/nightly/latest/MacMozilla-MachO.dmg.gz and discovered, that the meanwhile applied patches seem to be at least sufficient, It's not in that build - the only build with this code is Chimera.
Comment 32•22 years ago
|
||
I was directed to this bug from #mozilla by pinkerton with a request for help with a Mac OS 8/9 issue. However, I am not able to determine from the report what the problem is. Please forgive me if I'm being dense, but please let me know how I can help make this work on Mac OS 8/9. Thanks.
Assignee | ||
Comment 33•22 years ago
|
||
The problem was to have an nsLocalFile impl which supported the nsILocalFileMac interface. For Mach-0, on the trunk anyway, we're using the Unix nsLocalFile impl which does not support nsILocalFileMac and so does not interface well with file pickers, drag and drop, aliases, etc. For OS 8/9, we use the impl in xpcom/io/nsLocalFileMac so I'm not sure what the relevance of this is to OS 8/9.
Comment 34•22 years ago
|
||
i thought the relevance was using the HFS+ api's which would require OS9 for TARGET_CARBON. additionally, would this impact non TARGET_CARBON at all?
Assignee | ||
Comment 35•22 years ago
|
||
It won't affect non TARGET_CARBON at all. This is only for Mach-0. If we want to use this impl on CFM TARGET_CARBON, that's a separate issue.
Assignee | ||
Comment 36•22 years ago
|
||
nsILocalFileMac.idl was changed to add new routines for FSRefs and CFURLs. Since nsLocalFileMac descends from that, it needs those methods. Only supported for TARGET_CARBON and that's noted in the .idl Need review on this portion. The trunk patch is more or less the same as the patch reviewed already and checked into Chimera - along with the fixes made to on the Chimera branch.
Comment 37•22 years ago
|
||
Comment on attachment 100033 [details] [diff] [review] additional diffs for trunk r=sdagley I'll note a minor nit in that I'm not fond of multiple returns like: + if (!parentURL) + return NS_ERROR_FAILURE;
Attachment #100033 -
Flags: review+
Comment 38•22 years ago
|
||
+ FSRef fsRef; + if (::CFURLGetFSRef(aCFURL, &fsRef)) I'd prefer if (::CFURLGetFSRef(aCFURL, &fsRef) != noErr) and a comment about when this fails (file doesn't exist?). + CFURLRef parentURL = ::CFURLCreateCopyDeletingLastPathComponent(NULL, aCFURL); + if (!parentURL) + return NS_ERROR_FAILURE; Should this iterate over all non-root patch components, or is it OK to fail if more than the leaf is missing?
Assignee | ||
Comment 39•22 years ago
|
||
> + if (::CFURLGetFSRef(aCFURL, &fsRef)) > > I'd prefer > > if (::CFURLGetFSRef(aCFURL, &fsRef) != noErr) Then the logic would be wrong. CFURLGetFSRef doesn't return an OSErr. It returns a Boolean for success. I could say if (::CFURLGetFSRef(aCFURL, &fsRef) == PR_TRUE) to make that more clear. > Should this iterate over all non-root patch components, or is it OK to fail if more than the leaf is missing? It probably should but, with HFS, you run into problems like components other than the leaf being > 31 chars, nodes having different encodings (Kanji & MacRoman in one path), etc. Considering the limitations of HFS, I think this limitation is OK.
Comment 40•22 years ago
|
||
> It probably should but, with HFS, you run into problems...
If we only deal with missing leaves, we need to document that in the header that
exposes InitWithCFURL. If InitWithCFURL is called internally from any of the
other init methods that are XP, then we should deal with more than missing leaves.
With that, sr=sfraser
Assignee | ||
Comment 41•22 years ago
|
||
> If we only deal with missing leaves, we need to document that in the header I added a comment to the header. That method isn't called internally. It's not even called anywhere at this point. I added it for the sake of the file picker and drag 'n' drop based on http://developer.apple.com/technotes/tn/tn2022.html.
Attachment #88792 -
Attachment is obsolete: true
Attachment #89068 -
Attachment is obsolete: true
Attachment #89217 -
Attachment is obsolete: true
Attachment #100033 -
Attachment is obsolete: true
Assignee | ||
Comment 42•22 years ago
|
||
* The version of FSCopyObject has the fix from the Chimera branch which allows the object to be renamed when copying. * MoreFilesX is the same. * nsLocalFileOSX has fixes made on the Chimera branch.
Comment 43•22 years ago
|
||
Comment on attachment 100421 [details] [diff] [review] latest full patch[changed files] sr=sfraser, bringing r= forward
Attachment #100421 -
Flags: superreview+
Attachment #100421 -
Flags: review+
Comment 44•22 years ago
|
||
Comment on attachment 100422 [details] [diff] [review] latest full patch[new files] sr=sfraser, bringing r= forward
Attachment #100422 -
Flags: superreview+
Attachment #100422 -
Flags: review+
Assignee | ||
Comment 45•22 years ago
|
||
Checked in.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•