Right now AppConstants includes build flags (definitely should be in GeckoView), feature flags (definitely should not be in GeckoView), and version checking helpers. The former should migrate to GeckoViewConstants (or, looking ahead to a Gradle world, to org.mozilla.geckoview.BuildConfig) and the latter can either migrate or stay as AppConstants. The version checking helpers should just be replaced with inline expressions: lint does a better job with inline expressions than our level of indirection.

Comment on attachment 8816833 [details] Bug 1291366 - Part 1: Use GENERATED_FILES to produce AppConstants.java. https://reviewboard.mozilla.org/r/97360/#review97658 ::: mobile/android/base/generate_build_config.py:40 (Diff revision 1) > + > + CONFIG = defaultdict(lambda: None) > + CONFIG.update(buildconfig.substs) > + DEFINES = {} > + > + for var in ('MOZ_ANDROID_ACTIVITY_STREAM' gps: I would have liked to have sent these in as command line parameters, but the generated files documentation is clear that all parameters must be filenames (see http://searchfox.org/mozilla-central/source/python/mozbuild/mozbuild/frontend/context.py#993). I bet these just end up getting mashed into a Makefile somewhere, so here we are.

Comment on attachment 8816834 [details] Bug 1291366 - Part 2: Add o.m.geckoview.BuildConfig. https://reviewboard.mozilla.org/r/97362/#review97660 sebastian: this patch doesn't actually remove AppConstants from GeckoView, since there are two outstanding GeckoView consumers: crash handling and event dispatcher logging and behaviour flags. I'll address those in separate tickets.

Here's a happy try build. The findbugs errors predate this patch, I think, but I will investigate more thoroughly. https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a97274385e0c2ad2976f5eac9b6a36b9bfe61a9

Comment on attachment 8816830 [details] Bug 1291366 - Pre: Inline AppConstants.Versions in GeckoView. https://reviewboard.mozilla.org/r/97354/#review97822 ::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoAccessibility.java:55 (Diff revision 1) > JSONObject ret = new JSONObject(); > sEnabled = false; > AccessibilityManager accessibilityManager = > (AccessibilityManager) context.getSystemService(Context.ACCESSIBILITY_SERVICE); > sEnabled = accessibilityManager.isEnabled() && accessibilityManager.isTouchExplorationEnabled(); > - if (Versions.feature16Plus && sEnabled && sSelfBrailleClient == null) { > + if (Build.VERSION.SDK_INT >= 1 && sEnabled && sSelfBrailleClient == null) { Seems like there's a 6 missing here? ::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoAccessibility.java:239 (Diff revision 1) > sSelfBrailleClient.write(data); > } > > public static void setDelegate(View view) { > // Only use this delegate in Jelly Bean. > - if (Versions.feature16Plus) { > + if (Build.VERSION.SDK_INT >= 1) { ... and here?

Comment on attachment 8816831 [details] Bug 1291366 - Pre: Don't check against {MIN,MAX}_SDK_VERSION. https://reviewboard.mozilla.org/r/97356/#review97824

Comment on attachment 8816834 [details] Bug 1291366 - Part 2: Add o.m.geckoview.BuildConfig. https://reviewboard.mozilla.org/r/97362/#review97928

Comment on attachment 8816835 [details] Bug 1291366 - Part 3: Generate o.m.geckoview.BuildConfig using Gradle. https://reviewboard.mozilla.org/r/97364/#review97962

Comment on attachment 8816832 [details] Bug 1291366 - Pre: don't force generated/ in Java generated_sources. https://reviewboard.mozilla.org/r/97358/#review98612 Seems quite reasonable.

Comment on attachment 8816833 [details] Bug 1291366 - Part 1: Use GENERATED_FILES to produce AppConstants.java. https://reviewboard.mozilla.org/r/97360/#review97658 > gps: I would have liked to have sent these in as command line parameters, but the generated files documentation is clear that all parameters must be filenames (see http://searchfox.org/mozilla-central/source/python/mozbuild/mozbuild/frontend/context.py#993). I bet these just end up getting mashed into a Makefile somewhere, so here we are. Ugh. This script seems to have a lot of overlap with the existing preprocessor. Before I r+ this, is there not a way to avoid code reuse? Specifically, I'm a bit worried about the constant list getting out of sync. The fact we have to add so many constants seems to indicate a lack of defines or substs being set in configure.

