MFCEmbed should be a first-class embedding API, builtin to libxul/xulrunner when the toolkit is gtk/gtk2.

Jeez, what I *meant* was gtkmozembed.

Please don't do that. And please don't do the same for spidermonkey. These are both used by third party applications, and these third party applications don't need to be bloated by needing to load all the libxul stuff they don't need. XULRunner as a runtime could also be used as a way to provide some libraries for such third party applications (such as galeon, epiphany... or even the javascript library for perl). Needing them to link against libxul would be bloated, and needing to install an additional spidermonkey would also be bloated. BTW, i just noticed libgtkembedmoz.so was linked to libxul, while it didn't in the past. Is this link absolutely necessary ? (I'm not quite sure)

Please don't do that. And please don't do the same for spidermonkey. These are both used by third party applications, and these third party applications don't need to be bloated by needing to load all the libxul stuff they don't need. XULRunner as a runtime could also be used as a way to provide some libraries for such third party applications (such as galeon, epiphany... or even the javascript library for perl). Needing them to link against libxul would be bloated, and needing to install an additional spidermonkey would also be bloated. BTW, i just noticed libgtkembedmoz.so was linked to libxul, while it didn't in the past. Is this link absolutely necessary ? (I'm not quite sure)

libxul is not bloat, it is a significant performance boost for the core layout engine, similar to the way that firefox currently statically links most of gecko into the executable. Part of the goal for libxul is to make it *easier* for galeon/epiphany/camino/mozilla activex control to embed mozilla in a stable+frozen manner.

I don't know exactly for libgtkembedmoz, but it seems to me it is less bloat in firefox/mozilla at the moment than it is with libxul... considering libgtkembedmoz only is linked against libnspr and libxpcom in firefox/mozilla, while it is additionnally linked against libxul, which is a 10MB stripped library here. Adding spidermonkey in there would be even more bloat, especially for those software that use spidermonkey as js engine, and only spidermonkey. Yes, such software could use the standalone one, but : 1. the standalone spidermonkey has not been updated for 2 years IIRC, 2. you'll get 2 instances of spidermonkey on the disk, even if one is hidden, with all update problems you get in such cases. Dynamically loaded libraries have been made for a good reason. There ARE software that use spidermonkey, don't forget those.

Spidermonkey is not affected by this. You seem to ignore the fact that even though you are linking against NSPR+XPCOM, you are actually dynamically loading the other gecko components, which is much slower than linking to them directly. This will only change your footprint for the better. And please, have the discussion with me directly by email or something, not in an implementation bug.

