Closed
Bug 185681
Opened 22 years ago
Closed 22 years ago
Add generic GRE startup/shutdown code to xpcom glue
Categories
(Core :: XPCOM, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: dougt, Assigned: dougt)
References
Details
Attachments
(4 files, 6 obsolete files)
22.71 KB,
patch
|
darin.moz
:
superreview+
|
Details | Diff | Splinter Review |
1.44 KB,
patch
|
Details | Diff | Splinter Review | |
506 bytes,
patch
|
netscape
:
review+
|
Details | Diff | Splinter Review |
1.92 KB,
patch
|
netscape
:
review+
|
Details | Diff | Splinter Review |
in an effort to make it easy for people to use the GRE and XPCOM Glue, we should provide a simply way for application to startup and shutdown the GRE. The default behavior should be to find a GRE that matches the version that the glue was compiled against.
Assignee | ||
Comment 1•22 years ago
|
||
Assignee | ||
Comment 2•22 years ago
|
||
Comment on attachment 109444 [details] [diff] [review] first cut still going to need some linux love in this patch for parity.
Assignee | ||
Comment 3•22 years ago
|
||
I am still not set on exactly what the startup/shutdown API should look like, however, since this stuff is built into the embedding app, it can change without anyone bitching (too much).
Attachment #109444 -
Attachment is obsolete: true
Assignee | ||
Comment 4•22 years ago
|
||
The format of the gre.conf file is as follows: # comment [<version>] GRE_PATH=<path> The <version> is usually the milestone against which the GRE was built. The <path> is the absolute file path where the GRE was installed. For example, on my windows box, I have something like this: # doug't gre installs [1.3a] GRE_PATH=c:\builds\trunk\mozilla\dist\gre It should be clear that this follows the structure of what the windows registry was doing.
Assignee | ||
Comment 5•22 years ago
|
||
includes blizzard's suggests.
Attachment #109570 -
Attachment is obsolete: true
Assignee | ||
Comment 6•22 years ago
|
||
okay, now the real patch
Attachment #109860 -
Attachment is obsolete: true
Assignee | ||
Comment 7•22 years ago
|
||
Comment on attachment 109861 [details] [diff] [review] patch 4 I need to fix up the spacing in the two new files. Assuming that is done, can you two review this patch?
Attachment #109861 -
Flags: superreview?(darin)
Attachment #109861 -
Flags: review?(chak)
Comment 8•22 years ago
|
||
Comment on attachment 109861 [details] [diff] [review] patch 4 Doug...other than the following it looks good: 1. In the call to NS_InitXPCOM2(), inside of GRE_Startup(), we're always passing "nsnull" as the second param. Not really sure if GRE_Startup() should take a "binDirectory" param which will gets passed to NS_InitXPCOM2(). 2. There's some code currently #if 0'd inside of GRE_Startup(). When we eventually migrate embedding clients to use GRE_Startup() don't we need that code to be made available via GRE_Startup()
Assignee | ||
Comment 9•22 years ago
|
||
a) I don't want to pass any additional args to GRE_Startup. This API doesn't need to be frozen. b) I will remove that code. It was there before i realized that i didn't want to do autoreg and appstartup notification withing GRE_Startup. thanks chak.
Comment 10•22 years ago
|
||
Comment on attachment 109861 [details] [diff] [review] patch 4 >Index: Makefile.in >+// CPPSRCS += nsDebug.cpp >+CPPSRCS += nsDebugGlue.cpp remove commented out line. > SDK_HEADERS = \ >+ nsXPCOMGlue.h \ frozen? >Index: nsDebugGlue.cpp license? >Index: nsGREDirServiceProvider.cpp >+#ifdef XP_PC >+#define XPCOM_SEARCH_KEY "PATH" >+#define XPCOM_DLL_PATH_SUFFIX "\\xpcom.dll" >+#define GRE_CONF_PATH_SUFFIX "\\.gre.config" .gre.config is a strange name for a PC. >+ return localFile->QueryInterface(NS_GET_IID(nsIFile), (void**)_retval); nit: how about using CallQueryInterface instead. >+char * >+nsGREDirServiceProvider::GetGREDirectoryPath() >+{ >+ char* greConfHomePath= (char *)malloc(strlen(path) + strlen(GRE_CONF_PATH_SUFFIX) + 1); same as: char* greConfHomePath= (char *)malloc(strlen(path) + sizeof(GRE_CONF_PATH_SUFFIX)); >+ path = PR_GetEnv("MOZ_GRE_CONF"); >+ if (path) { >+ pGreLocation = GetPathFromConfigFile("/etc/gre.conf"); >+ if (pGreLocation) >+ return pGreLocation; >+ } path is not used inside the braces... should it be? >+#if XP_PC >+ strcpy(szKey, "Software\\mozilla.org\\GRE\\" MOZILLA_VERSION); no need to make a copy of the key, right? i.e.: const char *szKey = "Software\\mozilla.org\\GRE\\" MOZILLA_VERSION; >+nsGREDirServiceProvider::GetPathFromConfigFile(const char* filename) >+ PRInt32 versionLen = strlen(MOZILLA_VERSION); sizeof(MOZILLA_VERSION)-1; >+ // kill the line feed if any >+ versionLen = strlen(pGreLocation); >+ versionLen--; >+ >+ if (pGreLocation[versionLen] == '\n') >+ pGreLocation[versionLen] = '\0'; maybe you should create a new variable instead of reusing versionLen, or maybe you should just call versionLen something more generic like "len" as is, i had a little trouble following the code. >+ nsCOMPtr<nsILocalFile> tempLocal; >+ rv = NS_NewNativeLocalFile(nsDependentCString(pGreDir), PR_TRUE, getter_AddRefs(tempLocal)); >+ >+ if (tempLocal) >+ { >+ *aLocalFile = tempLocal; >+ NS_ADDREF(*aLocalFile); >+ rv = NS_OK; >+ } this seems wacky. how about just checking NS_SUCCEEDED(rv) instead. that way you don't have to set rv to NS_OK. >+nsGREDirServiceProvider::GetXPCOMPath() >+{ >+ char* xpcomPath = (char*) malloc(len + strlen(XPCOM_DLL_PATH_SUFFIX) + 1); len + sizeof(XPCOM_DLL_PATH_SUFFIX) >Index: nsXPCOMGlue.cpp >+#if 0 >+#include "nsIObserver.h" > #endif can this be removed? >+ NS_ADDREF( provider ); >+ rv = NS_InitXPCOM2(getter_AddRefs(servMan), nsnull, provider); >+ NS_IF_RELEASE(provider); NS_RELEASE should be fine. >+#if 0 ... >+#endif can the #if 0 code be removed? >Index: nsXPCOMGlue.h >+extern "C" >+nsresult NS_COM GRE_Startup(); >+ >+extern "C" >+nsresult NS_COM GRE_Shutdown(); NS_InitGRE, NS_ShutdownGRE? just seems like GRE_Startup/GRE_Shutdown deviate a lot from the normal function naming conventions. last nit: looks like indentation is not consistent everywhere.
Assignee | ||
Comment 11•22 years ago
|
||
> SDK_HEADERS = \ >+ nsXPCOMGlue.h \ frozen? as close to frozen the glue can come to. >+extern "C" >+nsresult NS_COM GRE_Startup(); >+ >+extern "C" >+nsresult NS_COM GRE_Shutdown(); NS_InitGRE, NS_ShutdownGRE? just seems like GRE_Startup/GRE_Shutdown deviate a lot from the normal function naming conventions. Is this a nit?
Assignee | ||
Comment 12•22 years ago
|
||
there was a typo in that path in nsGREDirServiceProvider.cpp line 240 -- it should read nsCOMPtr not nsCOMPtrq.
Assignee | ||
Comment 13•22 years ago
|
||
Adding support for mac OSX .bundle files. Moved the common defines into nsXPCOMPrivate.
Attachment #109861 -
Attachment is obsolete: true
Attachment #110026 -
Attachment is obsolete: true
Assignee | ||
Updated•22 years ago
|
Attachment #110249 -
Flags: superreview?(darin)
Attachment #110249 -
Flags: review?(chak)
Comment 14•22 years ago
|
||
Comment on attachment 110249 [details] [diff] [review] patch 6 Some indentation/tab issues in the following function: +nsGREDirServiceProvider::GetGREDirectoryPath()
Attachment #110249 -
Flags: review?(chak) → review+
Assignee | ||
Comment 15•22 years ago
|
||
includes GRE_Startup/GRE_Shutdown code.
Attachment #110249 -
Attachment is obsolete: true
Comment 16•22 years ago
|
||
Comment on attachment 110531 [details] [diff] [review] patch 7 >Index: glue/standalone/nsGREDirServiceProvider.cpp >+ char * path = PR_GetEnv("HOME"); >+ if (path) { >+ char* greConfHomePath= (char *)malloc(strlen(path) + sizeof(GRE_CONF_NAME) + sizeof(XPCOM_FILE_PATH_SEPARATOR) + 1); >+ sprintf(greConfHomePath, "%s" XPCOM_FILE_PATH_SEPARATOR GRE_CONF_NAME, path); nit: length of greConfHomePath is 2 bytes longer than necessary. i.e., sizeof(GRE_CONF_NAME) includes the NULL byte. >+#ifdef XP_UNIX >+ pGreLocation = GetPathFromConfigFile("/etc/gre.conf"); nit: should this string be a #define along with the others? >+ strcpy(szKey, "Software\\mozilla.org\\GRE\\" MOZILLA_VERSION); nit: same here. i guess my point is that it might be helpful to others reading this code to see all the strings in one place. take it or leave it ;-) >+ if( (cfg=fopen(filename,"r"))==nsnull) { nit: inconsistent space above and below >+ if (!strncmp (buffer+1, MOZILLA_VERSION, versionLen)) { >+ if (foundHeader && !strncmp (buffer, "GRE_PATH=", 9)) { >+ if(pGreDir) { >+ if (NS_SUCCEEDED(rv)) { nit: more style variations. as for the names GRE_Startup/GRE_Shutdown, it is up to you to decide what you prefer. i was just pointing out the inconsistency. perhaps GRE_ is a good prefix for all things specific to the GRE. sr=darin
Attachment #110531 -
Flags: superreview+
Assignee | ||
Comment 17•22 years ago
|
||
Checking in build/nsXPCOMPrivate.h; /cvsroot/mozilla/xpcom/build/nsXPCOMPrivate.h,v <-- nsXPCOMPrivate.h new revision: 1.5; previous revision: 1.4 done Checking in build/nsXPComInit.cpp; /cvsroot/mozilla/xpcom/build/nsXPComInit.cpp,v <-- nsXPComInit.cpp new revision: 1.173; previous revision: 1.172 done Checking in glue/Makefile.in; /cvsroot/mozilla/xpcom/glue/Makefile.in,v <-- Makefile.in new revision: 1.15; previous revision: 1.14 done Checking in glue/objs.mk; /cvsroot/mozilla/xpcom/glue/objs.mk,v <-- objs.mk new revision: 1.3; previous revision: 1.2 done Checking in glue/standalone/Makefile.in; /cvsroot/mozilla/xpcom/glue/standalone/Makefile.in,v <-- Makefile.in new revision: 1.11; previous revision: 1.10 done RCS file: /cvsroot/mozilla/xpcom/glue/standalone/nsDebugGlue.cpp,v done Checking in glue/standalone/nsDebugGlue.cpp; /cvsroot/mozilla/xpcom/glue/standalone/nsDebugGlue.cpp,v <-- nsDebugGlue.cpp initial revision: 1.1 done RCS file: /cvsroot/mozilla/xpcom/glue/standalone/nsGREDirServiceProvider.cpp,v done Checking in glue/standalone/nsGREDirServiceProvider.cpp; /cvsroot/mozilla/xpcom/glue/standalone/nsGREDirServiceProvider.cpp,v <-- nsGREDirServiceProvider.cpp initial revision: 1.1 done RCS file: /cvsroot/mozilla/xpcom/glue/standalone/nsGREDirServiceProvider.h,v done Checking in glue/standalone/nsGREDirServiceProvider.h; /cvsroot/mozilla/xpcom/glue/standalone/nsGREDirServiceProvider.h,v <-- nsGREDirServiceProvider.h initial revision: 1.1 done Checking in glue/standalone/nsXPCOMGlue.cpp; /cvsroot/mozilla/xpcom/glue/standalone/nsXPCOMGlue.cpp,v <-- nsXPCOMGlue.cpp new revision: 1.10; previous revision: 1.9 done Checking in glue/standalone/nsXPCOMGlue.h; /cvsroot/mozilla/xpcom/glue/standalone/nsXPCOMGlue.h,v <-- nsXPCOMGlue.h new revision: 1.3; previous revision: 1.2 done Thanks.
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Comment 18•22 years ago
|
||
Reopening. Mac classic builds when red as well as the BeOS, OS/2 & Irix builds. At quick glance at nsXPCOMPrivate.h shows that there are no ifdefs for mac classic & beos builds. OS/2 is incorrectly using the window code block (probably due to the use of XP_PC) and Irix has some weird linking error.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 19•22 years ago
|
||
Assignee | ||
Comment 20•22 years ago
|
||
Comment on attachment 110552 [details] [diff] [review] Fix ifdefs for beos & mac classic sounds fine to me. The XP_MAC stuff will have to change for OSX. r=dougt
Comment 21•22 years ago
|
||
jag/scc, the Irix build is failing because of unresolved symbols that come from the string libs by way of linking against libxpcomglue.a. Doug claims that xpcomglue doesn't directly call any string functions and that these references must be coming from the header files. I vaguely recall us running across this before a long time ago. I'm still searching for a magic compiler flag to disable that feature but I can't seem to find one. Can we move those functions which are defined in the header files into their corresponding .cpp files?
Comment 22•22 years ago
|
||
This check-in also broke "make install" on Linux. The problem is that now Makefile.in defines EXPORTS to be $(NULL) while rules.mk tests whether EXPORTS is defined, but never tests whether it is non-NULL. Result: make[3]: Entering `/mnt/spool/nogin/rpmBUILD/mozilla/xpcom/glue/standalone' ../../../config/nsinstall -t -m 644 /var/tmp/mozilla-root/usr/include/mozilla-1.3b/xpcom usage: ../../../config/nsinstall [-C cwd] [-L linkprefix] [-m mode] [-o owner] [-g group] [-DdltR] file [file ...] directory make[3]: *** [install] Exit 2
Comment 23•22 years ago
|
||
Note: I do not have CVS access, so somebody else will have to check it in.
Assignee | ||
Comment 24•22 years ago
|
||
should work - haven't tested it.
Comment 25•22 years ago
|
||
Comment on attachment 110567 [details] [diff] [review] Fix "make install" r=cls
Attachment #110567 -
Flags: review+
Comment 26•22 years ago
|
||
It'd be kinda nice if the tla GRE was explained somewhere in the source code. So far I didn't find it, and while I know what it means now, it's not obvious, IMO.
Comment 27•22 years ago
|
||
Possibly add a comment like the following (to the glue code) in appropriate places? http://lxr.mozilla.org/seamonkey/source/embedding/tests/mfcembed/winEmbedFileLocProvider.cpp#158
Updated•22 years ago
|
Attachment #110568 -
Flags: review+
Assignee | ||
Comment 28•22 years ago
|
||
Marking fixed. jst - Docs coming soon. I will post to the newsgroups when the docs are on mozilla.org.
Status: REOPENED → RESOLVED
Closed: 22 years ago → 22 years ago
Resolution: --- → FIXED
Comment 29•22 years ago
|
||
Comment on attachment 109861 [details] [diff] [review] patch 4 removing request flags on obsolete patch.
Attachment #109861 -
Flags: superreview?(darin)
Attachment #109861 -
Flags: review?(chak)
Updated•22 years ago
|
Attachment #110249 -
Flags: superreview?(darin)
You need to log in
before you can comment on or make changes to this bug.
Description
•