451909 - kill MOZ_XUL_APP now that all apps set it

Status: RESOLVED FIXED

Description

[Robert Kaiser]

Now that we live on Mercurial, all apps building on our platform set MOZ_XUL_APP - Camino was the last app that didn't set it, but they are not on hg yet and should not migrate to 1.9.1 unless they finally convert to being based on toolkit.

This means we can go for getting rid of code that is |ifndef MOZ_XUL_APP| and finally of the flag itself. As a first step, I'll probably do a patch that makes configure fail if the flag is not set so anything that might still build that way (I doubt it) is showing an error, then I'll remove |ifndef MOZ_XUL_APP| code step by step, removing the checks for the flag.

Comment 1

[Robert Kaiser]

Created attachment 335222 [details] [diff] [review]
error out on builds unsetting MOZ_XUL_APP (checked in)

This patch makes us error out on builds that unset MOZ_XUL_APP in confvars.sh (can only happening there as nothing in our build system unsets it but configure sets it by default).

Comment 2

[Arpad Borsos [:Swatinem]]

KaiRo: If you don't mind I would like to take over this bug.
I hope I understood it correctly that everything in code and makefiles that deals with MOZ_XUL_APP unset is to be removed, including complete directories like http://mxr.mozilla.org/comm-central/source/mozilla/embedding/browser/chrome/ ?

Comment 3

[Robert Kaiser]

Right. Feel free to take over, I'll check in this first patch though.
The URL I did set for this bug should actually point to all places that use MOZ_XUL_APP and all those places need changes. If we are only building a whole directy |ifndef MOZ_XUL_APP|, we should do a patch to remove all of it.

Please do step-by-step patches though, they are easier to review than one large patch :)

Comment 4

[Robert Kaiser]

Comment on attachment 335222 [details] [diff] [review]
error out on builds unsetting MOZ_XUL_APP (checked in)

OK, checked in this first patch. Now we can be really sure that everyone has the flag set and remove code that depends on it being unset (including the checks themselves).

Comment 5

[Arpad Borsos [:Swatinem]]

Created attachment 335598 [details] [diff] [review]
[checked in] mailnews cleanup

Comment 6

[Arpad Borsos [:Swatinem]]

Created attachment 335599 [details] [diff] [review]
[checked in] inspector cleanup

I really don't know who I should ask for review here.

Comment 7

[Arpad Borsos [:Swatinem]]

Created attachment 335600 [details] [diff] [review]
[checked in] docshell cleanup

Comment 8

[Arpad Borsos [:Swatinem]]

Created attachment 335601 [details] [diff] [review]
extensions cleanup (spellchecker, layout-debug)
[Checkin: Comment 49]

Comment 9

[Arpad Borsos [:Swatinem]]

Created attachment 335602 [details] [diff] [review]
[checked in] toolkit cleanup

Comment 10

[Arpad Borsos [:Swatinem]]

Created attachment 335605 [details] [diff] [review]
xpfe cleanup

Comment 11

[Arpad Borsos [:Swatinem]]

Created attachment 335606 [details] [diff] [review]
jar.mn cleanup (whole tree)
[Checkin: Comment 34]

Comment 12

[Arpad Borsos [:Swatinem]]

Created attachment 335607 [details] [diff] [review]
makefile cleanups (whole tree)

Comment 13

[Arpad Borsos [:Swatinem]]

Created attachment 335610 [details] [diff] [review]
makefile cleanups (whole tree) fixed

sorry, missed a few subdirs which can go away

Comment 14

[Arpad Borsos [:Swatinem]]

Created attachment 335611 [details]
list of hg removes

This is a list of hg rm commands that removes directories or files which got obsolete due to makefile or jar.mn changes.

Comment 15

[Arpad Borsos [:Swatinem]]

Created attachment 335618 [details] [diff] [review]
makefile cleanups (whole tree) v3

I'm sorry for yet another patch, forgot the makefiles in allmakefiles.sh.

Whats still left to do: the configure.ins and autoconf.mk.ins of c-c and m-c. Also, I haven't touched the code of extensions/wallet and extensions/typeaheadfind, I don't want to mess around with cvs right now.
I also found another problem in the list of hg removes: it's actually netwerk instead of network.

