When bug 328887 lands and SeaMonkey will use toolkit, a lot of code in xpfe/ will go unused. We should clean that up (cvs removing lots of stuff), so that the tree gets cleaner for everybody.

This patch removes those parts I could easily spot that we can kill in xpfe/ Additionally, those dirctories can be cvs removed: mozilla/xpfe/components/alerts/resources/ mozilla/xpfe/components/cookie/ mozilla/xpfe/components/updates/resources/ mozilla/xpfe/components/xfer/ mozilla/xpfe/components/filepicker/res/ mozilla/xpfe/components/console/resources/ mozilla/xpfe/components/find/resources/ Camino is still xpfe-based, so we unfortunately can't clean up as much as we'd want to - only the part about the alerts service in components/build can go away, as this is windows/gtk2-only and so not built for Camino, if I'm reading that Makefile syntax correctly ;-)

(In reply to comment #1) > Camino is still xpfe-based, so we unfortunately can't clean up as much as we'd > want to - only the part about the alerts service in components/build can go > away, as this is windows/gtk2-only and so not built for Camino, if I'm reading > that Makefile syntax correctly ;-) Well you're right that Camino is the only other one we need to worry about in xpfe/components (Minimo has MOZ_XPFE_COMPONENTS not defined, so it just does an exports cycle through xpfe/components - I've never been able to convince myself if thats really required now or not). Can I suggest that you do a patch that removes all of alerts, updates, console, startup, extensions and ask Camino to try it out/approve it? From the fact that Camino don't incorporate alerts or startup in the build section, I'm guessing that Camino don't actually use them (a quick search for the public interfaces in camino found no matches). For the others, I'm guessing that as we're able to remove the chrome dirs, we're probably safe in removing the source anyway. I've had good responses from them in the past wrt tidy up, so it'd be good to take this opportunity if we can.

From what I'm seeing, every Mozilla build process at least enters xpfe/components (if MOZ_XPFE_COMPONENTS is unset, it goes in at least in the export stage) - and everyone else than Minimo (which even has MOZ_XUL_APP=1 btw, through the "basic" embedding profile) is using the default embedding profile, which has this var turned on. Together with the MOZ_HAVE_BROWSER stuff in the Makefile and Camino, this looks a bit too hot for me.

Comment on attachment 267090 [details] [diff] [review] remove a list of definitions from the xpfe/ source (checked in) Last time I looked, filepicker and console had patches which are still waiting to be ported to toolkit, so they can't be CVS removed just yet. >-#ifndef MOZ_XUL_APP >-NS_GENERIC_FACTORY_CONSTRUCTOR(nsCmdLineService) >-NS_GENERIC_FACTORY_CONSTRUCTOR(nsAppStartup) >-NS_GENERIC_FACTORY_CONSTRUCTOR(nsUserInfo) >-#endif // !MOZ_XUL_APP Are you sure that Camino doesn't use these?

(In reply to comment #4) > (From update of attachment 267090 [details] [diff] [review]) > Last time I looked, filepicker and console had patches which are still waiting > to be ported to toolkit, so they can't be CVS removed just yet. At least their UI isn't used any more in suiterunner, so I don't think it's too useful to keep that around. > >-#ifndef MOZ_XUL_APP > >-NS_GENERIC_FACTORY_CONSTRUCTOR(nsCmdLineService) > >-NS_GENERIC_FACTORY_CONSTRUCTOR(nsAppStartup) > >-NS_GENERIC_FACTORY_CONSTRUCTOR(nsUserInfo) > >-#endif // !MOZ_XUL_APP > Are you sure that Camino doesn't use these? At least not here, as this is inside |#ifdef MOZ_SUITE| (even visible in the patch context).

Comment on attachment 267090 [details] [diff] [review] remove a list of definitions from the xpfe/ source (checked in) r=me for suite build config. I'd prefer it if you get benjamin's (or someone's) review as well. Also I assume you're going to be sorting out the MOZ_SUITE ifdefs in xpfe/global and xpfe/bootstrap in another patch?

Comment on attachment 267090 [details] [diff] [review] remove a list of definitions from the xpfe/ source (checked in) Requesting additional build system review as requested by Mark. And yes, I'll attack the additional, not as clear cases with additional patches.

Note: From what I see in http://mxr.mozilla.org/seamonkey/search?find=%2F&string=xpfe%2Fcomponents only search, bookmarks, and startup are directly referenced from elsewhere in the tree, everything else is built directly from within xpfe/components/ - oh, and http://mxr.mozilla.org/seamonkey/source/camino/Camino.xcode/project.pbxproj#16757 is directly referencing libappcomps.

This patch plus the removed files listed at its end kill off most of xpfe/bootstrap. From what I see, we might only need nsSigHandlers.cpp and showOSAlert.cpp from it in toolkit/xre, but I didn't dare to kill off more than I did in this step. This at least builds fine on Linux (SeaMonkey, Firefox, Thunderbird). Not sure if we could remove more in the "export" target - I just saw that no SHAREDCMMSRCS are defined anyways in that Makefile.

ad comment #9 - I just finished a build flawlessly (on Linux) with having only nsSigHandlers.cpp and showOSAlert in xpfe/bootstrap :)

Comment on attachment 267090 [details] [diff] [review] remove a list of definitions from the xpfe/ source (checked in) checked in this parts, cvs removes still pending, need to check back with Neil about comment #4

Comment on attachment 267633 [details] [diff] [review] kill a list of files from xpfe/bootstrap (checked in) checked in along with removing the files listed there. will get separate review about unused .cpp/.h files in xpfe/bootstrap - maybe via IRC

(In reply to comment #1) > mozilla/xpfe/components/alerts/resources/ > mozilla/xpfe/components/cookie/ > mozilla/xpfe/components/updates/resources/ > mozilla/xpfe/components/xfer/ > mozilla/xpfe/components/find/resources/ I cvs removed those now as well. > mozilla/xpfe/components/filepicker/res/ > mozilla/xpfe/components/console/resources/ Those were left alive per comment #4, need to sort out first what improvements went in on the xpfe side which are lacking on toolkit and file bugs for those.

I just did the rest of the possible cvs removes in xpfe/bootstrap - for reference, this leaves the following content in that directory: appleevents / - still built on mac nsDocLoadObserver[.cpp|.h] - used by appleevents nsSigHandlers.cpp - used by toolkit/xre showOSAlert.cpp - used by toolkit/xre

I have taken a look into xpfe/global and xpfe/communicator - we still need xpfe/global/resources/content/nsWidgetStateManager.js which is packed up by suite/ as long as we're using the old prefwindow, and I need to find out what Thunderbird needs from communicator, but apart from that, I think both can be killed.

Regarding comment #4 (and comment #13): Bug 266629 fixed all inconsistencies of filepicker, so we can actually get rid of filepicker/res/ (the public and src portions probably should also be moved at some point). Error Console is a different story, I filed bug bug 386899 for that.

filepicker/res/ has been removed after OK from Neil via IRC.

Robert, Are you still working on this ?

I will be once the blockers are resolved. There is a reason for bugs having blockers set. ;-)

As we don't have to take care of Camino still entering those directories now on Mercurial, I'd like to move on and kill xpfe/communicator/ and xpfe/global/ from mozilla-central. I've searched for any occurrences of those on mozilla-central or comm-central, and this patch fixes everything I found. Please see this review request as being for that patch _and_ the removal of those two directories - with one exception: xpfe/global/resources/content/nsWidgetStateManager.js is used by SeaMonkey until the switch to the new prefwindow is completed (bug 394522). Once that task is done (target is early September), we can remove that last file to complete the extinction of xpfe/global.

This is probably the last part of this bug - everything else that needs cleanup can be done in other bugs, including components/ subdirs that might be in need to get obsoleted. The patch here reorders the components/Makefile.in a bit to not have as many nested ifdefs and kills off the two directories that are only built for non-xulapps, which don't exist any more on hg (startup and xremote).

Comment on attachment 335223 [details] [diff] [review] clean up xpfe/components >+ifdef MOZ_SUITE >+DIRS += \ >+ related \ >+ $(NULL) Might as well make this DIRS += related

Comment on attachment 334092 [details] [diff] [review] patch to go along with killing off communicator/ and global/ (checked in) Thanks, pushed as http://hg.mozilla.org/mozilla-central/index.cgi/rev/d98ddf4499cc

New version of the patch, including more places where startup and xremote were referred to - see toolkit-makefiles.sh and bootstrap/appleevents changes in the beginning. Re-requesting sr for those changes as well - the rest of the patch is unchanged.

Comment on attachment 335380 [details] [diff] [review] clean up xpfe/components, v1.1 [Checkin: Comment 28] +ifdef MOZ_SUITE +DIRS += \ + related \ + $(NULL) + +ifdef SUITE_USING_XPFE_DM +DIRS += download-manager +endif + +ifndef MOZ_PLACES +DIRS += history +endif + +ifeq ($(OS_ARCH),WINNT) +DIRS += winhooks +endif +endif Could you stick a # MOZ_SUITE after the last endif here? It gets a little difficult to follow nested if blocks.

(In reply to comment #25) > Could you stick a # MOZ_SUITE after the last endif here? It gets a little > difficult to follow nested if blocks. Sure, will do that - even if I I decreased nesting with this patch.

Comment on attachment 335380 [details] [diff] [review] clean up xpfe/components, v1.1 [Checkin: Comment 28] >+DIRS += \ >+ related \ >+ $(NULL) Still might as well make this DIRS += related ...

Landed with nits addressed as http://hg.mozilla.org/mozilla-central/index.cgi/rev/1feec409d801 From what I see, we should have cleaned out all dead code from xpfe/ now, all further code should be tracked by appropriate bugs that switch over users of the old code to newer code.