382586 - Clean up /suite after suite moving to toolkit

Status: RESOLVED FIXED

(SeaMonkey :: Build Config, defect)

Description

[Mark Banner (:standard8)]

Now suite has moved to be an xul app on trunk, we need to clean up /suite:

- Anything that is ifndef MOZ_XUL_APP we need to remove (we also need to remove the ifdef statements and tidy up what they enclose).
- There are various files/directories (e.g. contents.rdf files and suite/components directory) that will need files removing as they are no longer required.

This should be quite easy to do, patches welcome. It'll save us time and make the development tree cleaner and easier to work with.

Comment 1

[Caio Tiago Oliveira (asrail)]

I can help with the ifdefs.

;)

Comment 2

[Robert Kaiser]

we also should remove components/ now.

Comment 3

[Mark Banner (:standard8)]

(In reply to comment #1)
> I can help with the ifdefs.
> 
> ;)
> 
Excellent. When you remove the ifdefs just search for MOZ_XUL_APP in the suite directory. Anything that's not MOZ_XUL_APP can come out. If it references directories then (e.g. components) then all the files within that need to be removed as well. In jar.mn files there'll be references to files that are no longer required as well.

There's several ways to do it, but if you could provide a patch and a list of files that'll need removal, that would be great :-)

Comment 4

[Caio Tiago Oliveira (asrail)]

(In reply to comment #3)
> Excellent. When you remove the ifdefs just search for MOZ_XUL_APP in the suite
> directory. Anything that's not MOZ_XUL_APP can come out. If it references
> directories then (e.g. components) then all the files within that need to be
> removed as well. In jar.mn files there'll be references to files that are no
> longer required as well.

I'll be in front of my tree in a few hours (I'm on the college right now).
My idea is, actually, to use a script to process the files as if MOZ_XUL_APP was set to 1.
I can change it to list the files referenced in jar.mn's, on the ifndef MOZ_XUL_APP part.

> There's several ways to do it, but if you could provide a patch and a list of
> files that'll need removal, that would be great :-)

The ifdef part is trivial, I can provide a patch soon.
To list all files maybe a quite hard and require manual processing, so it can take a bit longer, but I can look at it too.

Comment 5

[Robert Kaiser]

Using a script assumes you have loads of that, but this isn't true, the number should be quite nice to handle.
And for every line in jar.mns, you need to also remove the respective real file, so you need to process those manually anyways.
I think going over those cases manually is easier than automated, actually.

Comment 6

[Caio Tiago Oliveira (asrail)]