If someone could please request review from the right reviewers, I'm not yet familiar with the reviewers of all those modules.
Hope that was everything for now.

Comment 16

[Robert Kaiser]

Comment on attachment 335611 [details]
list of hg removes

>hg rm xpfe/components/startup
>hg rm xpfe/components/xremote

Those already have been removed.

Comment 17

[Robert Kaiser]

Comment on attachment 335618 [details] [diff] [review]
makefile cleanups (whole tree) v3

>diff -r 1d4a67cd6939 toolkit/toolkit-makefiles.sh
>diff -r 1d4a67cd6939 toolkit/toolkit-tiers.mk
>diff -r 1d4a67cd6939 xpfe/components/Makefile.in

Those files were changed as well even before you did that patch.

>diff -r 1d4a67cd6939 xpfe/components/winhooks/Makefile.in

You might as well leave out winhooks completely, we are about to remove it completely once the new SeaMonkey shellservice UI lands.

Comment 18

[Robert Kaiser]

Comment on attachment 335598 [details] [diff] [review]
[checked in] mailnews cleanup

I'm no mailnews owner or peer, forwarding to someone who actually can review there.

Comment 19

[Shawn Wilsher :sdwilsh]

Comment on attachment 335599 [details] [diff] [review]
[checked in] inspector cleanup

r=sdwilsh

Comment 20

[Arpad Borsos [:Swatinem]]

Created attachment 335815 [details] [diff] [review]
[checked in] makefile cleanups (whole tree) v4

