223900 - Clean up the MFCEmbed situation using the GRE, and other cleanup

Status: RESOLVED FIXED

(Core Graveyard :: Embedding: GRE Core, defect)

Description

[Benjamin Smedberg]

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 1

[Benjamin Smedberg]

Attachment: make MFCEmbed use the standalone glue

Comment 2

[Benjamin Smedberg]

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 3

[Doug Turner (:dougt)]

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?

Comment 4

[Benjamin Smedberg]

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

Comment 5

[Doug Turner (:dougt)]

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

Comment 6

[Adam Lock]

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.

Comment 7

[Benjamin Smedberg]

Attachment: address dougt's comments

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.

Comment 8

[[e-mail]]

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

[Adam Lock]

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

Comment 10

[Doug Turner (:dougt)]

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

Comment 11

[Benjamin Smedberg]

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

--BDS

Comment 12

[Doug Turner (:dougt)]

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

Comment 13

[Benjamin Smedberg]

Attachment: rearranged function definitions to preserse CVS blame

Comment 14

[Doug Turner (:dougt)]

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

Comment 15

[Benjamin Smedberg]

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

Comment 16

[Doug Turner (:dougt)]

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.

Comment 17

[Benjamin Smedberg]

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

Comment 18

[Benjamin Smedberg]

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

Comment 19

[Christian :Biesinger (don't email me, ping me on IRC)]

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

Comment 20

[Christian :Biesinger (don't email me, ping me on IRC)]

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

Comment 21

[amutch]

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.