Comment on attachment 8816833 [details] Bug 1291366 - Part 1: Use GENERATED_FILES to produce AppConstants.java. https://reviewboard.mozilla.org/r/97360/#review97658 > Ugh. > > This script seems to have a lot of overlap with the existing preprocessor. Before I r+ this, is there not a way to avoid code reuse? Specifically, I'm a bit worried about the constant list getting out of sync. The fact we have to add so many constants seems to indicate a lack of defines or substs being set in configure. So! Here we have two build systems intersecting. Let me provide some context on why I want the duplication (for now). The long term goal is for this to disappear entirely, and for us to *only* use the mechanism (implemented later in the patch series, reviewed by sebastian) provided by Gradle. A concurrent desire is to be able to build Fennec independently of the rest of the tree; that is, `cp -R mobile/android fennec-out-of-tree`, tweak some things, and not require the Mozilla build system at all. In follow-up, I intend to replace this preprocessor based approach with a single input data file that both this moz.build script and the Gradle build script consume to produce a bit-identical file, easing the transition. This just lets me get closer to doing that. Looking at the added constants, there are only 6, and 2 of them (MOZ_BUILDID and the Adjust key) should be handled specially. Two of the others are just to give values to the crash reporter during artifact builds, and the remaining two are leftovers of quoting differences between Autoconf, shell, and Java. Arguably the middle two make no sense, but we still need to give some values for the Java code; and the latter two I think I can remove eventually. There's a follow-up patch to this that I'll push as part of https://bugzilla.mozilla.org/show_bug.cgi?id=1317089 that will do a similar conversion from the preprocessor to a custom invocation, and I will try to ensure I don't duplicate this code again. In the meantime, perfect is the enemy of the good.

Comment on attachment 8816830 [details] Bug 1291366 - Pre: Inline AppConstants.Versions in GeckoView. https://reviewboard.mozilla.org/r/97354/#review97822 > Seems like there's a 6 missing here? Fixed these two, will push shortly. Thanks for catching these! Eventually, we'll get linting working in GeckoView too and that should catch these "for real".

Comment on attachment 8816833 [details] Bug 1291366 - Part 1: Use GENERATED_FILES to produce AppConstants.java. https://reviewboard.mozilla.org/r/97360/#review99200 ::: mobile/android/base/generate_build_config.py:1 (Diff revision 1) > +#!/bin/python Should be `#!/usr/bin/env python` (although it doesn't matter when invoked by the build system since it will always use the virtualenv python).

Comment on attachment 8816833 [details] Bug 1291366 - Part 1: Use GENERATED_FILES to produce AppConstants.java. https://reviewboard.mozilla.org/r/97360/#review97658 > So! Here we have two build systems intersecting. Let me provide some context on why I want the duplication (for now). The long term goal is for this to disappear entirely, and for us to *only* use the mechanism (implemented later in the patch series, reviewed by sebastian) provided by Gradle. A concurrent desire is to be able to build Fennec independently of the rest of the tree; that is, `cp -R mobile/android fennec-out-of-tree`, tweak some things, and not require the Mozilla build system at all. In follow-up, I intend to replace this preprocessor based approach with a single input data file that both this moz.build script and the Gradle build script consume to produce a bit-identical file, easing the transition. This just lets me get closer to doing that. > > Looking at the added constants, there are only 6, and 2 of them (MOZ_BUILDID and the Adjust key) should be handled specially. Two of the others are just to give values to the crash reporter during artifact builds, and the remaining two are leftovers of quoting differences between Autoconf, shell, and Java. Arguably the middle two make no sense, but we still need to give some values for the Java code; and the latter two I think I can remove eventually. > > There's a follow-up patch to this that I'll push as part of https://bugzilla.mozilla.org/show_bug.cgi?id=1317089 that will do a similar conversion from the preprocessor to a custom invocation, and I will try to ensure I don't duplicate this code again. In the meantime, perfect is the enemy of the good. gps and I discussed in the ticket, and there's follow-up to unify much of this and prevent duplication. Onwards!

Pushed by nalexander@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b98927d75021 Pre: Inline AppConstants.Versions in GeckoView. r=sebastian https://hg.mozilla.org/integration/autoland/rev/bc2b540eb1c0 Pre: Don't check against {MIN,MAX}_SDK_VERSION. r=sebastian https://hg.mozilla.org/integration/autoland/rev/fe00dc8bf42b Pre: don't force generated/ in Java generated_sources. r=gps https://hg.mozilla.org/integration/autoland/rev/420ee7d10265 Part 1: Use GENERATED_FILES to produce AppConstants.java. r=gps https://hg.mozilla.org/integration/autoland/rev/a0efb661275b Part 2: Add o.m.geckoview.BuildConfig. r=sebastian https://hg.mozilla.org/integration/autoland/rev/b25f84e78371 Part 3: Generate o.m.geckoview.BuildConfig using Gradle. r=sebastian

https://hg.mozilla.org/mozilla-central/rev/b98927d75021 https://hg.mozilla.org/mozilla-central/rev/bc2b540eb1c0 https://hg.mozilla.org/mozilla-central/rev/fe00dc8bf42b https://hg.mozilla.org/mozilla-central/rev/420ee7d10265 https://hg.mozilla.org/mozilla-central/rev/a0efb661275b https://hg.mozilla.org/mozilla-central/rev/b25f84e78371