+++ This bug was initially created as a clone of Bug #381157 +++ Bug 381157 requires some toolkit/ changes to enable the DLMGR. This bug will track those changes, adding a new define/makefile variable that Suite's build process can use. Due to current 1.9.0 rules this needs to apply and be pushed to mozilla-central first. r? sdwilsh for toolkit; sr? from neil for xpfe/ (If anyone knows a cleaner way to do these makefile ifdef's please tell me)

whops, correcting summary and component...

Is anyone else still using the xpfe download manager?

re: c#2, not that I could tell.

In that case, might it be easier to have a variable SUITE_USING_XPFE_DM which is turned on by default for suite so you can replace the toolkit ifndef MOZ_SUITE with ifndef SUITE_USING_XPFE_DM rather than the triple ifdef in your patch?

Comment on attachment 326074 [details] [diff] [review] patch to mozilla-central > ifneq ($(MOZ_BUILD_APP),camino) >+ifdef MOZ_SUITE >+ifdef SUITE_USING_TOOLKIT_DM > DIRS += download-manager >+endif >+else >+DIRS += download-manager >+endif > endif I don't think the ifdef MOZ_SUITE is necessary as nobody else should be running this Makefile except for camino which is already excluded. I also think your ifdef SUITE_USING_TOOLKIT_DM should be an ifndef. >+#if !defined(SUITE_USING_TOOLKIT_DM) I'd prefer #ifndef

Comment on attachment 326074 [details] [diff] [review] patch to mozilla-central Address Neil's comments first, thanks.

(In reply to comment #5) > I don't think the ifdef MOZ_SUITE is necessary as nobody else should be running > this Makefile except for camino which is already excluded. I also think your > ifdef SUITE_USING_TOOLKIT_DM should be an ifndef. Not quite right, everyone is still running this Makefile, but its only Suite/Camino currently running that part of the Makefile.

Here is the updated mozilla-central patch... (Note: suite/app-config.mk is included in this patch though would not be pushed to mozilla-central, but it would be applied to 1.9.0.x when it comes time)

Comment on attachment 326683 [details] [diff] [review] patch to mozilla-central >+++ b/suite/app-config.mk >@@ -0,0 +1,2 @@ >+SUITE_USING_XPFE_DM=1 >+DEFINES+="SUITE_USING_XPFE_DM=1" I don't think quotes are right here, and I think spaces would be a good idea. >\ No newline at end of file Fix please! >-ifndef MOZ_SUITE >-# XXX Suite doesn't want these just yet > ifdef MOZ_RDF >+ifndef SUITE_USING_XPFE_DM [Twice] Please leave the ifdefs in the same order i.e. suite first, then RDF. >-endif >-endif >+endif # SUITE_USING_XPFE_DM >+endif # MOZ_RDF And I don't see the point of reannotating these, it's only a 1-line ifdef! > ifneq ($(MOZ_BUILD_APP),camino) >+ifdef SUITE_USING_XPFE_DM You could probably remove the camino check; they won't set SUITE_USING_XPFE_DM

Comment on attachment 326683 [details] [diff] [review] patch to mozilla-central +DEFINES+="SUITE_USING_XPFE_DM=1" I think this needs to be: DEFINES+="-DSUITE_USING_XPFE_DM=1"

Pushed to mozilla-central, (minus suite/app-config.mk) going to let it bake a bit before committing to CVS (and requesting approval) pushing to ssh://hg.mozilla.org/mozilla-central/ searching for changes remote: adding changesets remote: adding manifests remote: adding file changes remote: added 1 changesets with 7 changes to 7 files

Comment on attachment 326826 [details] [diff] [review] Patch for checkin baking fine on mozilla-central. Should only affect seamonkey build.

Unfortunately, this patch is incomplete and I think the bug should be reopened. As app-config.mk gets included by config.mk, we need to have the latter loaded before using any such var in a Makefile.in!

I'm not sure if it's the ideal place for every one of those includes, but those includes of config.mk are needed for the new flag and MOZ_SUITE/MOZ_THUNDERBIRD to work in the new world, as it's now config.mk which loads those.

Comment on attachment 327280 [details] [diff] [review] supplemantal patch for including config.mk early enough Looks good.

supplementary patch checked in as changeset e7939e8a558e, so should be really fixed now.

Is this patch needed in CVS (1.9.0) since SeaMonkey is moving to hg? If so, please provide a combined patch with both the original patch and the follow up one.

Comment on attachment 326826 [details] [diff] [review] Patch for checkin removing approval request as it does appear we won't need this in 1.9.0 due to SeaMonkey's move happening even sooner than I expected.