Closed Bug 1291366 Opened 4 years ago Closed 4 years ago

[geckoview] Split AppConstants, stop using ANDROID_PACKAGE_NAME in GeckoView

Categories

(GeckoView :: General, defect)

defect
Not set
normal

Tracking

(firefox53 fixed)

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: nalexander, Unassigned)

References

Details

Attachments

(6 files)

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
Blocks: 1322175
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?
Attachment #8816830 - Flags: review?(s.kaspari) → review+
Comment on attachment 8816831 [details]
Bug 1291366 - Pre: Don't check against {MIN,MAX}_SDK_VERSION.

https://reviewboard.mozilla.org/r/97356/#review97824
Attachment #8816831 - Flags: review?(s.kaspari) → review+
Comment on attachment 8816834 [details]
Bug 1291366 - Part 2: Add o.m.geckoview.BuildConfig.

https://reviewboard.mozilla.org/r/97362/#review97928
Attachment #8816834 - Flags: review?(s.kaspari) → review+
Comment on attachment 8816835 [details]
Bug 1291366 - Part 3: Generate o.m.geckoview.BuildConfig using Gradle.

https://reviewboard.mozilla.org/r/97364/#review97962
Attachment #8816835 - Flags: review?(s.kaspari) → review+
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.
Attachment #8816832 - Flags: review?(gps) → review+
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).
Attachment #8816833 - Flags: review?(gps) → review+
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
Blocks: 1415778
Product: Firefox for Android → GeckoView
Target Milestone: Firefox 53 → mozilla53
You need to log in before you can comment on or make changes to this bug.