(In reply to comment #5)
> Using a script assumes you have loads of that, but this isn't true, the number
> should be quite nice to handle.
> And for every line in jar.mns, you need to also remove the respective real
> file, so you need to process those manually anyways.
> I think going over those cases manually is easier than automated, actually.
> 

mmm... maybe.
As I've said, I'll do it in a few hours, because I'll have a tree in front of mine and I'll be able to generate a patch.

I know it's outside the scope of '/suite'(this bug), but...
Should I care for things like this:
http://mxr.mozilla.org/seamonkey/source/xpinstall/packager/unix/Makefile.in#58

or ifdef MOZ_XUL_APP inside of ifdef MOZ_SUITE outside of '/suite'?

Comment 7

[Mark Banner (:standard8)]

(In reply to comment #6)
> I know it's outside the scope of '/suite'(this bug), but...
> Should I care for things like this:
> http://mxr.mozilla.org/seamonkey/source/xpinstall/packager/unix/Makefile.in#58
> 
> or ifdef MOZ_XUL_APP inside of ifdef MOZ_SUITE outside of '/suite'?

It may need doing, but just do /suite for now. Robert's got a bug on xpfe, benjamin wants to trash xpinstall anyway, and there's other parts of the tree that Camino are still using as a non-xul app. I was going to follow them up (once I got my head round some of them again) with their own bugs. I was thinking this was a good starter into getting the tidy up going whilst we're still sorting out the regressions etc.

Comment 8

[Worcester12345]

Beginning to wonder if a new branch will happen soon, and if so, should the completion of this bug block it.

Comment 9

[Caio Tiago Oliveira (asrail)]

Attachment: Removing ifdefs on /suite

Comment 10

[Caio Tiago Oliveira (asrail)]

Attachment: List of files to remove

Comment 11

[Caio Tiago Oliveira (asrail)]

Attachment: Two unsaved files...

Comment 12

[Mark Banner (:standard8)]

Comment on attachment 266868 [details] [diff] [review]
Two unsaved files...

Thanks for the patch, in general it looks good.

Some comments though:

DIRS		= \
 		branding \
 		browser \
 		common \
 		locales \
 		$(NULL)
 
-# XXX Once SeaMonkey becomes a fully fledged xul app, we can remove
-# this ifdef.
-ifdef MOZ_XUL_APP
 # if you add DIRS here, care that app is always at the bottom of the list
 # as it packages up the built files on mac...
 DIRS += \
 		profile \
 		build \
 		app \
 		$(NULL)

Please make this one DIRS structure, there is no reason to keep it as two now.

 tier_app_dirs += \
 	xpfe/components/search \
 	xpfe/components/bookmarks \
 	$(NULL)
 
-# When Suite becomes a full MOZ_XUL_APP we can remove this ifdef
-ifdef MOZ_XUL_APP
-tier_app_dirs += themes
-endif
-
 tier_app_dirs += suite
 
We need to keep the themes dir, so as above, please add/move themes and suite so that they one tier_app_dirs addition with the search and bookmarks.

-# XXX Once SeaMonkey becomes a fully fledged xul app, we can remove
-# this ifdef.
-ifdef MOZ_XUL_APP
 EXTRA_COMPONENTS = nsBrowserContentHandler.js
...
 EXTRA_COMPONENTS += nsAboutAbout.js

Again, please join the EXTRA_COMPONENTS items.

Comment 13

[Robert Kaiser]

Comment on attachment 266866 [details] [diff] [review]
List of files to remove

1) Neither Mark nor I are super-reviewers.
2) It doesn't make sense if we independetly review only the ifdef removal or the file removal, as both need the other in turn to be complete.

Directing this to Mark, as he already started with the other part of this.

Comment 14

[Caio Tiago Oliveira (asrail)]

Attachment: Addressing Mark comments

Comment 15

[Mark Banner (:standard8)]

Comment on attachment 266866 [details] [diff] [review]
List of files to remove

r=me, though I not you've got some duplication of files in here, and /mozilla/suite/components/xulappinfo/nsIXULAppInfo.idl doesn't actually exist under /suite (and shouldn't be removed anyway).

If I'm checking this in then I can cope with that, if not you may want to regenerate the list.

Comment 16

[Mark Banner (:standard8)]

Comment on attachment 266979 [details] [diff] [review]
Addressing Mark comments

Yep, that's much better. r=me. Like Robert said, I can't give sr, so redirecting to Neil (who can).

Comment 17

[Caio Tiago Oliveira (asrail)]

(In reply to comment #15)
> (From update of attachment 266866 [details] [diff] [review])
> r=me, though I not you've got some duplication of files in here, and
> /mozilla/suite/components/xulappinfo/nsIXULAppInfo.idl doesn't actually exist
> under /suite (and shouldn't be removed anyway).

About the duplication, I'll take a look.
About the idl:
-else
-# XXX This should completely go away in the future but still holds the
-# xulappinfo component needed as long as suite is not a fully fledged
-# MOZ_XUL_APP
-DIRS +=		components
-endif

I'm might to add this, but it was under the ifndef part.

> If I'm checking this in then I can cope with that, if not you may want to
> regenerate the list.

Yeah, I'll see what caused the duplication.

Comment 18

[Mark Banner (:standard8)]

(In reply to comment #17)
> About the idl:
> -else
> -# XXX This should completely go away in the future but still holds the
> -# xulappinfo component needed as long as suite is not a fully fledged
> -# MOZ_XUL_APP
> -DIRS +=                components
> -endif
> 
> I'm might to add this, but it was under the ifndef part.

Yes its under the ifndef part, but you'll find the idl file is actually in toolkit and shouldn't be removed. Everything in suite/components should be removed.

Comment 19

[Caio Tiago Oliveira (asrail)]

Attachment: Removing the duplicates and the non existent file

Comment 20

[Mark Banner (:standard8)]

I've checked in the patch and cvs removed the files on behalf of Asrail.

Thanks for the patch Asrail.