(In reply to comment #6) > discussion with me directly by email or something, not in an implementation bug. how about a newsgroup? npm.embedding maybe

I just ran into this today. Is anyone working on this?

roc: I don't have any code for this yet; it's pretty simple, though, you basically need to change the makefiles to change how things are linked. There is one problem though, which is that when you're linking against libxul you need to inform libxul of its location, so that it can identify and load component libs etc correctly: this will involve an API change to gtk_moz_embed_new or add an API for use before any of the existing gtkmozembed APIs to inform libxul where it is installed.

> when you're linking against libxul you need to inform libxul of its location... Is there really no way to support the way it worked in Gecko 1.7 without introducing new APIs?

It works in gecko 1.7 only because the embedder sets MOZILLA_FIVE_HOME. I think we can continue to support this, but I'd like to at least warn on the console because that's extremely fragile.

There is another issue, that doesn't really belong to this bug, but well... since you're talking about it... if firefox ever gets compiled against libxul, there will be a components problem, since the libxul components will be let's say, in /usr/lib/xulrunner/components and the firefox components will be in /usr/lib/mozilla-firefox/components. For the moment, it will only look at one of these locations. There should be something like a GRE_COMPONENTS_DIR and an APP_COMPONENTS_DIR...

Mike, that's not an issue, see http://lxr.mozilla.org/mozilla/source/toolkit/xre/nsXREDirProvider.cpp#443

So far I've found a couple of issues: -- We need to call XRE_InitEmbedding instead of NS_InitEmbedding to get the libxul static components set up. -- xulrunner wants to find xpti.dat in the xulrunner components directory, which seems wrong, but may be fixed by the previous change

(In reply to comment #14) > So far I've found a couple of issues: > > -- We need to call XRE_InitEmbedding instead of NS_InitEmbedding to get the > libxul static components set up. hmm.. i thought we fixed NS_InitXPCOM2 to load libxul's static components automatically. Perhaps the problem is that NS_InitEmbedding changed to support having the caller specify the list of static components explicitly. What is gtkmozembed passing? If it is passing NULL, then NS_InitXPCOM3 will pick up the default libxul static components. > -- xulrunner wants to find xpti.dat in the xulrunner components directory, > which seems wrong, but may be fixed by the previous change Hmm... does it do this after profile selection too?

(In reply to comment #15) > hmm.. i thought we fixed NS_InitXPCOM2 to load libxul's static components > automatically. Perhaps the problem is that NS_InitEmbedding changed to > support having the caller specify the list of static components explicitly. > What is gtkmozembed passing? If it is passing NULL, then NS_InitXPCOM3 will > pick up the default libxul static components. gtkmozembed is passing the list from nsStaticComponents.h, which is apparently empty ... I don't know why. > > -- xulrunner wants to find xpti.dat in the xulrunner components directory, > > which seems wrong, but may be fixed by the previous change > > Hmm... does it do this after profile selection too? I can't get that far without any components :-). I'll keep working on it.

> gtkmozembed is passing the list from nsStaticComponents.h, which is apparently > empty ... I don't know why. I think that is left-over cruft from the way we used to do static components. It should just pass NULL.

This depends on the profile-management changes in bug 321359.

I don't know mozilla's build system, so I cannot comment on the Makefile changes. >+ if (EmbedPrivate::sProfileDir && !strcmp(aKey, NS_APP_USER_PROFILE_50_DIR)) >+ return EmbedPrivate::sProfileDir->Clone(aResult); Should that set *aPersist = PR_TRUE too? I don't think gtkmozembed supports profile changes. Added after reading below: at least if the profile changes, we're after XRE_TermEmbedding() in PopStartup; doing XRE_InitEmbedding again in PushStartup afterwards, and I don't think the value is cached beyond XRE_TermEmbedding. >+ if (sProfileDir) >+ XRE_NotifyProfile(); I see that XRE_NotifyProfile is void, but is there really nothing in it that could fail, i.e. should it have (and check here) a return value? >+ NS_RELEASE(sProfileDir); >+ NS_RELEASE(sProfileLock); [...] >+ sProfileDir->Release(); [...] >+ sProfileLock->Release(); Why not use NS_RELEASE here too. >+#define GTKMOZEMBED_FUNCTIONS \ Might want to list the funcs from gtkmozembed_internal.h here too. >+ static const GREVersionRange greVersion = { >+ "1.9a", PR_TRUE, >+ "1.9", PR_TRUE >+ }; Should this hardcode 1.9 ? (Same for the other occurrences below in the other tests.) char xpcomPath[PATH_MAX]; What if PATH_MAX isn't defined?

> I see that XRE_NotifyProfile is void, but is there really nothing in it that > could fail, i.e. should it have (and check here) a return value? There is nothing that can fail, no. (or more precisely, we intentionally ignore the various failure modes because it doesn't make sense to allow observers to interrupt profile selection. > Might want to list the funcs from gtkmozembed_internal.h here too. No: I intend to freeze a few of these, but I don't want to glue the nonfrozen functions. > What if PATH_MAX isn't defined? When could that happen? We rely on PATH_MAX in various places already.

Comment on attachment 208277 [details] [diff] [review] Build gtkmozemed on the XRE embedding APIs, and incorporate it into libxul, rev. 2 I see you're removing the gtk_moz_embed_*_[flags|enums]_get_type functions now (I wonder why they were gtk1 only... seems useful on gtk2 too!)... if you do that, you should remove the gtk_moz_embed_*_*_values structs too.

(In reply to comment #21) > > What if PATH_MAX isn't defined? > > When could that happen? We rely on PATH_MAX in various places already. LXRing for PATH_MAX [http://lxr.mozilla.org/seamonkey/search?string=PATH_MAX] I see that some other parts of moz use #if[n]def PATH_MAX... not sure if it's too relevant; just sth. I wanted to mention.

Oh, and since the gtkmozembed .so will disappear, you'll also need to fix up the pkg-config file mozilla/build/unix/mozilla-gtkmozembed.pc.in which refers to it in the Libs: section.

Oops, this wasn't intentional, reopening. Sorry.

> Oh, and since the gtkmozembed .so will disappear, you'll also need to fix up > the pkg-config file mozilla/build/unix/mozilla-gtkmozembed.pc.in which refers > to it in the Libs: section. I should just remove this file, since it is essentially replaced by the not-yet-existing mozilla-xulrunner.pc.in ;-), can we leave this for a separate bug? (There are lots of xulrunner packaging changes that haven't happened yet.)

Removed the values to go with the [flags|enums]. I don't know anything about these functions/structures, so if they would be useful on GTK2 I'm happy to accept a patch to re-add them.

Comment on attachment 209464 [details] [diff] [review] Build gtkmozembed on the XRE embedding APIs, and incorporate it into libxul, rev. 2.1 If we want them, the enums should be autogenerated with glib-mkenums anyway.

Fixed on trunk.

when I tried to run TestGtkEmbed, I got it $$ cd <target_dir> $$ export LD_LIBRARY_PATH=. $$ export MOZILLA_FIVE_HOME=. $$$./run-mozilla.sh ./TestGtkEmbed Couldn't find a compatible GRE. sourcebase dated from 1st Feb (miniMo build). what am I missing ?

You're missing "export GRE_HOME=./libxpcom.so" or an installed xulrunner. it should be GRE_HOME=. but see bug 325450 I'm not sure about how minimo fits together in terms of linkage, but with the stock xulrunner build you shouldn't need to set LD_LIBRARY_PATH or MOZILLA_FIVE_HOME.

(In reply to comment #31) > You're missing "export GRE_HOME=./libxpcom.so" or an installed xulrunner. > > it should be GRE_HOME=. but see bug 325450 > > I'm not sure about how minimo fits together in terms of linkage, but with the > stock xulrunner build you shouldn't need to set LD_LIBRARY_PATH or > MOZILLA_FIVE_HOME. Benjamin, that's the point. Once I'm using the a minimo build, where minimo is built staticly, and there is no libxpcom.so, but dist/lib/libxpcom_core.a instead (it also gets linked into MiniMo executable, of course). so I added the following lines at mozilla_root_dir/xpcom/build/Makefile.in, aiming get libxpcom_core.a copied to $objdir/dist/bin during the build. libs:: $(srcdir)/$(LIB_PREFIX)xpcom_core.$(LIB_SUFFIX) $(INSTALL) $< $(DIST)/bin install:: $(srcdir)/$(LIB_PREFIX)xpcom_core.$(LIB_SUFFIX) $(SYSINSTALL) $(IFLAGS2) $< $(DESTDIR)$(mozappdir) Ok, done so, then I set the GRE_HOME as you said and ran TestGtkEmbed. >> export GRE_HOME=./libxpcom_core.a >> ./run-mozilla.sh ./TestGtkEmbed www.google.com Couldn't find a compatible GRE. (Now, it changed the error message, at least) :) * do my changes (at xpcom/build/Makefile.in) make sense for you ? * should I try this over a xulrunner build instead ?

(In reply to comment #31) > You're missing "export GRE_HOME=./libxpcom.so" or an installed xulrunner. > > it should be GRE_HOME=. but see bug 325450 It would be really nice if this were documented somewhere. I just wasted half an hour trying to debug a layout problem that required debugging in an embedding app. I should never have removed viewer. At least it started.