Closed Bug 185681 Opened 22 years ago Closed 22 years ago

Add generic GRE startup/shutdown code to xpcom glue

Categories

(Core :: XPCOM, defect)

x86
Windows XP
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: dougt, Assigned: dougt)

References

Details

Attachments

(4 files, 6 obsolete files)

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.
Attached patch first cut (obsolete) — Splinter Review
Comment on attachment 109444 [details] [diff] [review]
first cut

still going to need some linux love in this patch for parity.
Attached patch linux loving included (obsolete) — Splinter Review
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
Blocks: gre
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.

Attached patch patch 3 (obsolete) — Splinter Review
includes blizzard's suggests.
Attachment #109570 - Attachment is obsolete: true
Attached patch patch 4 (obsolete) — Splinter Review
okay, now the real patch
Attachment #109860 - Attachment is obsolete: true
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 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()
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 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.
Attached patch patch 5 (obsolete) — Splinter Review
> 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?
there was a typo in that path in nsGREDirServiceProvider.cpp line 240 -- it
should read nsCOMPtr not nsCOMPtrq.
Attached patch patch 6 (obsolete) — Splinter Review
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
Attachment #110249 - Flags: superreview?(darin)
Attachment #110249 - Flags: review?(chak)
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+
Attached patch patch 7Splinter Review
includes GRE_Startup/GRE_Shutdown code.
Attachment #110249 - Attachment is obsolete: true
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+
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
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 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
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?  

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
Note: I do not have CVS access, so somebody else will have to check it in.
Attached patch Fix IRIXSplinter Review
should work - haven't tested it.
Comment on attachment 110567 [details] [diff] [review]
Fix "make install"

r=cls
Attachment #110567 - Flags: review+
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.
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
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 ago22 years ago
Resolution: --- → FIXED
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)
Attachment #110249 - Flags: superreview?(darin)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: