1430413 - Unify Fennec-specific #ifndef usage in jar.mn files

Status: NEW

(Firefox Build System :: Android Studio and Gradle Integration, enhancement)

Description

[Jan Henning [:JanH]]

Some jar.mn files use MOZ_FENNEC to exclude certain files, but with b2g gone and only Fennec remaining as an Android build target, this only causes confusion why some #ifdefs/ifndefs use the one and others the other.

Comment 1

[[:fabrice] Fabrice Desré]

iirc, there are actually 3 preprocessor define in play here:
- ANDROID: basically means "we build native code on an android based platform". It's true for Android products, and was true for b2g too.
- MOZ_WIDGET_ANDROID: means that this is using the Android widget port. Was not true for b2g on Gonk (using MOZ_WIDGET_GONK) but was true for b2gdroid.
- MOZ_FENNEC: product specific define. True for fennec, was was for b2gdroid where MOZ_B2G was true.

So while MOZ_FENNEC implies MOZ_WIDGET_ANDROID and ANDROID beacuse Fennec doesn't run on any other platform (it used to run on Maemo), the semantics are different and worth keeping if Mozilla has any interest in making Gecko reusable.

Comment 2

[Jan Henning [:JanH]]

Hmm yes, you might have a point there. In that case I'd reverse this bug and at least unify the jar.mn files (which seems to be the only place that actually uses MOZ_FENNEC) to all use MOZ_FENNEC then, since those exclusions are now specific to Fennec and not automatically applicable to some hypothetical additional Android build target.

Comment 3

[Nick Alexander :nalexander [he/him]]

MOZ_FENNEC is a local define set in toolkit/, so I'm less annoyed by this inconsistency -- it's local, not global.  But that shows that the situation is _even worse than expected_ -- we're not using whatever the rest of the world uses to say "I'm mobile/android (Fennec)"!  Let's make this MOZ_WIDGET_ANDROID and get rid of the local include entirely.

Now, why are a bunch of toolkit/ things App-specific?  That's a layering violation fo' sho' -- but not necessarily one we're going to address.

Comment 4

[[:fabrice] Fabrice Desré]

(In reply to Nick Alexander :nalexander from comment #3)

> Now, why are a bunch of toolkit/ things App-specific?  That's a layering
> violation fo' sho' -- but not necessarily one we're going to address.

If Moz was serious about making Gecko a platform, this should be done though... If not, embrace the fact that gecko is just the backend for a couple of products and be done with this "toolkit" abstraction. The current messy situation is a win for no-one.

Comment 5

[Nick Alexander :nalexander [he/him]]

(In reply to [:fabrice] Fabrice Desré from comment #4)
> (In reply to Nick Alexander :nalexander from comment #3)
> 
> > Now, why are a bunch of toolkit/ things App-specific?  That's a layering
> > violation fo' sho' -- but not necessarily one we're going to address.
> 
> If Moz was serious about making Gecko a platform, this should be done
> though... If not, embrace the fact that gecko is just the backend for a
> couple of products and be done with this "toolkit" abstraction. The current
> messy situation is a win for no-one.

So, I just asked "why toolkit/" in a meeting I had :)  I have no idea what the future will hold, but I would also like to improve the messy situation.

Comment 6

[Jan Henning [:JanH]]

In the mean time, I've identified some more files in the omnijar that most probably (sometimes certainly) aren't actually required for Fennec and not all of them are in directories covered by the already existing MOZ_FENNEC defines. So where does that leave me now in the short term?

Comment 7

[Nick Alexander :nalexander [he/him]]

(In reply to Jan Henning [:JanH] from comment #6)
> In the mean time, I've identified some more files in the omnijar that most
> probably (sometimes certainly) aren't actually required for Fennec and not
> all of them are in directories covered by the already existing MOZ_FENNEC
> defines. So where does that leave me now in the short term?

I'll r+ patches to guard bits of the tree, and I think snorp and the GeckoView peers will too.  Let's guard with stanzas like

`CONFIG['MOZ_BUILD_APP'] != 'mobile/android'`

since we might as well be explicit that we're testing for Fennec (== mobile/android) and not, as Fabrice points out, some general notion of XUL applications that run on Android.

Comment 8

[Jan Henning [:JanH]]

(In reply to Nick Alexander :nalexander from comment #7)
> I'll r+ patches to guard bits of the tree, and I think snorp and the
> GeckoView peers will too.  Let's guard with stanzas like
> 
> `CONFIG['MOZ_BUILD_APP'] != 'mobile/android'`
> 
> since we might as well be explicit that we're testing for Fennec (==
> mobile/android) and not, as Fabrice points out, some general notion of XUL
> applications that run on Android.

Although that's not applicable to jar.mn files when you want to exclude only individual files, which means either pasting
> if CONFIG['MOZ_BUILD_APP'] == 'mobile/android':
>    DEFINES['MOZ_FENNEC'] = True
around even more moz.build files, or else globally defining some equivalent of this that's usable in preprocessed files as well.

Comment 9

[Nick Alexander :nalexander [he/him]]

(In reply to Jan Henning [:JanH] from comment #8)
> (In reply to Nick Alexander :nalexander from comment #7)
> > I'll r+ patches to guard bits of the tree, and I think snorp and the
> > GeckoView peers will too.  Let's guard with stanzas like
> > 
> > `CONFIG['MOZ_BUILD_APP'] != 'mobile/android'`
> > 
> > since we might as well be explicit that we're testing for Fennec (==
> > mobile/android) and not, as Fabrice points out, some general notion of XUL
> > applications that run on Android.
> 
> Although that's not applicable to jar.mn files when you want to exclude only
> individual files, which means either pasting
> > if CONFIG['MOZ_BUILD_APP'] == 'mobile/android':
> >    DEFINES['MOZ_FENNEC'] = True
> around even more moz.build files, or else globally defining some equivalent
> of this that's usable in preprocessed files as well.

We can guard by value in jar.mn files: for example, https://searchfox.org/mozilla-central/source/browser/extensions/pocket/locale/jar.mn#36.

Comment 10

[Jan Henning [:JanH]]

Ah, that was my missing clue - thanks!