Closed
Bug 241384
Opened 20 years ago
Closed 20 years ago
nsProfileDirServiceProvider should be GRE friendly
Categories
(Core Graveyard :: Embedding: GRE Core, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: mpgritti, Assigned: mpgritti)
References
Details
Attachments
(2 files, 9 obsolete files)
24.32 KB,
patch
|
darin.moz
:
superreview-
|
Details | Diff | Splinter Review |
19.96 KB,
patch
|
darin.moz
:
superreview+
|
Details | Diff | Splinter Review |
Problems are usage of nsString and nsIAtom. They both seem easily fixable.
Assignee | ||
Comment 1•20 years ago
|
||
Assignee | ||
Comment 2•20 years ago
|
||
Attachment #146857 -
Attachment is obsolete: true
Assignee | ||
Updated•20 years ago
|
Attachment #146866 -
Flags: review?(ccarlen)
Comment 3•20 years ago
|
||
I'm fine with replacing nsIAtom usage with strcmp, but the replacement for NS_NAMED_LITERAL_STRING seems a bit too ugly. Since creating Unicode string literals seems to be such a basic nescesity, can't that be added to nsEmbedString?
Comment 4•20 years ago
|
||
why is this code, which is part of core gecko, using nsEmbedString? that seems very wrong indeed. what am i missing?
Comment 5•20 years ago
|
||
ok, the answer to my question appears to be that embedders take this static lib and use it too. that's unfortunate since we also link it into profile.dll. in that case, there might be symbol conflicts between the nsAC?String defined in nsStringAPI.h and the nsAC?String defined in nsAC?String.h ... my concern is that other parts of profile.dll uses the internal definition of nsAC?String. i'd recommend using #ifdefs to compile profdirserviceprovider conditionally using either nsEmbedString or nsString. then compile it twice, and link the nsString version into profile.dll and let the other version be used by embedders.
Assignee | ||
Updated•20 years ago
|
Attachment #146866 -
Flags: review?(ccarlen)
Assignee | ||
Comment 6•20 years ago
|
||
Finally I dont think this is the right way to go. I dont think we should bind embedding to the old toolkit profile stuff. For one, the per user compreg.dat would be useful to embedding too.
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → WONTFIX
Assignee | ||
Comment 7•20 years ago
|
||
Reopening per discussion with brendan and bsmedberg on bug 205425. Create a standalone version of the profile code seem to be preferable over duplicating the code right away
Assignee | ||
Comment 8•20 years ago
|
||
I tried to avoid NS_NAMED_LITERAL_STRING mess where possible.
Attachment #146866 -
Attachment is obsolete: true
Assignee | ||
Updated•20 years ago
|
Attachment #147159 -
Flags: review?(ccarlen)
Comment 9•20 years ago
|
||
Comment on attachment 147159 [details] [diff] [review] create a standalone version of the static library >Index: dirserviceprovider/src/Makefile.in >=================================================================== > VPATH = @srcdir@ > > include $(DEPTH)/config/autoconf.mk >+include $(srcdir)/objs.mk objs.mk isn't in the diff, but I guess it just sets up MODULES_PROFILEDIRSERVICE_SRC_LCSRCS? > > MODULE = profdirserviceprovider > LIBRARY_NAME = profdirserviceprovider_s >@@ -53,11 +54,7 @@ > $(NULL) > endif > >-CPPSRCS = nsProfileDirServiceProvider.cpp >- >-ifdef MOZ_PROFILELOCKING >-CPPSRCS += nsProfileLock.cpp >-endif >+CPPSRCS = $(MODULES_PROFILEDIRSERVICE_SRC_LCSRCS) > > # we don't want the shared lib > FORCE_STATIC_LIB = 1 >Index: dirserviceprovider/src/nsProfileDirServiceProvider.cpp >=================================================================== >RCS file: /cvsroot/mozilla/profile/dirserviceprovider/src/nsProfileDirServiceProvider.cpp,v >retrieving revision 1.14 >diff -u -r1.14 nsProfileDirServiceProvider.cpp >--- dirserviceprovider/src/nsProfileDirServiceProvider.cpp 27 Feb 2004 20:11:01 -0000 1.14 >+++ dirserviceprovider/src/nsProfileDirServiceProvider.cpp 27 Apr 2004 18:14:38 -0000 >@@ -38,8 +38,6 @@ > > #include "nsProfileDirServiceProvider.h" > #include "nsProfileLock.h" >-#include "nsIAtom.h" >-#include "nsStaticAtom.h" > #include "nsILocalFile.h" > #include "nsDirectoryServiceDefs.h" > #include "nsAppDirectoryServiceDefs.h" >@@ -54,6 +52,21 @@ > > // File Name Defines > >+#ifdef STANDALONE_PROFILEDIRSERVICE >+#define PREFS_FILE_50_NAME nsEmbedCString("prefs.js") >+#define USER_CHROME_DIR_50_NAME nsEmbedCString("chrome") >+#define LOCAL_STORE_FILE_50_NAME nsEmbedCString("localstore.rdf") >+#define HISTORY_FILE_50_NAME nsEmbedCString("history.dat") >+#define PANELS_FILE_50_NAME nsEmbedCString("panels.rdf") >+#define MIME_TYPES_FILE_50_NAME nsEmbedCString("mimeTypes.rdf") >+#define BOOKMARKS_FILE_50_NAME nsEmbedCString("bookmarks.html") >+#define DOWNLOADS_FILE_50_NAME nsEmbedCString("downloads.rdf") >+#define SEARCH_FILE_50_NAME nsEmbedCString("search.rdf" ) >+#define MAIL_DIR_50_NAME nsEmbedCString("Mail") >+#define IMAP_MAIL_DIR_50_NAME nsEmbedCString("ImapMail") >+#define NEWS_DIR_50_NAME nsEmbedCString("News") >+#define MSG_FOLDER_CACHE_DIR_50_NAME nsEmbedCString("panacea.dat") >+#else > #define PREFS_FILE_50_NAME NS_LITERAL_CSTRING("prefs.js") > #define USER_CHROME_DIR_50_NAME NS_LITERAL_CSTRING("chrome") > #define LOCAL_STORE_FILE_50_NAME NS_LITERAL_CSTRING("localstore.rdf") >@@ -67,25 +80,7 @@ > #define IMAP_MAIL_DIR_50_NAME NS_LITERAL_CSTRING("ImapMail") > #define NEWS_DIR_50_NAME NS_LITERAL_CSTRING("News") > #define MSG_FOLDER_CACHE_DIR_50_NAME NS_LITERAL_CSTRING("panacea.dat") Some typedefs could save a number of #ifdefs throughout this code. #if STANDALONE_PROFILEDIRSERVICE typedef nsEmbedString nsUniversalString; #else typedef nsString nsUniversalString; #endif If you can come up with a better name than nsUniversalString ;-) >Index: dirserviceprovider/src/nsProfileLock.cpp >=================================================================== >-nsresult nsProfileLock::Lock(nsILocalFile* aFile) >-{ >-#if defined (XP_MACOSX) >- NS_NAMED_LITERAL_STRING(LOCKFILE_NAME, ".parentlock"); >- NS_NAMED_LITERAL_STRING(OLD_LOCKFILE_NAME, "parent.lock"); >-#elif defined (XP_UNIX) >- NS_NAMED_LITERAL_STRING(OLD_LOCKFILE_NAME, "lock"); >- NS_NAMED_LITERAL_STRING(LOCKFILE_NAME, ".parentlock"); >+#ifdef STANDALONE_PROFILEDIRSERVICE >+ >+#if defined (XP_UNIX) >+ #define OLD_LOCKFILE_NAME nsEmbedCString("lock") >+ #define LOCKFILE_NAME nsEmbedCString(".parentlock") > #else >- NS_NAMED_LITERAL_STRING(LOCKFILE_NAME, "parent.lock"); >+ #define LOCKFILE_NAME nsEmbedCString("parent.lock") > #endif > >+#else >+ >+#if defined (XP_UNIX) >+ #define OLD_LOCKFILE_NAME NS_LITERAL_CSTRING("lock") >+ #define LOCKFILE_NAME NS_LITERAL_CSTRING(".parentlock") >+#else >+ #define LOCKFILE_NAME NS_LITERAL_CSTRING("parent.lock") >+#endif >+ >+#endif This changes the definition of OLD_LOCKFILE_NAME for XP_MACOSX. With your change, OSX gets the UNIX def, which is different. >--- /dev/null 2004-02-23 22:02:56.000000000 +0100 >+++ dirserviceprovider/standalone/Makefile.in 2004-04-27 12:46:14.000000000 +0200 >@@ -0,0 +1,69 @@ >+# ***** BEGIN LICENSE BLOCK ***** <snip> >+# The Original Code is mozilla.org code. >+# >+# The Initial Developer of the Original Code is >+# Netscape Communications Corporation. Netscape? I don't think so ;-) >+# Portions created by the Initial Developer are Copyright (C) 2002 The date is wrong. Check this on all added files. Minusing for the >-#if defined (XP_MACOSX) and the license problems. See if you can clean things up with some #defines and typedefs but I won't require that.
Attachment #147159 -
Flags: review?(ccarlen) → review-
Assignee | ||
Comment 10•20 years ago
|
||
>This changes the definition of OLD_LOCKFILE_NAME for XP_MACOSX. With your
>change, OSX gets the UNIX def, which is different.
Yeah, but my code is using OLD_LOCKFILE_NAME only for UNIX. We need to special
case that code already for the conversion in macos code...
See:
+#ifdef STANDALONE_PROFILEDIRSERVICE
+ nsEmbedString oldLockFileName;
+ NS_CStringToUTF16(nsEmbedCString("parent.lock"),
+ NS_CSTRING_ENCODING_ASCII, oldLockFileName);
+#else
+ NS_NAMED_LITERAL_STRING(oldLockFileName, "parent.lock");
+#endif
The only alternative I can think of is a monster like this:
#ifdef STANDALONE_PROFILEDIRSERVICE
#if defined (XP_MACOSX)
NS_CStringToUTF16(nsEmbedCString(".parentlock"),
NS_CSTRING_ENCODING_ASCII, LOCKFILE_NAME);
NS_CStringToUTF16(nsEmbedCString("parent.lock"),
NS_CSTRING_ENCODING_ASCII, OLD_LOCKFILE_NAME);
#elif defined (XP_UNIX)
NS_CStringToUTF16(nsEmbedCString(".parentlock"),
NS_CSTRING_ENCODING_ASCII, LOCKFILE_NAME);
NS_CStringToUTF16(nsEmbedCString("lock"),
NS_CSTRING_ENCODING_ASCII, OLD_LOCKFILE_NAME);
#else
NS_CStringToUTF16(nsEmbedCString("parent.lock"),
NS_CSTRING_ENCODING_ASCII, LOCKFILE_NAME);
#endif
#else
#if defined (XP_MACOSX)
NS_NAMED_LITERAL_STRING(LOCKFILE_NAME, ".parentlock");
NS_NAMED_LITERAL_STRING(OLD_LOCKFILE_NAME, "parent.lock");
#elif defined (XP_UNIX)
NS_NAMED_LITERAL_STRING(OLD_LOCKFILE_NAME, "lock");
NS_NAMED_LITERAL_STRING(LOCKFILE_NAME, ".parentlock");
#else
NS_NAMED_LITERAL_STRING(LOCKFILE_NAME, "parent.lock");
#endif
#endif
Do you want me to make it this way ? Or do you see a better way ?
Assignee | ||
Comment 11•20 years ago
|
||
Attachment #147159 -
Attachment is obsolete: true
Assignee | ||
Comment 12•20 years ago
|
||
Comment on attachment 147402 [details] [diff] [review] fix license problems and add obj.mk I did not make changes to the profile lock file stuff, see my previous comment. The code works correctly I think, though if you think it's nicer using the big block of defines or yet another way let me know and I'll happily fix it.
Attachment #147402 -
Flags: review?(ccarlen)
Assignee | ||
Comment 13•20 years ago
|
||
Assignee | ||
Comment 14•20 years ago
|
||
Comment on attachment 147648 [details] [diff] [review] Alternative. More verbose but maybe more clear. Feel free to review the one you like better. I dont have a strong preference. No one of them is particularly nice but it's either this or duplicating profile code :/ (bug 205425)
Attachment #147648 -
Flags: review?(ccarlen)
Assignee | ||
Comment 15•20 years ago
|
||
I forgot to mention that the only actual difference between the two patches is in nsProfileLock::Lock. Attachment 147402 [details] [diff] uses CString when possible (so that we dont need conversion) and special case mac old lock. Attachment 147648 [details] [diff] just port the string definition at the top to embed string for the standalone case.
Assignee | ||
Comment 16•20 years ago
|
||
Assignee | ||
Updated•20 years ago
|
Attachment #147653 -
Flags: review?(ccarlen)
Assignee | ||
Comment 17•20 years ago
|
||
Attachment #147653 -
Attachment is obsolete: true
Assignee | ||
Updated•20 years ago
|
Attachment #147653 -
Flags: review?(ccarlen)
Assignee | ||
Comment 18•20 years ago
|
||
Comment on attachment 147656 [details] [diff] [review] Third alternative (update) Hopefully the last one, sorry for the spam. FWIW this is my favourite.
Attachment #147656 -
Flags: review?(ccarlen)
Comment 19•20 years ago
|
||
Sorry for the delay, was away for a few days. Looks better. Two things: +#ifdef STANDALONE_PROFILEDIRSERVICE + const char *lockPath = nsEmbedCString(lockFilePath).get(); +#else + const char *lockPath = PromiseFlatCString(lockFilePath).get(); +#endif + + mLockFileDesc = open(lockPath, O_WRONLY | O_CREAT | O_TRUNC, 0666); Take a look at http://lxr.mozilla.org/seamonkey/source/xpcom/string/public/nsTPromiseFlatString.h#60 According to that, in the !STANDALONE_PROFILEDIRSERVICE case, lockPath is a dangling pointer. There are two instances of this in the patch. This is good: +#ifdef STANDALONE_PROFILEDIRSERVICE + #define NAMED_LITERAL_STRING(stringname, filename) \ + nsEmbedString stringname; \ + NS_CStringToUTF16(nsEmbedCString(filename), \ + NS_CSTRING_ENCODING_ASCII, stringname); +#else + #define NAMED_LITERAL_STRING NS_NAMED_LITERAL_STRING +#endif I wish there was a corresponding CSTRING version and and was used in nsProfileDirServiceProvider.cpp. Check out the dangling pointer issue (Darin, is that comment still true?) and r=ccarlen.
Assignee | ||
Comment 20•20 years ago
|
||
> +#ifdef STANDALONE_PROFILEDIRSERVICE > + const char *lockPath = nsEmbedCString(lockFilePath).get(); > +#else > + const char *lockPath = PromiseFlatCString(lockFilePath).get(); > +#endif > + > + mLockFileDesc = open(lockPath, O_WRONLY | O_CREAT | O_TRUNC, 0666); > > Take a look at > http://lxr.mozilla.org/seamonkey/source/xpcom/string/public/nsTPromiseFlatString.h#60 > According to that, in the !STANDALONE_PROFILEDIRSERVICE case, lockPath is a > dangling pointer. There are two instances of this in the patch. Ouch, good catch. In the other case I've just put back the old code (but defined). For this I had to add an intermediate const char. I'm going to ask sr to darin so he can check this is ok. /me needs a clue about strings > I wish there was a corresponding CSTRING version and and was used in > nsProfileDirServiceProvider.cpp. Done.
Assignee | ||
Updated•20 years ago
|
Attachment #147648 -
Flags: review?(ccarlen)
Assignee | ||
Updated•20 years ago
|
Attachment #147402 -
Attachment is obsolete: true
Attachment #147402 -
Flags: review?(ccarlen)
Assignee | ||
Comment 21•20 years ago
|
||
Attachment #147648 -
Attachment is obsolete: true
Attachment #147656 -
Attachment is obsolete: true
Assignee | ||
Comment 22•20 years ago
|
||
Comment on attachment 147817 [details] [diff] [review] Fix dangling pointer Darin, could you please also check that the changes at nsProfileLock.cpp @@ -217,9 +221,16 @@ are correct ?
Attachment #147817 -
Flags: superreview?(darin)
Comment 23•20 years ago
|
||
+ const char *lockPath = nsEmbedCString(lockFilePath).get(); this looks like a dangling pointer as well (the nsEmbedCString object will be destroyed after this line)
Assignee | ||
Updated•20 years ago
|
Attachment #147817 -
Flags: superreview?(darin)
Assignee | ||
Comment 24•20 years ago
|
||
Attachment #147817 -
Attachment is obsolete: true
Assignee | ||
Updated•20 years ago
|
Attachment #147920 -
Flags: superreview?(darin)
Comment 25•20 years ago
|
||
Comment on attachment 147920 [details] [diff] [review] Really fix all dangling pointers please do something like typedef nsEmbedString nsString; in the STANDALONE_PROFILEDIRSERVICE case. that way you avoid extra #ifdef's. sound good? same goes for nsCAutoString I think. also, it looks like LITERAL_CSTRING doesn't need to take an argument in the nsEmbedCString case. ugh.. nsEmbedCString should have an IsEmpty() method. bad darin! use the EmptyString() function instead of nsString(). And maybe "#define EmptyString nsEmbedString" in the STANDALONE_PROFILEDIRSERVICE case. the fewer #ifdefs scattered throughout the code the beter, right? ;-) i would also recommend making PromiseFlatCString equate to nsEmbedCString ctor, and typedef nsPromiseFlatCString to nsEmbedCString. again, this eliminates more #ifdefs. seems like it would be good to put all of these #defines and typedefs in one of the profile header files. i have no problem with you #define'ing NS_NAMED_LITERAL_STRING in terms of the embedding string API. there's no need to invent custom macros. just use the standard macro names so you don't have to touch the body of the code.
Attachment #147920 -
Flags: superreview?(darin) → superreview-
Assignee | ||
Comment 26•20 years ago
|
||
> typedef nsEmbedString nsString;
>
> in the STANDALONE_PROFILEDIRSERVICE case. that way you avoid extra #ifdef's.
> sound good? same goes for nsCAutoString I think.
Frankly I dont like that approach. It hides the fact that you can use only the
nsString/nsEmbedString api intersection. If someone tries to modify
nsProfileLock.cpp using nsString only api I think he will get really confused.
IHMO it's much better to have some additional ugly defines than obscure it this way.
Anyway, it's up to you. I'm going to attach a patch that does it in the way you
suggested. I had to add nsProfileCore.h. nsProfileLock.h doesnt sounds like the
right place to put the defines to me. nsProfileDirServiceProvider.h is public
and the typedefs can cause conflicts: for example I was getting a conflict
between nsColor.h and nsProfileDirServiceProvider.h definition of nsString in
gtkmozembed.
Assignee | ||
Comment 27•20 years ago
|
||
Assignee | ||
Updated•20 years ago
|
Attachment #147974 -
Flags: superreview?(darin)
Comment 28•20 years ago
|
||
Comment on attachment 147974 [details] [diff] [review] Different approach suggested by darin I understand your concerns about changing the meaning of well-known string types, but I think that can be solved by ample documentation. Even a README.txt file might be good :-) At the very least, I think you should comment what's going on in nsProfileCore.h. Also for the record, we do a lot of this typedef'ing and #define'ing sort of thing in modules/libjar so that we can support two builds of that module: 1 for the core gecko and 1 for use by the installer (which does not has xpcom support). I'd also like to get feedback/MOA= from Conrad on this change.
Comment 29•20 years ago
|
||
Comment on attachment 147974 [details] [diff] [review] Different approach suggested by darin I like this approach. I asked for typedefs a few reviews ago, so I'm happy. I think it's OK to hide this stuff behind #defines and typedefs because the STANDALONE_PROFILEDIRSERVICE case is part of the default build. If somebody uses an nsString method not supported by nsEmbedString, they will have no choice but to know about it. But, please file a bug about the lack of IsEmpty() on nsEmbedString. Please rename nsProfileCore.h -> nsProfileStringTypes.h, put a README in that file, and r=ccarlen.
Attachment #147974 -
Flags: review+
Assignee | ||
Updated•20 years ago
|
Attachment #147974 -
Flags: superreview?(darin)
Assignee | ||
Comment 30•20 years ago
|
||
Attachment #147974 -
Attachment is obsolete: true
Assignee | ||
Updated•20 years ago
|
Attachment #148131 -
Flags: superreview?(darin)
Updated•20 years ago
|
Attachment #148131 -
Flags: superreview?(darin) → superreview+
Assignee | ||
Comment 31•20 years ago
|
||
Damn, there is a collision between this and 232691 :(
Comment 32•20 years ago
|
||
there don'T seem to be any conflicts here - checked in
Updated•20 years ago
|
Attachment #147656 -
Flags: review?(ccarlen)
Assignee | ||
Updated•20 years ago
|
Status: REOPENED → RESOLVED
Closed: 20 years ago → 20 years ago
Resolution: --- → FIXED
Updated•8 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•