Patch forthcoming, to clean up the code duplication between mfcembed and the GRE standalone glue. 1) Unify the "GRE-finding" code in the XPCOM standalone glue and 1a) provide a more flexible interface than GRE_Startup for embedders who need to provide a custom directory service 2) basic cleanup of the XPCOM glue for 2a) file path correctness (use : instead of ; for *nix environment separators) 2b) *nix/win discrepancies (don't honor the HOME envvar on win32, but do look in Current User\Software\mozilla.org\GRE 3) modify the MFCembed code so that it links against the xpcom standalone glue properly, and *not* against xpcom.dll or other mozilla libs

Comment on attachment 134295 [details] [diff] [review] make MFCEmbed use the standalone glue Note: this patch was produced against the PACKAGING_20030906_BRANCH and incorporates the patch in bug 220091. Does Adam Lock read bugmail and do reviews, etc?

Comment on attachment 134295 [details] [diff] [review] make MFCEmbed use the standalone glue why are you getting rid of the gre directory service provider file. If nothing else, it is a logical seperation between typical glue functionality and file specific functionality. Also, deleting this file removes all of the cvs history. Because of this, the diff is quite large. Why are you removing NS_COM from the exported functions? You probabably want to remove this fprintf: BOOL CMfcEmbedApp::InitInstance() { + fprintf(stderr, "CMfcEmbedApp::InitInstance\n"); I do not see the purpose o expose GRE_AddGREToEnvironment to embedders. When would you ever not want to call tihs after a XPCOMGlueStartup?

I will go back and keep the directoryservice file... it will be pretty small, because most of the actual functionality is in exported functions instead of class functions. > Why are you removing NS_COM from the exported functions? NS_COM is a dynamic-linking macro. Becase these functions are statically linked, there is no need for NS_COM. > I do not see the purpose o expose GRE_AddGREToEnvironment to embedders. When > would you ever not want to call tihs after a XPCOMGlueStartup? Oh, that's a good idea. I was calling it in GRE_Startup, but that is skipped by MFCembed, so I exported it separately and called it directly from mfcembed InitInstance. But if I put it in xpcomgluestartup I don't need to export it. --BDS

look at how NS_COM is defined -- it is nothing when XPCOM_GLUE is defined.

Yes I do read bugzilla, but only when I have the time. So resubmit when you address the other points and I will review the embedding side of things.

since NS_COM is a dynamic linking macro, I really don't think it belongs herel it certainly confused me at first. All other comments fixed.

>Also, deleting this file removes all of the cvs history. cvs history for deleted files is preserved (they can be found in the attic directory), presuming that's what you were referring to... (of course, someone needs to know to look there to find them, but you can always put something in your ci comment about it)

Comment on attachment 134295 [details] [diff] [review] make MFCEmbed use the standalone glue From an the embedding side of things, I like this patch. Doug might comment on the other side, but r=adamlock for mfcembed

i do not understand why you are moving any of the functions from nsGREDirServiceProvider. Can you leave them alone and just rename them?

Doug, I moved the functions to match the location of the header declarations. I really don't care, it's your call. --BDS

lets keep it where it is. easier to review, saves the cvs blame, and no real reason to move it.

Comment on attachment 134620 [details] [diff] [review] rearranged function definitions to preserse CVS blame why aren't these in a header file? +PRBool GRE_GetCurrentProcessDirectory(char* buffer); +PRBool GRE_GetPathFromConfigDir(const char* dirname, char* buffer); +PRBool GRE_GetPathFromConfigFile(const char* dirname, char* buffer); Are we suppose to rlease a reference from CFBundleGetMainBundle? I would guess yes, but I am not sure. I think these changes are okay. How was this tested? Could you rebuild (a new tree or clobber) with the following define set: GRE_DIRS_ONLY=1 Also, when I build normally (with this define set), I get this error: mozilla\xpcom\glue\standalone\nsXPCOMGlue.cpp(315) : error C2065: 'SetCurrentDirectory' : undeclared identifier

Doug, when I moved the bulk of the code back into nsGREDirServiceProvider.cpp I removed an extra #include <windows.h> from nsXPCOMGlue.cpp which I have now added back. I built with GRE_DIRS_ONLY and everything built correctly... is that configuration testable in some meaningful way, or did you just want to make sure it built correctly? --BDS

If I remember correctly, if you cleanly built, dist/bin will not inlude any of the GRE pieces. You can set up your env var to poing to dist/gre. Please verify that works.

The build system doesn't copy the resource://gre/res files into dist/gre, so I had to copy them manually. Other than that, this patch builds and runs correctly using GRE_DIRS_ONLY. --BDS

Fixed on trunk. This caused some abnormally high codesize increase to appear on the tinderboxen, part of which is real but most of which is fake: 1) since we link the string libs into mfcembed, we are paying ~40k for those. 2) codesighs is erroneously reporting BSS data segments as real codesize numbers. These segments are uninitialized data that does not occur in the binary image, and is created by the linker when the dll/so is loaded. Since the glue is linked into many of our utilities like regxpcom, the 4k of static uninitialized strings in this patch is multiplied out to some ridiculous number. --BDS

this seems to have broken AIX: http://tinderbox.mozilla.org/SeaMonkey-Ports/ "/usr/include/sys/param.h", line 91.9: 1540-0848 (S) The macro name "MAXPATHLEN" is already defined with a different definition. "/home/tbox/builds/tinderbox/AIX_5.1_Clobber/mozilla/xpcom/glue/standalone/nsGREDirServiceProvider.h", line 47.9: 1540-0425 (I) "MAXPATHLEN" is defined on line 47 of "/home/tbox/builds/tinderbox/AIX_5.1_Clobber/mozilla/xpcom/glue/standalone/nsGREDirServiceProvider.cpp". gmake[5]: *** [nsGREDirServiceProvider.o] Error 1

...and beos, too: /boot/develop/headers/posix/sys/param.h:7: warning: `MAXPATHLEN' redefined /boot/home/tinderbox/BeOS_5.0_Depend/mozilla/xpcom/glue/standalone/nsGREDirServiceProvider.h:47: warning: this is the location of the previous definition /boot/home/tinderbox/BeOS_5.0_Depend/mozilla/xpcom/glue/standalone/nsGREDirServiceProvider.cpp: In function `PRBool GRE_GetCurrentProcessDirectory(char *)': /boot/home/tinderbox/BeOS_5.0_Depend/mozilla/xpcom/glue/standalone/nsGREDirServiceProvider.cpp:248: `buf' undeclared (first use this function) /boot/home/tinderbox/BeOS_5.0_Depend/mozilla/xpcom/glue/standalone/nsGREDirServiceProvider.cpp:248: (Each undeclared identifier is reported only once /boot/home/tinderbox/BeOS_5.0_Depend/mozilla/xpcom/glue/standalone/nsGREDirServiceProvider.cpp:248: for each function it appears in.) make[5]: *** [nsGREDirServiceProvider.o] Error 1

Sorry for spamming this bug but is there some documentation or discussion online about the migration to GRE and how it affects embeddors? In agreement with the concept but need to know how we'll be impacted. Thanks.