Add generic GRE startup/shutdown code to xpcom glue

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
17 years ago
17 years ago

People

(Reporter: dougt, Assigned: dougt)

Tracking

Trunk
x86
Windows XP
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 6 obsolete attachments)

Assignee

Description

17 years ago
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

17 years ago
Posted patch first cut (obsolete) — Splinter Review
Assignee

Comment 2

17 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

17 years ago
Posted 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
Assignee

Updated

17 years ago
Blocks: gre
Assignee

Comment 4

17 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

17 years ago
Posted patch patch 3 (obsolete) — Splinter Review
includes blizzard's suggests.
Attachment #109570 - Attachment is obsolete: true
Assignee

Comment 6

17 years ago
Posted patch patch 4 (obsolete) — Splinter Review
okay, now the real patch
Attachment #109860 - Attachment is obsolete: true
Assignee

Comment 7

17 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

17 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

17 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

17 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

17 years ago
Posted 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?
Assignee

Comment 12

17 years ago
there was a typo in that path in nsGREDirServiceProvider.cpp line 240 -- it
should read nsCOMPtr not nsCOMPtrq.
Assignee

Comment 13

17 years ago
Posted 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
Assignee

Updated

17 years ago
Attachment #110249 - Flags: superreview?(darin)
Attachment #110249 - Flags: review?(chak)

Comment 14

17 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

17 years ago
Posted patch patch 7Splinter Review
includes GRE_Startup/GRE_Shutdown code.
Attachment #110249 - Attachment is obsolete: true

Comment 16

17 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

17 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
Last Resolved: 17 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 → ---
Assignee

Comment 20

17 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
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

17 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

17 years ago
Note: I do not have CVS access, so somebody else will have to check it in.
Assignee

Comment 24

17 years ago
Posted 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.

Comment 27

17 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
Assignee

Comment 28

17 years ago
Marking fixed.  

jst - Docs coming soon.  I will post to the newsgroups when the docs are on
mozilla.org.
Status: REOPENED → RESOLVED
Last Resolved: 17 years ago17 years ago
Resolution: --- → FIXED

Comment 29

17 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

17 years ago
Attachment #110249 - Flags: superreview?(darin)
You need to log in before you can comment on or make changes to this bug.