Closed
Bug 171883
Opened 22 years ago
Closed 21 years ago
xp_path.h can be removed
Categories
(SeaMonkey :: Build Config, defect, P5)
SeaMonkey
Build Config
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla1.4beta
People
(Reporter: joerg_brunsmann, Assigned: netscape)
References
Details
Attachments
(2 files, 3 obsolete files)
2.08 KB,
patch
|
bryner
:
review+
alecf
:
superreview+
|
Details | Diff | Splinter Review |
1.78 KB,
patch
|
wtc
:
review+
|
Details | Diff | Splinter Review |
The file http://lxr.mozilla.org/seamonkey/source/include/xp_path.h is included only in two places. First, ut is included in /content/html/content/src/nsFormSubmission.cpp where the macros of xp_path are not used. Also, xp_path.h is included in /modules/oji/src/nsJVMManager.cpp where some macros are used. But these macros can be exchanged by the equivalent definitions from NSPR. After that, xp_path.h can be deleted.
Assignee | ||
Comment 1•22 years ago
|
||
IIRC, the NSPR macros that xp_path.h duplicates are not available from public headers so the file itself cannot be removed if people are still using it.
Reporter | ||
Comment 2•22 years ago
|
||
Adding jkeiser@netscape.com to cc list. jkeiser: is it possible for you to delete this line http://lxr.mozilla.org/seamonkey/source/content/html/content/src/nsFormSubmissio n.cpp#56
Reporter | ||
Comment 3•22 years ago
|
||
There are possibilities: 1. xp_path.h defines system dependent path and directory separator. The directory separator is available here http://lxr.mozilla.org/seamonkey/source/nsprpub/pr/include/prsystem.h#51 The function NSPR_API(char) PR_GetPathSeparator(void) is missing, but could be added. 2. Move the SEPARATOR definitions from xp_path.h to http://lxr.mozilla.org/seamonkey/source/modules/oji/src/nsJVMManager.cpp . That this is no problem, can be verified by searching lxr.mozilla.org for SEPARATOR to see that other .cpp-files define their own SEPARATOR characters.
Assignee | ||
Comment 4•22 years ago
|
||
1) You're talking about changing the NSPR API to expose that extra function. You would have to get buy-in from the NSPR owners for that. 2) This is the easiest solution but not the correct one. I see _SEPARATOR being defined in a standalone programs (nsinstall) and standalone libraries (nspr, xpcom & js). OJI is neither; it should be using the definitions from one of the previously mentioned libraries since it depends upon them all.
Comment 5•22 years ago
|
||
It seems fine to add a new PR_GetPathSeparator function. I am worried that that function is not that useful because the directory and path separators NSPR uses on the Mac are Unix-centric and may not be what you want.
Comment 6•22 years ago
|
||
It is most likely possible to remove that #include, yes.
Reporter | ||
Comment 7•22 years ago
|
||
Ok, there's consensus for a new nspr function. If I understand correctly the definitions found in directory nsprpub/pr/include/ for the files _beos.h, _os2.h, _macos.h and _winnt.h match those unix-centric mac definitions found in xp_path.h jkeiser: can you or someone else please remove the #include. Thanks.
Comment 8•22 years ago
|
||
Such a new function would be good too, IIRC we do funky platform-specific path stuff in nsLocalFile. As to removing #include xp_path.h, wouldn't it be simpler if whoever removes the file also removes the include? I don't really see the need to go through two separate write/build/r=/sr=/commit cycles for what is essentially a single patch. If I'm missing some crucial element here, let me know.
Reporter | ||
Comment 9•22 years ago
|
||
The semantic of path and directory separator differ in xp_path.h and http://lxr.mozilla.org/seamonkey/source/xpcom/io/nsLocalFileCommon.cpp#125 . Furthermore, xp_path.h defines both PR_PATH_[DIRECTORY|SEPARATOR]_STR and PATH_[DIRECTORY|SEPARATOR]_STR for XP_MAC. My understanding is as follows: a directory separator separates directory names in absolute file names (in java: file.separator). A path separator separates several directories (in java: path.separator) in a complete path specification. The names DIRECTORY_SEPARATOR and PATH_SEPARATOR may be disleading. FILE_SEPARATOR and PATH_SEPARATOR might be a better solution. What is your perception? Also there's a difference in the definition in MAXPATHLEN for XP_MAC: it's 512 in xp_path.h and 31 in nsLocalFileCommon.cpp. Perhaps an additional function NSPR_API(int) PR_GetMaxPathLen(void) makes sense. Being pragmatic I suggest to leave out the definitions in nsLocalFileCommon.cpp for now and open another bug for merging all definitions of SEPARATORs and PATHLENs and just pay attention to get rid of xp_path.h
Reporter | ||
Comment 10•22 years ago
|
||
Goal: get rid of xp_path.h Barrier: /modules/oji/src/nsJVMManager.cpp Pragmatic solution: Upcoming patch
Reporter | ||
Comment 11•22 years ago
|
||
Assignee | ||
Updated•22 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee | ||
Comment 12•22 years ago
|
||
Comment on attachment 101812 [details] [diff] [review] reduce usage of SEPARATORs to minimum Why are you commenting out the GetClasspathAdditions function?
Attachment #101812 -
Flags: needs-work+
Reporter | ||
Comment 13•22 years ago
|
||
I commented out the GetClasspathAdditions function because it's not referenced. And since this function uses the PR_PATH_SEPARATOR preprocessor macro, deleting this function would reduce the usage of the macros defined in xp_path.h to only one macro (PR_DIRECTORY_SEPARATOR). If removing this function is not wanted, one has to define the macro PR_PATH_SEPARATOR as well in nsJVMManager.cpp. Both ways are ok, since one can still remove xp_path.h. Someone with deeper knowledge of nsJVMManager.cpp has to decide what to do, since it's the only file which uses xp_path.h. This person might also detect whether the nsJVMManager.cpp code will work on a mac given the unix-style SEPARATOR definitions from xp_path.h or if some conversions (see http://lxr.mozilla.org/seamonkey/source/modules/oji/src/nsJVMManager.cpp#103) should apply, like NSPR code does. BTW: The '#ifdef 0' in the patch should read '#if 0' .
Comment 14•22 years ago
|
||
Checked with the Java plugin group to see if xp_path.h was referenced in Java plugin code. The answer is No.
Assignee | ||
Comment 15•22 years ago
|
||
GetClasspathAdditions may not be directly referenced by any Mozilla code but since it *is* part of the exported interface, it may be used by 3rd party modules (in particular, the java plugin_. Removing an interface function just because it references an otherwise inconsequential define seems like an extremely bad idea. We don't need to get rid of xp_path.h that badly.
Severity: normal → trivial
Priority: -- → P5
Target Milestone: --- → Future
Comment 16•22 years ago
|
||
Currently, xp_path.h is used in oji module (some separators defined in xp_path.h are used in nsJVMManager.cpp). OJI is being re-designed at the moment, so untill the re-design is done,I don't think it would be a good idea to make any changes to make removing xp_path.h possible. Ccing Patrick to see if any implication on Mac's Java plugin as well.
Comment 17•22 years ago
|
||
also see bug 173634
Comment 18•22 years ago
|
||
Can we pull this in to 1.3? /be
Assignee | ||
Comment 19•21 years ago
|
||
Assignee | ||
Updated•21 years ago
|
Attachment #101812 -
Attachment is obsolete: true
Assignee | ||
Comment 20•21 years ago
|
||
Assignee | ||
Updated•21 years ago
|
Attachment #119038 -
Flags: review?(wtc)
Assignee | ||
Updated•21 years ago
|
Attachment #119039 -
Flags: superreview?(alecf)
Attachment #119039 -
Flags: review?(bryner)
Updated•21 years ago
|
Attachment #119039 -
Flags: review?(bryner) → review+
Comment 21•21 years ago
|
||
Comment on attachment 119039 [details] [diff] [review] bye xp_path.h sr=alecf
Attachment #119039 -
Flags: superreview?(alecf) → superreview+
Assignee | ||
Updated•21 years ago
|
Status: NEW → ASSIGNED
OS: Windows NT → All
Hardware: PC → All
Target Milestone: Future → mozilla1.4beta
Assignee | ||
Comment 22•21 years ago
|
||
Attachment #119038 -
Attachment is obsolete: true
Assignee | ||
Updated•21 years ago
|
Attachment #119038 -
Flags: review?(wtc)
Assignee | ||
Updated•21 years ago
|
Attachment #119373 -
Flags: review?(wtc)
Comment 23•21 years ago
|
||
Comment on attachment 119373 [details] [diff] [review] Update .def file too >+/* >+** Get the host' path separator. >+** Pathnames are then assumed to be of the form: >+** [<sep><root_component><sep>]*(<component><sep>)<leaf_name> >+*/ >+ >+NSPR_API(char) PR_GetPathSeparator(void); The comment was copied from PR_GetDirectorySeparator. It needs to be rewritten for PR_GetPathSeparator. Everything else is good.
Attachment #119373 -
Flags: review?(wtc) → review-
Comment 25•21 years ago
|
||
Comment on attachment 119381 [details] [diff] [review] Corrected description r=wtc.
Attachment #119381 -
Flags: review+
Assignee | ||
Comment 26•21 years ago
|
||
The patches have been checked in on the mozilla trunk, nspr client branch & nspr trunk.
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Updated•20 years ago
|
Product: Browser → Seamonkey
You need to log in
before you can comment on or make changes to this bug.
Description
•