(In reply to comment #17)
> (From update of attachment 335618 [details] [diff] [review])
> >diff -r 1d4a67cd6939 toolkit/toolkit-makefiles.sh
> >diff -r 1d4a67cd6939 toolkit/toolkit-tiers.mk
> >diff -r 1d4a67cd6939 xpfe/components/Makefile.in
> 
> Those files were changed as well even before you did that patch.

Yes, I noticed. Updated patch that applies fine after bug 380786 landed. I left winhooks in, shouldn't be that much of a problem I think.

Comment 21

[Ted Mielczarek [:ted.mielczarek]]

Comment on attachment 335815 [details] [diff] [review]
[checked in] makefile cleanups (whole tree) v4

diff -r 86a4475236c7 -r 90258f3e351f embedding/Makefile.in
-ifndef MOZ_XUL_APP
-DIRS += lite
-endif

You're just going to remove this dir, correct?

diff -r 86a4475236c7 -r 90258f3e351f extensions/pref/autoconfig/Makefile.in
-DIRS             = public resources src
+DIRS             = public src

Why did this directory become obsolete?

Comment 22

[Arpad Borsos [:Swatinem]]

(In reply to comment #21)
> (From update of attachment 335815 [details] [diff] [review])
> diff -r 86a4475236c7 -r 90258f3e351f embedding/Makefile.in
> -ifndef MOZ_XUL_APP
> -DIRS += lite
> -endif
> 
> You're just going to remove this dir, correct?

Yes, it's listed in the hg removes file I attached.

> diff -r 86a4475236c7 -r 90258f3e351f extensions/pref/autoconfig/Makefile.in
> -DIRS             = public resources src
> +DIRS             = public src
> 
> Why did this directory become obsolete?

The only thing in it is a jar.mn which is entirely #ifndef MOZ_XUL_APP, see http://mxr.mozilla.org/mozilla-central/source/extensions/pref/autoconfig/resources/jar.mn
The same is true for some other dirs/jars. I think its better to completely remove them, they are listed in the removes attachment as well.
Maybe you could review that removes file too? I didn't know who to ask for review there.

Comment 23

[[e-mail]]

Comment on attachment 335605 [details] [diff] [review]
xpfe cleanup

> #include "nsComponentManagerUtils.h"
> #include "nsServiceManagerUtils.h"
> 
>-#ifdef MOZ_XUL_APP
> #include "nsAppRunner.h"
>-#endif
You could get rid of that blank line too. r=me on nsContentTreeOwner.cpp (not shown here) and this file (both changes).

>diff -r 15ca53e61c8d xpfe/browser/src/nsBrowserInstance.cpp
I wouldn't bother touching nsBrowserInstance or nsWindowsHooks. As previously mentioned nsWindowsHooks will die shortly and if the code in nsBrowserInstance isn't going to be fixed to work with toolkit (which seems unlikely) then it and its callers should just be removed completely (although that probably deserves at least one new bug.)

Comment 24

[Mark Banner (:standard8)]

Comment on attachment 335599 [details] [diff] [review]
[checked in] inspector cleanup

Checked into DOMI repository, changeset 658:bf67ee2f541e

Comment 25

[Mark Banner (:standard8)]

Comment on attachment 335600 [details] [diff] [review]
[checked in] docshell cleanup

docshell, toolkit and makefile cleanups checked in with changeset id: 18576:da4096b97530

Comment 26

[Mark Banner (:standard8)]

I think I've checked in what I can here:

- xpfe patch needs update before checkin
- extensions cleanup needs review
- jar.mn cleanup needs review (ted possibly?)
- hg removes need review (again ted?)
- mailnews patch is awaiting review.

Comment 27

[Dan Mosedale (:dmose)]

Comment on attachment 335598 [details] [diff] [review]
[checked in] mailnews cleanup

r=dmose

Comment 28

[Mark Banner (:standard8)]

Comment on attachment 335598 [details] [diff] [review]
[checked in] mailnews cleanup

Checked in, changeset id: 257:9cc1aca930cc

Comment 29

[Arpad Borsos [:Swatinem]]

Created attachment 337740 [details] [diff] [review]
updated xpfe cleanup
[Checkin: Comment 33]

(In reply to comment #23)
> (From update of attachment 335605 [details] [diff] [review])
> You could get rid of that blank line too. r=me on nsContentTreeOwner.cpp (not
> shown here) and this file (both changes).

done

> I wouldn't bother touching nsBrowserInstance or nsWindowsHooks. As previously
> mentioned nsWindowsHooks will die shortly and if the code in nsBrowserInstance
> isn't going to be fixed to work with toolkit (which seems unlikely) then it and
> its callers should just be removed completely (although that probably deserves
> at least one new bug.)

Winhooks was removed, right. This patch is updated to apply cleanly after those changes. It was easier for me to just leave the BrowserInstance changes in. It should be dealt with in another bug I believe.
Apart from this, the patch is unchanged so I believe it doesn't need another review.

Comment 30

[Arpad Borsos [:Swatinem]]

Created attachment 337745 [details]
updated removes file
[Checkin: Comment 36]

removed the already removed directories and fixed the netwerk typo.

Comment 31

[Robert Kaiser]

(In reply to comment #29)
> Apart from this, the patch is unchanged so I believe it doesn't need another
> review.

Then you should mark the review+, i.e. "forward" it to the new patch.
Also, you should set the checkin-needed keyword so people know something here needs to be checked in.
Oh, and does the jar.mn patch have review yet?

Comment 32

[Arpad Borsos [:Swatinem]]

Well, ok then, the xpfe patch should be ready. The hg removes should only be checked in after the jars patch got review and check-in.

Comment 33

[Serge Gautherie (:sgautherie)]

Comment on attachment 337740 [details] [diff] [review]
updated xpfe cleanup
[Checkin: Comment 33]

http://hg.mozilla.org/mozilla-central/rev/d5e4a3e10202

Comment 34

[Serge Gautherie (:sgautherie)]

Comment on attachment 335606 [details] [diff] [review]
jar.mn cleanup (whole tree)
[Checkin: Comment 34]

http://hg.mozilla.org/mozilla-central/rev/9b921a3fdb5e

Comment 35

[Serge Gautherie (:sgautherie)]

Arpad,
in cases like this, please add a whiteboard note to be explicit about which patch(s) to check in;
also, when you forward a '+', you may copy/remind in the comment who you are forwarding from.
Thanks.

Comment 36

[Serge Gautherie (:sgautherie)]

Comment on attachment 337745 [details]
updated removes file
[Checkin: Comment 36]

http://hg.mozilla.org/mozilla-central/rev/f0f5edda2633

NB: Hg commands are (probably) fine for review, but please attach a patch for (easier) checkin.

Comment 37

[Serge Gautherie (:sgautherie)]

Reopening: doesn't seem fully fixed yet :-/

Comment 38

[Arpad Borsos [:Swatinem]]

Yes, the extensions patch (spellchecker, layout-debug) still needs review but I have no idea who should be reviewing that. And I will do the complete removal of that flag from configure once its users are all gone from the tree.
I'm not sure yet if I should completely remove it or leave the warning in and just set it to be always defined.

Comment 39

[Robert Kaiser]

(In reply to comment #38)
> I'm not sure yet if I should completely remove it or leave the warning in and
> just set it to be always defined.

IMHO, we should completely remove it - from both mozilla-central and comm-central.

Comment 40

[Ted Mielczarek [:ted.mielczarek]]

dbaron can review layout-debug. I'm not sure who would review spellchecker.

Comment 41

[Robert Kaiser]

spellchecker is currently pretty much un-owned, we are looking to find a solution but have none yet. It's probably best to look for people who did review or change code in that module somewhat recently (except mscott, who is pretty much unavailable).

Comment 43

[Steffen Wilberg]

Comment on attachment 335601 [details] [diff] [review]
extensions cleanup (spellchecker, layout-debug)
[Checkin: Comment 49]

Asking dbaron for layout-debug and smorgan for spellcheck.

Comment 44

[Stuart Morgan]

Comment on attachment 335601 [details] [diff] [review]
extensions cleanup (spellchecker, layout-debug)
[Checkin: Comment 49]

I have nothing to do with spellcheck.

Comment 45

[David Baron :dbaron: ⌚️UTC-8]

Comment on attachment 335601 [details] [diff] [review]
extensions cleanup (spellchecker, layout-debug)
[Checkin: Comment 49]

sr=dbaron

Comment 46

[Steffen Wilberg]

Comment on attachment 335601 [details] [diff] [review]
extensions cleanup (spellchecker, layout-debug)
[Checkin: Comment 49]

bienvenu, can you review a little dead code removal (MOZ_XUL_APP ifdefs) in spellcheck, please?

Comment 47

[David :Bienvenu]

Comment on attachment 335601 [details] [diff] [review]
extensions cleanup (spellchecker, layout-debug)
[Checkin: Comment 49]

r= me for spellcheck - layout debug is not something I'm remotely involved with, but you could switch dbaron's sr to an r= and sr=me for that - it looks fine.

Comment 48

[Arpad Borsos [:Swatinem]]

Adding checkin-needed with review flags according to comment #47.
extensions/(wallet|typeaheadfind) are still using the macro, so I can't completely remove the macro from configure yet :(

Comment 49

[Serge Gautherie (:sgautherie)]

Comment on attachment 335601 [details] [diff] [review]
extensions cleanup (spellchecker, layout-debug)
[Checkin: Comment 49]

http://hg.mozilla.org/mozilla-central/rev/f9f55538ed8c

Comment 50

[Robert Kaiser]

(In reply to comment #48)
> extensions/(wallet|typeaheadfind) are still using the macro, so I can't
> completely remove the macro from configure yet :(

Both should die/move in the next months (the former will, I hope we can do that for the latter as well in some way), I think we can leave the existence of the flag in for that atm.

Comment 51

[Arpad Borsos [:Swatinem]]

(In reply to comment #50)
> Both should die/move in the next months

Is the progress tracked in a bug? Can you make it block this one?

Comment 52

[Arpad Borsos [:Swatinem]]

Created attachment 344822 [details] [diff] [review]
configure cleanup, mozilla-central

Comment 53

[Arpad Borsos [:Swatinem]]

Created attachment 344823 [details] [diff] [review]
configure cleanup, comm-central

Ted: you are also responsible for the c-c build system, right?
The patches are not yet meant for checkin, as long as extensions/(wallet|typeaheadfind) aren't dealt with first. See comment #50.

Comment 54

[Ted Mielczarek [:ted.mielczarek]]

I don't own the c-c build system. You can ask Kairo or Standard8 for review.

Comment 55

[Ted Mielczarek [:ted.mielczarek]]

Comment on attachment 344822 [details] [diff] [review]
configure cleanup, mozilla-central

+MOZ_PROFILESHARING=

What's this change for? Looks fine otherwise.

Comment 56

[Justin Wood (:Callek)]

Comment on attachment 344823 [details] [diff] [review]
configure cleanup, comm-central

I am also a c-c build peer, I just focus on client.py :-)

c#55 intrigues me as well...

Also, we (c-c) can't remove MOZ_XUL_APP just yet, [see: http://mxr.mozilla.org/comm-central/search?find=%2Fmozilla%2Fextensions%2F&string=MOZ_XUL_APP] Both those dirs are currently pulled from CVS via client.py and a solution needs to be found for them first. (I'm not sure top of my head if camino uses them or not).

Comment 57

[Justin Wood (:Callek)]

actually, re: m-c and c-c final removals, I believe that removing MOZ_XUL_APP from m-c's autoconf.mk will break those, and c-c's autoconf.mk likely won't matter... KaiRo, have any ideas I'm overlooking?

Comment 58

[Arpad Borsos [:Swatinem]]

(In reply to comment #55)
> +MOZ_PROFILESHARING=
> 
> What's this change for? Looks fine otherwise.

Thats what the old code did. The code in question was added by bug 261232, so I'm CC-ing Benjamin who did that change. Maybe he can shed more light on that.

(In reply to comment #56)
> Also, we (c-c) can't remove MOZ_XUL_APP just yet

I'm well aware of that. Thats why I said that these patches are not yet meant for checkin before those extensions aren't cleaned up first.

Comment 59

[Justin Wood (:Callek)]

Comment on attachment 344823 [details] [diff] [review]
configure cleanup, comm-central

r+ re: c#58 then, thanks

Comment 60

[Robert Kaiser]

Just as a note, Justin is right, we probably can even remove the flag from c-c completely but we actually need to keep it in m-c for those modules in extensions/ directories as they are built with the m-c build system.

Comment 61

[Mark Banner (:standard8)]

Wallet is now "gone" - we're not building it any more (though currently we are still pulling it).

Comment 62

[Arpad Borsos [:Swatinem]]

(In reply to comment #61)
> Wallet is now "gone" - we're not building it any more (though currently we are
> still pulling it).

Good job, requesting checkin-needed for the last 2 patches then.

Comment 63

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Both patches need refreshing, they don't apply cleanly.

Comment 64

[Arpad Borsos [:Swatinem]]

Created attachment 357632 [details] [diff] [review]
configure cleanup, mozilla-central, refreshed
[Checkin: Comment 66]

Comment 65

[Arpad Borsos [:Swatinem]]

Created attachment 357633 [details] [diff] [review]
configure cleanup, comm-central, refreshed
[Checkin: Comment 67]

Both patches refreshed, carrying over r+

Comment 66

[Serge Gautherie (:sgautherie)]

Comment on attachment 357632 [details] [diff] [review]
configure cleanup, mozilla-central, refreshed
[Checkin: Comment 66]


http://hg.mozilla.org/mozilla-central/rev/cc99d1111d78

Comment 67

[Serge Gautherie (:sgautherie)]

Comment on attachment 357633 [details] [diff] [review]
configure cleanup, comm-central, refreshed
[Checkin: Comment 67]


http://hg.mozilla.org/comm-central/rev/31bb673606da

Comment 68

[Serge Gautherie (:sgautherie)]

(In reply to comment #67)

It seems one file remains to be processed:
http://mxr.mozilla.org/comm-central/search?string=MOZ_XUL_APP&case=on&find=%2Fsuite%2Finstaller%2Fwindows%2Fpackages

Comment 69

[Arpad Borsos [:Swatinem]]

Those are just comments saying when each of those packaged files was added, I left that comment in intentionally.
So yes, this is fixed now, thanks everyone for review & checkin :)