Closed Bug 797613 Opened 7 years ago Closed 7 years ago

identify B2G and Fennec via configure variable

Categories

(Firefox OS Graveyard :: GonkIntegration, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: myk, Assigned: myk)

Details

Attachments

(1 file, 2 obsolete files)

Attached patch patch that demonstrates idea (obsolete) — Splinter Review
It's hard to identify B2G and Fennec in preprocessor directives, because there's no single configuration variable that identifies either one.

Fennec always defines ANDROID, which B2G also defines sometimes.  And when B2G defines ANDROID, it also defines MOZ_WIDGET_GONK.  So it's possible to determine Fennec builds via (ANDROID && !MOZ_WIDGET_GONK).  And it's possible to determine some B2G builds via (ANDROID && MOZ_WIDGET_GONK).  But it isn't possible to determine all B2G builds that way.

Thus we end up with convoluted and ambiguous code like the following <http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.js#273>:

      #ifdef MOZ_PHOENIX
      # Firefox Desktop: installPackage not implemented
      #elifdef ANDROID
      #ifndef MOZ_WIDGET_GONK
      # Firefox Android (Fennec): installPackage not implemented
      #else
      # B2G Gonk: installPackage implemented
                                               Ci.mozIDOMApplicationRegistry2,
      #endif
      #else
      # B2G Desktop and others: installPackage implementation status varies
                                               Ci.mozIDOMApplicationRegistry2,
      #endif

If there were MOZ_B2G and MOZ_FENNEC vars for the B2G and Fennec projects, just like there's MOZ_PHOENIX for desktop Firefox, then that code could be as simple as:

    #ifdef MOZ_B2G
                                             Ci.mozIDOMApplicationRegistry2,
    #endif

And once Fennec adds support for packaged apps, it would become:

    #ifdef MOZ_B2G
                                             Ci.mozIDOMApplicationRegistry2,
    #elifdef MOZ_FENNEC
                                             Ci.mozIDOMApplicationRegistry2,
    #endif

Here's a prospective (not yet tested) patch that demonstrates the idea.

Fabrice, Mark: what do you think about doing something like this?
Attachment #667713 - Flags: feedback?(mark.finkle)
Attachment #667713 - Flags: feedback?(fabrice)
I support the idea!
Comment on attachment 667713 [details] [diff] [review]
patch that demonstrates idea

Sounds OK to me
Attachment #667713 - Flags: feedback?(mark.finkle) → feedback+
Assignee: nobody → myk
Status: NEW → ASSIGNED
Note that we now have MOZ_B2G defined for all b2g builds since bug 795715 landed.
MOZ_B2G by itself makes at least the preprocessor conditionals in Webapps.js a lot better.  I could swear I saw places where MOZ_FENNEC would simplify things too, but I don't see them now.  Nevertheless, as soon as we land support for packaged apps, we'll need MOZ_FENNEC in Webapps.js.  So this patch adds it while using MOZ_B2G to simplify the logic in Webapps.js.
Attachment #667713 - Attachment is obsolete: true
Attachment #667713 - Flags: feedback?(fabrice)
Attachment #682237 - Flags: review?(fabrice)
Comment on attachment 682237 [details] [diff] [review]
add MOZ_FENNEC, simplify Webapps.js

Jonas, perhaps you have cycles to review this?
Attachment #682237 - Flags: review?(fabrice) → review?(jonas)
Attachment #682237 - Attachment is obsolete: true
Attachment #682237 - Flags: review?(jonas)
Attachment #703658 - Flags: review?(jonas)
Comment on attachment 703658 [details] [diff] [review]
patch v3: rebase to tip

Review of attachment 703658 [details] [diff] [review]:
-----------------------------------------------------------------

I don't think I'm the right person to review this. In particular the confvars.sh change should be reviewed by someone that knows build issues better than me.
Attachment #703658 - Flags: review?(jonas) → review?(fabrice)
Comment on attachment 703658 [details] [diff] [review]
patch v3: rebase to tip

Review of attachment 703658 [details] [diff] [review]:
-----------------------------------------------------------------

r=me for Webapps.js only.

::: mobile/android/confvars.sh
@@ +47,5 @@
>  MOZ_APP_STATIC_INI=1
>  
>  MOZ_PER_WINDOW_PRIVATE_BROWSING=1
> +
> +MOZ_FENNEC=1

For b2g, we also have code in configure.in :
https://mxr.mozilla.org/mozilla-central/source/configure.in#4397

We need a real build peer to review that!
Attachment #703658 - Flags: review?(khuey)
Attachment #703658 - Flags: review?(fabrice)
Attachment #703658 - Flags: review+
Comment on attachment 703658 [details] [diff] [review]
patch v3: rebase to tip

Review of attachment 703658 [details] [diff] [review]:
-----------------------------------------------------------------

I don't understand what MOZ_FENNEC is supposed to do.
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #9)
> I don't understand what MOZ_FENNEC is supposed to do.

In the near future, some features of apps that are implemented for B2G will also get implemented for Fennec, like support for packaged apps.  But those features will remain unimplemented in Firefox for desktop, so we'll need to selectively enable them.

To date we've done that via complex, imprecise, and brittle preprocessor directive sequences like this one in Webapps.js <https://github.com/mozilla/mozilla-central/blob/c41e592ebb5214a3f64306e9bdaecc918f212b12/dom/apps/src/Webapps.js#L266-L278>.  Merely defining MOZ_B2G, as we did a few months ago, makes it possible to improve them, and this patch does that.

But once we implement those features for Fennec, we'll need to add directives to selectively enable those features for that project as well.

We could check ifdef ANDROID, which both B2G and Fennec define, but we'd still have to check ifdef MOZ_B2G, since B2G for desktop doesn't define ANDROID.  And the features would then get auto-enabled for any new project targeting Android, whether or not it supports them.

We could instead check if MOZ_APP_BASENAME==Fennec, which the text preprocessor supports, according to its docs <https://developer.mozilla.org/en-US/docs/Build/Text_Preprocessor>.  But I don't see any examples of that in the codebase, and b2g/, browser/ and xulrunner/ all define equivalents (MOZ_B2G, MOZ_PHOENIX, and MOZ_XULRUNNER, respectively):

http://mxr.mozilla.org/mozilla-central/source/b2g/confvars.sh
http://mxr.mozilla.org/mozilla-central/source/browser/confvars.sh
http://mxr.mozilla.org/mozilla-central/source/xulrunner/confvars.sh

Thus defining MOZ_FENNEC seems like the best practice (or at least the convention) in addition to being the simplest, most precise, and least brittle way to selectively enable features for Fennec.
Comment on attachment 703658 [details] [diff] [review]
patch v3: rebase to tip

Review of attachment 703658 [details] [diff] [review]:
-----------------------------------------------------------------

Ok, so the MOZ_FENNEC=1 is not intended to have any effect in Webapps.js today.  Got it.
Attachment #703658 - Flags: review?(khuey) → review+
Pushing to tryserver first just in case:

https://tbpl.mozilla.org/?tree=Try&rev=d609be99ceed
https://hg.mozilla.org/mozilla-central/rev/c29fe4384d8f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.